Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <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
In-Reply-To: <CAMHmoos8p7icOfsXT7q+-uThGCd-k04hPT0BWiCzNTPqH3CJUQ@mail.gmail.com>
References: <CAMHmoovK_m=Yd4uUPhsdHZ588+d1DF-fyS+uGKdRWvd33Q2u+A@mail.gmail.com>
 <87zh8uruou.fsf@toke.dk>
 <CAMHmoouFjdOSBAq00SMeauO2GvEnXMvJG+vEMwhYUD4rdW9rxg@mail.gmail.com>
 <87366ei2x7.fsf@toke.dk>
 <CAMHmoovWrXhMheEPapgEw9b_7S-jOZdJDRvAp-1Cb_i0=pBfEw@mail.gmail.com>
 <87lfk4ftnc.fsf@toke.dk>
 <CAMHmoos8p7icOfsXT7q+-uThGCd-k04hPT0BWiCzNTPqH3CJUQ@mail.gmail.com>
Date:   Fri, 11 Sep 2020 16:36:44 +0200
Message-ID: <87k0x09y83.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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;
 	}