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>

---

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;
--

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>

---

 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;
--

From: 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!

--

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

From: Johannes Berg <johannes@sipsolutions.net>
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

From: Johannes Berg <johannes@sipsolutions.net>
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).

--

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
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
That would be nice, but I don't think it's possible.
johannes

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

--

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

From: Johannes Berg <johannes@sipsolutions.net>
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 s...
[ message continues ]

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

From: Johannes Berg <johannes@sipsolutions.net>
Yes, NET_SKB_PAD is what I was going to use.

--

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)
Doesn't that break alignment though?
johannes

From: Johannes Berg <johannes@sipsolutions.net>
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
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.
--

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
Yeah, it just has to be a multiple of four I think.
johannes

Do you support TCP checksumming in hardware? -> no SG
NETIF_F_FRAGLIST I haven't understood, but it seems to apply mostly for
which header? The SKB header (skb->data .. skb->tail
I don't think so, but I'm not sure.
johannes

From: Johannes Berg <johannes@sipsolutions.net>
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

From: Johannes Berg <johannes@sipsolutions.net>
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

From: Johannes Berg <johannes@sipsolutions.net>
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.

--

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
When is protocol header equivalent to skb header and when not?
johannes

From: Johannes Berg <johannes@sipsolutions.net>
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.
--

We're not, we need to strip off the ethernet header and replace it with
Interesting, but I'm not really interested in looking into it right
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, ...)
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

From: Johannes Berg <johannes@sipsolutions.net>
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);

--

From: David Miller <davem@davemloft.net>

Date: Mon, 05 May 2008 15:37:30 -0700 (PDT)
--

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

From: Johannes Berg <johannes@sipsolutions.net>
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.

--

Which buffer? skb->data..skb->tail? And isn't that, in mac80211's case,
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

From: Johannes Berg <johannes@sipsolutions.net>

Date: Tue, 06 May 2008 01:23:55 +0200
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
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. :-/
--

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@gondor.apana.org.au>

Home Page: http://gondor.apana.org.au/~herbert/

PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

--

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
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

From: Johannes Berg <johannes@sipsolutions.net>
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]  [<c02...
[ message continues ]

From: Johannes Berg <johannes@sipsolutions.net>
I see, let me think about this for a while...
Thanks for the information.

--

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