ath9k: Add airtime fairness scheduler

This patch leaves the code for ath9k's internal per-node per-tid queues in place and just modifies the driver to also pull from the new mac80211 intermediate software queues, and implements the .wake_tx_queue method, which will cause mac80211 to deliver packets to be sent via the new intermediate queue.

Signed-off-by Tim Shepard <[email protected]>

Reworked to not require the global variable renaming in ath9k.

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

---
 drivers/net/wireless/ath/ath9k/ath9k.h     |  16 +++-
 drivers/net/wireless/ath/ath9k/debug_sta.c |   7 +-
 drivers/net/wireless/ath/ath9k/init.c      |   1 +
 drivers/net/wireless/ath/ath9k/main.c      |   1 +
 drivers/net/wireless/ath/ath9k/xmit.c      | 119 +++++++++++++++++++++++++----
 5 files changed, 125 insertions(+), 19 deletions(-)

drivers/net/wireless/ath/ath9k/ath9k.h +0 -2

index 93b3793..caeae10 100644
@@ -145,8 +145,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct ath_descdma *dd,
 #define BAW_WITHIN(_start, _bawsz, _seqno) \
 	((((_seqno) - (_start)) & 4095) < (_bawsz))
 
-#define ATH_AN_2_TID(_an, _tidno)  (&(_an)->tid[(_tidno)])
-
 #define IS_HT_RATE(rate)   (rate & 0x80)
 #define IS_CCK_RATE(rate)  ((rate >= 0x18) && (rate <= 0x1e))
 #define IS_OFDM_RATE(rate) ((rate >= 0x8) && (rate <= 0xf))

Quoted 34 lines · Toke Høiland-Jørgensen

This patch leaves the code for ath9k's internal per-node per-tid queues in place and just modifies the driver to also pull from the new mac80211 intermediate software queues, and implements the .wake_tx_queue method, which will cause mac80211 to deliver packets to be sent via the new intermediate queue.

Signed-off-by Tim Shepard <[email protected]>

Reworked to not require the global variable renaming in ath9k.

Signed-off-by Toke Høiland-Jørgensen <[email protected]>

---
 drivers/net/wireless/ath/ath9k/ath9k.h     |  16 +++-
 drivers/net/wireless/ath/ath9k/debug_sta.c |   7 +-
 drivers/net/wireless/ath/ath9k/init.c      |   1 +
 drivers/net/wireless/ath/ath9k/main.c      |   1 +
 drivers/net/wireless/ath/ath9k/xmit.c      | 119 +++++++++++++++++++++++++----
 5 files changed, 125 insertions(+), 19 deletions(-)

drivers/net/wireless/ath/ath9k/ath9k.h +0 -2

index 93b3793..caeae10 100644
@@ -145,8 +145,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct ath_descdma *dd,
 #define BAW_WITHIN(_start, _bawsz, _seqno) \
 	((((_seqno) - (_start)) & 4095) < (_bawsz))
 
-#define ATH_AN_2_TID(_an, _tidno)  (&(_an)->tid[(_tidno)])
-
 #define IS_HT_RATE(rate)   (rate & 0x80)
 #define IS_CCK_RATE(rate)  ((rate >= 0x18) && (rate <= 0x1e))
 #define IS_OFDM_RATE(rate) ((rate >= 0x8) && (rate <= 0xf))

@@ -232,8 +230,10 @@ struct ath_buf {
 

drivers/net/wireless/ath/ath9k/ath9k.h struct ath_buf { · line 233

Quoted 3 lines

drivers/net/wireless/ath/ath9k/ath9k.h +1 -0

 struct ath_atx_tid {
 	struct list_head list;
+	struct sk_buff_head i_q;

Do we really need a third queue here? Instead of adding yet another layer of queueing here, I think we should even get rid of buf_q.

Channel context based queue handling can be dealt with by stopping/starting relevant queues on channel context changes.

buf_q becomes unnecessary when you remove all code in the drv_tx codepath that moves frames to the intermediate queue.

Any frame that was pulled from the intermediate queue and prepared for tx, but which can't be sent right now can simply be queued to retry_q.

This will also help with getting the diffstat insertion/deletion ratio under control ;)

drivers/net/wireless/ath/ath9k/ath9k.h struct ath_buf { · line 236

Quoted 3 lines

drivers/net/wireless/ath/ath9k/ath9k.h +1 -0

 	struct sk_buff_head buf_q;
 	struct sk_buff_head retry_q;
+	struct ieee80211_txq *swq;

No need for this pointer, you can use container_of.

Quoted 35 lines · Felix Fietkau

Quoted 34 lines · Toke H?iland-J?rgensen

This patch leaves the code for ath9k's internal per-node per-tid queues in place and just modifies the driver to also pull from the new mac80211 intermediate software queues, and implements the .wake_tx_queue method, which will cause mac80211 to deliver packets to be sent via the new intermediate queue.

Signed-off-by Tim Shepard <[email protected]>

Reworked to not require the global variable renaming in ath9k.

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

---
 drivers/net/wireless/ath/ath9k/ath9k.h     |  16 +++-
 drivers/net/wireless/ath/ath9k/debug_sta.c |   7 +-
 drivers/net/wireless/ath/ath9k/init.c      |   1 +
 drivers/net/wireless/ath/ath9k/main.c      |   1 +
 drivers/net/wireless/ath/ath9k/xmit.c      | 119 +++++++++++++++++++++++++----
 5 files changed, 125 insertions(+), 19 deletions(-)

drivers/net/wireless/ath/ath9k/ath9k.h +0 -2

index 93b3793..caeae10 100644
@@ -145,8 +145,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct ath_descdma *dd,
 #define BAW_WITHIN(_start, _bawsz, _seqno) \
 	((((_seqno) - (_start)) & 4095) < (_bawsz))
 
-#define ATH_AN_2_TID(_an, _tidno)  (&(_an)->tid[(_tidno)])
-
 #define IS_HT_RATE(rate)   (rate & 0x80)
 #define IS_CCK_RATE(rate)  ((rate >= 0x18) && (rate <= 0x1e))
 #define IS_OFDM_RATE(rate) ((rate >= 0x8) && (rate <= 0xf))

@@ -232,8 +230,10 @@ struct ath_buf {
 

Quoted 5 lines

drivers/net/wireless/ath/ath9k/ath9k.h struct ath_buf { · line 233

Quoted 3 lines

drivers/net/wireless/ath/ath9k/ath9k.h +1 -0

 struct ath_atx_tid {
 	struct list_head list;
+	struct sk_buff_head i_q;

Do we really need a third queue here? Instead of adding yet another layer of queueing here, I think we should even get rid of buf_q.

This is definitely something that needs to be improved. One other sticking point related to this: in the current version of this patch ath_tid_has_buffered() gains a side effect of pulling from the mac80211 txq, which is obviously not so nice.

The obvious way to get rid of this is to export a txq_has_buffered() function at the mac80211 layer. But avoiding that may be possible; the sticking point is what to do with the code paths that do not dequeue packets, but check ath_tid_has_buffered() to decide whether to schedule the queue and/or to tell ieee80211_sta_set_buffered() about it (these are for instance ath_tx_aggr_sleep/wakeup(). Can those just be removed (i.e. don't call into ieee80211, and always schedule the txq on wakeup? I'm not familiar enough with the intermediate queues to make that call...

Quoted 48 lines · Toke H?iland-J?rgensen

Quoted 35 lines · Felix Fietkau

Quoted 34 lines · Toke H?iland-J?rgensen

This patch leaves the code for ath9k's internal per-node per-tid queues in place and just modifies the driver to also pull from the new mac80211 intermediate software queues, and implements the .wake_tx_queue method, which will cause mac80211 to deliver packets to be sent via the new intermediate queue.

Signed-off-by Tim Shepard <[email protected]>

Reworked to not require the global variable renaming in ath9k.

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

---
 drivers/net/wireless/ath/ath9k/ath9k.h     |  16 +++-
 drivers/net/wireless/ath/ath9k/debug_sta.c |   7 +-
 drivers/net/wireless/ath/ath9k/init.c      |   1 +
 drivers/net/wireless/ath/ath9k/main.c      |   1 +
 drivers/net/wireless/ath/ath9k/xmit.c      | 119 +++++++++++++++++++++++++----
 5 files changed, 125 insertions(+), 19 deletions(-)

drivers/net/wireless/ath/ath9k/ath9k.h +0 -2

index 93b3793..caeae10 100644
@@ -145,8 +145,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct ath_descdma *dd,
 #define BAW_WITHIN(_start, _bawsz, _seqno) \
 	((((_seqno) - (_start)) & 4095) < (_bawsz))
 
-#define ATH_AN_2_TID(_an, _tidno)  (&(_an)->tid[(_tidno)])
-
 #define IS_HT_RATE(rate)   (rate & 0x80)
 #define IS_CCK_RATE(rate)  ((rate >= 0x18) && (rate <= 0x1e))
 #define IS_OFDM_RATE(rate) ((rate >= 0x8) && (rate <= 0xf))

@@ -232,8 +230,10 @@ struct ath_buf {
 

Quoted 5 lines

drivers/net/wireless/ath/ath9k/ath9k.h struct ath_buf { · line 233

Quoted 3 lines

drivers/net/wireless/ath/ath9k/ath9k.h +1 -0

 struct ath_atx_tid {
 	struct list_head list;
+	struct sk_buff_head i_q;

Do we really need a third queue here? Instead of adding yet another layer of queueing here, I think we should even get rid of buf_q.

This is definitely something that needs to be improved. One other sticking point related to this: in the current version of this patch ath_tid_has_buffered() gains a side effect of pulling from the mac80211 txq, which is obviously not so nice.

Quoted 9 lines

The obvious way to get rid of this is to export a txq_has_buffered() function at the mac80211 layer. But avoiding that may be possible; the sticking point is what to do with the code paths that do not dequeue packets, but check ath_tid_has_buffered() to decide whether to schedule the queue and/or to tell ieee80211_sta_set_buffered() about it (these are for instance ath_tx_aggr_sleep/wakeup(). Can those just be removed (i.e. don't call into ieee80211, and always schedule the txq on wakeup? I'm not familiar enough with the intermediate queues to make that call...

For tx scheduling, we can use swq_nonempty and deal with false positives. For power save we should only use ieee80211_sta_set_buffered if the driver itself has buffered some frames. Indication of packets in the mac80211 intermediate queue is already taken care of inside mac80211.

Quoted 16 lines

drivers/net/wireless/ath/ath9k/ath9k.h +0 -2

index 93b3793..caeae10 100644
@@ -145,8 +145,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct ath_descdma *dd,
 #define BAW_WITHIN(_start, _bawsz, _seqno) \
 	((((_seqno) - (_start)) & 4095) < (_bawsz))
 
-#define ATH_AN_2_TID(_an, _tidno)  (&(_an)->tid[(_tidno)])
-
 #define IS_HT_RATE(rate)   (rate & 0x80)
 #define IS_CCK_RATE(rate)  ((rate >= 0x18) && (rate <= 0x1e))
 #define IS_OFDM_RATE(rate) ((rate >= 0x8) && (rate <= 0xf))

drivers/net/wireless/ath/ath9k/ath9k.h struct ath_buf { · line 233

Quoted 4 lines

drivers/net/wireless/ath/ath9k/ath9k.h +1 -0

@@ -232,8 +230,10 @@ struct ath_buf {
 
 struct ath_atx_tid {
 	struct list_head list;
+	struct sk_buff_head i_q;

Do we really need a third queue here? Instead of adding yet another layer of queueing here, I think we should even get rid of buf_q.

Channel context based queue handling can be dealt with by stopping/starting relevant queues on channel context changes.

buf_q becomes unnecessary when you remove all code in the drv_tx codepath that moves frames to the intermediate queue.

Any frame that was pulled from the intermediate queue and prepared for tx, but which can't be sent right now can simply be queued to retry_q.

This will also help with getting the diffstat insertion/deletion ratio under control ;)

drivers/net/wireless/ath/ath9k/ath9k.h struct ath_buf { · line 236

Quoted 3 lines

drivers/net/wireless/ath/ath9k/ath9k.h +1 -0

 	struct sk_buff_head buf_q;
 	struct sk_buff_head retry_q;
+	struct ieee80211_txq *swq;

No need for this pointer, you can use container_of.

Quoted 30 lines

Quoted 14 lines

drivers/net/wireless/ath/ath9k/ath9k.h +0 -2

index 93b3793..caeae10 100644
@@ -145,8 +145,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct ath_descdma *dd,
 #define BAW_WITHIN(_start, _bawsz, _seqno) \
 	((((_seqno) - (_start)) & 4095) < (_bawsz))
 
-#define ATH_AN_2_TID(_an, _tidno)  (&(_an)->tid[(_tidno)])
-
 #define IS_HT_RATE(rate)   (rate & 0x80)
 #define IS_CCK_RATE(rate)  ((rate >= 0x18) && (rate <= 0x1e))
 #define IS_OFDM_RATE(rate) ((rate >= 0x8) && (rate <= 0xf))

drivers/net/wireless/ath/ath9k/ath9k.h struct ath_buf { · line 233

Quoted 4 lines

drivers/net/wireless/ath/ath9k/ath9k.h +1 -0

@@ -232,8 +230,10 @@ struct ath_buf {
 
 struct ath_atx_tid {
 	struct list_head list;
+	struct sk_buff_head i_q;

Do we really need a third queue here? Instead of adding yet another layer of queueing here, I think we should even get rid of buf_q.

Channel context based queue handling can be dealt with by stopping/starting relevant queues on channel context changes.

buf_q becomes unnecessary when you remove all code in the drv_tx codepath that moves frames to the intermediate queue.

Any frame that was pulled from the intermediate queue and prepared for tx, but which can't be sent right now can simply be queued to retry_q.

Quoted 7 lines

This will also help with getting the diffstat insertion/deletion ratio under control ;)

drivers/net/wireless/ath/ath9k/ath9k.h struct ath_buf { · line 236

Quoted 3 lines

drivers/net/wireless/ath/ath9k/ath9k.h +1 -0

 	struct sk_buff_head buf_q;
 	struct sk_buff_head retry_q;
+	struct ieee80211_txq *swq;

No need for this pointer, you can use container_of.

Felix, great to hear from you and thanks for your feedback. I will try to work on this.

I was struggling to understand the channel context stuff, and I have no idea how to test it. (Is there anyone else listening who might be able to help with testing the channel context stuff as we improve this patch and simplify the ath9k driver's use of the new mac80211 intermediate queues?)

Felix, do you have any thoughts on the renaming of txq to hwx that I had done in my original version of this patch? I had a good e-mail discussion with Toke a week or two ago (cc these same various lists) and I believe he came to understand that perhaps the renaming I had done in the original version of this patch was worth doing.

Now in Toke's version of my patch he calls the ieee80211 txq a "swq" and the ath9k hardware queue is called a "txq". (I had called the ieee80211 txq a "txq" and I renamed the ath9k hardware queue "hwq"
throught all the ath9k driver code. This also made ath9k's names of
things more similar to mt76 which I was looking at as an example of a driver that uses your new ieee80211 txq mechanism.

I think the renaming is worth doing, but I also understand the renaming can be disruptive to others actively working on ath9k. It would be nice to have another opinion on this.

I think we should finish intermediate queues support first and then look into the rename later.

Hmm... if the renaming is going to go in mainline, I feel pretty strongly it should go in *before* a patch to switch over to use the intermediate queues. The whole point of the renaming was to make the code that uses the intermediate queues much more understandable (avoiding the unfortuante collision of "txq" meaning two different things throughout the code).

Once it is all done and everyone's done reading and trying to understand this code, there's much less reason to do the renaming.

Toke, how do you feel about this at this point?

I'm asking because I hope to have a new version of my patch soon (fixing a bug in how it handles tid->hwq->pending_frames and hq_max_pending[*] ),

Quoted 3 lines

 struct ath_atx_tid {
      struct list_head list;

+ struct sk_buff_head i_q;

Do we really need a third queue here? Instead of adding yet another layer of queueing here, I think we should even get rid of buf_q.

Channel context based queue handling can be dealt with by stopping/starting relevant queues on channel context changes.

buf_q becomes unnecessary when you remove all code in the drv_tx codepath that moves frames to the intermediate queue.

Any frame that was pulled from the intermediate queue and prepared for tx, but which can't be sent right now can simply be queued to retry_q.

This will also help with getting the diffstat insertion/deletion ratio under control ;)

elsewhere powersave was mentioned

drivers/net/wireless/ath/ath9k/xmit.c +28 -0

+static struct sk_buff *
+ath_tid_pull(struct ath_atx_tid *tid)
+{
+	struct ath_softc *sc = tid->an->sc;
+	struct ieee80211_hw *hw = sc->hw;
+	struct ath_tx_control txctl = {
+		.txq = tid->txq,
+		.sta = tid->an->sta,
+	};
+	struct sk_buff *skb;
+	struct ath_frame_info *fi;
+	int q;
+
+	if (!tid->has_queued)
+		return NULL;
+
+	skb = ieee80211_tx_dequeue(hw, container_of((void*)tid, struct ieee80211_txq, drv_priv));
+	if (!skb) {
+		tid->has_queued = false;
+		return NULL;
+	}
+
+	if (ath_tx_prepare(hw, skb, &txctl)) {
+		ieee80211_free_txskb(hw, skb);
+		return NULL;
+	}
+
+	q = skb_get_queue_mapping(skb);

What's the symptom of this? As I said I haven't noticed anything, but it might be worth looking out for.

Quoted 28 lines

drivers/net/wireless/ath/ath9k/xmit.c +28 -0

+static struct sk_buff *
+ath_tid_pull(struct ath_atx_tid *tid)
+{
+	struct ath_softc *sc = tid->an->sc;
+	struct ieee80211_hw *hw = sc->hw;
+	struct ath_tx_control txctl = {
+		.txq = tid->txq,
+		.sta = tid->an->sta,
+	};
+	struct sk_buff *skb;
+	struct ath_frame_info *fi;
+	int q;
+
+	if (!tid->has_queued)
+		return NULL;
+
+	skb = ieee80211_tx_dequeue(hw, container_of((void*)tid, struct ieee80211_txq, drv_priv));
+	if (!skb) {
+		tid->has_queued = false;
+		return NULL;
+	}
+
+	if (ath_tx_prepare(hw, skb, &txctl)) {
+		ieee80211_free_txskb(hw, skb);
+		return NULL;
+	}
+
+	q = skb_get_queue_mapping(skb);

drivers/net/wireless/ath/ath9k/xmit.c ath_get_skb_tid(struct ath_softc *sc, struct ath_node *an, struct sk_buff *skb) · line 235

Quoted 10 lines

drivers/net/wireless/ath/ath9k/xmit.c +10 -0

+	if (tid->txq == sc->tx.txq_map[q]) {
+		fi = get_frame_info(skb);
+		fi->txq = q;
+		++tid->txq->pending_frames;
+	}
+
+	return skb;
+ }
+
+

The increment of ->pending_frames lacks a corresponding check against sc->tx.txq_max_pending to see if we've reached the limit. (Which begs the question: what to do if it has?)

I discovered this bug doing experiments by trying to turn down the various /sys/kernel/debug/ieee80211/phy0/ath9k/qlen_* to low numbers (including as low as one, and then even zero) and found it had no effect.

The second more mysterious bug which I'm still struggling to understand is why doesn't large values in these ath9k/qlen_* (or more accurately, given the first bug above, the failure to check these qlen limit values at all) allow for increased hardware queue bloat (with observable delay).

You're right that it doesn't check the max. However, this is less of a problem now that there is no intermediate queueing in the driver; and indeed the utility of haven the qlen_* tunables is somewhat questionable with the patch applied: The only thing this is going to control is the size of the retry queue, and possible limit the size of the retry queue. [....]

Quoted 5 lines

The second more mysterious bug which I'm still struggling to understand is why doesn't large values in these ath9k/qlen_* (or more accurately, given the first bug above, the failure to check these qlen limit values at all) allow for increased hardware queue bloat (with observable delay).

Because there's a second limit in play (which has always been there): in ath_tx_sched_aggr() there is this check:

	if ((aggr && txq->axq_ampdu_depth >= ATH_AGGR_MIN_QDEPTH) ||
	    (!aggr && txq->axq_depth >= ATH_NON_AGGR_MIN_QDEPTH)) {
		__skb_queue_tail(&tid->retry_q, bf->bf_mpdu);
		*stop = true;
		return false;
	}

The two constants are 2 and 8 respectively. This means that, with aggregation enabled, no more than two full aggregates will be queued up. The size of the aggregates is dynamically computed from the current
rate: they are limited a maximum of four milliseconds of (estimated) airtime (for the BE queue; the others have different limits).

So in a sense there's already a dynamic limit on the hardware queues. Now, whether four milliseconds is the right maximum aggregate size might be worth discussing. It is the maximum allowed by the standard. Dave and I have been

Quoted 7 lines

You're right that it doesn't check the max. However, this is less of a problem now that there is no intermediate queueing in the driver; and indeed the utility of haven the qlen_* tunables is somewhat questionable with the patch applied: The only thing this is going to control is the size of the retry queue, and possible limit the size of the retry queue. [....]

The driver queues things up for the hardware to DMA and transmit. Something has to limit the amount of packets handed over to the hardware. (We lack access to hardware documentation (grrrr!) but it appears to me that the hardware has a hard limit on how many packets can be handed to it.)

Quoted 16 lines

Because there's a second limit in play (which has always been there): in ath_tx_sched_aggr() there is this check:

	if ((aggr && txq->axq_ampdu_depth >= ATH_AGGR_MIN_QDEPTH) ||
	    (!aggr && txq->axq_depth >= ATH_NON_AGGR_MIN_QDEPTH)) {
		__skb_queue_tail(&tid->retry_q, bf->bf_mpdu);
		*stop = true;
		return false;
	}

The two constants are 2 and 8 respectively. This means that, with aggregation enabled, no more than two full aggregates will be queued up. The size of the aggregates is dynamically computed from the current
rate: they are limited a maximum of four milliseconds of (estimated) airtime (for the BE queue; the others have different limits).

Quoted 4 lines

So in a sense there's already a dynamic limit on the hardware queues. Now, whether four milliseconds is the right maximum aggregate size might be worth discussing. It is the maximum allowed by the standard. Dave and I have been

Ah that may be the clue that I lacked. There's got to be a dependency on processor speed (how quickly the system and driver can get another packet hooked up for transmission after completions) but perhaps with aggregates being so large in time, with full aggregates even the slowest processors are fast enough to avoid starvation.

If there's no aggregation, a limit of some sort is needed (probably to prevent malfunction of the hardware/driver, but in any case to limit excess latency). And this limit will depend on processor speed (and will need to be autotuned at runtime).

Channel context based queue handling can be dealt with by stopping/starting relevant queues on channel context changes.

Thanks for unconfusing me a couple weeks ago, and cluing me into how the limit on ->pending_frames is not really relevant for the data packets that go through the new intermediate queues.

But I'm not sure if this would allow us to remove the limit on pending_frames because even though normal data packets would not normally build up that many packets, there are other packet types which bypass the intermediate queues and are transmitted directly (also in most cases bypassing the ath9k internal queues in the way ath9k worked before we patched it to use the mac80211 intermediate queues).

Along similar lines, from reading the code I think your patch has introduced a bug (but I don't know how to demonstrate it at runtime).

Looking in the body of ath_tx_start() at the result of applying your patch, we now see this:

	[...]

	/* Force queueing of all frames that belong to a virtual interface on
	 * a different channel context, to ensure that they are sent on the
	 * correct channel.
	 */
	if (((avp && avp->chanctx != sc->cur_chan) ||
	     sc->cur_chan->stopped) && !txctl->force_channel) {
		if (!txctl->an)
			txctl->an = &avp->mcast_node;
		queue = true;
		skip_uapsd = true;
	}

	if (!skip_uapsd && ps_resp) {
		ath_txq_unlock(sc, txq);
		txq = sc->tx.uapsdq;
		ath_txq_lock(sc, txq);
	} else if(WARN_ON(txctl->an && queue)) 
		ath_txq_unlock(sc, txq);
		return -EINVAL;
	}

	[...]

In the case where the first if body above is run to force queuing of all packets (not just normal data packets), then the else case of the second if statement above will surely run and its if statement will surely be true, so your new WARN_ON will happen.

Quoted 2 lines · Earlier Felix

Channel context based queue handling can be dealt with by stopping/starting relevant queues on channel context changes.

But I don't see how to handle the case here where packets get passed to the driver with ath_tx_start() and wind up in the scenario above.

Quoted 11 lines · Tim Shepard

Thanks for unconfusing me a couple weeks ago, and cluing me into how the limit on ->pending_frames is not really relevant for the data packets that go through the new intermediate queues.

But I'm not sure if this would allow us to remove the limit on pending_frames because even though normal data packets would not normally build up that many packets, there are other packet types which bypass the intermediate queues and are transmitted directly (also in most cases bypassing the ath9k internal queues in the way ath9k worked before we patched it to use the mac80211 intermediate queues).

Yes, but, well, since they're not queued they are not going to overflow anything. The aggregation building logic stops at two queued aggregates, so the default limit of 123 packets is never going to be hit when the queue is moved into the mac80211 layer. So keeping the knobs around only helps people who purposefully want to cripple their ability to do aggregation; and it won't be doing what it promises (limiting qlen), since that is now moved out of the driver. So IMO, removing the knobs is the right thing to do. I have already updated my patch to do so, which I'll send as a v2 once the other bits are resolved.

Quoted 7 lines

Quoted 2 lines · Earlier Felix

Channel context based queue handling can be dealt with by stopping/starting relevant queues on channel context changes.

But I don't see how to handle the case here where packets get passed to the driver with ath_tx_start() and wind up in the scenario above.

Well, presumably the upper layers won't try to transmit anything through the old TX path if the start/stop logic is implemented properly. The chanctx code already seems to call the ieee80211_{start,stop}_queue() functions when changing context, so not sure what else is needed. Guess I'll go see if I can provoke an actual triggering of the bug, unless Felix elaborates on what he means before I get around to it (poke, Felix? :)).

Quoted 7 lines

Well, presumably the upper layers won't try to transmit anything through the old TX path if the start/stop logic is implemented properly. The chanctx code already seems to call the ieee80211_{start,stop}_queue() functions when changing context, so not sure what else is needed. Guess I'll go see if I can provoke an actual triggering of the bug, unless Felix elaborates on what he means before I get around to it (poke, Felix? :)).

Then I guess the logic in ath_tx_start was a leftover from a time before some queue related rework happened to the chanctx code.
In that case you can simply remove the chanctx related software queueing stuff from ath_tx_start.

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke Høiland-Jørgensen <[email protected]>

---
Changes since v1:
- Remove the old intermediate queueing logic completely instead of just disabling it.
- Remove the qlen debug tunables.
- Remove the force_channel parameter from struct txctl (since we just removed the code path that was using it).

  drivers/net/wireless/ath/ath9k/ath9k.h     |  12 +-
  drivers/net/wireless/ath/ath9k/channel.c   |   2 -
  drivers/net/wireless/ath/ath9k/debug.c     |  14 +-
  drivers/net/wireless/ath/ath9k/debug.h     |   2 -
  drivers/net/wireless/ath/ath9k/debug_sta.c |   4 +-
  drivers/net/wireless/ath/ath9k/init.c      |   2 +-
  drivers/net/wireless/ath/ath9k/main.c      |   1 +
  drivers/net/wireless/ath/ath9k/xmit.c      | 307 +++++++++++------------------
  8 files changed, 130 insertions(+), 214 deletions(-)

drivers/net/wireless/ath/ath9k/ath9k.h +5 -7

index 5294595..daf972c 100644
@@ -91,7 +91,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct ath_descdma *dd,
  #define ATH_RXBUF               512
  #define ATH_TXBUF               512
  #define ATH_TXBUF_RESERVE       5
-#define ATH_MAX_QDEPTH          (ATH_TXBUF / 4 - ATH_TXBUF_RESERVE)
  #define ATH_TXMAXTRY            13
  #define ATH_MAX_SW_RETRIES      30
 

@@ -145,7 +144,9 @@ int ath_descdma_setup(struct ath_softc *sc, struct ath_descdma *dd,
  #define BAW_WITHIN(_start, _bawsz, _seqno) \
  	((((_seqno) - (_start)) & 4095) < (_bawsz))
 
-#define ATH_AN_2_TID(_an, _tidno)  (&(_an)->tid[(_tidno)])
+#define ATH_STA_2_TID(_sta, _tidno) ((struct ath_atx_tid *)(_sta)->txq[_tidno]->drv_priv)
+#define ATH_VIF_2_TID(_vif) ((struct ath_atx_tid *)(_vif)->txq->drv_priv)
+#define ATH_AN_2_TID(_an, _tidno) ((_an)->sta ? ATH_STA_2_TID((_an)->sta, _tidno) : ATH_VIF_2_TID((_an)->vif))
 
  #define IS_HT_RATE(rate)   (rate & 0x80)
  #define IS_CCK_RATE(rate)  ((rate >= 0x18) && (rate <= 0x1e))

@@ -164,7 +165,6 @@ struct ath_txq {
  	spinlock_t axq_lock;
  	u32 axq_depth;
  	u32 axq_ampdu_depth;
-	bool stopped;
  	bool axq_tx_inprogress;
  	struct list_head txq_fifo[ATH_TXFIFO_DEPTH];
  	u8 txq_headidx;

@@ -232,7 +232,6 @@ struct ath_buf {
 
  struct ath_atx_tid {
  	struct list_head list;
-	struct sk_buff_head buf_q;
  	struct sk_buff_head retry_q;
  	struct ath_node *an;
  	struct ath_txq *txq;

@@ -247,13 +246,13 @@ struct ath_atx_tid {
  	s8 bar_index;
  	bool active;
  	bool clear_ps_filter;
+	bool has_queued;
  };
 
  struct ath_node {
  	struct ath_softc *sc;
  	struct ieee80211_sta *sta; /* station struct we're part of */
  	struct ieee80211_vif *vif; /* interface with which we're associated */
-	struct ath_atx_tid tid[IEEE80211_NUM_TIDS];
 
  	u16 maxampdu;
  	u8 mpdudensity;

@@ -276,7 +275,6 @@ struct ath_tx_control {
  	struct ath_node *an;
  	struct ieee80211_sta *sta;
  	u8 paprd;
-	bool force_channel;
  };
 
 

@@ -293,7 +291,6 @@ struct ath_tx {
  	struct ath_descdma txdma;
  	struct ath_txq *txq_map[IEEE80211_NUM_ACS];
  	struct ath_txq *uapsdq;
-	u32 txq_max_pending[IEEE80211_NUM_ACS];
  	u16 max_aggr_framelen[IEEE80211_NUM_ACS][4][32];
  };
 

@@ -585,6 +582,7 @@ void ath9k_release_buffered_frames(struct ieee80211_hw *hw,
  				   u16 tids, int nframes,
  				   enum ieee80211_frame_release_type reason,
  				   bool more_data);
+void ath9k_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *queue);
 
  /********/
  /* VIFs */

drivers/net/wireless/ath/ath9k/channel.c +0 -2

index 319cb5f..a5ce016 100644
@@ -1007,7 +1007,6 @@ static void ath_scan_send_probe(struct ath_softc *sc,
  		goto error;
 
  	txctl.txq = sc->tx.txq_map[IEEE80211_AC_VO];
-	txctl.force_channel = true;
  	if (ath_tx_start(sc->hw, skb, &txctl))
  		goto error;
 

@@ -1130,7 +1129,6 @@ ath_chanctx_send_vif_ps_frame(struct ath_softc *sc, struct ath_vif *avp,
  	memset(&txctl, 0, sizeof(txctl));
  	txctl.txq = sc->tx.txq_map[IEEE80211_AC_VO];
  	txctl.sta = sta;
-	txctl.force_channel = true;
  	if (ath_tx_start(sc->hw, skb, &txctl)) {
  		ieee80211_free_txskb(sc->hw, skb);
  		return false;

drivers/net/wireless/ath/ath9k/debug.c +1 -13

index 6de64cf..48b181d 100644
@@ -600,7 +600,6 @@ static int read_file_xmit(struct seq_file *file, void *data)
  	PR("MPDUs XRetried:  ", xretries);
  	PR("Aggregates:      ", a_aggr);
  	PR("AMPDUs Queued HW:", a_queued_hw);
-	PR("AMPDUs Queued SW:", a_queued_sw);
  	PR("AMPDUs Completed:", a_completed);
  	PR("AMPDUs Retried:  ", a_retries);
  	PR("AMPDUs XRetried: ", a_xretries);

@@ -629,8 +628,7 @@ static void print_queue(struct ath_softc *sc, struct ath_txq *txq,
  	seq_printf(file, "%s: %d ", "qnum", txq->axq_qnum);
  	seq_printf(file, "%s: %2d ", "qdepth", txq->axq_depth);
  	seq_printf(file, "%s: %2d ", "ampdu-depth", txq->axq_ampdu_depth);
-	seq_printf(file, "%s: %3d ", "pending", txq->pending_frames);
-	seq_printf(file, "%s: %d\n", "stopped", txq->stopped);
+	seq_printf(file, "%s: %3d\n", "pending", txq->pending_frames);
 
  	ath_txq_unlock(sc, txq);
  }

@@ -1190,7 +1188,6 @@ static const char ath9k_gstrings_stats[][ETH_GSTRING_LEN] = {
  	AMKSTR(d_tx_mpdu_xretries),
  	AMKSTR(d_tx_aggregates),
  	AMKSTR(d_tx_ampdus_queued_hw),
-	AMKSTR(d_tx_ampdus_queued_sw),
  	AMKSTR(d_tx_ampdus_completed),
  	AMKSTR(d_tx_ampdu_retries),
  	AMKSTR(d_tx_ampdu_xretries),

@@ -1270,7 +1267,6 @@ void ath9k_get_et_stats(struct ieee80211_hw *hw,
  	AWDATA(xretries);
  	AWDATA(a_aggr);
  	AWDATA(a_queued_hw);
-	AWDATA(a_queued_sw);
  	AWDATA(a_completed);
  	AWDATA(a_retries);
  	AWDATA(a_xretries);

@@ -1328,14 +1324,6 @@ int ath9k_init_debug(struct ath_hw *ah)
  				    read_file_xmit);
  	debugfs_create_devm_seqfile(sc->dev, "queues", sc->debug.debugfs_phy,
  				    read_file_queues);
-	debugfs_create_u32("qlen_bk", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
-			   &sc->tx.txq_max_pending[IEEE80211_AC_BK]);
-	debugfs_create_u32("qlen_be", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
-			   &sc->tx.txq_max_pending[IEEE80211_AC_BE]);
-	debugfs_create_u32("qlen_vi", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
-			   &sc->tx.txq_max_pending[IEEE80211_AC_VI]);
-	debugfs_create_u32("qlen_vo", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
-			   &sc->tx.txq_max_pending[IEEE80211_AC_VO]);
  	debugfs_create_devm_seqfile(sc->dev, "misc", sc->debug.debugfs_phy,
  				    read_file_misc);
  	debugfs_create_devm_seqfile(sc->dev, "reset", sc->debug.debugfs_phy,

drivers/net/wireless/ath/ath9k/debug.h +0 -2

index cd68c5f..a078cdd 100644
@@ -147,7 +147,6 @@ struct ath_interrupt_stats {
   * @completed: Total MPDUs (non-aggr) completed
   * @a_aggr: Total no. of aggregates queued
   * @a_queued_hw: Total AMPDUs queued to hardware
- * @a_queued_sw: Total AMPDUs queued to software queues
   * @a_completed: Total AMPDUs completed
   * @a_retries: No. of AMPDUs retried (SW)
   * @a_xretries: No. of AMPDUs dropped due to xretries

@@ -174,7 +173,6 @@ struct ath_tx_stats {
  	u32 xretries;
  	u32 a_aggr;
  	u32 a_queued_hw;
-	u32 a_queued_sw;
  	u32 a_completed;
  	u32 a_retries;
  	u32 a_xretries;

drivers/net/wireless/ath/ath9k/debug_sta.c +2 -2

index c2ca57a..d789798 100644
@@ -52,8 +52,8 @@ static ssize_t read_file_node_aggr(struct file *file, char __user *user_buf,
  			 "TID", "SEQ_START", "SEQ_NEXT", "BAW_SIZE",
  			 "BAW_HEAD", "BAW_TAIL", "BAR_IDX", "SCHED", "PAUSED");
 
-	for (tidno = 0, tid = &an->tid[tidno];
-	     tidno < IEEE80211_NUM_TIDS; tidno++, tid++) {
+	for (tidno = 0; tidno < IEEE80211_NUM_TIDS; tidno++) {
+		tid = ATH_STA_2_TID(an->sta, tidno);
  		txq = tid->txq;
  		ath_txq_lock(sc, txq);
  		if (tid->active) {

drivers/net/wireless/ath/ath9k/init.c +1 -1

index 1c226d6..752cacb 100644
@@ -354,7 +354,6 @@ static int ath9k_init_queues(struct ath_softc *sc)
  	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
  		sc->tx.txq_map[i] = ath_txq_setup(sc, ATH9K_TX_QUEUE_DATA, i);
  		sc->tx.txq_map[i]->mac80211_qnum = i;
-		sc->tx.txq_max_pending[i] = ATH_MAX_QDEPTH;
  	}
  	return 0;
  }

@@ -867,6 +866,7 @@ static void ath9k_set_hw_capab(struct ath_softc *sc, struct ieee80211_hw *hw)
  	hw->max_rate_tries = 10;
  	hw->sta_data_size = sizeof(struct ath_node);
  	hw->vif_data_size = sizeof(struct ath_vif);
+	hw->txq_data_size = sizeof(struct ath_atx_tid);
  	hw->extra_tx_headroom = 4;
 
  	hw->wiphy->available_antennas_rx = BIT(ah->caps.max_rxchains) - 1;

drivers/net/wireless/ath/ath9k/main.c +1 -0

index 3aed43a..f584e19 100644
@@ -2673,4 +2673,5 @@ struct ieee80211_ops ath9k_ops = {
  	.sw_scan_start	    = ath9k_sw_scan_start,
  	.sw_scan_complete   = ath9k_sw_scan_complete,
  	.get_txpower        = ath9k_get_txpower,
+	.wake_tx_queue      = ath9k_wake_tx_queue,
  };

drivers/net/wireless/ath/ath9k/xmit.c +120 -187

index fe795fc..4077eeb 100644
@@ -65,6 +65,8 @@ static struct ath_buf *ath_tx_setup_buffer(struct ath_softc *sc,
  					   struct ath_txq *txq,
  					   struct ath_atx_tid *tid,
  					   struct sk_buff *skb);
+static int ath_tx_prepare(struct ieee80211_hw *hw, struct sk_buff *skb,
+			  struct ath_tx_control *txctl);
 
  enum {
  	MCS_HT20,

@@ -118,6 +120,26 @@ static void ath_tx_queue_tid(struct ath_softc *sc, struct ath_txq *txq,
  		list_add_tail(&tid->list, list);
  }
 
+void ath9k_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *queue)
+{
+	struct ath_softc *sc = hw->priv;
+	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	struct ath_atx_tid *tid = (struct ath_atx_tid *) queue->drv_priv;
+	struct ath_txq *txq = tid->txq;
+
+	ath_dbg(common, QUEUE, "Waking TX queue: %pM (%d)\n",
+		queue->sta ? queue->sta->addr : queue->vif->addr,
+		tid->tidno);
+
+	ath_txq_lock(sc, txq);
+
+	tid->has_queued = true;
+	ath_tx_queue_tid(sc, txq, tid);
+	ath_txq_schedule(sc, txq);
+
+	ath_txq_unlock(sc, txq);
+}
+
  static struct ath_frame_info *get_frame_info(struct sk_buff *skb)
  {
  	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);

@@ -145,7 +167,6 @@ static void ath_set_rates(struct ieee80211_vif *vif, struct ieee80211_sta *sta,
  static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,
  			     struct sk_buff *skb)
  {
-	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
  	struct ath_frame_info *fi = get_frame_info(skb);
  	int q = fi->txq;
 

@@ -156,14 +177,6 @@ static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,
  	if (WARN_ON(--txq->pending_frames < 0))
  		txq->pending_frames = 0;
 
-	if (txq->stopped &&
-	    txq->pending_frames < sc->tx.txq_max_pending[q]) {
-		if (ath9k_is_chanctx_enabled())
-			ieee80211_wake_queue(sc->hw, info->hw_queue);
-		else
-			ieee80211_wake_queue(sc->hw, q);
-		txq->stopped = false;
-	}
  }
 
  static struct ath_atx_tid *

@@ -173,9 +186,47 @@ ath_get_skb_tid(struct ath_softc *sc, struct ath_node *an, struct sk_buff *skb)
  	return ATH_AN_2_TID(an, tidno);
  }
 
+static struct sk_buff *
+ath_tid_pull(struct ath_atx_tid *tid)
+{
+	struct ath_softc *sc = tid->an->sc;
+	struct ieee80211_hw *hw = sc->hw;
+	struct ath_tx_control txctl = {
+		.txq = tid->txq,
+		.sta = tid->an->sta,
+	};
+	struct sk_buff *skb;
+	struct ath_frame_info *fi;
+	int q;
+
+	if (!tid->has_queued)
+		return NULL;
+
+	skb = ieee80211_tx_dequeue(hw, container_of((void*)tid, struct ieee80211_txq, drv_priv));
+	if (!skb) {
+		tid->has_queued = false;
+		return NULL;
+	}
+
+	if (ath_tx_prepare(hw, skb, &txctl)) {
+		ieee80211_free_txskb(hw, skb);
+		return NULL;
+	}
+
+	q = skb_get_queue_mapping(skb);
+	if (tid->txq == sc->tx.txq_map[q]) {
+		fi = get_frame_info(skb);
+		fi->txq = q;
+		++tid->txq->pending_frames;
+	}
+
+	return skb;
+ }
+
+
  static bool ath_tid_has_buffered(struct ath_atx_tid *tid)
  {
-	return !skb_queue_empty(&tid->buf_q) || !skb_queue_empty(&tid->retry_q);
+	return !skb_queue_empty(&tid->retry_q) || tid->has_queued;
  }
 
  static struct sk_buff *ath_tid_dequeue(struct ath_atx_tid *tid)

@@ -184,46 +235,11 @@ static struct sk_buff *ath_tid_dequeue(struct ath_atx_tid *tid)
 
  	skb = __skb_dequeue(&tid->retry_q);
  	if (!skb)
-		skb = __skb_dequeue(&tid->buf_q);
+		skb = ath_tid_pull(tid);
 
  	return skb;
  }
 
-/*
- * ath_tx_tid_change_state:
- * - clears a-mpdu flag of previous session
- * - force sequence number allocation to fix next BlockAck Window
- */
-static void
-ath_tx_tid_change_state(struct ath_softc *sc, struct ath_atx_tid *tid)
-{
-	struct ath_txq *txq = tid->txq;
-	struct ieee80211_tx_info *tx_info;
-	struct sk_buff *skb, *tskb;
-	struct ath_buf *bf;
-	struct ath_frame_info *fi;
-
-	skb_queue_walk_safe(&tid->buf_q, skb, tskb) {
-		fi = get_frame_info(skb);
-		bf = fi->bf;
-
-		tx_info = IEEE80211_SKB_CB(skb);
-		tx_info->flags &= ~IEEE80211_TX_CTL_AMPDU;
-
-		if (bf)
-			continue;
-
-		bf = ath_tx_setup_buffer(sc, txq, tid, skb);
-		if (!bf) {
-			__skb_unlink(skb, &tid->buf_q);
-			ath_txq_skb_done(sc, txq, skb);
-			ieee80211_free_txskb(sc->hw, skb);
-			continue;
-		}
-	}
-
-}
-
  static void ath_tx_flush_tid(struct ath_softc *sc, struct ath_atx_tid *tid)
  {
  	struct ath_txq *txq = tid->txq;

@@ -858,7 +874,7 @@ static int ath_compute_num_delims(struct ath_softc *sc, struct ath_atx_tid *tid,
 
  static struct ath_buf *
  ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
-			struct ath_atx_tid *tid, struct sk_buff_head **q)
+			struct ath_atx_tid *tid)
  {
  	struct ieee80211_tx_info *tx_info;
  	struct ath_frame_info *fi;

@@ -867,11 +883,7 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
  	u16 seqno;
 
  	while (1) {
-		*q = &tid->retry_q;
-		if (skb_queue_empty(*q))
-			*q = &tid->buf_q;
-
-		skb = skb_peek(*q);
+		skb = ath_tid_dequeue(tid);
  		if (!skb)
  			break;
 

@@ -883,7 +895,6 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
  			bf->bf_state.stale = false;
 
  		if (!bf) {
-			__skb_unlink(skb, *q);
  			ath_txq_skb_done(sc, txq, skb);
  			ieee80211_free_txskb(sc->hw, skb);
  			continue;

@@ -912,8 +923,16 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
  		seqno = bf->bf_state.seqno;
 
  		/* do not step over block-ack window */
-		if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno))
+		if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno)) {
+			__skb_queue_tail(&tid->retry_q, skb);
+
+			/* If there are other skbs in the retry q, they are
+			 * probably within the BAW, so loop immediately to get
+			 * one of them. Otherwise the queue can get stuck. */
+			if (!skb_queue_is_first(&tid->retry_q, skb))
+				continue;
  			break;
+		}
 
  		if (tid->bar_index > ATH_BA_INDEX(tid->seq_start, seqno)) {
  			struct ath_tx_status ts = {};

@@ -921,7 +940,6 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
 
  			INIT_LIST_HEAD(&bf_head);
  			list_add(&bf->list, &bf_head);
-			__skb_unlink(skb, *q);
  			ath_tx_update_baw(sc, tid, seqno);
  			ath_tx_complete_buf(sc, bf, txq, &bf_head, &ts, 0);
  			continue;

@@ -933,11 +951,10 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
  	return NULL;
  }
 
-static bool
+static int
  ath_tx_form_aggr(struct ath_softc *sc, struct ath_txq *txq,
  		 struct ath_atx_tid *tid, struct list_head *bf_q,
-		 struct ath_buf *bf_first, struct sk_buff_head *tid_q,
-		 int *aggr_len)
+		 struct ath_buf *bf_first)
  {
  #define PADBYTES(_len) ((4 - ((_len) % 4)) % 4)
  	struct ath_buf *bf = bf_first, *bf_prev = NULL;

@@ -947,12 +964,13 @@ ath_tx_form_aggr(struct ath_softc *sc, struct ath_txq *txq,
  	struct ieee80211_tx_info *tx_info;
  	struct ath_frame_info *fi;
  	struct sk_buff *skb;
-	bool closed = false;
+
 
  	bf = bf_first;
  	aggr_limit = ath_lookup_rate(sc, bf, tid);
 
-	do {
+	while (bf)
+	{
  		skb = bf->bf_mpdu;
  		fi = get_frame_info(skb);
 

@@ -961,12 +979,12 @@ ath_tx_form_aggr(struct ath_softc *sc, struct ath_txq *txq,
  		if (nframes) {
  			if (aggr_limit < al + bpad + al_delta ||
  			    ath_lookup_legacy(bf) || nframes >= h_baw)
-				break;
+				goto stop;
 
  			tx_info = IEEE80211_SKB_CB(bf->bf_mpdu);
  			if ((tx_info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE) ||
  			    !(tx_info->flags & IEEE80211_TX_CTL_AMPDU))
-				break;
+				goto stop;
  		}
 
  		/* add padding for previous frame to aggregation length */

@@ -988,20 +1006,18 @@ ath_tx_form_aggr(struct ath_softc *sc, struct ath_txq *txq,
  			ath_tx_addto_baw(sc, tid, bf);
  		bf->bf_state.ndelim = ndelim;
 
-		__skb_unlink(skb, tid_q);
  		list_add_tail(&bf->list, bf_q);
  		if (bf_prev)
  			bf_prev->bf_next = bf;
 
  		bf_prev = bf;
 
-		bf = ath_tx_get_tid_subframe(sc, txq, tid, &tid_q);
-		if (!bf) {
-			closed = true;
-			break;
-		}
-	} while (ath_tid_has_buffered(tid));
-
+		bf = ath_tx_get_tid_subframe(sc, txq, tid);
+	}
+	goto finish;
+stop:
+	__skb_queue_tail(&tid->retry_q, bf->bf_mpdu);
+finish:
  	bf = bf_first;
  	bf->bf_lastbf = bf_prev;
 

@@ -1012,9 +1028,7 @@ ath_tx_form_aggr(struct ath_softc *sc, struct ath_txq *txq,
  		TX_STAT_INC(txq->axq_qnum, a_aggr);
  	}
 
-	*aggr_len = al;
-
-	return closed;
+	return al;
  #undef PADBYTES
  }
 

@@ -1391,18 +1405,15 @@ static void ath_tx_fill_desc(struct ath_softc *sc, struct ath_buf *bf,
  static void
  ath_tx_form_burst(struct ath_softc *sc, struct ath_txq *txq,
  		  struct ath_atx_tid *tid, struct list_head *bf_q,
-		  struct ath_buf *bf_first, struct sk_buff_head *tid_q)
+		  struct ath_buf *bf_first)
  {
  	struct ath_buf *bf = bf_first, *bf_prev = NULL;
-	struct sk_buff *skb;
  	int nframes = 0;
 
  	do {
  		struct ieee80211_tx_info *tx_info;
-		skb = bf->bf_mpdu;
 
  		nframes++;
-		__skb_unlink(skb, tid_q);
  		list_add_tail(&bf->list, bf_q);
  		if (bf_prev)
  			bf_prev->bf_next = bf;

@@ -1411,13 +1422,15 @@ ath_tx_form_burst(struct ath_softc *sc, struct ath_txq *txq,
  		if (nframes >= 2)
  			break;
 
-		bf = ath_tx_get_tid_subframe(sc, txq, tid, &tid_q);
+		bf = ath_tx_get_tid_subframe(sc, txq, tid);
  		if (!bf)
  			break;
 
  		tx_info = IEEE80211_SKB_CB(bf->bf_mpdu);
-		if (tx_info->flags & IEEE80211_TX_CTL_AMPDU)
+		if (tx_info->flags & IEEE80211_TX_CTL_AMPDU) {
+			__skb_queue_tail(&tid->retry_q, bf->bf_mpdu);
  			break;
+		}
 
  		ath_set_rates(tid->an->vif, tid->an->sta, bf);
  	} while (1);

@@ -1428,34 +1441,33 @@ static bool ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
  {
  	struct ath_buf *bf;
  	struct ieee80211_tx_info *tx_info;
-	struct sk_buff_head *tid_q;
  	struct list_head bf_q;
  	int aggr_len = 0;
-	bool aggr, last = true;
+	bool aggr;
 
  	if (!ath_tid_has_buffered(tid))
  		return false;
 
  	INIT_LIST_HEAD(&bf_q);
 
-	bf = ath_tx_get_tid_subframe(sc, txq, tid, &tid_q);
+	bf = ath_tx_get_tid_subframe(sc, txq, tid);
  	if (!bf)
  		return false;
 
  	tx_info = IEEE80211_SKB_CB(bf->bf_mpdu);
  	aggr = !!(tx_info->flags & IEEE80211_TX_CTL_AMPDU);
  	if ((aggr && txq->axq_ampdu_depth >= ATH_AGGR_MIN_QDEPTH) ||
-		(!aggr && txq->axq_depth >= ATH_NON_AGGR_MIN_QDEPTH)) {
+	    (!aggr && txq->axq_depth >= ATH_NON_AGGR_MIN_QDEPTH)) {
+		__skb_queue_tail(&tid->retry_q, bf->bf_mpdu);
  		*stop = true;
  		return false;
  	}
 
  	ath_set_rates(tid->an->vif, tid->an->sta, bf);
  	if (aggr)
-		last = ath_tx_form_aggr(sc, txq, tid, &bf_q, bf,
-					tid_q, &aggr_len);
+		aggr_len = ath_tx_form_aggr(sc, txq, tid, &bf_q, bf);
  	else
-		ath_tx_form_burst(sc, txq, tid, &bf_q, bf, tid_q);
+		ath_tx_form_burst(sc, txq, tid, &bf_q, bf);
 
  	if (list_empty(&bf_q))
  		return false;

@@ -1498,9 +1510,6 @@ int ath_tx_aggr_start(struct ath_softc *sc, struct ieee80211_sta *sta,
  		an->mpdudensity = density;
  	}
 
-	/* force sequence number allocation for pending frames */
-	ath_tx_tid_change_state(sc, txtid);
-
  	txtid->active = true;
  	*ssn = txtid->seq_start = txtid->seq_next;
  	txtid->bar_index = -1;

@@ -1525,7 +1534,6 @@ void ath_tx_aggr_stop(struct ath_softc *sc, struct ieee80211_sta *sta, u16 tid)
  	ath_txq_lock(sc, txq);
  	txtid->active = false;
  	ath_tx_flush_tid(sc, txtid);
-	ath_tx_tid_change_state(sc, txtid);
  	ath_txq_unlock_complete(sc, txq);
  }
 

@@ -1535,14 +1543,12 @@ void ath_tx_aggr_sleep(struct ieee80211_sta *sta, struct ath_softc *sc,
  	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
  	struct ath_atx_tid *tid;
  	struct ath_txq *txq;
-	bool buffered;
  	int tidno;
 
  	ath_dbg(common, XMIT, "%s called\n", __func__);
 
-	for (tidno = 0, tid = &an->tid[tidno];
-	     tidno < IEEE80211_NUM_TIDS; tidno++, tid++) {
-
+	for (tidno = 0; tidno < IEEE80211_NUM_TIDS; tidno++) {
+		tid = ATH_AN_2_TID(an, tidno);
  		txq = tid->txq;
 
  		ath_txq_lock(sc, txq);

@@ -1552,13 +1558,9 @@ void ath_tx_aggr_sleep(struct ieee80211_sta *sta, struct ath_softc *sc,
  			continue;
  		}
 
-		buffered = ath_tid_has_buffered(tid);
-
  		list_del_init(&tid->list);
 
  		ath_txq_unlock(sc, txq);
-
-		ieee80211_sta_set_buffered(sta, tidno, buffered);
  	}
  }
 

@@ -1571,19 +1573,12 @@ void ath_tx_aggr_wakeup(struct ath_softc *sc, struct ath_node *an)
 
  	ath_dbg(common, XMIT, "%s called\n", __func__);
 
-	for (tidno = 0, tid = &an->tid[tidno];
-	     tidno < IEEE80211_NUM_TIDS; tidno++, tid++) {
-
+	for (tidno = 0; tidno < IEEE80211_NUM_TIDS; tidno++) {
+		tid = ATH_AN_2_TID(an, tidno);
  		txq = tid->txq;
 
  		ath_txq_lock(sc, txq);
  		tid->clear_ps_filter = true;
-
-		if (ath_tid_has_buffered(tid)) {
-			ath_tx_queue_tid(sc, txq, tid);
-			ath_txq_schedule(sc, txq);
-		}
-
  		ath_txq_unlock_complete(sc, txq);
  	}
  }

@@ -1606,11 +1601,6 @@ void ath_tx_aggr_resume(struct ath_softc *sc, struct ieee80211_sta *sta,
 
  	tid->baw_size = IEEE80211_MIN_AMPDU_BUF << sta->ht_cap.ampdu_factor;
 
-	if (ath_tid_has_buffered(tid)) {
-		ath_tx_queue_tid(sc, txq, tid);
-		ath_txq_schedule(sc, txq);
-	}
-
  	ath_txq_unlock_complete(sc, txq);
  }
 

@@ -1626,7 +1616,6 @@ void ath9k_release_buffered_frames(struct ieee80211_hw *hw,
  	struct ieee80211_tx_info *info;
  	struct list_head bf_q;
  	struct ath_buf *bf_tail = NULL, *bf;
-	struct sk_buff_head *tid_q;
  	int sent = 0;
  	int i;
 

@@ -1641,11 +1630,10 @@ void ath9k_release_buffered_frames(struct ieee80211_hw *hw,
 
  		ath_txq_lock(sc, tid->txq);
  		while (nframes > 0) {
-			bf = ath_tx_get_tid_subframe(sc, sc->tx.uapsdq, tid, &tid_q);
+			bf = ath_tx_get_tid_subframe(sc, sc->tx.uapsdq, tid);
  			if (!bf)
  				break;
 
-			__skb_unlink(bf->bf_mpdu, tid_q);
  			list_add_tail(&bf->list, &bf_q);
  			ath_set_rates(tid->an->vif, tid->an->sta, bf);
  			if (bf_isampdu(bf)) {

@@ -1660,7 +1648,7 @@ void ath9k_release_buffered_frames(struct ieee80211_hw *hw,
  			sent++;
  			TX_STAT_INC(txq->axq_qnum, a_queued_hw);
 
-			if (an->sta && !ath_tid_has_buffered(tid))
+			if (an->sta && skb_queue_empty(&tid->retry_q))
  				ieee80211_sta_set_buffered(an->sta, i, false);
  		}
  		ath_txq_unlock_complete(sc, tid->txq);

@@ -1887,13 +1875,7 @@ bool ath_drain_all_txq(struct ath_softc *sc)
  		if (!ATH_TXQ_SETUP(sc, i))
  			continue;
 
-		/*
-		 * The caller will resume queues with ieee80211_wake_queues.
-		 * Mark the queue as not stopped to prevent ath_tx_complete
-		 * from waking the queue too early.
-		 */
  		txq = &sc->tx.txq[i];
-		txq->stopped = false;
  		ath_draintxq(sc, txq);
  	}
 

@@ -2293,15 +2275,12 @@ int ath_tx_start(struct ieee80211_hw *hw, struct sk_buff *skb,
  	struct ath_txq *txq = txctl->txq;
  	struct ath_atx_tid *tid = NULL;
  	struct ath_buf *bf;
-	bool queue, skip_uapsd = false, ps_resp;
+	bool ps_resp;
  	int q, ret;
 
  	if (vif)
  		avp = (void *)vif->drv_priv;
 
-	if (info->flags & IEEE80211_TX_CTL_TX_OFFCHAN)
-		txctl->force_channel = true;
-
  	ps_resp = !!(info->control.flags & IEEE80211_TX_CTRL_PS_RESPONSE);
 
  	ret = ath_tx_prepare(hw, skb, txctl);

@@ -2316,63 +2295,13 @@ int ath_tx_start(struct ieee80211_hw *hw, struct sk_buff *skb,
 
  	q = skb_get_queue_mapping(skb);
 
+	if (ps_resp)
+		txq = sc->tx.uapsdq;
+
  	ath_txq_lock(sc, txq);
  	if (txq == sc->tx.txq_map[q]) {
  		fi->txq = q;
-		if (++txq->pending_frames > sc->tx.txq_max_pending[q] &&
-		    !txq->stopped) {
-			if (ath9k_is_chanctx_enabled())
-				ieee80211_stop_queue(sc->hw, info->hw_queue);
-			else
-				ieee80211_stop_queue(sc->hw, q);
-			txq->stopped = true;
-		}
-	}
-
-	queue = ieee80211_is_data_present(hdr->frame_control);
-
-	/* If chanctx, queue all null frames while NOA could be there */
-	if (ath9k_is_chanctx_enabled() &&
-	    ieee80211_is_nullfunc(hdr->frame_control) &&
-	    !txctl->force_channel)
-		queue = true;
-
-	/* Force queueing of all frames that belong to a virtual interface on
-	 * a different channel context, to ensure that they are sent on the
-	 * correct channel.
-	 */
-	if (((avp && avp->chanctx != sc->cur_chan) ||
-	     sc->cur_chan->stopped) && !txctl->force_channel) {
-		if (!txctl->an)
-			txctl->an = &avp->mcast_node;
-		queue = true;
-		skip_uapsd = true;
-	}
-
-	if (txctl->an && queue)
-		tid = ath_get_skb_tid(sc, txctl->an, skb);
-
-	if (!skip_uapsd && ps_resp) {
-		ath_txq_unlock(sc, txq);
-		txq = sc->tx.uapsdq;
-		ath_txq_lock(sc, txq);
-	} else if (txctl->an && queue) {
-		WARN_ON(tid->txq != txctl->txq);
-
-		if (info->flags & IEEE80211_TX_CTL_CLEAR_PS_FILT)
-			tid->clear_ps_filter = true;
-
-		/*
-		 * Add this frame to software queue for scheduling later
-		 * for aggregation.
-		 */
-		TX_STAT_INC(txq->axq_qnum, a_queued_sw);
-		__skb_queue_tail(&tid->buf_q, skb);
-		if (!txctl->an->sleeping)
-			ath_tx_queue_tid(sc, txq, tid);
-
-		ath_txq_schedule(sc, txq);
-		goto out;
+		++txq->pending_frames;
  	}
 
  	bf = ath_tx_setup_buffer(sc, txq, tid, skb);

@@ -2856,9 +2785,8 @@ void ath_tx_node_init(struct ath_softc *sc, struct ath_node *an)
  	struct ath_atx_tid *tid;
  	int tidno, acno;
 
-	for (tidno = 0, tid = &an->tid[tidno];
-	     tidno < IEEE80211_NUM_TIDS;
-	     tidno++, tid++) {
+	for (tidno = 0; tidno < IEEE80211_NUM_TIDS; tidno++) {
+		tid = ATH_AN_2_TID(an, tidno);
  		tid->an        = an;
  		tid->tidno     = tidno;
  		tid->seq_start = tid->seq_next = 0;

@@ -2866,11 +2794,14 @@ void ath_tx_node_init(struct ath_softc *sc, struct ath_node *an)
  		tid->baw_head  = tid->baw_tail = 0;
  		tid->active	   = false;
  		tid->clear_ps_filter = true;
-		__skb_queue_head_init(&tid->buf_q);
+		tid->has_queued  = false;
  		__skb_queue_head_init(&tid->retry_q);
  		INIT_LIST_HEAD(&tid->list);
  		acno = TID_TO_WME_AC(tidno);
  		tid->txq = sc->tx.txq_map[acno];
+
+		if (!an->sta)
+			break; /* just one multicast ath_atx_tid */
  	}
  }
 

@@ -2880,9 +2811,8 @@ void ath_tx_node_cleanup(struct ath_softc *sc, struct ath_node *an)
  	struct ath_txq *txq;
  	int tidno;
 
-	for (tidno = 0, tid = &an->tid[tidno];
-	     tidno < IEEE80211_NUM_TIDS; tidno++, tid++) {
-
+	for (tidno = 0; tidno < IEEE80211_NUM_TIDS; tidno++) {
+		tid = ATH_AN_2_TID(an, tidno);
  		txq = tid->txq;
 
  		ath_txq_lock(sc, txq);

@@ -2894,6 +2824,9 @@ void ath_tx_node_cleanup(struct ath_softc *sc, struct ath_node *an)
  		tid->active = false;
 
  		ath_txq_unlock(sc, txq);
+
+		if (!an->sta)
+			break; /* just one multicast ath_atx_tid */
  	}
  }
 

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

---
Changes since v1:
- Remove the old intermediate queueing logic completely instead of just disabling it.
- Remove the qlen debug tunables.
- Remove the force_channel parameter from struct txctl (since we just removed the code path that was using it).

 drivers/net/wireless/ath/ath9k/ath9k.h     |  12 +-
 drivers/net/wireless/ath/ath9k/channel.c   |   2 -
 drivers/net/wireless/ath/ath9k/debug.c     |  14 +-
 drivers/net/wireless/ath/ath9k/debug.h     |   2 -
 drivers/net/wireless/ath/ath9k/debug_sta.c |   4 +-
 drivers/net/wireless/ath/ath9k/init.c      |   2 +-
 drivers/net/wireless/ath/ath9k/main.c      |   1 +
 drivers/net/wireless/ath/ath9k/xmit.c      | 307 +++++++++++------------------
 8 files changed, 130 insertions(+), 214 deletions(-)

drivers/net/wireless/ath/ath9k/xmit.c +0 -0

index fe795fc..4077eeb 100644
@@ -912,8 +923,16 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
 		seqno = bf->bf_state.seqno;
 

Quoted 23 lines · Toke Høiland-Jørgensen

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke Høiland-Jørgensen <[email protected]>

---
Changes since v1:
- Remove the old intermediate queueing logic completely instead of just disabling it.
- Remove the qlen debug tunables.
- Remove the force_channel parameter from struct txctl (since we just removed the code path that was using it).

Quoted 9 lines

 drivers/net/wireless/ath/ath9k/ath9k.h     |  12 +-
 drivers/net/wireless/ath/ath9k/channel.c   |   2 -
 drivers/net/wireless/ath/ath9k/debug.c     |  14 +-
 drivers/net/wireless/ath/ath9k/debug.h     |   2 -
 drivers/net/wireless/ath/ath9k/debug_sta.c |   4 +-
 drivers/net/wireless/ath/ath9k/init.c      |   2 +-
 drivers/net/wireless/ath/ath9k/main.c      |   1 +
 drivers/net/wireless/ath/ath9k/xmit.c      | 307 +++++++++++------------------
 8 files changed, 130 insertions(+), 214 deletions(-)

Nice work!

Quoted 7 lines

drivers/net/wireless/ath/ath9k/xmit.c +0 -0

index fe795fc..4077eeb 100644
@@ -912,8 +923,16 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
 		seqno = bf->bf_state.seqno;
 

drivers/net/wireless/ath/ath9k/xmit.c ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq, · line 933

Quoted 10 lines

drivers/net/wireless/ath/ath9k/xmit.c +8 -1

 		/* do not step over block-ack window */
-		if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno))
+		if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno)) {
+			__skb_queue_tail(&tid->retry_q, skb);
+
+			/* If there are other skbs in the retry q, they are
+			 * probably within the BAW, so loop immediately to get
+			 * one of them. Otherwise the queue can get stuck. */
+			if (!skb_queue_is_first(&tid->retry_q, skb))
+				continue;

Not sure if this can happen, but if we ever somehow end up with two skbs in the retry queue that do not fit into the Block-Ack window, there's potential for an infinite loop here.

Quoted 33 lines · Felix Fietkau

Quoted 23 lines · Toke H?iland-J?rgensen

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

---
Changes since v1:
- Remove the old intermediate queueing logic completely instead of just disabling it.
- Remove the qlen debug tunables.
- Remove the force_channel parameter from struct txctl (since we just removed the code path that was using it).

Quoted 9 lines

 drivers/net/wireless/ath/ath9k/ath9k.h     |  12 +-
 drivers/net/wireless/ath/ath9k/channel.c   |   2 -
 drivers/net/wireless/ath/ath9k/debug.c     |  14 +-
 drivers/net/wireless/ath/ath9k/debug.h     |   2 -
 drivers/net/wireless/ath/ath9k/debug_sta.c |   4 +-
 drivers/net/wireless/ath/ath9k/init.c      |   2 +-
 drivers/net/wireless/ath/ath9k/main.c      |   1 +
 drivers/net/wireless/ath/ath9k/xmit.c      | 307 +++++++++++------------------
 8 files changed, 130 insertions(+), 214 deletions(-)

Quoted 1 line

Nice work!

Thanks :)

Quoted 7 lines

Quoted 7 lines

drivers/net/wireless/ath/ath9k/xmit.c +0 -0

index fe795fc..4077eeb 100644
@@ -912,8 +923,16 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
 		seqno = bf->bf_state.seqno;
 

Quoted 13 lines

drivers/net/wireless/ath/ath9k/xmit.c ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq, · line 933

Quoted 10 lines

drivers/net/wireless/ath/ath9k/xmit.c +8 -1

 		/* do not step over block-ack window */
-		if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno))
+		if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno)) {
+			__skb_queue_tail(&tid->retry_q, skb);
+
+			/* If there are other skbs in the retry q, they are
+			 * probably within the BAW, so loop immediately to get
+			 * one of them. Otherwise the queue can get stuck. */
+			if (!skb_queue_is_first(&tid->retry_q, skb))
+				continue;

Not sure if this can happen, but if we ever somehow end up with two skbs in the retry queue that do not fit into the Block-Ack window, there's potential for an infinite loop here.

Yes, I realise that (v1 contained a comment on that). However, I don't actually think it can happen: The code will only ever put one skb from the intermediate queues on the retry queue (ath_tid_pull() is only called if the retry queue is empty). Everything else on there are actual retries that have been put on there by ath_tx_complete_aggr(). Figure the latter will always be within the BAW?

Quoted 60 lines · Toke Høiland-Jørgensen

Quoted 33 lines · Felix Fietkau

Quoted 23 lines · Toke Høiland-Jørgensen

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke Høiland-Jørgensen <[email protected]>

---
Changes since v1:
- Remove the old intermediate queueing logic completely instead of just disabling it.
- Remove the qlen debug tunables.
- Remove the force_channel parameter from struct txctl (since we just removed the code path that was using it).

Quoted 9 lines

 drivers/net/wireless/ath/ath9k/ath9k.h     |  12 +-
 drivers/net/wireless/ath/ath9k/channel.c   |   2 -
 drivers/net/wireless/ath/ath9k/debug.c     |  14 +-
 drivers/net/wireless/ath/ath9k/debug.h     |   2 -
 drivers/net/wireless/ath/ath9k/debug_sta.c |   4 +-
 drivers/net/wireless/ath/ath9k/init.c      |   2 +-
 drivers/net/wireless/ath/ath9k/main.c      |   1 +
 drivers/net/wireless/ath/ath9k/xmit.c      | 307 +++++++++++------------------
 8 files changed, 130 insertions(+), 214 deletions(-)

Quoted 1 line

Nice work!

Thanks :)

Quoted 7 lines

Quoted 7 lines

drivers/net/wireless/ath/ath9k/xmit.c +0 -0

index fe795fc..4077eeb 100644
@@ -912,8 +923,16 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
 		seqno = bf->bf_state.seqno;
 

Quoted 13 lines

drivers/net/wireless/ath/ath9k/xmit.c ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq, · line 933

Quoted 10 lines

drivers/net/wireless/ath/ath9k/xmit.c +8 -1

 		/* do not step over block-ack window */
-		if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno))
+		if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno)) {
+			__skb_queue_tail(&tid->retry_q, skb);
+
+			/* If there are other skbs in the retry q, they are
+			 * probably within the BAW, so loop immediately to get
+			 * one of them. Otherwise the queue can get stuck. */
+			if (!skb_queue_is_first(&tid->retry_q, skb))
+				continue;

Not sure if this can happen, but if we ever somehow end up with two skbs in the retry queue that do not fit into the Block-Ack window, there's potential for an infinite loop here.

Quoted 6 lines

Yes, I realise that (v1 contained a comment on that). However, I don't actually think it can happen: The code will only ever put one skb from the intermediate queues on the retry queue (ath_tid_pull() is only called if the retry queue is empty). Everything else on there are actual retries that have been put on there by ath_tx_complete_aggr(). Figure the latter will always be within the BAW?

I think it would be a good idea to have a check there (with WARN_ON), in case there's some weird corner case with seqno handling, software retry and aggregation state changes.

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

Quoted 10 lines · Toke H?iland-J?rgensen

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Quoted 5 lines

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

Nice work.

Because this is such a significant change, and to maximise testing time, I'm planning to queue this for 4.9 (so I would apply this to ath-next in 3-4 weeks after the merge window closes). But anyone who wants to test this can use master-pending branch from my ath.git tree (uses wireless-testing as the baseline). Sounds good?

Testing and review feedback very welcome!

Quoted 11 lines · Kalle Valo

Quoted 10 lines · Toke H?iland-J?rgensen

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Quoted 7 lines

Quoted 5 lines

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

Nice work.

Thanks :)

Quoted 5 lines

Because this is such a significant change, and to maximise testing time, I'm planning to queue this for 4.9 (so I would apply this to ath-next in 3-4 weeks after the merge window closes). But anyone who wants to test this can use master-pending branch from my ath.git tree (uses wireless-testing as the baseline). Sounds good?

Sounds good to me. I'm planning on backporting this and Michael's mac80211 FQ-CoDel patches to 4.4 and post them for inclusion in LEDE. Hopefully that will get it some more testing as well.

Quoted 29 lines · Toke H?iland-J?rgensen

Quoted 11 lines · Kalle Valo

Quoted 10 lines · Toke H?iland-J?rgensen

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Quoted 7 lines

Quoted 5 lines

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

Nice work.

Thanks :)

Quoted 5 lines

Because this is such a significant change, and to maximise testing time, I'm planning to queue this for 4.9 (so I would apply this to ath-next in 3-4 weeks after the merge window closes). But anyone who wants to test this can use master-pending branch from my ath.git tree (uses wireless-testing as the baseline). Sounds good?

Quoted 3 lines

Sounds good to me. I'm planning on backporting this and Michael's mac80211 FQ-CoDel patches to 4.4 and post them for inclusion in LEDE. Hopefully that will get it some more testing as well.

I've pushed a backport of this into my LEDE staging tree:
https://git.lede-project.org/?p=lede/nbd/staging.git;a=summary

I don't have time for testing it myself at the moment, but I'll try to get some people to do so.

Quoted 30 lines · Felix Fietkau

Quoted 29 lines · Toke H?iland-J?rgensen

Quoted 11 lines · Kalle Valo

Quoted 10 lines · Toke H?iland-J?rgensen

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Quoted 7 lines

Quoted 5 lines

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

Nice work.

Thanks :)

Quoted 5 lines

Because this is such a significant change, and to maximise testing time, I'm planning to queue this for 4.9 (so I would apply this to ath-next in 3-4 weeks after the merge window closes). But anyone who wants to test this can use master-pending branch from my ath.git tree (uses wireless-testing as the baseline). Sounds good?

Quoted 5 lines

Quoted 3 lines

Sounds good to me. I'm planning on backporting this and Michael's mac80211 FQ-CoDel patches to 4.4 and post them for inclusion in LEDE. Hopefully that will get it some more testing as well.

I've pushed a backport of this into my LEDE staging tree:
https://git.lede-project.org/?p=lede/nbd/staging.git;a=summary

Awesome! What about the FQ-CoDel mac80211 patches themselves? I have a tree where I've separated out the needed patches and rebased them on mainline 4.4.9. Can I post that somewhere (or just email you the series) and get you to include those as well? Or do I just dump the patch files into the LEDE patches dir and send that as a patch to LEDE? (I see your patch also refreshed subsequent patches; is there a script to do that automatically?)

Quoted 38 lines · Toke Høiland-Jørgensen

Quoted 30 lines · Felix Fietkau

Quoted 29 lines · Toke Høiland-Jørgensen

Quoted 11 lines · Kalle Valo

Quoted 10 lines · Toke Høiland-Jørgensen

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Quoted 7 lines

Quoted 5 lines

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke Høiland-Jørgensen <[email protected]>

Nice work.

Thanks :)

Quoted 5 lines

Because this is such a significant change, and to maximise testing time, I'm planning to queue this for 4.9 (so I would apply this to ath-next in 3-4 weeks after the merge window closes). But anyone who wants to test this can use master-pending branch from my ath.git tree (uses wireless-testing as the baseline). Sounds good?

Quoted 5 lines

Quoted 3 lines

Sounds good to me. I'm planning on backporting this and Michael's mac80211 FQ-CoDel patches to 4.4 and post them for inclusion in LEDE. Hopefully that will get it some more testing as well.

I've pushed a backport of this into my LEDE staging tree:
https://git.lede-project.org/?p=lede/nbd/staging.git;a=summary

Quoted 7 lines

Awesome! What about the FQ-CoDel mac80211 patches themselves? I have a tree where I've separated out the needed patches and rebased them on mainline 4.4.9. Can I post that somewhere (or just email you the series) and get you to include those as well? Or do I just dump the patch files into the LEDE patches dir and send that as a patch to LEDE? (I see your patch also refreshed subsequent patches; is there a script to do that automatically?)

You don't need to do anything here. LEDE does not use mac80211 and drivers from the kernel tree, it's built using backports. It's currently using a backports snapshot that I built myself from wireless-testing 2016-06-20, which already includes FQ-Codel.

Quoted 34 lines

Quoted 27 lines · Felix Fietkau

Quoted 26 lines · Toke H?iland-J?rgensen

Quoted 11 lines · Kalle Valo

Quoted 10 lines · Toke H?iland-J?rgensen

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Quoted 6 lines

Quoted 5 lines

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

Nice work.

Thanks :)

Quoted 5 lines

Because this is such a significant change, and to maximise testing time, I'm planning to queue this for 4.9 (so I would apply this to ath-next in 3-4 weeks after the merge window closes). But anyone who wants to test this can use master-pending branch from my ath.git tree (uses wireless-testing as the baseline). Sounds good?

Quoted 5 lines

Quoted 3 lines

Sounds good to me. I'm planning on backporting this and Michael's mac80211 FQ-CoDel patches to 4.4 and post them for inclusion in LEDE. Hopefully that will get it some more testing as well.

I've pushed a backport of this into my LEDE staging tree:
https://git.lede-project.org/?p=lede/nbd/staging.git;a=summary

Quoted 7 lines

Awesome! What about the FQ-CoDel mac80211 patches themselves? I have a tree where I've separated out the needed patches and rebased them on mainline 4.4.9. Can I post that somewhere (or just email you the series) and get you to include those as well? Or do I just dump the patch files into the LEDE patches dir and send that as a patch to LEDE? (I see your patch also refreshed subsequent patches; is there a script to do that automatically?)

You don't need to do anything here. LEDE does not use mac80211 and drivers from the kernel tree, it's built using backports. It's currently using a backports snapshot that I built myself from wireless-testing 2016-06-20, which already includes FQ-Codel.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

for me it crashes on wds sta on 3.18 kernels.

need to solder a serial to get more logs

The old code path in ath_tx_start that would queue packets has been removed completely,

as has the qlen limit tunables (since there's no
longer a queue in the driver to limit).

drivers/net/wireless/ath/ath9k/ath9k.h +0 -0

index 5294595..daf972c 100644

Quoted 2 lines

The old code path in ath_tx_start that would queue packets has been removed completely,

It seems to me that this breaks the ath9k driver when non-data packets which mac80211 will not queue on the new intermediate queues, see ieee80211_drv_tx( ) in mac80211/tx.c where it says

	if (!ieee80211_is_data(hdr->frame_control))
		goto tx_normal;

This means that non-data packets can come down from mac80211 via ath_tx --> ath_tx_start which might be for a vif that is not on the same channel, and so cannot be sent straight away but must be queued. Maybe this problem should be fixed in mac80211, but as of now this is
a problem. It appears to me that your new patch just sends them on
the wrong channel.

Quoted 8 lines

as has the qlen limit tunables (since there's no
longer a queue in the driver to limit).

drivers/net/wireless/ath/ath9k/ath9k.h +0 -0

index 5294595..daf972c 100644

drivers/net/wireless/ath/ath9k/ath9k.h int ath_descdma_setup(struct ath_softc *sc, struct ath_descdma *dd, · line 95

Quoted 6 lines

drivers/net/wireless/ath/ath9k/ath9k.h +0 -1

@@ -91,7 +91,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct ath_descdma *dd,
 #define ATH_RXBUF               512
 #define ATH_TXBUF               512
 #define ATH_TXBUF_RESERVE       5
-#define ATH_MAX_QDEPTH          (ATH_TXBUF / 4 - ATH_TXBUF_RESERVE)
 #define ATH_TXMAXTRY            13
 #define ATH_MAX_SW_RETRIES      30

I thought the purpose of ATH_MAX_QDEPTH was due to a limit on the depth of a hardware FIFO.

Not all packets that get handed to the hardware come through a software queue (e.g. those that bypass the intermediate queueing in mac80211 now) and (it seems to me) there needs to be a limit to prevent overflowing a hardware fifo. Yes, normal data packets are (much) further limited as you taught me a few weeks ago, but not all packets are subject to that constraint (as far as I can understand at the moment). I'm not entirely sure of the details, but I think it is the sorts of packets sent directly by hostapd and wpa_supplicant that bypass all the queueing. And maybe those things aren't likely to be

sending a burst of hundreds of packets in a very short period of time (where it could overrun the FIFO), but there may be other tools that send raw 802.11 non-data packets which could then overflow the above limit, and it seems you are removing the check. Actually I think my original version of this patch may have had a flaw in that some combination of non-data and data packets could be combined to overflow this limit (since I failed to check overflowing this limit where I pulled from the mac80211 intermediate queue).

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

---
Changes since v3 (most due to Felix; thanks!):
- Correctly notify mac80211 when there are packets in the retry queue on powersave start/stop.
- Get rid of ath_tx_aggr_resume().
- Some readability changes and additional WARN_ON/BUG_ON in appropriate places.

Quoted 15 lines · Toke H=C3=B8iland-J=C3=B8rgensen <[email protected]> writes:

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H=C3=B8iland-J=C3=B8rgensen <[email protected]>

Quoted 7 lines

---
Changes since v3 (most due to Felix; thanks!):
- Correctly notify mac80211 when there are packets in the retry queue on powersave start/stop.
- Get rid of ath_tx_aggr_resume().
- Some readability changes and additional WARN_ON/BUG_ON in appropriate places.

This is great work but due to the regressions I'm not sure if this will be ready for 4.9. To get more testing time I wonder if we should wait for 4.10? IMHO applying this in the end of the cycle is too risky and we should try to maximise the time linux-next by applying this just after -rc1 is released.

Thoughts?

Quoted 25 lines · Kalle Valo

Quoted 15 lines · Toke H?iland-J?rgensen

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 when a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H?iland-J?rgensen <[email protected]>

Quoted 7 lines

---
Changes since v3 (most due to Felix; thanks!):
- Correctly notify mac80211 when there are packets in the retry queue on powersave start/stop.
- Get rid of ath_tx_aggr_resume().
- Some readability changes and additional WARN_ON/BUG_ON in appropriate places.

Quoted 7 lines

This is great work but due to the regressions I'm not sure if this will be ready for 4.9. To get more testing time I wonder if we should wait for 4.10? IMHO applying this in the end of the cycle is too risky and we should try to maximise the time linux-next by applying this just after -rc1 is released.

Thoughts?

Well, now that we understand what is causing the throughput regressions, fixing them should be fairly straight forward (yeah, famous last words, but still...). I already have a patch for the fast path and will go poke at the slow path next. It'll probably require another workaround or two, so I guess it won't be the architecturally clean ideal solution; but it would make it possible to have something that works for 4.9 and then iterate for a cleaner design for 4.10.

Quoted 7 lines · Toke H=C3=B8iland-J=C3=B8rgensen <[email protected]> writes:

Quoted 5 lines · Kalle Valo

Quoted 3 lines · Toke H=C3=B8iland-J=C3=B8rgensen <[email protected]> writes:

This switches ath9k over to using the mac80211 intermediate software queueing mechanism for data packets. It removes the queueing inside the driver, except for the retry queue, and instead pulls from mac80211 wh=

Quoted 28 lines

Quoted 20 lines

Quoted 12 lines

a packet is needed. The retry queue is used to store a packet that was pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been removed completely, as has the qlen limit tunables (since there's no longer a queue in the driver to limit).

Based on Tim's original patch set, but reworked quite thoroughly.

Cc Tim Shepard <[email protected]>

Cc Felix Fietkau <[email protected]>

Signed-off-by Toke H=C3=B8iland-J=C3=B8rgensen <[email protected]>

Quoted 7 lines

---
Changes since v3 (most due to Felix; thanks!):
- Correctly notify mac80211 when there are packets in the retry queue on powersave start/stop.
- Get rid of ath_tx_aggr_resume().
- Some readability changes and additional WARN_ON/BUG_ON in appropriate places.

Quoted 7 lines

This is great work but due to the regressions I'm not sure if this will be ready for 4.9. To get more testing time I wonder if we should wait for 4.10? IMHO applying this in the end of the cycle is too risky and we should try to maximise the time linux-next by applying this just after -rc1 is released.

Thoughts?

Quoted 7 lines

Well, now that we understand what is causing the throughput regressions, fixing them should be fairly straight forward (yeah, famous last words, but still...). I already have a patch for the fast path and will go poke at the slow path next. It'll probably require another workaround or two, so I guess it won't be the architecturally clean ideal solution; but it would make it possible to have something that works for 4.9 and then iterate for a cleaner design for 4.10.

But if we try to rush this to 4.9 it won't be in linux-next for long. We are now in -rc3 and let's say that the patches are ready to apply in two weeks. That would leave us only two weeks of -next time before the merge window, which I think is not enough for a controversial patch like this one. There might be other bugs lurking which haven't been found yet.

Quoted 16 lines

Quoted 15 lines

Quoted 7 lines

This is great work but due to the regressions I'm not sure if this will be ready for 4.9. To get more testing time I wonder if we should wait for 4.10? IMHO applying this in the end of the cycle is too risky and we should try to maximise the time linux-next by applying this just after -rc1 is released.

Thoughts?

Well, now that we understand what is causing the throughput regressions, fixing them should be fairly straight forward (yeah, famous last words, but still...). I already have a patch for the fast path and will go poke at the slow path next. It'll probably require another workaround or two, so I guess it won't be the architecturally clean ideal solution; but it would make it possible to have something that works for 4.9 and then iterate for a cleaner design for 4.10.

Quoted 5 lines

But if we try to rush this to 4.9 it won't be in linux-next for long. We are now in -rc3 and let's say that the patches are ready to apply in two weeks. That would leave us only two weeks of -next time before the merge window, which I think is not enough for a controversial patch like this one. There might be other bugs lurking which haven't been found yet.

What, other hidden bugs? Unpossible! :)

Would it be possible to merge the partial solution (which is ready now, basically) and fix the slow path in a separate patch later?

(Just spit-balling here; I'm still fairly new to this process. But I am concerned that we'll hit a catch-22 where we can't get wider testing before it's "ready" and we can't prove that it's "ready" until we've had wider testing...)

Quoted 16 lines · Toke Høiland-Jørgensen

Quoted 16 lines

Quoted 15 lines

Quoted 7 lines

This is great work but due to the regressions I'm not sure if this will be ready for 4.9. To get more testing time I wonder if we should wait for 4.10? IMHO applying this in the end of the cycle is too risky and we should try to maximise the time linux-next by applying this just after -rc1 is released.

Thoughts?

Well, now that we understand what is causing the throughput regressions, fixing them should be fairly straight forward (yeah, famous last words, but still...). I already have a patch for the fast path and will go poke at the slow path next. It'll probably require another workaround or two, so I guess it won't be the architecturally clean ideal solution; but it would make it possible to have something that works for 4.9 and then iterate for a cleaner design for 4.10.

Quoted 7 lines

Quoted 5 lines

But if we try to rush this to 4.9 it won't be in linux-next for long. We are now in -rc3 and let's say that the patches are ready to apply in two weeks. That would leave us only two weeks of -next time before the merge window, which I think is not enough for a controversial patch like this one. There might be other bugs lurking which haven't been found yet.

What, other hidden bugs? Unpossible! :)

Yeah, right ;)

Quoted 2 lines

Would it be possible to merge the partial solution (which is ready now, basically) and fix the slow path in a separate patch later?

What do you mean with partial solution? You mean ath9k users would suffer from regressions until they are fixed? We can't do that.

Quoted 4 lines

(Just spit-balling here; I'm still fairly new to this process. But I am concerned that we'll hit a catch-22 where we can't get wider testing before it's "ready" and we can't prove that it's "ready" until we've had wider testing...)

I understand your point, but I don't want to rush this to 4.9 and then start getting lots of bug reports and eventually forced to revert it. If we just found a new serious regression the chances are that there are more lurking somewhere and this patch is just not ready yet.

Quoted 9 lines · Toke H=C3=B8iland-J=C3=B8rgensen <[email protected]> writes:

Quoted 9 lines

Quoted 9 lines

Quoted 7 lines

This is great work but due to the regressions I'm not sure if this will be ready for 4.9. To get more testing time I wonder if we should wait for 4.10? IMHO applying this in the end of the cycle is too risky and we should try to maximise the time linux-next by applying this just after -rc1 is released.

Thoughts?

Well, now that we understand what is causing the throughput regression=

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

fixing them should be fairly straight forward (yeah, famous last words, but still...). I already have a patch for the fast path and will go po=

Quoted 1 line

Quoted 1 line

Quoted 1 line

at the slow path next. It'll probably require another workaround or tw=

Quoted 11 lines

Quoted 9 lines

Quoted 3 lines

so I guess it won't be the architecturally clean ideal solution; but it would make it possible to have something that works for 4.9 and then iterate for a cleaner design for 4.10.

But if we try to rush this to 4.9 it won't be in linux-next for long. We are now in -rc3 and let's say that the patches are ready to apply in two weeks. That would leave us only two weeks of -next time before the merge window, which I think is not enough for a controversial patch like this one. There might be other bugs lurking which haven't been found yet.

What, other hidden bugs? Unpossible! :)

Yeah, right ;)

Quoted 2 lines

Would it be possible to merge the partial solution (which is ready now, basically) and fix the slow path in a separate patch later?

What do you mean with partial solution? You mean ath9k users would suffer from regressions until they are fixed? We can't do that.

Quoted 4 lines

(Just spit-balling here; I'm still fairly new to this process. But I am concerned that we'll hit a catch-22 where we can't get wider testing before it's "ready" and we can't prove that it's "ready" until we've had wider testing...)

I understand your point, but I don't want to rush this to 4.9 and then start getting lots of bug reports and eventually forced to revert it. If we just found a new serious regression the chances are that there are more lurking somewhere and this patch is just not ready yet.

Quoted 6 lines · Kalle Valo

Toke H?iland-J?rgensen <[email protected]> writes:

I understand your point, but I don't want to rush this to 4.9 and then start getting lots of bug reports and eventually forced to revert it. If we just found a new serious regression the chances are that there are more lurking somewhere and this patch is just not ready yet.

So, the changes to mac80211 that fixes the known regressions of this patch have gone in.

Any chance of seeing this merged during the current merge window? :)

Quoted 9 lines · Toke H=C3=B8iland-J=C3=B8rgensen <[email protected]> writes:

Quoted 6 lines · Kalle Valo

Toke H=C3=B8iland-J=C3=B8rgensen <[email protected]> writes:

I understand your point, but I don't want to rush this to 4.9 and then start getting lots of bug reports and eventually forced to revert it. If we just found a new serious regression the chances are that there are more lurking somewhere and this patch is just not ready yet.

Quoted 2 lines

So, the changes to mac80211 that fixes the known regressions of this patch have gone in.

I guess you mean this commit:

bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ de=

(Just making sure that I have the same commit in my tree when I apply this)

Quoted 1 line

Any chance of seeing this merged during the current merge window? :)

I sent last new feature ("-next") patches for 4.9 last week, sorry. So this has to wait for 4.10.

And I assume I need to take v5:

https://patchwork.kernel.org/patch/9311037/

Quoted 14 lines · Kalle Valo

Quoted 11 lines · Toke H?iland-J?rgensen

Quoted 6 lines · Kalle Valo

Toke H?iland-J?rgensen <[email protected]> writes:

I understand your point, but I don't want to rush this to 4.9 and then start getting lots of bug reports and eventually forced to revert it. If we just found a new serious regression the chances are that there are more lurking somewhere and this patch is just not ready yet.

So, the changes to mac80211 that fixes the known regressions of this patch have gone in.

Quoted 6 lines

I guess you mean this commit:

bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

(Just making sure that I have the same commit in my tree when I apply this)

Yup, that's the one :)

Quoted 2 lines

Quoted 1 line

Any chance of seeing this merged during the current merge window? :)

Quoted 2 lines

I sent last new feature ("-next") patches for 4.9 last week, sorry. So this has to wait for 4.10.

Ah, right, I think I got my merge windows confused. You already said you wouldn't take it for 4.9. So I guess what I'm asking is for you to put it into the appropriate -next tree so it can get some wider exposure ahead of the *next* merge window...

Quoted 3 lines

And I assume I need to take v5:

https://patchwork.kernel.org/patch/9311037/

Yes. Haven't noticed anything that changed since that might conflict with it, but let me know if I missed something and you want a refreshed version.

Quoted 2 lines · Toke H=C3=B8iland-J=C3=B8rgensen <[email protected]> writes:

Kalle Valo <[email protected]> writes:

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines · Toke H=C3=B8iland-J=C3=B8rgensen <[email protected]> writes:

Quoted 4 lines · Kalle Valo

Toke H=C3=B8iland-J=C3=B8rgensen <[email protected]> writes:

I understand your point, but I don't want to rush this to 4.9 and then start getting lots of bug reports and eventually forced to revert it.=

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

we just found a new serious regression the chances are that there are more lurking somewhere and this patch is just not ready yet.

So, the changes to mac80211 that fixes the known regressions of this patch have gone in.

I guess you mean this commit:

bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ =

Quoted 8 lines

Quoted 3 lines

(Just making sure that I have the same commit in my tree when I apply this)

Yup, that's the one :)

Quoted 2 lines

Quoted 1 line

Any chance of seeing this merged during the current merge window? :)

Quoted 7 lines

Quoted 2 lines

I sent last new feature ("-next") patches for 4.9 last week, sorry. So this has to wait for 4.10.

Ah, right, I think I got my merge windows confused. You already said you wouldn't take it for 4.9. So I guess what I'm asking is for you to put it into the appropriate -next tree so it can get some wider exposure ahead of the *next* merge window...

Yeah, we have plenty of time for 4.10 :) So my plan is to apply this after I open wireless-drivers-next in 2-3 weeks or so. That would mean that the patch would hit Linus' tree when 4.10-rc1 is released (estimated to happen on 2017-01-01). The timing is actually perfect as now we get maximal testing time on -next.

Quoted 7 lines

Quoted 3 lines

And I assume I need to take v5:

https://patchwork.kernel.org/patch/9311037/

Yes. Haven't noticed anything that changed since that might conflict with it, but let me know if I missed something and you want a refreshed version.

Thanks, I'll let you know if there are any problems.