diff mbox series

mac80211: check ATF flag in ieee80211_next_txq()

Message ID d9aef825d186a91ff91f6a81045d49d375533b14.1609894402.git.ryder.lee@mediatek.com
State New
Headers show
Series mac80211: check ATF flag in ieee80211_next_txq() | expand

Commit Message

Ryder Lee Jan. 6, 2021, 1:01 a.m. UTC 
The selected txq should be scheduled unconditionally if
NL80211_EXT_FEATURE_AIRTIME_FAIRNESS is not set by driver.

Also put the sta to the end of the active_txqs list if
deficit is negative then move on to the next txq.

Tested-by: Sujuan Chen <sujuan.chen@mediatek.com>
Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 net/mac80211/tx.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Ryder Lee Jan. 7, 2021, 2:11 a.m. UTC | #1 
On Wed, 2021-01-06 at 16:41 +0100, Toke Høiland-Jørgensen wrote:
> Felix Fietkau <nbd@nbd.name> writes:

> 

> > On 2021-01-06 11:51, Toke Høiland-Jørgensen wrote:

> >> Ryder Lee <ryder.lee@mediatek.com> writes:

> >> 

> >>> The selected txq should be scheduled unconditionally if

> >>> NL80211_EXT_FEATURE_AIRTIME_FAIRNESS is not set by driver.

> >>>

> >>> Also put the sta to the end of the active_txqs list if

> >>> deficit is negative then move on to the next txq.

> >> 

> >> Why is this needed? If the feature is not set, no airtime should ever be

> >> accounted to the station, and so sta->airtime[txqi->txq.ac].deficit will

> >> always be 0 - so you're just adding another check that doesn't actually

> >> change the behaviour, aren't you?

> >

> > I think it might make sense to keep airtime reporting even when airtime

> > fairness is disabled at run time, so this patch makes sense to me.

> > Instead of this patch, the right place to deal with this would probably

> > be ieee80211_sta_register_airtime.

> 

> When the fairness mechanism is user-disabled I agree it makes sense to

> still keep the accounting; and in fact that's what

> ieee80211_sta_register_airtime() already does when the accounting is

> turned off by way of the airtime_flags field... So don't think anything

> else is needed there either?

> 

> -Toke


Not sure I get this right. Are you talking about local->airtime_flags =
AIRTIME_USE_TX | AIRTIME_USE_RX ? I think that's different and we still
need to take NL80211_EXT_FEATURE_AIRTIME_FAIRNESS into account, right?

Ryder
Ryder Lee Jan. 8, 2021, 2:25 a.m. UTC | #2 
On Thu, 2021-01-07 at 14:08 +0100, Toke Høiland-Jørgensen wrote:
> Ryder Lee <ryder.lee@mediatek.com> writes:

> 

> > On Wed, 2021-01-06 at 16:41 +0100, Toke Høiland-Jørgensen wrote:

> >> Felix Fietkau <nbd@nbd.name> writes:

> >> 

> >> > On 2021-01-06 11:51, Toke Høiland-Jørgensen wrote:

> >> >> Ryder Lee <ryder.lee@mediatek.com> writes:

> >> >> 

> >> >>> The selected txq should be scheduled unconditionally if

> >> >>> NL80211_EXT_FEATURE_AIRTIME_FAIRNESS is not set by driver.

> >> >>>

> >> >>> Also put the sta to the end of the active_txqs list if

> >> >>> deficit is negative then move on to the next txq.

> >> >> 

> >> >> Why is this needed? If the feature is not set, no airtime should ever be

> >> >> accounted to the station, and so sta->airtime[txqi->txq.ac].deficit will

> >> >> always be 0 - so you're just adding another check that doesn't actually

> >> >> change the behaviour, aren't you?

> >> >

> >> > I think it might make sense to keep airtime reporting even when airtime

> >> > fairness is disabled at run time, so this patch makes sense to me.

> >> > Instead of this patch, the right place to deal with this would probably

> >> > be ieee80211_sta_register_airtime.

> >> 

> >> When the fairness mechanism is user-disabled I agree it makes sense to

> >> still keep the accounting; and in fact that's what

> >> ieee80211_sta_register_airtime() already does when the accounting is

> >> turned off by way of the airtime_flags field... So don't think anything

> >> else is needed there either?

> >> 

> >> -Toke

> >

> > Not sure I get this right. Are you talking about local->airtime_flags =

> > AIRTIME_USE_TX | AIRTIME_USE_RX ? I think that's different and we still

> > need to take NL80211_EXT_FEATURE_AIRTIME_FAIRNESS into account, right?

> 

> I just meant that what Felix was asking for (a way *for the user* to

> disable airtime fairness while still getting the airtime usage

> accounted) is possible by setting those flags. The EXT_FEATURE flag is

> meant as a way for the driver to signal to mac80211 that it supports

> reporting airtime at all; so ideally it should be a flag that is only

> set once.

> 

> Going back and reading your initial response it seems like you may be

> toggling the flag dynamically in the driver, though? Is this accurate?

> And if so, why? Is it not enough for you to fiddle with the

> USE_TX/USE_RX flags? :)

> 

> -Toke


Gotcha. We just set it once indeed. So the way you think is disable the
EXT_FEATURE flag and clear AIRTIME_USE_TX through debugfs
(DEBUGFS_ADD_MODE(airtime_flags, 0600)) in the meantime.

Ryder
diff mbox series

Patch

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 6422da6690f7..5640c9428596 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3760,14 +3760,19 @@  struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
 		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;
+		s64 deficit = 0;
 
 		if (aql_check)
 			found_eligible_txq = true;
 
-		if (deficit < 0)
-			sta->airtime[txqi->txq.ac].deficit +=
-				sta->airtime_weight;
+		if (wiphy_ext_feature_isset(local->hw.wiphy,
+					    NL80211_EXT_FEATURE_AIRTIME_FAIRNESS)) {
+			deficit = sta->airtime[txqi->txq.ac].deficit;
+
+			if (deficit < 0)
+				sta->airtime[txqi->txq.ac].deficit +=
+					sta->airtime_weight;
+		}
 
 		if (deficit < 0 || !aql_check) {
 			list_move_tail(&txqi->schedule_order,