Re: [v8 PATCH 2/2] mac80211: Implement Airtime-based Queue Limit (AQL)

[Kan Yan <kyan@google.com>]

> > +     if (sta) {
> > +             atomic_sub(tx_airtime, &sta->airtime[ac].aql_tx_pending);
> > +             tx_pending = atomic_read(&sta->airtime[ac].aql_tx_pending);
> 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 underflow: %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øiland-Jørgensen <toke@redhat.com> wrote:
>
> Kan Yan <kyan@google.com> writes:
>
> > In order for the Fq_CoDel algorithm integrated in mac80211 layer to operate
> > effectively to control excessive queueing latency, the CoDel algorithm
> > requires an accurate measure of how long packets stays in the queue, AKA
> > sojourn time. The sojourn time measured at the mac80211 layer doesn't
> > include queueing latency in the lower layer (firmware/hardware) and CoDel
> > expects lower layer to have a short queue. However, most 802.11ac chipsets
> > 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 is
> > almost always lower than the CoDel latency target, hence CoDel does little
> > to control the latency, even when the lower layer queue causes excessive
> > 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 rate
> > 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 lower
> > layer to form 1-2 large aggregations to keep hardware fully utilized and
> > retains the rest of the frames in mac80211 layer to be controlled by the
> > CoDel algorithm.
> >
> > Signed-off-by: Kan Yan <kyan@google.com>
> > [ Toke: Keep API to set pending airtime internal, fix nits in commit msg ]
> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > ---
> [...]
>
> > +     if (sta) {
> > +             atomic_sub(tx_airtime, &sta->airtime[ac].aql_tx_pending);
> > +             tx_pending = atomic_read(&sta->airtime[ac].aql_tx_pending);
>
> 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 underflow: %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
>