netdev - Re: [PATCH net-next 1/2] net/tcp: optimise locking for blocking splice

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e972fc86-b884-3600-4e16-c9dbb53c6464@gmail.com>
Date: Mon, 19 Jun 2023 10:27:38 +0100
From: Pavel Begunkov <asml.silence@...il.com>
To: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 edumazet@...gle.com, davem@...emloft.net, dsahern@...nel.org, kuba@...nel.org
Subject: Re: [PATCH net-next 1/2] net/tcp: optimise locking for blocking
 splice

On 5/24/23 13:51, Pavel Begunkov wrote:
> On 5/23/23 14:52, Paolo Abeni wrote:
>> On Fri, 2023-05-19 at 14:33 +0100, Pavel Begunkov wrote:
>>> Even when tcp_splice_read() reads all it was asked for, for blocking
>>> sockets it'll release and immediately regrab the socket lock, loop
>>> around and break on the while check.
>>>
>>> Check tss.len right after we adjust it, and return if we're done.
>>> That saves us one release_sock(); lock_sock(); pair per successful
>>> blocking splice read.
>>>
>>> Signed-off-by: Pavel Begunkov <asml.silence@...il.com>
>>> ---
>>>   net/ipv4/tcp.c | 8 +++++---
>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>>> index 4d6392c16b7a..bf7627f37e69 100644
>>> --- a/net/ipv4/tcp.c
>>> +++ b/net/ipv4/tcp.c
>>> @@ -789,13 +789,15 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
>>>        */
>>>       if (unlikely(*ppos))
>>>           return -ESPIPE;
>>> +    if (unlikely(!tss.len))
>>> +        return 0;
>>>       ret = spliced = 0;
>>>       lock_sock(sk);
>>>       timeo = sock_rcvtimeo(sk, sock->file->f_flags & O_NONBLOCK);
>>> -    while (tss.len) {
>>> +    while (true) {
>>>           ret = __tcp_splice_read(sk, &tss);
>>>           if (ret < 0)
>>>               break;
>>> @@ -835,10 +837,10 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
>>>               }
>>>               continue;
>>>           }
>>> -        tss.len -= ret;
>>>           spliced += ret;
>>> +        tss.len -= ret;
>>
>> The patch LGTM. The only minor thing that I note is that the above
>> chunk is not needed. Perhaps avoiding unneeded delta could be worthy.
> 
> It keeps it closer to the tss.len test, so I'd leave it for that reason,
> but on the other hand the compiler should be perfectly able to optimise it
> regardless (i.e. sub;cmp;jcc; vs sub;jcc;). I don't have a hard feeling
> on that, can change if you want.

Is there anything I can do to help here? I think the patch is
fine, but can amend the change per Paolo's suggestion if required.

-- 
Pavel Begunkov