| Message ID | 4A5537DA.1060200@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Eric Dumazet wrote: > Tantilov, Emil S a écrit : >> Eric Dumazet wrote: >>> David Miller a écrit : >>>> From: "Tantilov, Emil S" <emil.s.tantilov@intel.com> >>>> Date: Wed, 8 Jul 2009 11:02:22 -0600 >>>> >>>>> Still seeing traces during the test even with this patch applied: >>>>> >>>>> [ 1089.430093] ------------[ cut here ]------------ >>>>> [ 1089.435667] WARNING: at include/net/sock.h:423 >>>>> udp_lib_unhash+0x73/0xa0() [ 1089.435670] Hardware name: S5520HC >>>> Ok I'll back this out for now, needs more investigation >>>> obviously. >>> Hmm... I never said it was supposed to fix Emil problem, just that >>> I discovered one potential problem by code inspection. >>> >>> I could not find yet sk_refcnt mismatch. >>> As we do less atomic ops per packet than before, some old bug could >>> surface now... >>> >>> Emil, is it easy to reproduce this problem, considering I have a >>> similar platform than yours (dual quad core machine, E5450 cpus @ >>> 3GHz) ? >> >> Eric, >> >> It should be easy to reproduce. At least I have been able to >> consistently >> reproduce it on several different systems with different drivers >> (e1000, e1000e, igb). >> >> The test I'm running is a mix of IPV4/6 TCP/UDP traffic with netperf >> (also mixing different types TCP/UDP_STREAM, TCP_MAERTS, TCP_UDP_RR >> etc). How much this matters I don't know - it's possible that just >> UDP traffic would do it. I also think it may have something to do >> with IPv6 because of the trace, but I am not sure. >> >> If you need more information let me know. >> > > OK thanks, this was helpful, corking or not corking, that is the > question :) > > I think ip6_push_pending_frames() & ip_push_pending_frames > have a problem after recent commit > 2b85a34e911bf483c27cfdd124aeb1605145dc80 (net: No more expensive > sock_hold()/sock_put() on each tx) > > [PATCH] net: ip_push_pending_frames() fix > > After commit 2b85a34e911bf483c27cfdd124aeb1605145dc80 > (net: No more expensive sock_hold()/sock_put() on each tx) > we do not take any more references on sk->sk_refcnt on outgoing > packets. > > I forgot to delete two __sock_put() from ip_push_pending_frames() > and ip6_push_pending_frames(). > > Reported-by: Emil S Tantilov <emils.tantilov@gmail.com> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > --- > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c > index 2470262..7d08210 100644 > --- a/net/ipv4/ip_output.c > +++ b/net/ipv4/ip_output.c > @@ -1243,7 +1243,6 @@ int ip_push_pending_frames(struct sock *sk) > skb->len += tmp_skb->len; > skb->data_len += tmp_skb->len; > skb->truesize += tmp_skb->truesize; > - __sock_put(tmp_skb->sk); > tmp_skb->destructor = NULL; > tmp_skb->sk = NULL; > } > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > index 7c76e3d..87f8419 100644 > --- a/net/ipv6/ip6_output.c > +++ b/net/ipv6/ip6_output.c > @@ -1484,7 +1484,6 @@ int ip6_push_pending_frames(struct sock *sk) > skb->len += tmp_skb->len; > skb->data_len += tmp_skb->len; > skb->truesize += tmp_skb->truesize; > - __sock_put(tmp_skb->sk); > tmp_skb->destructor = NULL; > tmp_skb->sk = NULL; > } Thanks Eric, With this patch the test ran all night without issues. Emil -- 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
Tantilov, Emil S a écrit : > Eric Dumazet wrote: >> Tantilov, Emil S a écrit : >>> Eric Dumazet wrote: >>>> David Miller a écrit : >>>>> From: "Tantilov, Emil S" <emil.s.tantilov@intel.com> >>>>> Date: Wed, 8 Jul 2009 11:02:22 -0600 >>>>> >>>>>> Still seeing traces during the test even with this patch applied: >>>>>> >>>>>> [ 1089.430093] ------------[ cut here ]------------ >>>>>> [ 1089.435667] WARNING: at include/net/sock.h:423 >>>>>> udp_lib_unhash+0x73/0xa0() [ 1089.435670] Hardware name: S5520HC >>>>> Ok I'll back this out for now, needs more investigation >>>>> obviously. >>>> Hmm... I never said it was supposed to fix Emil problem, just that >>>> I discovered one potential problem by code inspection. >>>> >>>> I could not find yet sk_refcnt mismatch. >>>> As we do less atomic ops per packet than before, some old bug could >>>> surface now... >>>> >>>> Emil, is it easy to reproduce this problem, considering I have a >>>> similar platform than yours (dual quad core machine, E5450 cpus @ >>>> 3GHz) ? >>> Eric, >>> >>> It should be easy to reproduce. At least I have been able to >>> consistently >>> reproduce it on several different systems with different drivers >>> (e1000, e1000e, igb). >>> >>> The test I'm running is a mix of IPV4/6 TCP/UDP traffic with netperf >>> (also mixing different types TCP/UDP_STREAM, TCP_MAERTS, TCP_UDP_RR >>> etc). How much this matters I don't know - it's possible that just >>> UDP traffic would do it. I also think it may have something to do >>> with IPv6 because of the trace, but I am not sure. >>> >>> If you need more information let me know. >>> >> OK thanks, this was helpful, corking or not corking, that is the >> question :) >> >> I think ip6_push_pending_frames() & ip_push_pending_frames >> have a problem after recent commit >> 2b85a34e911bf483c27cfdd124aeb1605145dc80 (net: No more expensive >> sock_hold()/sock_put() on each tx) >> >> [PATCH] net: ip_push_pending_frames() fix >> >> After commit 2b85a34e911bf483c27cfdd124aeb1605145dc80 >> (net: No more expensive sock_hold()/sock_put() on each tx) >> we do not take any more references on sk->sk_refcnt on outgoing >> packets. >> >> I forgot to delete two __sock_put() from ip_push_pending_frames() >> and ip6_push_pending_frames(). >> >> Reported-by: Emil S Tantilov <emils.tantilov@gmail.com> >> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> >> --- >> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c >> index 2470262..7d08210 100644 >> --- a/net/ipv4/ip_output.c >> +++ b/net/ipv4/ip_output.c >> @@ -1243,7 +1243,6 @@ int ip_push_pending_frames(struct sock *sk) >> skb->len += tmp_skb->len; >> skb->data_len += tmp_skb->len; >> skb->truesize += tmp_skb->truesize; >> - __sock_put(tmp_skb->sk); >> tmp_skb->destructor = NULL; >> tmp_skb->sk = NULL; >> } >> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c >> index 7c76e3d..87f8419 100644 >> --- a/net/ipv6/ip6_output.c >> +++ b/net/ipv6/ip6_output.c >> @@ -1484,7 +1484,6 @@ int ip6_push_pending_frames(struct sock *sk) >> skb->len += tmp_skb->len; >> skb->data_len += tmp_skb->len; >> skb->truesize += tmp_skb->truesize; >> - __sock_put(tmp_skb->sk); >> tmp_skb->destructor = NULL; >> tmp_skb->sk = NULL; >> } > > Thanks Eric, > > With this patch the test ran all night without issues. > Thanks a lot Emil for testing and your feedback. David, could you please add another tag ? Tested-by: Emil S Tantilov <emils.tantilov@gmail.com> 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
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 09 Jul 2009 02:20:42 +0200 > [PATCH] net: ip_push_pending_frames() fix > > After commit 2b85a34e911bf483c27cfdd124aeb1605145dc80 > (net: No more expensive sock_hold()/sock_put() on each tx) > we do not take any more references on sk->sk_refcnt on outgoing packets. > > I forgot to delete two __sock_put() from ip_push_pending_frames() > and ip6_push_pending_frames(). > > Reported-by: Emil S Tantilov <emils.tantilov@gmail.com> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Applied, with the Tested-by marker added. -- 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/ip_output.c b/net/ipv4/ip_output.c index 2470262..7d08210 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -1243,7 +1243,6 @@ int ip_push_pending_frames(struct sock *sk) skb->len += tmp_skb->len; skb->data_len += tmp_skb->len; skb->truesize += tmp_skb->truesize; - __sock_put(tmp_skb->sk); tmp_skb->destructor = NULL; tmp_skb->sk = NULL; } diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index 7c76e3d..87f8419 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -1484,7 +1484,6 @@ int ip6_push_pending_frames(struct sock *sk) skb->len += tmp_skb->len; skb->data_len += tmp_skb->len; skb->truesize += tmp_skb->truesize; - __sock_put(tmp_skb->sk); tmp_skb->destructor = NULL; tmp_skb->sk = NULL; }