[PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

skb->head is currently allocated from kmalloc(). This is convenient but
has the drawback the data cannot be converted to a page fragment if
needed.

We have three spots were it hurts :

1) GRO aggregation

 When a linear skb must be appended to another skb, GRO uses the
frag_list fallback, very inefficient since we keep all struct sk_buff
around. So drivers enabling GRO but delivering linear skbs to network
stack aren't enabling full GRO power.

2) splice(socket -> pipe).

 We must copy the linear part to a page fragment.
 This kind of defeats splice() purpose (zero copy claim)

3) TCP coalescing.

 Recently introduced, this permits to group several contiguous segments
into a single skb. This shortens queue lengths and save kernel memory,
and greatly reduce probabilities of TCP collapses. This coalescing
doesnt work on linear skbs (or we would need to copy data, this would be
too slow)

Given all these issues, the following patch introduces the possibility
of having skb->head be a fragment in itself. We use a new skb flag,
skb->head_frag to carry this information.

build_skb() is changed to accept a frag_size argument. Drivers willing
to provide a page fragment instead of kmalloc() data will set a non zero
value, set to the fragment size.

Then, on situations we need to convert the skb head to a frag in itself,
we can check if skb->head_frag is set and avoid the copies or various
fallbacks we have.

This means drivers currently using frags could be updated to avoid the
current skb->head allocation and reduce their memory footprint (aka skb
truesize). (thats 512 or 1024 bytes saved per skb). This also makes
bpf/netfilter faster since the 'first frag' will be part of skb linear
part, no need to copy data.

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>
---
 drivers/net/ethernet/broadcom/bnx2.c            |    2 -
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |    4 +-
 drivers/net/ethernet/broadcom/tg3.c             |    2 -
 include/linux/skbuff.h                          |    5 +-
 net/core/skbuff.c                               |   24 ++++++++++----
 5 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index ab55979..ac7b744 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -3006,7 +3006,7 @@ error:
 
 	dma_unmap_single(&bp->pdev->dev, dma_addr, bp->rx_buf_use_size,
 			 PCI_DMA_FROMDEVICE);
-	skb = build_skb(data);
+	skb = build_skb(data, 0);
 	if (!skb) {
 		kfree(data);
 		goto error;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 60d5b54..9c5bc5d 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -513,7 +513,7 @@ static inline void bnx2x_tpa_stop(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 	dma_unmap_single(&bp->pdev->dev, dma_unmap_addr(rx_buf, mapping),
 			 fp->rx_buf_size, DMA_FROM_DEVICE);
 	if (likely(new_data))
-		skb = build_skb(data);
+		skb = build_skb(data, 0);
 
 	if (likely(skb)) {
 #ifdef BNX2X_STOP_ON_ERROR
@@ -721,7 +721,7 @@ int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
 						 dma_unmap_addr(rx_buf, mapping),
 						 fp->rx_buf_size,
 						 DMA_FROM_DEVICE);
-				skb = build_skb(data);
+				skb = build_skb(data, 0);
 				if (unlikely(!skb)) {
 					kfree(data);
 					fp->eth_q_stats.rx_skb_alloc_failed++;
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 0c3e7c7..d481b0a 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -5844,7 +5844,7 @@ static int tg3_rx(struct tg3_napi *tnapi, int budget)
 			pci_unmap_single(tp->pdev, dma_addr, skb_size,
 					 PCI_DMA_FROMDEVICE);
 
-			skb = build_skb(data);
+			skb = build_skb(data, 0);
 			if (!skb) {
 				kfree(data);
 				goto drop_it_no_recycle;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 4a656b5..9d28a22 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -470,7 +470,8 @@ struct sk_buff {
 	__u8			wifi_acked_valid:1;
 	__u8			wifi_acked:1;
 	__u8			no_fcs:1;
-	/* 9/11 bit hole (depending on ndisc_nodetype presence) */
+	__u8			head_frag:1;
+	/* 8/10 bit hole (depending on ndisc_nodetype presence) */
 	kmemcheck_bitfield_end(flags2);
 
 #ifdef CONFIG_NET_DMA
@@ -562,7 +563,7 @@ extern void consume_skb(struct sk_buff *skb);
 extern void	       __kfree_skb(struct sk_buff *skb);
 extern struct sk_buff *__alloc_skb(unsigned int size,
 				   gfp_t priority, int fclone, int node);
-extern struct sk_buff *build_skb(void *data);
+extern struct sk_buff *build_skb(void *data, unsigned int frag_size);
 static inline struct sk_buff *alloc_skb(unsigned int size,
 					gfp_t priority)
 {
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 2342a72..effa75d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -245,6 +245,7 @@ EXPORT_SYMBOL(__alloc_skb);
 /**
  * build_skb - build a network buffer
  * @data: data buffer provided by caller
+ * @frag_size: size of fragment, or 0 if head was kmalloced
  *
  * Allocate a new &sk_buff. Caller provides space holding head and
  * skb_shared_info. @data must have been allocated by kmalloc()
@@ -258,20 +259,21 @@ EXPORT_SYMBOL(__alloc_skb);
  *  before giving packet to stack.
  *  RX rings only contains data buffers, not full skbs.
  */
-struct sk_buff *build_skb(void *data)
+struct sk_buff *build_skb(void *data, unsigned int frag_size)
 {
 	struct skb_shared_info *shinfo;
 	struct sk_buff *skb;
-	unsigned int size;
+	unsigned int size = frag_size ? : ksize(data);
 
 	skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
 	if (!skb)
 		return NULL;
 
-	size = ksize(data) - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+	size -= SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
 	memset(skb, 0, offsetof(struct sk_buff, tail));
 	skb->truesize = SKB_TRUESIZE(size);
+	skb->head_frag = frag_size != 0;
 	atomic_set(&skb->users, 1);
 	skb->head = data;
 	skb->data = data;
@@ -376,6 +378,14 @@ static void skb_clone_fraglist(struct sk_buff *skb)
 		skb_get(list);
 }
 
+static void skb_free_head(struct sk_buff *skb)
+{
+	if (skb->head_frag)
+		put_page(virt_to_head_page(skb->head));
+	else
+		kfree(skb->head);
+}
+
 static void skb_release_data(struct sk_buff *skb)
 {
 	if (!skb->cloned ||
@@ -402,7 +412,7 @@ static void skb_release_data(struct sk_buff *skb)
 		if (skb_has_frag_list(skb))
 			skb_drop_fraglist(skb);
 
-		kfree(skb->head);
+		skb_free_head(skb);
 	}
 }
 
@@ -644,6 +654,7 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
 	C(tail);
 	C(end);
 	C(head);
+	C(head_frag);
 	C(data);
 	C(truesize);
 	atomic_set(&n->users, 1);
@@ -940,7 +951,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 		fastpath = atomic_read(&skb_shinfo(skb)->dataref) == delta;
 	}
 
-	if (fastpath &&
+	if (fastpath && !skb->head_frag &&
 	    size + sizeof(struct skb_shared_info) <= ksize(skb->head)) {
 		memmove(skb->head + size, skb_shinfo(skb),
 			offsetof(struct skb_shared_info,
@@ -967,7 +978,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 	       offsetof(struct skb_shared_info, frags[skb_shinfo(skb)->nr_frags]));
 
 	if (fastpath) {
-		kfree(skb->head);
+		skb_free_head(skb);
 	} else {
 		/* copy this zero copy skb frags */
 		if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
@@ -985,6 +996,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 	off = (data + nhead) - skb->head;
 
 	skb->head     = data;
+	skb->head_frag = 0;
 adjust_others:
 	skb->data    += off;
 #ifdef NET_SKBUFF_DATA_USES_OFFSET

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Alexander Duyck]

On 04/27/2012 03:33 AM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> skb->head is currently allocated from kmalloc(). This is convenient but
> has the drawback the data cannot be converted to a page fragment if
> needed.
>
> We have three spots were it hurts :
>
> 1) GRO aggregation
>
>  When a linear skb must be appended to another skb, GRO uses the
> frag_list fallback, very inefficient since we keep all struct sk_buff
> around. So drivers enabling GRO but delivering linear skbs to network
> stack aren't enabling full GRO power.
>
> 2) splice(socket -> pipe).
>
>  We must copy the linear part to a page fragment.
>  This kind of defeats splice() purpose (zero copy claim)
>
> 3) TCP coalescing.
>
>  Recently introduced, this permits to group several contiguous segments
> into a single skb. This shortens queue lengths and save kernel memory,
> and greatly reduce probabilities of TCP collapses. This coalescing
> doesnt work on linear skbs (or we would need to copy data, this would be
> too slow)
>
> Given all these issues, the following patch introduces the possibility
> of having skb->head be a fragment in itself. We use a new skb flag,
> skb->head_frag to carry this information.
>
> build_skb() is changed to accept a frag_size argument. Drivers willing
> to provide a page fragment instead of kmalloc() data will set a non zero
> value, set to the fragment size.
>
> Then, on situations we need to convert the skb head to a frag in itself,
> we can check if skb->head_frag is set and avoid the copies or various
> fallbacks we have.
>
> This means drivers currently using frags could be updated to avoid the
> current skb->head allocation and reduce their memory footprint (aka skb
> truesize). (thats 512 or 1024 bytes saved per skb). This also makes
> bpf/netfilter faster since the 'first frag' will be part of skb linear
> part, no need to copy data.
[...]
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 2342a72..effa75d 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -245,6 +245,7 @@ EXPORT_SYMBOL(__alloc_skb);
>  /**
>   * build_skb - build a network buffer
>   * @data: data buffer provided by caller
> + * @frag_size: size of fragment, or 0 if head was kmalloced
>   *
>   * Allocate a new &sk_buff. Caller provides space holding head and
>   * skb_shared_info. @data must have been allocated by kmalloc()
> @@ -258,20 +259,21 @@ EXPORT_SYMBOL(__alloc_skb);
>   *  before giving packet to stack.
>   *  RX rings only contains data buffers, not full skbs.
>   */
> -struct sk_buff *build_skb(void *data)
> +struct sk_buff *build_skb(void *data, unsigned int frag_size)
>  {
>  	struct skb_shared_info *shinfo;
>  	struct sk_buff *skb;
> -	unsigned int size;
> +	unsigned int size = frag_size ? : ksize(data);
>  
>  	skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
>  	if (!skb)
>  		return NULL;
>  
> -	size = ksize(data) - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +	size -= SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>  
>  	memset(skb, 0, offsetof(struct sk_buff, tail));
>  	skb->truesize = SKB_TRUESIZE(size);
> +	skb->head_frag = frag_size != 0;
>  	atomic_set(&skb->users, 1);
>  	skb->head = data;
>  	skb->data = data;

This doesn't seem right to me.  You are only counting the piece of the
page that got filled with data and the piece that will get overwritten
with the shared info.  What about the rest of the page?  It looks like
in the tg3 patch you have the driver using a half page.  Based on this
function I suspect the resultant truesize would be something like 64 +
256 + 320 for an ack.  Shouldn't your truesize in that case be 2048 + 256?

Thanks,

Alex

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Eric Dumazet]

On Fri, 2012-04-27 at 17:27 -0700, Alexander Duyck wrote:

> This doesn't seem right to me.  You are only counting the piece of the
> page that got filled with data and the piece that will get overwritten
> with the shared info.  What about the rest of the page?  It looks like
> in the tg3 patch you have the driver using a half page.  Based on this
> function I suspect the resultant truesize would be something like 64 +
> 256 + 320 for an ack.  Shouldn't your truesize in that case be 2048 + 256?

Hi Alex

Not sure what you mean, since there is no such change in this patch.

driver must pass the truesize of the fragment, not the used one to
build_skb(). This is what I did in tg3 patch.

Therefore, all packets will have truesize = 2048 + 256.

The tg3 patch could be generalized to take care of special MTU, and be
able to use 1024 or 4096 as well for the frag size.

Lets discuss of the existing truesize problem.

There is one problem with fragments : We track the used length of them,
not their truesize.

So for example, GRO is not able to track resulting truesize.

We could split the "__u32 size;" to :

__u32 size:24;
__u32 truesizelog:8;

this_frag_truesize = max(1U << frag->truesizelog, frag->size);


  

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Eric Dumazet]

On Fri, 2012-04-27 at 17:27 -0700, Alexander Duyck wrote:
> On 04/27/2012 03:33 AM, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >

> > +struct sk_buff *build_skb(void *data, unsigned int frag_size)
> >  {
> >  	struct skb_shared_info *shinfo;
> >  	struct sk_buff *skb;
> > -	unsigned int size;
> > +	unsigned int size = frag_size ? : ksize(data);
> >  
> >  	skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
> >  	if (!skb)
> >  		return NULL;
> >  
> > -	size = ksize(data) - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > +	size -= SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> >  
> >  	memset(skb, 0, offsetof(struct sk_buff, tail));
> >  	skb->truesize = SKB_TRUESIZE(size);
> > +	skb->head_frag = frag_size != 0;
> >  	atomic_set(&skb->users, 1);
> >  	skb->head = data;
> >  	skb->data = data;
> 
> This doesn't seem right to me.  You are only counting the piece of the
> page that got filled with data and the piece that will get overwritten
> with the shared info.  What about the rest of the page?  It looks like
> in the tg3 patch you have the driver using a half page.  Based on this
> function I suspect the resultant truesize would be something like 64 +
> 256 + 320 for an ack.  Shouldn't your truesize in that case be 2048 + 256?

Re-reading your mail, I think you missed fact that tg3 driver currently
uses a kmalloc(64+1500+14+SKB_DATA_ALIGN(sizeof(struct
skb_shared_info)) , so basically does a kmalloc(2048). But this is done
before NIC fills the frame in it, so we cant know what will be the frame
length...

So build_skb() does later a ksize(data) and this gives 2048, even for a
small ACK packet ...

So the spirit of this patch is not to change any truesize.

tg3 for example splits a PAGE_SIZE into 2048 bytes frags (2 frags on x86
for example). Its done about the same in Intel IGB driver (IGB assumes
PAGE_SIZE is 4096 since it uses PAGE_SIZE/2)

The only thing that is changed here is where skb->head is allocated
from : kmalloc() caches or a frag from a page (and one reference to
page->_count)

Hope this clears your concern ?

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Alexander Duyck]

On Sat, Apr 28, 2012 at 12:16 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2012-04-27 at 17:27 -0700, Alexander Duyck wrote:
>> On 04/27/2012 03:33 AM, Eric Dumazet wrote:
>> > From: Eric Dumazet <edumazet@google.com>
>> >
>
>> > +struct sk_buff *build_skb(void *data, unsigned int frag_size)
>> >  {
>> >     struct skb_shared_info *shinfo;
>> >     struct sk_buff *skb;
>> > -   unsigned int size;
>> > +   unsigned int size = frag_size ? : ksize(data);
>> >
>> >     skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
>> >     if (!skb)
>> >             return NULL;
>> >
>> > -   size = ksize(data) - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>> > +   size -= SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>> >
>> >     memset(skb, 0, offsetof(struct sk_buff, tail));
>> >     skb->truesize = SKB_TRUESIZE(size);
>> > +   skb->head_frag = frag_size != 0;
>> >     atomic_set(&skb->users, 1);
>> >     skb->head = data;
>> >     skb->data = data;
>>
>> This doesn't seem right to me.  You are only counting the piece of the
>> page that got filled with data and the piece that will get overwritten
>> with the shared info.  What about the rest of the page?  It looks like
>> in the tg3 patch you have the driver using a half page.  Based on this
>> function I suspect the resultant truesize would be something like 64 +
>> 256 + 320 for an ack.  Shouldn't your truesize in that case be 2048 + 256?
>
> Re-reading your mail, I think you missed fact that tg3 driver currently
> uses a kmalloc(64+1500+14+SKB_DATA_ALIGN(sizeof(struct
> skb_shared_info)) , so basically does a kmalloc(2048). But this is done
> before NIC fills the frame in it, so we cant know what will be the frame
> length...
>
> So build_skb() does later a ksize(data) and this gives 2048, even for a
> small ACK packet ...
>
> So the spirit of this patch is not to change any truesize.
>
> tg3 for example splits a PAGE_SIZE into 2048 bytes frags (2 frags on x86
> for example). Its done about the same in Intel IGB driver (IGB assumes
> PAGE_SIZE is 4096 since it uses PAGE_SIZE/2)
>
> The only thing that is changed here is where skb->head is allocated
> from : kmalloc() caches or a frag from a page (and one reference to
> page->_count)
>
> Hope this clears your concern ?

I had misread what was happening here.  I didn't fully understand what
build_skb was doing before reading the patch and that is what led me
astray.

I was thinking there would be a bigger gain with a change like this
but essentially all you are doing is making the packets more mergable
resulting in you being able to drop the 256 bytes for the sk_buff in
either GRO or when it gets queued for the socket.

What caught my eye about all this is that I could probably cut a lot
of work out of the ixgbe driver and FCoE path by modifying things so
that FCoE packets could be received as a page, and then treated as a
linear skb without needing calls to skb_linearize.

Thanks,

Alex

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[David Miller]

From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Sat, 28 Apr 2012 01:27:24 -0700

> I was thinking there would be a bigger gain with a change like this
> but essentially all you are doing is making the packets more mergable
> resulting in you being able to drop the 256 bytes for the sk_buff in
> either GRO or when it gets queued for the socket.

He's also avoiding the SLUB slow path almost all of the time.

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 27 Apr 2012 12:33:38 +0200

> From: Eric Dumazet <edumazet@google.com>
> 
> skb->head is currently allocated from kmalloc(). This is convenient but
> has the drawback the data cannot be converted to a page fragment if
> needed.
 ...
> Given all these issues, the following patch introduces the possibility
> of having skb->head be a fragment in itself. We use a new skb flag,
> skb->head_frag to carry this information.
 ...
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Li Yu]

于 2012年04月27日 18:33, Eric Dumazet 写道:
> From: Eric Dumazet <edumazet@google.com>
>
> skb->head is currently allocated from kmalloc(). This is convenient but
> has the drawback the data cannot be converted to a page fragment if
> needed.
>

Hi, Eric,

I have a question about this patch, why data are allocated from 
kmalloc() can not be converted to page fragment ? We have its kernel
mapped address and length, so we can get its page and offset in the 
page. If the skb is not cloned (shared with others), such page and its
offset should be can use safely, in my words.

I suspected that I may lost important something in slab internals, is right?

Thanks

Yu

> We have three spots were it hurts :
>
> 1) GRO aggregation
>
>   When a linear skb must be appended to another skb, GRO uses the
> frag_list fallback, very inefficient since we keep all struct sk_buff
> around. So drivers enabling GRO but delivering linear skbs to network
> stack aren't enabling full GRO power.
>
> 2) splice(socket -> pipe).
>
>   We must copy the linear part to a page fragment.
>   This kind of defeats splice() purpose (zero copy claim)
>
> 3) TCP coalescing.
>
>   Recently introduced, this permits to group several contiguous segments
> into a single skb. This shortens queue lengths and save kernel memory,
> and greatly reduce probabilities of TCP collapses. This coalescing
> doesnt work on linear skbs (or we would need to copy data, this would be
> too slow)
>
> Given all these issues, the following patch introduces the possibility
> of having skb->head be a fragment in itself. We use a new skb flag,
> skb->head_frag to carry this information.
>
> build_skb() is changed to accept a frag_size argument. Drivers willing
> to provide a page fragment instead of kmalloc() data will set a non zero
> value, set to the fragment size.
>
> Then, on situations we need to convert the skb head to a frag in itself,
> we can check if skb->head_frag is set and avoid the copies or various
> fallbacks we have.
>
> This means drivers currently using frags could be updated to avoid the
> current skb->head allocation and reduce their memory footprint (aka skb
> truesize). (thats 512 or 1024 bytes saved per skb). This also makes
> bpf/netfilter faster since the 'first frag' will be part of skb linear
> part, no need to copy data.
>
> 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>
> ---
>   drivers/net/ethernet/broadcom/bnx2.c            |    2 -
>   drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |    4 +-
>   drivers/net/ethernet/broadcom/tg3.c             |    2 -
>   include/linux/skbuff.h                          |    5 +-
>   net/core/skbuff.c                               |   24 ++++++++++----
>   5 files changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
> index ab55979..ac7b744 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -3006,7 +3006,7 @@ error:
>
>   	dma_unmap_single(&bp->pdev->dev, dma_addr, bp->rx_buf_use_size,
>   			 PCI_DMA_FROMDEVICE);
> -	skb = build_skb(data);
> +	skb = build_skb(data, 0);
>   	if (!skb) {
>   		kfree(data);
>   		goto error;
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> index 60d5b54..9c5bc5d 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -513,7 +513,7 @@ static inline void bnx2x_tpa_stop(struct bnx2x *bp, struct bnx2x_fastpath *fp,
>   	dma_unmap_single(&bp->pdev->dev, dma_unmap_addr(rx_buf, mapping),
>   			 fp->rx_buf_size, DMA_FROM_DEVICE);
>   	if (likely(new_data))
> -		skb = build_skb(data);
> +		skb = build_skb(data, 0);
>
>   	if (likely(skb)) {
>   #ifdef BNX2X_STOP_ON_ERROR
> @@ -721,7 +721,7 @@ int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
>   						 dma_unmap_addr(rx_buf, mapping),
>   						 fp->rx_buf_size,
>   						 DMA_FROM_DEVICE);
> -				skb = build_skb(data);
> +				skb = build_skb(data, 0);
>   				if (unlikely(!skb)) {
>   					kfree(data);
>   					fp->eth_q_stats.rx_skb_alloc_failed++;
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 0c3e7c7..d481b0a 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -5844,7 +5844,7 @@ static int tg3_rx(struct tg3_napi *tnapi, int budget)
>   			pci_unmap_single(tp->pdev, dma_addr, skb_size,
>   					 PCI_DMA_FROMDEVICE);
>
> -			skb = build_skb(data);
> +			skb = build_skb(data, 0);
>   			if (!skb) {
>   				kfree(data);
>   				goto drop_it_no_recycle;
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 4a656b5..9d28a22 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -470,7 +470,8 @@ struct sk_buff {
>   	__u8			wifi_acked_valid:1;
>   	__u8			wifi_acked:1;
>   	__u8			no_fcs:1;
> -	/* 9/11 bit hole (depending on ndisc_nodetype presence) */
> +	__u8			head_frag:1;
> +	/* 8/10 bit hole (depending on ndisc_nodetype presence) */
>   	kmemcheck_bitfield_end(flags2);
>
>   #ifdef CONFIG_NET_DMA
> @@ -562,7 +563,7 @@ extern void consume_skb(struct sk_buff *skb);
>   extern void	       __kfree_skb(struct sk_buff *skb);
>   extern struct sk_buff *__alloc_skb(unsigned int size,
>   				   gfp_t priority, int fclone, int node);
> -extern struct sk_buff *build_skb(void *data);
> +extern struct sk_buff *build_skb(void *data, unsigned int frag_size);
>   static inline struct sk_buff *alloc_skb(unsigned int size,
>   					gfp_t priority)
>   {
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 2342a72..effa75d 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -245,6 +245,7 @@ EXPORT_SYMBOL(__alloc_skb);
>   /**
>    * build_skb - build a network buffer
>    * @data: data buffer provided by caller
> + * @frag_size: size of fragment, or 0 if head was kmalloced
>    *
>    * Allocate a new &sk_buff. Caller provides space holding head and
>    * skb_shared_info. @data must have been allocated by kmalloc()
> @@ -258,20 +259,21 @@ EXPORT_SYMBOL(__alloc_skb);
>    *  before giving packet to stack.
>    *  RX rings only contains data buffers, not full skbs.
>    */
> -struct sk_buff *build_skb(void *data)
> +struct sk_buff *build_skb(void *data, unsigned int frag_size)
>   {
>   	struct skb_shared_info *shinfo;
>   	struct sk_buff *skb;
> -	unsigned int size;
> +	unsigned int size = frag_size ? : ksize(data);
>
>   	skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
>   	if (!skb)
>   		return NULL;
>
> -	size = ksize(data) - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +	size -= SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>
>   	memset(skb, 0, offsetof(struct sk_buff, tail));
>   	skb->truesize = SKB_TRUESIZE(size);
> +	skb->head_frag = frag_size != 0;
>   	atomic_set(&skb->users, 1);
>   	skb->head = data;
>   	skb->data = data;
> @@ -376,6 +378,14 @@ static void skb_clone_fraglist(struct sk_buff *skb)
>   		skb_get(list);
>   }
>
> +static void skb_free_head(struct sk_buff *skb)
> +{
> +	if (skb->head_frag)
> +		put_page(virt_to_head_page(skb->head));
> +	else
> +		kfree(skb->head);
> +}
> +
>   static void skb_release_data(struct sk_buff *skb)
>   {
>   	if (!skb->cloned ||
> @@ -402,7 +412,7 @@ static void skb_release_data(struct sk_buff *skb)
>   		if (skb_has_frag_list(skb))
>   			skb_drop_fraglist(skb);
>
> -		kfree(skb->head);
> +		skb_free_head(skb);
>   	}
>   }
>
> @@ -644,6 +654,7 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
>   	C(tail);
>   	C(end);
>   	C(head);
> +	C(head_frag);
>   	C(data);
>   	C(truesize);
>   	atomic_set(&n->users, 1);
> @@ -940,7 +951,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>   		fastpath = atomic_read(&skb_shinfo(skb)->dataref) == delta;
>   	}
>
> -	if (fastpath &&
> +	if (fastpath && !skb->head_frag &&
>   	    size + sizeof(struct skb_shared_info) <= ksize(skb->head)) {
>   		memmove(skb->head + size, skb_shinfo(skb),
>   			offsetof(struct skb_shared_info,
> @@ -967,7 +978,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>   	       offsetof(struct skb_shared_info, frags[skb_shinfo(skb)->nr_frags]));
>
>   	if (fastpath) {
> -		kfree(skb->head);
> +		skb_free_head(skb);
>   	} else {
>   		/* copy this zero copy skb frags */
>   		if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
> @@ -985,6 +996,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>   	off = (data + nhead) - skb->head;
>
>   	skb->head     = data;
> +	skb->head_frag = 0;
>   adjust_others:
>   	skb->data    += off;
>   #ifdef NET_SKBUFF_DATA_USES_OFFSET
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Li Yu]

于 2012年11月07日 16:20, Li Yu 写道:
> 于 2012年04月27日 18:33, Eric Dumazet 写道:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> skb->head is currently allocated from kmalloc(). This is convenient but
>> has the drawback the data cannot be converted to a page fragment if
>> needed.
>>
>
> Hi, Eric,
>
> I have a question about this patch, why data are allocated from
> kmalloc() can not be converted to page fragment ? We have its kernel
> mapped address and length, so we can get its page and offset in the
> page. If the skb is not cloned (shared with others), such page and its
> offset should be can use safely, in my words.
>
> I suspected that I may lost important something in slab internals, is
> right?
>
> Thanks
>
> Yu
>

Or since slab allocated object may be across multiple pages?
I saw __netdev_alloc_skb() only use build_skb() with non-zero
fragsz if it is less than PAGE_SIZE.

Thanks

Yu

[snip]

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Eric Dumazet]

On Wed, 2012-11-07 at 16:35 +0800, Li Yu wrote:
> 于 2012年11月07日 16:20, Li Yu 写道:
> > 于 2012年04月27日 18:33, Eric Dumazet 写道:
> >> From: Eric Dumazet <edumazet@google.com>
> >>
> >> skb->head is currently allocated from kmalloc(). This is convenient but
> >> has the drawback the data cannot be converted to a page fragment if
> >> needed.
> >>
> >
> > Hi, Eric,
> >
> > I have a question about this patch, why data are allocated from
> > kmalloc() can not be converted to page fragment ? We have its kernel
> > mapped address and length, so we can get its page and offset in the
> > page. If the skb is not cloned (shared with others), such page and its
> > offset should be can use safely, in my words.
> >
> > I suspected that I may lost important something in slab internals, is
> > right?
> >
> > Thanks
> >
> > Yu
> >
> 
> Or since slab allocated object may be across multiple pages?
> I saw __netdev_alloc_skb() only use build_skb() with non-zero
> fragsz if it is less than PAGE_SIZE.
> 

SLAB allocators have their own way to handle a page, we cant interfere
with it.

So no, a kmalloced() object cannot be converted to a page fragment.

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Li Yu]

于 2012年11月07日 19:04, Eric Dumazet 写道:
> On Wed, 2012-11-07 at 16:35 +0800, Li Yu wrote:
>> 于 2012年11月07日 16:20, Li Yu 写道:
>>> 于 2012年04月27日 18:33, Eric Dumazet 写道:
>>>> From: Eric Dumazet <edumazet@google.com>
>>>>
>>>> skb->head is currently allocated from kmalloc(). This is convenient but
>>>> has the drawback the data cannot be converted to a page fragment if
>>>> needed.
>>>>
>>>
>>> Hi, Eric,
>>>
>>> I have a question about this patch, why data are allocated from
>>> kmalloc() can not be converted to page fragment ? We have its kernel
>>> mapped address and length, so we can get its page and offset in the
>>> page. If the skb is not cloned (shared with others), such page and its
>>> offset should be can use safely, in my words.
>>>
>>> I suspected that I may lost important something in slab internals, is
>>> right?
>>>
>>> Thanks
>>>
>>> Yu
>>>
>>
>> Or since slab allocated object may be across multiple pages?
>> I saw __netdev_alloc_skb() only use build_skb() with non-zero
>> fragsz if it is less than PAGE_SIZE.
>>
>
> SLAB allocators have their own way to handle a page, we cant interfere
> with it.
>
> So no, a kmalloced() object cannot be converted to a page fragment.
>

I think that all kmalloced() objects are continuous on both virtual
and physical address space, so such page fragment converting should be
safe until we use slab allocator to free them.

I ask this question since I am working on a patch to try to directly
forward the data from rx queue of a socket to tx queue of another 
socket. We noticed that the splice itself still has visible costs when 
forwarding small messages, and hope such direct forwarding optimization
can improve this, this idea comes from TCP friends.

Thanks

Yu

>
>
>

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Eric Dumazet]

On Thu, 2012-11-08 at 15:46 +0800, Li Yu wrote:

> I think that all kmalloced() objects are continuous on both virtual
> and physical address space, so such page fragment converting should be
> safe until we use slab allocator to free them.
> 

Its not safe at all. Good luck with coping with SLAB, SLUB, SLOB, and
various debugging facilities on top of them.

> I ask this question since I am working on a patch to try to directly
> forward the data from rx queue of a socket to tx queue of another 
> socket. We noticed that the splice itself still has visible costs when 
> forwarding small messages, and hope such direct forwarding optimization
> can improve this, this idea comes from TCP friends.

If not using splice, you directly transfert one skb coming from one
side, to the other side, so you dont care skb->head being a frag or not.

If messages are small, there is probably no hope trying to coalesce them
(or else fixing the application would be much easier)

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Li Yu]

于 2012年11月08日 21:30, Eric Dumazet 写道:
> On Thu, 2012-11-08 at 15:46 +0800, Li Yu wrote:
>
>> I think that all kmalloced() objects are continuous on both virtual
>> and physical address space, so such page fragment converting should be
>> safe until we use slab allocator to free them.
>>
>
> Its not safe at all. Good luck with coping with SLAB, SLUB, SLOB, and
> various debugging facilities on top of them.
>

The slab class API semantic should not change among them. the debugging
facilities should only add or modify metadata around user bytes too.
However, I also decided giving up use kmalloced() objects since it is 
hard to free them safely them without some extra efforts after doing
page fragmentation on them.

I think that adding skb->frag_head indeed is a better choice, this means 
I have to rework some NIC drivers :(

>> I ask this question since I am working on a patch to try to directly
>> forward the data from rx queue of a socket to tx queue of another
>> socket. We noticed that the splice itself still has visible costs when
>> forwarding small messages, and hope such direct forwarding optimization
>> can improve this, this idea comes from TCP friends.
>
> If not using splice, you directly transfert one skb coming from one
> side, to the other side, so you dont care skb->head being a frag or not.
>
> If messages are small, there is probably no hope trying to coalesce them
> (or else fixing the application would be much easier)
>

This patch is hoped that can work well for big messages too, so I
selected moving page fragment, and I think this should be easier a bit
because I can directly reuse tcp_sendpage() interface or just need a bit 
of changes on it :)

If it can work well, I will send it out.

Thans very much for your time, :)

Yu

>
>
>

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Eric Dumazet]

On Fri, 2012-11-09 at 10:31 +0800, Li Yu wrote:

> I think that adding skb->frag_head indeed is a better choice, this means 
> I have to rework some NIC drivers :(

Not sure what you mean, since most (if not all) NIC drivers already use
skb->frag_head in their RX path.

Re: [PATCH 1/4 net-next] net: allow skb->head to be a page fragment

[Li Yu]

于 2012年11月09日 10:37, Eric Dumazet 写道:
> On Fri, 2012-11-09 at 10:31 +0800, Li Yu wrote:
>
>> I think that adding skb->frag_head indeed is a better choice, this means
>> I have to rework some NIC drivers :(
>
> Not sure what you mean, since most (if not all) NIC drivers already use
> skb->frag_head in their RX path.
>

skb = build_skb(data, 0); in bnx2 and bnx2x ......

:)

Yu