From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Miguel Catalan Cid <miguel.catalan@i2cat.net>
Cc: make-wifi-fast@lists.bufferbloat.net, linux-wireless@vger.kernel.org
Subject: Re: [Make-wifi-fast] Support for airtime scheduling using ath10k
Date: Fri, 11 Sep 2020 16:36:44 +0200 [thread overview]
Message-ID: <87k0x09y83.fsf@toke.dk> (raw)
In-Reply-To: <CAMHmoos8p7icOfsXT7q+-uThGCd-k04hPT0BWiCzNTPqH3CJUQ@mail.gmail.com>
Miguel Catalan Cid <miguel.catalan@i2cat.net> writes:
> Hi,
>
> after doing some tests with different ath10k Wi-Fi cards and clients, I
> found the following behaviour when combining AQL and the airtime
> scheduler:
Thank you for taking a closer look at this!
> - When using the default AQL limits (threshold 24000, limits per AC
> 5000/12000), the airtime scheduler is not working at all, regardless of the
> airtime weights of the STAs. Indeed, in some cases, slower stations were
> able to use a higher amount of airtime, leading to unfairness. I was
> thinking that maybe the default AQL limits are too high to these slow
> stations, allowing them to obtain too much pending airtime. I already used
> the last patches from Felix Fietkau with the same results.
Hmm, I think I can see what's going on here: The AQL patches changed the
behaviour of ieee80211_next_txq() so it will keep iterating as long as
there's at least one station that's eligible for transmission from the
AQL PoV. Which means all station deficits will keep increasing, erasing
the fairness throttling...
> - Indeed, I was able to activate the airtime scheduler by fixing lower AQL
> limits (e.g. threshold of 5000, limits per AC 0/5000). This way, it seems
> that the STAs start competing again for the airtime, and their behaviour
> follows the airtime weights. However, slower STAs lose a bit of
> performance due to these lower limits.
... and the effect of lowering the AQL parameters is that this situation
will occur less often, making the fairness scheduler work better.
> - The airtime weights have to be higher (e.g. 10000 vs 20000 to obtain a
> 33% vs 66% relation); I found the same behaviour using ath9k and 11n cards,
> so I guess this is due to the aggregation of packets.
This is a bit more puzzling, but I guess it's related to the above.
Also, for ath9k, the airtime check *always* returns true, which just
exacerbates the behaviour.
> Looking into the code, it seems that the key airtime check is the one
> in ieee80211_tx_dequeue. To enable the airtime scheduling, the
> "ieee80211_txq_airtime_check" function has to return false more usually;
> maybe it is just a matter of adjusting the AQL limits according to the
> airtime weights or to modify a bit the "ieee80211_txq_airtime_check"
> function to consider the airtime weight or the deficit of the
> stations.
Yup, you're right on point here, it's a bad interaction between those
two throttling mechanisms. The fairness mechanism relies on not
reshuffling the RR rotation.
I'll think a bit more about what we can do about this. May need to set
up a box for testing a few ideas, but I have been meaning to do that
anyway.
In the meantime, I wonder what happens if we just don't do that? I.e.,
something like the below (completely untested)?
-Toke
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index d2136007e2eb..3a2898cbb111 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3743,8 +3743,7 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
{
struct ieee80211_local *local = hw_to_local(hw);
struct ieee80211_txq *ret = NULL;
- struct txq_info *txqi = NULL, *head = NULL;
- bool found_eligible_txq = false;
+ struct txq_info *txqi = NULL;
spin_lock_bh(&local->active_txq_lock[ac]);
@@ -3755,34 +3754,22 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
if (!txqi)
goto out;
- if (txqi == head) {
- if (!found_eligible_txq)
- goto out;
- else
- found_eligible_txq = false;
- }
-
- if (!head)
- head = txqi;
-
if (txqi->txq.sta) {
struct sta_info *sta = container_of(txqi->txq.sta,
struct sta_info, sta);
- bool aql_check = ieee80211_txq_airtime_check(hw, &txqi->txq);
s64 deficit = sta->airtime[txqi->txq.ac].deficit;
- if (aql_check)
- found_eligible_txq = true;
-
- if (deficit < 0)
+ if (deficit < 0) {
sta->airtime[txqi->txq.ac].deficit +=
sta->airtime_weight;
- if (deficit < 0 || !aql_check) {
list_move_tail(&txqi->schedule_order,
&local->active_txqs[txqi->txq.ac]);
goto begin;
}
+
+ if (ieee80211_txq_airtime_check(hw, &txqi->txq))
+ goto out;
}
prev parent reply other threads:[~2020-09-11 14:36 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-22 16:22 Miguel Catalan Cid
2020-06-23 9:35 ` Toke Høiland-Jørgensen
2020-06-29 9:55 ` Miguel Catalan Cid
2020-06-29 10:25 ` Toke Høiland-Jørgensen
2020-06-30 15:33 ` Miguel Catalan Cid
2020-06-30 15:41 ` Toke Høiland-Jørgensen
2020-09-07 10:29 ` Miguel Catalan Cid
2020-09-11 14:36 ` Toke Høiland-Jørgensen [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://lists.bufferbloat.net/postorius/lists/make-wifi-fast.lists.bufferbloat.net/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87k0x09y83.fsf@toke.dk \
--to=toke@redhat.com \
--cc=linux-wireless@vger.kernel.org \
--cc=make-wifi-fast@lists.bufferbloat.net \
--cc=miguel.catalan@i2cat.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox