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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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