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

[Eric Dumazet <edumazet@google.com>]

On Mon, Jun 19, 2023 at 11:27 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> 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@gmail.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.
>

We prefer seeing patches focusing on the change, instead of also doing
arbitrary changes
making future backports more likely to conflict.

Thanks.