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

[Alexander Duyck <alexander.duyck@gmail.com>]

On Mon, Apr 30, 2012 at 6:27 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2012-04-30 at 16:36 -0700, Alexander Duyck wrote:
>> 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"
>> >

[...]

>> > 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.
>>
>
> Is it a general question, or specific to this patch ?
>
> If its a general problem, we already check dataref where appropriate.
> Fact that skb->head is a kmalloc() or frag doesnt matter.
>
> If specific to GRO, see my first answer. GRO owns each skb.
>
> adding a BUG() would make no sense here.

The question I had was more specific to GRO.  As long as we have
skb->users == 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.

Thanks,

Alex