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;
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>
To:     Felix Fietkau <nbd@nbd.name>,
        Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org,
        make-wifi-fast@lists.bufferbloat.net,
        John Crispin <john@phrozen.org>,
        Lorenzo Bianconi <lorenzo@kernel.org>
Subject: Re: [PATCH RFC/RFT 4/4] mac80211: Apply Airtime-based Queue Limit (AQL) on packet dequeue
In-Reply-To: <08f0ed6e-b746-9689-6dc8-7c0ea705666d@nbd.name>
References: <156889576422.191202.5906619710809654631.stgit@alrua-x1> <156889576869.191202.510507546538322707.stgit@alrua-x1> <08f0ed6e-b746-9689-6dc8-7c0ea705666d@nbd.name>
Date:   Thu, 26 Sep 2019 15:17:34 +0200
Message-ID: <87wodv19jl.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Felix Fietkau <nbd@nbd.name> writes:

> On 2019-09-19 14:22, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> From: Toke H=C3=B8iland-J=C3=B8rgensen <toke@redhat.com>
>>=20
>> Some devices have deep buffers in firmware and/or hardware which prevent=
s
>> the FQ structure in mac80211 from effectively limiting bufferbloat on th=
e
>> link. For Ethernet devices we have BQL to limit the lower-level queues, =
but
>> this cannot be applied to mac80211 because transmit rates can vary wildl=
y
>> between packets depending on which station we are transmitting it to.
>>=20
>> To overcome this, we can use airtime-based queue limiting (AQL), where w=
e
>> estimate the transmission time for each packet before dequeueing it, and
>> use that to limit the amount of data in-flight to the hardware. This ide=
a
>> was originally implemented as part of the out-of-tree airtime fairness
>> patch to ath10k[0] in chromiumos.
>>=20
>> This patch ports that idea over to mac80211. The basic idea is simple
>> enough: Whenever we dequeue a packet from the TXQs and send it to the
>> driver, we estimate its airtime usage, based on the last recorded TX rat=
e
>> of the station that packet is destined for. We keep a running per-AC tot=
al
>> of airtime queued for the whole device, and when that total climbs above=
 8
>> ms' worth of data (corresponding to two maximum-sized aggregates), we
>> simply throttle the queues until it drops down again.
>>=20
>> The estimated airtime for each skb is stored in the tx_info, so we can
>> subtract the same amount from the running total when the skb is freed or
>> recycled. The throttling mechanism relies on this accounting to be
>> accurate (i.e., that we are not freeing skbs without subtracting any
>> airtime they were accounted for), so we put the subtraction into
>> ieee80211_report_used_skb().
>>=20
>> This patch does *not* include any mechanism to wake a throttled TXQ agai=
n,
>> on the assumption that this will happen anyway as a side effect of whate=
ver
>> freed the skb (most commonly a TX completion).
>>=20
>> The throttling mechanism only kicks in if the queued airtime total goes
>> above the limit. Since mac80211 calculates the time based on the reporte=
d
>> last_tx_time from the driver, the whole throttling mechanism only kicks =
in
>> for drivers that actually report this value. With the exception of
>> multicast, where we always calculate an estimated tx time on the assumpt=
ion
>> that multicast is transmitted at the lowest (6 Mbps) rate.
>>=20
>> The throttling added in this patch is in addition to any throttling alre=
ady
>> performed by the airtime fairness mechanism, and in principle the two
>> mechanisms are orthogonal (and currently also uses two different sources=
 of
>> airtime). In the future, we could amend this, using the airtime estimate=
s
>> calculated by this mechanism as a fallback input to the airtime fairness
>> scheduler, to enable airtime fairness even on drivers that don't have a
>> hardware source of airtime usage for each station.
>>=20
>> [0] https://chromium-review.googlesource.com/c/chromiumos/third_party/ke=
rnel/+/588190/13/drivers/net/wireless-4.2/ath/ath10k/mac.c#3845
> One thing that might be missing here is dealing with airtime accounting
> of frames that remain queued in the driver/hardware because the station
> is in powersave 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?

-Toke