Re: [Make-wifi-fast] [RFC/RFT 2/5] ath9k: use mac80211 intermediate software queues

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

Tim Shepard <shep@alum.mit.edu> writes:

>> Reworked to not require the global variable renaming in ath9k.
>> 
>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>
> Huh.  I wonder why you did that?  Is it really better to call the
> ieee80211_txq a swq and call the ath9k hardware queue a txq?  I
> thought doing the renaming made for more readable and much more
> maintainable code (where searching for text strings produced much
> cleaner results when trying to track down all references).

Well, the immediate reason was that at the time I just spent two weeks
figuring out how ath9k worked and what the different concepts were, and
suddenly they start to mean something different. The txq->hwq renaming
would probably make sense in itself, but suddenly the same variable
(txq) meant something different, which was quite confusing. So I found
it was easier to change your patch to not require the renaming.

A secondary reason was to keep the patch delta as small as possible. I'm
definitely not the right person to make that call, but I found the
global renaming somewhat intrusive.

As for the first point, I think it would be easier if you did not call
the mac80211 queues 'txq' but something else ('swq' like I did, perhaps;
I also considered 'imq' for intermediate queue). This will at least make
it clear at a glance that it is something different than 'txq' was
previously.

> I am grateful to learn that someone has read my patched version of
> ath9k at least enough to do this rework. Any comments on the actual
> work?

Well, it seems to work well enough (haven't noticed the pending_frames
issue, but on the other hand I haven't been looking very hard). However,
it does fell like somewhat of a temporary solution. Having another
intermediate queue in the driver (tid->i_q) and having a side-effect in
ath_tid_has_buffered() seems a bit iffy to me. Wouldn't the 'right' way
to do it be to have ath_tid_has_buffered() ask the mac layer if it has
any frames queued, and then have ath_tx_get_tid_subframe() basically
translate straight to a call to ieee80211_tx_dequeue() (taking into
account the retry queue)? Not sure if that covers all code paths, but
the current approach seems like it has one too many layers of queues?

Haven't given the above a lot of thought, but since you asked... :)

-Toke