correct dev_alloc_skb kerneldoc
Subsystems (2)
PATCH 2/2 correct dev_alloc_skb kerneldoc
dev_alloc_skb is designated for RX descriptors, not TX. (Some drivers use it for the latter anyway, but that's a different story)
include/linux/skbuff.h +2 -2
@@ -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
Quoted 29 lines · On Fri, 7 Jul 2006 11:09:57 +0200 Christoph Hellwig <[email protected]> wrote:
dev_alloc_skbis 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 <[email protected]>Index:
linux-2.6/include/linux/skbuff.h
===================================================================include/linux/skbuff.h +2
-3@@ -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 -
Quoted 3 lines
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
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.
Signature · 2 lines
Stephen Hemminger <[email protected]> Quis custodiet ipsos custodes?
David Miller
maintainerFrom: Stephen Hemminger <[email protected]>
Date: Fri, 7 Jul 2006 15:52:55 -0700
Quoted 1 line
What is the point of
dev_alloc_skbanyway? 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.
Quoted 4 lines · David Miller
Quoted 1 line
What is the point of
dev_alloc_skbanyway? 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,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtQuoted 10 lines · David Miller
From: Stephen Hemminger <[email protected]>
Date: Fri, 7 Jul 2006 15:52:55 -0700Quoted 1 line
What is the point of
dev_alloc_skbanyway? 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?
David Miller
maintainerFrom: Christoph Hellwig <[email protected]>
Date: Sat, 8 Jul 2006 12:48:12 +0200
Quoted 6 lines
My plan is to give
dev_alloc_skba struct netdevice * argument and introduce aalloc_netdev_nodeso 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 :)
Quoted 5 lines · David Miller
Quoted 5 lines
My plan is to give
dev_alloc_skba struct netdevice * argument and introduce aalloc_netdev_nodeso 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.
Quoted 7 lines
Quoted 1 line
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.
Quoted 5 lines
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.
On Thu, 13 Jul 2006 22:36:16 +0200 Christoph Hellwig wrote:
...
Quoted 5 lines
I gave converting the
dev_alloc_skbprototype 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:
...
Quoted 3 lines
-
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
David Miller
maintainerFrom: Christoph Hellwig <[email protected]>
Date: Thu, 13 Jul 2006 22:36:16 +0200
Quoted 8 lines
-
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 usingdev_alloc_skbwith 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}:
Quoted 8 lines
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 normalalloc_skblike 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.
Quoted 6 lines · In message <[email protected]>,David Miller writes:
From: Christoph Hellwig <[email protected]>
Date: Thu, 13 Jul 2006 22:36:16 +0200Quoted 4 lines
-
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 usingdev_alloc_skbwith 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.
Quoted 2 lines
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.
Quoted 46 lines · On Thu, 13 Jul 2006 22:36:16 +0200 Christoph Hellwig <[email protected]> wrote:
Quoted 5 lines · David Miller
Quoted 5 lines
My plan is to give
dev_alloc_skba struct netdevice * argument and introduce aalloc_netdev_nodeso 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.Quoted 7 lines
Quoted 1 line
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_skbprototype 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 usingdev_alloc_skbwith 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 normalalloc_skblike 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_skbout of line because otherwise we'd need to includenetdevice.hinskbuff.hwhich creates lots of problems (and moving it out of lines shaves 10kb off a allyesconfig), thedev_alloc_skbprototype changes and some experimental patches to makedev_alloc_skbnodeaware.
Quoted 8 lines
Quoted 5 lines
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->devcan 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.
Signature · 2 lines
Stephen Hemminger <[email protected]> "And in the Packet there writ down that doome"
Quoted 1 line
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.
David Miller
maintainerFrom: Christoph Hellwig <[email protected]>
Date: Thu, 13 Jul 2006 23:29:19 +0200
Quoted 4 lines
Quoted 1 line
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.
Quoted 3 lines · Christoph Hellwig
-
net/irda/*:
They allocate the skb in protocol code for TX. Should probably do a normalalloc_skblike 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.
Quoted 7 lines
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_skbout of line because otherwise we'd need to includenetdevice.hinskbuff.hwhich creates lots of problems (and moving it out of lines shaves 10kb off a allyesconfig), thedev_alloc_skbprototype changes and some experimental patches to makedev_alloc_skbnodeaware.
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.
Quoted 11 lines · Samuel Ortiz
Quoted 7 lines
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_skbout of line because otherwise we'd need to includenetdevice.hinskbuff.hwhich creates lots of problems (and moving it out of lines shaves 10kb off a allyesconfig), thedev_alloc_skbprototype changes and some experimental patches to makedev_alloc_skbnodeaware.In those patches you change
dev_alloc_skbprototype, but then the new netdev argument is not used by__dev_alloc_skb()to allocate thesk_buffon the correct node. Do you plan to introduce analloc_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.
David Miller
maintainerFrom: Christoph Hellwig <[email protected]>
Date: Fri, 7 Jul 2006 11:09:57 +0200
Quoted 4 lines
dev_alloc_skbis 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 <[email protected]>
Also applied, thanks a lot.