alloc skb based on a given data buffer

alloc skb based on a given data buffer

[Zhu Yi]

[ Cc netdev and change the subject ]

On Mon, 2009-09-14 at 21:06 +0800, Mel Gorman wrote:
> > Essentially, the hardware only requires an order-1 allocation aligned on
> > 256 bytes boundary. But as it is used as an SKB, a trailing struct
> > skb_shared_info is added. This forces us to both increase the order and
> > do alignment ourselves. I believe some improvement could be done here.
> > But it should not be an easy one.
> > 
> 
> Probably not. I can only assume that moving the location of
> skb_shared_info such that it is sometimes located after the buffer and
> sometimes allocated via a separate kmalloc() would be a significant
> undertaking.

Shall I propose below function as a variant to alloc_skb()?

struct sk_buff *alloc_skb_attach_buff(void *data_buff,
				      unsigned int real_size,
				      unsigned int size, gfp_t mask);

If real_size >= size + sizeof(struct skb_shared_info), we can still put
the shinfo at the end of the buffer. Otherwise we can allocate from a
new slab that put sk_buff and shinfo together. Of course, kfree_skbmem()
needs to recognize it.

This way, device drivers can allocate the Rx buffers with their own size
and alignment requirement. i.e. do an order-1 page allocation directly
with free_pages() in the iwlagn driver for a 256 bytes aligned 8K Rx
buffer. After DMA is finished, drivers can use the above function to
assemble an skb based on the Rx buffer. It should resolve the problem
for requiring an order-2 allocation by alloc_skb() in the first place.

Comments are welcome.

Thanks,
-yi

Re: alloc skb based on a given data buffer

[David Miller]

From: Zhu Yi <yi.zhu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Tue, 15 Sep 2009 16:30:20 +0800

> This way, device drivers can allocate the Rx buffers with their own size
> and alignment requirement. i.e. do an order-1 page allocation directly
> with free_pages() in the iwlagn driver for a 256 bytes aligned 8K Rx
> buffer. After DMA is finished, drivers can use the above function to
> assemble an skb based on the Rx buffer. It should resolve the problem
> for requiring an order-2 allocation by alloc_skb() in the first place.

You can create paged RX skbs just like drivers such as niu.c
and others already do, there is no need for special APIs for
this.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: alloc skb based on a given data buffer

[Zhu Yi]

On Tue, 2009-09-15 at 16:33 +0800, David Miller wrote:
> From: Zhu Yi <yi.zhu@intel.com>
> Date: Tue, 15 Sep 2009 16:30:20 +0800
> 
> > This way, device drivers can allocate the Rx buffers with their own size
> > and alignment requirement. i.e. do an order-1 page allocation directly
> > with free_pages() in the iwlagn driver for a 256 bytes aligned 8K Rx
> > buffer. After DMA is finished, drivers can use the above function to
> > assemble an skb based on the Rx buffer. It should resolve the problem
> > for requiring an order-2 allocation by alloc_skb() in the first place.
> 
> You can create paged RX skbs just like drivers such as niu.c
> and others already do, there is no need for special APIs for
> this.

Thanks. So we can put the 8K buffer into 2 skb_shinfo()->frags[] slots
and set nr_frags to 2, right? Is this supported allover the network code
already? At a first glance, I didn't find any frags handling in mac80211
stack.

Thanks,
-yi

Re: alloc skb based on a given data buffer

[David Miller]

From: Zhu Yi <yi.zhu@intel.com>
Date: Tue, 15 Sep 2009 16:57:29 +0800

> Thanks. So we can put the 8K buffer into 2 skb_shinfo()->frags[] slots
> and set nr_frags to 2, right? Is this supported allover the network code
> already? At a first glance, I didn't find any frags handling in mac80211
> stack.

You have to pre-pull the link level protocol headers into the
linear area, but that's it.

Again, see niu.c for details, it does:

static void niu_rx_skb_append(struct sk_buff *skb, struct page *page,
			      u32 offset, u32 size)
{
	int i = skb_shinfo(skb)->nr_frags;
	skb_frag_t *frag = &skb_shinfo(skb)->frags[i];

	frag->page = page;
	frag->page_offset = offset;
	frag->size = size;

	skb->len += size;
	skb->data_len += size;
	skb->truesize += size;

	skb_shinfo(skb)->nr_frags = i + 1;
}

to add pages to SKBs and then at the end it goes:

	skb_reserve(skb, NET_IP_ALIGN);
	__pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX));

Right before giving the SKB to the networking stack.  NIU_RXPULL_MAX
should be a value that will be large enough to cover the largest
possible link level header.

Re: alloc skb based on a given data buffer

[Zhu Yi]

On Tue, 2009-09-15 at 17:09 +0800, David Miller wrote:
> From: Zhu Yi <yi.zhu@intel.com>
> Date: Tue, 15 Sep 2009 16:57:29 +0800
> 
> > Thanks. So we can put the 8K buffer into 2 skb_shinfo()->frags[] slots
> > and set nr_frags to 2, right? Is this supported allover the network code
> > already? At a first glance, I didn't find any frags handling in mac80211
> > stack.
> 
> You have to pre-pull the link level protocol headers into the
> linear area, but that's it.
> 
> Again, see niu.c for details, it does:
> 
> static void niu_rx_skb_append(struct sk_buff *skb, struct page *page,
> 			      u32 offset, u32 size)
> {
> 	int i = skb_shinfo(skb)->nr_frags;
> 	skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> 
> 	frag->page = page;
> 	frag->page_offset = offset;
> 	frag->size = size;
> 
> 	skb->len += size;
> 	skb->data_len += size;
> 	skb->truesize += size;
> 
> 	skb_shinfo(skb)->nr_frags = i + 1;
> }
> 
> to add pages to SKBs and then at the end it goes:
> 
> 	skb_reserve(skb, NET_IP_ALIGN);
> 	__pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX));
> 
> Right before giving the SKB to the networking stack.  NIU_RXPULL_MAX
> should be a value that will be large enough to cover the largest
> possible link level header.

I see. Thanks for this info. I'll try implementing the same for iwlagn.

Thanks,
-yi

Re: alloc skb based on a given data buffer

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]

On Tue, 2009-09-15 at 17:15 +0800, Zhu Yi wrote:
> On Tue, 2009-09-15 at 17:09 +0800, David Miller wrote:
> > From: Zhu Yi <yi.zhu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Date: Tue, 15 Sep 2009 16:57:29 +0800
> > 
> > > Thanks. So we can put the 8K buffer into 2 skb_shinfo()->frags[] slots
> > > and set nr_frags to 2, right? Is this supported allover the network code
> > > already? At a first glance, I didn't find any frags handling in mac80211
> > > stack.
> > 
> > You have to pre-pull the link level protocol headers into the
> > linear area, but that's it.
> > 
> > Again, see niu.c for details, it does:
> > 
> > static void niu_rx_skb_append(struct sk_buff *skb, struct page *page,
> > 			      u32 offset, u32 size)
> > {
> > 	int i = skb_shinfo(skb)->nr_frags;
> > 	skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> > 
> > 	frag->page = page;
> > 	frag->page_offset = offset;
> > 	frag->size = size;
> > 
> > 	skb->len += size;
> > 	skb->data_len += size;
> > 	skb->truesize += size;
> > 
> > 	skb_shinfo(skb)->nr_frags = i + 1;
> > }
> > 
> > to add pages to SKBs and then at the end it goes:
> > 
> > 	skb_reserve(skb, NET_IP_ALIGN);
> > 	__pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX));
> > 
> > Right before giving the SKB to the networking stack.  NIU_RXPULL_MAX
> > should be a value that will be large enough to cover the largest
> > possible link level header.
> 
> I see. Thanks for this info. I'll try implementing the same for iwlagn.

Hold, mac80211 can't cope with that at this point for sw crypto and
possibly other things.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: alloc skb based on a given data buffer

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Tue, 15 Sep 2009 08:30:31 -0700

> Hold, mac80211 can't cope with that at this point for sw crypto and
> possibly other things.

Then it should skb_linearize() at input, or similar.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: alloc skb based on a given data buffer

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

On Tue, 2009-09-15 at 14:16 -0700, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Tue, 15 Sep 2009 08:30:31 -0700
> 
> > Hold, mac80211 can't cope with that at this point for sw crypto and
> > possibly other things.
> 
> Then it should skb_linearize() at input, or similar.

Not all that much won then since that again means an order-2 allocation.
Fewer, certainly, as most packets are actually much smaller than that.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]