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@toke.dk>
To:     Felix Fietkau <nbd@nbd.name>, Ryder Lee <ryder.lee@mediatek.com>
Cc:     Lorenzo Bianconi <lorenzo.bianconi@redhat.com>,
        Shayne Chen <shayne.chen@mediatek.com>,
        linux-wireless@vger.kernel.org, linux-mediatek@lists.infradead.org
Subject: Re: [PATCH] mac80211: check ATF flag in ieee80211_next_txq()
In-Reply-To: <68ab89bc-70f4-e822-3d96-21ba3bd6511f@nbd.name>
References: <d9aef825d186a91ff91f6a81045d49d375533b14.1609894402.git.ryder.lee@mediatek.com>
 <87r1my49us.fsf@toke.dk> <68ab89bc-70f4-e822-3d96-21ba3bd6511f@nbd.name>
Date:   Wed, 06 Jan 2021 16:41:02 +0100
Message-ID: <87k0sq3wfl.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Felix Fietkau <nbd@nbd.name> writes:

> On 2021-01-06 11:51, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Ryder Lee <ryder.lee@mediatek.com> writes:
>>=20
>>> 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.
>>=20
>> 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