On Mon, Apr 30, 2012 at 6:27 PM, Eric Dumazet [off-list ref] wrote:

On Mon, 2012-04-30 at 16:36 -0700, Alexander Duyck wrote:

quoted

On 04/30/2012 11:10 AM, Eric Dumazet wrote:

quoted

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"

[...]

quoted

quoted

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