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
