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:     Kan Yan <kyan@google.com>
Cc:     Lorenzo Bianconi <lorenzo@kernel.org>,
        Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org,
        make-wifi-fast@lists.bufferbloat.net,
        John Crispin <john@phrozen.org>, Felix Fietkau <nbd@nbd.name>
Subject: Re: [PATCH RFC/RFT 4/4] mac80211: Apply Airtime-based Queue Limit (AQL) on packet dequeue
In-Reply-To: <CA+iem5toLj4f-3tuJgoLx1=R2PzRUGPu+c1j7zauw7=izOUmgw@mail.gmail.com>
References: <156889576422.191202.5906619710809654631.stgit@alrua-x1> <156889576869.191202.510507546538322707.stgit@alrua-x1> <20190920120639.GA6456@localhost.localdomain> <87k1a39lgt.fsf@toke.dk> <20190920130604.GB6456@localhost.localdomain> <87h8579jpj.fsf@toke.dk> <CA+iem5toLj4f-3tuJgoLx1=R2PzRUGPu+c1j7zauw7=izOUmgw@mail.gmail.com>
Date:   Sat, 21 Sep 2019 14:11:52 +0200
Message-ID: <87d0ft7ss7.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=WINDOWS-1252
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Kan Yan <kyan@google.com> writes:

> Hi Toke,
>
> There is an updated version of AQL in the chromiumos tree implemented
> in the mac80211 driver, instead of in the ath10k driver as the
> original version:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/=
+/1703105/7
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/=
+/1703106/6

Ah, that's awesome! Thank you for brining this up :)

> It is based on a more recent kernel (4.14) and integrated with the
> airtime fairness tx scheduler in mac80211. This version has been
> tested rather extensively. I intended to use it as the basis for my
> effort to bring AQL upstream, but get sidetracked by other things. I
> can clean it up and send a patchset next week if you think that is the
> right path.

Yes, please do! AFAICT, the main difference is that your version keeps
the airtime calculation itself in the driver, while mine passes up the
rate and lets mac80211 do the calculation of airtime. Other than that,
the differences are minor, no?

I'm not actually sure which approach is best; I suspect doing all the
accounting in mac80211 will help with integrating this into drivers that
use minstrel; we can just add a hook in that and be done with it.
Whereas if the driver has to do the accounting, we would need to add
that to each driver (mt76, iwl(?)).

But of course, doing things in mac80211 depends on stuffing even more
stuff into the already overloaded cb field; and I'm not actually
entirely sure what I've done with that will actually work. WDYT?

In any case, if you post your series we'll have something to contrast
against, which I think will be useful to help us converge on something
we can all be happy with.

Of course we'll also have to eventually integrate this with the other
series that Yibo recently re-posted (the virtual time scheduler). I
think that will be relatively straight forward, except I'm not sure your
atomic patches will work when we also have to update the rbtree. Any
thoughts on that series in general?

> Sorry for the long delay and slack off on the upstream effort.

Hehe, no worries. I only posted this because Dave finally bugged me into
doing something about this at LPC. And hey, we're making progress now,
so that's good! :)

> There is some concern in this thread regarding the accuracy of the
> estimated airtime using the last reported TX rate. It is indeed a
> rather crude method and did not include retries in the calculation.
> Besides, there are lags between firmware changing rate and host driver
> get the rate update. The 16us IFS overhead is only correct for 5G and
> it is actually 10us for 2.4 G. However, that hardly matters. The goal
> of AQL is to prevent the firmware/hardware queue from getting bloated
> or starved. There is a lot of headroom in the queue length limit (8-10
> ms) to tolerate inaccuracy in the estimate airtime. AQL doesn't
> control the fine grained TX packet scheduling. It is handled by the
> airtime fairness scheduler and ultimately firmware.

Yeah, this was basically the point I was trying to make; this limit
doesn't need to be that accurate, we just need a rough estimate. If we
want to get the latency even lower later, we're better off fiddling with
the queue limit value than trying to improve the airtime estimate.

> There are two TX airtimes in the newer version (chromiumos 4.14
> kernel): The estimated airtime for frames pending in the queue and the
> airtime reported by the firmware for the frame transmitted, which
> should be accurate as the firmware supposed to take retries and
> aggregation into account. The airtime fairness scheduler that does the
> TX packet scheduling should use the TX airtime reported by the
> firmware. That's the reason why the original implementation in the
> ChromiumOS tree tries to factor in aggregation size when estimate the
> airtime overhead and the later version doesn't even bother with that.

Yup, makes sense. Looking at the version you linked to, though, it seems
you're calling ieee80211_sta_register_airtime() with the estimated value
as well? So are you double-accounting airtime, or are you adjusting for
the accurate values somewhere else I don't see in that series?

-Toke