Bernhard Voelker <[email protected]> writes:
> On 3/31/26 05:08, Collin Funk wrote:
>> I attached a v2 patch [...]
>> Subject: [PATCH v2] cat: use splice if operating on non-files or if
>> copy_file_range fails
>>
>> On a AMD Ryzen 7 3700X system:
>>
>> $ timeout 10 taskset 1 ./src/cat-prev /dev/zero \
>> | taskset 2 pv -r > /dev/null
>> [1.84GiB/s]
>> $ timeout 10 taskset 1 ./src/cat /dev/zero \
>> | taskset 2 pv -r > /dev/null
>> [7.92GiB/s]
>>
>> On a Power10 system:
>>
>> $ taskset 1 ./src/yes | timeout 10 taskset 2 ./src/cat-prev \
>> | taskset 3 pv -r > /dev/null
>> [12.6GiB/s]
>> $ taskset 1 ./src/yes | timeout 10 taskset 2 ./src/cat \
>> | taskset 3 pv -r > /dev/null
>> [61.3GiB/s]
>
> The Power10 example references src/yes instead of src/cat.
Well, using './src/cat /dev/zero' uses read/write there since slice
fails with EINVAL. Using 'yes' piped into 'cat' seemed reasonable
enough, and the performance improvement seemed in the same ballpark as
it was on my machine.
>> diff --git a/NEWS b/NEWS
>> index 2fabd07b7..2411e64d2 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -39,6 +39,10 @@ GNU coreutils NEWS -*-
>> outline -*-
>>
>> ** Improvements
>>
>> + 'cat' now uses zero-copy I/O on Linux when the input or output are not
>> regular
>> + files to significantly increase throughput.
>> + E.g., throughput improved 5x from 12.6GiB/s to 61.3GiB/s on a Power10
>> system.
>> +
>
> "are not regular files" ... that sounds a bit vague.
> AFAIU splice is used when at least one of either input or output is a pipe,
> isn't it?
Yeah, that wording sounds better to me.
I wrote it since we use copy_file_range if the following expression is
true:
out_isreg && S_ISREG (istat_buf.st_mode)
Otherwise, we try using splice. So I was kind of just translating the
code to English.
>> diff --git a/src/cat.c b/src/cat.c
>> index f9c92005c..11a2df450 100644
>> --- a/src/cat.c
>> +++ b/src/cat.c
>
> [...]
>
>> @@ -545,6 +548,109 @@ copy_cat (void)
>> }
>> }
>>
>> +/* Copy data from input to output using splice if possible.
>> + Return 1 if successful, 0 if ordinary read+write should be tried,
>> + -1 if a serious problem has been diagnosed. */
>> +
>> +static int
>> +splice_cat (void)
>> +{
>> + bool some_copied = false;
>
> [...]
>
>> + return ok ? some_copied : -1;
>> +}
>
> This returns a bool as int.
> I'd suggest sticking clearly to 'int' for clarity.
I thought about doing this as well. But I figured I should stick to how
I wrote it since it matches the current copy_cat function which looks
something like this:
for (bool some_copied = false; ; some_copied = true)
if (copy_file_range (...))
{
case 0:
return some_copied;
case -1:
return -1;
}
Collin
