TCP splice improvements

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 <[email protected]>

---
 net/ipv4/tcp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

net/ipv4/tcp.c +3 -1

index 4d6392c16b7a..bf7627f37e69 100644
@@ -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;

Quoted 28 lines · 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 <[email protected]>

---
  net/ipv4/tcp.c | 8 +++++---
  1 file changed, 5 insertions(+), 3 deletions(-)

net/ipv4/tcp.c +2 -0

index 4d6392c16b7a..bf7627f37e69 100644
@@ -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);
 

net/ipv4/tcp.c ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, · line 841

Quoted 13 lines

net/ipv4/tcp.c +2 -2

  	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.

Quoted 34 lines · Paolo Abeni

Quoted 33 lines · 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 <[email protected]>

---
  net/ipv4/tcp.c | 8 +++++---

  1 file changed, 5 insertions(+), 3 deletions(-)

net/ipv4/tcp.c +3 -1

index 4d6392c16b7a..bf7627f37e69 100644
@@ -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,
               }

Quoted 8 lines

net/ipv4/tcp.c ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, · line 841

Quoted 5 lines

net/ipv4/tcp.c +1 -1

               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.

Quoted 44 lines · Pavel Begunkov

Quoted 42 lines · Paolo Abeni

Quoted 32 lines · 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 <[email protected]>

---
  net/ipv4/tcp.c | 8 +++++---
  1 file changed, 5 insertions(+), 3 deletions(-)

net/ipv4/tcp.c +3 -1

index 4d6392c16b7a..bf7627f37e69 100644
@@ -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;

net/ipv4/tcp.c ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, · line 841

Quoted 6 lines

net/ipv4/tcp.c +1 -1

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

Quoted 4 lines

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.

Quoted 51 lines · Pavel Begunkov

Quoted 48 lines · Pavel Begunkov

Quoted 34 lines · Paolo Abeni

Quoted 33 lines · 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 <[email protected]>

---
   net/ipv4/tcp.c | 8 +++++---
   1 file changed, 5 insertions(+), 3 deletions(-)

net/ipv4/tcp.c +3 -1

index 4d6392c16b7a..bf7627f37e69 100644
@@ -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,
               }

Quoted 8 lines

net/ipv4/tcp.c ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, · line 841

Quoted 5 lines

net/ipv4/tcp.c +1 -1

               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.

Quoted 3 lines

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.