[PATCH v11 0/4] Add Airtime Queue Limits (AQL) to mac80211

[PATCH v11 0/4] Add Airtime Queue Limits (AQL) to mac80211

[Kan Yan]

This series is a first attempt at porting the Airtime Queue Limits concept from
the out-of-tree ath10k implementation[0] to mac80211. This version takes Kan's
patch to do the throttling in mac80211, and replaces the driver API with the
mechanism from the previous version of Toke's series, which instead calculated the
expected airtime at dequeue time inside mac80211, storing it in the SKB cb
field.

This series also imports Felix' airtime calculation code from mt76 into
mac80211, adjusting the API so it can be used from TX dequeue, by extracting the
latest TX rate from the tx_stats structure kept for each station.

This series has been tested on the QCA9984 platform. The test data can be found in:
https://drive.google.com/drive/folders/16Th2PljrKXVWDgM1pqDredMdSNcqJGOB

[0] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1703105/7

Changelog:

v11:
  - Fix softlockup in ieee80211_next_txq()
  - Increase the default queue limit

v10:
  - Fix return value from ieee80211_info_set_tx_time_est()

v9:
  - Use atomic_sub_return() instead of separate atomic_sub() and atomic_read()
  - Add getter/setter for tx_time_est
  - Use get_sta_info_by_addrs() to find the station in
    ieee80211_report_used_skb()
  - Integrate everything back into one series

v8:
  - Includes Toke's v7 version of "mac80211: Import airtime calculation code from mt76"
  - Don't clobber sta's customized queue limit when configuring the default via debugfs
  - Fix a racing condition when reset aql_tx_pending.

v7:
  - Fix aql_total_pending_airtime underflow due to insufficient locking.

v6:
  - Fix sta lookup in ieee80211_report_used_skb().
  - Move call to ieee80211_sta_update_pending_airtime() to a bit later in
    __ieee80211_tx_status()
v5:
  - Add missing export of ieee80211_calc_rx_airtime() and make
    ieee80211_calc_tx_airtime_rate() static (kbuildbot).
  - Use skb_get_queue_mapping() to get the AC from the skb.
  - Take basic rate configuration for the BSS into account when calculating
    multicast rate.
v4:
  - Fix calculation that clamps the maximum airtime to fit into 10 bits
  - Incorporate Rich Brown's nits for the commit message in Kan's patch
  - Add fewer local variables to ieee80211_tx_dequeue()
v3:
  - Move the tx_time_est field so it's shared with ack_frame_id, and use units
    of 4us for the value stored in it.
  - Move the addition of the Ethernet header size into ieee80211_calc_expected_tx_airtime()
v2:
  - Integrate Kan's approach to airtime throttling.
  - Hopefully fix the cb struct alignment on big-endian architectures.

Kan Yan (1):
  mac80211: Implement Airtime-based Queue Limit (AQL)

Toke Høiland-Jørgensen (3):
  mac80211: Add new sta_info getter by sta/vif addrs
  mac80211: Import airtime calculation code from mt76
  mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

 include/net/cfg80211.h     |   7 +
 include/net/mac80211.h     |  57 ++++
 net/mac80211/Makefile      |   3 +-
 net/mac80211/airtime.c     | 597 +++++++++++++++++++++++++++++++++++++
 net/mac80211/debugfs.c     |  85 ++++++
 net/mac80211/debugfs_sta.c |  43 ++-
 net/mac80211/ieee80211_i.h |   8 +
 net/mac80211/main.c        |  10 +-
 net/mac80211/sta_info.c    |  58 ++++
 net/mac80211/sta_info.h    |  11 +
 net/mac80211/status.c      |  36 ++-
 net/mac80211/tx.c          |  69 ++++-
 12 files changed, 961 insertions(+), 23 deletions(-)
 create mode 100644 net/mac80211/airtime.c

-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH v11 1/4] mac80211: Add new sta_info getter by sta/vif addrs

[Kan Yan]

From: Toke Høiland-Jørgensen <toke@redhat.com>

In ieee80211_tx_status() we don't have an sdata struct when looking up the
destination sta. Instead, we just do a lookup by the vif addr that is the
source of the packet being completed. Factor this out into a new sta_info
getter helper, since we need to use it for accounting AQL as well.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/mac80211/sta_info.c | 20 ++++++++++++++++++++
 net/mac80211/sta_info.h |  3 +++
 net/mac80211/status.c   | 10 ++--------
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index bd11fef2139f..465d83b13582 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -210,6 +210,26 @@ struct sta_info *sta_info_get_bss(struct ieee80211_sub_if_data *sdata,
 	return NULL;
 }
 
+struct sta_info *sta_info_get_by_addrs(struct ieee80211_local *local,
+				       const u8 *sta_addr, const u8 *vif_addr)
+{
+	struct rhlist_head *tmp;
+	struct sta_info *sta;
+
+	rcu_read_lock();
+	for_each_sta_info(local, sta_addr, sta, tmp) {
+		if (ether_addr_equal(vif_addr, sta->sdata->vif.addr)) {
+			rcu_read_unlock();
+			/* this is safe as the caller must already hold
+			 * another rcu read section or the mutex
+			 */
+			return sta;
+		}
+	}
+	rcu_read_unlock();
+	return NULL;
+}
+
 struct sta_info *sta_info_get_by_idx(struct ieee80211_sub_if_data *sdata,
 				     int idx)
 {
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 369c2dddce52..80e76569144e 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -725,6 +725,9 @@ struct sta_info *sta_info_get(struct ieee80211_sub_if_data *sdata,
 struct sta_info *sta_info_get_bss(struct ieee80211_sub_if_data *sdata,
 				  const u8 *addr);
 
+struct sta_info *sta_info_get_by_addrs(struct ieee80211_local *local,
+				       const u8 *sta_addr, const u8 *vif_addr);
+
 #define for_each_sta_info(local, _addr, _sta, _tmp)			\
 	rhl_for_each_entry_rcu(_sta, _tmp,				\
 			       sta_info_hash_lookup(local, _addr), hash_node)
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index ab8ba5835ca0..0e51def35b8a 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -1073,19 +1073,13 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 		.skb = skb,
 		.info = IEEE80211_SKB_CB(skb),
 	};
-	struct rhlist_head *tmp;
 	struct sta_info *sta;
 
 	rcu_read_lock();
 
-	for_each_sta_info(local, hdr->addr1, sta, tmp) {
-		/* skip wrong virtual interface */
-		if (!ether_addr_equal(hdr->addr2, sta->sdata->vif.addr))
-			continue;
-
+	sta = sta_info_get_by_addrs(local, hdr->addr1, hdr->addr2);
+	if (sta)
 		status.sta = &sta->sta;
-		break;
-	}
 
 	__ieee80211_tx_status(hw, &status);
 	rcu_read_unlock();
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH v11 2/4] mac80211: Import airtime calculation code from mt76

[Kan Yan]

From: Toke Høiland-Jørgensen <toke@redhat.com>

Felix recently added code to calculate airtime of packets to the mt76
driver. Import this into mac80211 so we can use it for airtime queue limit
calculations.

The airtime.c file is copied verbatim from the mt76 driver, and adjusted to
be usable in mac80211. This involves:

- Switching to mac80211 data structures.
- Adding support for 160 MHz channels and HE mode.
- Moving the symbol and duration calculations around a bit to avoid
  rounding with the higher rates and longer symbol times used for HE rates.

The per-rate TX rate calculation is also split out to its own function so
it can be used directly for the AQL calculations later.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/mac80211.h     |  29 ++
 net/mac80211/Makefile      |   3 +-
 net/mac80211/airtime.c     | 597 +++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h |   4 +
 4 files changed, 632 insertions(+), 1 deletion(-)
 create mode 100644 net/mac80211/airtime.c

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index c643a19dce96..6fc26a051ba0 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -6424,4 +6424,33 @@ void ieee80211_nan_func_match(struct ieee80211_vif *vif,
 			      struct cfg80211_nan_match_params *match,
 			      gfp_t gfp);
 
+/**
+ * ieee80211_calc_rx_airtime - calculate estimated transmission airtime for RX.
+ *
+ * This function calculates the estimated airtime usage of a frame based on the
+ * rate information in the RX status struct and the frame length.
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @status: &struct ieee80211_rx_status containing the transmission rate
+ *          information.
+ * @len: frame length in bytes
+ */
+u32 ieee80211_calc_rx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_rx_status *status,
+			      int len);
+
+/**
+ * ieee80211_calc_tx_airtime - calculate estimated transmission airtime for TX.
+ *
+ * This function calculates the estimated airtime usage of a frame based on the
+ * rate information in the TX info struct and the frame length.
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @info: &struct ieee80211_tx_info of the frame.
+ * @len: frame length in bytes
+ */
+u32 ieee80211_calc_tx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_tx_info *info,
+			      int len);
+
 #endif /* MAC80211_H */
diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile
index 4f03ebe732fa..6cbb1286d6c0 100644
--- a/net/mac80211/Makefile
+++ b/net/mac80211/Makefile
@@ -32,7 +32,8 @@ mac80211-y := \
 	chan.o \
 	trace.o mlme.o \
 	tdls.o \
-	ocb.o
+	ocb.o \
+	airtime.o
 
 mac80211-$(CONFIG_MAC80211_LEDS) += led.o
 mac80211-$(CONFIG_MAC80211_DEBUGFS) += \
diff --git a/net/mac80211/airtime.c b/net/mac80211/airtime.c
new file mode 100644
index 000000000000..0c2a0bb94727
--- /dev/null
+++ b/net/mac80211/airtime.c
@@ -0,0 +1,597 @@
+// SPDX-License-Identifier: ISC
+/*
+ * Copyright (C) 2019 Felix Fietkau <nbd@nbd.name>
+ */
+
+#include <net/mac80211.h>
+#include "ieee80211_i.h"
+#include "sta_info.h"
+
+#define AVG_PKT_SIZE	1024
+
+/* Number of bits for an average sized packet */
+#define MCS_NBITS (AVG_PKT_SIZE << 3)
+
+/* Number of kilo-symbols (symbols * 1024) for a packet with (bps) bits per
+ * symbol. We use k-symbols to avoid rounding in the _TIME macros below.
+ */
+#define MCS_N_KSYMS(bps) DIV_ROUND_UP(MCS_NBITS << 10, (bps))
+
+/* Transmission time (in 1024 * usec) for a packet containing (ksyms) * 1024
+ * symbols.
+ */
+#define MCS_SYMBOL_TIME(sgi, ksyms)					\
+	(sgi ?								\
+	  ((ksyms) * 4 * 18) / 20 :		/* 3.6 us per sym */	\
+	  ((ksyms) * 4)			/* 4.0 us per sym */	\
+	)
+
+/* Transmit duration for the raw data part of an average sized packet */
+#define MCS_DURATION(streams, sgi, bps) \
+	((u32)MCS_SYMBOL_TIME(sgi, MCS_N_KSYMS((streams) * (bps))))
+
+#define MCS_DURATION_S(shift, streams, sgi, bps)		\
+	((u16)((MCS_DURATION(streams, sgi, bps) >> shift)))
+
+/* These should match the values in enum nl80211_he_gi */
+#define HE_GI_08 0
+#define HE_GI_16 1
+#define HE_GI_32 2
+
+/* Transmission time (1024 usec) for a packet containing (ksyms) * k-symbols */
+#define HE_SYMBOL_TIME(gi, ksyms)					\
+	(gi == HE_GI_08 ?						\
+	 ((ksyms) * 16 * 17) / 20 :		/* 13.6 us per sym */	\
+	 (gi == HE_GI_16 ?						\
+	  ((ksyms) * 16 * 18) / 20 :		/* 14.4 us per sym */	\
+	  ((ksyms) * 16)			/* 16.0 us per sym */	\
+	 ))
+
+/* Transmit duration for the raw data part of an average sized packet */
+#define HE_DURATION(streams, gi, bps) \
+	((u32)HE_SYMBOL_TIME(gi, MCS_N_KSYMS((streams) * (bps))))
+
+#define HE_DURATION_S(shift, streams, gi, bps)		\
+	(HE_DURATION(streams, gi, bps) >> shift)
+
+#define BW_20			0
+#define BW_40			1
+#define BW_80			2
+#define BW_160			3
+
+/*
+ * Define group sort order: HT40 -> SGI -> #streams
+ */
+#define IEEE80211_MAX_STREAMS		4
+#define IEEE80211_HT_STREAM_GROUPS	4 /* BW(=2) * SGI(=2) */
+#define IEEE80211_VHT_STREAM_GROUPS	8 /* BW(=4) * SGI(=2) */
+
+#define IEEE80211_HE_MAX_STREAMS	8
+#define IEEE80211_HE_STREAM_GROUPS	12 /* BW(=4) * GI(=3) */
+
+#define IEEE80211_HT_GROUPS_NB	(IEEE80211_MAX_STREAMS *	\
+				 IEEE80211_HT_STREAM_GROUPS)
+#define IEEE80211_VHT_GROUPS_NB	(IEEE80211_MAX_STREAMS *	\
+					 IEEE80211_VHT_STREAM_GROUPS)
+#define IEEE80211_HE_GROUPS_NB	(IEEE80211_HE_MAX_STREAMS *	\
+				 IEEE80211_HE_STREAM_GROUPS)
+#define IEEE80211_GROUPS_NB	(IEEE80211_HT_GROUPS_NB +	\
+				 IEEE80211_VHT_GROUPS_NB +	\
+				 IEEE80211_HE_GROUPS_NB)
+
+#define IEEE80211_HT_GROUP_0	0
+#define IEEE80211_VHT_GROUP_0	(IEEE80211_HT_GROUP_0 + IEEE80211_HT_GROUPS_NB)
+#define IEEE80211_HE_GROUP_0	(IEEE80211_VHT_GROUP_0 + IEEE80211_VHT_GROUPS_NB)
+
+#define MCS_GROUP_RATES		12
+
+#define HT_GROUP_IDX(_streams, _sgi, _ht40)	\
+	IEEE80211_HT_GROUP_0 +			\
+	IEEE80211_MAX_STREAMS * 2 * _ht40 +	\
+	IEEE80211_MAX_STREAMS * _sgi +		\
+	_streams - 1
+
+#define _MAX(a, b) (((a)>(b))?(a):(b))
+
+#define GROUP_SHIFT(duration)						\
+	_MAX(0, 16 - __builtin_clz(duration))
+
+/* MCS rate information for an MCS group */
+#define __MCS_GROUP(_streams, _sgi, _ht40, _s)				\
+	[HT_GROUP_IDX(_streams, _sgi, _ht40)] = {			\
+	.shift = _s,							\
+	.duration = {							\
+		MCS_DURATION_S(_s, _streams, _sgi, _ht40 ? 54 : 26),	\
+		MCS_DURATION_S(_s, _streams, _sgi, _ht40 ? 108 : 52),	\
+		MCS_DURATION_S(_s, _streams, _sgi, _ht40 ? 162 : 78),	\
+		MCS_DURATION_S(_s, _streams, _sgi, _ht40 ? 216 : 104),	\
+		MCS_DURATION_S(_s, _streams, _sgi, _ht40 ? 324 : 156),	\
+		MCS_DURATION_S(_s, _streams, _sgi, _ht40 ? 432 : 208),	\
+		MCS_DURATION_S(_s, _streams, _sgi, _ht40 ? 486 : 234),	\
+		MCS_DURATION_S(_s, _streams, _sgi, _ht40 ? 540 : 260)	\
+	}								\
+}
+
+#define MCS_GROUP_SHIFT(_streams, _sgi, _ht40)				\
+	GROUP_SHIFT(MCS_DURATION(_streams, _sgi, _ht40 ? 54 : 26))
+
+#define MCS_GROUP(_streams, _sgi, _ht40)				\
+	__MCS_GROUP(_streams, _sgi, _ht40,				\
+		    MCS_GROUP_SHIFT(_streams, _sgi, _ht40))
+
+#define VHT_GROUP_IDX(_streams, _sgi, _bw)				\
+	(IEEE80211_VHT_GROUP_0 +					\
+	 IEEE80211_MAX_STREAMS * 2 * (_bw) +				\
+	 IEEE80211_MAX_STREAMS * (_sgi) +				\
+	 (_streams) - 1)
+
+#define BW2VBPS(_bw, r4, r3, r2, r1)					\
+	(_bw == BW_160 ? r4 : _bw == BW_80 ? r3 : _bw == BW_40 ? r2 : r1)
+
+#define __VHT_GROUP(_streams, _sgi, _bw, _s)				\
+	[VHT_GROUP_IDX(_streams, _sgi, _bw)] = {			\
+	.shift = _s,							\
+	.duration = {							\
+		MCS_DURATION_S(_s, _streams, _sgi,			\
+			       BW2VBPS(_bw,  234,  117,  54,  26)),	\
+		MCS_DURATION_S(_s, _streams, _sgi,			\
+			       BW2VBPS(_bw,  468,  234, 108,  52)),	\
+		MCS_DURATION_S(_s, _streams, _sgi,			\
+			       BW2VBPS(_bw,  702,  351, 162,  78)),	\
+		MCS_DURATION_S(_s, _streams, _sgi,			\
+			       BW2VBPS(_bw,  936,  468, 216, 104)),	\
+		MCS_DURATION_S(_s, _streams, _sgi,			\
+			       BW2VBPS(_bw, 1404,  702, 324, 156)),	\
+		MCS_DURATION_S(_s, _streams, _sgi,			\
+			       BW2VBPS(_bw, 1872,  936, 432, 208)),	\
+		MCS_DURATION_S(_s, _streams, _sgi,			\
+			       BW2VBPS(_bw, 2106, 1053, 486, 234)),	\
+		MCS_DURATION_S(_s, _streams, _sgi,			\
+			       BW2VBPS(_bw, 2340, 1170, 540, 260)),	\
+		MCS_DURATION_S(_s, _streams, _sgi,			\
+			       BW2VBPS(_bw, 2808, 1404, 648, 312)),	\
+		MCS_DURATION_S(_s, _streams, _sgi,			\
+			       BW2VBPS(_bw, 3120, 1560, 720, 346))	\
+        }								\
+}
+
+#define VHT_GROUP_SHIFT(_streams, _sgi, _bw)				\
+	GROUP_SHIFT(MCS_DURATION(_streams, _sgi,			\
+				 BW2VBPS(_bw, 243, 117,  54,  26)))
+
+#define VHT_GROUP(_streams, _sgi, _bw)					\
+	__VHT_GROUP(_streams, _sgi, _bw,				\
+		    VHT_GROUP_SHIFT(_streams, _sgi, _bw))
+
+
+#define HE_GROUP_IDX(_streams, _gi, _bw)				\
+	(IEEE80211_HE_GROUP_0 +					\
+	 IEEE80211_HE_MAX_STREAMS * 2 * (_bw) +			\
+	 IEEE80211_HE_MAX_STREAMS * (_gi) +				\
+	 (_streams) - 1)
+
+#define __HE_GROUP(_streams, _gi, _bw, _s)				\
+	[HE_GROUP_IDX(_streams, _gi, _bw)] = {			\
+	.shift = _s,							\
+	.duration = {							\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw,   979,  489,  230,  115)),	\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw,  1958,  979,  475,  230)),	\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw,  2937, 1468,  705,  345)),	\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw,  3916, 1958,  936,  475)),	\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw,  5875, 2937, 1411,  705)),	\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw,  7833, 3916, 1872,  936)),	\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw,  8827, 4406, 2102, 1051)),	\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw,  9806, 4896, 2347, 1166)),	\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw, 11764, 5875, 2808, 1411)),	\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw, 13060, 6523, 3124, 1555)),	\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw, 14702, 7344, 3513, 1756)),	\
+		HE_DURATION_S(_s, _streams, _gi,			\
+			      BW2VBPS(_bw, 16329, 8164, 3902, 1944))	\
+        }								\
+}
+
+#define HE_GROUP_SHIFT(_streams, _gi, _bw)				\
+	GROUP_SHIFT(HE_DURATION(_streams, _gi,			\
+				BW2VBPS(_bw,   979,  489,  230,  115)))
+
+#define HE_GROUP(_streams, _gi, _bw)					\
+	__HE_GROUP(_streams, _gi, _bw,				\
+		   HE_GROUP_SHIFT(_streams, _gi, _bw))
+struct mcs_group {
+	u8 shift;
+	u16 duration[MCS_GROUP_RATES];
+};
+
+static const struct mcs_group airtime_mcs_groups[] = {
+	MCS_GROUP(1, 0, BW_20),
+	MCS_GROUP(2, 0, BW_20),
+	MCS_GROUP(3, 0, BW_20),
+	MCS_GROUP(4, 0, BW_20),
+
+	MCS_GROUP(1, 1, BW_20),
+	MCS_GROUP(2, 1, BW_20),
+	MCS_GROUP(3, 1, BW_20),
+	MCS_GROUP(4, 1, BW_20),
+
+	MCS_GROUP(1, 0, BW_40),
+	MCS_GROUP(2, 0, BW_40),
+	MCS_GROUP(3, 0, BW_40),
+	MCS_GROUP(4, 0, BW_40),
+
+	MCS_GROUP(1, 1, BW_40),
+	MCS_GROUP(2, 1, BW_40),
+	MCS_GROUP(3, 1, BW_40),
+	MCS_GROUP(4, 1, BW_40),
+
+	VHT_GROUP(1, 0, BW_20),
+	VHT_GROUP(2, 0, BW_20),
+	VHT_GROUP(3, 0, BW_20),
+	VHT_GROUP(4, 0, BW_20),
+
+	VHT_GROUP(1, 1, BW_20),
+	VHT_GROUP(2, 1, BW_20),
+	VHT_GROUP(3, 1, BW_20),
+	VHT_GROUP(4, 1, BW_20),
+
+	VHT_GROUP(1, 0, BW_40),
+	VHT_GROUP(2, 0, BW_40),
+	VHT_GROUP(3, 0, BW_40),
+	VHT_GROUP(4, 0, BW_40),
+
+	VHT_GROUP(1, 1, BW_40),
+	VHT_GROUP(2, 1, BW_40),
+	VHT_GROUP(3, 1, BW_40),
+	VHT_GROUP(4, 1, BW_40),
+
+	VHT_GROUP(1, 0, BW_80),
+	VHT_GROUP(2, 0, BW_80),
+	VHT_GROUP(3, 0, BW_80),
+	VHT_GROUP(4, 0, BW_80),
+
+	VHT_GROUP(1, 1, BW_80),
+	VHT_GROUP(2, 1, BW_80),
+	VHT_GROUP(3, 1, BW_80),
+	VHT_GROUP(4, 1, BW_80),
+
+	VHT_GROUP(1, 0, BW_160),
+	VHT_GROUP(2, 0, BW_160),
+	VHT_GROUP(3, 0, BW_160),
+	VHT_GROUP(4, 0, BW_160),
+
+	VHT_GROUP(1, 1, BW_160),
+	VHT_GROUP(2, 1, BW_160),
+	VHT_GROUP(3, 1, BW_160),
+	VHT_GROUP(4, 1, BW_160),
+
+	HE_GROUP(1, HE_GI_08, BW_20),
+	HE_GROUP(2, HE_GI_08, BW_20),
+	HE_GROUP(3, HE_GI_08, BW_20),
+	HE_GROUP(4, HE_GI_08, BW_20),
+	HE_GROUP(5, HE_GI_08, BW_20),
+	HE_GROUP(6, HE_GI_08, BW_20),
+	HE_GROUP(7, HE_GI_08, BW_20),
+	HE_GROUP(8, HE_GI_08, BW_20),
+
+	HE_GROUP(1, HE_GI_16, BW_20),
+	HE_GROUP(2, HE_GI_16, BW_20),
+	HE_GROUP(3, HE_GI_16, BW_20),
+	HE_GROUP(4, HE_GI_16, BW_20),
+	HE_GROUP(5, HE_GI_16, BW_20),
+	HE_GROUP(6, HE_GI_16, BW_20),
+	HE_GROUP(7, HE_GI_16, BW_20),
+	HE_GROUP(8, HE_GI_16, BW_20),
+
+	HE_GROUP(1, HE_GI_32, BW_20),
+	HE_GROUP(2, HE_GI_32, BW_20),
+	HE_GROUP(3, HE_GI_32, BW_20),
+	HE_GROUP(4, HE_GI_32, BW_20),
+	HE_GROUP(5, HE_GI_32, BW_20),
+	HE_GROUP(6, HE_GI_32, BW_20),
+	HE_GROUP(7, HE_GI_32, BW_20),
+	HE_GROUP(8, HE_GI_32, BW_20),
+
+	HE_GROUP(1, HE_GI_08, BW_40),
+	HE_GROUP(2, HE_GI_08, BW_40),
+	HE_GROUP(3, HE_GI_08, BW_40),
+	HE_GROUP(4, HE_GI_08, BW_40),
+	HE_GROUP(5, HE_GI_08, BW_40),
+	HE_GROUP(6, HE_GI_08, BW_40),
+	HE_GROUP(7, HE_GI_08, BW_40),
+	HE_GROUP(8, HE_GI_08, BW_40),
+
+	HE_GROUP(1, HE_GI_16, BW_40),
+	HE_GROUP(2, HE_GI_16, BW_40),
+	HE_GROUP(3, HE_GI_16, BW_40),
+	HE_GROUP(4, HE_GI_16, BW_40),
+	HE_GROUP(5, HE_GI_16, BW_40),
+	HE_GROUP(6, HE_GI_16, BW_40),
+	HE_GROUP(7, HE_GI_16, BW_40),
+	HE_GROUP(8, HE_GI_16, BW_40),
+
+	HE_GROUP(1, HE_GI_32, BW_40),
+	HE_GROUP(2, HE_GI_32, BW_40),
+	HE_GROUP(3, HE_GI_32, BW_40),
+	HE_GROUP(4, HE_GI_32, BW_40),
+	HE_GROUP(5, HE_GI_32, BW_40),
+	HE_GROUP(6, HE_GI_32, BW_40),
+	HE_GROUP(7, HE_GI_32, BW_40),
+	HE_GROUP(8, HE_GI_32, BW_40),
+
+	HE_GROUP(1, HE_GI_08, BW_80),
+	HE_GROUP(2, HE_GI_08, BW_80),
+	HE_GROUP(3, HE_GI_08, BW_80),
+	HE_GROUP(4, HE_GI_08, BW_80),
+	HE_GROUP(5, HE_GI_08, BW_80),
+	HE_GROUP(6, HE_GI_08, BW_80),
+	HE_GROUP(7, HE_GI_08, BW_80),
+	HE_GROUP(8, HE_GI_08, BW_80),
+
+	HE_GROUP(1, HE_GI_16, BW_80),
+	HE_GROUP(2, HE_GI_16, BW_80),
+	HE_GROUP(3, HE_GI_16, BW_80),
+	HE_GROUP(4, HE_GI_16, BW_80),
+	HE_GROUP(5, HE_GI_16, BW_80),
+	HE_GROUP(6, HE_GI_16, BW_80),
+	HE_GROUP(7, HE_GI_16, BW_80),
+	HE_GROUP(8, HE_GI_16, BW_80),
+
+	HE_GROUP(1, HE_GI_32, BW_80),
+	HE_GROUP(2, HE_GI_32, BW_80),
+	HE_GROUP(3, HE_GI_32, BW_80),
+	HE_GROUP(4, HE_GI_32, BW_80),
+	HE_GROUP(5, HE_GI_32, BW_80),
+	HE_GROUP(6, HE_GI_32, BW_80),
+	HE_GROUP(7, HE_GI_32, BW_80),
+	HE_GROUP(8, HE_GI_32, BW_80),
+
+	HE_GROUP(1, HE_GI_08, BW_160),
+	HE_GROUP(2, HE_GI_08, BW_160),
+	HE_GROUP(3, HE_GI_08, BW_160),
+	HE_GROUP(4, HE_GI_08, BW_160),
+	HE_GROUP(5, HE_GI_08, BW_160),
+	HE_GROUP(6, HE_GI_08, BW_160),
+	HE_GROUP(7, HE_GI_08, BW_160),
+	HE_GROUP(8, HE_GI_08, BW_160),
+
+	HE_GROUP(1, HE_GI_16, BW_160),
+	HE_GROUP(2, HE_GI_16, BW_160),
+	HE_GROUP(3, HE_GI_16, BW_160),
+	HE_GROUP(4, HE_GI_16, BW_160),
+	HE_GROUP(5, HE_GI_16, BW_160),
+	HE_GROUP(6, HE_GI_16, BW_160),
+	HE_GROUP(7, HE_GI_16, BW_160),
+	HE_GROUP(8, HE_GI_16, BW_160),
+
+	HE_GROUP(1, HE_GI_32, BW_160),
+	HE_GROUP(2, HE_GI_32, BW_160),
+	HE_GROUP(3, HE_GI_32, BW_160),
+	HE_GROUP(4, HE_GI_32, BW_160),
+	HE_GROUP(5, HE_GI_32, BW_160),
+	HE_GROUP(6, HE_GI_32, BW_160),
+	HE_GROUP(7, HE_GI_32, BW_160),
+	HE_GROUP(8, HE_GI_32, BW_160),
+};
+
+static u32
+ieee80211_calc_legacy_rate_duration(u16 bitrate, bool short_pre,
+				    bool cck, int len)
+{
+	u32 duration;
+
+	if (cck) {
+		duration = 144 + 48; /* preamble + PLCP */
+		if (short_pre)
+			duration >>= 1;
+
+		duration += 10; /* SIFS */
+	} else {
+		duration = 20 + 16; /* premable + SIFS */
+	}
+
+	len <<= 3;
+	duration += (len * 10) / bitrate;
+
+	return duration;
+}
+
+u32 ieee80211_calc_rx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_rx_status *status,
+			      int len)
+{
+	struct ieee80211_supported_band *sband;
+	const struct ieee80211_rate *rate;
+	bool sgi = status->enc_flags & RX_ENC_FLAG_SHORT_GI;
+	bool sp = status->enc_flags & RX_ENC_FLAG_SHORTPRE;
+	int bw, streams;
+	int group, idx;
+	u32 duration;
+	bool cck;
+
+	switch (status->bw) {
+	case RATE_INFO_BW_20:
+		bw = BW_20;
+		break;
+	case RATE_INFO_BW_40:
+		bw = BW_40;
+		break;
+	case RATE_INFO_BW_80:
+		bw = BW_80;
+		break;
+	case RATE_INFO_BW_160:
+		bw = BW_160;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
+	switch (status->encoding) {
+	case RX_ENC_LEGACY:
+		if (WARN_ON_ONCE(status->band > NL80211_BAND_5GHZ))
+			return 0;
+
+		sband = hw->wiphy->bands[status->band];
+		if (!sband || status->rate_idx > sband->n_bitrates)
+			return 0;
+
+		rate = &sband->bitrates[status->rate_idx];
+		cck = rate->flags & IEEE80211_RATE_MANDATORY_B;
+
+		return ieee80211_calc_legacy_rate_duration(rate->bitrate, sp,
+							   cck, len);
+
+	case RX_ENC_VHT:
+		streams = status->nss;
+		idx = status->rate_idx;
+		group = VHT_GROUP_IDX(streams, sgi, bw);
+		break;
+	case RX_ENC_HT:
+		streams = ((status->rate_idx >> 3) & 3) + 1;
+		idx = status->rate_idx & 7;
+		group = HT_GROUP_IDX(streams, sgi, bw);
+		break;
+	case RX_ENC_HE:
+		streams = status->nss;
+		idx = status->rate_idx;
+		group = HE_GROUP_IDX(streams, status->he_gi, bw);
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
+	if (WARN_ON_ONCE((status->encoding != RX_ENC_HE && streams > 4) ||
+			 (status->encoding == RX_ENC_HE && streams > 8)))
+		return 0;
+
+	duration = airtime_mcs_groups[group].duration[idx];
+	duration <<= airtime_mcs_groups[group].shift;
+	duration *= len;
+	duration /= AVG_PKT_SIZE;
+	duration /= 1024;
+
+	duration += 36 + (streams << 2);
+
+	return duration;
+}
+EXPORT_SYMBOL_GPL(ieee80211_calc_rx_airtime);
+
+static u32 ieee80211_calc_tx_airtime_rate(struct ieee80211_hw *hw,
+					  struct ieee80211_tx_rate *rate,
+					  u8 band, int len)
+{
+	struct ieee80211_rx_status stat = {
+		.band = band,
+	};
+
+	if (rate->idx < 0 || !rate->count)
+		return 0;
+
+	if (rate->flags & IEEE80211_TX_RC_80_MHZ_WIDTH)
+		stat.bw = RATE_INFO_BW_80;
+	else if (rate->flags & IEEE80211_TX_RC_40_MHZ_WIDTH)
+		stat.bw = RATE_INFO_BW_40;
+	else
+		stat.bw = RATE_INFO_BW_20;
+
+	stat.enc_flags = 0;
+	if (rate->flags & IEEE80211_TX_RC_USE_SHORT_PREAMBLE)
+		stat.enc_flags |= RX_ENC_FLAG_SHORTPRE;
+	if (rate->flags & IEEE80211_TX_RC_SHORT_GI)
+		stat.enc_flags |= RX_ENC_FLAG_SHORT_GI;
+
+	stat.rate_idx = rate->idx;
+	if (rate->flags & IEEE80211_TX_RC_VHT_MCS) {
+		stat.encoding = RX_ENC_VHT;
+		stat.rate_idx = ieee80211_rate_get_vht_mcs(rate);
+		stat.nss = ieee80211_rate_get_vht_nss(rate);
+	} else if (rate->flags & IEEE80211_TX_RC_MCS) {
+		stat.encoding = RX_ENC_HT;
+	} else {
+		stat.encoding = RX_ENC_LEGACY;
+	}
+
+	return ieee80211_calc_rx_airtime(hw, &stat, len);
+}
+
+u32 ieee80211_calc_tx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_tx_info *info,
+			      int len)
+{
+	u32 duration = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(info->status.rates); i++) {
+		struct ieee80211_tx_rate *rate = &info->status.rates[i];
+		u32 cur_duration;
+
+		cur_duration = ieee80211_calc_tx_airtime_rate(hw, rate,
+							      info->band, len);
+		if (!cur_duration)
+			break;
+
+		duration += cur_duration * rate->count;
+	}
+
+	return duration;
+}
+EXPORT_SYMBOL_GPL(ieee80211_calc_tx_airtime);
+
+u32 ieee80211_calc_expected_tx_airtime(struct ieee80211_hw *hw,
+				       struct ieee80211_vif *vif,
+				       struct ieee80211_sta *pubsta,
+				       int len)
+{
+	struct ieee80211_supported_band *sband;
+	struct ieee80211_chanctx_conf *conf;
+	int rateidx, shift = 0;
+	bool cck, short_pream;
+	u32 basic_rates;
+	u8 band = 0;
+	u16 rate;
+
+	len += 38; /* Ethernet header length */
+
+	conf = rcu_dereference(vif->chanctx_conf);
+	if (conf) {
+		band = conf->def.chan->band;
+		shift = ieee80211_chandef_get_shift(&conf->def);
+	}
+
+	if (pubsta) {
+		struct sta_info *sta = container_of(pubsta, struct sta_info,
+						    sta);
+
+		return ieee80211_calc_tx_airtime_rate(hw,
+						      &sta->tx_stats.last_rate,
+						      band, len);
+	}
+
+	if (!conf)
+		return 0;
+
+	/* No station to get latest rate from, so calculate the worst-case
+	 * duration using the lowest configured basic rate.
+	 */
+	sband = hw->wiphy->bands[band];
+
+	basic_rates = vif->bss_conf.basic_rates;
+	short_pream = vif->bss_conf.use_short_preamble;
+
+	rateidx = basic_rates ? ffs(basic_rates) - 1 : 0;
+	rate = sband->bitrates[rateidx].bitrate << shift;
+	cck = sband->bitrates[rateidx].flags & IEEE80211_RATE_MANDATORY_B;
+
+	return ieee80211_calc_legacy_rate_duration(rate, short_pream, cck, len);
+}
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 05406e9c05b3..225ea4e3cd76 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -2249,6 +2249,10 @@ const char *ieee80211_get_reason_code_string(u16 reason_code);
 
 extern const struct ethtool_ops ieee80211_ethtool_ops;
 
+u32 ieee80211_calc_expected_tx_airtime(struct ieee80211_hw *hw,
+				       struct ieee80211_vif *vif,
+				       struct ieee80211_sta *pubsta,
+				       int len);
 #ifdef CONFIG_MAC80211_NOINLINE
 #define debug_noinline noinline
 #else
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH v11 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)

[Kan Yan]

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>
---
 include/net/cfg80211.h     |  7 ++++
 include/net/mac80211.h     | 12 ++++++
 net/mac80211/debugfs.c     | 85 ++++++++++++++++++++++++++++++++++++++
 net/mac80211/debugfs_sta.c | 43 ++++++++++++++-----
 net/mac80211/ieee80211_i.h |  4 ++
 net/mac80211/main.c        | 10 ++++-
 net/mac80211/sta_info.c    | 38 +++++++++++++++++
 net/mac80211/sta_info.h    |  8 ++++
 net/mac80211/tx.c          | 51 +++++++++++++++++++++--
 9 files changed, 244 insertions(+), 14 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index e309cc826b40..b0f440857aeb 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2606,6 +2606,13 @@ enum wiphy_params_flags {
 
 #define IEEE80211_DEFAULT_AIRTIME_WEIGHT	256
 
+/* The per TXQ device queue limit in airtime */
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L	5000
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H	12000
+
+/* The per interface airtime threshold to switch to lower queue limit */
+#define IEEE80211_AQL_THRESHOLD			24000
+
 /**
  * struct cfg80211_pmksa - PMK Security Association
  *
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 6fc26a051ba0..ba3f33cc41ea 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5565,6 +5565,18 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
 void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
 				    u32 tx_airtime, u32 rx_airtime);
 
+/**
+ * ieee80211_txq_airtime_check - check if a txq can send frame to device
+ *
+ * @hw: pointer obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Return true if the AQL's airtime limit has not been reached and the txq can
+ * continue to send more packets to the device. Otherwise return false.
+ */
+bool
+ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
+
 /**
  * ieee80211_iter_keys - iterate keys programmed into the device
  * @hw: pointer obtained from ieee80211_alloc_hw()
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 568b3b276931..399d4e8b8546 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -148,6 +148,87 @@ static const struct file_operations aqm_ops = {
 	.llseek = default_llseek,
 };
 
+static ssize_t aql_txq_limit_read(struct file *file,
+				  char __user *user_buf,
+				  size_t count,
+				  loff_t *ppos)
+{
+	struct ieee80211_local *local = file->private_data;
+	char buf[400];
+	int len = 0;
+
+	len = scnprintf(buf, sizeof(buf),
+			"AC	AQL limit low	AQL limit high\n"
+			"VO	%u		%u\n"
+			"VI	%u		%u\n"
+			"BE	%u		%u\n"
+			"BK	%u		%u\n",
+			local->aql_txq_limit_low[IEEE80211_AC_VO],
+			local->aql_txq_limit_high[IEEE80211_AC_VO],
+			local->aql_txq_limit_low[IEEE80211_AC_VI],
+			local->aql_txq_limit_high[IEEE80211_AC_VI],
+			local->aql_txq_limit_low[IEEE80211_AC_BE],
+			local->aql_txq_limit_high[IEEE80211_AC_BE],
+			local->aql_txq_limit_low[IEEE80211_AC_BK],
+			local->aql_txq_limit_high[IEEE80211_AC_BK]);
+	return simple_read_from_buffer(user_buf, count, ppos,
+				       buf, len);
+}
+
+static ssize_t aql_txq_limit_write(struct file *file,
+				   const char __user *user_buf,
+				   size_t count,
+				   loff_t *ppos)
+{
+	struct ieee80211_local *local = file->private_data;
+	char buf[100];
+	size_t len;
+	u32 ac, q_limit_low, q_limit_high, q_limit_low_old, q_limit_high_old;
+	struct sta_info *sta;
+
+	if (count > sizeof(buf))
+		return -EINVAL;
+
+	if (copy_from_user(buf, user_buf, count))
+		return -EFAULT;
+
+	buf[sizeof(buf) - 1] = 0;
+	len = strlen(buf);
+	if (len > 0 && buf[len - 1] == '\n')
+		buf[len - 1] = 0;
+
+	if (sscanf(buf, "%u %u %u", &ac, &q_limit_low, &q_limit_high) != 3)
+		return -EINVAL;
+
+	if (ac >= IEEE80211_NUM_ACS)
+		return -EINVAL;
+
+	q_limit_low_old = local->aql_txq_limit_low[ac];
+	q_limit_high_old = local->aql_txq_limit_high[ac];
+
+	local->aql_txq_limit_low[ac] = q_limit_low;
+	local->aql_txq_limit_high[ac] = q_limit_high;
+
+	mutex_lock(&local->sta_mtx);
+	list_for_each_entry(sta, &local->sta_list, list) {
+		/* If a sta has customized queue limits, keep it */
+		if (sta->airtime[ac].aql_limit_low == q_limit_low_old &&
+		    sta->airtime[ac].aql_limit_high == q_limit_high_old) {
+			sta->airtime[ac].aql_limit_low = q_limit_low;
+			sta->airtime[ac].aql_limit_high = q_limit_high;
+		}
+	}
+	mutex_unlock(&local->sta_mtx);
+	return count;
+}
+
+static const struct file_operations aql_txq_limit_ops = {
+	.write = aql_txq_limit_write,
+	.read = aql_txq_limit_read,
+	.open = simple_open,
+	.llseek = default_llseek,
+};
+
 static ssize_t force_tx_status_read(struct file *file,
 				    char __user *user_buf,
 				    size_t count,
@@ -441,6 +522,10 @@ void debugfs_hw_add(struct ieee80211_local *local)
 	debugfs_create_u16("airtime_flags", 0600,
 			   phyd, &local->airtime_flags);
 
+	DEBUGFS_ADD(aql_txq_limit);
+	debugfs_create_u32("aql_threshold", 0600,
+			   phyd, &local->aql_threshold);
+
 	statsd = debugfs_create_dir("statistics", phyd);
 
 	/* if the dir failed, don't put all the other things into the root! */
diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index c8ad20c28c43..0185e6e5e5d1 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -197,10 +197,12 @@ static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
 {
 	struct sta_info *sta = file->private_data;
 	struct ieee80211_local *local = sta->sdata->local;
-	size_t bufsz = 200;
+	size_t bufsz = 400;
 	char *buf = kzalloc(bufsz, GFP_KERNEL), *p = buf;
 	u64 rx_airtime = 0, tx_airtime = 0;
 	s64 deficit[IEEE80211_NUM_ACS];
+	u32 q_depth[IEEE80211_NUM_ACS];
+	u32 q_limit_l[IEEE80211_NUM_ACS], q_limit_h[IEEE80211_NUM_ACS];
 	ssize_t rv;
 	int ac;
 
@@ -212,19 +214,22 @@ static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
 		rx_airtime += sta->airtime[ac].rx_airtime;
 		tx_airtime += sta->airtime[ac].tx_airtime;
 		deficit[ac] = sta->airtime[ac].deficit;
+		q_limit_l[ac] = sta->airtime[ac].aql_limit_low;
+		q_limit_h[ac] = sta->airtime[ac].aql_limit_high;
 		spin_unlock_bh(&local->active_txq_lock[ac]);
+		q_depth[ac] = atomic_read(&sta->airtime[ac].aql_tx_pending);
 	}
 
 	p += scnprintf(p, bufsz + buf - p,
 		"RX: %llu us\nTX: %llu us\nWeight: %u\n"
-		"Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n",
-		rx_airtime,
-		tx_airtime,
-		sta->airtime_weight,
-		deficit[0],
-		deficit[1],
-		deficit[2],
-		deficit[3]);
+		"Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n"
+		"Q depth: VO: %u us VI: %u us BE: %u us BK: %u us\n"
+		"Q limit[low/high]: VO: %u/%u VI: %u/%u BE: %u/%u BK: %u/%u\n",
+		rx_airtime, tx_airtime, sta->airtime_weight,
+		deficit[0], deficit[1], deficit[2], deficit[3],
+		q_depth[0], q_depth[1], q_depth[2], q_depth[3],
+		q_limit_l[0], q_limit_h[0], q_limit_l[1], q_limit_h[1],
+		q_limit_l[2], q_limit_h[2], q_limit_l[3], q_limit_h[3]),
 
 	rv = simple_read_from_buffer(userbuf, count, ppos, buf, p - buf);
 	kfree(buf);
@@ -236,7 +241,25 @@ static ssize_t sta_airtime_write(struct file *file, const char __user *userbuf,
 {
 	struct sta_info *sta = file->private_data;
 	struct ieee80211_local *local = sta->sdata->local;
-	int ac;
+	u32 ac, q_limit_l, q_limit_h;
+	char _buf[100] = {}, *buf = _buf;
+
+	if (count > sizeof(_buf))
+		return -EINVAL;
+
+	if (copy_from_user(buf, userbuf, count))
+		return -EFAULT;
+
+	buf[sizeof(_buf) - 1] = '\0';
+	if (sscanf(buf, "queue limit %u %u %u", &ac, &q_limit_l, &q_limit_h)
+	    != 3)
+		return -EINVAL;
+
+	if (ac >= IEEE80211_NUM_ACS)
+		return -EINVAL;
+
+	sta->airtime[ac].aql_limit_low = q_limit_l;
+	sta->airtime[ac].aql_limit_high = q_limit_h;
 
 	for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
 		spin_lock_bh(&local->active_txq_lock[ac]);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 225ea4e3cd76..ad15b3be8bb3 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1142,6 +1142,10 @@ struct ieee80211_local {
 	u16 schedule_round[IEEE80211_NUM_ACS];
 
 	u16 airtime_flags;
+	u32 aql_txq_limit_low[IEEE80211_NUM_ACS];
+	u32 aql_txq_limit_high[IEEE80211_NUM_ACS];
+	u32 aql_threshold;
+	atomic_t aql_total_pending_airtime;
 
 	const struct ieee80211_ops *ops;
 
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index aba094b4ccfc..071ea92a3748 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -667,8 +667,16 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
 		INIT_LIST_HEAD(&local->active_txqs[i]);
 		spin_lock_init(&local->active_txq_lock[i]);
+		local->aql_txq_limit_low[i] = IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L;
+		local->aql_txq_limit_high[i] =
+			IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H;
 	}
-	local->airtime_flags = AIRTIME_USE_TX | AIRTIME_USE_RX;
+
+	local->airtime_flags = AIRTIME_USE_TX |
+			       AIRTIME_USE_RX |
+			       AIRTIME_USE_AQL;
+	local->aql_threshold = IEEE80211_AQL_THRESHOLD;
+	atomic_set(&local->aql_total_pending_airtime, 0);
 
 	INIT_LIST_HEAD(&local->chanctx_list);
 	mutex_init(&local->chanctx_mtx);
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 465d83b13582..1fe02022a31a 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -416,6 +416,9 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 		skb_queue_head_init(&sta->ps_tx_buf[i]);
 		skb_queue_head_init(&sta->tx_filtered[i]);
 		sta->airtime[i].deficit = sta->airtime_weight;
+		atomic_set(&sta->airtime[i].aql_tx_pending, 0);
+		sta->airtime[i].aql_limit_low = local->aql_txq_limit_low[i];
+		sta->airtime[i].aql_limit_high = local->aql_txq_limit_high[i];
 	}
 
 	for (i = 0; i < IEEE80211_NUM_TIDS; i++)
@@ -1913,6 +1916,41 @@ void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
 }
 EXPORT_SYMBOL(ieee80211_sta_register_airtime);
 
+void ieee80211_sta_update_pending_airtime(struct ieee80211_local *local,
+					  struct sta_info *sta, u8 ac,
+					  u16 tx_airtime, bool tx_completed)
+{
+	int tx_pending;
+
+	if (!tx_completed) {
+		if (sta)
+			atomic_add(tx_airtime,
+				   &sta->airtime[ac].aql_tx_pending);
+
+		atomic_add(tx_airtime, &local->aql_total_pending_airtime);
+		return;
+	}
+
+	if (sta) {
+		tx_pending = atomic_sub_return(tx_airtime,
+					       &sta->airtime[ac].aql_tx_pending);
+		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);
+	}
+
+	tx_pending = atomic_sub_return(tx_airtime,
+				       &local->aql_total_pending_airtime);
+	if (WARN_ONCE(tx_pending < 0,
+		      "Device %s AC %d pending airtime underflow: %u, %u",
+		      wiphy_name(local->hw.wiphy), ac, tx_pending,
+		      tx_airtime))
+		atomic_cmpxchg(&local->aql_total_pending_airtime,
+			       tx_pending, 0);
+}
+
 int sta_info_move_state(struct sta_info *sta,
 			enum ieee80211_sta_state new_state)
 {
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 80e76569144e..0608d49b3826 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -127,13 +127,21 @@ enum ieee80211_agg_stop_reason {
 /* Debugfs flags to enable/disable use of RX/TX airtime in scheduler */
 #define AIRTIME_USE_TX		BIT(0)
 #define AIRTIME_USE_RX		BIT(1)
+#define AIRTIME_USE_AQL		BIT(2)
 
 struct airtime_info {
 	u64 rx_airtime;
 	u64 tx_airtime;
 	s64 deficit;
+	atomic_t aql_tx_pending; /* Estimated airtime for frames pending */
+	u32 aql_limit_low;
+	u32 aql_limit_high;
 };
 
+void ieee80211_sta_update_pending_airtime(struct ieee80211_local *local,
+					  struct sta_info *sta, u8 ac,
+					  u16 tx_airtime, bool tx_completed);
+
 struct sta_info;
 
 /**
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index db38be1b75fa..59db921d1086 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3674,7 +3674,8 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
 	struct ieee80211_txq *ret = NULL;
-	struct txq_info *txqi = NULL;
+	struct txq_info *txqi = NULL, *head = NULL;
+	bool found_eligible_txq = false;
 
 	spin_lock_bh(&local->active_txq_lock[ac]);
 
@@ -3685,13 +3686,30 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
 	if (!txqi)
 		goto out;
 
+	if (txqi == head) {
+		if (!found_eligible_txq)
+			goto out;
+		else
+			found_eligible_txq = false;
+	}
+
+	if (!head)
+		head = txqi;
+
 	if (txqi->txq.sta) {
 		struct sta_info *sta = container_of(txqi->txq.sta,
-						struct sta_info, sta);
+						    struct sta_info, sta);
+		bool aql_check = ieee80211_txq_airtime_check(hw, &txqi->txq);
+		s64 deficit = sta->airtime[txqi->txq.ac].deficit;
 
-		if (sta->airtime[txqi->txq.ac].deficit < 0) {
+		if (aql_check)
+			found_eligible_txq = true;
+
+		if (deficit < 0)
 			sta->airtime[txqi->txq.ac].deficit +=
 				sta->airtime_weight;
+
+		if (deficit < 0 || !aql_check) {
 			list_move_tail(&txqi->schedule_order,
 				       &local->active_txqs[txqi->txq.ac]);
 			goto begin;
@@ -3745,6 +3763,33 @@ void __ieee80211_schedule_txq(struct ieee80211_hw *hw,
 }
 EXPORT_SYMBOL(__ieee80211_schedule_txq);
 
+bool ieee80211_txq_airtime_check(struct ieee80211_hw *hw,
+				 struct ieee80211_txq *txq)
+{
+	struct sta_info *sta;
+	struct ieee80211_local *local = hw_to_local(hw);
+
+	if (!(local->airtime_flags & AIRTIME_USE_AQL))
+		return true;
+
+	if (!txq->sta)
+		return true;
+
+	sta = container_of(txq->sta, struct sta_info, sta);
+	if (atomic_read(&sta->airtime[txq->ac].aql_tx_pending) <
+	    sta->airtime[txq->ac].aql_limit_low)
+		return true;
+
+	if (atomic_read(&local->aql_total_pending_airtime) <
+	    local->aql_threshold &&
+	    atomic_read(&sta->airtime[txq->ac].aql_tx_pending) <
+	    sta->airtime[txq->ac].aql_limit_high)
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL(ieee80211_txq_airtime_check);
+
 bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
 				struct ieee80211_txq *txq)
 {
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Kan Yan]

From: Toke Høiland-Jørgensen <toke@redhat.com>

The previous commit added the ability to throttle stations when they queue
too much airtime in the hardware. This commit enables the functionality by
calculating the expected airtime usage of each packet that is dequeued from
the TXQs in mac80211, and accounting that as pending airtime.

The estimated airtime for each skb is stored in the tx_info, so we can
subtract the same amount from the running total when the skb is freed or
recycled. The throttling mechanism relies on this accounting to be
accurate (i.e., that we are not freeing skbs without subtracting any
airtime they were accounted for), so we put the subtraction into
ieee80211_report_used_skb(). As an optimisation, we also subtract the
airtime on regular TX completion, zeroing out the value stored in the
packet afterwards, to avoid having to do an expensive lookup of the station
from the packet data on every packet.

This patch does *not* include any mechanism to wake a throttled TXQ again,
on the assumption that this will happen anyway as a side effect of whatever
freed the skb (most commonly a TX completion).

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/mac80211.h | 16 ++++++++++++++++
 net/mac80211/status.c  | 26 ++++++++++++++++++++++++++
 net/mac80211/tx.c      | 18 ++++++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ba3f33cc41ea..aa145808e57a 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1060,6 +1060,22 @@ struct ieee80211_tx_info {
 	};
 };
 
+static inline u16
+ieee80211_info_set_tx_time_est(struct ieee80211_tx_info *info, u16 tx_time_est)
+{
+	/* We only have 10 bits in tx_time_est, so store airtime
+	 * in increments of 4us and clamp the maximum to 2**12-1
+	 */
+	info->tx_time_est = min_t(u16, tx_time_est, 4095) >> 2;
+	return info->tx_time_est << 2;
+}
+
+static inline u16
+ieee80211_info_get_tx_time_est(struct ieee80211_tx_info *info)
+{
+	return info->tx_time_est << 2;
+}
+
 /**
  * struct ieee80211_tx_status - extended tx status info for rate control
  *
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 0e51def35b8a..39da82b35be9 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -670,12 +670,26 @@ static void ieee80211_report_used_skb(struct ieee80211_local *local,
 				      struct sk_buff *skb, bool dropped)
 {
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+	u16 tx_time_est = ieee80211_info_get_tx_time_est(info);
 	struct ieee80211_hdr *hdr = (void *)skb->data;
 	bool acked = info->flags & IEEE80211_TX_STAT_ACK;
 
 	if (dropped)
 		acked = false;
 
+	if (tx_time_est) {
+		struct sta_info *sta;
+
+		rcu_read_lock();
+
+		sta = sta_info_get_by_addrs(local, hdr->addr1, hdr->addr2);
+		ieee80211_sta_update_pending_airtime(local, sta,
+						     skb_get_queue_mapping(skb),
+						     tx_time_est,
+						     true);
+		rcu_read_unlock();
+	}
+
 	if (info->flags & IEEE80211_TX_INTFL_MLME_CONN_TX) {
 		struct ieee80211_sub_if_data *sdata;
 
@@ -877,6 +891,7 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
 	struct ieee80211_bar *bar;
 	int shift = 0;
 	int tid = IEEE80211_NUM_TIDS;
+	u16 tx_time_est;
 
 	rates_idx = ieee80211_tx_get_rates(hw, info, &retry_count);
 
@@ -986,6 +1001,17 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
 			ieee80211_sta_register_airtime(&sta->sta, tid,
 						       info->status.tx_time, 0);
 
+		if ((tx_time_est = ieee80211_info_get_tx_time_est(info)) > 0) {
+			/* Do this here to avoid the expensive lookup of the sta
+			 * in ieee80211_report_used_skb().
+			 */
+			ieee80211_sta_update_pending_airtime(local, sta,
+							     skb_get_queue_mapping(skb),
+							     tx_time_est,
+							     true);
+			ieee80211_info_set_tx_time_est(info, 0);
+		}
+
 		if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS)) {
 			if (info->flags & IEEE80211_TX_STAT_ACK) {
 				if (sta->status_stats.lost_packets)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 59db921d1086..dfbaab92dbf2 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3551,6 +3551,9 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 
 	WARN_ON_ONCE(softirq_count() == 0);
 
+	if (!ieee80211_txq_airtime_check(hw, txq))
+		return NULL;
+
 begin:
 	spin_lock_bh(&fq->lock);
 
@@ -3661,6 +3664,21 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	}
 
 	IEEE80211_SKB_CB(skb)->control.vif = vif;
+
+	if (local->airtime_flags & AIRTIME_USE_AQL) {
+		u32 airtime;
+
+		airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
+							     skb->len);
+		if (airtime) {
+			airtime = ieee80211_info_set_tx_time_est(info, airtime);
+			ieee80211_sta_update_pending_airtime(local, tx.sta,
+							     txq->ac,
+							     airtime,
+							     false);
+		}
+	}
+
 	return skb;
 
 out:
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH v11 2/4] mac80211: Import airtime calculation code from mt76

[Johannes Berg]

On Mon, 2019-11-18 at 22:06 -0800, Kan Yan wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> Felix recently added code to calculate airtime of packets to the mt76
> driver. Import this into mac80211 so we can use it for airtime queue limit
> calculations.
> 
> The airtime.c file is copied verbatim from the mt76 driver, and adjusted to
> be usable in mac80211. This involves:
> 
> - Switching to mac80211 data structures.
> - Adding support for 160 MHz channels and HE mode.
> - Moving the symbol and duration calculations around a bit to avoid
>   rounding with the higher rates and longer symbol times used for HE rates.

:)

I'll apply this, I guess, but I do wonder to what extent it overlaps
with cfg80211_calculate_bitrate()?

johannes

Re: [PATCH v11 2/4] mac80211: Import airtime calculation code from mt76

[Johannes Berg]

On Mon, 2019-11-18 at 22:06 -0800, Kan Yan wrote:
> 
> +#define HE_GROUP_IDX(_streams, _gi, _bw)				\
> +	(IEEE80211_HE_GROUP_0 +					\
> +	 IEEE80211_HE_MAX_STREAMS * 2 * (_bw) +			\
> +	 IEEE80211_HE_MAX_STREAMS * (_gi) +				\
> +	 (_streams) - 1)

I'll also fix that to be " * 3" instead of 2, since there are 3 possible
_gi values.

johannes

Re: [PATCH v11 2/4] mac80211: Import airtime calculation code from mt76

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Mon, 2019-11-18 at 22:06 -0800, Kan Yan wrote:
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> Felix recently added code to calculate airtime of packets to the mt76
>> driver. Import this into mac80211 so we can use it for airtime queue limit
>> calculations.
>> 
>> The airtime.c file is copied verbatim from the mt76 driver, and adjusted to
>> be usable in mac80211. This involves:
>> 
>> - Switching to mac80211 data structures.
>> - Adding support for 160 MHz channels and HE mode.
>> - Moving the symbol and duration calculations around a bit to avoid
>>   rounding with the higher rates and longer symbol times used for HE rates.
>
> :)
>
> I'll apply this, I guess,

Great!

> but I do wonder to what extent it overlaps with
> cfg80211_calculate_bitrate()?

Well, one calculates bitrates while the other calculates airtime? ;)

But yeah, I get what you mean. I think Felix went through quite some
pains to structure this code to avoid divisions in the fast path. I
guess that is the main blocker for cfg80211_calculate_bitrate() to be
used instead (assuming we do want to consolidate them eventually). Not
sure if that can be fixed easily though?

-Toke

Re: [PATCH v11 2/4] mac80211: Import airtime calculation code from mt76

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Mon, 2019-11-18 at 22:06 -0800, Kan Yan wrote:
>> 
>> +#define HE_GROUP_IDX(_streams, _gi, _bw)				\
>> +	(IEEE80211_HE_GROUP_0 +					\
>> +	 IEEE80211_HE_MAX_STREAMS * 2 * (_bw) +			\
>> +	 IEEE80211_HE_MAX_STREAMS * (_gi) +				\
>> +	 (_streams) - 1)
>
> I'll also fix that to be " * 3" instead of 2, since there are 3 possible
> _gi values.

Ah yes, good catch, thanks!

-Toke

Re: [PATCH v11 2/4] mac80211: Import airtime calculation code from mt76

[Johannes Berg]

> Well, one calculates bitrates while the other calculates airtime? ;)

:)

> But yeah, I get what you mean. I think Felix went through quite some
> pains to structure this code to avoid divisions in the fast path. I
> guess that is the main blocker for cfg80211_calculate_bitrate() to be
> used instead (assuming we do want to consolidate them eventually). Not
> sure if that can be fixed easily though?

We could also do it the other way around?

Or maybe we should keep both and use them as a sanity check for each
other :P

johannes

Re: [PATCH v11 2/4] mac80211: Import airtime calculation code from mt76

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

>> Well, one calculates bitrates while the other calculates airtime? ;)
>
> :)
>
>> But yeah, I get what you mean. I think Felix went through quite some
>> pains to structure this code to avoid divisions in the fast path. I
>> guess that is the main blocker for cfg80211_calculate_bitrate() to be
>> used instead (assuming we do want to consolidate them eventually). Not
>> sure if that can be fixed easily though?
>
> We could also do it the other way around?

as in bitrate = 1/airtime? Dunno, maybe? How important is precision to
the bitrate calculations?

> Or maybe we should keep both and use them as a sanity check for each
> other :P

Hmm, yeah, it should be possible to write a selftest to iterate through
all rates and compare the output of each calculation, shouldn't it?
Where would be a good place to put that?

-Toke

Re: [PATCH v11 2/4] mac80211: Import airtime calculation code from mt76

[Johannes Berg]

On Fri, 2019-11-22 at 14:11 +0100, Toke Høiland-Jørgensen wrote:
> 
> as in bitrate = 1/airtime? Dunno, maybe? How important is precision to
> the bitrate calculations?

Not *that* important, it's just sort of advisory for userspace.

Though I think there's at least one place in mac80211 related to
radiotap that actually does use it for airtime calculation, which should
probably be changed to the new code now ...

> > Or maybe we should keep both and use them as a sanity check for each
> > other :P
> 
> Hmm, yeah, it should be possible to write a selftest to iterate through
> all rates and compare the output of each calculation, shouldn't it?
> Where would be a good place to put that?

No idea ... make a new kselftest thing for wireless?

johannes

Re: [PATCH v11 2/4] mac80211: Import airtime calculation code from mt76

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2019-11-22 at 14:11 +0100, Toke Høiland-Jørgensen wrote:
>> 
>> as in bitrate = 1/airtime? Dunno, maybe? How important is precision to
>> the bitrate calculations?
>
> Not *that* important, it's just sort of advisory for userspace.
>
> Though I think there's at least one place in mac80211 related to
> radiotap that actually does use it for airtime calculation, which should
> probably be changed to the new code now ...

Yeah, I guess so. The ath9k driver is also doing software calculations
for RX, so that could also be converted; and mt76 should be, of course...

>> > Or maybe we should keep both and use them as a sanity check for each
>> > other :P
>> 
>> Hmm, yeah, it should be possible to write a selftest to iterate through
>> all rates and compare the output of each calculation, shouldn't it?
>> Where would be a good place to put that?
>
> No idea ... make a new kselftest thing for wireless?

Right; was just wondering if there was something like that already which
I didn't know about :)

-Toke

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Felix Fietkau]

Hi,

I've take a closer look at the AQL implementation, and I found some
corner cases that need to be addressed soon:

- AQL estimated airtime does not take into account A-MPDU, so it is
significantly overestimating airtime use for aggregated traffic,
especially on high rates.
My proposed solution would be to check for a running aggregation session
and set estimated tx time to something like:
expected_airtime(16 * skb->len) / 16.

- We need an API that allows the driver to change the pending airtime
values, e.g. subtract estimated tx time for a packet.
mt76 an ath9k can queue packets inside the driver that are not currently
in the hardware queues. Typically if the txqs have more data than what
gets put into the hardware queue, both drivers will pull an extra frame
and queue it in its private txq struct. This frame will get used on the
next txq scheduling round for that particular station.
If you have lots of stations doing traffic (or having driver buffered
frames in powersave mode), this could use up a sizable chunk of the AQL
budget.
While removing the airtime of those packages would lead to AQL
temporarily underestimating airtime, I think it would be better than
overestimating it.

I will work on some patches. What do you think about these issues and my
proposed fixes?

- Felix

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

> Hi,
>
> I've take a closer look at the AQL implementation, and I found some
> corner cases that need to be addressed soon:
>
> - AQL estimated airtime does not take into account A-MPDU, so it is
> significantly overestimating airtime use for aggregated traffic,
> especially on high rates.
> My proposed solution would be to check for a running aggregation session
> and set estimated tx time to something like:
> expected_airtime(16 * skb->len) / 16.

This seems reasonable. Not sure if it'll do anything for ath10k (does
mac80211 know the aggregation state for that?), but maybe this is not
such a big issue on that hardware?

> - We need an API that allows the driver to change the pending airtime
> values, e.g. subtract estimated tx time for a packet.
> mt76 an ath9k can queue packets inside the driver that are not currently
> in the hardware queues. Typically if the txqs have more data than what
> gets put into the hardware queue, both drivers will pull an extra frame
> and queue it in its private txq struct. This frame will get used on the
> next txq scheduling round for that particular station.
> If you have lots of stations doing traffic (or having driver buffered
> frames in powersave mode), this could use up a sizable chunk of the AQL
> budget.

I'm a bit more skeptical about this. If the driver buffers a bunch of
packets that are not accounted that will hurt that station due to extra
latency when it wakes up. For ath9k, this is the retry_q you're talking
about, right? The number of packets queued on that is fairly limited,
isn't it? What kind of powersave buffering is the driver doing, and why
can't it leave the packets on the TXQ? That would allow them to be
scheduled along with any new ones that might have arrived in the
meantime, which would be a benefit for latency.

I can see how it can be a problem that many stations in powersave can
block transmissions for *other* stations, though. Maybe an alternative
to the driver subtracting airtime could be to have mac80211 do something
like this when a station is put into powersave mode:

local->aql_total_pending_airtime -= sum(sta->airtime[ac].aql_tx_pending)

(where sum is the summation over all ACs)

and the reverse when the station wakes back up? That should keep the
whole device from throttling but still prevent a big queue building up
for that sta when it wakes back up. Would that work, do you think?

-Toke

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Kan Yan]

> - AQL estimated airtime does not take into account A-MPDU, so it is
> significantly overestimating airtime use for aggregated traffic,
> especially on high rates.
> My proposed solution would be to check for a running aggregation session
> and set estimated tx time to something like:
> expected_airtime(16 * skb->len) / 16.

Yes, that's a known limitation that needs to be improved. I actually
post a comment for this patch:
"[PATCH v10 2/4] mac80211: Import airtime calculation code from mt76"
>
> When a txq is subjected to the queue limit,
> there is usually a significant amount of frames being queued and those
> frames are likely being sent out in large aggregations. So, in most
> cases when AQL is active, the medium access overhead can be amortized
> over many frames and the per frame overhead could be considerably
> lower than 36, especially at higher data rate. As a result, the
> pending airtime calculated this way could be higher than the actual
> airtime. In my test, I have to compensate that by increasing
> "aql_txq_limit" via debugfs to get the same peak throughput as without
> AQL.


> My proposed solution would be to check for a running aggregation session
> and set estimated tx time to something like:
> expected_airtime(16 * skb->len) / 16.

I think that's a reasonable approximation, but doubts aggregation
information is available in all architectures. In some architecture,
firmware may only report aggregation information after the frame has
been transmitted.

In my earlier version of AQL for the out-of-tree ChromeOS kernel,
A-MPDU is dealt this way:  The the medium access overhead is only
counted once for each TXQ for all frames released to the hardware over
a 4ms period, assuming those frames are likely to be aggregated
together.

Instead of calculating the estimated airtime using the last known phy
rate, then try to add some estimated overhead for medium access time,
another potentially better approach is to use average data rate, which
is byte_transmited/firmware_reported_actual_airtime. The average rate
not only includes medium access overhead, but also takes retries into
account.

> - We need an API that allows the driver to change the pending airtime
> values, e.g. subtract estimated tx time for a packet.
> mt76 an ath9k can queue packets inside the driver that are not currently
> in the hardware queues. Typically if the txqs have more data than what
> gets put into the hardware queue, both drivers will pull an extra frame
> and queue it in its private txq struct. This frame will get used on the
> next txq scheduling round for that particular station.
> If you have lots of stations doing traffic (or having driver buffered
> frames in powersave mode), this could use up a sizable chunk of the AQL
> budget.

Not sure I fully understand your concerns here. The AQL budget is per
STA, per TID, so frames queued in the driver's special queue for one
station won't impact another station's budget. Those frames are
counted towards the per interface pending airtime, which could trigger
AQL to switch to use the lower queue limit. In this case, that could
be the desirable behavior when there is heavy traffic.

Regards,
Kan



On Wed, Feb 26, 2020 at 1:56 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Felix Fietkau <nbd@nbd.name> writes:
>
> > Hi,
> >
> > I've take a closer look at the AQL implementation, and I found some
> > corner cases that need to be addressed soon:
> >
> > - AQL estimated airtime does not take into account A-MPDU, so it is
> > significantly overestimating airtime use for aggregated traffic,
> > especially on high rates.
> > My proposed solution would be to check for a running aggregation session
> > and set estimated tx time to something like:
> > expected_airtime(16 * skb->len) / 16.
>
> This seems reasonable. Not sure if it'll do anything for ath10k (does
> mac80211 know the aggregation state for that?), but maybe this is not
> such a big issue on that hardware?
>
> > - We need an API that allows the driver to change the pending airtime
> > values, e.g. subtract estimated tx time for a packet.
> > mt76 an ath9k can queue packets inside the driver that are not currently
> > in the hardware queues. Typically if the txqs have more data than what
> > gets put into the hardware queue, both drivers will pull an extra frame
> > and queue it in its private txq struct. This frame will get used on the
> > next txq scheduling round for that particular station.
> > If you have lots of stations doing traffic (or having driver buffered
> > frames in powersave mode), this could use up a sizable chunk of the AQL
> > budget.
>
> I'm a bit more skeptical about this. If the driver buffers a bunch of
> packets that are not accounted that will hurt that station due to extra
> latency when it wakes up. For ath9k, this is the retry_q you're talking
> about, right? The number of packets queued on that is fairly limited,
> isn't it? What kind of powersave buffering is the driver doing, and why
> can't it leave the packets on the TXQ? That would allow them to be
> scheduled along with any new ones that might have arrived in the
> meantime, which would be a benefit for latency.
>
> I can see how it can be a problem that many stations in powersave can
> block transmissions for *other* stations, though. Maybe an alternative
> to the driver subtracting airtime could be to have mac80211 do something
> like this when a station is put into powersave mode:
>
> local->aql_total_pending_airtime -= sum(sta->airtime[ac].aql_tx_pending)
>
> (where sum is the summation over all ACs)
>
> and the reverse when the station wakes back up? That should keep the
> whole device from throttling but still prevent a big queue building up
> for that sta when it wakes back up. Would that work, do you think?
>
> -Toke
>

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Felix Fietkau]

On 2020-02-26 22:56, Toke Høiland-Jørgensen wrote:
> Felix Fietkau <nbd@nbd.name> writes:
>> - We need an API that allows the driver to change the pending airtime
>> values, e.g. subtract estimated tx time for a packet.
>> mt76 an ath9k can queue packets inside the driver that are not currently
>> in the hardware queues. Typically if the txqs have more data than what
>> gets put into the hardware queue, both drivers will pull an extra frame
>> and queue it in its private txq struct. This frame will get used on the
>> next txq scheduling round for that particular station.
>> If you have lots of stations doing traffic (or having driver buffered
>> frames in powersave mode), this could use up a sizable chunk of the AQL
>> budget.
> 
> I'm a bit more skeptical about this. If the driver buffers a bunch of
> packets that are not accounted that will hurt that station due to extra
> latency when it wakes up. For ath9k, this is the retry_q you're talking
> about, right? The number of packets queued on that is fairly limited,
> isn't it? What kind of powersave buffering is the driver doing, and why
> can't it leave the packets on the TXQ? That would allow them to be
> scheduled along with any new ones that might have arrived in the
> meantime, which would be a benefit for latency.
For mt76 there should be max. 1 frame in the retry queue, it's just a
frame that was pulled from the txq in a transmission attempt but that it
couldn't put in the hw queue because it didn't fit in the current
aggregate batch.
If such a frame is in the retry queue and the sta ends up switching to
powersave mode, that frame stays buffered in the driver queue until the
sta wakes up.

> I can see how it can be a problem that many stations in powersave can
> block transmissions for *other* stations, though. Maybe an alternative
> to the driver subtracting airtime could be to have mac80211 do something
> like this when a station is put into powersave mode:
> 
> local->aql_total_pending_airtime -= sum(sta->airtime[ac].aql_tx_pending)
> 
> (where sum is the summation over all ACs)
> 
> and the reverse when the station wakes back up? That should keep the
> whole device from throttling but still prevent a big queue building up
> for that sta when it wakes back up. Would that work, do you think?
That solves most of the issue but is still incomplete. It also gets
tricky when the driver starts pulling dequeueing packets in powersave
mode (e.g. on PS-Poll).
The remaining corner case is when there are a large number of stations
that each have a single frame in their retry queue (see above). Having
those frames counted for pending airtime could reduce the aggregation
size for each station, even though those frames are not in the hw queue.

- Felix

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Justin Capella]

> ieee80211_report_used_skb(). As an optimisation, we also subtract the
> airtime on regular TX completion, zeroing out the value stored in the
> packet afterwards, to avoid having to do an expensive lookup of the station
> from the packet data on every packet.
>
> This patch does *not* include any mechanism to wake a throttled TXQ again,
> on the assumption that this will happen anyway as a side effect of whatever
> freed the skb (most commonly a TX completion).

I recall a recent patch for ath10k sdio that disabled tx
acknowledgement for performance gains and am wondering if that will be
problematic? Presumably not since it would be caught at the dequeue,
but thought I'd ask-- wondering what the effect of failed tx's or
block acknowledgement is on this stuff I'll need to study the code
some more

https://lore.kernel.org/linux-wireless/0101016eb1903db0-ef7063b4-0f42-4a01-8886-327541e6c1a4-000000@us-west-2.amazonses.com/T/#t

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

> On 2020-02-26 22:56, Toke Høiland-Jørgensen wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>>> - We need an API that allows the driver to change the pending airtime
>>> values, e.g. subtract estimated tx time for a packet.
>>> mt76 an ath9k can queue packets inside the driver that are not currently
>>> in the hardware queues. Typically if the txqs have more data than what
>>> gets put into the hardware queue, both drivers will pull an extra frame
>>> and queue it in its private txq struct. This frame will get used on the
>>> next txq scheduling round for that particular station.
>>> If you have lots of stations doing traffic (or having driver buffered
>>> frames in powersave mode), this could use up a sizable chunk of the AQL
>>> budget.
>> 
>> I'm a bit more skeptical about this. If the driver buffers a bunch of
>> packets that are not accounted that will hurt that station due to extra
>> latency when it wakes up. For ath9k, this is the retry_q you're talking
>> about, right? The number of packets queued on that is fairly limited,
>> isn't it? What kind of powersave buffering is the driver doing, and why
>> can't it leave the packets on the TXQ? That would allow them to be
>> scheduled along with any new ones that might have arrived in the
>> meantime, which would be a benefit for latency.
> For mt76 there should be max. 1 frame in the retry queue, it's just a
> frame that was pulled from the txq in a transmission attempt but that it
> couldn't put in the hw queue because it didn't fit in the current
> aggregate batch.

Wait, if it's only a single frame that is queued in the driver, how is
this causing problems? We deliberately set the limit so there was a bit
of slack above the size of an aggregate for things like this. Could you
please describe in a bit more detail what symptoms you are seeing of
this problem? :)

-Toke

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Toke Høiland-Jørgensen]

Justin Capella <justincapella@gmail.com> writes:

>> ieee80211_report_used_skb(). As an optimisation, we also subtract the
>> airtime on regular TX completion, zeroing out the value stored in the
>> packet afterwards, to avoid having to do an expensive lookup of the station
>> from the packet data on every packet.
>>
>> This patch does *not* include any mechanism to wake a throttled TXQ again,
>> on the assumption that this will happen anyway as a side effect of whatever
>> freed the skb (most commonly a TX completion).
>
> I recall a recent patch for ath10k sdio that disabled tx
> acknowledgement for performance gains and am wondering if that will be
> problematic? Presumably not since it would be caught at the dequeue,
> but thought I'd ask-- wondering what the effect of failed tx's or
> block acknowledgement is on this stuff I'll need to study the code
> some more
>
> https://lore.kernel.org/linux-wireless/0101016eb1903db0-ef7063b4-0f42-4a01-8886-327541e6c1a4-000000@us-west-2.amazonses.com/T/#t

It looks like that patch will just end up disabling AQL (because packets
will be immediately completed as far as mac80211 is concerned) and
replace it with whatever that credit-based scheme does? No idea how that
will impact latency; you should go ask the developers of that series! As
usual, the patch description only mentions throughput numbers :/

-Toke

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Felix Fietkau]

On 2020-02-27 09:24, Kan Yan wrote:
>> - AQL estimated airtime does not take into account A-MPDU, so it is
>> significantly overestimating airtime use for aggregated traffic,
>> especially on high rates.
>> My proposed solution would be to check for a running aggregation session
>> and set estimated tx time to something like:
>> expected_airtime(16 * skb->len) / 16.
> 
> Yes, that's a known limitation that needs to be improved. I actually
> post a comment for this patch:
> "[PATCH v10 2/4] mac80211: Import airtime calculation code from mt76"
>>
>> When a txq is subjected to the queue limit,
>> there is usually a significant amount of frames being queued and those
>> frames are likely being sent out in large aggregations. So, in most
>> cases when AQL is active, the medium access overhead can be amortized
>> over many frames and the per frame overhead could be considerably
>> lower than 36, especially at higher data rate. As a result, the
>> pending airtime calculated this way could be higher than the actual
>> airtime. In my test, I have to compensate that by increasing
>> "aql_txq_limit" via debugfs to get the same peak throughput as without
>> AQL.
> 
> 
>> My proposed solution would be to check for a running aggregation session
>> and set estimated tx time to something like:
>> expected_airtime(16 * skb->len) / 16.
> 
> I think that's a reasonable approximation, but doubts aggregation
> information is available in all architectures. In some architecture,
> firmware may only report aggregation information after the frame has
> been transmitted.
I'm not proposing using frame transmission aggregation. It would be a
simple approximation where it would use a fixed assumed average
aggregation size if an aggregation session is established.

> In my earlier version of AQL for the out-of-tree ChromeOS kernel,
> A-MPDU is dealt this way:  The the medium access overhead is only
> counted once for each TXQ for all frames released to the hardware over
> a 4ms period, assuming those frames are likely to be aggregated
> together.
I think that would be more accurate, but probably also more expensive to
calculate.

> Instead of calculating the estimated airtime using the last known phy
> rate, then try to add some estimated overhead for medium access time,
> another potentially better approach is to use average data rate, which
> is byte_transmited/firmware_reported_actual_airtime. The average rate
> not only includes medium access overhead, but also takes retries into
> account.
Also an interesting idea, though probably not available on all hardware.

>> - We need an API that allows the driver to change the pending airtime
>> values, e.g. subtract estimated tx time for a packet.
>> mt76 an ath9k can queue packets inside the driver that are not currently
>> in the hardware queues. Typically if the txqs have more data than what
>> gets put into the hardware queue, both drivers will pull an extra frame
>> and queue it in its private txq struct. This frame will get used on the
>> next txq scheduling round for that particular station.
>> If you have lots of stations doing traffic (or having driver buffered
>> frames in powersave mode), this could use up a sizable chunk of the AQL
>> budget.
> 
> Not sure I fully understand your concerns here. The AQL budget is per
> STA, per TID, so frames queued in the driver's special queue for one
> station won't impact another station's budget. Those frames are
> counted towards the per interface pending airtime, which could trigger
> AQL to switch to use the lower queue limit. In this case, that could
> be the desirable behavior when there is heavy traffic.
Yes, the per interface limit is what I'm concerned about. I'm not sure
if it will be an issue in practice, it's just something that I noticed
while reviewing the code.

- Felix

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

>> Not sure I fully understand your concerns here. The AQL budget is per
>> STA, per TID, so frames queued in the driver's special queue for one
>> station won't impact another station's budget. Those frames are
>> counted towards the per interface pending airtime, which could trigger
>> AQL to switch to use the lower queue limit. In this case, that could
>> be the desirable behavior when there is heavy traffic.
> Yes, the per interface limit is what I'm concerned about. I'm not sure
> if it will be an issue in practice, it's just something that I noticed
> while reviewing the code.

Ah, right. Note that it's not a hard limit, though; it's just a
threshold for when the per-station limit switches to a smaller value.
The idea is that when there are many stations with outstanding data,
each station's latency will be higher since they have to also wait for
their turn to transmit. And so we compensate for this by lowering each
station's queue limit, since the hardware is unlikely to become idle
when there are many stations to send to.

The lower limit is still 5ms of data, though, so there should still be
enough for a full aggregate queued for each station. Arguably the limit
could actually be made *smaller*: On a very busy network with lots of
stations transmitting at once, IMO we *don't* want to send full-sized
aggregates as that will add up to way too much latency. Better to
sacrifice a bit of network efficiency to raise responsiveness :)

-Toke

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Felix Fietkau]

On 2020-02-27 11:07, Toke Høiland-Jørgensen wrote:
> Felix Fietkau <nbd@nbd.name> writes:
> 
>> On 2020-02-26 22:56, Toke Høiland-Jørgensen wrote:
>>> Felix Fietkau <nbd@nbd.name> writes:
>>>> - We need an API that allows the driver to change the pending airtime
>>>> values, e.g. subtract estimated tx time for a packet.
>>>> mt76 an ath9k can queue packets inside the driver that are not currently
>>>> in the hardware queues. Typically if the txqs have more data than what
>>>> gets put into the hardware queue, both drivers will pull an extra frame
>>>> and queue it in its private txq struct. This frame will get used on the
>>>> next txq scheduling round for that particular station.
>>>> If you have lots of stations doing traffic (or having driver buffered
>>>> frames in powersave mode), this could use up a sizable chunk of the AQL
>>>> budget.
>>> 
>>> I'm a bit more skeptical about this. If the driver buffers a bunch of
>>> packets that are not accounted that will hurt that station due to extra
>>> latency when it wakes up. For ath9k, this is the retry_q you're talking
>>> about, right? The number of packets queued on that is fairly limited,
>>> isn't it? What kind of powersave buffering is the driver doing, and why
>>> can't it leave the packets on the TXQ? That would allow them to be
>>> scheduled along with any new ones that might have arrived in the
>>> meantime, which would be a benefit for latency.
>> For mt76 there should be max. 1 frame in the retry queue, it's just a
>> frame that was pulled from the txq in a transmission attempt but that it
>> couldn't put in the hw queue because it didn't fit in the current
>> aggregate batch.
> 
> Wait, if it's only a single frame that is queued in the driver, how is
> this causing problems? We deliberately set the limit so there was a bit
> of slack above the size of an aggregate for things like this. Could you
> please describe in a bit more detail what symptoms you are seeing of
> this problem? :)
It would be a single frame per sta/txq. I don't know if it will cause
problems in practice, it's just a potential corner case that I found
during review. I'd imagine this would probably show up in some
benchmarks at least.
I'm not seeing any symptoms myself, but I also haven't run any intricate
tests yet.

- Felix

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

> On 2020-02-27 11:07, Toke Høiland-Jørgensen wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>> 
>>> On 2020-02-26 22:56, Toke Høiland-Jørgensen wrote:
>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>>> - We need an API that allows the driver to change the pending airtime
>>>>> values, e.g. subtract estimated tx time for a packet.
>>>>> mt76 an ath9k can queue packets inside the driver that are not currently
>>>>> in the hardware queues. Typically if the txqs have more data than what
>>>>> gets put into the hardware queue, both drivers will pull an extra frame
>>>>> and queue it in its private txq struct. This frame will get used on the
>>>>> next txq scheduling round for that particular station.
>>>>> If you have lots of stations doing traffic (or having driver buffered
>>>>> frames in powersave mode), this could use up a sizable chunk of the AQL
>>>>> budget.
>>>> 
>>>> I'm a bit more skeptical about this. If the driver buffers a bunch of
>>>> packets that are not accounted that will hurt that station due to extra
>>>> latency when it wakes up. For ath9k, this is the retry_q you're talking
>>>> about, right? The number of packets queued on that is fairly limited,
>>>> isn't it? What kind of powersave buffering is the driver doing, and why
>>>> can't it leave the packets on the TXQ? That would allow them to be
>>>> scheduled along with any new ones that might have arrived in the
>>>> meantime, which would be a benefit for latency.
>>> For mt76 there should be max. 1 frame in the retry queue, it's just a
>>> frame that was pulled from the txq in a transmission attempt but that it
>>> couldn't put in the hw queue because it didn't fit in the current
>>> aggregate batch.
>> 
>> Wait, if it's only a single frame that is queued in the driver, how is
>> this causing problems? We deliberately set the limit so there was a bit
>> of slack above the size of an aggregate for things like this. Could you
>> please describe in a bit more detail what symptoms you are seeing of
>> this problem? :)
> It would be a single frame per sta/txq. I don't know if it will cause
> problems in practice, it's just a potential corner case that I found
> during review. I'd imagine this would probably show up in some
> benchmarks at least.
> I'm not seeing any symptoms myself, but I also haven't run any intricate
> tests yet.

See my other reply; I'm not convinced this behaviour is wrong :)

-Toke

Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Felix Fietkau]

On 2020-02-27 12:07, Toke Høiland-Jørgensen wrote:
> Felix Fietkau <nbd@nbd.name> writes:
> 
>>> Not sure I fully understand your concerns here. The AQL budget is per
>>> STA, per TID, so frames queued in the driver's special queue for one
>>> station won't impact another station's budget. Those frames are
>>> counted towards the per interface pending airtime, which could trigger
>>> AQL to switch to use the lower queue limit. In this case, that could
>>> be the desirable behavior when there is heavy traffic.
>> Yes, the per interface limit is what I'm concerned about. I'm not sure
>> if it will be an issue in practice, it's just something that I noticed
>> while reviewing the code.
> 
> Ah, right. Note that it's not a hard limit, though; it's just a
> threshold for when the per-station limit switches to a smaller value.
> The idea is that when there are many stations with outstanding data,
> each station's latency will be higher since they have to also wait for
> their turn to transmit. And so we compensate for this by lowering each
> station's queue limit, since the hardware is unlikely to become idle
> when there are many stations to send to.
> 
> The lower limit is still 5ms of data, though, so there should still be
> enough for a full aggregate queued for each station. Arguably the limit
> could actually be made *smaller*: On a very busy network with lots of
> stations transmitting at once, IMO we *don't* want to send full-sized
> aggregates as that will add up to way too much latency. Better to
> sacrifice a bit of network efficiency to raise responsiveness :)
Makes sense, thanks.

- Felix