[PATCH net-next v2] net/tcp: optimise locking for blocking splice

[PATCH net-next v2] net/tcp: optimise locking for blocking splice

[Pavel Begunkov]

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>
---

v2: go with Paolo's suggestion
    aggressively shrink the patch

 net/ipv4/tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 71b42eef9dbf..d56edc2c885f 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -839,7 +839,7 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
 		tss.len -= ret;
 		spliced += ret;
 
-		if (!timeo)
+		if (!tss.len || !timeo)
 			break;
 		release_sock(sk);
 		lock_sock(sk);
-- 
2.40.0

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

[Eric Dumazet]

On Fri, Jun 23, 2023 at 2:40 PM Pavel Begunkov <asml.silence@gmail.com> 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>
> ---
>
> v2: go with Paolo's suggestion
>     aggressively shrink the patch
>
>  net/ipv4/tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 71b42eef9dbf..d56edc2c885f 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -839,7 +839,7 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
>                 tss.len -= ret;
>                 spliced += ret;
>
> -               if (!timeo)
> +               if (!tss.len || !timeo)
>                         break;
>                 release_sock(sk);
>                 lock_sock(sk);

SGTM, thanks.

Reviewed-by: Eric Dumazet <edumazet@google.com>

I wonder if the "release_sock();sock_lock();"  could be replaced by
sk_flush_backlog() anyway ?
Or is there any other reason for this dance ?

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 71b42eef9dbf527098963bc03deecf55042e2021..d03d38060944d63d2728a7bf90a5c117b7852d8b
100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -841,8 +841,7 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,

                if (!timeo)
                        break;
-               release_sock(sk);
-               lock_sock(sk);
+               sk_flush_backlog();

                if (sk->sk_err || sk->sk_state == TCP_CLOSE ||
                    (sk->sk_shutdown & RCV_SHUTDOWN) ||

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

[Pavel Begunkov]

On 6/23/23 15:17, Eric Dumazet wrote:
> On Fri, Jun 23, 2023 at 2:40 PM Pavel Begunkov <asml.silence@gmail.com> 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>
>> ---
>>
>> v2: go with Paolo's suggestion
>>      aggressively shrink the patch
>>
>>   net/ipv4/tcp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>> index 71b42eef9dbf..d56edc2c885f 100644
>> --- a/net/ipv4/tcp.c
>> +++ b/net/ipv4/tcp.c
>> @@ -839,7 +839,7 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
>>                  tss.len -= ret;
>>                  spliced += ret;
>>
>> -               if (!timeo)
>> +               if (!tss.len || !timeo)
>>                          break;
>>                  release_sock(sk);
>>                  lock_sock(sk);
> 
> SGTM, thanks.
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> 
> I wonder if the "release_sock();sock_lock();"  could be replaced by
> sk_flush_backlog() anyway ?
> Or is there any other reason for this dance ?

Now as you mentioned, it definitely sounds like that. And the code
is 15 years old, perhaps nobody was paying attention.


> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 71b42eef9dbf527098963bc03deecf55042e2021..d03d38060944d63d2728a7bf90a5c117b7852d8b
> 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -841,8 +841,7 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
> 
>                  if (!timeo)
>                          break;
> -               release_sock(sk);
> -               lock_sock(sk);
> +               sk_flush_backlog();
> 
>                  if (sk->sk_err || sk->sk_state == TCP_CLOSE ||
>                      (sk->sk_shutdown & RCV_SHUTDOWN) ||

-- 
Pavel Begunkov

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 23 Jun 2023 13:38:55 +0100 you 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.
> 
> [...]

Here is the summary with links:
  - [net-next,v2] net/tcp: optimise locking for blocking splice
    https://git.kernel.org/netdev/net-next/c/2fe11c9d36ee

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html