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: <20191115014846.126007-1-kyan@google.com> <20191115014846.126007-3-kyan@google.com>
 <87k181mh7f.fsf@toke.dk>
In-Reply-To: <87k181mh7f.fsf@toke.dk>
From:   Kan Yan <kyan@google.com>
Date:   Fri, 15 Nov 2019 18:21:55 -0800
Message-ID: <CA+iem5vnp7Sd6jUqhbBiijCDFygSt+g=fh5y43pN9Rp+gegCeA@mail.gmail.com>
Subject: Re: [v8 PATCH 2/2] mac80211: Implement Airtime-based Queue Limit (AQL)
To:     =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= <toke@redhat.com>
Cc:     Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Make-Wifi-fast <make-wifi-fast@lists.bufferbloat.net>,
        Felix Fietkau <nbd@nbd.name>, Yibo Zhao <yiboz@codeaurora.org>,
        John Crispin <john@phrozen.org>,
        Lorenzo Bianconi <lorenzo@kernel.org>,
        Rajkumar Manoharan <rmanohar@codeaurora.org>,
        Kevin Hayes <kevinhayes@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

> > +     if (sta) {
> > +             atomic_sub(tx_airtime, &sta->airtime[ac].aql_tx_pending);
> > +             tx_pending =3D atomic_read(&sta->airtime[ac].aql_tx_pendi=
ng);
> This is still racy, since you're splitting it over two calls; you'll
> need to use atomic_sub_return() instead.
> I figure we've converged now to the point where it actually makes sense
> to collect everything back into a single series; so I can just fix this
> and re-send the full series.

Thanks for help fixing this. Yes, atomic_sub_return() is better.

>
>
> > +             if (WARN_ONCE(tx_pending < 0,
> > +                           "STA %pM AC %d txq pending airtime underflo=
w: %u, %u",
> > +                           sta->addr, ac, tx_pending, tx_airtime))
> > +                     atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending,
> > +                                    tx_pending, 0);
> This could still fail if there's a concurrent modification (and you're
> not checking the return of the cmpxchg). But at least it won't clobber
> any updated value, so I guess that is acceptable since we're in "should
> never happen" territory here :)

I did this on purpose since I really don't like adding a loop to retry
here. If aql_tx_pending indeed goes negative (should never happens and
we got WARN_ONCE() to catch it) and the subsequent atomic_cmpxchg()
failed (rare racing occasions), it is still ok. In this case,
aql_tx_pending carries a negative offset and will be reset in one of
the calls to ieee80211_sta_update_pending_airtime() later.
aql_tx_pending being negative doesn't have much side-effects, such as
causing txq stuck.

On Fri, Nov 15, 2019 at 4:56 AM Toke H=C3=B8iland-J=C3=B8rgensen <toke@redh=
at.com> wrote:
>
> Kan Yan <kyan@google.com> writes:
>
> > In order for the Fq_CoDel algorithm integrated in mac80211 layer to ope=
rate
> > effectively to control excessive queueing latency, the CoDel algorithm
> > requires an accurate measure of how long packets stays in the queue, AK=
A
> > sojourn time. The sojourn time measured at the mac80211 layer doesn't
> > include queueing latency in the lower layer (firmware/hardware) and CoD=
el
> > expects lower layer to have a short queue. However, most 802.11ac chips=
ets
> > offload tasks such TX aggregation to firmware or hardware, thus have a =
deep
> > lower layer queue.
> >
> > Without a mechanism to control the lower layer queue size, packets only
> > stay in mac80211 layer transiently before being sent to firmware queue.
> > As a result, the sojourn time measured by CoDel in the mac80211 layer i=
s
> > almost always lower than the CoDel latency target, hence CoDel does lit=
tle
> > to control the latency, even when the lower layer queue causes excessiv=
e
> > latency.
> >
> > The Byte Queue Limits (BQL) mechanism is commonly used to address the
> > similar issue with wired network interface. However, this method cannot=
 be
> > applied directly to the wireless network interface. "Bytes" is not a
> > suitable measure of queue depth in the wireless network, as the data ra=
te
> > can vary dramatically from station to station in the same network, from=
 a
> > few Mbps to over Gbps.
> >
> > This patch implements an Airtime-based Queue Limit (AQL) to make CoDel =
work
> > effectively with wireless drivers that utilized firmware/hardware
> > offloading. AQL allows each txq to release just enough packets to the l=
ower
> > layer to form 1-2 large aggregations to keep hardware fully utilized an=
d
> > retains the rest of the frames in mac80211 layer to be controlled by th=
e
> > CoDel algorithm.
> >
> > Signed-off-by: Kan Yan <kyan@google.com>
> > [ Toke: Keep API to set pending airtime internal, fix nits in commit ms=
g ]
> > Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen <toke@redhat.com>
> > ---
> [...]
>
> > +     if (sta) {
> > +             atomic_sub(tx_airtime, &sta->airtime[ac].aql_tx_pending);
> > +             tx_pending =3D atomic_read(&sta->airtime[ac].aql_tx_pendi=
ng);
>
> This is still racy, since you're splitting it over two calls; you'll
> need to use atomic_sub_return() instead.
>
> I figure we've converged now to the point where it actually makes sense
> to collect everything back into a single series; so I can just fix this
> and re-send the full series.
>
> > +             if (WARN_ONCE(tx_pending < 0,
> > +                           "STA %pM AC %d txq pending airtime underflo=
w: %u, %u",
> > +                           sta->addr, ac, tx_pending, tx_airtime))
> > +                     atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending,
> > +                                    tx_pending, 0);
>
> This could still fail if there's a concurrent modification (and you're
> not checking the return of the cmpxchg). But at least it won't clobber
> any updated value, so I guess that is acceptable since we're in "should
> never happen" territory here :)
>
> -Toke
>