Re: [PATCH] mac80211: check ATF flag in ieee80211_next_txq()

[Ryder Lee <ryder.lee@mediatek.com>]

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