Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
From: Christoph Hellwig [EMAIL PROTECTED] 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 [EMAIL PROTECTED] Also applied, thanks a lot. - 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
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
In message [EMAIL PROTECTED],David Miller writes:
From: Christoph Hellwig [EMAIL PROTECTED]
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.
-
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
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
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.
-
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
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
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 - 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
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
From: Christoph Hellwig [EMAIL PROTECTED]
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.
-
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
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
On Thu, 13 Jul 2006 22:36:16 +0200
Christoph Hellwig [EMAIL PROTECTED] 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 [EMAIL PROTECTED]
And in the Packet there writ down that doome
-
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
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
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. - 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
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
From: Christoph Hellwig [EMAIL PROTECTED] 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. - 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
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
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. - 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
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
On Fri, Jul 07, 2006 at 04:55:27PM -0700, David Miller wrote: From: Stephen Hemminger [EMAIL PROTECTED] 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? - 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
[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) Signed-off-by: Christoph Hellwig [EMAIL PROTECTED] Index: linux-2.6/include/linux/skbuff.h === --- linux-2.6.orig/include/linux/skbuff.h 2006-07-07 11:05:30.0 +0200 +++ linux-2.6/include/linux/skbuff.h2006-07-07 11:06:31.0 +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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
On Fri, 7 Jul 2006 11:09:57 +0200 Christoph Hellwig [EMAIL PROTECTED] 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 [EMAIL PROTECTED] Index: linux-2.6/include/linux/skbuff.h === --- linux-2.6.orig/include/linux/skbuff.h 2006-07-07 11:05:30.0 +0200 +++ linux-2.6/include/linux/skbuff.h 2006-07-07 11:06:31.0 +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 [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. -- Stephen Hemminger [EMAIL PROTECTED] Quis custodiet ipsos custodes? - 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
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
From: Stephen Hemminger [EMAIL PROTECTED] 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. - 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
Re: [PATCH 2/2] correct dev_alloc_skb kerneldoc
David Miller [EMAIL PROTECTED] 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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
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
