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

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

[Toke Høiland-Jørgensen]

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

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

As before, I've only compile tested this (lacking the proper hardware to do more
testing). So I'm hoping someone with a proper testing setup can take the whole
thing for a spin... :)

The series is also available in my git repo here:
https://git.kernel.org/pub/scm/linux/kernel/git/toke/linux.git/log/?h=mac80211-aql-02

Changelog:

v2:
  - Integrate Kan's approach to airtime throttling.
  - Hopefully fix the cb struct alignment on big-endian architectures.

---

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

Toke Høiland-Jørgensen (3):
      mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
      mac80211: Import airtime calculation code from mt76
      mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue


 include/net/cfg80211.h     |    7 +
 include/net/mac80211.h     |   52 +++++-
 net/mac80211/Makefile      |    3 
 net/mac80211/airtime.c     |  375 ++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/debugfs.c     |   78 +++++++++
 net/mac80211/debugfs_sta.c |   43 ++++-
 net/mac80211/ieee80211_i.h |    8 +
 net/mac80211/main.c        |    9 +
 net/mac80211/sta_info.c    |   32 ++++
 net/mac80211/sta_info.h    |    8 +
 net/mac80211/status.c      |   38 ++++
 net/mac80211/tx.c          |   62 +++++++
 12 files changed, 693 insertions(+), 22 deletions(-)
 create mode 100644 net/mac80211/airtime.c

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

[Toke Høiland-Jørgensen]

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

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

As before, I've only compile tested this (lacking the proper hardware to do more
testing). So I'm hoping someone with a proper testing setup can take the whole
thing for a spin... :)

The series is also available in my git repo here:
https://git.kernel.org/pub/scm/linux/kernel/git/toke/linux.git/log/?h=mac80211-aql-02

Changelog:

v2:
  - Integrate Kan's approach to airtime throttling.
  - Hopefully fix the cb struct alignment on big-endian architectures.

---

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

Toke Høiland-Jørgensen (3):
      mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
      mac80211: Import airtime calculation code from mt76
      mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue


 include/net/cfg80211.h     |    7 +
 include/net/mac80211.h     |   52 +++++-
 net/mac80211/Makefile      |    3 
 net/mac80211/airtime.c     |  375 ++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/debugfs.c     |   78 +++++++++
 net/mac80211/debugfs_sta.c |   43 ++++-
 net/mac80211/ieee80211_i.h |    8 +
 net/mac80211/main.c        |    9 +
 net/mac80211/sta_info.c    |   32 ++++
 net/mac80211/sta_info.h    |    8 +
 net/mac80211/status.c      |   38 ++++
 net/mac80211/tx.c          |   62 +++++++
 12 files changed, 693 insertions(+), 22 deletions(-)
 create mode 100644 net/mac80211/airtime.c


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

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

To implement airtime queue limiting, we need to keep a running account of
the estimated airtime of all skbs queued into the device. Do to this
correctly, we need to store the airtime estimate into the skb so we can
decrease the outstanding balance when the skb is freed. This means that the
time estimate must be stored somewhere that will survive for the lifetime
of the skb.

Fortunately, we had a couple of bytes left in the 'status' field in the
ieee80211_tx_info; and since we only plan to calculate the airtime estimate
after the skb is dequeued from the FQ structure, on the control side we can
share the space with the codel enqueue time. And by rearranging the order
of elements it is possible to have the position of the new tx_time_est line
up between the control and status structs, so the value will survive from
when mac80211 hands the packet to the driver, and until the driver either
frees it, or hands it back through TX status.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/mac80211.h |   26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d69081c38788..49f8ea0af5f8 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -975,20 +975,23 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate)
  * @control.short_preamble: use short preamble (CCK only)
  * @control.skip_table: skip externally configured rate table
  * @control.jiffies: timestamp for expiry on powersave clients
+ * @control.enqueue_time: enqueue time (for iTXQs)
+ * @control.tx_time_est: estimated airtime usage (shared with @status)
+ * @control.reserved: unused field to ensure alignment of data structure
+ * @control.flags: control flags, see &enum mac80211_tx_control_flags
  * @control.vif: virtual interface (may be NULL)
  * @control.hw_key: key to encrypt with (may be NULL)
- * @control.flags: control flags, see &enum mac80211_tx_control_flags
- * @control.enqueue_time: enqueue time (for iTXQs)
  * @driver_rates: alias to @control.rates to reserve space
  * @pad: padding
  * @rate_driver_data: driver use area if driver needs @control.rates
  * @status: union part for status data
  * @status.rates: attempted rates
  * @status.ack_signal: ACK signal
+ * @status.tx_time_est: estimated airtime of skb (shared with @control)
+ * @status.tx_time: actual airtime consumed for transmission
  * @status.ampdu_ack_len: AMPDU ack length
  * @status.ampdu_len: AMPDU length
  * @status.antenna: (legacy, kept only for iwlegacy)
- * @status.tx_time: airtime consumed for transmission
  * @status.is_valid_ack_signal: ACK signal is valid
  * @status.status_driver_data: driver use area
  * @ack: union part for pure ACK data
@@ -1026,11 +1029,17 @@ struct ieee80211_tx_info {
 				/* only needed before rate control */
 				unsigned long jiffies;
 			};
+			union {
+				codel_time_t enqueue_time;
+				struct {
+					u16 tx_time_est; /* shared with status */
+					u16 reserved; /* padding for alignment */
+				};
+			};
+			u32 flags;
 			/* NB: vif can be NULL for injected frames */
 			struct ieee80211_vif *vif;
 			struct ieee80211_key_conf *hw_key;
-			u32 flags;
-			codel_time_t enqueue_time;
 		} control;
 		struct {
 			u64 cookie;
@@ -1038,12 +1047,13 @@ struct ieee80211_tx_info {
 		struct {
 			struct ieee80211_tx_rate rates[IEEE80211_TX_MAX_RATES];
 			s32 ack_signal;
+			u16 tx_time_est; /* shared with control */
+			u16 tx_time;
 			u8 ampdu_ack_len;
 			u8 ampdu_len;
 			u8 antenna;
-			u16 tx_time;
 			bool is_valid_ack_signal;
-			void *status_driver_data[19 / sizeof(void *)];
+			void *status_driver_data[16 / sizeof(void *)];
 		} status;
 		struct {
 			struct ieee80211_tx_rate driver_rates[
@@ -1126,6 +1136,8 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
 		     offsetof(struct ieee80211_tx_info, control.rates));
 	BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
 		     offsetof(struct ieee80211_tx_info, driver_rates));
+	BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, control.tx_time_est) !=
+		     offsetof(struct ieee80211_tx_info, status.tx_time_est));
 	BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8);
 	/* clear the rate counts */
 	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)

[PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

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

To implement airtime queue limiting, we need to keep a running account of
the estimated airtime of all skbs queued into the device. Do to this
correctly, we need to store the airtime estimate into the skb so we can
decrease the outstanding balance when the skb is freed. This means that the
time estimate must be stored somewhere that will survive for the lifetime
of the skb.

Fortunately, we had a couple of bytes left in the 'status' field in the
ieee80211_tx_info; and since we only plan to calculate the airtime estimate
after the skb is dequeued from the FQ structure, on the control side we can
share the space with the codel enqueue time. And by rearranging the order
of elements it is possible to have the position of the new tx_time_est line
up between the control and status structs, so the value will survive from
when mac80211 hands the packet to the driver, and until the driver either
frees it, or hands it back through TX status.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/mac80211.h |   26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d69081c38788..49f8ea0af5f8 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -975,20 +975,23 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate)
  * @control.short_preamble: use short preamble (CCK only)
  * @control.skip_table: skip externally configured rate table
  * @control.jiffies: timestamp for expiry on powersave clients
+ * @control.enqueue_time: enqueue time (for iTXQs)
+ * @control.tx_time_est: estimated airtime usage (shared with @status)
+ * @control.reserved: unused field to ensure alignment of data structure
+ * @control.flags: control flags, see &enum mac80211_tx_control_flags
  * @control.vif: virtual interface (may be NULL)
  * @control.hw_key: key to encrypt with (may be NULL)
- * @control.flags: control flags, see &enum mac80211_tx_control_flags
- * @control.enqueue_time: enqueue time (for iTXQs)
  * @driver_rates: alias to @control.rates to reserve space
  * @pad: padding
  * @rate_driver_data: driver use area if driver needs @control.rates
  * @status: union part for status data
  * @status.rates: attempted rates
  * @status.ack_signal: ACK signal
+ * @status.tx_time_est: estimated airtime of skb (shared with @control)
+ * @status.tx_time: actual airtime consumed for transmission
  * @status.ampdu_ack_len: AMPDU ack length
  * @status.ampdu_len: AMPDU length
  * @status.antenna: (legacy, kept only for iwlegacy)
- * @status.tx_time: airtime consumed for transmission
  * @status.is_valid_ack_signal: ACK signal is valid
  * @status.status_driver_data: driver use area
  * @ack: union part for pure ACK data
@@ -1026,11 +1029,17 @@ struct ieee80211_tx_info {
 				/* only needed before rate control */
 				unsigned long jiffies;
 			};
+			union {
+				codel_time_t enqueue_time;
+				struct {
+					u16 tx_time_est; /* shared with status */
+					u16 reserved; /* padding for alignment */
+				};
+			};
+			u32 flags;
 			/* NB: vif can be NULL for injected frames */
 			struct ieee80211_vif *vif;
 			struct ieee80211_key_conf *hw_key;
-			u32 flags;
-			codel_time_t enqueue_time;
 		} control;
 		struct {
 			u64 cookie;
@@ -1038,12 +1047,13 @@ struct ieee80211_tx_info {
 		struct {
 			struct ieee80211_tx_rate rates[IEEE80211_TX_MAX_RATES];
 			s32 ack_signal;
+			u16 tx_time_est; /* shared with control */
+			u16 tx_time;
 			u8 ampdu_ack_len;
 			u8 ampdu_len;
 			u8 antenna;
-			u16 tx_time;
 			bool is_valid_ack_signal;
-			void *status_driver_data[19 / sizeof(void *)];
+			void *status_driver_data[16 / sizeof(void *)];
 		} status;
 		struct {
 			struct ieee80211_tx_rate driver_rates[
@@ -1126,6 +1136,8 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
 		     offsetof(struct ieee80211_tx_info, control.rates));
 	BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
 		     offsetof(struct ieee80211_tx_info, driver_rates));
+	BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, control.tx_time_est) !=
+		     offsetof(struct ieee80211_tx_info, status.tx_time_est));
 	BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8);
 	/* clear the rate counts */
 	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Toke Høiland-Jørgensen]

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 later.

The airtime.c file is copied verbatim from the mt76 driver, and adjusted to
use mac80211 data structures instead (which is fairly straight forward).
The per-rate TX rate calculation is split out to its own
function (ieee80211_calc_tx_airtime_rate()) so it can be used directly for
the AQL calculations added in a subsequent patch.

The only thing that it was not possible to port directly was the bit that
read the internal driver flags of struct ieee80211_rate to determine
whether a rate is using CCK or OFDM encoding. Instead, just look at the
rate index, since at least mt76 and ath10k both seem to have the same
number of CCK rates (4) in their tables.

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

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 49f8ea0af5f8..7619eb74d612 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -6434,4 +6434,18 @@ void ieee80211_nan_func_match(struct ieee80211_vif *vif,
 			      struct cfg80211_nan_match_params *match,
 			      gfp_t gfp);
 
+/**
+ * ieee80211_calc_tx_airtime - calculate estimated transmission airtime.
+ *
+ * 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..7a18d5405756
--- /dev/null
+++ b/net/mac80211/airtime.c
@@ -0,0 +1,375 @@
+// 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 symbols for a packet with (bps) bits per symbol */
+#define MCS_NSYMS(bps) DIV_ROUND_UP(MCS_NBITS, (bps))
+
+/* Transmission time (1024 usec) for a packet containing (syms) * symbols */
+#define MCS_SYMBOL_TIME(sgi, syms)					\
+	(sgi ?								\
+	  ((syms) * 18 * 1024 + 4 * 1024) / 5 :	/* syms * 3.6 us */	\
+	  ((syms) * 1024) << 2			/* syms * 4 us */	\
+	)
+
+/* Transmit duration for the raw data part of an average sized packet */
+#define MCS_DURATION(streams, sgi, bps) \
+	MCS_SYMBOL_TIME(sgi, MCS_NSYMS((streams) * (bps)))
+
+#define BW_20			0
+#define BW_40			1
+#define BW_80			2
+
+/*
+ * 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	6 /* BW(=3) * SGI(=2) */
+
+#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_GROUPS_NB	(IEEE80211_HT_GROUPS_NB +	\
+				 IEEE80211_VHT_GROUPS_NB)
+
+#define IEEE80211_HT_GROUP_0	0
+#define IEEE80211_VHT_GROUP_0	(IEEE80211_HT_GROUP_0 + IEEE80211_HT_GROUPS_NB)
+
+#define MCS_GROUP_RATES		10
+#define CCK_NUM_RATES			4
+
+#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(_streams, _sgi, _ht40 ? 54 : 26) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 108 : 52) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 162 : 78) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 216 : 104) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 324 : 156) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 432 : 208) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 486 : 234) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 540 : 260) >> _s	\
+	}								\
+}
+
+#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, r3, r2, r1)					\
+	(_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(_streams, _sgi,				\
+			     BW2VBPS(_bw,  117,  54,  26)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw,  234, 108,  52)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw,  351, 162,  78)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw,  468, 216, 104)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw,  702, 324, 156)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw,  936, 432, 208)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw, 1053, 486, 234)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw, 1170, 540, 260)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw, 1404, 648, 312)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw, 1560, 720, 346)) >> _s	\
+	}								\
+}
+
+#define VHT_GROUP_SHIFT(_streams, _sgi, _bw)				\
+	GROUP_SHIFT(MCS_DURATION(_streams, _sgi,			\
+				 BW2VBPS(_bw,  117,  54,  26)))
+
+#define VHT_GROUP(_streams, _sgi, _bw)					\
+	__VHT_GROUP(_streams, _sgi, _bw,				\
+		    VHT_GROUP_SHIFT(_streams, _sgi, _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),
+};
+
+static u32
+ieee80211_calc_legacy_rate_duration(const struct ieee80211_rate *rate,
+				    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) / rate->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;
+	u32 duration;
+	int group, idx;
+	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;
+	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 = (status->rate_idx < CCK_NUM_RATES);
+
+		return ieee80211_calc_legacy_rate_duration(rate, 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;
+	default:
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
+	if (WARN_ON_ONCE(streams > 4))
+		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;
+}
+
+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;
+	struct ieee80211_rate *rate;
+	struct sta_info *sta;
+	u8 band = 0;
+
+	conf = rcu_dereference(vif->chanctx_conf);
+	if (conf)
+		band = conf->def.chan->band;
+
+	if (pubsta) {
+		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 for the lowest rate.
+	 */
+	sband = hw->wiphy->bands[band];
+	rate = &sband->bitrates[0];
+
+	return ieee80211_calc_legacy_rate_duration(rate, false, true, 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

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

[Toke Høiland-Jørgensen]

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 later.

The airtime.c file is copied verbatim from the mt76 driver, and adjusted to
use mac80211 data structures instead (which is fairly straight forward).
The per-rate TX rate calculation is split out to its own
function (ieee80211_calc_tx_airtime_rate()) so it can be used directly for
the AQL calculations added in a subsequent patch.

The only thing that it was not possible to port directly was the bit that
read the internal driver flags of struct ieee80211_rate to determine
whether a rate is using CCK or OFDM encoding. Instead, just look at the
rate index, since at least mt76 and ath10k both seem to have the same
number of CCK rates (4) in their tables.

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

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 49f8ea0af5f8..7619eb74d612 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -6434,4 +6434,18 @@ void ieee80211_nan_func_match(struct ieee80211_vif *vif,
 			      struct cfg80211_nan_match_params *match,
 			      gfp_t gfp);
 
+/**
+ * ieee80211_calc_tx_airtime - calculate estimated transmission airtime.
+ *
+ * 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..7a18d5405756
--- /dev/null
+++ b/net/mac80211/airtime.c
@@ -0,0 +1,375 @@
+// 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 symbols for a packet with (bps) bits per symbol */
+#define MCS_NSYMS(bps) DIV_ROUND_UP(MCS_NBITS, (bps))
+
+/* Transmission time (1024 usec) for a packet containing (syms) * symbols */
+#define MCS_SYMBOL_TIME(sgi, syms)					\
+	(sgi ?								\
+	  ((syms) * 18 * 1024 + 4 * 1024) / 5 :	/* syms * 3.6 us */	\
+	  ((syms) * 1024) << 2			/* syms * 4 us */	\
+	)
+
+/* Transmit duration for the raw data part of an average sized packet */
+#define MCS_DURATION(streams, sgi, bps) \
+	MCS_SYMBOL_TIME(sgi, MCS_NSYMS((streams) * (bps)))
+
+#define BW_20			0
+#define BW_40			1
+#define BW_80			2
+
+/*
+ * 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	6 /* BW(=3) * SGI(=2) */
+
+#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_GROUPS_NB	(IEEE80211_HT_GROUPS_NB +	\
+				 IEEE80211_VHT_GROUPS_NB)
+
+#define IEEE80211_HT_GROUP_0	0
+#define IEEE80211_VHT_GROUP_0	(IEEE80211_HT_GROUP_0 + IEEE80211_HT_GROUPS_NB)
+
+#define MCS_GROUP_RATES		10
+#define CCK_NUM_RATES			4
+
+#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(_streams, _sgi, _ht40 ? 54 : 26) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 108 : 52) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 162 : 78) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 216 : 104) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 324 : 156) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 432 : 208) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 486 : 234) >> _s,	\
+		MCS_DURATION(_streams, _sgi, _ht40 ? 540 : 260) >> _s	\
+	}								\
+}
+
+#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, r3, r2, r1)					\
+	(_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(_streams, _sgi,				\
+			     BW2VBPS(_bw,  117,  54,  26)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw,  234, 108,  52)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw,  351, 162,  78)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw,  468, 216, 104)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw,  702, 324, 156)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw,  936, 432, 208)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw, 1053, 486, 234)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw, 1170, 540, 260)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw, 1404, 648, 312)) >> _s,	\
+		MCS_DURATION(_streams, _sgi,				\
+			     BW2VBPS(_bw, 1560, 720, 346)) >> _s	\
+	}								\
+}
+
+#define VHT_GROUP_SHIFT(_streams, _sgi, _bw)				\
+	GROUP_SHIFT(MCS_DURATION(_streams, _sgi,			\
+				 BW2VBPS(_bw,  117,  54,  26)))
+
+#define VHT_GROUP(_streams, _sgi, _bw)					\
+	__VHT_GROUP(_streams, _sgi, _bw,				\
+		    VHT_GROUP_SHIFT(_streams, _sgi, _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),
+};
+
+static u32
+ieee80211_calc_legacy_rate_duration(const struct ieee80211_rate *rate,
+				    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) / rate->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;
+	u32 duration;
+	int group, idx;
+	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;
+	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 = (status->rate_idx < CCK_NUM_RATES);
+
+		return ieee80211_calc_legacy_rate_duration(rate, 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;
+	default:
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
+	if (WARN_ON_ONCE(streams > 4))
+		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;
+}
+
+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;
+	struct ieee80211_rate *rate;
+	struct sta_info *sta;
+	u8 band = 0;
+
+	conf = rcu_dereference(vif->chanctx_conf);
+	if (conf)
+		band = conf->def.chan->band;
+
+	if (pubsta) {
+		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 for the lowest rate.
+	 */
+	sband = hw->wiphy->bands[band];
+	rate = &sband->bitrates[0];
+
+	return ieee80211_calc_legacy_rate_duration(rate, false, true, 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


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Toke Høiland-Jørgensen]

From: Kan Yan <kyan@google.com>

In order for the Fq_CoDel integrated in mac80211 layer operates effectively
to control excessive queueing latency, the CoDel algorithm requires an
accurate measure of how long the packets stays in the queue, aka sojourn
time. The sojourn time measured at mac80211 layer doesn't include queueing
latency in 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
stays 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.

Byte Queue limits (BQL) is commonly used to address the similar issue with
wired network interface. However, this method cannot be applied directly
to the wireless network interface. Byte 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 implemented an Airtime-based Queue Limit (AQL) to make CoDel
works effectively with wireless drivers that utilized firmware/hardware
offloading. AQL only allows each txq to release just enough packets to the
lower layer to form 1-2 large aggregations to keep hardware fully utilized
and keep the rest of frames in mac80211 layer to be controlled by the CoDel
algorithm.

Signed-off-by: Kan Yan <kyan@google.com>
[ Toke: Get rid of the driver API to set pending airtime ]
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     |   78 ++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/debugfs_sta.c |   43 +++++++++++++++++++-----
 net/mac80211/ieee80211_i.h |    4 ++
 net/mac80211/main.c        |    9 +++++
 net/mac80211/sta_info.c    |   32 ++++++++++++++++++
 net/mac80211/sta_info.h    |    8 +++++
 net/mac80211/tx.c          |   46 ++++++++++++++++++++++++--
 9 files changed, 225 insertions(+), 14 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ff45c3e1abff..8d50c0a60dbd 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 7619eb74d612..b5727a20754c 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5575,6 +5575,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..d77ea0e51c1d 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -148,6 +148,80 @@ 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;
+	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;
+
+	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) {
+		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 +515,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..9f9b8f5ed86a 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;
+		q_depth[ac] = sta->airtime[ac].aql_tx_pending;
 		spin_unlock_bh(&local->active_txq_lock[ac]);
 	}
 
 	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..6fa690757388 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;
+	u32 aql_total_pending_airtime;
 
 	const struct ieee80211_ops *ops;
 
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index aba094b4ccfc..0792c9b9c850 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -667,8 +667,15 @@ 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;
 
 	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 bd11fef2139f..64bacf4f068c 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;
+		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,35 @@ 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)
+{
+	spin_lock_bh(&local->active_txq_lock[ac]);
+	if (tx_completed) {
+		if (sta) {
+			if (WARN_ONCE(sta->airtime[ac].aql_tx_pending < tx_airtime,
+				      "TXQ pending airtime underflow: %u, %u",
+				      sta->airtime[ac].aql_tx_pending, tx_airtime))
+				sta->airtime[ac].aql_tx_pending = 0;
+			else
+				sta->airtime[ac].aql_tx_pending -= tx_airtime;
+		}
+
+		if (WARN_ONCE(local->aql_total_pending_airtime < tx_airtime,
+			      "Device pending airtime underflow: %u, %u",
+			      local->aql_total_pending_airtime, tx_airtime))
+			local->aql_total_pending_airtime = 0;
+		else
+			local->aql_total_pending_airtime -= tx_airtime;
+	} else {
+		if (sta)
+			sta->airtime[ac].aql_tx_pending += tx_airtime;
+		local->aql_total_pending_airtime += tx_airtime;
+	}
+	spin_unlock_bh(&local->active_txq_lock[ac]);
+}
+
 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..4e4d76e81b0f 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;
+	u32 aql_tx_pending; /* Estimated airtime for frames pending in queue */
+	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 938c10f7955b..405f622b3fe0 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3665,7 +3665,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]);
 
@@ -3676,13 +3677,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;
@@ -3736,6 +3750,32 @@ 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 (sta->airtime[txq->ac].aql_tx_pending <
+	    sta->airtime[txq->ac].aql_limit_low)
+		return true;
+
+	if (local->aql_total_pending_airtime < local->aql_threshold &&
+	    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)
 {

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

[Toke Høiland-Jørgensen]

From: Kan Yan <kyan@google.com>

In order for the Fq_CoDel integrated in mac80211 layer operates effectively
to control excessive queueing latency, the CoDel algorithm requires an
accurate measure of how long the packets stays in the queue, aka sojourn
time. The sojourn time measured at mac80211 layer doesn't include queueing
latency in 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
stays 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.

Byte Queue limits (BQL) is commonly used to address the similar issue with
wired network interface. However, this method cannot be applied directly
to the wireless network interface. Byte 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 implemented an Airtime-based Queue Limit (AQL) to make CoDel
works effectively with wireless drivers that utilized firmware/hardware
offloading. AQL only allows each txq to release just enough packets to the
lower layer to form 1-2 large aggregations to keep hardware fully utilized
and keep the rest of frames in mac80211 layer to be controlled by the CoDel
algorithm.

Signed-off-by: Kan Yan <kyan@google.com>
[ Toke: Get rid of the driver API to set pending airtime ]
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     |   78 ++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/debugfs_sta.c |   43 +++++++++++++++++++-----
 net/mac80211/ieee80211_i.h |    4 ++
 net/mac80211/main.c        |    9 +++++
 net/mac80211/sta_info.c    |   32 ++++++++++++++++++
 net/mac80211/sta_info.h    |    8 +++++
 net/mac80211/tx.c          |   46 ++++++++++++++++++++++++--
 9 files changed, 225 insertions(+), 14 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ff45c3e1abff..8d50c0a60dbd 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 7619eb74d612..b5727a20754c 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5575,6 +5575,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..d77ea0e51c1d 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -148,6 +148,80 @@ 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;
+	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;
+
+	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) {
+		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 +515,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..9f9b8f5ed86a 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;
+		q_depth[ac] = sta->airtime[ac].aql_tx_pending;
 		spin_unlock_bh(&local->active_txq_lock[ac]);
 	}
 
 	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..6fa690757388 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;
+	u32 aql_total_pending_airtime;
 
 	const struct ieee80211_ops *ops;
 
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index aba094b4ccfc..0792c9b9c850 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -667,8 +667,15 @@ 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;
 
 	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 bd11fef2139f..64bacf4f068c 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;
+		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,35 @@ 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)
+{
+	spin_lock_bh(&local->active_txq_lock[ac]);
+	if (tx_completed) {
+		if (sta) {
+			if (WARN_ONCE(sta->airtime[ac].aql_tx_pending < tx_airtime,
+				      "TXQ pending airtime underflow: %u, %u",
+				      sta->airtime[ac].aql_tx_pending, tx_airtime))
+				sta->airtime[ac].aql_tx_pending = 0;
+			else
+				sta->airtime[ac].aql_tx_pending -= tx_airtime;
+		}
+
+		if (WARN_ONCE(local->aql_total_pending_airtime < tx_airtime,
+			      "Device pending airtime underflow: %u, %u",
+			      local->aql_total_pending_airtime, tx_airtime))
+			local->aql_total_pending_airtime = 0;
+		else
+			local->aql_total_pending_airtime -= tx_airtime;
+	} else {
+		if (sta)
+			sta->airtime[ac].aql_tx_pending += tx_airtime;
+		local->aql_total_pending_airtime += tx_airtime;
+	}
+	spin_unlock_bh(&local->active_txq_lock[ac]);
+}
+
 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..4e4d76e81b0f 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;
+	u32 aql_tx_pending; /* Estimated airtime for frames pending in queue */
+	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 938c10f7955b..405f622b3fe0 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3665,7 +3665,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]);
 
@@ -3676,13 +3677,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;
@@ -3736,6 +3750,32 @@ 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 (sta->airtime[txq->ac].aql_tx_pending <
+	    sta->airtime[txq->ac].aql_limit_low)
+		return true;
+
+	if (local->aql_total_pending_airtime < local->aql_threshold &&
+	    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)
 {


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Toke Høiland-Jørgensen]

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

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

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

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

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/mac80211/status.c |   38 ++++++++++++++++++++++++++++++++++++++
 net/mac80211/tx.c     |   16 ++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index ab8ba5835ca0..ce990a1e9043 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -676,6 +676,33 @@ static void ieee80211_report_used_skb(struct ieee80211_local *local,
 	if (dropped)
 		acked = false;
 
+	if (info->status.tx_time_est) {
+		struct ieee80211_sub_if_data *sdata;
+		struct sta_info *sta = NULL;
+		u8 *qc, ac;
+		int tid;
+
+		rcu_read_lock();
+
+		sdata = ieee80211_sdata_from_skb(local, skb);
+		if (sdata)
+			sta = sta_info_get_bss(sdata, skb_mac_header(skb));
+
+		if (ieee80211_is_data_qos(hdr->frame_control)) {
+			qc = ieee80211_get_qos_ctl(hdr);
+			tid = qc[0] & 0xf;
+			ac = ieee80211_ac_from_tid(tid);
+		} else {
+			ac = IEEE80211_AC_BE;
+		}
+
+		ieee80211_sta_update_pending_airtime(local, sta, ac,
+						     info->status.tx_time_est,
+						     true);
+		rcu_read_unlock();
+
+	}
+
 	if (info->flags & IEEE80211_TX_INTFL_MLME_CONN_TX) {
 		struct ieee80211_sub_if_data *sdata;
 
@@ -930,6 +957,17 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
 			tid = qc[0] & 0xf;
 		}
 
+		if (info->status.tx_time_est) {
+			/* Do this here to avoid the expensive lookup of the sta
+			 * in ieee80211_report_used_skb().
+			 */
+			ieee80211_sta_update_pending_airtime(local, sta,
+							     ieee80211_ac_from_tid(tid),
+							     info->status.tx_time_est,
+							     true);
+			info->status.tx_time_est = 0;
+		}
+
 		if (!acked && ieee80211_is_back_req(fc)) {
 			u16 control;
 
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 405f622b3fe0..b6b47171b340 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3539,9 +3539,14 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	struct ieee80211_tx_data tx;
 	ieee80211_tx_result r;
 	struct ieee80211_vif *vif = txq->vif;
+	u8 ac = txq->ac;
+	u32 airtime;
 
 	WARN_ON_ONCE(softirq_count() == 0);
 
+	if (!ieee80211_txq_airtime_check(hw, txq))
+		return NULL;
+
 begin:
 	spin_lock_bh(&fq->lock);
 
@@ -3652,6 +3657,17 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	}
 
 	IEEE80211_SKB_CB(skb)->control.vif = vif;
+
+	if (local->airtime_flags & AIRTIME_USE_AQL) {
+		airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
+							     skb->len + 38);
+		if (airtime) {
+			info->control.tx_time_est = airtime;
+			ieee80211_sta_update_pending_airtime(local, tx.sta, ac,
+							     airtime, false);
+		}
+	}
+
 	return skb;
 
 out:

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

[Toke Høiland-Jørgensen]

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

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

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

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

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/mac80211/status.c |   38 ++++++++++++++++++++++++++++++++++++++
 net/mac80211/tx.c     |   16 ++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index ab8ba5835ca0..ce990a1e9043 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -676,6 +676,33 @@ static void ieee80211_report_used_skb(struct ieee80211_local *local,
 	if (dropped)
 		acked = false;
 
+	if (info->status.tx_time_est) {
+		struct ieee80211_sub_if_data *sdata;
+		struct sta_info *sta = NULL;
+		u8 *qc, ac;
+		int tid;
+
+		rcu_read_lock();
+
+		sdata = ieee80211_sdata_from_skb(local, skb);
+		if (sdata)
+			sta = sta_info_get_bss(sdata, skb_mac_header(skb));
+
+		if (ieee80211_is_data_qos(hdr->frame_control)) {
+			qc = ieee80211_get_qos_ctl(hdr);
+			tid = qc[0] & 0xf;
+			ac = ieee80211_ac_from_tid(tid);
+		} else {
+			ac = IEEE80211_AC_BE;
+		}
+
+		ieee80211_sta_update_pending_airtime(local, sta, ac,
+						     info->status.tx_time_est,
+						     true);
+		rcu_read_unlock();
+
+	}
+
 	if (info->flags & IEEE80211_TX_INTFL_MLME_CONN_TX) {
 		struct ieee80211_sub_if_data *sdata;
 
@@ -930,6 +957,17 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
 			tid = qc[0] & 0xf;
 		}
 
+		if (info->status.tx_time_est) {
+			/* Do this here to avoid the expensive lookup of the sta
+			 * in ieee80211_report_used_skb().
+			 */
+			ieee80211_sta_update_pending_airtime(local, sta,
+							     ieee80211_ac_from_tid(tid),
+							     info->status.tx_time_est,
+							     true);
+			info->status.tx_time_est = 0;
+		}
+
 		if (!acked && ieee80211_is_back_req(fc)) {
 			u16 control;
 
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 405f622b3fe0..b6b47171b340 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3539,9 +3539,14 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	struct ieee80211_tx_data tx;
 	ieee80211_tx_result r;
 	struct ieee80211_vif *vif = txq->vif;
+	u8 ac = txq->ac;
+	u32 airtime;
 
 	WARN_ON_ONCE(softirq_count() == 0);
 
+	if (!ieee80211_txq_airtime_check(hw, txq))
+		return NULL;
+
 begin:
 	spin_lock_bh(&fq->lock);
 
@@ -3652,6 +3657,17 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	}
 
 	IEEE80211_SKB_CB(skb)->control.vif = vif;
+
+	if (local->airtime_flags & AIRTIME_USE_AQL) {
+		airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
+							     skb->len + 38);
+		if (airtime) {
+			info->control.tx_time_est = airtime;
+			ieee80211_sta_update_pending_airtime(local, tx.sta, ac,
+							     airtime, false);
+		}
+	}
+
 	return skb;
 
 out:


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Kan Yan]

Hi Toke,

Thanks for getting this done! I will give it a try in the next few
days.  A few comments:

> The estimated airtime for each skb is stored in the tx_info, so we can
> subtract the same amount from the running total when the skb is freed or
> recycled.

Looks like ath10k driver zero out the info->status before calling
ieee80211_tx_status(...):
int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
                         const struct htt_tx_done *tx_done)
{
 ...
        info = IEEE80211_SKB_CB(msdu);
        memset(&info->status, 0, sizeof(info->status));
...
        ieee80211_tx_status(htt->ar->hw, msdu);
}

We need either restore the info->status.tx_time_est or calling
ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
get erased.

> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
> +                                                            skb->len + 38);

I think it is better to put the "+  38" that takes care of the header
overhead inside ieee80211_calc_expected_tx_airtime().

Kan


On Tue, Oct 15, 2019 at 10:19 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> The previous commit added the ability to throttle stations when they queue
> too much airtime in the hardware. This commit enables the functionality by
> calculating the expected airtime usage of each packet that is dequeued from
> the TXQs in mac80211, and accounting that as pending airtime.
>
> The estimated airtime for each skb is stored in the tx_info, so we can
> subtract the same amount from the running total when the skb is freed or
> recycled. The throttling mechanism relies on this accounting to be
> accurate (i.e., that we are not freeing skbs without subtracting any
> airtime they were accounted for), so we put the subtraction into
> ieee80211_report_used_skb(). As an optimisation, we also subtract the
> airtime on regular TX completion, zeroing out the value stored in the
> packet afterwards, to avoid having to do an expensive lookup of the
> station from the packet data on every packet.
>
> This patch does *not* include any mechanism to wake a throttled TXQ again,
> on the assumption that this will happen anyway as a side effect of whatever
> freed the skb (most commonly a TX completion).
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  net/mac80211/status.c |   38 ++++++++++++++++++++++++++++++++++++++
>  net/mac80211/tx.c     |   16 ++++++++++++++++
>  2 files changed, 54 insertions(+)
>
> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
> index ab8ba5835ca0..ce990a1e9043 100644
> --- a/net/mac80211/status.c
> +++ b/net/mac80211/status.c
> @@ -676,6 +676,33 @@ static void ieee80211_report_used_skb(struct ieee80211_local *local,
>         if (dropped)
>                 acked = false;
>
> +       if (info->status.tx_time_est) {
> +               struct ieee80211_sub_if_data *sdata;
> +               struct sta_info *sta = NULL;
> +               u8 *qc, ac;
> +               int tid;
> +
> +               rcu_read_lock();
> +
> +               sdata = ieee80211_sdata_from_skb(local, skb);
> +               if (sdata)
> +                       sta = sta_info_get_bss(sdata, skb_mac_header(skb));
> +
> +               if (ieee80211_is_data_qos(hdr->frame_control)) {
> +                       qc = ieee80211_get_qos_ctl(hdr);
> +                       tid = qc[0] & 0xf;
> +                       ac = ieee80211_ac_from_tid(tid);
> +               } else {
> +                       ac = IEEE80211_AC_BE;
> +               }
> +
> +               ieee80211_sta_update_pending_airtime(local, sta, ac,
> +                                                    info->status.tx_time_est,
> +                                                    true);
> +               rcu_read_unlock();
> +
> +       }
> +
>         if (info->flags & IEEE80211_TX_INTFL_MLME_CONN_TX) {
>                 struct ieee80211_sub_if_data *sdata;
>
> @@ -930,6 +957,17 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
>                         tid = qc[0] & 0xf;
>                 }
>
> +               if (info->status.tx_time_est) {
> +                       /* Do this here to avoid the expensive lookup of the sta
> +                        * in ieee80211_report_used_skb().
> +                        */
> +                       ieee80211_sta_update_pending_airtime(local, sta,
> +                                                            ieee80211_ac_from_tid(tid),
> +                                                            info->status.tx_time_est,
> +                                                            true);
> +                       info->status.tx_time_est = 0;
> +               }
> +
>                 if (!acked && ieee80211_is_back_req(fc)) {
>                         u16 control;
>
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 405f622b3fe0..b6b47171b340 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3539,9 +3539,14 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
>         struct ieee80211_tx_data tx;
>         ieee80211_tx_result r;
>         struct ieee80211_vif *vif = txq->vif;
> +       u8 ac = txq->ac;
> +       u32 airtime;
>
>         WARN_ON_ONCE(softirq_count() == 0);
>
> +       if (!ieee80211_txq_airtime_check(hw, txq))
> +               return NULL;
> +
>  begin:
>         spin_lock_bh(&fq->lock);
>
> @@ -3652,6 +3657,17 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
>         }
>
>         IEEE80211_SKB_CB(skb)->control.vif = vif;
> +
> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
> +                                                            skb->len + 38);
> +               if (airtime) {
> +                       info->control.tx_time_est = airtime;
> +                       ieee80211_sta_update_pending_airtime(local, tx.sta, ac,
> +                                                            airtime, false);
> +               }
> +       }
> +
>         return skb;
>
>  out:
>

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

[Kan Yan]

Hi Toke,

Thanks for getting this done! I will give it a try in the next few
days.  A few comments:

> The estimated airtime for each skb is stored in the tx_info, so we can
> subtract the same amount from the running total when the skb is freed or
> recycled.

Looks like ath10k driver zero out the info->status before calling
ieee80211_tx_status(...):
int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
                         const struct htt_tx_done *tx_done)
{
 ...
        info = IEEE80211_SKB_CB(msdu);
        memset(&info->status, 0, sizeof(info->status));
...
        ieee80211_tx_status(htt->ar->hw, msdu);
}

We need either restore the info->status.tx_time_est or calling
ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
get erased.

> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
> +                                                            skb->len + 38);

I think it is better to put the "+  38" that takes care of the header
overhead inside ieee80211_calc_expected_tx_airtime().

Kan


On Tue, Oct 15, 2019 at 10:19 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> The previous commit added the ability to throttle stations when they queue
> too much airtime in the hardware. This commit enables the functionality by
> calculating the expected airtime usage of each packet that is dequeued from
> the TXQs in mac80211, and accounting that as pending airtime.
>
> The estimated airtime for each skb is stored in the tx_info, so we can
> subtract the same amount from the running total when the skb is freed or
> recycled. The throttling mechanism relies on this accounting to be
> accurate (i.e., that we are not freeing skbs without subtracting any
> airtime they were accounted for), so we put the subtraction into
> ieee80211_report_used_skb(). As an optimisation, we also subtract the
> airtime on regular TX completion, zeroing out the value stored in the
> packet afterwards, to avoid having to do an expensive lookup of the
> station from the packet data on every packet.
>
> This patch does *not* include any mechanism to wake a throttled TXQ again,
> on the assumption that this will happen anyway as a side effect of whatever
> freed the skb (most commonly a TX completion).
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  net/mac80211/status.c |   38 ++++++++++++++++++++++++++++++++++++++
>  net/mac80211/tx.c     |   16 ++++++++++++++++
>  2 files changed, 54 insertions(+)
>
> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
> index ab8ba5835ca0..ce990a1e9043 100644
> --- a/net/mac80211/status.c
> +++ b/net/mac80211/status.c
> @@ -676,6 +676,33 @@ static void ieee80211_report_used_skb(struct ieee80211_local *local,
>         if (dropped)
>                 acked = false;
>
> +       if (info->status.tx_time_est) {
> +               struct ieee80211_sub_if_data *sdata;
> +               struct sta_info *sta = NULL;
> +               u8 *qc, ac;
> +               int tid;
> +
> +               rcu_read_lock();
> +
> +               sdata = ieee80211_sdata_from_skb(local, skb);
> +               if (sdata)
> +                       sta = sta_info_get_bss(sdata, skb_mac_header(skb));
> +
> +               if (ieee80211_is_data_qos(hdr->frame_control)) {
> +                       qc = ieee80211_get_qos_ctl(hdr);
> +                       tid = qc[0] & 0xf;
> +                       ac = ieee80211_ac_from_tid(tid);
> +               } else {
> +                       ac = IEEE80211_AC_BE;
> +               }
> +
> +               ieee80211_sta_update_pending_airtime(local, sta, ac,
> +                                                    info->status.tx_time_est,
> +                                                    true);
> +               rcu_read_unlock();
> +
> +       }
> +
>         if (info->flags & IEEE80211_TX_INTFL_MLME_CONN_TX) {
>                 struct ieee80211_sub_if_data *sdata;
>
> @@ -930,6 +957,17 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
>                         tid = qc[0] & 0xf;
>                 }
>
> +               if (info->status.tx_time_est) {
> +                       /* Do this here to avoid the expensive lookup of the sta
> +                        * in ieee80211_report_used_skb().
> +                        */
> +                       ieee80211_sta_update_pending_airtime(local, sta,
> +                                                            ieee80211_ac_from_tid(tid),
> +                                                            info->status.tx_time_est,
> +                                                            true);
> +                       info->status.tx_time_est = 0;
> +               }
> +
>                 if (!acked && ieee80211_is_back_req(fc)) {
>                         u16 control;
>
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 405f622b3fe0..b6b47171b340 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3539,9 +3539,14 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
>         struct ieee80211_tx_data tx;
>         ieee80211_tx_result r;
>         struct ieee80211_vif *vif = txq->vif;
> +       u8 ac = txq->ac;
> +       u32 airtime;
>
>         WARN_ON_ONCE(softirq_count() == 0);
>
> +       if (!ieee80211_txq_airtime_check(hw, txq))
> +               return NULL;
> +
>  begin:
>         spin_lock_bh(&fq->lock);
>
> @@ -3652,6 +3657,17 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
>         }
>
>         IEEE80211_SKB_CB(skb)->control.vif = vif;
> +
> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
> +                                                            skb->len + 38);
> +               if (airtime) {
> +                       info->control.tx_time_est = airtime;
> +                       ieee80211_sta_update_pending_airtime(local, tx.sta, ac,
> +                                                            airtime, false);
> +               }
> +       }
> +
>         return skb;
>
>  out:
>

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Toke Høiland-Jørgensen]

Kan Yan <kyan@google.com> writes:

> Hi Toke,
>
> Thanks for getting this done! I will give it a try in the next few
> days.  A few comments:
>
>> The estimated airtime for each skb is stored in the tx_info, so we can
>> subtract the same amount from the running total when the skb is freed or
>> recycled.
>
> Looks like ath10k driver zero out the info->status before calling
> ieee80211_tx_status(...):
> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>                          const struct htt_tx_done *tx_done)
> {
>  ...
>         info = IEEE80211_SKB_CB(msdu);
>         memset(&info->status, 0, sizeof(info->status));
> ...
>         ieee80211_tx_status(htt->ar->hw, msdu);
> }

Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
it's only ath10k and iwl that do this, though, so it's probably
manageable to just fix this. I think the simplest solution is just to
restore the field after clearing, no?

> We need either restore the info->status.tx_time_est or calling
> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
> get erased.
>
>> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
>> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>> +                                                            skb->len + 38);
>
> I think it is better to put the "+  38" that takes care of the header
> overhead inside ieee80211_calc_expected_tx_airtime().

Hmm, no strong opinion about this; but yeah, since we have a dedicated
function for this use I guess there's no harm in adding it there :)

-Toke

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

[Toke Høiland-Jørgensen]

Kan Yan <kyan@google.com> writes:

> Hi Toke,
>
> Thanks for getting this done! I will give it a try in the next few
> days.  A few comments:
>
>> The estimated airtime for each skb is stored in the tx_info, so we can
>> subtract the same amount from the running total when the skb is freed or
>> recycled.
>
> Looks like ath10k driver zero out the info->status before calling
> ieee80211_tx_status(...):
> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>                          const struct htt_tx_done *tx_done)
> {
>  ...
>         info = IEEE80211_SKB_CB(msdu);
>         memset(&info->status, 0, sizeof(info->status));
> ...
>         ieee80211_tx_status(htt->ar->hw, msdu);
> }

Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
it's only ath10k and iwl that do this, though, so it's probably
manageable to just fix this. I think the simplest solution is just to
restore the field after clearing, no?

> We need either restore the info->status.tx_time_est or calling
> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
> get erased.
>
>> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
>> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>> +                                                            skb->len + 38);
>
> I think it is better to put the "+  38" that takes care of the header
> overhead inside ieee80211_calc_expected_tx_airtime().

Hmm, no strong opinion about this; but yeah, since we have a dedicated
function for this use I guess there's no harm in adding it there :)

-Toke


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Sebastian Moeller]

> On Oct 17, 2019, at 11:44, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
> Kan Yan <kyan@google.com> writes:
> 
>> Hi Toke,
>> 
>> Thanks for getting this done! I will give it a try in the next few
>> days.  A few comments:
>> 
>>> The estimated airtime for each skb is stored in the tx_info, so we can
>>> subtract the same amount from the running total when the skb is freed or
>>> recycled.
>> 
>> Looks like ath10k driver zero out the info->status before calling
>> ieee80211_tx_status(...):
>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>                         const struct htt_tx_done *tx_done)
>> {
>> ...
>>        info = IEEE80211_SKB_CB(msdu);
>>        memset(&info->status, 0, sizeof(info->status));
>> ...
>>        ieee80211_tx_status(htt->ar->hw, msdu);
>> }
> 
> Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
> it's only ath10k and iwl that do this, though, so it's probably
> manageable to just fix this. I think the simplest solution is just to
> restore the field after clearing, no?
> 
>> We need either restore the info->status.tx_time_est or calling
>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
>> get erased.
>> 
>>> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
>>> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>>> +                                                            skb->len + 38);
>> 
>> I think it is better to put the "+  38" that takes care of the header
>> overhead inside ieee80211_calc_expected_tx_airtime().
> 
> Hmm, no strong opinion about this; but yeah, since we have a dedicated
> function for this use I guess there's no harm in adding it there :)
> 

Silly question, is this Overhead guaranteed to be 38 Bytes for all eternity? Otherwise a variable or a preprocessor constant might be more future proof?

Best Regards
	Sebastian


> -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 v2 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

[Sebastian Moeller]

> On Oct 17, 2019, at 11:44, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
> Kan Yan <kyan@google.com> writes:
> 
>> Hi Toke,
>> 
>> Thanks for getting this done! I will give it a try in the next few
>> days.  A few comments:
>> 
>>> The estimated airtime for each skb is stored in the tx_info, so we can
>>> subtract the same amount from the running total when the skb is freed or
>>> recycled.
>> 
>> Looks like ath10k driver zero out the info->status before calling
>> ieee80211_tx_status(...):
>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>                         const struct htt_tx_done *tx_done)
>> {
>> ...
>>        info = IEEE80211_SKB_CB(msdu);
>>        memset(&info->status, 0, sizeof(info->status));
>> ...
>>        ieee80211_tx_status(htt->ar->hw, msdu);
>> }
> 
> Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
> it's only ath10k and iwl that do this, though, so it's probably
> manageable to just fix this. I think the simplest solution is just to
> restore the field after clearing, no?
> 
>> We need either restore the info->status.tx_time_est or calling
>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
>> get erased.
>> 
>>> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
>>> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>>> +                                                            skb->len + 38);
>> 
>> I think it is better to put the "+  38" that takes care of the header
>> overhead inside ieee80211_calc_expected_tx_airtime().
> 
> Hmm, no strong opinion about this; but yeah, since we have a dedicated
> function for this use I guess there's no harm in adding it there :)
> 

Silly question, is this Overhead guaranteed to be 38 Bytes for all eternity? Otherwise a variable or a preprocessor constant might be more future proof?

Best Regards
	Sebastian


> -Toke
> 
> _______________________________________________
> Make-wifi-fast mailing list
> Make-wifi-fast@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/make-wifi-fast


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Toke Høiland-Jørgensen]

Sebastian Moeller <moeller0@gmx.de> writes:

>> On Oct 17, 2019, at 11:44, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> 
>> Kan Yan <kyan@google.com> writes:
>> 
>>> Hi Toke,
>>> 
>>> Thanks for getting this done! I will give it a try in the next few
>>> days.  A few comments:
>>> 
>>>> The estimated airtime for each skb is stored in the tx_info, so we can
>>>> subtract the same amount from the running total when the skb is freed or
>>>> recycled.
>>> 
>>> Looks like ath10k driver zero out the info->status before calling
>>> ieee80211_tx_status(...):
>>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>>                         const struct htt_tx_done *tx_done)
>>> {
>>> ...
>>>        info = IEEE80211_SKB_CB(msdu);
>>>        memset(&info->status, 0, sizeof(info->status));
>>> ...
>>>        ieee80211_tx_status(htt->ar->hw, msdu);
>>> }
>> 
>> Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
>> it's only ath10k and iwl that do this, though, so it's probably
>> manageable to just fix this. I think the simplest solution is just to
>> restore the field after clearing, no?
>> 
>>> We need either restore the info->status.tx_time_est or calling
>>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
>>> get erased.
>>> 
>>>> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
>>>> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>>>> +                                                            skb->len + 38);
>>> 
>>> I think it is better to put the "+  38" that takes care of the header
>>> overhead inside ieee80211_calc_expected_tx_airtime().
>> 
>> Hmm, no strong opinion about this; but yeah, since we have a dedicated
>> function for this use I guess there's no harm in adding it there :)
>> 
>
> Silly question, is this Overhead guaranteed to be 38 Bytes for all
> eternity? Otherwise a variable or a preprocessor constant might be
> more future proof?

Well, yeah, as long as we're sending Ethernet packets. Which is kinda
baked into the WiFi standard :)

-Toke

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

[Toke Høiland-Jørgensen]

Sebastian Moeller <moeller0@gmx.de> writes:

>> On Oct 17, 2019, at 11:44, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> 
>> Kan Yan <kyan@google.com> writes:
>> 
>>> Hi Toke,
>>> 
>>> Thanks for getting this done! I will give it a try in the next few
>>> days.  A few comments:
>>> 
>>>> The estimated airtime for each skb is stored in the tx_info, so we can
>>>> subtract the same amount from the running total when the skb is freed or
>>>> recycled.
>>> 
>>> Looks like ath10k driver zero out the info->status before calling
>>> ieee80211_tx_status(...):
>>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>>                         const struct htt_tx_done *tx_done)
>>> {
>>> ...
>>>        info = IEEE80211_SKB_CB(msdu);
>>>        memset(&info->status, 0, sizeof(info->status));
>>> ...
>>>        ieee80211_tx_status(htt->ar->hw, msdu);
>>> }
>> 
>> Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
>> it's only ath10k and iwl that do this, though, so it's probably
>> manageable to just fix this. I think the simplest solution is just to
>> restore the field after clearing, no?
>> 
>>> We need either restore the info->status.tx_time_est or calling
>>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
>>> get erased.
>>> 
>>>> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
>>>> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>>>> +                                                            skb->len + 38);
>>> 
>>> I think it is better to put the "+  38" that takes care of the header
>>> overhead inside ieee80211_calc_expected_tx_airtime().
>> 
>> Hmm, no strong opinion about this; but yeah, since we have a dedicated
>> function for this use I guess there's no harm in adding it there :)
>> 
>
> Silly question, is this Overhead guaranteed to be 38 Bytes for all
> eternity? Otherwise a variable or a preprocessor constant might be
> more future proof?

Well, yeah, as long as we're sending Ethernet packets. Which is kinda
baked into the WiFi standard :)

-Toke


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Sebastian Moeller]

What about VLAN tags?

Best Regards
	Sebastian

> On Oct 17, 2019, at 12:24, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
> Sebastian Moeller <moeller0@gmx.de> writes:
> 
>>> On Oct 17, 2019, at 11:44, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>> 
>>> Kan Yan <kyan@google.com> writes:
>>> 
>>>> Hi Toke,
>>>> 
>>>> Thanks for getting this done! I will give it a try in the next few
>>>> days.  A few comments:
>>>> 
>>>>> The estimated airtime for each skb is stored in the tx_info, so we can
>>>>> subtract the same amount from the running total when the skb is freed or
>>>>> recycled.
>>>> 
>>>> Looks like ath10k driver zero out the info->status before calling
>>>> ieee80211_tx_status(...):
>>>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>>>                        const struct htt_tx_done *tx_done)
>>>> {
>>>> ...
>>>>       info = IEEE80211_SKB_CB(msdu);
>>>>       memset(&info->status, 0, sizeof(info->status));
>>>> ...
>>>>       ieee80211_tx_status(htt->ar->hw, msdu);
>>>> }
>>> 
>>> Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
>>> it's only ath10k and iwl that do this, though, so it's probably
>>> manageable to just fix this. I think the simplest solution is just to
>>> restore the field after clearing, no?
>>> 
>>>> We need either restore the info->status.tx_time_est or calling
>>>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
>>>> get erased.
>>>> 
>>>>> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
>>>>> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>>>>> +                                                            skb->len + 38);
>>>> 
>>>> I think it is better to put the "+  38" that takes care of the header
>>>> overhead inside ieee80211_calc_expected_tx_airtime().
>>> 
>>> Hmm, no strong opinion about this; but yeah, since we have a dedicated
>>> function for this use I guess there's no harm in adding it there :)
>>> 
>> 
>> Silly question, is this Overhead guaranteed to be 38 Bytes for all
>> eternity? Otherwise a variable or a preprocessor constant might be
>> more future proof?
> 
> Well, yeah, as long as we're sending Ethernet packets. Which is kinda
> baked into the WiFi standard :)
> 
> -Toke

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

[Sebastian Moeller]

What about VLAN tags?

Best Regards
	Sebastian

> On Oct 17, 2019, at 12:24, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
> Sebastian Moeller <moeller0@gmx.de> writes:
> 
>>> On Oct 17, 2019, at 11:44, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>> 
>>> Kan Yan <kyan@google.com> writes:
>>> 
>>>> Hi Toke,
>>>> 
>>>> Thanks for getting this done! I will give it a try in the next few
>>>> days.  A few comments:
>>>> 
>>>>> The estimated airtime for each skb is stored in the tx_info, so we can
>>>>> subtract the same amount from the running total when the skb is freed or
>>>>> recycled.
>>>> 
>>>> Looks like ath10k driver zero out the info->status before calling
>>>> ieee80211_tx_status(...):
>>>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>>>                        const struct htt_tx_done *tx_done)
>>>> {
>>>> ...
>>>>       info = IEEE80211_SKB_CB(msdu);
>>>>       memset(&info->status, 0, sizeof(info->status));
>>>> ...
>>>>       ieee80211_tx_status(htt->ar->hw, msdu);
>>>> }
>>> 
>>> Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
>>> it's only ath10k and iwl that do this, though, so it's probably
>>> manageable to just fix this. I think the simplest solution is just to
>>> restore the field after clearing, no?
>>> 
>>>> We need either restore the info->status.tx_time_est or calling
>>>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
>>>> get erased.
>>>> 
>>>>> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
>>>>> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>>>>> +                                                            skb->len + 38);
>>>> 
>>>> I think it is better to put the "+  38" that takes care of the header
>>>> overhead inside ieee80211_calc_expected_tx_airtime().
>>> 
>>> Hmm, no strong opinion about this; but yeah, since we have a dedicated
>>> function for this use I guess there's no harm in adding it there :)
>>> 
>> 
>> Silly question, is this Overhead guaranteed to be 38 Bytes for all
>> eternity? Otherwise a variable or a preprocessor constant might be
>> more future proof?
> 
> Well, yeah, as long as we're sending Ethernet packets. Which is kinda
> baked into the WiFi standard :)
> 
> -Toke


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Kan Yan]

The "tx_time_est" field, shared by control and status, is not able to
survive until the skb returns to the mac80211 layer in some
architectures. The same space is defined as driver_data and some
wireless drivers use it for other purposes, as the cb in the sk_buff
is free to be used by any layer.

In the case of ath10k, the tx_time_est get clobbered by
struct ath10k_skb_cb {
        dma_addr_t paddr;
        u8 flags;
        u8 eid;
        u16 msdu_id;
        u16 airtime_est;
        struct ieee80211_vif *vif;
        struct ieee80211_txq *txq;
} __packed;

Do you think shrink driver_data by 2 bytes and use that space for
tx_time_est to make it persistent across mac80211 and wireless driver
layer an acceptable solution?

Kan




On Tue, Oct 15, 2019 at 10:19 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> To implement airtime queue limiting, we need to keep a running account of
> the estimated airtime of all skbs queued into the device. Do to this
> correctly, we need to store the airtime estimate into the skb so we can
> decrease the outstanding balance when the skb is freed. This means that the
> time estimate must be stored somewhere that will survive for the lifetime
> of the skb.
>
> Fortunately, we had a couple of bytes left in the 'status' field in the
> ieee80211_tx_info; and since we only plan to calculate the airtime estimate
> after the skb is dequeued from the FQ structure, on the control side we can
> share the space with the codel enqueue time. And by rearranging the order
> of elements it is possible to have the position of the new tx_time_est line
> up between the control and status structs, so the value will survive from
> when mac80211 hands the packet to the driver, and until the driver either
> frees it, or hands it back through TX status.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  include/net/mac80211.h |   26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index d69081c38788..49f8ea0af5f8 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -975,20 +975,23 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate)
>   * @control.short_preamble: use short preamble (CCK only)
>   * @control.skip_table: skip externally configured rate table
>   * @control.jiffies: timestamp for expiry on powersave clients
> + * @control.enqueue_time: enqueue time (for iTXQs)
> + * @control.tx_time_est: estimated airtime usage (shared with @status)
> + * @control.reserved: unused field to ensure alignment of data structure
> + * @control.flags: control flags, see &enum mac80211_tx_control_flags
>   * @control.vif: virtual interface (may be NULL)
>   * @control.hw_key: key to encrypt with (may be NULL)
> - * @control.flags: control flags, see &enum mac80211_tx_control_flags
> - * @control.enqueue_time: enqueue time (for iTXQs)
>   * @driver_rates: alias to @control.rates to reserve space
>   * @pad: padding
>   * @rate_driver_data: driver use area if driver needs @control.rates
>   * @status: union part for status data
>   * @status.rates: attempted rates
>   * @status.ack_signal: ACK signal
> + * @status.tx_time_est: estimated airtime of skb (shared with @control)
> + * @status.tx_time: actual airtime consumed for transmission
>   * @status.ampdu_ack_len: AMPDU ack length
>   * @status.ampdu_len: AMPDU length
>   * @status.antenna: (legacy, kept only for iwlegacy)
> - * @status.tx_time: airtime consumed for transmission
>   * @status.is_valid_ack_signal: ACK signal is valid
>   * @status.status_driver_data: driver use area
>   * @ack: union part for pure ACK data
> @@ -1026,11 +1029,17 @@ struct ieee80211_tx_info {
>                                 /* only needed before rate control */
>                                 unsigned long jiffies;
>                         };
> +                       union {
> +                               codel_time_t enqueue_time;
> +                               struct {
> +                                       u16 tx_time_est; /* shared with status */
> +                                       u16 reserved; /* padding for alignment */
> +                               };
> +                       };
> +                       u32 flags;
>                         /* NB: vif can be NULL for injected frames */
>                         struct ieee80211_vif *vif;
>                         struct ieee80211_key_conf *hw_key;
> -                       u32 flags;
> -                       codel_time_t enqueue_time;
>                 } control;
>                 struct {
>                         u64 cookie;
> @@ -1038,12 +1047,13 @@ struct ieee80211_tx_info {
>                 struct {
>                         struct ieee80211_tx_rate rates[IEEE80211_TX_MAX_RATES];
>                         s32 ack_signal;
> +                       u16 tx_time_est; /* shared with control */
> +                       u16 tx_time;
>                         u8 ampdu_ack_len;
>                         u8 ampdu_len;
>                         u8 antenna;
> -                       u16 tx_time;
>                         bool is_valid_ack_signal;
> -                       void *status_driver_data[19 / sizeof(void *)];
> +                       void *status_driver_data[16 / sizeof(void *)];
>                 } status;
>                 struct {
>                         struct ieee80211_tx_rate driver_rates[
> @@ -1126,6 +1136,8 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
>                      offsetof(struct ieee80211_tx_info, control.rates));
>         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
>                      offsetof(struct ieee80211_tx_info, driver_rates));
> +       BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, control.tx_time_est) !=
> +                    offsetof(struct ieee80211_tx_info, status.tx_time_est));
>         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8);
>         /* clear the rate counts */
>         for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
>

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Kan Yan]

The "tx_time_est" field, shared by control and status, is not able to
survive until the skb returns to the mac80211 layer in some
architectures. The same space is defined as driver_data and some
wireless drivers use it for other purposes, as the cb in the sk_buff
is free to be used by any layer.

In the case of ath10k, the tx_time_est get clobbered by
struct ath10k_skb_cb {
        dma_addr_t paddr;
        u8 flags;
        u8 eid;
        u16 msdu_id;
        u16 airtime_est;
        struct ieee80211_vif *vif;
        struct ieee80211_txq *txq;
} __packed;

Do you think shrink driver_data by 2 bytes and use that space for
tx_time_est to make it persistent across mac80211 and wireless driver
layer an acceptable solution?

Kan




On Tue, Oct 15, 2019 at 10:19 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> To implement airtime queue limiting, we need to keep a running account of
> the estimated airtime of all skbs queued into the device. Do to this
> correctly, we need to store the airtime estimate into the skb so we can
> decrease the outstanding balance when the skb is freed. This means that the
> time estimate must be stored somewhere that will survive for the lifetime
> of the skb.
>
> Fortunately, we had a couple of bytes left in the 'status' field in the
> ieee80211_tx_info; and since we only plan to calculate the airtime estimate
> after the skb is dequeued from the FQ structure, on the control side we can
> share the space with the codel enqueue time. And by rearranging the order
> of elements it is possible to have the position of the new tx_time_est line
> up between the control and status structs, so the value will survive from
> when mac80211 hands the packet to the driver, and until the driver either
> frees it, or hands it back through TX status.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  include/net/mac80211.h |   26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index d69081c38788..49f8ea0af5f8 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -975,20 +975,23 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate)
>   * @control.short_preamble: use short preamble (CCK only)
>   * @control.skip_table: skip externally configured rate table
>   * @control.jiffies: timestamp for expiry on powersave clients
> + * @control.enqueue_time: enqueue time (for iTXQs)
> + * @control.tx_time_est: estimated airtime usage (shared with @status)
> + * @control.reserved: unused field to ensure alignment of data structure
> + * @control.flags: control flags, see &enum mac80211_tx_control_flags
>   * @control.vif: virtual interface (may be NULL)
>   * @control.hw_key: key to encrypt with (may be NULL)
> - * @control.flags: control flags, see &enum mac80211_tx_control_flags
> - * @control.enqueue_time: enqueue time (for iTXQs)
>   * @driver_rates: alias to @control.rates to reserve space
>   * @pad: padding
>   * @rate_driver_data: driver use area if driver needs @control.rates
>   * @status: union part for status data
>   * @status.rates: attempted rates
>   * @status.ack_signal: ACK signal
> + * @status.tx_time_est: estimated airtime of skb (shared with @control)
> + * @status.tx_time: actual airtime consumed for transmission
>   * @status.ampdu_ack_len: AMPDU ack length
>   * @status.ampdu_len: AMPDU length
>   * @status.antenna: (legacy, kept only for iwlegacy)
> - * @status.tx_time: airtime consumed for transmission
>   * @status.is_valid_ack_signal: ACK signal is valid
>   * @status.status_driver_data: driver use area
>   * @ack: union part for pure ACK data
> @@ -1026,11 +1029,17 @@ struct ieee80211_tx_info {
>                                 /* only needed before rate control */
>                                 unsigned long jiffies;
>                         };
> +                       union {
> +                               codel_time_t enqueue_time;
> +                               struct {
> +                                       u16 tx_time_est; /* shared with status */
> +                                       u16 reserved; /* padding for alignment */
> +                               };
> +                       };
> +                       u32 flags;
>                         /* NB: vif can be NULL for injected frames */
>                         struct ieee80211_vif *vif;
>                         struct ieee80211_key_conf *hw_key;
> -                       u32 flags;
> -                       codel_time_t enqueue_time;
>                 } control;
>                 struct {
>                         u64 cookie;
> @@ -1038,12 +1047,13 @@ struct ieee80211_tx_info {
>                 struct {
>                         struct ieee80211_tx_rate rates[IEEE80211_TX_MAX_RATES];
>                         s32 ack_signal;
> +                       u16 tx_time_est; /* shared with control */
> +                       u16 tx_time;
>                         u8 ampdu_ack_len;
>                         u8 ampdu_len;
>                         u8 antenna;
> -                       u16 tx_time;
>                         bool is_valid_ack_signal;
> -                       void *status_driver_data[19 / sizeof(void *)];
> +                       void *status_driver_data[16 / sizeof(void *)];
>                 } status;
>                 struct {
>                         struct ieee80211_tx_rate driver_rates[
> @@ -1126,6 +1136,8 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
>                      offsetof(struct ieee80211_tx_info, control.rates));
>         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
>                      offsetof(struct ieee80211_tx_info, driver_rates));
> +       BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, control.tx_time_est) !=
> +                    offsetof(struct ieee80211_tx_info, status.tx_time_est));
>         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8);
>         /* clear the rate counts */
>         for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
>

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Kan Yan]

I don't think it is hard to take care of extra header size for frames
with VLAN tags, but the question is how far are we willing to go down
this rabbit hole. There are other factors like retries and aggregation
that are difficult to be taken into account. However, I doubt that
matters, a ballpark estimate of airtime is sufficient for the purpose
of implementing a queue limit.

On Thu, Oct 17, 2019 at 3:25 AM Sebastian Moeller <moeller0@gmx.de> wrote:
>
> What about VLAN tags?
>
> Best Regards
>         Sebastian
>
> > On Oct 17, 2019, at 12:24, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >
> > Sebastian Moeller <moeller0@gmx.de> writes:
> >
> >>> On Oct 17, 2019, at 11:44, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>>
> >>> Kan Yan <kyan@google.com> writes:
> >>>
> >>>> Hi Toke,
> >>>>
> >>>> Thanks for getting this done! I will give it a try in the next few
> >>>> days.  A few comments:
> >>>>
> >>>>> The estimated airtime for each skb is stored in the tx_info, so we can
> >>>>> subtract the same amount from the running total when the skb is freed or
> >>>>> recycled.
> >>>>
> >>>> Looks like ath10k driver zero out the info->status before calling
> >>>> ieee80211_tx_status(...):
> >>>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
> >>>>                        const struct htt_tx_done *tx_done)
> >>>> {
> >>>> ...
> >>>>       info = IEEE80211_SKB_CB(msdu);
> >>>>       memset(&info->status, 0, sizeof(info->status));
> >>>> ...
> >>>>       ieee80211_tx_status(htt->ar->hw, msdu);
> >>>> }
> >>>
> >>> Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
> >>> it's only ath10k and iwl that do this, though, so it's probably
> >>> manageable to just fix this. I think the simplest solution is just to
> >>> restore the field after clearing, no?
> >>>
> >>>> We need either restore the info->status.tx_time_est or calling
> >>>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
> >>>> get erased.
> >>>>
> >>>>> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
> >>>>> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
> >>>>> +                                                            skb->len + 38);
> >>>>
> >>>> I think it is better to put the "+  38" that takes care of the header
> >>>> overhead inside ieee80211_calc_expected_tx_airtime().
> >>>
> >>> Hmm, no strong opinion about this; but yeah, since we have a dedicated
> >>> function for this use I guess there's no harm in adding it there :)
> >>>
> >>
> >> Silly question, is this Overhead guaranteed to be 38 Bytes for all
> >> eternity? Otherwise a variable or a preprocessor constant might be
> >> more future proof?
> >
> > Well, yeah, as long as we're sending Ethernet packets. Which is kinda
> > baked into the WiFi standard :)
> >
> > -Toke
>

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

[Kan Yan]

I don't think it is hard to take care of extra header size for frames
with VLAN tags, but the question is how far are we willing to go down
this rabbit hole. There are other factors like retries and aggregation
that are difficult to be taken into account. However, I doubt that
matters, a ballpark estimate of airtime is sufficient for the purpose
of implementing a queue limit.

On Thu, Oct 17, 2019 at 3:25 AM Sebastian Moeller <moeller0@gmx.de> wrote:
>
> What about VLAN tags?
>
> Best Regards
>         Sebastian
>
> > On Oct 17, 2019, at 12:24, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >
> > Sebastian Moeller <moeller0@gmx.de> writes:
> >
> >>> On Oct 17, 2019, at 11:44, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>>
> >>> Kan Yan <kyan@google.com> writes:
> >>>
> >>>> Hi Toke,
> >>>>
> >>>> Thanks for getting this done! I will give it a try in the next few
> >>>> days.  A few comments:
> >>>>
> >>>>> The estimated airtime for each skb is stored in the tx_info, so we can
> >>>>> subtract the same amount from the running total when the skb is freed or
> >>>>> recycled.
> >>>>
> >>>> Looks like ath10k driver zero out the info->status before calling
> >>>> ieee80211_tx_status(...):
> >>>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
> >>>>                        const struct htt_tx_done *tx_done)
> >>>> {
> >>>> ...
> >>>>       info = IEEE80211_SKB_CB(msdu);
> >>>>       memset(&info->status, 0, sizeof(info->status));
> >>>> ...
> >>>>       ieee80211_tx_status(htt->ar->hw, msdu);
> >>>> }
> >>>
> >>> Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
> >>> it's only ath10k and iwl that do this, though, so it's probably
> >>> manageable to just fix this. I think the simplest solution is just to
> >>> restore the field after clearing, no?
> >>>
> >>>> We need either restore the info->status.tx_time_est or calling
> >>>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
> >>>> get erased.
> >>>>
> >>>>> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
> >>>>> +               airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
> >>>>> +                                                            skb->len + 38);
> >>>>
> >>>> I think it is better to put the "+  38" that takes care of the header
> >>>> overhead inside ieee80211_calc_expected_tx_airtime().
> >>>
> >>> Hmm, no strong opinion about this; but yeah, since we have a dedicated
> >>> function for this use I guess there's no harm in adding it there :)
> >>>
> >>
> >> Silly question, is this Overhead guaranteed to be 38 Bytes for all
> >> eternity? Otherwise a variable or a preprocessor constant might be
> >> more future proof?
> >
> > Well, yeah, as long as we're sending Ethernet packets. Which is kinda
> > baked into the WiFi standard :)
> >
> > -Toke
>

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

Kan Yan <kyan@google.com> writes:

> The "tx_time_est" field, shared by control and status, is not able to
> survive until the skb returns to the mac80211 layer in some
> architectures. The same space is defined as driver_data and some
> wireless drivers use it for other purposes, as the cb in the sk_buff
> is free to be used by any layer.
>
> In the case of ath10k, the tx_time_est get clobbered by
> struct ath10k_skb_cb {
>         dma_addr_t paddr;
>         u8 flags;
>         u8 eid;
>         u16 msdu_id;
>         u16 airtime_est;
>         struct ieee80211_vif *vif;
>         struct ieee80211_txq *txq;
> } __packed;

Ah, bugger, of course the driver that actually needs this is using the
full driver_data space :P

> Do you think shrink driver_data by 2 bytes and use that space for
> tx_time_est to make it persistent across mac80211 and wireless driver
> layer an acceptable solution?

Hmm, the driver_data field is defined as an array of pointers, so we can
only shrink it in increments of sizeof(void *). I think it may be
feasible to shrink it (as in, I don't think any drivers are actually
using the full 40 bytes), but doing this in a way that will gain us a
2-byte space that is also usable in the case driver_data is *not* used
(i.e., it needs be able to align with a field in .control and .status as
well) would require some serious surgery of the whole ieee80211_tx_info...

However, there's a nice juicy 'u16 ack_frame_id' at the start of
ieee80211_tx_info. Could we potentially use that? We could use the top
bit as a disambiguation flag; I think we're fine with 15 bits for the TX
time itself (a single packet won't exceed 8ms or TX time), so if we can
live with 15 bits of ACK frame ID space, that could be a way forward?

Johannes, what do you think?

-Toke

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

Kan Yan <kyan@google.com> writes:

> The "tx_time_est" field, shared by control and status, is not able to
> survive until the skb returns to the mac80211 layer in some
> architectures. The same space is defined as driver_data and some
> wireless drivers use it for other purposes, as the cb in the sk_buff
> is free to be used by any layer.
>
> In the case of ath10k, the tx_time_est get clobbered by
> struct ath10k_skb_cb {
>         dma_addr_t paddr;
>         u8 flags;
>         u8 eid;
>         u16 msdu_id;
>         u16 airtime_est;
>         struct ieee80211_vif *vif;
>         struct ieee80211_txq *txq;
> } __packed;

Ah, bugger, of course the driver that actually needs this is using the
full driver_data space :P

> Do you think shrink driver_data by 2 bytes and use that space for
> tx_time_est to make it persistent across mac80211 and wireless driver
> layer an acceptable solution?

Hmm, the driver_data field is defined as an array of pointers, so we can
only shrink it in increments of sizeof(void *). I think it may be
feasible to shrink it (as in, I don't think any drivers are actually
using the full 40 bytes), but doing this in a way that will gain us a
2-byte space that is also usable in the case driver_data is *not* used
(i.e., it needs be able to align with a field in .control and .status as
well) would require some serious surgery of the whole ieee80211_tx_info...

However, there's a nice juicy 'u16 ack_frame_id' at the start of
ieee80211_tx_info. Could we potentially use that? We could use the top
bit as a disambiguation flag; I think we're fine with 15 bits for the TX
time itself (a single packet won't exceed 8ms or TX time), so if we can
live with 15 bits of ACK frame ID space, that could be a way forward?

Johannes, what do you think?

-Toke


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Johannes Berg]

On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote:

> However, there's a nice juicy 'u16 ack_frame_id' at the start of
> ieee80211_tx_info. Could we potentially use that? We could use the top
> bit as a disambiguation flag; I think we're fine with 15 bits for the TX
> time itself (a single packet won't exceed 8ms or TX time), so if we can
> live with 15 bits of ACK frame ID space, that could be a way forward?

I was going to say that should work as we only ever have a handful of
ACK frame IDs, but ... you still need the airtime even for a frame that
userspace wants to know the ACK status of, no?

We could pull the ack_frame_id out-of-line using the skb extensions
framework, but I'm not sure we should allocate one of the possible 8
extension IDs for that either ...

What we really should do is convert all (relevant) drivers to use rate
tables instead of having all the rates in the TX info, then we'd get a
lot of space, but that's a lot of work ...

johannes

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Johannes Berg]

On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote:

> However, there's a nice juicy 'u16 ack_frame_id' at the start of
> ieee80211_tx_info. Could we potentially use that? We could use the top
> bit as a disambiguation flag; I think we're fine with 15 bits for the TX
> time itself (a single packet won't exceed 8ms or TX time), so if we can
> live with 15 bits of ACK frame ID space, that could be a way forward?

I was going to say that should work as we only ever have a handful of
ACK frame IDs, but ... you still need the airtime even for a frame that
userspace wants to know the ACK status of, no?

We could pull the ack_frame_id out-of-line using the skb extensions
framework, but I'm not sure we should allocate one of the possible 8
extension IDs for that either ...

What we really should do is convert all (relevant) drivers to use rate
tables instead of having all the rates in the TX info, then we'd get a
lot of space, but that's a lot of work ...

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Johannes Berg]

On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
> Kan Yan <kyan@google.com> writes:
> 
> > The "tx_time_est" field, shared by control and status, is not able to
> > survive until the skb returns to the mac80211 layer in some
> > architectures. The same space is defined as driver_data and some
> > wireless drivers use it for other purposes, as the cb in the sk_buff
> > is free to be used by any layer.
> > 
> > In the case of ath10k, the tx_time_est get clobbered by
> > struct ath10k_skb_cb {
> >         dma_addr_t paddr;
> >         u8 flags;
> >         u8 eid;
> >         u16 msdu_id;
> >         u16 airtime_est;
> >         struct ieee80211_vif *vif;
> >         struct ieee80211_txq *txq;
> > } __packed;
> 
> Ah, bugger, of course the driver that actually needs this is using the
> full driver_data space :P

Looks like you could shrink *this* fairly easily though.

E.g. most likely vif == txq->vif unless txq==NULL, so it's down to 22
bytes plus a bit/flag for knowing whether the pointer is a vif directly
(if no TXQ) or a TXQ.

> > Do you think shrink driver_data by 2 bytes and use that space for
> > tx_time_est to make it persistent across mac80211 and wireless driver
> > layer an acceptable solution?
> 
> Hmm, the driver_data field is defined as an array of pointers, so we can
> only shrink it in increments of sizeof(void *). I think it may be
> feasible to shrink it (as in, I don't think any drivers are actually
> using the full 40 bytes),

It doesn't have to be defined like that, just was most convenient as
driers were using pointers there.

> but doing this in a way that will gain us a
> 2-byte space that is also usable in the case driver_data is *not* used
> (i.e., it needs be able to align with a field in .control and .status as
> well) would require some serious surgery of the whole ieee80211_tx_info...

Yeah, good point, this doesn't help at all ...

johannes

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Johannes Berg]

On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
> Kan Yan <kyan@google.com> writes:
> 
> > The "tx_time_est" field, shared by control and status, is not able to
> > survive until the skb returns to the mac80211 layer in some
> > architectures. The same space is defined as driver_data and some
> > wireless drivers use it for other purposes, as the cb in the sk_buff
> > is free to be used by any layer.
> > 
> > In the case of ath10k, the tx_time_est get clobbered by
> > struct ath10k_skb_cb {
> >         dma_addr_t paddr;
> >         u8 flags;
> >         u8 eid;
> >         u16 msdu_id;
> >         u16 airtime_est;
> >         struct ieee80211_vif *vif;
> >         struct ieee80211_txq *txq;
> > } __packed;
> 
> Ah, bugger, of course the driver that actually needs this is using the
> full driver_data space :P

Looks like you could shrink *this* fairly easily though.

E.g. most likely vif == txq->vif unless txq==NULL, so it's down to 22
bytes plus a bit/flag for knowing whether the pointer is a vif directly
(if no TXQ) or a TXQ.

> > Do you think shrink driver_data by 2 bytes and use that space for
> > tx_time_est to make it persistent across mac80211 and wireless driver
> > layer an acceptable solution?
> 
> Hmm, the driver_data field is defined as an array of pointers, so we can
> only shrink it in increments of sizeof(void *). I think it may be
> feasible to shrink it (as in, I don't think any drivers are actually
> using the full 40 bytes),

It doesn't have to be defined like that, just was most convenient as
driers were using pointers there.

> but doing this in a way that will gain us a
> 2-byte space that is also usable in the case driver_data is *not* used
> (i.e., it needs be able to align with a field in .control and .status as
> well) would require some serious surgery of the whole ieee80211_tx_info...

Yeah, good point, this doesn't help at all ...

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Ben Greear]

On 10/18/2019 05:35 AM, Johannes Berg wrote:
> On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
>> Kan Yan <kyan@google.com> writes:
>>
>>> The "tx_time_est" field, shared by control and status, is not able to
>>> survive until the skb returns to the mac80211 layer in some
>>> architectures. The same space is defined as driver_data and some
>>> wireless drivers use it for other purposes, as the cb in the sk_buff
>>> is free to be used by any layer.
>>>
>>> In the case of ath10k, the tx_time_est get clobbered by
>>> struct ath10k_skb_cb {
>>>         dma_addr_t paddr;
>>>         u8 flags;
>>>         u8 eid;
>>>         u16 msdu_id;
>>>         u16 airtime_est;
>>>         struct ieee80211_vif *vif;
>>>         struct ieee80211_txq *txq;
>>> } __packed;
>>
>> Ah, bugger, of course the driver that actually needs this is using the
>> full driver_data space :P
>
> Looks like you could shrink *this* fairly easily though.
>
> E.g. most likely vif == txq->vif unless txq==NULL, so it's down to 22
> bytes plus a bit/flag for knowing whether the pointer is a vif directly
> (if no TXQ) or a TXQ.

And of course you get two bits in every pointer (0x3) and likely the
dma addr too.  Plenty of space!

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Ben Greear]

On 10/18/2019 05:35 AM, Johannes Berg wrote:
> On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
>> Kan Yan <kyan@google.com> writes:
>>
>>> The "tx_time_est" field, shared by control and status, is not able to
>>> survive until the skb returns to the mac80211 layer in some
>>> architectures. The same space is defined as driver_data and some
>>> wireless drivers use it for other purposes, as the cb in the sk_buff
>>> is free to be used by any layer.
>>>
>>> In the case of ath10k, the tx_time_est get clobbered by
>>> struct ath10k_skb_cb {
>>>         dma_addr_t paddr;
>>>         u8 flags;
>>>         u8 eid;
>>>         u16 msdu_id;
>>>         u16 airtime_est;
>>>         struct ieee80211_vif *vif;
>>>         struct ieee80211_txq *txq;
>>> } __packed;
>>
>> Ah, bugger, of course the driver that actually needs this is using the
>> full driver_data space :P
>
> Looks like you could shrink *this* fairly easily though.
>
> E.g. most likely vif == txq->vif unless txq==NULL, so it's down to 22
> bytes plus a bit/flag for knowing whether the pointer is a vif directly
> (if no TXQ) or a TXQ.

And of course you get two bits in every pointer (0x3) and likely the
dma addr too.  Plenty of space!

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
>
>> However, there's a nice juicy 'u16 ack_frame_id' at the start of
>> ieee80211_tx_info. Could we potentially use that? We could use the top
>> bit as a disambiguation flag; I think we're fine with 15 bits for the TX
>> time itself (a single packet won't exceed 8ms or TX time), so if we can
>> live with 15 bits of ACK frame ID space, that could be a way forward?
>
> I was going to say that should work as we only ever have a handful of
> ACK frame IDs, but ... you still need the airtime even for a frame that
> userspace wants to know the ACK status of, no?

Oh, right.

Well, let's try to do the actual math... A full-size (1538 bytes) packet
takes ~2050 microseconds to transmit at 6 Mbps. Adding in overhead, it's
certainly still less that 4096 us, so 12 bits is plenty. That leaves
four bits for the ACK status ID if we just split the u16; if we only
ever have "a handful", that should be enough, no?

We could also split 5/11. That would support up to 32 ACK IDs, and we
can just truncate the airtime at 2048 us, which is not a big deal I'd
say. We could even split 6/10 and only need to truncate the TX time at
rates below 13 Mbps... Or we could sacrifice a bit to the flag and only
truncate if the ACK status flag is set.

Think it mostly depends on what is the smallest ID space for ACK IDs we
can live with? :)

-Toke

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
>
>> However, there's a nice juicy 'u16 ack_frame_id' at the start of
>> ieee80211_tx_info. Could we potentially use that? We could use the top
>> bit as a disambiguation flag; I think we're fine with 15 bits for the TX
>> time itself (a single packet won't exceed 8ms or TX time), so if we can
>> live with 15 bits of ACK frame ID space, that could be a way forward?
>
> I was going to say that should work as we only ever have a handful of
> ACK frame IDs, but ... you still need the airtime even for a frame that
> userspace wants to know the ACK status of, no?

Oh, right.

Well, let's try to do the actual math... A full-size (1538 bytes) packet
takes ~2050 microseconds to transmit at 6 Mbps. Adding in overhead, it's
certainly still less that 4096 us, so 12 bits is plenty. That leaves
four bits for the ACK status ID if we just split the u16; if we only
ever have "a handful", that should be enough, no?

We could also split 5/11. That would support up to 32 ACK IDs, and we
can just truncate the airtime at 2048 us, which is not a big deal I'd
say. We could even split 6/10 and only need to truncate the TX time at
rates below 13 Mbps... Or we could sacrifice a bit to the flag and only
truncate if the ACK status flag is set.

Think it mostly depends on what is the smallest ID space for ACK IDs we
can live with? :)

-Toke


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Johannes Berg]

On Fri, 2019-10-18 at 15:31 +0200, Toke Høiland-Jørgensen wrote:

> Well, let's try to do the actual math... A full-size (1538 bytes) packet
> takes ~2050 microseconds to transmit at 6 Mbps. Adding in overhead, it's
> certainly still less that 4096 us, so 12 bits is plenty.

What about A-MSDUs? But I guess maximum continous transmissions are at
most 4ms anyway, so a single packet should never be longer.

> That leaves
> four bits for the ACK status ID if we just split the u16; if we only
> ever have "a handful", that should be enough, no?

It's how many are in flight at a time, 16 doesn't seem likely to happen,
but I don't really know what applications are doing with it now.
Probably only wpa_s for the EAPOL TX status.

> We could also split 5/11. That would support up to 32 ACK IDs, and we
> can just truncate the airtime at 2048 us, which is not a big deal I'd
> say.

We can also play with the units of the airtime, e.g. making that a
multiple of 2 or 4 us? Seems unlikely to matter much?

> Think it mostly depends on what is the smallest ID space for ACK IDs we
> can live with? :)

:)

TBH, I don't really know. In a lot of hardware using this is really bad
for performance so it shouldn't be used much, so ...

johannes

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Johannes Berg]

On Fri, 2019-10-18 at 15:31 +0200, Toke Høiland-Jørgensen wrote:

> Well, let's try to do the actual math... A full-size (1538 bytes) packet
> takes ~2050 microseconds to transmit at 6 Mbps. Adding in overhead, it's
> certainly still less that 4096 us, so 12 bits is plenty.

What about A-MSDUs? But I guess maximum continous transmissions are at
most 4ms anyway, so a single packet should never be longer.

> That leaves
> four bits for the ACK status ID if we just split the u16; if we only
> ever have "a handful", that should be enough, no?

It's how many are in flight at a time, 16 doesn't seem likely to happen,
but I don't really know what applications are doing with it now.
Probably only wpa_s for the EAPOL TX status.

> We could also split 5/11. That would support up to 32 ACK IDs, and we
> can just truncate the airtime at 2048 us, which is not a big deal I'd
> say.

We can also play with the units of the airtime, e.g. making that a
multiple of 2 or 4 us? Seems unlikely to matter much?

> Think it mostly depends on what is the smallest ID space for ACK IDs we
> can live with? :)

:)

TBH, I don't really know. In a lot of hardware using this is really bad
for performance so it shouldn't be used much, so ...

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2019-10-18 at 15:31 +0200, Toke Høiland-Jørgensen wrote:
>
>> Well, let's try to do the actual math... A full-size (1538 bytes) packet
>> takes ~2050 microseconds to transmit at 6 Mbps. Adding in overhead, it's
>> certainly still less that 4096 us, so 12 bits is plenty.
>
> What about A-MSDUs? But I guess maximum continous transmissions are at
> most 4ms anyway, so a single packet should never be longer.

Ah yeah, those could be a bit bigger, but yeah, 4ms should at least be
enough.

>> That leaves
>> four bits for the ACK status ID if we just split the u16; if we only
>> ever have "a handful", that should be enough, no?
>
> It's how many are in flight at a time, 16 doesn't seem likely to happen,
> but I don't really know what applications are doing with it now.
> Probably only wpa_s for the EAPOL TX status.

Right. Well in that case, let's try it. As long as we fail in a
reasonable way, we can just see if we run into anything that breaks? I
guess in this case that means rejecting requests from userspace if we
run out of IDs rather than silently wrapping and returning wrong data :)

>> We could also split 5/11. That would support up to 32 ACK IDs, and we
>> can just truncate the airtime at 2048 us, which is not a big deal I'd
>> say.
>
> We can also play with the units of the airtime, e.g. making that a
> multiple of 2 or 4 us? Seems unlikely to matter much?

Sure, that's a good point! Increments of 4us means we can fit 4ms is 10
bits, leaving plenty of space for ACK IDs (hopefully).

I'll rework the series to use that instead :)

-Toke

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2019-10-18 at 15:31 +0200, Toke Høiland-Jørgensen wrote:
>
>> Well, let's try to do the actual math... A full-size (1538 bytes) packet
>> takes ~2050 microseconds to transmit at 6 Mbps. Adding in overhead, it's
>> certainly still less that 4096 us, so 12 bits is plenty.
>
> What about A-MSDUs? But I guess maximum continous transmissions are at
> most 4ms anyway, so a single packet should never be longer.

Ah yeah, those could be a bit bigger, but yeah, 4ms should at least be
enough.

>> That leaves
>> four bits for the ACK status ID if we just split the u16; if we only
>> ever have "a handful", that should be enough, no?
>
> It's how many are in flight at a time, 16 doesn't seem likely to happen,
> but I don't really know what applications are doing with it now.
> Probably only wpa_s for the EAPOL TX status.

Right. Well in that case, let's try it. As long as we fail in a
reasonable way, we can just see if we run into anything that breaks? I
guess in this case that means rejecting requests from userspace if we
run out of IDs rather than silently wrapping and returning wrong data :)

>> We could also split 5/11. That would support up to 32 ACK IDs, and we
>> can just truncate the airtime at 2048 us, which is not a big deal I'd
>> say.
>
> We can also play with the units of the airtime, e.g. making that a
> multiple of 2 or 4 us? Seems unlikely to matter much?

Sure, that's a good point! Increments of 4us means we can fit 4ms is 10
bits, leaving plenty of space for ACK IDs (hopefully).

I'll rework the series to use that instead :)

-Toke


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Johannes Berg]

On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote:

> Right. Well in that case, let's try it. As long as we fail in a
> reasonable way, we can just see if we run into anything that breaks? I
> guess in this case that means rejecting requests from userspace if we
> run out of IDs rather than silently wrapping and returning wrong data :)

We can't reject due to how this works, but if the idr_alloc() fails then
we'll just not give a status back to userspace later.

> > > We could also split 5/11. That would support up to 32 ACK IDs, and we
> > > can just truncate the airtime at 2048 us, which is not a big deal I'd
> > > say.
> > 
> > We can also play with the units of the airtime, e.g. making that a
> > multiple of 2 or 4 us? Seems unlikely to matter much?
> 
> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10
> bits, leaving plenty of space for ACK IDs (hopefully).
> 
> I'll rework the series to use that instead :)

OK.

There are two places that call idr_alloc() with a hardcoded limit of
0x10000, you'll have to fix those to have the right limit according to
the bits you leave for the ACK id.

johannes

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Johannes Berg]

On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote:

> Right. Well in that case, let's try it. As long as we fail in a
> reasonable way, we can just see if we run into anything that breaks? I
> guess in this case that means rejecting requests from userspace if we
> run out of IDs rather than silently wrapping and returning wrong data :)

We can't reject due to how this works, but if the idr_alloc() fails then
we'll just not give a status back to userspace later.

> > > We could also split 5/11. That would support up to 32 ACK IDs, and we
> > > can just truncate the airtime at 2048 us, which is not a big deal I'd
> > > say.
> > 
> > We can also play with the units of the airtime, e.g. making that a
> > multiple of 2 or 4 us? Seems unlikely to matter much?
> 
> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10
> bits, leaving plenty of space for ACK IDs (hopefully).
> 
> I'll rework the series to use that instead :)

OK.

There are two places that call idr_alloc() with a hardcoded limit of
0x10000, you'll have to fix those to have the right limit according to
the bits you leave for the ACK id.

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Johannes Berg]

On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote:
> 
> > We can also play with the units of the airtime, e.g. making that a
> > multiple of 2 or 4 us? Seems unlikely to matter much?
> 
> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10
> bits, leaving plenty of space for ACK IDs (hopefully).

If you do need more bits (e.g. to be on the safe side and have space for
8ms) you could also steal bits out of 'band' (we only need 3 I think)
and 'hw_queue' (not sure what the limit really is, but there aren't many
users, seems like only iwlwifi/dvm and hwsim care, and those certainly
don't need >32 queues).

Of course if you leave more bits for later that's good too ;-)

johannes

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Johannes Berg]

On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote:
> 
> > We can also play with the units of the airtime, e.g. making that a
> > multiple of 2 or 4 us? Seems unlikely to matter much?
> 
> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10
> bits, leaving plenty of space for ACK IDs (hopefully).

If you do need more bits (e.g. to be on the safe side and have space for
8ms) you could also steal bits out of 'band' (we only need 3 I think)
and 'hw_queue' (not sure what the limit really is, but there aren't many
users, seems like only iwlwifi/dvm and hwsim care, and those certainly
don't need >32 queues).

Of course if you leave more bits for later that's good too ;-)

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Johannes Berg]

On Thu, 2019-10-17 at 18:11 -0700, Kan Yan wrote:
> I don't think it is hard to take care of extra header size for frames
> with VLAN tags

VLAN tags are payload as far as wifi is concerned, so no need to take
that into account ...

johannes

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

[Johannes Berg]

On Thu, 2019-10-17 at 18:11 -0700, Kan Yan wrote:
> I don't think it is hard to take care of extra header size for frames
> with VLAN tags

VLAN tags are payload as far as wifi is concerned, so no need to take
that into account ...

johannes



_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote:
>
>> Right. Well in that case, let's try it. As long as we fail in a
>> reasonable way, we can just see if we run into anything that breaks? I
>> guess in this case that means rejecting requests from userspace if we
>> run out of IDs rather than silently wrapping and returning wrong data :)
>
> We can't reject due to how this works, but if the idr_alloc() fails then
> we'll just not give a status back to userspace later.

OK, I guess someone will notice if that suddenly starts happening all
the time ;)

>> > > We could also split 5/11. That would support up to 32 ACK IDs, and we
>> > > can just truncate the airtime at 2048 us, which is not a big deal I'd
>> > > say.
>> > 
>> > We can also play with the units of the airtime, e.g. making that a
>> > multiple of 2 or 4 us? Seems unlikely to matter much?
>> 
>> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10
>> bits, leaving plenty of space for ACK IDs (hopefully).
>> 
>> I'll rework the series to use that instead :)
>
> OK.
>
> There are two places that call idr_alloc() with a hardcoded limit of
> 0x10000, you'll have to fix those to have the right limit according to
> the bits you leave for the ACK id.

Yup, found those. Will send a new version of the series in a bit.

-Toke

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote:
>
>> Right. Well in that case, let's try it. As long as we fail in a
>> reasonable way, we can just see if we run into anything that breaks? I
>> guess in this case that means rejecting requests from userspace if we
>> run out of IDs rather than silently wrapping and returning wrong data :)
>
> We can't reject due to how this works, but if the idr_alloc() fails then
> we'll just not give a status back to userspace later.

OK, I guess someone will notice if that suddenly starts happening all
the time ;)

>> > > We could also split 5/11. That would support up to 32 ACK IDs, and we
>> > > can just truncate the airtime at 2048 us, which is not a big deal I'd
>> > > say.
>> > 
>> > We can also play with the units of the airtime, e.g. making that a
>> > multiple of 2 or 4 us? Seems unlikely to matter much?
>> 
>> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10
>> bits, leaving plenty of space for ACK IDs (hopefully).
>> 
>> I'll rework the series to use that instead :)
>
> OK.
>
> There are two places that call idr_alloc() with a hardcoded limit of
> 0x10000, you'll have to fix those to have the right limit according to
> the bits you leave for the ACK id.

Yup, found those. Will send a new version of the series in a bit.

-Toke


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote:
>> 
>> > We can also play with the units of the airtime, e.g. making that a
>> > multiple of 2 or 4 us? Seems unlikely to matter much?
>> 
>> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10
>> bits, leaving plenty of space for ACK IDs (hopefully).
>
> If you do need more bits (e.g. to be on the safe side and have space for
> 8ms) you could also steal bits out of 'band' (we only need 3 I think)
> and 'hw_queue' (not sure what the limit really is, but there aren't many
> users, seems like only iwlwifi/dvm and hwsim care, and those certainly
> don't need >32 queues).
>
> Of course if you leave more bits for later that's good too ;-)

Yeah, let's leave that for later. After all, with the limits we
currently have configured, if a single packet takes up 4096 us, that
will trigger the per-station queue throttling in itself, so I think
we'll be fine (famous last words).

-Toke

Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote:
>> 
>> > We can also play with the units of the airtime, e.g. making that a
>> > multiple of 2 or 4 us? Seems unlikely to matter much?
>> 
>> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10
>> bits, leaving plenty of space for ACK IDs (hopefully).
>
> If you do need more bits (e.g. to be on the safe side and have space for
> 8ms) you could also steal bits out of 'band' (we only need 3 I think)
> and 'hw_queue' (not sure what the limit really is, but there aren't many
> users, seems like only iwlwifi/dvm and hwsim care, and those certainly
> don't need >32 queues).
>
> Of course if you leave more bits for later that's good too ;-)

Yeah, let's leave that for later. After all, with the limits we
currently have configured, if a single packet takes up 4096 us, that
will trigger the per-station queue throttling in itself, so I think
we'll be fine (famous last words).

-Toke


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k