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>, johannes@sipsolutions.net
Cc:     linux-wireless@vger.kernel.org,
        make-wifi-fast@lists.bufferbloat.net, nbd@nbd.name,
        yiboz@codeaurora.org, john@phrozen.org, lorenzo@kernel.org,
        rmanohar@codeaurora.org, kevinhayes@google.com,
        Kan Yan <kyan@google.com>
Subject: Re: [v8 PATCH 2/2] mac80211: Implement Airtime-based Queue Limit (AQL)
In-Reply-To: <20191115014846.126007-3-kyan@google.com>
References: <20191115014846.126007-1-kyan@google.com> <20191115014846.126007-3-kyan@google.com>
Date:   Fri, 15 Nov 2019 13:56:04 +0100
Message-ID: <87k181mh7f.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

Kan Yan <kyan@google.com> writes:

> In order for the Fq_CoDel algorithm integrated in mac80211 layer to opera=
te
> 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 chipset=
s
> offload tasks such TX aggregation to firmware or hardware, thus have a de=
ep
> 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 littl=
e
> 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 b=
e
> 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 wo=
rk
> effectively with wireless drivers that utilized firmware/hardware
> offloading. AQL allows each txq to release just enough packets to the low=
er
> 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=C3=B8iland-J=C3=B8rgensen <toke@redhat.com>
> ---
[...]

> +=09if (sta) {
> +=09=09atomic_sub(tx_airtime, &sta->airtime[ac].aql_tx_pending);
> +=09=09tx_pending =3D 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.

> +=09=09if (WARN_ONCE(tx_pending < 0,
> +=09=09=09      "STA %pM AC %d txq pending airtime underflow: %u, %u",
> +=09=09=09      sta->addr, ac, tx_pending, tx_airtime))
> +=09=09=09atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending,
> +=09=09=09=09       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