Re: [PATCH 3/4 v2 net-next] net: make GRO aware of skb->head_frag

[Alexander Duyck <alexander.h.duyck@intel.com>]

On 04/30/2012 11:10 AM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> 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 fragment"
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Maciej Żenczykowski <maze@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Tom Herbert <therbert@google.com>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Ben Hutchings <bhutchings@solarflare.com>
> Cc: Matt Carlson <mcarlson@broadcom.com>
> Cc: Michael Chan <mchan@broadcom.com>
> ---
>
> 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 {
>  
>  	/* Free the skb? */
>  	int free;
> +#define NAPI_GRO_FREE		  1
> +#define NAPI_GRO_FREE_STOLEN_HEAD 2
>  };
>  
>  #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 struct 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;
>  
>  	case GRO_MERGED_FREE:
> -		consume_skb(skb);
> +		if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD)
> +			kmem_cache_free(skbuff_head_cache, skb);
> +		else
> +			__kfree_skb(skb);
>  		break;
>  
>  	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 <trace/events/skb.h>
>  #include <linux/highmem.h>
>  
> -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;
>  
>  static void sock_pipe_buf_release(struct pipe_inode_info *pipe,
> @@ -2901,6 +2901,31 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb)
>  
>  		NAPI_GRO_CB(skb)->free = 1;
>  		goto done;
> +	} else if (skb->head_frag) {
> +		int nr_frags = pinfo->nr_frags;
> +		skb_frag_t *frag = pinfo->frags + nr_frags;
> +		struct page *page = virt_to_head_page(skb->head);
> +		unsigned int first_size = headlen - offset;
> +		unsigned int first_offset;
> +
> +		if (nr_frags + 1 + skbinfo->nr_frags > MAX_SKB_FRAGS)
> +			return -E2BIG;
> +
> +		first_offset = skb->data -
> +			       (unsigned char *)page_address(page) +
> +			       offset;
> +
> +		pinfo->nr_frags = nr_frags + 1 + skbinfo->nr_frags;
> +
> +		frag->page.p	  = page;
> +		frag->page_offset = 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 = NAPI_GRO_FREE_STOLEN_HEAD;
> +		goto done;
>  	} else if (skb_gro_len(p) != pinfo->gso_size)
>  		return -E2BIG;
>  
>
>
Maybe I missed something, but shouldn't you be checking skb->cloned, and
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. 
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