On Mon, Mar 23, 2009 at 05:28:41PM +0100, Johannes Berg wrote:

Instead of stopping the entire AC queue when enabling aggregation
(which was only done for hardware with aggregation queues) buffer
the packets for each station, and release them to the pending skb
queue once aggregation is turned on successfully.

Thank you for the nice compromised solution.

We get a little more code, but it becomes conceptually simpler and
we can remove the entire virtual queue mechanism from mac80211 in
a follow-up patch.

This changes how mac80211 behaves towards drivers that support
aggregation but have no hardware queues -- those drivers will now
not be handed packets while the aggregation session is being
established, but only after it has been fully established.

That's fine from a logical point of view as compromise here as our
ampdu start should be relatively quick. Now while I can say this
from a logical point of view this patch obviously needed more
testing but lets see how it holds up and I'm glad you're the one
who wrote it. I'm confident there won't be any issues but that
is something I cannot gaurantee just based on the review. We now
need to go out and tests this. All other patches previous to this
make 100% perfect sense.

I'm particularly interested in seeing results with AP support.
Did you get to test that with ath9k by any chance?

Hauke -- just a heads up since you're quick with this now :)  :
skb_queue_splice_tail_init() needs some backporting to 2.6.27. I'd do
it but I'm beat and calling it a day now.

More comments below. I've tried to remove all the hunks I didn't have
comments on.

ath9k's ampdu start is pretty quick anyway, so this
won't be for long.

No point in stopping the queue if the STA's tid queue is empty.

Is it possible for ieee80211_stop_tx_ba_cb() to be called while
state of the tid is not yet operational? from what I can tell
that's the only case when we would have added skbs to the STA's tid
pending queue.

 
 	*state = HT_AGG_STATE_IDLE;
+	/* from now on packets are no longer put onto sta->pending */

I think it would help to refer to the pendign queue consitantly instead
as something like "STA's TID pending queue" as that is what it is. We also
now have the local->pending queue. STA's pending queue seems to imply
its also used for non-aggregation sessions.

See -- if the state is not operation nothing would have gone
been put into the STA's tid pending queue. Might also be worth
mentioning that in the comment.

Hm, why were we not crashing here before? I figure we would have
with something like this:

state = &tx->sta->ampdu_mlme.tid_state_tx[tid];

That it itself should be separate patch, but too late now. Anyway
the new added check for IEEE80211_HW_AMPDU_AGGREGATION should go
to stable.

 		unsigned long flags;
+		struct tid_ampdu_tx *tid_tx;
+
 		qc = ieee80211_get_qos_ctl(hdr);
 		tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
 
 		spin_lock_irqsave(&tx->sta->lock, flags);
+		/*
+		 * XXX: This spinlock could be fairly expensive, but see the
+		 *	comment in agg-tx.c:ieee80211_agg_tx_operational().
+		 *	One way to solve this would be to do something RCU-like
+		 *	for managing the tid_tx struct and using atomic bitops
+		 *	for the actual state -- by introducing an actual
+		 *	'operational' bit that would be possible. It would
+		 *	require changing ieee80211_agg_tx_operational() to
+		 *	set that bit, and changing the way tid_tx is managed
+		 *	everywhere, including races between that bit and
+		 *	tid_tx going away (tid_tx being added can be easily
+		 *	committed to memory before the 'operational' bit).
+		 */
+		tid_tx = tx->sta->ampdu_mlme.tid_tx[tid];
 		state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
-		if (*state == HT_AGG_STATE_OPERATIONAL)
+		if (*state == HT_AGG_STATE_OPERATIONAL) {
 			info->flags |= IEEE80211_TX_CTL_AMPDU;

See, when its operational we don't add stuff to the STA's TID pending 
queue.

This piece:

+		} else if (*state != HT_AGG_STATE_IDLE) {
+			/* in progress */
+			queued = true;
+			info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
+			__skb_queue_tail(&tid_tx->pending, skb);
+		}

Can probably be ported to stable too, to just TX_DROP.

For someone who hasn't read all the pending queue code stuff the above would be
a little brain twister. It might be good to leave a comment explaining why
txpendign would be set and why we add it to the head (i.e. because the "skb" sent
at a previous time was put into a pending queue). Maybe even rename txpending to
something like skb_was_pending.

All these NETDEV_TX_OK could've gone into a separate patch.

  Luis