This patch makes mac80211 use the GSO infrastructure for segmentation,

but not really all of it because we do not register a protocol handler

(nor can do that easily since a lot of functions need to be done before

segmentation and another bunch afterwards, and we need to keep rcu-

protected structures for both.)
Hence, the way it works is that if a packet is to be fragmented then

we call dev_skb_segment() on it and get a fragment list in skb->next.

We then go to pass this fragment list to the driver one by one but if

at any point the driver is unable to handle more data on its queues

we reject the skb (with the remaining fragments) which will cause it

to be stored into dev->gso_skb and handed to us (once the queue is

woken again) fragment by fragment which we detect and hand on to the

driver.
This has the benefit of removing a lot of code that is rarely used

(since fragmentation in IEEE 802.11 isn't all that common any more)

and is rather complex.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Cc: Herbert Xu <herbert@gondor.apana.org.au>

---

Herbert, would you mind taking a look at ieee80211_tx_h_fragment and

__ieee80211_tx?
The diffstat is negative despite adding lots of comments :)
 include/net/mac80211.h     |   11

 net/mac80211/ieee80211_i.h |   19 -

 net/mac80211/main.c        |   33 +-

 net/mac80211/mlme.c        |    1

 net/mac80211/tx.c          |  589 +++++++++++++++++++++------------------------

 net/mac80211/util.c        |   30 --

 net/mac80211/wep.c         |   28 +-

 net/mac80211/wme.c         |    9

 net/mac80211/wpa.c         |   46 ++-

 9 files changed, 370 insertions(+), 396 deletions(-)
--- everything.orig/net/mac80211/ieee80211_i.h	2008-04-30 03:44:52.000000000 +0200

+++ everything/net/mac80211/ieee80211_i.h	2008-04-30 13:37:40.000000000 +0200

@@ -147,7 +147,6 @@ typedef unsigned __bitwise__ ieee80211_t

 #define IEEE80211_TX_UNICAST		BIT(1)

 #define IEEE80211_TX_PS_BUFFERED	BIT(2)

 #define IEEE80211_TX_PROBE_LAST_F...
[ message continues ]

Your idea of using skb_segment to remove duplicate code is great.
However, using skb->gso_skb for the push-back doesn't work.  For

example, what is going to happen when I enable software GSO on a

wireless device and then send a real GSO packet to it where each

GSO fragment also required wireless fragmentation?
I suggest that you just use skb_segment for the actual fragmentation

but keep the original infrastructure for handling the fragments.
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

--

Good point. Somehow I thought this couldn't happen but of course it can
But I want to get rid of that much more than I want to get rid of the

fragmentation code :/ Also, if I need to keep that code, I will

absolutely not use skb_segment either as it's more efficient to just

trim the existing skb to make the first segment out of it (like the code

does now) rather than reallocate that too.
However, I'm sure it's buggy under certain circumstances we never

understood (as indicated by a WARN_ON triggering that shouldn't and lots

of reports about that) and I don't like the extra tasklet. I guess I

could remove it and copy the existing code into the wireless qdisc, but

that would be weird.
But you're right, it doesn't work this way. I'll have to think of

something. Maybe I can rip out all the retry logic and just store an skb

myself (like dev->gso_skb) and return NETDEV_TX_BUSY when I used that

one and the queue got full because of it. That'll requeue a lot when

non-gso skbs are coming in, but mind you this is not a common case, most

of the time fragmentation will not be enabled and even when it is device

queues should be long enough.
johannes

Yes keeping your own gso_skb sounds like a good solution.
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

--

This patch rewrites mac80211's fragmentation handling to

 (a) use skb_segment

 (b) get rid of the tasklet etc. and just push responsibility

     to the qdisc/core code
This can result in excessive requeues when the device only

accepts a fragment at a time or so then we'll always ask the

generic code to try again but then won't accept the skb.

However, devices that only accept a single skb at a time won't

get good performance anyhow so it doesn't matter.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---

John, this has no more dependencies on other patches except ones I

submitted earlier, please merge unless somebody objects.
 include/net/mac80211.h     |   10

 net/mac80211/ieee80211_i.h |   32 --

 net/mac80211/main.c        |   46 ++-

 net/mac80211/tx.c          |  629 ++++++++++++++++++++++-----------------------

 net/mac80211/util.c        |   30 --

 net/mac80211/wep.c         |   26 -

 net/mac80211/wme.c         |    9

 net/mac80211/wpa.c         |   44 +--

 8 files changed, 395 insertions(+), 431 deletions(-)
--- everything.orig/net/mac80211/ieee80211_i.h	2008-05-07 11:13:00.000000000 +0200

+++ everything/net/mac80211/ieee80211_i.h	2008-05-07 11:49:11.000000000 +0200

@@ -147,7 +147,6 @@ typedef unsigned __bitwise__ ieee80211_t

 #define IEEE80211_TX_UNICAST		BIT(1)

 #define IEEE80211_TX_PS_BUFFERED	BIT(2)

 #define IEEE80211_TX_PROBE_LAST_FRAG	BIT(3)

-#define IEEE80211_TX_INJECTED		BIT(4)
 struct ieee80211_tx_data {

 	struct sk_buff *skb;

@@ -165,11 +164,6 @@ struct ieee80211_tx_data {

 	 * when using CTS protection with IEEE 802.11g. */

 	s8 last_frag_rate_idx;
-	/* Extra fragments (in addition to the first fragment

-	 * in skb) */

-	struct sk_buff **extra_frag;

-	int num_extra_frag;

-

 	u16 fc, ethertype;

 	unsigned int flags;

 };

@@ -215,19 +209,10 @@ struct ieee80211_rx_data {

 #define IEEE80211_TXPD_AMPDU		BIT(4)

 /* Stored in sk_buff->cb */

 struct ieee80211_tx_packet_data {

+	u32 flags;

 	int ifindex;

+	u16 queue;

 	unsig...
[ message continues ]

Returning busy should be avoided because not everything expects it.

For example, this can cause tcpdump to see the same packet twice.
Besides, I'm not sure if this will even work if you're fiddling

with skb->next.  Perhaps you can stash it in a pointer local to

your device.
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

--

Hmm. Why does busy exist then? Historical accident? I really really

don't want to keep all that logic that does "if device enables queue

then first check if maybe we have pending packets and try to send them
You're looking at an internal helper function that is never called with

skb->next assigned; if it returns BUSY for a fragmented skb then later

in __ieee80211_tx() I will store the fragmented frame away and return

OK.
But if I really need to avoid returning busy I need to hook into all the

queue management stuff which I'd like to avoid.
johannes

Yes it's historical baggage.  These days the biggest users would

be LLTX drivers which should all be converted to non-LLTX and

drivers that don't keep track of queue status properly.
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

--

Hmmm mmmm. I'll clean it up later, it gets easier with my patch that

puts the control info into skb->cb so I'll do that later in a separate

patch. The way I see it, this is actually *correct* from a "what hits

the air" point of view and the current code is crappy.
johannes

So there's no way to actually fail in a TX handler? Drivers

are doomed to drop the packet, if they cannot handle it due to

ring overflow?
--

Greetings Michael.

--

You're supposed to stop the queue before the ring overflows.
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

--

Ok, what about DMA error. kmalloc error or something else?

ring overflow was a bad example, of course.
--

Greetings Michael.

--

If your NIC gets DMA errors all the time then I suggest you

invest in some new hardware.  Ditto if you fail to kmalloc all

the time.
If they're rare as they should be then dropping the packet is

perfectly reasonable.
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

--

Ok, why is nobody actually answering my _question_? :)

So I'll try to answer it by myself: the hard_start_xmit callback

is actually supposed to return "void", but it returns "int" just

for historical reasons. So there is no way to fail in a TX handler.

If a failure happens, the driver must free and drop the skb.

How likely errors are is a completely different story. One could argue

that errors are unlikely for any part of the kernel, except for very

sone special cases.
So does that also apply to the wireless stack? Should we change drivers

to always return 0. later we can change the function prototype to return void.
--

Greetings Michael.

--

Pretty much.  When lockless drivers were the vogue the return

value was useful but as we've since found better ways to scale

NICs in an SMP environment (first the tg3 model, and then multi-

queue TX) that don't have the problems associated with requeueing

the packet, any non-zero return value should only be used by

existing code.
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

--

"..., except for some very special cases."
--

Greetings Michael.

--

From: Herbert Xu <herbert@gondor.apana.org.au>
Right, and this is why drivers choose a TX wakeup threshold such

that they can accept an arbitrarily sized TSO frame.
For example, from drivers/net/tg3.c's ->hard_start_xmit():
	if (unlikely(tg3_tx_avail(tp) <= (MAX_SKB_FRAGS + 1))) {

		netif_stop_queue(dev);

		if (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH(tp))

			netif_wake_queue(tp->dev);

	}
The driver is responsible for stopping the queue _before_ it

enters a state where there is not enough space in the queue

to accept a packet.
This is why most drivers make the following kind of BUG check

at the start of their ->hard_start_xmit()
	if (unlikely(tg3_tx_avail(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {

		if (!netif_queue_stopped(dev)) {

			netif_stop_queue(dev);
			/* This is a hard error, log it. */

			printk(KERN_ERR PFX "%s: BUG! Tx Ring full when "

			       "queue awake!\n", dev->name);

		}

		return NETDEV_TX_BUSY;

	}

--

Dave, please allow me to ask a heretical question.  Returning TX_BUSY has some

appeal for virtio_net: is it fundamentally a flawed idea, or simply a matter

of coding?
Currently we have no virtio interface to ask how many descriptors are left;

it's not clear that it's a fair question to ask, since for Xen it's depends

on the actual buffers we're trying to put in the descirptors.
Thanks,

Rusty.

--

From: Rusty Russell <rusty@rustcorp.com.au>
Allowing TX_BUSY adds a special case to the caller which we'd
Two things:
1) You can always make sure that you have enough space for a

   TSO frame, with arbitrary page boundaries and thus buffer

   chopping.
   It can even be estimated, and if violated by some corner case

   you can punt and drop.
2) You can queue inside of the driver one packet when you hit

   the limits unexpectedly, netif_stop_queue(), and return

   success.  Spit this packet out right before waking the

   queue again.
Really, there are no hard reasons to ever return TX_BUSY,

it's always a bug.
In fact, I want to move things more and more towards the driver

queueing TX packets internally instead of the networking mid-layer.
That will ahve benefits for things like TX multiqueue, we won't

need any locking at all, nor have any knowledge about multiple

queues at all, if the driver takes care of providing the buffer

between what the kernel gives it and what the device can handle

at the moment.

--

Yes, this is what we'd have to do.  Wasting room in the ring feels wrong 
I put a patch in to do exactly that at Herbert's prompting, for 2.6.26, but

it's buggy in (at least) two ways.  I have a fix for this, which adds a new

tasklet to xmit the packet.  There's still some subtle race, however, since

I'm still seeing a stuck packet.  I'll have to revert to TX_BUSY for 2.6.26

if I can't find it (unlikely).
But it's *simple*, and seems like a common thing to want.  Why not change 
That would be great: then I could shove the packet back on the queue myself

and not have to ask you about it.  It's adding a *second* queue inside the

driver which feels terribly ugly...
Cheers,

Rusty.

--

From: Rusty Russell <rusty@rustcorp.com.au>
My description describes how I want the mid-layer queue to disappear

entirely.  Queueing would be done by the driver only.
Only the driver knows all of these details about how much space there

is, what packet chopping has to take place and what effects that has

on queue space, multiqueue configurations, how one wants to hash to

multiple queues, how to lock this stuff the most efficiently.
The kernel should just get out of the way and let the driver take care

of everything.
Sure, we can have some standard helpers to deal with the most common

cases.  But anything non-trivial right now is painful because the

mid-layer model tries to be too helpful when it should just get out

of the way.

--

Ok, the more I ponder this, the more I like it.  It has a very nice side

benefit for virtio_net: we can xmit a whole bunch of packets before notifying

the host.  Real NICs might gain similarly.
The bit I can't see is what to do about qdisc if the driver manages its own

queue(s).  Leave the qdisc as currently in place and have the driver call

dev_dequeue_skb() (or some wrapper) directly?  Modulo locking issues, that

should be a fairly simple change.
Thanks,

Rusty.

--

From: Rusty Russell <rusty@rustcorp.com.au>
I'd like to approach a state where the device is just a black hole

that the qdisc injects packets into.  At least theoretically, that's

what the network is once the packet leaves the device anyways.  Nobody

really notices as long as flows don't get reordered.
I realize it isn't feasible to retain many of the qualities that some

qdiscs want (rates, qfull handling, etc.), so we'll have to provide

some handling for that, ideally in some cheap slowpath test.
But for things like tx queue backlog overflow the behavior would

be essentially the same.  The only change is that the txqueuelen

parameter is handled inside of the driver (again, perhaps via

helpers).
Our TX path is way too complicated and, frankly, restrictive.
This is why we don't have real parallel TX multiqueue support as a

simple patch to some drivers.  We have fundamental restrictions that

keep that from happening.

--

I think this sounds quite reasonable.
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

--

Incidentally, mac80211 has a tasklet for this as well, and I'm pretty

sure it's also buggy in the corner cases (rather than inefficient or

something)
johannes

I don't think returning TX_BUSY is fundamentally flawed (unlike

LLTX which is).  However, until somebody (i.e., you :) writes

the code to handle it properly, i.e., not making the requeued

packet go through AF_PCAKET twice, it's something that shouldn't

be encouraged.
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

--

Yes yes yes I know :) It's just quite awkward to handle when your

hard_start_xmit() actually increases the number of packets, you don't

really know how deep the queue is etc. I'll add an assertion like that

to mac80211 for the non-fragmented case and clean up the fragmented case

to retry by itself without upper layer involvement after the tx->cb

patch.
johannes