Re: [RFT] net80211 TX serialisation, take #4
Here we go: http://people.freebsd.org/~adrian/ath/20130225-net80211-txlock.diff See what you think. Adrian ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [RFT] net80211 TX serialisation, take #4
On Mon, Feb 25, 2013 at 12:27 AM, Adrian Chadd adr...@freebsd.org wrote:
The trouble with using a queue is that things will dequeue frames out
of order, bceause multiple dequeue contexts (ie, each call to
driver_start / driver_transmit) will execute in-parallel.
Actually, to prevent that, there is an extra queue.
To begin
driver_queue:packet1--packet2--3--4--...
working_queue:empty
hadware_queue:empty
After multiple thread dequeue
lock dequeue enqueue unlock
lock dequeue enqueue unlock
They look like,
driver_queue:2--3--4--...
working_queue:1--2 (the lock preserves the order)
hardware_queue: empty
If a thread has packet #1 finished first, there is no problem.
If a thread has packet #2 finished first, it will try to dequeue the
packet #1 (head of the queue) from the working_queue. But the packet
isn't marked as completed, so it will just return.
Queue still look like
driver_queue:2--3--4--...
(of course, other threads are processing remaining packets,
but to simplify, no change in driver_queue.)
working_queue: 1--2
hardware: empty
Later, once the thread with #1 finished processing, it will
lock while (completed) {dequeue enqueue} unlock
At the end, queue look like,
driver_queue:2--3--4--...
working_queue: empty
hardware_queue: 1--2 (the lock preserves the order)
Just wanted to clarify. I will soon test the latest txlock patch.
AK
___
freebsd-wireless@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-wireless
To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [RFT] net80211 TX serialisation, take #4
On Sun, Feb 24, 2013 at 12:45 AM, Adrian Chadd adr...@freebsd.org wrote:
On 23 February 2013 22:30, PseudoCylon moonlightak...@yahoo.ca wrote:
So, this is all pretty terrible. The only sane solution for now is to
make my VAP TX lock an IC TX lock,and grab said IC TX lock for all
VAPs. That way the driver can grab the IC TX lock when it's doing
deferred sends and it'll be sure the lock is held when it decides to
grab/increment sequence numbers from ni-ni_txseqs[].
I don't think
lock();
ni-ni_txseqs[]++;
unlock();
can fix the problem because no one can guarantee which process/thread
grabs the lock next.
Yup, that's definitely a problem.
The problem here is that packets coming into net80211 - either via
vap-input() (bpf injection) or vap-transmit() (802.3) , have to do
this:
There isn't any sequence relations between bpf injection and packets
from vap-transmit(). First come first enqueue should work.
80211 stack generated packets --\
bpf_injection - ieee80211_output()--\
vap_transmit() - vap_queue - 80211stack -- driver_queue
* processed in order;
* the same order as they're then pushed into the power save / age queue;
* the same order that these frames get assigned sequence numbers and
enforce state (eg considering things for ampdu, power save, etc);
* the same order that they're queued to the driver;
* the same order that they're dequeued from the driver;
* the same order that they're processed (eg crypto encaps);
* the same order that it's then passed down into the driver - either
via direct dispatch or deferred queue.
* .. and do this without tearing my hair out.
The sequence will be messed up during moving packets from one queue to
another, i.e from driver queue to hardware queue. As long as packets
are in a queue (in a linked list) sequence won't change unless we
explicitly write such code. So...
Saving your hair option 1
tx()
{
for() {
lock();
dequeue(m);/* assuming queue is in order */
ni_txseqs[]++
enqueue_working_queue(m);
unlock();
...
process m
...
lock();
/*
* m may change here.
* Whichever the thread does dequeues, m0 will be
* the head of the queue, so sequence will be kept intact.
* But, need to test if processing of m0 has been completed.
*/
dequeue_working_queue(m0);
enqueue_driver_queue(m0); /* or hardware_queue() */
unlock();
}
}
This will keep sequence intact.
Right. This is similar to my idea (or two.)
There's a few other issues though!
ic_raw_xmit() is called by a bunch of places to generate both raw
frames and management frames. This bypasses the vap queue and calls
the driver direct. As an example, injected EAPOL frames have CCMP IV
numbers (as they're encrypted!) but not necessarily sequence numbers.
Because the CCMP IV gets calculated int he driver at some point after
it's been queued, ANY slight race here causes some other frame to get
queued with a CCMP IV -after- the EAPOL frame, and it will get
dropped. Then you get your session dropped. :-)
That's a tricky one. (I didn't think about encryption.) Queue only
maintains the order of packets. It doesn't mean packets are processed
in order. If packets need to be processed in order, should be done
while the lock is held. I think this should do the trick.
lock();
dequeue_working_queue();/* or driver_queue */
if (IEEE80211_FC1_WEP)
ieee80211_crypto_encap();
pass_to_hardware();
unlock();
ic_raw_xmit() is also an ic method, not a vap method. Yes, it bypasses
the vap queue.
the same as bpf injection part
There's deferred transmission going on (eg ath_start() getting called
from TX completion, as an example.) Should that be called under the
above lock() in your example?
Yes. With encryption stuff, if_start or if_transmission will look like this.
if_start/_transmit()
{
for () {
lock();
dequeue_driver_queue();
enqueue_working_queue();
unlock();
...
process
...
lock();
dequeue_working_queue();
if (IEEE80211_FC1_WEP)
ieee80211_crypto_encap();
pass_to_hardware();
unlock();
}
}
And the TX sequence number stuff in iwn and ath, because the
aggregation code reuses the ni_txseqs[] array. So yes, the locking
wouldn't clash with the rest of net80211 (as only either the non-agg
TX path in net80211 or the TX path in ath/iwn would be using that
variable) but it's still a pain in the ass.
If we do a good job on serialization -- matching actual order of
packets sent out and
Re: [RFT] net80211 TX serialisation, take #4
--
Message: 12
Date: Fri, 22 Feb 2013 17:18:44 -0800
From: Adrian Chadd adr...@freebsd.org
To: freebsd-wireless@freebsd.org
Subject: Re: [RFT] net80211 TX serialisation, take #4
Message-ID:
CAJ-Vmom8HNLNc-=CogiF1v2pJHcn73rB0w9EOHoBtTjAp=j...@mail.gmail.com
Content-Type: text/plain; charset=ISO-8859-1
On 22 February 2013 15:25, Adrian Chadd adr...@freebsd.org wrote:
So, this is all pretty terrible. The only sane solution for now is to
make my VAP TX lock an IC TX lock,and grab said IC TX lock for all
VAPs. That way the driver can grab the IC TX lock when it's doing
deferred sends and it'll be sure the lock is held when it decides to
grab/increment sequence numbers from ni-ni_txseqs[].
I don't think
lock();
ni-ni_txseqs[]++;
unlock();
can fix the problem because no one can guarantee which process/thread
grabs the lock next.
i.e. concurrently, without lock,
- Thread A is processing packet seq#1
- Thread B is processing packet seq#2
When the time to do ni_txseqs[]++, how can we guarantee thread A grabs
the lock before thread B? In other word, if thread A always grabs the
lock first, we don't need to serialize the Tx, do we?
* .. and do this without tearing my hair out.
The sequence will be messed up during moving packets from one queue to
another, i.e from driver queue to hardware queue. As long as packets
are in a queue (in a linked list) sequence won't change unless we
explicitly write such code. So...
Saving your hair option 1
tx()
{
for() {
lock();
dequeue(m);/* assuming queue is in order */
ni_txseqs[]++
enqueue_working_queue(m);
unlock();
...
process m
...
lock();
/*
* m may change here.
* Whichever the thread does dequeues, m0 will be
* the head of the queue, so sequence will be kept intact.
* But, need to test if processing of m0 has been completed.
*/
dequeue_working_queue(m0);
enqueue_driver_queue(m0); /* or hardware_queue() */
unlock();
}
}
This will keep sequence intact.
Saving your hair option 2
Shave your head until you fix the problem. Hair will grow again, you
know. No bad hair day is a bonus.
AK
___
freebsd-wireless@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-wireless
To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [RFT] net80211 TX serialisation, take #4
On 23 February 2013 22:30, PseudoCylon moonlightak...@yahoo.ca wrote:
So, this is all pretty terrible. The only sane solution for now is to
make my VAP TX lock an IC TX lock,and grab said IC TX lock for all
VAPs. That way the driver can grab the IC TX lock when it's doing
deferred sends and it'll be sure the lock is held when it decides to
grab/increment sequence numbers from ni-ni_txseqs[].
I don't think
lock();
ni-ni_txseqs[]++;
unlock();
can fix the problem because no one can guarantee which process/thread
grabs the lock next.
Yup, that's definitely a problem.
The problem here is that packets coming into net80211 - either via
vap-input() (bpf injection) or vap-transmit() (802.3) , have to do
this:
* processed in order;
* the same order as they're then pushed into the power save / age queue;
* the same order that these frames get assigned sequence numbers and
enforce state (eg considering things for ampdu, power save, etc);
* the same order that they're queued to the driver;
* the same order that they're dequeued from the driver;
* the same order that they're processed (eg crypto encaps);
* the same order that it's then passed down into the driver - either
via direct dispatch or deferred queue.
* .. and do this without tearing my hair out.
The sequence will be messed up during moving packets from one queue to
another, i.e from driver queue to hardware queue. As long as packets
are in a queue (in a linked list) sequence won't change unless we
explicitly write such code. So...
Saving your hair option 1
tx()
{
for() {
lock();
dequeue(m);/* assuming queue is in order */
ni_txseqs[]++
enqueue_working_queue(m);
unlock();
...
process m
...
lock();
/*
* m may change here.
* Whichever the thread does dequeues, m0 will be
* the head of the queue, so sequence will be kept intact.
* But, need to test if processing of m0 has been completed.
*/
dequeue_working_queue(m0);
enqueue_driver_queue(m0); /* or hardware_queue() */
unlock();
}
}
This will keep sequence intact.
Right. This is similar to my idea (or two.)
There's a few other issues though!
ic_raw_xmit() is called by a bunch of places to generate both raw
frames and management frames. This bypasses the vap queue and calls
the driver direct. As an example, injected EAPOL frames have CCMP IV
numbers (as they're encrypted!) but not necessarily sequence numbers.
Because the CCMP IV gets calculated int he driver at some point after
it's been queued, ANY slight race here causes some other frame to get
queued with a CCMP IV -after- the EAPOL frame, and it will get
dropped. Then you get your session dropped. :-)
ic_raw_xmit() is also an ic method, not a vap method. Yes, it bypasses
the vap queue.
There's deferred transmission going on (eg ath_start() getting called
from TX completion, as an example.) Should that be called under the
above lock() in your example?
And the TX sequence number stuff in iwn and ath, because the
aggregation code reuses the ni_txseqs[] array. So yes, the locking
wouldn't clash with the rest of net80211 (as only either the non-agg
TX path in net80211 or the TX path in ath/iwn would be using that
variable) but it's still a pain in the ass.
Grr.
The ridiculously complicated, broken-down-into-bits version:
* add a new mbuf tag to mbufs (optional) which lets us tag an mbuf
with a bpf TX params struct (ie, what gets optionally passed into the
raw xmit call);
* calls to ic_raw_xmit() become a VAP raw xmit;
* the VAP raw xmit pushes mbufs into the same ifnet queue that the VAP
frames have (ie right now the VAP ifnet queue) - ie, serialise the raw
and non-raw VAP frames before it gets processed for 802.11 state;
* .. then move the VAP transmit to your model - which is
+ vap transmit and raw transmit just push frames into the vap ifnet queue;
+ vap transmit then schedules a deferred task or a direct dispatch +
deferred task if we cant grab the VAP TX lock;
+ the vap transmit task/dispatch routine handles the frames one at a
time - either inside a task queue or inside the VAP TX lock - thus all
of the 802.3 - 802.11 encapsulation and state handling is correctly
serialised _and_ the order of sequence numbers and frame handling is
maintained;
+ then those frames have to be queued to the driver in _exactly_ the
same order - so either the VAP TX task/process queues them into a
driver staging queue and kicks the driver side ,or it goes into a
separate task to do driver dispatch;
+ .. and then it's up to the driver to dequeue each frame, handle it
and push them into the hardware in the same order.
Now, that's totally, ridiculously complicated but it does avoid a
bunch of reentrant
[RFT] net80211 TX serialisation, take #4
Hi, Here's take four of the TX serialisation. http://people.freebsd.org/~adrian/ath/20130223-net80211-tx-lock-4.diff This patch increases the lock reach so it locks the encap path for both data and management frames, so the path between sequence number allocation and driver queuing is held. There are some drivers that directly access ni_txseqs[] (ie, iwn and ath) and I'll have to think about this a little more. Sometimes it'll be called with the VAP TX lock held (ie, if it's called from driver if_transmit / driver if_start / ic_raw_xmit) but sometimes the TX path is called from the addba response callback, the TX completion methods, a software frame taskqueue. None of these grab the VAP TX lock at any point. I'd like to avoid making the VAP TX lock a reentrant lock (ew.) I'm open to ideas/suggestions. Thanks, Adrian ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [RFT] net80211 TX serialisation, take #4
On 22 February 2013 15:25, Adrian Chadd adr...@freebsd.org wrote: Hi, Here's take four of the TX serialisation. http://people.freebsd.org/~adrian/ath/20130223-net80211-tx-lock-4.diff This patch increases the lock reach so it locks the encap path for both data and management frames, so the path between sequence number allocation and driver queuing is held. There are some drivers that directly access ni_txseqs[] (ie, iwn and ath) and I'll have to think about this a little more. Sometimes it'll be called with the VAP TX lock held (ie, if it's called from driver if_transmit / driver if_start / ic_raw_xmit) but sometimes the TX path is called from the addba response callback, the TX completion methods, a software frame taskqueue. None of these grab the VAP TX lock at any point. I'd like to avoid making the VAP TX lock a reentrant lock (ew.) Well, it turns out that this is almost-but-not-really-right. The problem is this: * A frame is going out on VAP A * so the VAP TX lock is held for VAP A * then the driver if_transmit() method is called, which will (for now) map to enqueue and call driver if_start() * now, if_start() will dequeue the top frame, which may be for VAP B this is fine so far, as the VAP lock is intended to serialise stuff through to the driver transmission phase. It's not designed to serialise _between_ VAPs. However, then we have a little hiccup. iwn and ath use the ni_txseqs[] space for TX sequence numbers when transmitting aggregate frames. I guess mwl does sequence number allocation in-firmware. So to be correct, i should grab the VAP lock when the driver transmit code wants to assign sequence numbers. However I can't do that: (a) It may already have it held from the net80211 call; (b) it may NOT already have it held (eg from a deferred call to the driver start method - eg, if the driver calls if_start after TX completion has occured, or upon driver reset to start TX again); (c) it may have a VAP lock held from a _different_ VAP, because of the conditions above. So, this is all pretty terrible. The only sane solution for now is to make my VAP TX lock an IC TX lock,and grab said IC TX lock for all VAPs. That way the driver can grab the IC TX lock when it's doing deferred sends and it'll be sure the lock is held when it decides to grab/increment sequence numbers from ni-ni_txseqs[]. This is all pretty terrible. Honestly, what I really need is a way to do this: * serialise frame TX handling in net80211 so the sequence of frames handed to the driver _matches exactly_ the sequence going through the VAP TX code, the VAP management TX code, etc.; * call the driver without the VAP or IC TX lock held - at this point, the frames should be in the driver queue _in the order_ they were processed via the VAP TX and management / raw TX path; * the driver can grab the VAP / IC TX lock if it needs to allocate sequence numbers itself; * the driver is then responsible for ensuring that frames are processed to software/hardware queuing in the same order they were received from net80211 (so CCMP IV sequence assignment is 'correct' too); * .. and do this without tearing my hair out. So the short term fix is: * convert my VAP TX Lock to an IC TX lock; * for the drivers that touch the ni_txseqs[] values (ath, iwn at least) - have them grab the IC TX lock before any deferred if_start/if_transmit call - that way the driver TX path holds the same locks no matter whether it came from the net80211 stack or a deferred start; * chase down and eliminate any and all of the rest of the subtle packets out of sequence crap that occurs when you're doing high throughput 802.11n with CCMP encryption; * .. think about how to properly separate out the queuing from the driver processing. I'm open to suggestions here. Adrian ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [RFT] net80211 TX serialisation, take #4
.. and as a reference, Linux mac80211 seems to just do TX through a single-threaded workqueue. Ie, all of the mac80211 TX is done deferred and serialised that way. Grrr.. I'm very tempted to just do this and be done with it for now. Adrian ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
