Patchwork tcp: splice: fix an infinite loop in tcp_read_sock()

login
register
mail settings
Submitter Eric Dumazet
Date Jan. 10, 2013, 5:06 p.m.
Message ID <1357837570.27446.2285.camel@edumazet-glaptop>
Download mbox | patch
Permalink /patch/211097/
State Accepted
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Jan. 10, 2013, 5:06 p.m.
From: Eric Dumazet <edumazet@google.com>

commit 02275a2ee7c0 (tcp: don't abort splice() after small transfers)
added a regression.


[   83.843570] INFO: rcu_sched self-detected stall on CPU
[   83.844575] INFO: rcu_sched detected stalls on CPUs/tasks: { 6} (detected by 0, t=21002 jiffies, g=4457, c=4456, q=13132)
[   83.844582] Task dump for CPU 6:
[   83.844584] netperf         R  running task        0  8966   8952 0x0000000c
[   83.844587]  0000000000000000 0000000000000006 0000000000006c6c 0000000000000000
[   83.844589]  000000000000006c 0000000000000096 ffffffff819ce2bc ffffffffffffff10
[   83.844592]  ffffffff81088679 0000000000000010 0000000000000246 ffff880c4b9ddcd8
[   83.844594] Call Trace:
[   83.844596]  [<ffffffff81088679>] ? vprintk_emit+0x1c9/0x4c0
[   83.844601]  [<ffffffff815ad449>] ? schedule+0x29/0x70
[   83.844606]  [<ffffffff81537bd2>] ? tcp_splice_data_recv+0x42/0x50
[   83.844610]  [<ffffffff8153beaa>] ? tcp_read_sock+0xda/0x260
[   83.844613]  [<ffffffff81537b90>] ? tcp_prequeue_process+0xb0/0xb0
[   83.844615]  [<ffffffff8153c0f0>] ? tcp_splice_read+0xc0/0x250
[   83.844618]  [<ffffffff814dc0c2>] ? sock_splice_read+0x22/0x30
[   83.844622]  [<ffffffff811b820b>] ? do_splice_to+0x7b/0xa0
[   83.844627]  [<ffffffff811ba4bc>] ? sys_splice+0x59c/0x5d0
[   83.844630]  [<ffffffff8119745b>] ? putname+0x2b/0x40
[   83.844633]  [<ffffffff8118bcb4>] ? do_sys_open+0x174/0x1e0
[   83.844636]  [<ffffffff815b6202>] ? system_call_fastpath+0x16/0x1b


if recv_actor() returns 0, we should stop immediately,
because looping wont give a chance to drain the pipe.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Willy Tarreau <w@1wt.eu>
---
 net/ipv4/tcp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 10, 2013, 10:37 p.m.
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 10 Jan 2013 09:06:10 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> commit 02275a2ee7c0 (tcp: don't abort splice() after small transfers)
> added a regression.
 ...
> if recv_actor() returns 0, we should stop immediately,
> because looping wont give a chance to drain the pipe.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
willy tarreau - Jan. 10, 2013, 11:01 p.m.
Hi Eric,

On Thu, Jan 10, 2013 at 09:06:10AM -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> commit 02275a2ee7c0 (tcp: don't abort splice() after small transfers)
> added a regression.
> 
> 
> [   83.843570] INFO: rcu_sched self-detected stall on CPU
> [   83.844575] INFO: rcu_sched detected stalls on CPUs/tasks: { 6} (detected by 0, t=21002 jiffies, g=4457, c=4456, q=13132)
> [   83.844582] Task dump for CPU 6:
> [   83.844584] netperf         R  running task        0  8966   8952 0x0000000c
> [   83.844587]  0000000000000000 0000000000000006 0000000000006c6c 0000000000000000
> [   83.844589]  000000000000006c 0000000000000096 ffffffff819ce2bc ffffffffffffff10
> [   83.844592]  ffffffff81088679 0000000000000010 0000000000000246 ffff880c4b9ddcd8
> [   83.844594] Call Trace:
> [   83.844596]  [<ffffffff81088679>] ? vprintk_emit+0x1c9/0x4c0
> [   83.844601]  [<ffffffff815ad449>] ? schedule+0x29/0x70
> [   83.844606]  [<ffffffff81537bd2>] ? tcp_splice_data_recv+0x42/0x50
> [   83.844610]  [<ffffffff8153beaa>] ? tcp_read_sock+0xda/0x260
> [   83.844613]  [<ffffffff81537b90>] ? tcp_prequeue_process+0xb0/0xb0
> [   83.844615]  [<ffffffff8153c0f0>] ? tcp_splice_read+0xc0/0x250
> [   83.844618]  [<ffffffff814dc0c2>] ? sock_splice_read+0x22/0x30
> [   83.844622]  [<ffffffff811b820b>] ? do_splice_to+0x7b/0xa0
> [   83.844627]  [<ffffffff811ba4bc>] ? sys_splice+0x59c/0x5d0
> [   83.844630]  [<ffffffff8119745b>] ? putname+0x2b/0x40
> [   83.844633]  [<ffffffff8118bcb4>] ? do_sys_open+0x174/0x1e0
> [   83.844636]  [<ffffffff815b6202>] ? system_call_fastpath+0x16/0x1b
> 
> 
> if recv_actor() returns 0, we should stop immediately,
> because looping wont give a chance to drain the pipe.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Willy Tarreau <w@1wt.eu>
> ---
>  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 1ca2536..5f173dc 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1482,7 +1482,7 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
>  					break;
>  			}
>  			used = recv_actor(desc, skb, offset, len);
> -			if (used < 0) {
> +			if (used <= 0) {
>  				if (!copied)
>  					copied = used;
>  				break;

Thanks for catching this one. I'm amazed we didn't notice it earlier,
I've been running my stress-test kernels with this patch applied since
we did it.

Willy

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Jan. 10, 2013, 11:05 p.m.
On Fri, 2013-01-11 at 00:01 +0100, Willy Tarreau wrote:
> Hi Eric,

> Thanks for catching this one. I'm amazed we didn't notice it earlier,
> I've been running my stress-test kernels with this patch applied since
> we did it.
> 

Thats because you splice(    very_large_amount_of_bytes), so you dont
hit this bug.

netperf does the splice (    exact_amount_of_bytes ) so hits this pretty
fast on loopback at least.

Thanks !


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
willy tarreau - Jan. 10, 2013, 11:21 p.m.
On Thu, Jan 10, 2013 at 03:05:55PM -0800, Eric Dumazet wrote:
> On Fri, 2013-01-11 at 00:01 +0100, Willy Tarreau wrote:
> > Hi Eric,
> 
> > Thanks for catching this one. I'm amazed we didn't notice it earlier,
> > I've been running my stress-test kernels with this patch applied since
> > we did it.
> > 
> 
> Thats because you splice(    very_large_amount_of_bytes), so you dont
> hit this bug.

Not always, I use many sizes (from 1k to very large).

> netperf does the splice (    exact_amount_of_bytes ) so hits this pretty
> fast on loopback at least.

OK I see, if we need an exact size to trigger it, that explains it !

Thanks for the explanation,
Willy

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rick Jones - Jan. 10, 2013, 11:48 p.m.
On 01/10/2013 03:21 PM, Willy Tarreau wrote:
> On Thu, Jan 10, 2013 at 03:05:55PM -0800, Eric Dumazet wrote:
>> Thats because you splice(    very_large_amount_of_bytes), so you dont
>> hit this bug.
>
> Not always, I use many sizes (from 1k to very large).
>
>> netperf does the splice (    exact_amount_of_bytes ) so hits this pretty
>> fast on loopback at least.
>
> OK I see, if we need an exact size to trigger it, that explains it !

Netperf does not use a specific size all the time - the size it uses on 
the receive will be the "receive_size" calculated the same way it has 
been since the beginning - either a size specified by a test-specific -M 
option, or based on the value of SO_RCVBUF at the time the socket was 
created.

The kernel of the code making the splice calls - recv_data_no_copy() in 
src/nettest_omni.c looks like:

recv_data_no_copy(SOCKET data_socket, struct ring_elt *recv_ring, 
uint32_t bytes_to_recv, struct sockaddr *source, netperf_socklen_t 
*sourcelen, uint32_t flags, uint32_t *num_receives) {

...

   do {

     bytes_recvd = splice(data_socket,
			 NULL,
			 pfd[1],
			 NULL,
			 bytes_left,
			 my_flags);


     if (bytes_recvd > 0) {
       /* per Eric Dumazet, we should just let this second splice call
	 move as many bytes as it can and not worry about how much.
	 this should make the call more robust when made on a system
	 under memory pressure */
       splice(pfd[0], NULL, fdnull, NULL, 1 << 30, my_flags);
       bytes_left -= bytes_recvd;
     }
     else {
       break;
     }
     my_recvs++; /* should the pair of splices count as one? */
   } while ((bytes_left > 0) && (flags & NETPERF_WAITALL));

where NETPERF_WAITALL is only set for an _RR test.  Bytes_left is 
initialized to bytes_to_recv which is the "receive_size." my_flags is 
set to 0x03.

Now, if there are no test-specific -M option (or -s or -S depending on 
the test) netperf will, from run to run use the same receive_size - 
under Linux chances are quite good that will be 87380.

happy benchmarking,

rick jones
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1ca2536..5f173dc 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1482,7 +1482,7 @@  int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 					break;
 			}
 			used = recv_actor(desc, skb, offset, len);
-			if (used < 0) {
+			if (used <= 0) {
 				if (!copied)
 					copied = used;
 				break;