| Message ID | 1357837570.27446.2285.camel@edumazet-glaptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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;