[ath9k-devel] [PATCH 1/2] ath9k: use mac80211 intermediate software queues

Discussion:

Felix Fietkau

2016-06-17 13:28:26 UTC

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.
Reworked to not require the global variable renaming in ath9k.
---
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(-)
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 93b3793..caeae10 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -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 {
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 ;)

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

Felix Fietkau

2016-06-17 13:48:13 UTC

Permalink

Post by Felix Fietkau

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

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.

- Felix

Felix Fietkau

2016-06-17 16:33:36 UTC

Permalink

Post by Felix Fietkau

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.

One more thing that I forgot in my previous reply: on PS wakeup, the
driver does not need to schedule the intermediate queues itself -
mac80211 will call drv_wake_tx_queue if frames are pending.

- Felix

Dave Taht

2016-06-17 14:10:17 UTC

Permalink

Post by Felix Fietkau

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.

Less queues, more filling!

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

what can be done to reduce the impact of channel scans?

http://blog.cerowrt.org/post/disabling_channel_scans/

Post by Felix Fietkau
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 ;)

The ideas here can apply elsewhere, also. Are you still actively
working with the mt76?

Anything else "out there" besides that and the ath5k worth looking at?

Am I seeing patches and firmware changes for better statistic keeping
on the ath10k that look promising for airtime fairness... or am I
delusional?

Post by Felix Fietkau
elsewhere powersave was mentioned

How big can a powersave queue get?

Felix Fietkau

2016-06-17 14:35:50 UTC

Permalink

Post by Felix Fietkau

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 93b3793..caeae10 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -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 {
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 ;)

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.

- Felix