[RFC] mac80211: assign needed_headroom/tailroom for netdevs

[RFC] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

This assigns the netdev's needed_headroom/tailroom members
to take advantage of pre-allocated space for 802.11 headers.

Signed-off-by: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
---
Builds on my other patch to add those fields, of course!

 net/mac80211/iface.c |    8 ++++++++
 net/mac80211/tx.c    |   14 +++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

--- everything.orig/net/mac80211/iface.c	2008-05-04 15:40:53.000000000 +0200
+++ everything/net/mac80211/iface.c	2008-05-04 16:08:45.000000000 +0200
@@ -53,6 +53,14 @@ int ieee80211_if_add(struct net_device *
 	if (!ndev)
 		return -ENOMEM;
 
+	ndev->needed_headroom = local->tx_headroom +
+				4*6 /* four MAC addresses */
+				+ 2 + 2 + 2 + 2 /* ctl, dur, seq, qos */
+				+ 6 /* mesh */
+				- ETH_HLEN /* ethernet hard_header_len */
+				+ IEEE80211_ENCRYPT_HEADROOM;
+	ndev->needed_tailroom = IEEE80211_ENCRYPT_TAILROOM;
+
 	ret = dev_alloc_name(ndev, ndev->name);
 	if (ret < 0)
 		goto fail;


--
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: [RFC] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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

On Sun, 2008-05-04 at 23:24 +0200, Johannes Berg wrote:
> This assigns the netdev's needed_headroom/tailroom members
> to take advantage of pre-allocated space for 802.11 headers.
> 
> Signed-off-by: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> ---
> Builds on my other patch to add those fields, of course!
> 
>  net/mac80211/iface.c |    8 ++++++++
>  net/mac80211/tx.c    |   14 +++++++++-----
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> --- everything.orig/net/mac80211/iface.c	2008-05-04 15:40:53.000000000 +0200
> +++ everything/net/mac80211/iface.c	2008-05-04 16:08:45.000000000 +0200
> @@ -53,6 +53,14 @@ int ieee80211_if_add(struct net_device *
>  	if (!ndev)
>  		return -ENOMEM;
>  
> +	ndev->needed_headroom = local->tx_headroom +
> +				4*6 /* four MAC addresses */
> +				+ 2 + 2 + 2 + 2 /* ctl, dur, seq, qos */
> +				+ 6 /* mesh */
> +				- ETH_HLEN /* ethernet hard_header_len */
> +				+ IEEE80211_ENCRYPT_HEADROOM;

Although this seems to have worked for me, that was most likely
coincidence because I had two bytes extra headroom and no fourth MAC
address, I actually forgot 8 bytes here (rfc1042/bridge tunnel
encapsulation)

johannes

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

[RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

This assigns the netdev's needed_headroom/tailroom members
to take advantage of pre-allocated space for 802.11 headers.

Signed-off-by: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
---
 net/mac80211/iface.c |    9 +++++++++
 1 file changed, 9 insertions(+)

--- everything.orig/net/mac80211/iface.c	2008-05-04 23:24:17.000000000 +0200
+++ everything/net/mac80211/iface.c	2008-05-04 23:31:46.000000000 +0200
@@ -53,6 +53,15 @@ int ieee80211_if_add(struct net_device *
 	if (!ndev)
 		return -ENOMEM;
 
+	ndev->needed_headroom = local->tx_headroom +
+				4*6 /* four MAC addresses */
+				+ 2 + 2 + 2 + 2 /* ctl, dur, seq, qos */
+				+ 6 /* mesh */
+				+ 8 /* rfc1042/bridge tunnel */
+				- ETH_HLEN /* ethernet hard_header_len */
+				+ IEEE80211_ENCRYPT_HEADROOM;
+	ndev->needed_tailroom = IEEE80211_ENCRYPT_TAILROOM;
+
 	ret = dev_alloc_name(ndev, ndev->name);
 	if (ret < 0)
 		goto fail;


--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Sun, 04 May 2008 23:32:25 +0200

> --- everything.orig/net/mac80211/iface.c	2008-05-04 23:24:17.000000000 +0200
> +++ everything/net/mac80211/iface.c	2008-05-04 23:31:46.000000000 +0200
> @@ -53,6 +53,15 @@ int ieee80211_if_add(struct net_device *
>  	if (!ndev)
>  		return -ENOMEM;
>  
> +	ndev->needed_headroom = local->tx_headroom +
> +				4*6 /* four MAC addresses */
> +				+ 2 + 2 + 2 + 2 /* ctl, dur, seq, qos */
> +				+ 6 /* mesh */
> +				+ 8 /* rfc1042/bridge tunnel */
> +				- ETH_HLEN /* ethernet hard_header_len */
> +				+ IEEE80211_ENCRYPT_HEADROOM;
> +	ndev->needed_tailroom = IEEE80211_ENCRYPT_TAILROOM;
> +
>  	ret = dev_alloc_name(ndev, ndev->name);
>  	if (ret < 0)
>  		goto fail;

Where is the patch that adds these new members to struct netdevice,
and where is the code that uses these new values?

I even searched the netdev archives and couldn't find it.

Anyways I see your basic idea and this may be the best way to handle
the problem.  The invariants would be:

1) LL_MAX_HEADER has to include all of these bits.

2) LL_RESERVED_SPACE*() has to take the new needed_headroom
   into account.

Your patch which I can't find, which adds netdev->needed_*, probably
does all of that.  But I'm just making sure :-)

Note that what would be really nice is if we could assert, in
dev_queue_xmit, that the SKB has all of the necessary headroom, and
give a WARN_ON_ONCE() backtrace if not.

If we can ensure that, things like mac80211 and others will not need
to skb_realloc_headroom() or anything like that unless they need to
modify packet contents after skb->data and the packet is shared
(ie. the pskb_expand_headroom(skb, 0, 0, GFP_*) case the TSO drivers
use).
--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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


> Where is the patch that adds these new members to struct netdevice,
> and where is the code that uses these new values?

Inlined below this time.

> Anyways I see your basic idea and this may be the best way to handle
> the problem.  The invariants would be:
> 
> 1) LL_MAX_HEADER has to include all of these bits.

Yes. Well, it has to be long enough for it, yes, but mac80211 has
drivers that want 100 bytes more headroom for themselves and probably
won't get added to LL_MAX_HEADER. But still putting that much into
needed_headroom is beneficial for many cases even if that means that
some packets will have to be copy-expanded. This, however, means that
you cannot put an assertion into dev_queue_xmit.

Also, taking into account tailroom isn't trivial because you need to
reserve the headroom but not the tailroom etc. I'm still thinking of a
way to avoid reserving tailroom completely when the hardware will handle
the crypto-checksumming but right now we don't have hardware that does
all crypto, only some algorithms. But we can even update
needed_headroom/tailroom on the fly.

> 2) LL_RESERVED_SPACE*() has to take the new needed_headroom
>    into account.

Check. I added LL_ALLOCATED_SPACE() for the tailroom and used that in
places for the allocation. I'm fairly sure I didn't get all of them
though.

> Your patch which I can't find, which adds netdev->needed_*, probably
> does all of that.  But I'm just making sure :-)
> 
> Note that what would be really nice is if we could assert, in
> dev_queue_xmit, that the SKB has all of the necessary headroom, and
> give a WARN_ON_ONCE() backtrace if not.
> 
> If we can ensure that, things like mac80211 and others will not need
> to skb_realloc_headroom() or anything like that unless they need to
> modify packet contents after skb->data and the packet is shared
> (ie. the pskb_expand_headroom(skb, 0, 0, GFP_*) case the TSO drivers
> use).

That would be nice, but I don't think it's possible.

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Tomas Winkler]

>  > If we can ensure that, things like mac80211 and others will not need
>  > to skb_realloc_headroom() or anything like that unless they need to
>  > modify packet contents after skb->data and the packet is shared
>  > (ie. the pskb_expand_headroom(skb, 0, 0, GFP_*) case the TSO drivers
>  > use).
>
>  That would be nice, but I don't think it's possible.
>
Not sure this is connected so sorry if I'm hijacking on wrong thread.
I might miss some emails on this thread.
I would like to request some comments on the following observation.
I'm just diving into this part of the kernel so please fix me if I've
missed something.

I've did some measurement of the TX path  on an embedded system (2.6.23 kernel)
When bridging packets from an ethernet device to wireless  there is
loss of 12% in the CPU utilization and equivalent throughput reduction
in data packets that are checked and expanded in
ieee80211_subif_start_xmit function.
The expansion is just due to bigger size of the 80211 header size, yet
the whole packet is reallocated and copied. As we are reaching 11n
rates 200bps and up this starts to be visible.
When header was reserved to the  proper side already in the ethernet
driver the overhead was gone. Yet this doesn't seems to be a correct
solution for bridging or forwarding.
For example Iwlwifi HW supports scattered packets this would allow
just reallocate the header, providing data portion is aligned.


Thanks
Tomas






>  johannes
>
--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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


> I've did some measurement of the TX path  on an embedded system (2.6.23 kernel)
> When bridging packets from an ethernet device to wireless  there is
> loss of 12% in the CPU utilization and equivalent throughput reduction
> in data packets that are checked and expanded in
> ieee80211_subif_start_xmit function.

Yeah, I figured.

> The expansion is just due to bigger size of the 80211 header size, yet
> the whole packet is reallocated and copied. As we are reaching 11n
> rates 200bps and up this starts to be visible.
> When header was reserved to the  proper side already in the ethernet
> driver the overhead was gone. Yet this doesn't seems to be a correct
> solution for bridging or forwarding.
> For example Iwlwifi HW supports scattered packets this would allow
> just reallocate the header, providing data portion is aligned.

Right. Not all hardware supports this though, but even when it does I
don't see what we can do unless we want to do all this inside mac80211
which I'd rather not.

SKBs don't have a way to say "I need N bytes writable headroom but I can
do s/g operation for the rest" which is what we'd need. If that was
available, we could even work with clones, then we could pull away the
ethernet header and build the 802.11/device headers in separate buffers,
demoting the old 'skb head' buffer to a data buffer...

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Tomas Winkler]

On Mon, May 5, 2008 at 6:22 PM, Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org> wrote:
>
>  > I've did some measurement of the TX path  on an embedded system (2.6.23 kernel)
>  > When bridging packets from an ethernet device to wireless  there is
>  > loss of 12% in the CPU utilization and equivalent throughput reduction
>  > in data packets that are checked and expanded in
>  > ieee80211_subif_start_xmit function.
>
>  Yeah, I figured.
>
>
>  > The expansion is just due to bigger size of the 80211 header size, yet
>  > the whole packet is reallocated and copied. As we are reaching 11n
>  > rates 200bps and up this starts to be visible.
>  > When header was reserved to the  proper side already in the ethernet
>  > driver the overhead was gone. Yet this doesn't seems to be a correct
>  > solution for bridging or forwarding.
>  > For example Iwlwifi HW supports scattered packets this would allow
>  > just reallocate the header, providing data portion is aligned.
>
>  Right. Not all hardware supports this though,
I would set  NETIF_F_SG OR FRAGLIS to features...?  (What actually is
the difference?)

but even when it does I
>  don't see what we can do unless we want to do all this inside mac80211
>  which I'd rather not.

Like Sending mac80211 header OOB as xmit function argument?  This will
probably affect all the wme code as well...
Not good.



>  SKBs don't have a way to say "I need N bytes writable headroom but I can
>  do s/g operation for the rest" which is what we'd need. If that was
>  available, we could even work with clones, then we could pull away the
>  ethernet header and build the 802.11/device headers in separate buffers,
>  demoting the old 'skb head' buffer to a data buffer...
>

Isn't this a requirement that header fits into a continuous buffer ?
It's sounds strange to me that there is no solution for efficient
bridging... Can bridging code handle this if we have native interface?


Thanks
Tomas

>  johannes
>
--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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


> I would set  NETIF_F_SG OR FRAGLIS to features...?  (What actually is
> the difference?)

Do you support TCP checksumming in hardware? -> no SG

NETIF_F_FRAGLIST I haven't understood, but it seems to apply mostly for
IP fragmentation?

> but even when it does I
> >  don't see what we can do unless we want to do all this inside mac80211
> >  which I'd rather not.
> 
> Like Sending mac80211 header OOB as xmit function argument?  This will
> probably affect all the wme code as well...
> Not good.

Yep.

> Isn't this a requirement that header fits into a continuous buffer ?

which header? The SKB header (skb->data .. skb->tail
(skb->head..skb->end)) or the 802.11/device header?

> It's sounds strange to me that there is no solution for efficient
> bridging... Can bridging code handle this if we have native interface?

I don't think so, but I'm not sure.

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 05 May 2008 19:57:34 +0200

> NETIF_F_FRAGLIST I haven't understood, but it seems to apply mostly for
> IP fragmentation?

That's right.  It's for cards that can do IP fragmentation in
hardware.

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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

On Mon, 2008-05-05 at 11:58 -0700, David Miller wrote:
> From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> Date: Mon, 05 May 2008 19:57:34 +0200
> 
> > NETIF_F_FRAGLIST I haven't understood, but it seems to apply mostly for
> > IP fragmentation?
> 
> That's right.  It's for cards that can do IP fragmentation in
> hardware.

What does "do IP fragmentation in hardware" mean?

ip_output.c says:

 *      This IP datagram is too large to be sent in one piece.  Break it up into
 *      smaller pieces (each of size equal to IP header plus
 *      a block of the data of the original IP data part) that will yet fit in a
 *      single device frame, and queue such a frame for sending.

and later

                        /* Prepare header of the next frame,
                         * before previous one went down. */

so that seems to indicate that if the hardware just sends it all out in
the right order it'll be fine.

So the point is that the hardware will have to split it up into ethernet
frames on the IP boundaries?

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 05 May 2008 21:05:15 +0200

> On Mon, 2008-05-05 at 11:58 -0700, David Miller wrote:
> > From: Johannes Berg <johannes@sipsolutions.net>
> > Date: Mon, 05 May 2008 19:57:34 +0200
> > 
> > > NETIF_F_FRAGLIST I haven't understood, but it seems to apply mostly for
> > > IP fragmentation?
> > 
> > That's right.  It's for cards that can do IP fragmentation in
> > hardware.
> 
> What does "do IP fragmentation in hardware" mean?

My bad, it merely allows for devices that can handle
frag lists in sk_buff as transmit packets.

I guess you could use this, btw, to take on arbitrary things
to the front of frames to handle big headers.  But that's more
expensive than just getting the LL header length right.

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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


> My bad, it merely allows for devices that can handle
> frag lists in sk_buff as transmit packets.

:)

> I guess you could use this, btw, to take on arbitrary things
> to the front of frames to handle big headers.  But that's more
> expensive than just getting the LL header length right.

Not necessarily because we seem to be getting a lot of cloned frames
that we need to copy all the time. If we would remove the ethernet
header and stick that skb into the fraglist of a new skb we'd win.

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 05 May 2008 21:57:36 +0200

> Not necessarily because we seem to be getting a lot of cloned frames
> that we need to copy all the time. If we would remove the ethernet
> header and stick that skb into the fraglist of a new skb we'd win.

And the copy is necessary because you need more tailroom or something?

I don't understand why simply having all the space you need is not
better and faster :-)

Even if you have a clone, you can muck with the data in the IP and TCP
header areas, so long as skb_header_cloned() is false.

So I can only conclude that your problematic case is that of
encryption, right?  That's the only situation where I can imagine you
need to modify with packet data outside of the protocol headers.

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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


> And the copy is necessary because you need more tailroom or something?
> 
> I don't understand why simply having all the space you need is not
> better and faster :-)
> 
> Even if you have a clone, you can muck with the data in the IP and TCP
> header areas, so long as skb_header_cloned() is false.

I think it's probably all because I don't understand skb_cloned() vs.
skb_header_cloned() and mac80211 came with checks for skb_cloned() that
I didn't spot.

So you're saying the check there to see if we can add 802.11 headers
should depend on skb_header_cloned() and not skb_cloned()?

> So I can only conclude that your problematic case is that of
> encryption, right?  That's the only situation where I can imagine you
> need to modify with packet data outside of the protocol headers.

When is protocol header equivalent to skb header and when not?

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Mon, 05 May 2008 22:10:14 +0200

> So you're saying the check there to see if we can add 802.11 headers
> should depend on skb_header_cloned() and not skb_cloned()?

If you're pushing things in front (adding headers), you should basically
always be OK.

The problem is the skb->data pointer, that has to remain stable
once dev_queue_xmit() gets the frame because taps like AF_PACKET
can have access to the packet (via net/core/dev.c:dev_queue_xmit_nit()).

(If you read the AF_PACKET code, and notice how it munges the
 headers, note that it only does so temporarily and restores the
 skb->data and skb->len state before returning back up to
 it's caller, dev_hard_start_xmit())

I think the AF_PACKET stuff is actually superfluous on the transmit
side, we always give it clones so it can modify skb->data however it
pleases without having to restore anything.  Someone with some time
should look more closely into this :-)  The problem here seems to
be that pt->func is used for both receive and transmit paths, so it
must be mindful to handle both cases properly.

BTW, this gets back to the topic of the pain caused by pretending this
stuff is ethernet when it isn't.  If we really made these wireless
devices look the way they should, tcpdump could see the headers
correctly.  I'm sure you have all sorts of hacks in the 80211 code to
make 802.11 header capture possible via side-band stuff, but it would
have been so much nicer (and transparently solved these header space
issues) if we didn't try to put lipstick on a pig :-)

Anyways, back to the original topic.  I think you can avoid COW'ing
clones if you don't modify skb->data and give the drivers some other
way to know where the wireless headers really start.

Alternatively, you can just clone the packet.  At that point the
skb->data etc. members are your's to modify to your heart's content.
And because nobody can look below the original skb->data value, you
can stick your headers there.

--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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

On Mon, 2008-05-05 at 13:44 -0700, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Mon, 05 May 2008 22:10:14 +0200
> 
> > So you're saying the check there to see if we can add 802.11 headers
> > should depend on skb_header_cloned() and not skb_cloned()?
> 
> If you're pushing things in front (adding headers), you should basically
> always be OK.

We're not, we need to strip off the ethernet header and replace it with
802.11.

> The problem is the skb->data pointer, that has to remain stable
> once dev_queue_xmit() gets the frame because taps like AF_PACKET
> can have access to the packet (via net/core/dev.c:dev_queue_xmit_nit()).
> 
> (If you read the AF_PACKET code, and notice how it munges the
>  headers, note that it only does so temporarily and restores the
>  skb->data and skb->len state before returning back up to
>  it's caller, dev_hard_start_xmit())
> 
> I think the AF_PACKET stuff is actually superfluous on the transmit
> side, we always give it clones so it can modify skb->data however it
> pleases without having to restore anything.  Someone with some time
> should look more closely into this :-)  The problem here seems to
> be that pt->func is used for both receive and transmit paths, so it
> must be mindful to handle both cases properly.

Interesting, but I'm not really interested in looking into it right
now :)

> BTW, this gets back to the topic of the pain caused by pretending this
> stuff is ethernet when it isn't.  If we really made these wireless
> devices look the way they should, tcpdump could see the headers
> correctly.  I'm sure you have all sorts of hacks in the 80211 code to
> make 802.11 header capture possible via side-band stuff, but it would
> have been so much nicer (and transparently solved these header space
> issues) if we didn't try to put lipstick on a pig :-)

We can discuss this to no end and already have; I just don't see how
you'd want to handle the BSSID or QoS fields with things like dhclient.
As for header capture, that is actually not very interesting, the
interesting stuff is the transmit indication (was the frame ack'ed, ...)
and that's still not possible that way.

> Anyways, back to the original topic.  I think you can avoid COW'ing
> clones if you don't modify skb->data and give the drivers some other
> way to know where the wireless headers really start.
> 
> Alternatively, you can just clone the packet.  At that point the
> skb->data etc. members are your's to modify to your heart's content.
> And because nobody can look below the original skb->data value, you
> can stick your headers there.

But I cannot modify the ethernet header that'll live on in the skb data
so that's not useful. Also, if two mac80211 netdevs are bridged together
they'd stomp on each other no?

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Mon, 05 May 2008 22:57:46 +0200

> But I cannot modify the ethernet header that'll live on in the skb data
> so that's not useful. Also, if two mac80211 netdevs are bridged together
> they'd stomp on each other no?

I see, let me think about this for a while...

Thanks for the information.
--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Mon, 05 May 2008 22:57:46 +0200

> But I cannot modify the ethernet header that'll live on in the skb data
> so that's not useful.

Actually, you can, if skb_header_cloned() returns false.

When skb_header_cloned() returns false you can change the
headers however you like.

I'm not sure if it helps the bridging case or not, but in
any event, the following patch should work and reduce the
number of cases where pskb_expand_head() is necessary on
TX.

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index f35eaea..86f0e36 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1562,13 +1562,13 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
 	 * be cloned. This could happen, e.g., with Linux bridge code passing
 	 * us broadcast frames. */
 
-	if (head_need > 0 || skb_cloned(skb)) {
+	if (head_need > 0 || skb_header_cloned(skb)) {
 #if 0
 		printk(KERN_DEBUG "%s: need to reallocate buffer for %d bytes "
 		       "of headroom\n", dev->name, head_need);
 #endif
 
-		if (skb_cloned(skb))
+		if (skb_header_cloned(skb))
 			I802_DEBUG_INC(local->tx_expand_skb_head_cloned);
 		else
 			I802_DEBUG_INC(local->tx_expand_skb_head);
--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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

On Mon, 2008-05-05 at 15:37 -0700, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Mon, 05 May 2008 22:57:46 +0200
> 
> > But I cannot modify the ethernet header that'll live on in the skb data
> > so that's not useful.
> 
> Actually, you can, if skb_header_cloned() returns false.
> 
> When skb_header_cloned() returns false you can change the
> headers however you like.
> 
> I'm not sure if it helps the bridging case or not, but in
> any event, the following patch should work and reduce the
> number of cases where pskb_expand_head() is necessary on
> TX.

Right, that makes sense. But will it ever return false? skb_cloned()
returns true often enough, and we only accept linear skbs, but I think I
don't understand yet what skb_header_cloned() vs. skb_cloned() refers
to. Which exactly is the header space I'm allowed to modify when
skb_header_cloned() returns false?

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Mon, 05 May 2008 17:22:03 +0200

> 
> > I've did some measurement of the TX path  on an embedded system (2.6.23 kernel)
> > When bridging packets from an ethernet device to wireless  there is
> > loss of 12% in the CPU utilization and equivalent throughput reduction
> > in data packets that are checked and expanded in
> > ieee80211_subif_start_xmit function.
> 
> Yeah, I figured.

I think we can handle this without clones or copies.

First, the skb_header_cloned() patch I posted in another reply will
get rid of copying due to clones.  I've included it below for
completeness.

The next problem is to make sure there is enough space available.  And
all that's needed is some help from the bridging layer and some hooks
into netdev_alloc_skb().

On bridge transmit, it knows the input and output devices, and the
requirements of LL header space on the transmit side.

If the transmit requirements are not met for the received packet, we
can tag the difference into the input netdev.

Using that information we can allocate extra space in
netdev_alloc_skb(), and do an skb_reserve().

The only requirement is that the ethernet driver serving input packets
uses netdev_alloc_skb().  The most important drivers already do, and
those which do not are trivially converted.

The following along with Johannes's needed_header et al. patch should
take care of the overhead.

If this proves to be a working solution, we can do something similar
for IPv4 and IPv6 forwarding.  At that point, we should make this
"adjust input device extra space" a helper function that all of
these spots can call.

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7c1d446..6c06fba 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -600,6 +600,7 @@ struct net_device
  * Cache line mostly used on receive path (including eth_type_trans())
  */
 	unsigned long		last_rx;	/* Time of last Rx	*/
+	unsigned int		rx_alloc_extra;
 	/* Interface address info used in eth_type_trans() */
 	unsigned char		dev_addr[MAX_ADDR_LEN];	/* hw address, (before bcast 
 							because most packets are unicast) */
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index bdd7c35..d2b2272 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -42,6 +42,22 @@ int br_dev_queue_push_xmit(struct sk_buff *skb)
 		if (nf_bridge_maybe_copy_header(skb))
 			kfree_skb(skb);
 		else {
+			unsigned int headroom = skb_headroom(skb);
+			unsigned int hh_len = LL_RESERVED_SPACE(skb->dev);
+
+			if (headroom < hh_len) {
+				struct net_device *in_dev;
+				unsigned int extra;
+
+				in_dev = __dev_get_by_index(dev_net(skb->dev),
+							    skb->iif);
+				BUG_ON(!in_dev);
+
+				extra = hh_len - headroom;
+				if (extra >= in_dex->rx_alloc_extra)
+					in_dev->rx_alloc_extra = extra;
+			}
+
 			skb_push(skb, ETH_HLEN);
 
 			dev_queue_xmit(skb);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5c459f2..74a2515 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -255,11 +255,12 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
 		unsigned int length, gfp_t gfp_mask)
 {
 	int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
+	unsigned int extra = dev->rx_alloc_extra + NET_SKB_PAD;
 	struct sk_buff *skb;
 
-	skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, 0, node);
+	skb = __alloc_skb(length + extra, gfp_mask, 0, node);
 	if (likely(skb)) {
-		skb_reserve(skb, NET_SKB_PAD);
+		skb_reserve(skb, extra);
 		skb->dev = dev;
 	}
 	return skb;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index f35eaea..86f0e36 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1562,13 +1562,13 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
 	 * be cloned. This could happen, e.g., with Linux bridge code passing
 	 * us broadcast frames. */
 
-	if (head_need > 0 || skb_cloned(skb)) {
+	if (head_need > 0 || skb_header_cloned(skb)) {
 #if 0
 		printk(KERN_DEBUG "%s: need to reallocate buffer for %d bytes "
 		       "of headroom\n", dev->name, head_need);
 #endif
 
-		if (skb_cloned(skb))
+		if (skb_header_cloned(skb))
 			I802_DEBUG_INC(local->tx_expand_skb_head_cloned);
 		else
 			I802_DEBUG_INC(local->tx_expand_skb_head);
--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Tue, 06 May 2008 00:44:07 +0200

> Right, that makes sense. But will it ever return false? skb_cloned()
> returns true often enough, and we only accept linear skbs, but I think I
> don't understand yet what skb_header_cloned() vs. skb_cloned() refers
> to. Which exactly is the header space I'm allowed to modify when
> skb_header_cloned() returns false?

skb_header_cloned() will return false always if the buffer is not
cloned.

If it is cloned, it makes sure the data reference count allows
for modification of the buffer.

For normal traffic, you should see skb_header_cloned() always return
false unless a network tap like tcpdump is registered.
--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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


> First, the skb_header_cloned() patch I posted in another reply will
> get rid of copying due to clones.  I've included it below for
> completeness.
> 
> The next problem is to make sure there is enough space available.  And
> all that's needed is some help from the bridging layer and some hooks
> into netdev_alloc_skb().
> 
> On bridge transmit, it knows the input and output devices, and the
> requirements of LL header space on the transmit side.
> 
> If the transmit requirements are not met for the received packet, we
> can tag the difference into the input netdev.
> 
> Using that information we can allocate extra space in
> netdev_alloc_skb(), and do an skb_reserve().
> 
> The only requirement is that the ethernet driver serving input packets
> uses netdev_alloc_skb().  The most important drivers already do, and
> those which do not are trivially converted.
> 
> The following along with Johannes's needed_header et al. patch should
> take care of the overhead.
> 
> If this proves to be a working solution, we can do something similar
> for IPv4 and IPv6 forwarding.  At that point, we should make this
> "adjust input device extra space" a helper function that all of
> these spots can call.

This looks feasible, thanks for looking into it. But isn't, when the
bridge gives us a cloned skb, the complete header the complete data
since it won't be a fraglist skb? And then we copy it all anyway? That's
just for group addressed frames, of course, so still a lot better I
guess. The main point is getting enough headroom.

Also, should there be some sort of timer that resets the rx_alloc_extra
again so that when you bridge it once with p54 (needs heaps of headroom)
you don't suffer forever?

> @@ -255,11 +255,12 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
>  		unsigned int length, gfp_t gfp_mask)
>  {
>  	int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
> +	unsigned int extra = dev->rx_alloc_extra + NET_SKB_PAD;
>  	struct sk_buff *skb;
>  
> -	skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, 0, node);
> +	skb = __alloc_skb(length + extra, gfp_mask, 0, node);
>  	if (likely(skb)) {
> -		skb_reserve(skb, NET_SKB_PAD);
> +		skb_reserve(skb, extra);

Doesn't that break alignment though?

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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

On Mon, 2008-05-05 at 16:14 -0700, David Miller wrote:

> > Right, that makes sense. But will it ever return false? skb_cloned()
> > returns true often enough, and we only accept linear skbs, but I think I
> > don't understand yet what skb_header_cloned() vs. skb_cloned() refers
> > to. Which exactly is the header space I'm allowed to modify when
> > skb_header_cloned() returns false?
> 
> skb_header_cloned() will return false always if the buffer is not
> cloned.

Right, it's less than skb_cloned().

> If it is cloned, it makes sure the data reference count allows
> for modification of the buffer.

Which buffer? skb->data..skb->tail? And isn't that, in mac80211's case,
at least currently all the buffer anyway?

> For normal traffic, you should see skb_header_cloned() always return
> false unless a network tap like tcpdump is registered.

Interesting point. I'll have to see when socket filters are run,
wpa_supplicant could have a tap open I think. Maybe that's why I'm
seeing so many cloned packets. I think I'll stacktrace skb_clone and
print that out.

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Tue, 06 May 2008 01:17:14 +0200

> Also, should there be some sort of timer that resets the rx_alloc_extra
> again so that when you bridge it once with p54 (needs heaps of headroom)
> you don't suffer forever?

We're talking about, what, up to 128 bytes or something like that?
If you're bridging over your wireless device you've already invested
in assuming those kinds of costs.

I don't think this aspect is really worth worrying about.  The current
behavior is so much incredibly worse. :-)

> > @@ -255,11 +255,12 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
> >  		unsigned int length, gfp_t gfp_mask)
> >  {
> >  	int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
> > +	unsigned int extra = dev->rx_alloc_extra + NET_SKB_PAD;
> >  	struct sk_buff *skb;
> >  
> > -	skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, 0, node);
> > +	skb = __alloc_skb(length + extra, gfp_mask, 0, node);
> >  	if (likely(skb)) {
> > -		skb_reserve(skb, NET_SKB_PAD);
> > +		skb_reserve(skb, extra);
> 
> Doesn't that break alignment though?

Good catch, we'll have to align the rx_alloc_extra to some modulus or
similar.  Ideally, at the spot where rx_alloc_extra is set instead
of here.

--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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

On Mon, 2008-05-05 at 16:24 -0700, David Miller wrote:

> > Also, should there be some sort of timer that resets the rx_alloc_extra
> > again so that when you bridge it once with p54 (needs heaps of headroom)
> > you don't suffer forever?
> 
> We're talking about, what, up to 128 bytes or something like that?
> If you're bridging over your wireless device you've already invested
> in assuming those kinds of costs.

About that much at most, yeah. But if you then decide that you don't
want to bridge wireless after all... anyway doesn't matter, was just a
thought.

> I don't think this aspect is really worth worrying about.  The current
> behavior is so much incredibly worse. :-)

That's true.

> > > @@ -255,11 +255,12 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
> > >  		unsigned int length, gfp_t gfp_mask)
> > >  {
> > >  	int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
> > > +	unsigned int extra = dev->rx_alloc_extra + NET_SKB_PAD;
> > >  	struct sk_buff *skb;
> > >  
> > > -	skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, 0, node);
> > > +	skb = __alloc_skb(length + extra, gfp_mask, 0, node);
> > >  	if (likely(skb)) {
> > > -		skb_reserve(skb, NET_SKB_PAD);
> > > +		skb_reserve(skb, extra);
> > 
> > Doesn't that break alignment though?
> 
> Good catch, we'll have to align the rx_alloc_extra to some modulus or
> similar.  Ideally, at the spot where rx_alloc_extra is set instead
> of here.

Yeah, it just has to be a multiple of four I think.

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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

On Mon, 2008-05-05 at 16:03 -0700, David Miller wrote:

> @@ -255,11 +255,12 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
>  		unsigned int length, gfp_t gfp_mask)
>  {
>  	int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
> +	unsigned int extra = dev->rx_alloc_extra + NET_SKB_PAD;
>  	struct sk_buff *skb;
>  
> -	skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, 0, node);
> +	skb = __alloc_skb(length + extra, gfp_mask, 0, node);
>  	if (likely(skb)) {
> -		skb_reserve(skb, NET_SKB_PAD);
> +		skb_reserve(skb, extra);

Come to think of it, if you want to bridge two wireless AP-mode devices
with different headroom .... Nah.

four-byte multiple isn't quite right though, but I'm not sure what would
be, probably multiple of NET_SKB_PAD?

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Tue, 06 May 2008 01:23:55 +0200

[ Herbert CC:'d ]

> On Mon, 2008-05-05 at 16:14 -0700, David Miller wrote:
> 
> > If it is cloned, it makes sure the data reference count allows
> > for modification of the buffer.
> 
> Which buffer? skb->data..skb->tail? And isn't that, in mac80211's case,
> at least currently all the buffer anyway?

Right.

For TSO or cases using frag lists of frag page vectors, it gets
more complicated.

Basically, skb_header_cloned()==false means that you can modify
anything covered by skb->data..skb->tail

> Interesting point. I'll have to see when socket filters are run,
> wpa_supplicant could have a tap open I think. Maybe that's why I'm
> seeing so many cloned packets. I think I'll stacktrace skb_clone and
> print that out.

Note that we can even optimize that case even further if we really
have to.

But try to see why the filter used by WPA Supplicant is not effective.
If the filter fails to match, the packet is free'd immediately using
kfree_skb() which should undo clone'age.

Actually, I think I see a potential problem with how the
->dataref bit fields are managed in this situation.  This is
Herbert's creation, so let's ask him :-)

It seems that if we have a TCP packet for which skb_header_cloned()
is false, and this enters dev_hard_start_xmit(), if any network
taps take the packet, we'll never return to skb_header_cloned()==false
state even if every tap immediately kfree_skb(skb)'s the packet.

When we clone, we'll increment only the low 16-bit part of ->dataref.

On free, since skb->nohdr is set, we'll decrement both of (1 <<
SKB_DATAREF_SHIFT) and the low 16-bit counter.

Therefore the packet never becomes re-writable, right?  Is there some
easy way to cure this Herbert?  Apparently having a tap registered
constantly merely to watch for filtered traffic is quite common. :-/

--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Tue, 06 May 2008 01:36:31 +0200

> four-byte multiple isn't quite right though, but I'm not sure what would
> be, probably multiple of NET_SKB_PAD?

Yes, NET_SKB_PAD is what I was going to use.
--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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


> > Which buffer? skb->data..skb->tail? And isn't that, in mac80211's case,
> > at least currently all the buffer anyway?
> 
> Right.
> 
> For TSO or cases using frag lists of frag page vectors, it gets
> more complicated.

Yeah, but at least right now we don't accept any of those. That might
change, but even then we'll only care about the headroom (and maybe for
TKIP the crypto MIC that's at the end, assuming the hardware does the
encryption which it usually does), but we can stick that elsewhere and
just DMA it on.

> Basically, skb_header_cloned()==false means that you can modify
> anything covered by skb->data..skb->tail

Ok.

> > Interesting point. I'll have to see when socket filters are run,
> > wpa_supplicant could have a tap open I think. Maybe that's why I'm
> > seeing so many cloned packets. I think I'll stacktrace skb_clone and
> > print that out.
> 
> Note that we can even optimize that case even further if we really
> have to.
> 
> But try to see why the filter used by WPA Supplicant is not effective.
> If the filter fails to match, the packet is free'd immediately using
> kfree_skb() which should undo clone'age.

Right. I just checked, and realised I remembered wrongly, wpa_supplicant
actually binds the socket to the ethertype it wants so
dev_queue_xmit_nit() shouldn't be called. I'll have to check why I got
so many cloned packets, best see if it still happens with the
header_cloned() change.

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Tue, 06 May 2008 02:01:16 +0200

> Right. I just checked, and realised I remembered wrongly, wpa_supplicant
> actually binds the socket to the ethertype it wants so
> dev_queue_xmit_nit() shouldn't be called. I'll have to check why I got
> so many cloned packets, best see if it still happens with the
> header_cloned() change.

skb_cloned() is fine, that's what every TCP packet is.

skb_header_cloned() is what we expect to be false

I look forward to the results of your investigation :-)
--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Herbert Xu]

On Mon, May 05, 2008 at 04:39:16PM -0700, David Miller wrote:
> 
> On free, since skb->nohdr is set, we'll decrement both of (1 <<
> SKB_DATAREF_SHIFT) and the low 16-bit counter.

skb->nohdr is cleared for clones.  Only the copy held by TCP
should have it set.  So this should work correctly, in theory :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
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 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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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

On Mon, 2008-05-05 at 17:08 -0700, David Miller wrote:
> From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> Date: Tue, 06 May 2008 02:01:16 +0200
> 
> > Right. I just checked, and realised I remembered wrongly, wpa_supplicant
> > actually binds the socket to the ethertype it wants so
> > dev_queue_xmit_nit() shouldn't be called. I'll have to check why I got
> > so many cloned packets, best see if it still happens with the
> > header_cloned() change.
> 
> skb_cloned() is fine, that's what every TCP packet is.
> 
> skb_header_cloned() is what we expect to be false
> 
> I look forward to the results of your investigation :-)

This is indeed the case. With that patch of yours (plus changes to make
it print out things) I occasionally see header-cloned SKBs [1] and due
to my spacing patch haven't yet seen one that didn't have enough
headroom. So this will optimise TCP a lot. I haven't tried bridging yet
as AP mode is rather unstable at the moment.

johannes

[1] those coming from ipv6, udp (possibly zeroconf stuff):

[34805.481404] MAC80211: got header cloned skb
[34805.481422]  [<c0272f78>] __copy_skb_header+0x1bc/0x20c
[34805.481449]  [<c0273084>] __skb_clone+0x2c/0xf4
[34805.481459]  [<c027b210>] dev_hard_start_xmit+0xf0/0x2f0
[34805.481473]  [<c028ed2c>] __qdisc_run+0x248/0x27c
[34805.481488]  [<c027baa0>] dev_queue_xmit+0x300/0x3cc
[34805.481498]  [<c02f0180>] ip6_output_finish+0xac/0x12c
[34805.481515]  [<c02f2ffc>] ip6_output+0x56c/0xe64
[34805.481524]  [<c02f1a78>] ip6_local_out+0x34/0x48
[34805.481533]  [<c02f1e50>] ip6_push_pending_frames+0x3c4/0x524
[34805.481542]  [<c0306ed8>] udp_v6_push_pending_frames+0x1e0/0x340
[34805.481559]  [<c03077e4>] udpv6_sendmsg+0x744/0xa24
[34805.481569]  [<c02cdad8>] inet_sendmsg+0x4c/0x78
[34805.481580]  [<c026bcc4>] sock_sendmsg+0xac/0xe4
[34805.481589]  [<c026bee0>] sys_sendmsg+0x1e4/0x2a0
[34805.481599]  [<c026cb48>] sys_socketcall+0xcc/0x1dc
[34805.481608]  [<c00124cc>] ret_from_syscall+0x0/0x38

[34789.711392] MAC80211: got header cloned skb
[34789.711411]  [<c0272f78>] __copy_skb_header+0x1bc/0x20c
[34789.711438]  [<c0273084>] __skb_clone+0x2c/0xf4
[34789.711448]  [<c027b210>] dev_hard_start_xmit+0xf0/0x2f0
[34789.711462]  [<c028ed2c>] __qdisc_run+0x248/0x27c
[34789.711478]  [<c027baa0>] dev_queue_xmit+0x300/0x3cc
[34789.711487]  [<c02a5c8c>] ip_finish_output+0x1e8/0x354
[34789.711504]  [<c02a61e8>] ip_local_out+0x34/0x48
[34789.711514]  [<c02a64a0>] ip_push_pending_frames+0x2a4/0x3d8
[34789.711525]  [<c02c52bc>] udp_push_pending_frames+0x128/0x39c
[34789.711543]  [<c02c6b7c>] udp_sendmsg+0x348/0x660
[34789.711553]  [<c02cdad8>] inet_sendmsg+0x4c/0x78
[34789.711564]  [<c026bcc4>] sock_sendmsg+0xac/0xe4
[34789.711574]  [<c026bee0>] sys_sendmsg+0x1e4/0x2a0
[34789.711583]  [<c026cb48>] sys_socketcall+0xcc/0x1dc
[34789.711593]  [<c00124cc>] ret_from_syscall+0x0/0x38


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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Sun, 04 May 2008 23:32:25 +0200

> This assigns the netdev's needed_headroom/tailroom members
> to take advantage of pre-allocated space for 802.11 headers.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Now that I've applied the prerequisite patch, I've added this one
too and will do a lot of build testing just to make sure there
aren't any surprises.

Thanks!

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 05 May 2008 15:37:30 -0700 (PDT)

Just a heads up that I'm adding this to my tree as well.

> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index f35eaea..86f0e36 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1562,13 +1562,13 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
>  	 * be cloned. This could happen, e.g., with Linux bridge code passing
>  	 * us broadcast frames. */
>  
> -	if (head_need > 0 || skb_cloned(skb)) {
> +	if (head_need > 0 || skb_header_cloned(skb)) {
>  #if 0
>  		printk(KERN_DEBUG "%s: need to reallocate buffer for %d bytes "
>  		       "of headroom\n", dev->name, head_need);
>  #endif
>  
> -		if (skb_cloned(skb))
> +		if (skb_header_cloned(skb))
>  			I802_DEBUG_INC(local->tx_expand_skb_head_cloned);
>  		else
>  			I802_DEBUG_INC(local->tx_expand_skb_head);

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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

On Mon, 2008-05-12 at 20:52 -0700, David Miller wrote:
> From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> Date: Sun, 04 May 2008 23:32:25 +0200
> 
> > This assigns the netdev's needed_headroom/tailroom members
> > to take advantage of pre-allocated space for 802.11 headers.
> > 
> > Signed-off-by: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> 
> Now that I've applied the prerequisite patch, I've added this one
> too and will do a lot of build testing just to make sure there
> aren't any surprises.

Thanks! Will you put in something like the patch that made dev_alloc_skb
reserve more with the bridging headroom as well, or do you want (me) to
look at routing first?

johannes

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

Re: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Tue, 13 May 2008 11:01:13 +0200

> Will you put in something like the patch that made dev_alloc_skb
> reserve more with the bridging headroom as well, or do you want (me)
> to look at routing first?

I'm trying to do this one set of changes at a time. :-)

I'll get to the netdev_alloc_skb() bits after this stuff
has had some time to cook.
--
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: [RFC v2] mac80211: assign needed_headroom/tailroom for netdevs

[Johannes Berg]

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

On Tue, 2008-05-13 at 02:45 -0700, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Tue, 13 May 2008 11:01:13 +0200
> 
> > Will you put in something like the patch that made dev_alloc_skb
> > reserve more with the bridging headroom as well, or do you want (me)
> > to look at routing first?
> 
> I'm trying to do this one set of changes at a time. :-)
> 
> I'll get to the netdev_alloc_skb() bits after this stuff
> has had some time to cook.

Sure, sounds good, thanks.

johannes

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