[PATCH 2/2] correct dev_alloc_skb kerneldoc

[PATCH 2/2] correct dev_alloc_skb kerneldoc

[Christoph Hellwig]

dev_alloc_skb is designated for RX descriptors, not TX.  (Some drivers
use it for the latter anyway, but that's a different story)


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/include/linux/skbuff.h
===================================================================
--- linux-2.6.orig/include/linux/skbuff.h	2006-07-07 11:05:30.000000000 +0200
+++ linux-2.6/include/linux/skbuff.h	2006-07-07 11:06:31.000000000 +0200
@@ -1067,7 +1067,7 @@
 }
 
 /**
- *	__dev_alloc_skb - allocate an skbuff for sending
+ *	__dev_alloc_skb - allocate an skbuff for receiving
  *	@length: length to allocate
  *	@gfp_mask: get_free_pages mask, passed to alloc_skb
  *
@@ -1088,7 +1088,7 @@
 }
 
 /**
- *	dev_alloc_skb - allocate an skbuff for sending
+ *	dev_alloc_skb - allocate an skbuff for receiving
  *	@length: length to allocate
  *
  *	Allocate a new &sk_buff and assign it a usage count of one. The

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[Stephen Hemminger]

On Fri, 7 Jul 2006 11:09:57 +0200
Christoph Hellwig <hch@lst.de> wrote:

> dev_alloc_skb is designated for RX descriptors, not TX.  (Some drivers
> use it for the latter anyway, but that's a different story)
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6/include/linux/skbuff.h
> ===================================================================
> --- linux-2.6.orig/include/linux/skbuff.h	2006-07-07 11:05:30.000000000 +0200
> +++ linux-2.6/include/linux/skbuff.h	2006-07-07 11:06:31.000000000 +0200
> @@ -1067,7 +1067,7 @@
>  }
>  
>  /**
> - *	__dev_alloc_skb - allocate an skbuff for sending
> + *	__dev_alloc_skb - allocate an skbuff for receiving
>   *	@length: length to allocate
>   *	@gfp_mask: get_free_pages mask, passed to alloc_skb
>   *
> @@ -1088,7 +1088,7 @@
>  }
>  
>  /**
> - *	dev_alloc_skb - allocate an skbuff for sending
> + *	dev_alloc_skb - allocate an skbuff for receiving
>   *	@length: length to allocate
>   *
>   *	Allocate a new &sk_buff and assign it a usage count of one. The
> -
> 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

What is the point of dev_alloc_skb anyway? all it does is add header space.

-- 
Stephen Hemminger <shemminger@osdl.org>
Quis custodiet ipsos custodes?

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Fri, 7 Jul 2006 15:52:55 -0700

> What is the point of dev_alloc_skb anyway? all it does is add header space.

In stone-age times it actually had specific semantics, but yes today
it is just a synonym.

It's going to be hard to get rid of it, every single network driver
out there references it.

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[Herbert Xu]

David Miller <davem@davemloft.net> wrote:
>> What is the point of dev_alloc_skb anyway? all it does is add header space.
> 
> In stone-age times it actually had specific semantics, but yes today
> it is just a synonym.

Does anyone still need those 16 bytes of header space?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[Christoph Hellwig]

On Fri, Jul 07, 2006 at 04:55:27PM -0700, David Miller wrote:
> From: Stephen Hemminger <shemminger@osdl.org>
> Date: Fri, 7 Jul 2006 15:52:55 -0700
> 
> > What is the point of dev_alloc_skb anyway? all it does is add header space.
> 
> In stone-age times it actually had specific semantics, but yes today
> it is just a synonym.
> 
> It's going to be hard to get rid of it, every single network driver
> out there references it.

On powerpc64 it actually makes a difference because it now allocates the
right headerspace as pad to ge the higher layer protocol headers aligned
for best dma performance.

Now what lead me up to this patch is something else.  On the Cell
hardware we badly need to allocate rx buffers on the local node, because
the access to memory on a different node is horrible slow (thank rambus!)

My plan is to give dev_alloc_skb a struct netdevice * argument and
introduce a alloc_netdev_node so the driver can tell what node the
device is on.  This gives a significant speedup for cell.  I already
have this implemented in fact but only converted a handfull of drivers.

Does this approach sound okay?

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[David Miller]

From: Christoph Hellwig <hch@lst.de>
Date: Sat, 8 Jul 2006 12:48:12 +0200

> My plan is to give dev_alloc_skb a struct netdevice * argument and
> introduce a alloc_netdev_node so the driver can tell what node the
> device is on.  This gives a significant speedup for cell.  I already
> have this implemented in fact but only converted a handfull of drivers.
> 
> Does this approach sound okay?

I do not think it is feasible to add a netdevice argument to
such a widely used interface.

I would instead recommend that you add a new interface, and
we convert drivers gradually.

That netdevice argument is useful for other things.  For example,
you can use it to apply a per-netdev random memory allocation
failure setting, to test the robustness of drivers.  I came up
with that idea something like 7 years ago but never got around
to doing it :)

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[Christoph Hellwig]

On Sat, Jul 08, 2006 at 01:37:38PM -0700, David Miller wrote:
> > My plan is to give dev_alloc_skb a struct netdevice * argument and
> > introduce a alloc_netdev_node so the driver can tell what node the
> > device is on.  This gives a significant speedup for cell.  I already
> > have this implemented in fact but only converted a handfull of drivers.
> > 
> > Does this approach sound okay?
> 
> I do not think it is feasible to add a netdevice argument to
> such a widely used interface.
> 
> I would instead recommend that you add a new interface, and
> we convert drivers gradually.

I gave converting the dev_alloc_skb prototype everywhere a spin and it
doesn't look too bad.  There's only a few places that don't have a
netdevice at hand, and all these look bogus:

 - drivers/atm/{idt77252.c, nicstar.c}, drivers/usb/atm/usbatm.c:
	These use private skb queues and do odd things.  I can't see
	any point for using dev_alloc_skb with it's additional headroom
	reservation here.
 - drivers/isdn/<everything> :
	These don't even have a netdevice!  Also I can't see why they
	need the headroom.
 - drivers/net/{ppp_async.c,drivers/net/ppp_async.c}:
   	They take data from the underlying tty device.  Because of that
	there's not point for he additional headroom.
 - net/irda/*:
   	They allocate the skb in protocol code for TX.  Should probably
	do a normal alloc_skb like all the other protocol code.
 - drivers/infiniband/hw/ipath/ipath_driver.c:
   	Uses skbbuffs in a driver that has absolutely nothing to do
	with networking.  Duh..

I've added the respective maintainers to the cc list here so they can
comment on these usages.

The patchkit for this is at http://verein.lst.de/~hch/patches.skb.tgz, it
includes the first two cleanup patches I posted previously (Any plans
to put them in?), a patch to move __dev_alloc_skb out of line because
otherwise we'd need to include netdevice.h in skbuff.h which creates
lots of problems (and moving it out of lines shaves 10kb off a
allyesconfig), the dev_alloc_skb prototype changes and some experimental
patches to make dev_alloc_skb nodeaware.

> That netdevice argument is useful for other things.  For example,
> you can use it to apply a per-netdev random memory allocation
> failure setting, to test the robustness of drivers.  I came up
> with that idea something like 7 years ago but never got around
> to doing it :)

Sounds cool.  While doing the change I also noticed that assigning
skb->dev can move in there aswell, but I haven't done that change yet.

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[Randy.Dunlap]

On Thu, 13 Jul 2006 22:36:16 +0200 Christoph Hellwig wrote:

...
> 
> I gave converting the dev_alloc_skb prototype everywhere a spin and it
> doesn't look too bad.  There's only a few places that don't have a
> netdevice at hand, and all these look bogus:
> 
...

>  - drivers/infiniband/hw/ipath/ipath_driver.c:
>    	Uses skbbuffs in a driver that has absolutely nothing to do
> 	with networking.  Duh..

Doesn't using netlink at all have that same effect?  (using skbs
in some code that may have nothing to do with networking)

Or should netlink be limited in its scope?

---
~Randy

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[David Miller]

From: Christoph Hellwig <hch@lst.de>
Date: Thu, 13 Jul 2006 22:36:16 +0200

>  - drivers/atm/{idt77252.c, nicstar.c}, drivers/usb/atm/usbatm.c:
> 	These use private skb queues and do odd things.  I can't see
> 	any point for using dev_alloc_skb with it's additional headroom
> 	reservation here.
>  - drivers/isdn/<everything> :
> 	These don't even have a netdevice!  Also I can't see why they
> 	need the headroom.
>  - drivers/net/{ppp_async.c,drivers/net/ppp_async.c}:
>    	They take data from the underlying tty device.  Because of that
> 	there's not point for he additional headroom.
>  - net/irda/*:
>    	They allocate the skb in protocol code for TX.  Should probably
> 	do a normal alloc_skb like all the other protocol code.
>  - drivers/infiniband/hw/ipath/ipath_driver.c:
>    	Uses skbbuffs in a driver that has absolutely nothing to do
> 	with networking.  Duh..

Any of these cases that DMA into the skb allocated will need
the headroom, as explained by the PowerPC folks.

I can see how PPP just copies into the skb, but some of the
ISDN bits will be doing DMA won't they?

I agree that IRDA's transmit side shouldn't be calling
dev_alloc_skb.

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[Stephen Hemminger]

On Thu, 13 Jul 2006 22:36:16 +0200
Christoph Hellwig <hch@lst.de> wrote:

> On Sat, Jul 08, 2006 at 01:37:38PM -0700, David Miller wrote:
> > > My plan is to give dev_alloc_skb a struct netdevice * argument and
> > > introduce a alloc_netdev_node so the driver can tell what node the
> > > device is on.  This gives a significant speedup for cell.  I already
> > > have this implemented in fact but only converted a handfull of drivers.
> > > 
> > > Does this approach sound okay?
> > 
> > I do not think it is feasible to add a netdevice argument to
> > such a widely used interface.
> > 
> > I would instead recommend that you add a new interface, and
> > we convert drivers gradually.
> 
> I gave converting the dev_alloc_skb prototype everywhere a spin and it
> doesn't look too bad.  There's only a few places that don't have a
> netdevice at hand, and all these look bogus:
> 
>  - drivers/atm/{idt77252.c, nicstar.c}, drivers/usb/atm/usbatm.c:
> 	These use private skb queues and do odd things.  I can't see
> 	any point for using dev_alloc_skb with it's additional headroom
> 	reservation here.
>  - drivers/isdn/<everything> :
> 	These don't even have a netdevice!  Also I can't see why they
> 	need the headroom.
>  - drivers/net/{ppp_async.c,drivers/net/ppp_async.c}:
>    	They take data from the underlying tty device.  Because of that
> 	there's not point for he additional headroom.
>  - net/irda/*:
>    	They allocate the skb in protocol code for TX.  Should probably
> 	do a normal alloc_skb like all the other protocol code.
>  - drivers/infiniband/hw/ipath/ipath_driver.c:
>    	Uses skbbuffs in a driver that has absolutely nothing to do
> 	with networking.  Duh..
> 
> I've added the respective maintainers to the cc list here so they can
> comment on these usages.
> 
> The patchkit for this is at http://verein.lst.de/~hch/patches.skb.tgz, it
> includes the first two cleanup patches I posted previously (Any plans
> to put them in?), a patch to move __dev_alloc_skb out of line because
> otherwise we'd need to include netdevice.h in skbuff.h which creates
> lots of problems (and moving it out of lines shaves 10kb off a
> allyesconfig), the dev_alloc_skb prototype changes and some experimental
> patches to make dev_alloc_skb nodeaware.
> 
> > That netdevice argument is useful for other things.  For example,
> > you can use it to apply a per-netdev random memory allocation
> > failure setting, to test the robustness of drivers.  I came up
> > with that idea something like 7 years ago but never got around
> > to doing it :)
> 
> Sounds cool.  While doing the change I also noticed that assigning
> skb->dev can move in there aswell, but I haven't done that change yet.

What's the new name? For sanity, don't reuse the name dev_alloc_skb.

-- 
Stephen Hemminger <shemminger@osdl.org>
"And in the Packet there writ down that doome"

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[Christoph Hellwig]

> What's the new name? For sanity, don't reuse the name dev_alloc_skb.

dev_alloc_skb.  It's the by far sanest name for it, while the old
usage without dev argument was misleading.

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[David Miller]

From: Christoph Hellwig <hch@lst.de>
Date: Thu, 13 Jul 2006 23:29:19 +0200

> > What's the new name? For sanity, don't reuse the name dev_alloc_skb.
> 
> dev_alloc_skb.  It's the by far sanest name for it, while the old
> usage without dev argument was misleading.

As much as I would like to make a clean break, you just can't
do this Christoph.

There is so much out of tree code that we will unfairly break
by doing this.

Add a new name, schedule dev_alloc_skb for a 6 month removal,
and convert as much of the in-tree uses as you like.  Mark
dev_alloc_skb deprecated while you're at it.

This is consistent with how we handle any other widely used
interface in the kernel.

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[Christoph Hellwig]

On Fri, Jul 14, 2006 at 10:27:27AM +0300, Samuel Ortiz wrote:
> > The patchkit for this is at http://verein.lst.de/~hch/patches.skb.tgz, it
> > includes the first two cleanup patches I posted previously (Any plans
> > to put them in?), a patch to move __dev_alloc_skb out of line because
> > otherwise we'd need to include netdevice.h in skbuff.h which creates
> > lots of problems (and moving it out of lines shaves 10kb off a
> > allyesconfig), the dev_alloc_skb prototype changes and some experimental
> > patches to make dev_alloc_skb nodeaware.
> In those patches you change dev_alloc_skb prototype, but then the new netdev
> argument is not used by __dev_alloc_skb() to allocate the sk_buff on the
> correct node. Do you plan to introduce an alloc_skb_node() later or am I 
> missing something ? 

The nodeaware-skb_alloc patch later in the series introduces a node
argument to __alloc_skb.  the nodeaware-netdev patch adds a node
argument to struct netdevice that dev_alloc_skb then passes to
__alloc_skb.  All of these patches are in the tarball my previous mail
points to.

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[Samuel Ortiz]

On Thu, Jul 13, 2006 at 10:36:16PM +0200, Christoph Hellwig wrote:
>  - net/irda/*:
>    	They allocate the skb in protocol code for TX.  Should probably
> 	do a normal alloc_skb like all the other protocol code.
Agreed. I will come up with a patch replacing all the useless dev_alloc_skb()
calls from the IrDA stack.


> The patchkit for this is at http://verein.lst.de/~hch/patches.skb.tgz, it
> includes the first two cleanup patches I posted previously (Any plans
> to put them in?), a patch to move __dev_alloc_skb out of line because
> otherwise we'd need to include netdevice.h in skbuff.h which creates
> lots of problems (and moving it out of lines shaves 10kb off a
> allyesconfig), the dev_alloc_skb prototype changes and some experimental
> patches to make dev_alloc_skb nodeaware.
In those patches you change dev_alloc_skb prototype, but then the new netdev
argument is not used by __dev_alloc_skb() to allocate the sk_buff on the
correct node. Do you plan to introduce an alloc_skb_node() later or am I 
missing something ? 

Cheers,
Samuel.

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[chas williams - CONTRACTOR]

In message <20060713.134735.23014475.davem@davemloft.net>,David Miller writes:
>From: Christoph Hellwig <hch@lst.de>
>Date: Thu, 13 Jul 2006 22:36:16 +0200
>>  - drivers/atm/{idt77252.c, nicstar.c}, drivers/usb/atm/usbatm.c:
>> 	These use private skb queues and do odd things.  I can't see
>> 	any point for using dev_alloc_skb with it's additional headroom
>> 	reservation here.

as i recall, for simple rfc1483 encapsulation, the LLC/SNAP header of 8
bytes needs to be replaced by an ether header of 14 bytes.  its likely
this is the reason dev_alloc_skb() is used.  most of the other drivers
use atm_alloc_charge() which does a skb_reserve(), but these drivers
dont dma directly into a skb.

>Any of these cases that DMA into the skb allocated will need
>the headroom, as explained by the PowerPC folks.

this only works for "ordinary" ip traffic (which i admit is the most
common case).  but since the nicstar/idt77252 devices have a peak
speed of 155Mb/s i doubt they would suffer much.  considering they still
use virt_to_bus/bus_to_virt i would be a bit surprised if they worked
(atleast on ppc64).  they can be simply converted to alloc_skb() and
manually skb_reserve() a little overhead.

usbatm should probably be converted to use atm_alloc_charge() since
the vcc is known at this point unlike the nicstar/idt77252 case.

Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc

[David Miller]

From: Christoph Hellwig <hch@lst.de>
Date: Fri, 7 Jul 2006 11:09:57 +0200

> dev_alloc_skb is designated for RX descriptors, not TX.  (Some drivers
> use it for the latter anyway, but that's a different story)
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Also applied, thanks a lot.