From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Duyck Subject: Re: [PATCH 3/4 v2 net-next] net: make GRO aware of skb->head_frag Date: Tue, 01 May 2012 16:10:50 -0700 Message-ID: <4FA06D7A.6090800@intel.com> References: <1335523026.2775.236.camel@edumazet-glaptop> <1335809434.2296.9.camel@edumazet-glaptop> <4F9F21E2.3080407@intel.com> <1335835677.11396.5.camel@edumazet-glaptop> <1335854378.11396.26.camel@edumazet-glaptop> <4FA00C9F.8080409@intel.com> <1335891892.22133.23.camel@edumazet-glaptop> <4FA06A94.8050704@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Alexander Duyck , David Miller , netdev , Neal Cardwell , Tom Herbert , Jeff Kirsher , Michael Chan , Matt Carlson , Herbert Xu , Ben Hutchings , =?UTF-8?B?SWxwbyBKw6RydmluZW4=?= , =?UTF-8?B?TWFjaWVqIMW7ZW5jenlrb3dza2k=?= To: Eric Dumazet Return-path: Received: from mga09.intel.com ([134.134.136.24]:60104 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755231Ab2EAXKv (ORCPT ); Tue, 1 May 2012 19:10:51 -0400 In-Reply-To: <4FA06A94.8050704@intel.com> Sender: netdev-owner@vger.kernel.org List-ID: On 05/01/2012 03:58 PM, Alexander Duyck wrote: > On 05/01/2012 10:04 AM, Eric Dumazet wrote: >> On Tue, 2012-05-01 at 09:17 -0700, Alexander Duyck wrote: >>> On 04/30/2012 11:39 PM, Eric Dumazet wrote: >>>> On Mon, 2012-04-30 at 22:33 -0700, Alexander Duyck wrote: >>>> >>>>> The question I had was more specific to GRO. As long as we have >>>>> skb->users =3D=3D 1 and the skb isn't cloned we should be fine. = It just >>>>> hadn't occurred to me before that napi_gro_receive had the extra >>>>> requirement that the skb couldn't be cloned. >>>>> >>>> OK >>>> >>>> By the way, even if skb was cloned, we would be allowed to steal >>>> skb->head. >>>> >>>> When we clone an oskb we : >>>> >>>> 1) allocate a struct nskb sk_buff (or use the shadow in case of TC= P) >>>> 2) increment dataref >>> The problem I have is with this piece right here. So you increment >>> dataref. Now you have an skb that is still pointing to the shared = info >>> on this page and dataref is 2. What about the side that is stealin= g the >>> head? Is it going to be tracking the dataref as well and decrement= ing >>> it before put_page or does it just assume that dataref is 1 and cal= l >>> put_page directly? I am guessing the latter since I didn't see any= thing >>> that allowed for tracking the dataref of stolen heads. >> The only changed thing is the kfree() replaced by put_page() >> >> This kfree() was done when last reference to dataref was released. >> >> If we had a problem before, we have same problem after my patch. >> >> Truth is : In TCP (coalesce and splice()) and GRO, we owns skbs. >> >> (See the various __kfree_skb(skb) calls in net/ipv4/tcp_input.c >> There is one exception in ipv6 / treq->pktopts ) but its for SYN pac= ket >> and this wont be merged with a previous packet. > Eric, > > I think I have found a bug, although it is not specific to this patch > but it is related. It looks like the TCP coalesce code is causing > tcpdump to fail when using frags. Based on the comments in the patch= I > am assuming you have an ixgbe adapter to test with as that is what I > reproduced this on. To reproduce the issue all you need to do is run > "tcpdump -i ethX > /dev/null" on one console, and on a second console > run a netperf TCP_MAERTS test to some other server. Tcpdump will exi= t > out with a message about bad address like this: > > tcpdump: verbose output suppressed, use -v or -vv for full protocol d= ecode > listening on eth1, link-type EN10MB (Ethernet), capture size 96 bytes > tcpdump: pcap_loop: recvfrom: Bad address > 682 packets captured > 2357 packets received by filter > 1543 packets dropped by kernel > > > A bisect of the issue tracked it down to: > > 1402d366019fedaa2b024f2bac06b7cc9a8782e1 is first bad commit > commit 1402d366019fedaa2b024f2bac06b7cc9a8782e1 > Author: Eric Dumazet > Date: Mon Apr 23 07:11:42 2012 +0000 > > tcp: introduce tcp_try_coalesce > =20 > commit c8628155ece3 (tcp: reduce out_of_order memory use) took ca= re of > coalescing tcp segments provided by legacy devices (linear skbs) > =20 > We extend this idea to fragged skbs, as their truesize can be hea= vy. > =20 > ixgbe for example uses 256+1024+PAGE_SIZE/2 =3D 3328 bytes per se= gment. > =20 > Use this coalescing strategy for receive queue too. > =20 > This contributes to reduce number of tcp collapses, at minimal co= st, and > reduces memory overhead and packets drops. > =20 > Signed-off-by: Eric Dumazet > Cc: Neal Cardwell > Cc: Tom Herbert > Cc: Maciej =C3=85=C2=BBenczykowski > Cc: Ilpo J=C3=83=C2=A4rvinen > Acked-by: Neal Cardwell > Signed-off-by: David S. Miller > > :040000 040000 8ca3e0b4e6c6a8f375fd800069d24203880623f3 2576d34c5c9cf= c717a11e2ebe054143956716b93 M net > > I suspect we are dealing with either a shared or cloned skb in this > case, though I haven't verified which it is yet. > > Thanks, > > Alex > One additional note. It looks like LRO and GRO need to be disabled to trigger the bug. If either of them are enabled it doesn't seem to occur. Likely due to the fact that they are doing the coalescing befor= e it gets up to the tcp_try_coalesce call. Thanks, Alex