| Message ID | 80736a2cc6d478c383ea565ba825eaf4d1abd876.1687523671.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2fe11c9d36ee2f3dc3c642588c5d9a22190674c9 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net/tcp: optimise locking for blocking splice | expand |
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) ||
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) ||
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!
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);
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(-)