Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <156889576422.191202.5906619710809654631.stgit@alrua-x1>
 <156889576869.191202.510507546538322707.stgit@alrua-x1> <08f0ed6e-b746-9689-6dc8-7c0ea705666d@nbd.name>
 <87wodv19jl.fsf@toke.dk> <b0239b81-e3db-1f10-22cb-ad06536b97a8@nbd.name> <87tv8z13wv.fsf@toke.dk>
In-Reply-To: <87tv8z13wv.fsf@toke.dk>
From:   Kan Yan <kyan@google.com>
Date:   Thu, 26 Sep 2019 19:42:49 -0700
Message-ID: <CA+iem5uir8GOq2psAVuUParY3MimsGhvdefUopiwHov128CQQA@mail.gmail.com>
Subject: Re: [PATCH RFC/RFT 4/4] mac80211: Apply Airtime-based Queue Limit
 (AQL) on packet dequeue
To:     =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= <toke@redhat.com>
Cc:     Felix Fietkau <nbd@nbd.name>,
        Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org,
        make-wifi-fast@lists.bufferbloat.net,
        John Crispin <john@phrozen.org>,
        Lorenzo Bianconi <lorenzo@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

> No, but on tx_completion we could do something like this:
> airtime = CB(skb)->tx_time ?: CB(skb)->tx_time_est;
> ieee80211_report_airtime(sta, airtime);
> That way, if the driver sets the tx_time field to something the firmware
> reports, we'll use that, and otherwise we'd fall back to the estimate.
> Of course, there would need to be a way for the driver to opt out of
> this, for drivers that report out of band airtime like ath10k does :)

I doubt that will work, unless firmware can do per frame airtime
update in the skb. It is more likely that firmware provides out of
band airtime update for a period of time, like an aggregation. That's
the case for ath10k: https://patchwork.kernel.org/patch/10684689

Regarding handling frame for station enters power saving mode:
>
> >> Oh, right. Didn't know that could happen (I assumed those would be
> >> flushed out or something). But if we're going to go with Kan's
> >> suggestion of having per-station accounting as well as a global
> >> accounting for the device, we could just subtract the station's
> >> outstanding balance from the device total when it goes into powersave
> >> mode, and add it back once it wakes up again. At least I think that
> >> would work, no?
> >Yes, I think that would work.
> Great! Will incorporate that, then.

I think that could work but things could be a bit more complicated.
Not sure I fully understand the usage of airtime_weight_sum in  [PATCH
V3 1/4] mac80211: Switch to a virtual time-based airtime scheduler:

in ieee80211_schedule_txq():
   local->airtime_weight_sum[ac] += sta->airtime_weight;

in ieee80211_sta_register_airtime():
   weight_sum = local->airtime_weight_sum[ac] ?: sta->airtime_weight;
   local->airtime_v_t[ac] += airtime / weight_sum;
   sta->airtime[ac].v_t += airtime / sta->airtime_weight;

in  __ieee80211_unschedule_txq
 local->airtime_weight_sum[ac] -= sta->airtime_weight;

I assume the purpose of airtime_weight_sum is to count station's
virtual airtime proportional to the number of active stations for
fairness.  My concern is the per interface
local->airtime_weight_sum[ac] get updated when packets are released
from a txq to lower layer, but it doesn't mean the airtime will be
consumed (packets get transmitted) shortly, due to events like station
enter power save mode, so the weight_sum used in
ieee80211_sta_register_airtime() maybe inaccurate. For architects
using firmware/hardware offloading, firmware ultimately controls
packet scheduling and has quite a lot of autonomy. The host driver's
airtime_weight_sum may get out of sync with the number of active
stations actually scheduled by firmware even without power saving
events.

Is this a correct understanding? I kind of think the original method
of airtime accounting using deficit maybe more robust in this regard.