[PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Kan Yan]

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

This version has been tested on QCA9984 platform.

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

Changelog:

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 (1):
  mac80211: Import airtime calculation code from mt76

 include/net/cfg80211.h     |   7 +
 include/net/mac80211.h     |  41 +++
 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    |  38 +++
 net/mac80211/sta_info.h    |   8 +
 net/mac80211/tx.c          |  47 ++-
 11 files changed, 872 insertions(+), 15 deletions(-)
 create mode 100644 net/mac80211/airtime.c

-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[v8 PATCH 1/2] 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 02a8a4986f21..4319596ef472 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -6417,4 +6417,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.rc1.363.gb1bccd3e3d-goog

[v8 PATCH 2/2] 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          | 47 +++++++++++++++++++--
 9 files changed, 240 insertions(+), 14 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 4ab2c49423dc..15f9f04de149 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2602,6 +2602,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	4000
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H	8000
+
+/* 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 4319596ef472..269be699e89c 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5558,6 +5558,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 2d05c4cfaf6d..6cca0853f183 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 8d3a2389b055..6df995d9f4b1 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -396,6 +396,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++)
@@ -1893,6 +1896,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) {
+		atomic_sub(tx_airtime, &sta->airtime[ac].aql_tx_pending);
+		tx_pending = atomic_read(&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);
+	}
+
+	atomic_sub(tx_airtime, &local->aql_total_pending_airtime);
+	tx_pending = atomic_read(&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 369c2dddce52..c8823bd0e55f 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 238c47be5fe4..f53d56ef535a 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3672,7 +3672,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]);
 
@@ -3683,13 +3684,26 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
 	if (!txqi)
 		goto out;
 
+	if (txqi == head && !found_eligible_txq)
+		goto out;
+
+	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 (aql_check)
+			found_eligible_txq = true;
 
-		if (sta->airtime[txqi->txq.ac].deficit < 0) {
+		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;
@@ -3743,6 +3757,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.rc1.363.gb1bccd3e3d-goog

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

[Kan Yan]

I have tested it with Toke's patch "[PATCH v6 4/4] mac80211: Use
Airtime-based Queue Limits (AQL) on packet dequeue", but didn't
include it here, as it is self contained and Toke has plan to update
it.

The platform (QCA9984) used in my test doesn't support 802.11ax, so I
was not able to test the HE mode support added in v7 update of "Import
airtime calculation code from mt76" from Toke.

On Thu, Nov 14, 2019 at 5:48 PM Kan Yan <kyan@google.com> wrote:
>
> This patch series port the Airtime Queue Limits concept from the out-of-tree
> ath10k implementation[0] to mac80211. This version takes my patch to do the
> throttling in mac80211, and replaces the driver API with the mechanism from
> Toke's series, which instead calculated the expected airtime at dequeue time
> inside mac80211, storing it in the SKB cb field.
>
> This version has been tested on QCA9984 platform.
>
> [0] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1703105/7
>
> Changelog:
>
> 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 (1):
>   mac80211: Import airtime calculation code from mt76
>
>  include/net/cfg80211.h     |   7 +
>  include/net/mac80211.h     |  41 +++
>  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    |  38 +++
>  net/mac80211/sta_info.h    |   8 +
>  net/mac80211/tx.c          |  47 ++-
>  11 files changed, 872 insertions(+), 15 deletions(-)
>  create mode 100644 net/mac80211/airtime.c
>
> --
> 2.24.0.rc1.363.gb1bccd3e3d-goog
>

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Dave Taht]

On Thu, Nov 14, 2019 at 6:04 PM Kan Yan <kyan@google.com> wrote:
>
> I have tested it with Toke's patch "[PATCH v6 4/4] mac80211: Use
> Airtime-based Queue Limits (AQL) on packet dequeue", but didn't
> include it here, as it is self contained and Toke has plan to update
> it.
>
> The platform (QCA9984) used in my test

I do keep hoping for pretty pictures. Got any? :-P

>  doesn't support 802.11ax, so I
> was not able to test the HE mode support added in v7 update of "Import
> airtime calculation code from mt76" from Toke.

Is there an ax QCAXXXX platform, m.2 card, or mini-pci card worth
testing at this point?

How are they handling mu-mimo?

I have a round of tests scheduled for intel's ax200 chips, soon. Not sure
what, if any, of this new work might apply.

> On Thu, Nov 14, 2019 at 5:48 PM Kan Yan <kyan@google.com> wrote:
> >
> > This patch series port the Airtime Queue Limits concept from the out-of-tree
> > ath10k implementation[0] to mac80211. This version takes my patch to do the
> > throttling in mac80211, and replaces the driver API with the mechanism from
> > Toke's series, which instead calculated the expected airtime at dequeue time
> > inside mac80211, storing it in the SKB cb field.
> >
> > This version has been tested on QCA9984 platform.
> >
> > [0] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1703105/7
> >
> > Changelog:
> >
> > 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 (1):
> >   mac80211: Import airtime calculation code from mt76
> >
> >  include/net/cfg80211.h     |   7 +
> >  include/net/mac80211.h     |  41 +++
> >  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    |  38 +++
> >  net/mac80211/sta_info.h    |   8 +
> >  net/mac80211/tx.c          |  47 ++-
> >  11 files changed, 872 insertions(+), 15 deletions(-)
> >  create mode 100644 net/mac80211/airtime.c
> >
> > --
> > 2.24.0.rc1.363.gb1bccd3e3d-goog
> >
> _______________________________________________
> Make-wifi-fast mailing list
> Make-wifi-fast@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/make-wifi-fast



-- 

Dave Täht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-205-9740

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

[Toke Høiland-Jørgensen]

Kan Yan <kyan@google.com> writes:

> In order for the Fq_CoDel algorithm integrated in mac80211 layer to operate
> effectively to control excessive queueing latency, the CoDel algorithm
> requires an accurate measure of how long packets stays in the queue, AKA
> sojourn time. The sojourn time measured at the mac80211 layer doesn't
> include queueing latency in the lower layer (firmware/hardware) and CoDel
> expects lower layer to have a short queue. However, most 802.11ac chipsets
> offload tasks such TX aggregation to firmware or hardware, thus have a deep
> lower layer queue.
>
> Without a mechanism to control the lower layer queue size, packets only
> stay in mac80211 layer transiently before being sent to firmware queue.
> As a result, the sojourn time measured by CoDel in the mac80211 layer is
> almost always lower than the CoDel latency target, hence CoDel does little
> to control the latency, even when the lower layer queue causes excessive
> latency.
>
> The Byte Queue Limits (BQL) mechanism is commonly used to address the
> similar issue with wired network interface. However, this method cannot be
> applied directly to the wireless network interface. "Bytes" is not a
> suitable measure of queue depth in the wireless network, as the data rate
> can vary dramatically from station to station in the same network, from a
> few Mbps to over Gbps.
>
> This patch implements an Airtime-based Queue Limit (AQL) to make CoDel work
> effectively with wireless drivers that utilized firmware/hardware
> offloading. AQL allows each txq to release just enough packets to the lower
> layer to form 1-2 large aggregations to keep hardware fully utilized and
> retains the rest of the frames in mac80211 layer to be controlled by the
> CoDel algorithm.
>
> Signed-off-by: Kan Yan <kyan@google.com>
> [ Toke: Keep API to set pending airtime internal, fix nits in commit msg ]
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
[...]

> +	if (sta) {
> +		atomic_sub(tx_airtime, &sta->airtime[ac].aql_tx_pending);
> +		tx_pending = atomic_read(&sta->airtime[ac].aql_tx_pending);

This is still racy, since you're splitting it over two calls; you'll
need to use atomic_sub_return() instead.

I figure we've converged now to the point where it actually makes sense
to collect everything back into a single series; so I can just fix this
and re-send the full series.

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

This could still fail if there's a concurrent modification (and you're
not checking the return of the cmpxchg). But at least it won't clobber
any updated value, so I guess that is acceptable since we're in "should
never happen" territory here :)

-Toke

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

[Kan Yan]

> > +     if (sta) {
> > +             atomic_sub(tx_airtime, &sta->airtime[ac].aql_tx_pending);
> > +             tx_pending = atomic_read(&sta->airtime[ac].aql_tx_pending);
> This is still racy, since you're splitting it over two calls; you'll
> need to use atomic_sub_return() instead.
> I figure we've converged now to the point where it actually makes sense
> to collect everything back into a single series; so I can just fix this
> and re-send the full series.

Thanks for help fixing this. Yes, atomic_sub_return() is better.

>
>
> > +             if (WARN_ONCE(tx_pending < 0,
> > +                           "STA %pM AC %d txq pending airtime underflow: %u, %u",
> > +                           sta->addr, ac, tx_pending, tx_airtime))
> > +                     atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending,
> > +                                    tx_pending, 0);
> This could still fail if there's a concurrent modification (and you're
> not checking the return of the cmpxchg). But at least it won't clobber
> any updated value, so I guess that is acceptable since we're in "should
> never happen" territory here :)

I did this on purpose since I really don't like adding a loop to retry
here. If aql_tx_pending indeed goes negative (should never happens and
we got WARN_ONCE() to catch it) and the subsequent atomic_cmpxchg()
failed (rare racing occasions), it is still ok. In this case,
aql_tx_pending carries a negative offset and will be reset in one of
the calls to ieee80211_sta_update_pending_airtime() later.
aql_tx_pending being negative doesn't have much side-effects, such as
causing txq stuck.

On Fri, Nov 15, 2019 at 4:56 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Kan Yan <kyan@google.com> writes:
>
> > In order for the Fq_CoDel algorithm integrated in mac80211 layer to operate
> > effectively to control excessive queueing latency, the CoDel algorithm
> > requires an accurate measure of how long packets stays in the queue, AKA
> > sojourn time. The sojourn time measured at the mac80211 layer doesn't
> > include queueing latency in the lower layer (firmware/hardware) and CoDel
> > expects lower layer to have a short queue. However, most 802.11ac chipsets
> > offload tasks such TX aggregation to firmware or hardware, thus have a deep
> > lower layer queue.
> >
> > Without a mechanism to control the lower layer queue size, packets only
> > stay in mac80211 layer transiently before being sent to firmware queue.
> > As a result, the sojourn time measured by CoDel in the mac80211 layer is
> > almost always lower than the CoDel latency target, hence CoDel does little
> > to control the latency, even when the lower layer queue causes excessive
> > latency.
> >
> > The Byte Queue Limits (BQL) mechanism is commonly used to address the
> > similar issue with wired network interface. However, this method cannot be
> > applied directly to the wireless network interface. "Bytes" is not a
> > suitable measure of queue depth in the wireless network, as the data rate
> > can vary dramatically from station to station in the same network, from a
> > few Mbps to over Gbps.
> >
> > This patch implements an Airtime-based Queue Limit (AQL) to make CoDel work
> > effectively with wireless drivers that utilized firmware/hardware
> > offloading. AQL allows each txq to release just enough packets to the lower
> > layer to form 1-2 large aggregations to keep hardware fully utilized and
> > retains the rest of the frames in mac80211 layer to be controlled by the
> > CoDel algorithm.
> >
> > Signed-off-by: Kan Yan <kyan@google.com>
> > [ Toke: Keep API to set pending airtime internal, fix nits in commit msg ]
> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > ---
> [...]
>
> > +     if (sta) {
> > +             atomic_sub(tx_airtime, &sta->airtime[ac].aql_tx_pending);
> > +             tx_pending = atomic_read(&sta->airtime[ac].aql_tx_pending);
>
> This is still racy, since you're splitting it over two calls; you'll
> need to use atomic_sub_return() instead.
>
> I figure we've converged now to the point where it actually makes sense
> to collect everything back into a single series; so I can just fix this
> and re-send the full series.
>
> > +             if (WARN_ONCE(tx_pending < 0,
> > +                           "STA %pM AC %d txq pending airtime underflow: %u, %u",
> > +                           sta->addr, ac, tx_pending, tx_airtime))
> > +                     atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending,
> > +                                    tx_pending, 0);
>
> This could still fail if there's a concurrent modification (and you're
> not checking the return of the cmpxchg). But at least it won't clobber
> any updated value, so I guess that is acceptable since we're in "should
> never happen" territory here :)
>
> -Toke
>

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Dave Taht]

On Fri, Nov 15, 2019 at 4:10 PM Kan Yan <kyan@google.com> wrote:
>
> > I do keep hoping for pretty pictures. Got any? :-P
>
> Certainly! I do have some :). Here is the link:
> https://drive.google.com/corp/drive/folders/14OIuQEHOUiIoNrVnKprj6rBYFNZ0Coif

Those were lovely, thanks!!!! Big win. Since you are on patch v10
now.... Any chance you could turn ecn on and off and give it a go
again in your next test run?

Also:

--step-size=.04 --socket-stats # the first is helpful to gain more
detail, the second as to the behavior of the tcp stack. You might need
to run as root (and It's only useful on the tcp_nup test) for the
latter (and have the right ss utility)

Secondly - and AFTER this patchset stablizes, I'd like us to look into
returning the codel default to 10ms or less
from it's currently 20ms or worse setting. Tis another easy test

And y'all know how much I love the rrul_be and rrul tests.....


> >
> > Is there an ax QCAXXXX platform, m.2 card, or mini-pci card worth
> > testing at this point?
>
> It will be great if someone with 11.ax platform can help give it a try.
>
> > How are they handling mu-mimo?
>
> I think it should still work. The queue length in airtime for each individual queue is unchanged, even the multiple queues are allowed to transmit concurrently with mu-mimo.
>
>> I have a round of tests scheduled for intel's ax200 chips, soon. Not sure
>> what, if any, of this new work might apply.
>
> It will be very interesting to know how it performance on 802.11ax platforms. Supposedly 802.11ax already fixed the latency problem so the benefit of this patch should be less significant.
>
>
> On Thu, Nov 14, 2019 at 6:07 PM Dave Taht <dave.taht@gmail.com> wrote:
>>
>> On Thu, Nov 14, 2019 at 6:04 PM Kan Yan <kyan@google.com> wrote:
>> >
>> > I have tested it with Toke's patch "[PATCH v6 4/4] mac80211: Use
>> > Airtime-based Queue Limits (AQL) on packet dequeue", but didn't
>> > include it here, as it is self contained and Toke has plan to update
>> > it.
>> >
>> > The platform (QCA9984) used in my test
>>
>> I do keep hoping for pretty pictures. Got any? :-P
>>
>> >  doesn't support 802.11ax, so I
>> > was not able to test the HE mode support added in v7 update of "Import
>> > airtime calculation code from mt76" from Toke.
>>
>> Is there an ax QCAXXXX platform, m.2 card, or mini-pci card worth
>> testing at this point?
>>
>> How are they handling mu-mimo?
>>
>> I have a round of tests scheduled for intel's ax200 chips, soon. Not sure
>> what, if any, of this new work might apply.
>>
>> > On Thu, Nov 14, 2019 at 5:48 PM Kan Yan <kyan@google.com> wrote:
>> > >
>> > > This patch series port the Airtime Queue Limits concept from the out-of-tree
>> > > ath10k implementation[0] to mac80211. This version takes my patch to do the
>> > > throttling in mac80211, and replaces the driver API with the mechanism from
>> > > Toke's series, which instead calculated the expected airtime at dequeue time
>> > > inside mac80211, storing it in the SKB cb field.
>> > >
>> > > This version has been tested on QCA9984 platform.
>> > >
>> > > [0] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1703105/7
>> > >
>> > > Changelog:
>> > >
>> > > 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 (1):
>> > >   mac80211: Import airtime calculation code from mt76
>> > >
>> > >  include/net/cfg80211.h     |   7 +
>> > >  include/net/mac80211.h     |  41 +++
>> > >  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    |  38 +++
>> > >  net/mac80211/sta_info.h    |   8 +
>> > >  net/mac80211/tx.c          |  47 ++-
>> > >  11 files changed, 872 insertions(+), 15 deletions(-)
>> > >  create mode 100644 net/mac80211/airtime.c
>> > >
>> > > --
>> > > 2.24.0.rc1.363.gb1bccd3e3d-goog
>> > >
>> > _______________________________________________
>> > Make-wifi-fast mailing list
>> > Make-wifi-fast@lists.bufferbloat.net
>> > https://lists.bufferbloat.net/listinfo/make-wifi-fast
>>
>>
>>
>> --
>>
>> Dave Täht
>> CTO, TekLibre, LLC
>> http://www.teklibre.com
>> Tel: 1-831-205-9740



-- 

Dave Täht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-205-9740

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Kan Yan]

> Those were lovely, thanks!!!! Big win. Since you are on patch v10
> now.... Any chance you could turn ecn on and off and give it a go
> again in your next test run?
>
>
> Also:
>
> --step-size=.04 --socket-stats # the first is helpful to gain more
> detail, the second as to the behavior of the tcp stack.

Thanks for the feedback! I will do more tests in a few days.


> Secondly - and AFTER this patchset stablizes, I'd like us to look into
> returning the codel default to 10ms or less
> from it's currently 20ms or worse setting. Tis another easy test

Smaller CoDel "target" doesn't work well with wireless because the
dequeue behavior in wireless driver is very bursty. It is quite often
dequeues dozens of packets in one burst after one large aggregation is
completed, so smaller CoDel "target" can cause unnecessary packet
drop.


On Mon, Nov 18, 2019 at 1:08 PM Dave Taht <dave.taht@gmail.com> wrote:
>
> On Fri, Nov 15, 2019 at 4:10 PM Kan Yan <kyan@google.com> wrote:
> >
> > > I do keep hoping for pretty pictures. Got any? :-P
> >
> > Certainly! I do have some :). Here is the link:
> > https://drive.google.com/corp/drive/folders/14OIuQEHOUiIoNrVnKprj6rBYFNZ0Coif
>
> Those were lovely, thanks!!!! Big win. Since you are on patch v10
> now.... Any chance you could turn ecn on and off and give it a go
> again in your next test run?
>
> Also:
>
> --step-size=.04 --socket-stats # the first is helpful to gain more
> detail, the second as to the behavior of the tcp stack. You might need
> to run as root (and It's only useful on the tcp_nup test) for the
> latter (and have the right ss utility)
>
> Secondly - and AFTER this patchset stablizes, I'd like us to look into
> returning the codel default to 10ms or less
> from it's currently 20ms or worse setting. Tis another easy test
>
> And y'all know how much I love the rrul_be and rrul tests.....
>
>
> > >
> > > Is there an ax QCAXXXX platform, m.2 card, or mini-pci card worth
> > > testing at this point?
> >
> > It will be great if someone with 11.ax platform can help give it a try.
> >
> > > How are they handling mu-mimo?
> >
> > I think it should still work. The queue length in airtime for each individual queue is unchanged, even the multiple queues are allowed to transmit concurrently with mu-mimo.
> >
> >> I have a round of tests scheduled for intel's ax200 chips, soon. Not sure
> >> what, if any, of this new work might apply.
> >
> > It will be very interesting to know how it performance on 802.11ax platforms. Supposedly 802.11ax already fixed the latency problem so the benefit of this patch should be less significant.
> >
> >
> > On Thu, Nov 14, 2019 at 6:07 PM Dave Taht <dave.taht@gmail.com> wrote:
> >>
> >> On Thu, Nov 14, 2019 at 6:04 PM Kan Yan <kyan@google.com> wrote:
> >> >
> >> > I have tested it with Toke's patch "[PATCH v6 4/4] mac80211: Use
> >> > Airtime-based Queue Limits (AQL) on packet dequeue", but didn't
> >> > include it here, as it is self contained and Toke has plan to update
> >> > it.
> >> >
> >> > The platform (QCA9984) used in my test
> >>
> >> I do keep hoping for pretty pictures. Got any? :-P
> >>
> >> >  doesn't support 802.11ax, so I
> >> > was not able to test the HE mode support added in v7 update of "Import
> >> > airtime calculation code from mt76" from Toke.
> >>
> >> Is there an ax QCAXXXX platform, m.2 card, or mini-pci card worth
> >> testing at this point?
> >>
> >> How are they handling mu-mimo?
> >>
> >> I have a round of tests scheduled for intel's ax200 chips, soon. Not sure
> >> what, if any, of this new work might apply.
> >>
> >> > On Thu, Nov 14, 2019 at 5:48 PM Kan Yan <kyan@google.com> wrote:
> >> > >
> >> > > This patch series port the Airtime Queue Limits concept from the out-of-tree
> >> > > ath10k implementation[0] to mac80211. This version takes my patch to do the
> >> > > throttling in mac80211, and replaces the driver API with the mechanism from
> >> > > Toke's series, which instead calculated the expected airtime at dequeue time
> >> > > inside mac80211, storing it in the SKB cb field.
> >> > >
> >> > > This version has been tested on QCA9984 platform.
> >> > >
> >> > > [0] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1703105/7
> >> > >
> >> > > Changelog:
> >> > >
> >> > > 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 (1):
> >> > >   mac80211: Import airtime calculation code from mt76
> >> > >
> >> > >  include/net/cfg80211.h     |   7 +
> >> > >  include/net/mac80211.h     |  41 +++
> >> > >  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    |  38 +++
> >> > >  net/mac80211/sta_info.h    |   8 +
> >> > >  net/mac80211/tx.c          |  47 ++-
> >> > >  11 files changed, 872 insertions(+), 15 deletions(-)
> >> > >  create mode 100644 net/mac80211/airtime.c
> >> > >
> >> > > --
> >> > > 2.24.0.rc1.363.gb1bccd3e3d-goog
> >> > >
> >> > _______________________________________________
> >> > Make-wifi-fast mailing list
> >> > Make-wifi-fast@lists.bufferbloat.net
> >> > https://lists.bufferbloat.net/listinfo/make-wifi-fast
> >>
> >>
> >>
> >> --
> >>
> >> Dave Täht
> >> CTO, TekLibre, LLC
> >> http://www.teklibre.com
> >> Tel: 1-831-205-9740
>
>
>
> --
>
> Dave Täht
> CTO, TekLibre, LLC
> http://www.teklibre.com
> Tel: 1-831-205-9740

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Toke Høiland-Jørgensen]

Kan Yan <kyan@google.com> writes:

>> Those were lovely, thanks!!!! Big win. Since you are on patch v10
>> now.... Any chance you could turn ecn on and off and give it a go
>> again in your next test run?
>>
>>
>> Also:
>>
>> --step-size=.04 --socket-stats # the first is helpful to gain more
>> detail, the second as to the behavior of the tcp stack.
>
> Thanks for the feedback! I will do more tests in a few days.
>
>
>> Secondly - and AFTER this patchset stablizes, I'd like us to look into
>> returning the codel default to 10ms or less
>> from it's currently 20ms or worse setting. Tis another easy test
>
> Smaller CoDel "target" doesn't work well with wireless because the
> dequeue behavior in wireless driver is very bursty. It is quite often
> dequeues dozens of packets in one burst after one large aggregation is
> completed, so smaller CoDel "target" can cause unnecessary packet
> drop.

It would be interesting to get some samples of the actual sojourn time
as seen by CoDel in mac80211. Might be doable with bpftrace...

-Toke

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Kan Yan]

> It would be interesting to get some samples of the actual sojourn time
> as seen by CoDel in mac80211. Might be doable with bpftrace...

I will try to add some trace event to get the sojourn time for the
next round of tests.


On Wed, Nov 20, 2019 at 2:14 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Kan Yan <kyan@google.com> writes:
>
> >> Those were lovely, thanks!!!! Big win. Since you are on patch v10
> >> now.... Any chance you could turn ecn on and off and give it a go
> >> again in your next test run?
> >>
> >>
> >> Also:
> >>
> >> --step-size=.04 --socket-stats # the first is helpful to gain more
> >> detail, the second as to the behavior of the tcp stack.
> >
> > Thanks for the feedback! I will do more tests in a few days.
> >
> >
> >> Secondly - and AFTER this patchset stablizes, I'd like us to look into
> >> returning the codel default to 10ms or less
> >> from it's currently 20ms or worse setting. Tis another easy test
> >
> > Smaller CoDel "target" doesn't work well with wireless because the
> > dequeue behavior in wireless driver is very bursty. It is quite often
> > dequeues dozens of packets in one burst after one large aggregation is
> > completed, so smaller CoDel "target" can cause unnecessary packet
> > drop.
>
> It would be interesting to get some samples of the actual sojourn time
> as seen by CoDel in mac80211. Might be doable with bpftrace...
>
> -Toke
>

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Toke Høiland-Jørgensen]

Kan Yan <kyan@google.com> writes:

>> It would be interesting to get some samples of the actual sojourn time
>> as seen by CoDel in mac80211. Might be doable with bpftrace...
>
> I will try to add some trace event to get the sojourn time for the
> next round of tests.

In theory, this ought to produce a histogram of sojourn times (in
microseconds):

bpftrace -e 'kretprobe:codel_skb_time_func { @sojourn = lhist((nsecs - (retval << 10))/1000, 0, 100000, 1000); }'


Can't get the CoDel drop mechanism to trigger on my system at all,
though (a laptop running on iwl). I guess because there's queue
backpressure to userspace first?

It would be interesting to see if it works for you, assuming you can get
bpftrace to work on your test system :)

-Toke

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Toke Høiland-Jørgensen]

Kan Yan <kyan@google.com> writes:

>> In theory, this ought to produce a histogram of sojourn times (in
>> microseconds):
>> bpftrace -e 'kretprobe:codel_skb_time_func { @sojourn = lhist((nsecs -
> (retval << 10))/1000, 0, 100000, 1000); }'
>
> Thanks for the tips!
>
>> Can't get the CoDel drop mechanism to trigger on my system at all,
>> though (a laptop running on iwl). I guess because there's queue
>> backpressure to userspace first?
>
> What's the tcp_congestion_control in your system? Maybe it is BBR that
> prevents bufferbloat.

It's not BBR, just plain old CUBIC. I've seen the issue before that it's
almost impossible to build a queue in the mac80211 layer when the TCP
session is originated on the local machine, though...

>> It would be interesting to see if it works for you, assuming you can get
>> bpftrace to work on your test system :)
>
> I can enable required kernel configuration easily, but cross-compile
> bpftrace for an ARM64 platform may take some time and effort.

Yeah, bpftrace can be a bit of a pain to get running; but it may be
worth the investment longer term as well. It really is quite useful! :)

Some links:

Install guide:
https://github.com/iovisor/bpftrace/blob/master/INSTALL.md

Tutorial by one-liners:
https://github.com/iovisor/bpftrace/blob/master/docs/tutorial_one_liners.md

Reference guide:
https://github.com/iovisor/bpftrace/blob/master/docs/reference_guide.md#5-tracepoint-static-tracing-kernel-level

-Toke

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Kan Yan]

> Yeah, bpftrace can be a bit of a pain to get running; but it may be
> worth the investment longer term as well. It really is quite useful! :)

My attempt to build bpftrace didn't work out, so I just got the
sojourn time using old fashioned trace event.
The raw trace, parsed data in csv format and plots can be found here:
https://drive.google.com/open?id=1Mg_wHu7elYAdkXz4u--42qGCVE1nrILV

All tests are done with 2 TCP download sessions that oversubscribed
the link bandwidth.
With AQL on, the mean sojourn time about ~20000us, matches the default
codel "target".
With AQL off, the mean sojourn time is less than 4us even the latency
is off the charts, just as we expected that fd_codel with mac80211
alone is not effective for drivers with deep firmware/hardware queues.

> Any chance you could turn ecn on and off and give it a go
> again in your next test run?

ECN on shows very similar results as with ECN off. "aqm" stats from
debugfs shows it is doing ECN marking instead of dropping packets as
expected. Flent test data also is in the same link.



On Fri, Nov 22, 2019 at 2:45 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Kan Yan <kyan@google.com> writes:
>
> >> In theory, this ought to produce a histogram of sojourn times (in
> >> microseconds):
> >> bpftrace -e 'kretprobe:codel_skb_time_func { @sojourn = lhist((nsecs -
> > (retval << 10))/1000, 0, 100000, 1000); }'
> >
> > Thanks for the tips!
> >
> >> Can't get the CoDel drop mechanism to trigger on my system at all,
> >> though (a laptop running on iwl). I guess because there's queue
> >> backpressure to userspace first?
> >
> > What's the tcp_congestion_control in your system? Maybe it is BBR that
> > prevents bufferbloat.
>
> It's not BBR, just plain old CUBIC. I've seen the issue before that it's
> almost impossible to build a queue in the mac80211 layer when the TCP
> session is originated on the local machine, though...
>
> >> It would be interesting to see if it works for you, assuming you can get
> >> bpftrace to work on your test system :)
> >
> > I can enable required kernel configuration easily, but cross-compile
> > bpftrace for an ARM64 platform may take some time and effort.
>
> Yeah, bpftrace can be a bit of a pain to get running; but it may be
> worth the investment longer term as well. It really is quite useful! :)
>
> Some links:
>
> Install guide:
> https://github.com/iovisor/bpftrace/blob/master/INSTALL.md
>
> Tutorial by one-liners:
> https://github.com/iovisor/bpftrace/blob/master/docs/tutorial_one_liners.md
>
> Reference guide:
> https://github.com/iovisor/bpftrace/blob/master/docs/reference_guide.md#5-tracepoint-static-tracing-kernel-level
>
> -Toke
>

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Toke Høiland-Jørgensen]

Kan Yan <kyan@google.com> writes:

>> Yeah, bpftrace can be a bit of a pain to get running; but it may be
>> worth the investment longer term as well. It really is quite useful! :)
>
> My attempt to build bpftrace didn't work out, so I just got the
> sojourn time using old fashioned trace event.
> The raw trace, parsed data in csv format and plots can be found here:
> https://drive.google.com/open?id=1Mg_wHu7elYAdkXz4u--42qGCVE1nrILV
>
> All tests are done with 2 TCP download sessions that oversubscribed
> the link bandwidth.
> With AQL on, the mean sojourn time about ~20000us, matches the default
> codel "target".

Yeah, since CoDel is trying to control the latency to 20ms, it makes
sense that the value is clustered around that. That means that the
algorithm is working as they're supposed to :)

While you're running tests, could you do one with the target changed to
10ms, just to see what it looks like? Both sojourn time values and
throughput would be interesting here, of course.

> With AQL off, the mean sojourn time is less than 4us even the latency
> is off the charts, just as we expected that fd_codel with mac80211
> alone is not effective for drivers with deep firmware/hardware queues.

Yup, also kinda expected; but another good way to visualise the impact.
Nice!

-Toke

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Dave Taht]

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

> Kan Yan <kyan@google.com> writes:
>
>>> Yeah, bpftrace can be a bit of a pain to get running; but it may be
>>> worth the investment longer term as well. It really is quite useful! :)
>>
>> My attempt to build bpftrace didn't work out, so I just got the
>> sojourn time using old fashioned trace event.
>> The raw trace, parsed data in csv format and plots can be found here:
>> https://drive.google.com/open?id=1Mg_wHu7elYAdkXz4u--42qGCVE1nrILV
>>
>> All tests are done with 2 TCP download sessions that oversubscribed
>> the link bandwidth.
>> With AQL on, the mean sojourn time about ~20000us, matches the default
>> codel "target".
>
> Yeah, since CoDel is trying to control the latency to 20ms, it makes
> sense that the value is clustered around that. That means that the
> algorithm is working as they're supposed to :)
>
> While you're running tests, could you do one with the target changed to
> 10ms, just to see what it looks like? Both sojourn time values and
> throughput would be interesting here, of course.
>
>> With AQL off, the mean sojourn time is less than 4us even the latency
>> is off the charts, just as we expected that fd_codel with mac80211
>> alone is not effective for drivers with deep firmware/hardware queues

I hope to take a close look at the iwl ax200 chips soon. Unless
someone beats me to it. Can we get these sort of stats out of it?

Has anyone looked at the marvell chips of late?

>
> Yup, also kinda expected; but another good way to visualise the impact.
> Nice!
>
> -Toke
>
> _______________________________________________
> Make-wifi-fast mailing list
> Make-wifi-fast@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/make-wifi-fast

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Kan Yan]

Dave Taht <dave@taht.net> writes:

> I hope to take a close look at the iwl ax200 chips soon. Unless
> someone beats me to it. Can we get these sort of stats out of it?

Here is a patch for the trace event I used to get the sojourn time:
https://drive.google.com/open?id=1Mq8BO_kcneXBqf3m5Rz5xhEMj9jNbcJv

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

> While you're running tests, could you do one with the target changed to
> 10ms, just to see what it looks like? Both sojourn time values and
> throughput would be interesting here, of course.

Apologize for the late reply. Here is the test results with target set to 10ms.
The trace for the sojourn time:
https://drive.google.com/open?id=1MEy_wbKKdl22yF17hZaGzpv3uOz6orTi

Flent test for 20 ms target time vs 10 ms target time:
https://drive.google.com/open?id=1leIWe0-L0XE78eFvlmRJlNmYgbpoH8xZ

The sojourn time measured during throughput test with a relative good
5G connection has mean value around 11 ms, pretty close to the 10 ms
target.

A smaller CoDel "target" time could help reduce latency, but it may
drop packets too aggressively for stations with low data rate and
hurts throughput, as shown in one of the tests with 2.4 GHz client.

Overall, I think AQL and fq_codel works well, at least with ath10k.
The current target value of 20 ms is a reasonable default.  It is
relatively conservative that helps stations with weak signal to
maintain stable throughput. Although, a debugfs entry that allows
runtime adjustment of target value could be useful.

On Tue, Nov 26, 2019 at 6:13 PM Dave Taht <dave@taht.net> wrote:
>
> Toke Høiland-Jørgensen <toke@redhat.com> writes:
>
> > Kan Yan <kyan@google.com> writes:
> >
> >>> Yeah, bpftrace can be a bit of a pain to get running; but it may be
> >>> worth the investment longer term as well. It really is quite useful! :)
> >>
> >> My attempt to build bpftrace didn't work out, so I just got the
> >> sojourn time using old fashioned trace event.
> >> The raw trace, parsed data in csv format and plots can be found here:
> >> https://drive.google.com/open?id=1Mg_wHu7elYAdkXz4u--42qGCVE1nrILV
> >>
> >> All tests are done with 2 TCP download sessions that oversubscribed
> >> the link bandwidth.
> >> With AQL on, the mean sojourn time about ~20000us, matches the default
> >> codel "target".
> >
> > Yeah, since CoDel is trying to control the latency to 20ms, it makes
> > sense that the value is clustered around that. That means that the
> > algorithm is working as they're supposed to :)
> >
> > While you're running tests, could you do one with the target changed to
> > 10ms, just to see what it looks like? Both sojourn time values and
> > throughput would be interesting here, of course.
> >
> >> With AQL off, the mean sojourn time is less than 4us even the latency
> >> is off the charts, just as we expected that fd_codel with mac80211
> >> alone is not effective for drivers with deep firmware/hardware queues
>
> I hope to take a close look at the iwl ax200 chips soon. Unless
> someone beats me to it. Can we get these sort of stats out of it?
>
> Has anyone looked at the marvell chips of late?
>
> >
> > Yup, also kinda expected; but another good way to visualise the impact.
> > Nice!
> >
> > -Toke
> >
> > _______________________________________________
> > Make-wifi-fast mailing list
> > Make-wifi-fast@lists.bufferbloat.net
> > https://lists.bufferbloat.net/listinfo/make-wifi-fast

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Kalle Valo]

Kan Yan <kyan@google.com> writes:

> Dave Taht <dave@taht.net> writes:
>
>> I hope to take a close look at the iwl ax200 chips soon. Unless
>> someone beats me to it. Can we get these sort of stats out of it?
>
> Here is a patch for the trace event I used to get the sojourn time:
> https://drive.google.com/open?id=1Mq8BO_kcneXBqf3m5Rz5xhEMj9jNbcJv
>
> Toke Høiland-Jørgensen <toke@redhat.com> writes:
>
>> While you're running tests, could you do one with the target changed to
>> 10ms, just to see what it looks like? Both sojourn time values and
>> throughput would be interesting here, of course.
>
> Apologize for the late reply. Here is the test results with target set to 10ms.
> The trace for the sojourn time:
> https://drive.google.com/open?id=1MEy_wbKKdl22yF17hZaGzpv3uOz6orTi
>
> Flent test for 20 ms target time vs 10 ms target time:
> https://drive.google.com/open?id=1leIWe0-L0XE78eFvlmRJlNmYgbpoH8xZ
>
> The sojourn time measured during throughput test with a relative good
> 5G connection has mean value around 11 ms, pretty close to the 10 ms
> target.
>
> A smaller CoDel "target" time could help reduce latency, but it may
> drop packets too aggressively for stations with low data rate and
> hurts throughput, as shown in one of the tests with 2.4 GHz client.
>
> Overall, I think AQL and fq_codel works well, at least with ath10k.
> The current target value of 20 ms is a reasonable default.  It is
> relatively conservative that helps stations with weak signal to
> maintain stable throughput. Although, a debugfs entry that allows
> runtime adjustment of target value could be useful.

Why not make it configurable via nl80211? We should use debugfs only for
testing and debugging, not in production builds, and to me the use case
for this value sounds like more than just testing.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Johannes Berg]

On Wed, 2019-12-04 at 04:47 +0000, Kalle Valo wrote:
> 
> > Overall, I think AQL and fq_codel works well, at least with ath10k.
> > The current target value of 20 ms is a reasonable default.  It is
> > relatively conservative that helps stations with weak signal to
> > maintain stable throughput. Although, a debugfs entry that allows
> > runtime adjustment of target value could be useful.
> 
> Why not make it configurable via nl80211? We should use debugfs only for
> testing and debugging, not in production builds, and to me the use case
> for this value sounds like more than just testing.

On the other hand, what application/tool or even user would be able to
set this correctly?

johannes

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2019-12-04 at 04:47 +0000, Kalle Valo wrote:
>> 
>> > Overall, I think AQL and fq_codel works well, at least with ath10k.
>> > The current target value of 20 ms is a reasonable default.  It is
>> > relatively conservative that helps stations with weak signal to
>> > maintain stable throughput. Although, a debugfs entry that allows
>> > runtime adjustment of target value could be useful.
>> 
>> Why not make it configurable via nl80211? We should use debugfs only for
>> testing and debugging, not in production builds, and to me the use case
>> for this value sounds like more than just testing.
>
> On the other hand, what application/tool or even user would be able to
> set this correctly?

Well, it's not inconceivable that someone might write a tool to
dynamically tune this; we do allow it to be changed at the qdisc layer
after all.

But until such a time as someone does that, I agree that it's not
terribly likely such a knob is going to see much use. As Kan's results
show, for inter-flow latency (like what the separate ping in his test is
showing), the FQ part takes care of the latency, and what's left there
is the AQL buffering. So I'm a little bit "meh" about this; wouldn't
object to making it a knob, but don't think I'm going to spend the time
writing that patch myself :)

-Toke

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Dave Taht]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2019-12-04 at 04:47 +0000, Kalle Valo wrote:
>> 
>> > Overall, I think AQL and fq_codel works well, at least with ath10k.
>> > The current target value of 20 ms is a reasonable default.

>> > It is
>> > relatively conservative that helps stations with weak signal to
>> > maintain stable throughput.

This statement is overbroad and largely incorrect.

>>> Although, a debugfs entry that allows
>> > runtime adjustment of target value could be useful.
>> 
>> Why not make it configurable via nl80211? We should use debugfs only for
>> testing and debugging, not in production builds, and to me the use case
>> for this value sounds like more than just testing.

I certainly lean towards making it configurable AND autotuning it
better.

> On the other hand, what application/tool or even user would be able to
> set this correctly?

The guideline from the theory ("Power") is the target should 5-10% of
the interval, and the interval fairly close to the most commonly
observed max RTT. I should try to stress (based on some statements made
here) - that you have to *consistently* exceed the target for the
interval, in order for codel to have any effect at all. Please try to
internalize that - the smoothing comes from the interval... 100ms is
quite a large interval....

Judging from kan's (rather noisy) data set 10ms is a good default on
5ghz. There is zero difference in throughput as near as I can tell.

It would be interesting to try 3ms (as there's up to 8ms of
buffering in the driver) to add to this dataset, helpful also
to be measuring the actual tcp rtt rather in addition to the fq behavior.

I see what looks like channel scan behavior in the data. (on the
client?) Running tests for 5 minutes will show the impact and frequency
of channel scans better.

The 20ms figure we used initially was due to a variety of factors:

* This was the first ever attempt at applying an AQM technology to wifi!!!
** FIXED: http://blog.cerowrt.org/post/real_results/
* We were debugging the FQ component, primarily.
** FIXED: http://blog.cerowrt.org/post/crypto_fq_bug/
* We were working on backports and on integrating a zillion other pieces
  all in motion.
** sorta FIXED. I know dang full well how many darn variables there
   are, as well as how much the network stack has changed since the initial work.
*  We were working on 2.4ghz which has a baseline rate of 1Mbit (13ms target)
   Our rule of thumb is that min target needs to MTU*1.5. There was also a
   a fudge factor to account for half duplex operation and the minimum
   size of a txop. 
** FIXED: 5ghz has a baseline rate of 6mbits.
* We didn't have tools to look at tcp rtts at the time
** FIXED: flent --socket-stats tcp_nup
* We had issues with power save
** Everybody has issues with powersave...
** These are still extant on many platforms, notably ones that wake up
   and dump all their accumulated mcast data into the link. Not our problem.
* channel scans: http://blog.cerowrt.org/post/disabling_channel_scans/
**  Non background channel scans are very damaging. I am unsure from this
    data if that's what we are seeing from the client? Or the ath10k?
    the ability to do these in the background or notmight be a factor in
    autotuning things better.
* We had MAJOR issues with TSQ
** FIXED: https://lwn.net/Articles/757643/

Honestly the TSQ interaction was the biggest barrier to figuring out
what was going wrong at the time we upstreamed this, and a tcp_nup test,
now, with TSQ closer to "right", AQL in place and the reduced target
should be interesting. I think the data we have now on TSQ vs wifi on
this chip, is now totally obsolete.

* We had issues with mcast
** I think we still have many issues with multicast but improving that
   is a separate problem entirely.
* We ran out of time and money, and had hit it so far out of the park
  ( https://lwn.net/Articles/705884/ ) 
  that it seemed like sleeping more and tweaking things less was a win.

Judging from the results we now get on 5ghz and on ac, it seems good to
reduce the target to 10ms (or less!) on 5ghz ghz, especially on ac,
which will result in a less path inflation and no loss in throughput.

I have been running with a 6ms target for several years now on my
802.11n 5ghz devices. (I advertise a 3ms rather than the default txop
size also) These are, admittedly, mostly used as backhaul
links (so I didn't have tsq, aql, rate changes, etc) , but seing a path
inflation of no more than 30ms under full bidirectional load is
nice. (and still 22ms worse than it could be in a more perfect world)

Another thing I keep trying to stress: TCP's ability to grab more
bandwidth is quadratic relative the delay.

>
> johannes

Re: [Make-wifi-fast] [PATCH v8 0/2] Implement Airtime-based Queue Limit (AQL)

[Kan Yan]

Dave Taht (taht.net) writes:

> Judging from kan's (rather noisy) data set 10ms is a good default on
> 5ghz. There is zero difference in throughput as near as I can tell.
> It would be interesting to try 3ms (as there's up to 8ms of
> buffering in the driver) to add to this dataset, helpful also
> to be measuring the actual tcp rtt rather in addition to the fq behavior.

One large aggregation in 11ac can last 4-5 ms, with bursting,
firmware/hardware can complete as much 8 -10 ms worth of frames in one
shot and then try to dequeue more frames, so the jitter for the
sojourn time can be as high as 8-10 ms. Setting the default target to
something less than 10ms can cause unnecessary packet drop in some
occasions.


On Fri, Dec 6, 2019 at 11:53 AM Dave Taht <dave@taht.net> wrote:
>
> Johannes Berg <johannes@sipsolutions.net> writes:
>
> > On Wed, 2019-12-04 at 04:47 +0000, Kalle Valo wrote:
> >>
> >> > Overall, I think AQL and fq_codel works well, at least with ath10k.
> >> > The current target value of 20 ms is a reasonable default.
>
> >> > It is
> >> > relatively conservative that helps stations with weak signal to
> >> > maintain stable throughput.
>
> This statement is overbroad and largely incorrect.
>
> >>> Although, a debugfs entry that allows
> >> > runtime adjustment of target value could be useful.
> >>
> >> Why not make it configurable via nl80211? We should use debugfs only for
> >> testing and debugging, not in production builds, and to me the use case
> >> for this value sounds like more than just testing.
>
> I certainly lean towards making it configurable AND autotuning it
> better.
>
> > On the other hand, what application/tool or even user would be able to
> > set this correctly?
>
> The guideline from the theory ("Power") is the target should 5-10% of
> the interval, and the interval fairly close to the most commonly
> observed max RTT. I should try to stress (based on some statements made
> here) - that you have to *consistently* exceed the target for the
> interval, in order for codel to have any effect at all. Please try to
> internalize that - the smoothing comes from the interval... 100ms is
> quite a large interval....
>
> Judging from kan's (rather noisy) data set 10ms is a good default on
> 5ghz. There is zero difference in throughput as near as I can tell.
>
> It would be interesting to try 3ms (as there's up to 8ms of
> buffering in the driver) to add to this dataset, helpful also
> to be measuring the actual tcp rtt rather in addition to the fq behavior.
>
> I see what looks like channel scan behavior in the data. (on the
> client?) Running tests for 5 minutes will show the impact and frequency
> of channel scans better.
>
> The 20ms figure we used initially was due to a variety of factors:
>
> * This was the first ever attempt at applying an AQM technology to wifi!!!
> ** FIXED: http://blog.cerowrt.org/post/real_results/
> * We were debugging the FQ component, primarily.
> ** FIXED: http://blog.cerowrt.org/post/crypto_fq_bug/
> * We were working on backports and on integrating a zillion other pieces
>   all in motion.
> ** sorta FIXED. I know dang full well how many darn variables there
>    are, as well as how much the network stack has changed since the initial work.
> *  We were working on 2.4ghz which has a baseline rate of 1Mbit (13ms target)
>    Our rule of thumb is that min target needs to MTU*1.5. There was also a
>    a fudge factor to account for half duplex operation and the minimum
>    size of a txop.
> ** FIXED: 5ghz has a baseline rate of 6mbits.
> * We didn't have tools to look at tcp rtts at the time
> ** FIXED: flent --socket-stats tcp_nup
> * We had issues with power save
> ** Everybody has issues with powersave...
> ** These are still extant on many platforms, notably ones that wake up
>    and dump all their accumulated mcast data into the link. Not our problem.
> * channel scans: http://blog.cerowrt.org/post/disabling_channel_scans/
> **  Non background channel scans are very damaging. I am unsure from this
>     data if that's what we are seeing from the client? Or the ath10k?
>     the ability to do these in the background or notmight be a factor in
>     autotuning things better.
> * We had MAJOR issues with TSQ
> ** FIXED: https://lwn.net/Articles/757643/
>
> Honestly the TSQ interaction was the biggest barrier to figuring out
> what was going wrong at the time we upstreamed this, and a tcp_nup test,
> now, with TSQ closer to "right", AQL in place and the reduced target
> should be interesting. I think the data we have now on TSQ vs wifi on
> this chip, is now totally obsolete.
>
> * We had issues with mcast
> ** I think we still have many issues with multicast but improving that
>    is a separate problem entirely.
> * We ran out of time and money, and had hit it so far out of the park
>   ( https://lwn.net/Articles/705884/ )
>   that it seemed like sleeping more and tweaking things less was a win.
>
> Judging from the results we now get on 5ghz and on ac, it seems good to
> reduce the target to 10ms (or less!) on 5ghz ghz, especially on ac,
> which will result in a less path inflation and no loss in throughput.
>
> I have been running with a 6ms target for several years now on my
> 802.11n 5ghz devices. (I advertise a 3ms rather than the default txop
> size also) These are, admittedly, mostly used as backhaul
> links (so I didn't have tsq, aql, rate changes, etc) , but seing a path
> inflation of no more than 30ms under full bidirectional load is
> nice. (and still 22ms worse than it could be in a more perfect world)
>
> Another thing I keep trying to stress: TCP's ability to grab more
> bandwidth is quadratic relative the delay.
>
> >
> > johannes