[PATCH v1 0/4] wifi: ath11k: enable airtime queue limits and fairness

[PATCH v1 0/4] wifi: ath11k: enable airtime queue limits and fairness

[Julius Bairaktaris]

ath11k advertises neither NL80211_EXT_FEATURE_AQL nor the airtime
fairness bit, and every user of either inside mac80211 is gated on the
corresponding feature. Both are inert on this driver.

AQL matters here more than it would for a driver with its own TXQ
scheduler, because it is the only brake. ath11k uses
ieee80211_handle_wake_tx_queue(), whose wake_tx_push_queue() drains a
selected txq whole into the driver, and the check at the top of
ieee80211_tx_dequeue() is what stops that drain. Without the feature bit
mac80211 holds no backlog at all: on a saturated download every queued
byte sits in the hardware TX rings, where there is no AQM and no flow
separation.

Measured on an IPQ8074 AP against one HE 160 MHz station at MCS 11
receiving 280 Mbit/s, read from that station's own aql file: its BE queue
depth reaches 6356 us of the 12000 us default limit, and 2972 us when the
limit is lowered to 500/1000 us. Without patch 2 that file is not created
and the limits are never consulted.

Patch 1 converts the last tx completion path in dp_tx.c that still frees a
frame with dev_kfree_skb_any(), leaking the ack-status clone and its idr
entry. It is independent of the rest and carries Cc: stable.

Patch 2 advertises AQL. The rate its estimator needs is already kept in
arsta->last_txrate and already reaches mac80211.

Patch 3 exposes the outstanding-MSDU count the driver already keeps, so
soc_dp_stats can report how deep the transmit path is and not only when it
overflowed.

Patch 4 registers per-station tx airtime and advertises airtime fairness.
Unlike AQL there is no estimator fallback for the deficit, so the bit
cannot be set without the reporting. The fairness it delivers is coarse
for reasons stated in that patch, and it is last so the first three can be
applied without it.

An AQL enable was proposed once before and left at changes-requested for a
commit message that did not say why or show a measurement. This series
answers both, and needs no custom .wake_tx_queue: ieee80211_tx_dequeue()
performs the check itself. It is orthogonal to the in-flight
"ath11k/ath12k: implement TX flow control" series, which replaces that
callback for an unrelated reason and leaves the check where it is.

ath10k and mt76 set both feature bits.

Julius Bairaktaris (4):
  wifi: ath11k: free tx skbs through ieee80211_free_txskb()
  wifi: ath11k: enable airtime queue limits
  wifi: ath11k: report the pending tx MSDU count in soc_dp_stats
  wifi: ath11k: report tx airtime and enable airtime fairness

 drivers/net/wireless/ath/ath11k/debugfs.c | 12 ++++++++++++
 drivers/net/wireless/ath/ath11k/dp_rx.c   | 10 ++++++++++
 drivers/net/wireless/ath/ath11k/dp_tx.c   |  3 ++-
 drivers/net/wireless/ath/ath11k/mac.c     |  3 +++
 4 files changed, 27 insertions(+), 1 deletion(-)

base-commit: ca800a9302764c445de0da0e84d2252400a770ee
-- 
2.53.0

[PATCH v1 1/4] wifi: ath11k: free tx skbs through ieee80211_free_txskb()

[Julius Bairaktaris]

ath11k_dp_tx_free_txbuf(), reached from the HTT tx completion handler for
the REINJ and INSPECT statuses, releases the frame with
dev_kfree_skb_any(). Every other tx completion path in dp_tx.c hands the
skb back to mac80211, and this one is the last that does not.

A frame freed that way keeps whatever state mac80211 was holding for it.
A data frame whose socket asked for tx status has a clone parked in
local->ack_status_frames and an index allocated from an idr bounded at
8191 entries; ieee80211_free_txskb() releases both through
ieee80211_report_used_skb(), and dev_kfree_skb_any() leaks them for the
lifetime of the hw. Commit 29d15589f084 ("wifi: ath11k: Cleanup mac80211
references on failure during tx_complete") converted three sibling paths
for exactly that reason and left this one.

Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Cc: stable@vger.kernel.org
Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.9.0.1-02146-QCAHKSWPL_SILICONZ-1
Assisted-by: Claude:claude-opus-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 drivers/net/wireless/ath/ath11k/dp_tx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/dp_tx.c b/drivers/net/wireless/ath/ath11k/dp_tx.c
index cac970c92806..52c4a9924a31 100644
--- a/drivers/net/wireless/ath/ath11k/dp_tx.c
+++ b/drivers/net/wireless/ath/ath11k/dp_tx.c
@@ -305,9 +305,10 @@ static void ath11k_dp_tx_free_txbuf(struct ath11k_base *ab, u8 mac_id,
 	skb_cb = ATH11K_SKB_CB(msdu);
 
 	dma_unmap_single(ab->dev, skb_cb->paddr, msdu->len, DMA_TO_DEVICE);
-	dev_kfree_skb_any(msdu);
 
 	ar = ab->pdevs[mac_id].ar;
+	ieee80211_free_txskb(ar->hw, msdu);
+
 	if (atomic_dec_and_test(&ar->dp.num_tx_pending))
 		wake_up(&ar->dp.tx_empty_waitq);
 }
-- 
2.53.0

[PATCH v1 2/4] wifi: ath11k: enable airtime queue limits

[Julius Bairaktaris]

ath11k does not advertise NL80211_EXT_FEATURE_AQL, and every user of AQL in
mac80211 is gated on that bit: the airtime charge in
ieee80211_tx_dequeue(), ieee80211_txq_airtime_check(),
ieee80211_sta_update_pending_airtime(), and the per-station aql file in
debugfs, which is not even created. AQL is not untuned on this driver, it
is absent.

That matters here more than it would elsewhere, because AQL is the only
brake. ath11k uses ieee80211_handle_wake_tx_queue(), whose
wake_tx_push_queue() drains a selected txq whole into the driver, and the
check at the top of ieee80211_tx_dequeue() is what stops that drain.
Without the feature bit nothing does, so mac80211 holds no backlog and
every queued byte of a saturated download sits in the hardware TX rings,
where there is no AQM and no flow separation.

Measured on an IPQ8074 AP against one HE 160 MHz station at MCS 11, taken
from that station's own aql file while it received 280 Mbit/s: its BE queue
depth reaches 6356 us of the 12000 us default limit, and 2972 us when the
limit is lowered to 500/1000 us. Without this patch the file does not exist
and the limits are never consulted.

The rate the estimator needs is already there: ath11k keeps a per-station
rate_info in arsta->last_txrate and passes it to ieee80211_tx_status_ext(),
which stores it as tx_stats.last_rate_info, and that is what
ieee80211_calc_expected_tx_airtime() reads. Where no rate is known yet the
estimator returns zero, no airtime is charged, and AQL stays inert, so this
cannot behave worse than the current code.

The estimate is only as fresh as arsta->last_txrate, which is updated from
the HTT PPDU stats when a descriptor is evicted from ar->ppdu_stats_info,
so the rate behind it can lag a rate change by up to
HTT_PPDU_DESC_MAX_DEPTH PPDUs.

ieee80211_sta_update_pending_airtime() returns the airtime but does not
reschedule the txq, and ath11k has no completion-side push, so a queue held
back by AQL is re-poked only by the next enqueue or queue wake.

An AQL enable was proposed once before [1] with a custom .wake_tx_queue and
a tx-completion driven push. That scheduler is not needed:
ieee80211_tx_dequeue() performs the check itself, so the generic handler is
already braked.

ath10k and mt76 set the same feature bit.

Link: https://lore.kernel.org/all/20230501130725.7171-1-quic_tamizhr@quicinc.com/ [1]
Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.9.0.1-02146-QCAHKSWPL_SILICONZ-1
Assisted-by: Claude:claude-opus-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 drivers/net/wireless/ath/ath11k/mac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index 2d55cdc4d165..38f641bbc53c 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -10592,6 +10592,7 @@ static int __ath11k_mac_register(struct ath11k *ar)
 	ar->hw->sta_data_size = sizeof(struct ath11k_sta);
 
 	wiphy_ext_feature_set(ar->hw->wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST);
+	wiphy_ext_feature_set(ar->hw->wiphy, NL80211_EXT_FEATURE_AQL);
 	wiphy_ext_feature_set(ar->hw->wiphy, NL80211_EXT_FEATURE_STA_TX_PWR);
 	if (test_bit(WMI_TLV_SERVICE_BSS_COLOR_OFFLOAD,
 		     ar->ab->wmi_ab.svc_map)) {
-- 
2.53.0

[PATCH v1 3/4] wifi: ath11k: report the pending tx MSDU count in soc_dp_stats

[Julius Bairaktaris]

ar->dp.num_tx_pending counts the MSDUs the hardware has not completed
yet, and is used to wait for the transmit path to drain. It is never
shown.

soc_dp_stats can therefore say when the transmit path overflowed - TCL
ring full, misc transmit failures - and not how deep it is at this
instant. The bound those counters report against is the tx descriptor
idr, DP_TX_IDR_SIZE entries per ring, which is large enough that
occupancy rather than exhaustion is what characterises the path under
load.

Print it per radio, beside the failure counters it complements.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.9.0.1-02146-QCAHKSWPL_SILICONZ-1
Assisted-by: Claude:claude-opus-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 drivers/net/wireless/ath/ath11k/debugfs.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c
index 70922612d3fb..9dc586b68136 100644
--- a/drivers/net/wireless/ath/ath11k/debugfs.c
+++ b/drivers/net/wireless/ath/ath11k/debugfs.c
@@ -714,6 +714,18 @@ static ssize_t ath11k_debugfs_dump_soc_dp_stats(struct file *file,
 			 "\nMisc Transmit Failures: %d\n",
 			 atomic_read(&soc_stats->tx_err.misc_fail));
 
+	/* What the hardware still owes a completion for. The counters above say
+	 * when the transmit path overflowed; none of them says how deep it is.
+	 */
+	len += scnprintf(buf + len, size - len, "\nPending Tx MSDUs:\n");
+	for (i = 0; i < ab->num_radios; i++) {
+		struct ath11k *ar = ab->pdevs[i].ar;
+
+		if (ar)
+			len += scnprintf(buf + len, size - len, "radio%d: %d\n",
+					 i, atomic_read(&ar->dp.num_tx_pending));
+	}
+
 	len += ath11k_debugfs_dump_soc_ring_bp_stats(ab, buf + len, size - len);
 
 	if (len > size)
-- 
2.53.0

[PATCH v1 4/4] wifi: ath11k: report tx airtime and enable airtime fairness

[Julius Bairaktaris]

mac80211's airtime scheduler charges sta->airtime[ac].deficit from one
place, ieee80211_sta_register_airtime(). Unlike AQL there is no estimator
fallback, and ath11k never calls it, so the ATF feature bit cannot be
advertised on its own: the deficit would only ever be
replenished, every station would stay permanently in credit, and the DRR
would be inert. The bit is not a no-op in that state either, because it
also moves txq insertion to the head of the active list, which
__ieee80211_schedule_txq() documents as safe only because the DRR moves a
station back once its deficit goes negative.

fes_duration_us arrives with every PPDU's stats and is already
accumulated into arsta->tx_duration a few lines above. Register it, and
set the feature bit in the same change so the two cannot be separated.

Measured on an IPQ8074 AP with two stations on one radio, a 1x1 VHT
client at 433 Mbit/s and a 2x2 HE client, both receiving at once. Before,
'iw station set <mac> airtime_weight' is refused with -EOPNOTSUPP and the
per-station airtime file reads zero. After, the weight is accepted and
read back, TX airtime accumulates, and the per-AC deficits move.

The fairness this delivers is coarse. Weighting the fast station 4:1 moves
the slow station's share of airtime from 59.5% to 52.1% and the fast
station's throughput from 315 to 392 Mbit/s: the direction is correct, the
magnitude is not the ratio requested. The granularity is bounded by the
driver's push model, since ieee80211_handle_wake_tx_queue() drains a
selected txq whole into the hardware and the scheduler therefore reorders
only between selections. Tightening the AQL limit to shorten those drains
does not improve it: the share moves by one point and aggregate
throughput falls from 401 to 178 Mbit/s. Finer control would require a TXQ
scheduler in the driver.

Two properties of the source are worth stating. fes_duration_us spans the
frame exchange sequence, so it includes the SIFS and the responding
BlockAck, time the radio spends receiving; the airtime registered is
therefore larger than the airtime transmitted. And it is a per-PPDU field,
so on a downlink MU PPDU each user is charged the whole sequence. No
per-user duration is reported in these stats. On the firmware tested here
every PPDU carried exactly one user, measured over 21051 PPDUs, so the MU
case is latent rather than observed.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.9.0.1-02146-QCAHKSWPL_SILICONZ-1
Assisted-by: Claude:claude-opus-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 drivers/net/wireless/ath/ath11k/dp_rx.c | 9 +++++++++
 drivers/net/wireless/ath/ath11k/mac.c   | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/net/wireless/ath/ath11k/dp_rx.c b/drivers/net/wireless/ath/ath11k/dp_rx.c
index 33425707c084..33c6fd9553a6 100644
--- a/drivers/net/wireless/ath/ath11k/dp_rx.c
+++ b/drivers/net/wireless/ath/ath11k/dp_rx.c
@@ -1485,6 +1485,15 @@ ath11k_update_per_peer_tx_stats(struct ath11k *ar,
 
 	arsta->txrate.bw = ath11k_mac_bw_to_mac80211_bw(bw);
 	arsta->tx_duration += tx_duration;
+
+	/* tid is HTT_PPDU_STATS_NON_QOS_TID when no ack/BA TLV came with the
+	 * PPDU; mask it so that airtime lands in a real access category rather
+	 * than one chosen by the spare bits.
+	 */
+	if (tx_duration)
+		ieee80211_sta_register_airtime(sta,
+					       tid & IEEE80211_QOS_CTL_TID_MASK,
+					       tx_duration, 0);
 	memcpy(&arsta->last_txrate, &arsta->txrate, sizeof(struct rate_info));
 
 	/* PPDU stats reported for mgmt packet doesn't have valid tx bytes.
diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index 38f641bbc53c..40a2b6d2f804 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -10593,6 +10593,8 @@ static int __ath11k_mac_register(struct ath11k *ar)
 
 	wiphy_ext_feature_set(ar->hw->wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST);
 	wiphy_ext_feature_set(ar->hw->wiphy, NL80211_EXT_FEATURE_AQL);
+	wiphy_ext_feature_set(ar->hw->wiphy,
+			      NL80211_EXT_FEATURE_AIRTIME_FAIRNESS);
 	wiphy_ext_feature_set(ar->hw->wiphy, NL80211_EXT_FEATURE_STA_TX_PWR);
 	if (test_bit(WMI_TLV_SERVICE_BSS_COLOR_OFFLOAD,
 		     ar->ab->wmi_ab.svc_map)) {
-- 
2.53.0

Re: [PATCH v1 2/4] wifi: ath11k: enable airtime queue limits

[Toke Høiland-Jørgensen]

Julius Bairaktaris <julius@bairaktaris.de> writes:

> ath11k does not advertise NL80211_EXT_FEATURE_AQL, and every user of AQL in
> mac80211 is gated on that bit: the airtime charge in
> ieee80211_tx_dequeue(), ieee80211_txq_airtime_check(),
> ieee80211_sta_update_pending_airtime(), and the per-station aql file in
> debugfs, which is not even created. AQL is not untuned on this driver, it
> is absent.
>
> That matters here more than it would elsewhere, because AQL is the only
> brake. ath11k uses ieee80211_handle_wake_tx_queue(), whose
> wake_tx_push_queue() drains a selected txq whole into the driver, and the
> check at the top of ieee80211_tx_dequeue() is what stops that drain.
> Without the feature bit nothing does, so mac80211 holds no backlog and
> every queued byte of a saturated download sits in the hardware TX rings,
> where there is no AQM and no flow separation.
>
> Measured on an IPQ8074 AP against one HE 160 MHz station at MCS 11, taken
> from that station's own aql file while it received 280 Mbit/s: its BE queue
> depth reaches 6356 us of the 12000 us default limit, and 2972 us when the
> limit is lowered to 500/1000 us. Without this patch the file does not exist
> and the limits are never consulted.

Did you test if this actually has any impact on latency under load for
the station in question?

> The rate the estimator needs is already there: ath11k keeps a per-station
> rate_info in arsta->last_txrate and passes it to ieee80211_tx_status_ext(),
> which stores it as tx_stats.last_rate_info, and that is what
> ieee80211_calc_expected_tx_airtime() reads. Where no rate is known yet the
> estimator returns zero, no airtime is charged, and AQL stays inert, so this
> cannot behave worse than the current code.
>
> The estimate is only as fresh as arsta->last_txrate, which is updated from
> the HTT PPDU stats when a descriptor is evicted from ar->ppdu_stats_info,
> so the rate behind it can lag a rate change by up to
> HTT_PPDU_DESC_MAX_DEPTH PPDUs.
>
> ieee80211_sta_update_pending_airtime() returns the airtime but does not
> reschedule the txq, and ath11k has no completion-side push, so a queue held
> back by AQL is re-poked only by the next enqueue or queue wake.

This sounds like it could leave to stuck queues? :/

-Toke

Re: [PATCH v1 4/4] wifi: ath11k: report tx airtime and enable airtime fairness

[Toke Høiland-Jørgensen]

Julius Bairaktaris <julius@bairaktaris.de> writes:

> mac80211's airtime scheduler charges sta->airtime[ac].deficit from one
> place, ieee80211_sta_register_airtime(). Unlike AQL there is no estimator
> fallback, and ath11k never calls it, so the ATF feature bit cannot be
> advertised on its own: the deficit would only ever be
> replenished, every station would stay permanently in credit, and the DRR
> would be inert. The bit is not a no-op in that state either, because it
> also moves txq insertion to the head of the active list, which
> __ieee80211_schedule_txq() documents as safe only because the DRR moves a
> station back once its deficit goes negative.
>
> fes_duration_us arrives with every PPDU's stats and is already
> accumulated into arsta->tx_duration a few lines above. Register it, and
> set the feature bit in the same change so the two cannot be separated.
>
> Measured on an IPQ8074 AP with two stations on one radio, a 1x1 VHT
> client at 433 Mbit/s and a 2x2 HE client, both receiving at once. Before,
> 'iw station set <mac> airtime_weight' is refused with -EOPNOTSUPP and the
> per-station airtime file reads zero. After, the weight is accepted and
> read back, TX airtime accumulates, and the per-AC deficits move.
>
> The fairness this delivers is coarse. Weighting the fast station 4:1 moves
> the slow station's share of airtime from 59.5% to 52.1% and the fast
> station's throughput from 315 to 392 Mbit/s: the direction is correct, the
> magnitude is not the ratio requested.

How consistent is this change between runs? Seems a bit marginal if the
policy was supposed to give a 4:1 ratio between stations, so does this
work at all?

-Toke