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: Mon, 30 Apr 2012 16:36:02 -0700 Message-ID: <4F9F21E2.3080407@intel.com> References: <1335523026.2775.236.camel@edumazet-glaptop> <1335809434.2296.9.camel@edumazet-glaptop> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: 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 mga03.intel.com ([143.182.124.21]:5178 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756767Ab2D3XgD (ORCPT ); Mon, 30 Apr 2012 19:36:03 -0400 In-Reply-To: <1335809434.2296.9.camel@edumazet-glaptop> Sender: netdev-owner@vger.kernel.org List-ID: On 04/30/2012 11:10 AM, Eric Dumazet wrote: > From: Eric Dumazet > > GRO can check if skb to be merged has its skb->head mapped to a page > fragment, instead of a kmalloc() area. > > We 'upgrade' skb->head as a fragment in itself > > This avoids the frag_list fallback, and permits to build true GRO skb > (one sk_buff and up to 16 fragments), using less memory. > > This reduces number of cache misses when user makes its copy, since a > single sk_buff is fetched. > > This is a followup of patch "net: allow skb->head to be a page fragme= nt" > > Signed-off-by: Eric Dumazet > Cc: Ilpo J=C3=A4rvinen > Cc: Herbert Xu > Cc: Maciej =C5=BBenczykowski > Cc: Neal Cardwell > Cc: Tom Herbert > Cc: Jeff Kirsher > Cc: Ben Hutchings > Cc: Matt Carlson > Cc: Michael Chan > --- > > v2: change skb->head by skb->data to compute correct first_offset in > frag > > include/linux/netdevice.h | 2 ++ > include/linux/skbuff.h | 1 + > net/core/dev.c | 5 ++++- > net/core/skbuff.c | 27 ++++++++++++++++++++++++++- > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index e0b70e9..7f377fb 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1509,6 +1509,8 @@ struct napi_gro_cb { > =20 > /* Free the skb? */ > int free; > +#define NAPI_GRO_FREE 1 > +#define NAPI_GRO_FREE_STOLEN_HEAD 2 > }; > =20 > #define NAPI_GRO_CB(skb) ((struct napi_gro_cb *)(skb)->cb) > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index 9d28a22..2c75e98 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -561,6 +561,7 @@ static inline struct rtable *skb_rtable(const str= uct sk_buff *skb) > extern void kfree_skb(struct sk_buff *skb); > extern void consume_skb(struct sk_buff *skb); > extern void __kfree_skb(struct sk_buff *skb); > +extern struct kmem_cache *skbuff_head_cache; > extern struct sk_buff *__alloc_skb(unsigned int size, > gfp_t priority, int fclone, int node); > extern struct sk_buff *build_skb(void *data, unsigned int frag_size)= ; > diff --git a/net/core/dev.c b/net/core/dev.c > index 501f3cc..a2be59f 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -3546,7 +3546,10 @@ gro_result_t napi_skb_finish(gro_result_t ret,= struct sk_buff *skb) > break; > =20 > case GRO_MERGED_FREE: > - consume_skb(skb); > + if (NAPI_GRO_CB(skb)->free =3D=3D NAPI_GRO_FREE_STOLEN_HEAD) > + kmem_cache_free(skbuff_head_cache, skb); > + else > + __kfree_skb(skb); > break; > =20 > case GRO_HELD: How can you go from consume_skb() which checks for skb->users to a case that doesn't, or are you guaranteed that there is no way to get here with skb->users greater than 1? > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index effa75d..2ad1ee7 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -69,7 +69,7 @@ > #include > #include > =20 > -static struct kmem_cache *skbuff_head_cache __read_mostly; > +struct kmem_cache *skbuff_head_cache __read_mostly; > static struct kmem_cache *skbuff_fclone_cache __read_mostly; > =20 > static void sock_pipe_buf_release(struct pipe_inode_info *pipe, > @@ -2901,6 +2901,31 @@ int skb_gro_receive(struct sk_buff **head, str= uct sk_buff *skb) > =20 > NAPI_GRO_CB(skb)->free =3D 1; > goto done; > + } else if (skb->head_frag) { > + int nr_frags =3D pinfo->nr_frags; > + skb_frag_t *frag =3D pinfo->frags + nr_frags; > + struct page *page =3D virt_to_head_page(skb->head); > + unsigned int first_size =3D headlen - offset; > + unsigned int first_offset; > + > + if (nr_frags + 1 + skbinfo->nr_frags > MAX_SKB_FRAGS) > + return -E2BIG; > + > + first_offset =3D skb->data - > + (unsigned char *)page_address(page) + > + offset; > + > + pinfo->nr_frags =3D nr_frags + 1 + skbinfo->nr_frags; > + > + frag->page.p =3D page; > + frag->page_offset =3D first_offset; > + skb_frag_size_set(frag, first_size); > + > + memcpy(frag + 1, skbinfo->frags, sizeof(*frag) * skbinfo->nr_frags= ); > + /* We dont need to clear skbinfo->nr_frags here */ > + > + NAPI_GRO_CB(skb)->free =3D NAPI_GRO_FREE_STOLEN_HEAD; > + goto done; > } else if (skb_gro_len(p) !=3D pinfo->gso_size) > return -E2BIG; > =20 > > Maybe I missed something, but shouldn't you be checking skb->cloned, an= d skb_shinfo()->dataref before you can consider just dropping the sk_buff? It seems like if you are sharing the frag with a clone you would have to retain the skb->head so that you can track the dataref.=20 Otherwise you will likely cause issues because the stack could end up freeing the sk_buff, or the GRO frame will be capable of calling put_page and freeing the page out from under the clone. Thanks, Alex