Re: [Make-wifi-fast] [PATCH 1/2] ath9k: use mac80211 intermediate software queues

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Felix Fietkau <nbd@nbd.name> writes:

> On 2016-06-17 11:09, Toke Høiland-Jørgensen wrote:
>> 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 <shep@alum.mit.edu>
>> 
>> Reworked to not require the global variable renaming in ath9k.
>> 
>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>> ---
>>  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.

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


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

Noted.

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

Right.

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

Yes, that would be good ;)

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

Ah, cool, thanks!

-Toke