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.
OK, makes sense. But you left it called txq in mac80211. So someone
reading the mac80211 code and ath9k at the same time (to understand
the whole stack) winds up with txq meaning different things, including
in places in ath9k where it has to reference a field in a structure
defined by mac80211's header files to point to one of its txqs.
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'm not the one who called them txq. That was Felix's patch.
He also wrote the mt76 driver and in that driver txq consistently
means the mac80211 intermediate queue and not a driver internal queue
or a hardware queue. So I was trying to converge ath9k to be more
like the one and only example I had of a driver that uses the
intermediate queue.
Well, finding a common and minimally invasive (re)naming scheme would be good.
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... :)
Thanks. I've gotten no other feedback that suggests anyone else has
read the code. So I very much appreciate it.
I not only read it but tested it extensively against the ath9k stock,
ath10k stock, ath10k -michal's fqmac 3.5 patches, and your ath9k
patches...
After losing my temper at the impact of channel scans...
( https://plus.google.com/u/0/107942175615993706558/posts/WA915Pt4SRN
), I got told how to get rid of them for testing, and started redoing
the work when I got back from vacation. The reductions in latency
without a decline in throughput were wonderful vs stock in all cases.
The latency reductions at lower rates and speed of response to rate
changes was not what I'd hoped for, but given all the other pieces
flying in loose formation (like the need for an airtime fair scheduler
also), I'm still pretty happy.
http://blog.cerowrt.org/flent/nmfixed/20mbit_not_entirely_convinced_weve_won_completely.svg
http://blog.cerowrt.org/flent/nmfixed/160_vs_80_half_delay.svg
(More plots and data in that directory and in the channel_scan directory.)
At the moment however I plan to pause until cleaner patch sets arrive....
Yes, I didn't like that side effect either, but (at least for my first
attempt) wanted to leave the old transmit path working, in particular
because I couldn't see how to get all the chanctx stuff working right
without leaving the old driver-internal queues in place. (I'm not
even sure what I would have to do to test the driver with
CONFIG_ATH9K_CHANNEL_CONTEXT turned on. I did test that it compiles
without error when CONFIG_ATH9K_CHANNEL_CONTEXT=y before I released my
v2 patch, and I tried to understand it enough to avoid breaking
anything. (My v1 patch broke compilation when
CONFIG_ATH9K_CHANNEL_CONTEXT=y as some robot notified me after I
posted it.)
I looked for a way to ask mac80211 if there are any packets left in
the intermediate queue without dequeueing a packet and I failed to
find such an interface.
qdisc->peek like function? A bitmap that says these stations have data
outstanding?
I guess I should have submitted a patch to
add that to mac80211. Or maybe there's a way to refactor the
aggregation code in ath9k so that it can cleanly work with the
existing ieee80211_tx_dequeue() without having to add another
interface to mac80211, but I didn't figure it out. It would have been
a bigger patch to ath9k, and require more thinking when reading the
patch to see if it works (assuming pre-patch ath9k works).
My current approach was an attempt to get it working. Yes, the code
looks like it has another layer of queues, but at run time there's
only one packet at a time on the internal queue that packets wind up
on from the mac80211 intermediate queue and by putting it on that
queue it can interface to the same code that pulls from that queue or
the previously existing queues (which the chanctx code seems to
require remain around without major surgery).
I thought about first patching ath9k to remove all the chanctx stuff
just to have a simpler starting point to work on, but I wouldn't
expect that patch to ever be accepted upstream (assuming that somebody
somewhere is getting useful stuff from the chanctx stuff).
Again, thanks for looking at this patch.
I'm now working on figuring out the right way to fix my bug in
the <= v2 patch where I fail to respect the hwq_max_pending
values on the new transmit path, and I plan to post a v3 patch
when I get that done.
-Tim Shepard
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Dave Täht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org