All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 0/4] Add Airtime Queue Limits (AQL) to mac80211
@ 2019-10-23  9:58 ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-23  9:58 UTC (permalink / raw)
  To: Johannes Berg
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

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-06

Changelog:

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

---

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

Toke Høiland-Jørgensen (3):
      mac80211: Shrink the size of ack_frame_id 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     |   45 +++++
 net/mac80211/Makefile      |    3 
 net/mac80211/airtime.c     |  390 ++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/cfg.c         |    2 
 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      |   33 ++++
 net/mac80211/tx.c          |   69 +++++++-
 13 files changed, 709 insertions(+), 18 deletions(-)
 create mode 100644 net/mac80211/airtime.c


^ permalink raw reply	[flat|nested] 43+ messages in thread
* [PATCH v6 0/4] Add Airtime Queue Limits (AQL) to mac80211
@ 2019-10-23  9:58 ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-23  9:58 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

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-06

Changelog:

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

---

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

Toke Høiland-Jørgensen (3):
      mac80211: Shrink the size of ack_frame_id 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     |   45 +++++
 net/mac80211/Makefile      |    3 
 net/mac80211/airtime.c     |  390 ++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/cfg.c         |    2 
 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      |   33 ++++
 net/mac80211/tx.c          |   69 +++++++-
 13 files changed, 709 insertions(+), 18 deletions(-)
 create mode 100644 net/mac80211/airtime.c


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* [PATCH v6 1/4] mac80211: Shrink the size of ack_frame_id to make room for tx_time_est
  2019-10-23  9:58 ` Toke Høiland-Jørgensen
@ 2019-10-23  9:59   ` Toke Høiland-Jørgensen
  -1 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-23  9:59 UTC (permalink / raw)
  To: Johannes Berg
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

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.

To get this, decrease the size of the ack_frame_id field to 6 bits, and
lower the size of the ID space accordingly. This leaves 10 bits for use for
tx_time_est, which is enough to store a maximum of 4096 us, if we shift the
values so they become units of 4us.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/mac80211.h |    4 +++-
 net/mac80211/cfg.c     |    2 +-
 net/mac80211/tx.c      |    2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d69081c38788..4288ace72c2b 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -967,6 +967,7 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate)
  * @band: the band to transmit on (use for checking for races)
  * @hw_queue: HW queue to put the frame on, skb_get_queue_mapping() gives the AC
  * @ack_frame_id: internal frame ID for TX status, used internally
+ * @tx_time_est: TX time estimate in units of 4us, used internally
  * @control: union part for control data
  * @control.rates: TX rates array to try
  * @control.rts_cts_rate_idx: rate for RTS or CTS
@@ -1007,7 +1008,8 @@ struct ieee80211_tx_info {
 
 	u8 hw_queue;
 
-	u16 ack_frame_id;
+	u16 ack_frame_id:6;
+	u16 tx_time_est:10;
 
 	union {
 		struct {
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 70739e746c13..4fb7f1f12109 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3428,7 +3428,7 @@ int ieee80211_attach_ack_skb(struct ieee80211_local *local, struct sk_buff *skb,
 
 	spin_lock_irqsave(&local->ack_status_lock, spin_flags);
 	id = idr_alloc(&local->ack_status_frames, ack_skb,
-		       1, 0x10000, GFP_ATOMIC);
+		       1, 0x40, GFP_ATOMIC);
 	spin_unlock_irqrestore(&local->ack_status_lock, spin_flags);
 
 	if (id < 0) {
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 938c10f7955b..a16c2f863702 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2719,7 +2719,7 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata,
 
 			spin_lock_irqsave(&local->ack_status_lock, flags);
 			id = idr_alloc(&local->ack_status_frames, ack_skb,
-				       1, 0x10000, GFP_ATOMIC);
+				       1, 0x40, GFP_ATOMIC);
 			spin_unlock_irqrestore(&local->ack_status_lock, flags);
 
 			if (id >= 0) {


^ permalink raw reply related	[flat|nested] 43+ messages in thread
* [PATCH v6 1/4] mac80211: Shrink the size of ack_frame_id to make room for tx_time_est
@ 2019-10-23  9:59   ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-23  9:59 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

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.

To get this, decrease the size of the ack_frame_id field to 6 bits, and
lower the size of the ID space accordingly. This leaves 10 bits for use for
tx_time_est, which is enough to store a maximum of 4096 us, if we shift the
values so they become units of 4us.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/mac80211.h |    4 +++-
 net/mac80211/cfg.c     |    2 +-
 net/mac80211/tx.c      |    2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d69081c38788..4288ace72c2b 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -967,6 +967,7 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate)
  * @band: the band to transmit on (use for checking for races)
  * @hw_queue: HW queue to put the frame on, skb_get_queue_mapping() gives the AC
  * @ack_frame_id: internal frame ID for TX status, used internally
+ * @tx_time_est: TX time estimate in units of 4us, used internally
  * @control: union part for control data
  * @control.rates: TX rates array to try
  * @control.rts_cts_rate_idx: rate for RTS or CTS
@@ -1007,7 +1008,8 @@ struct ieee80211_tx_info {
 
 	u8 hw_queue;
 
-	u16 ack_frame_id;
+	u16 ack_frame_id:6;
+	u16 tx_time_est:10;
 
 	union {
 		struct {
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 70739e746c13..4fb7f1f12109 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3428,7 +3428,7 @@ int ieee80211_attach_ack_skb(struct ieee80211_local *local, struct sk_buff *skb,
 
 	spin_lock_irqsave(&local->ack_status_lock, spin_flags);
 	id = idr_alloc(&local->ack_status_frames, ack_skb,
-		       1, 0x10000, GFP_ATOMIC);
+		       1, 0x40, GFP_ATOMIC);
 	spin_unlock_irqrestore(&local->ack_status_lock, spin_flags);
 
 	if (id < 0) {
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 938c10f7955b..a16c2f863702 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2719,7 +2719,7 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata,
 
 			spin_lock_irqsave(&local->ack_status_lock, flags);
 			id = idr_alloc(&local->ack_status_frames, ack_skb,
-				       1, 0x10000, GFP_ATOMIC);
+				       1, 0x40, GFP_ATOMIC);
 			spin_unlock_irqrestore(&local->ack_status_lock, flags);
 
 			if (id >= 0) {


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

^ permalink raw reply related	[flat|nested] 43+ messages in thread
* [PATCH v6 2/4] mac80211: Import airtime calculation code from mt76
  2019-10-23  9:58 ` Toke Høiland-Jørgensen
@ 2019-10-23  9:59   ` Toke Høiland-Jørgensen
  -1 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-23  9:59 UTC (permalink / raw)
  To: Johannes Berg
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

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     |   29 +++
 net/mac80211/Makefile      |    3 
 net/mac80211/airtime.c     |  390 ++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h |    4 
 4 files changed, 425 insertions(+), 1 deletion(-)
 create mode 100644 net/mac80211/airtime.c

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 4288ace72c2b..8a3e0544a026 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -6424,4 +6424,33 @@ void ieee80211_nan_func_match(struct ieee80211_vif *vif,
 			      struct cfg80211_nan_match_params *match,
 			      gfp_t gfp);
 
+/**
+ * ieee80211_calc_rx_airtime - calculate estimated transmission airtime for RX.
+ *
+ * This function calculates the estimated airtime usage of a frame based on the
+ * rate information in the RX status struct and the frame length.
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @status: &struct ieee80211_rx_status containing the transmission rate
+ *          information.
+ * @len: frame length in bytes
+ */
+u32 ieee80211_calc_rx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_rx_status *status,
+			      int len);
+
+/**
+ * ieee80211_calc_tx_airtime - calculate estimated transmission airtime for TX.
+ *
+ * This function calculates the estimated airtime usage of a frame based on the
+ * rate information in the TX info struct and the frame length.
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @info: &struct ieee80211_tx_info of the frame.
+ * @len: frame length in bytes
+ */
+u32 ieee80211_calc_tx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_tx_info *info,
+			      int len);
+
 #endif /* MAC80211_H */
diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile
index 4f03ebe732fa..6cbb1286d6c0 100644
--- a/net/mac80211/Makefile
+++ b/net/mac80211/Makefile
@@ -32,7 +32,8 @@ mac80211-y := \
 	chan.o \
 	trace.o mlme.o \
 	tdls.o \
-	ocb.o
+	ocb.o \
+	airtime.o
 
 mac80211-$(CONFIG_MAC80211_LEDS) += led.o
 mac80211-$(CONFIG_MAC80211_DEBUGFS) += \
diff --git a/net/mac80211/airtime.c b/net/mac80211/airtime.c
new file mode 100644
index 000000000000..a521f3730381
--- /dev/null
+++ b/net/mac80211/airtime.c
@@ -0,0 +1,390 @@
+// 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(u16 bitrate, bool short_pre,
+				    bool cck, int len)
+{
+	u32 duration;
+
+	if (cck) {
+		duration = 144 + 48; /* preamble + PLCP */
+		if (short_pre)
+			duration >>= 1;
+
+		duration += 10; /* SIFS */
+	} else {
+		duration = 20 + 16; /* premable + SIFS */
+	}
+
+	len <<= 3;
+	duration += (len * 10) / bitrate;
+
+	return duration;
+}
+
+u32 ieee80211_calc_rx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_rx_status *status,
+			      int len)
+{
+	struct ieee80211_supported_band *sband;
+	const struct ieee80211_rate *rate;
+	bool sgi = status->enc_flags & RX_ENC_FLAG_SHORT_GI;
+	bool sp = status->enc_flags & RX_ENC_FLAG_SHORTPRE;
+	int bw, streams;
+	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->bitrate, sp,
+							   cck, len);
+
+	case RX_ENC_VHT:
+		streams = status->nss;
+		idx = status->rate_idx;
+		group = VHT_GROUP_IDX(streams, sgi, bw);
+		break;
+	case RX_ENC_HT:
+		streams = ((status->rate_idx >> 3) & 3) + 1;
+		idx = status->rate_idx & 7;
+		group = HT_GROUP_IDX(streams, sgi, bw);
+		break;
+	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;
+}
+EXPORT_SYMBOL_GPL(ieee80211_calc_rx_airtime);
+
+static u32 ieee80211_calc_tx_airtime_rate(struct ieee80211_hw *hw,
+					  struct ieee80211_tx_rate *rate,
+					  u8 band, int len)
+{
+	struct ieee80211_rx_status stat = {
+		.band = band,
+	};
+
+	if (rate->idx < 0 || !rate->count)
+		return 0;
+
+	if (rate->flags & IEEE80211_TX_RC_80_MHZ_WIDTH)
+		stat.bw = RATE_INFO_BW_80;
+	else if (rate->flags & IEEE80211_TX_RC_40_MHZ_WIDTH)
+		stat.bw = RATE_INFO_BW_40;
+	else
+		stat.bw = RATE_INFO_BW_20;
+
+	stat.enc_flags = 0;
+	if (rate->flags & IEEE80211_TX_RC_USE_SHORT_PREAMBLE)
+		stat.enc_flags |= RX_ENC_FLAG_SHORTPRE;
+	if (rate->flags & IEEE80211_TX_RC_SHORT_GI)
+		stat.enc_flags |= RX_ENC_FLAG_SHORT_GI;
+
+	stat.rate_idx = rate->idx;
+	if (rate->flags & IEEE80211_TX_RC_VHT_MCS) {
+		stat.encoding = RX_ENC_VHT;
+		stat.rate_idx = ieee80211_rate_get_vht_mcs(rate);
+		stat.nss = ieee80211_rate_get_vht_nss(rate);
+	} else if (rate->flags & IEEE80211_TX_RC_MCS) {
+		stat.encoding = RX_ENC_HT;
+	} else {
+		stat.encoding = RX_ENC_LEGACY;
+	}
+
+	return ieee80211_calc_rx_airtime(hw, &stat, len);
+}
+
+u32 ieee80211_calc_tx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_tx_info *info,
+			      int len)
+{
+	u32 duration = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(info->status.rates); i++) {
+		struct ieee80211_tx_rate *rate = &info->status.rates[i];
+		u32 cur_duration;
+
+		cur_duration = ieee80211_calc_tx_airtime_rate(hw, rate,
+							      info->band, len);
+		if (!cur_duration)
+			break;
+
+		duration += cur_duration * rate->count;
+	}
+
+	return duration;
+}
+EXPORT_SYMBOL_GPL(ieee80211_calc_tx_airtime);
+
+u32 ieee80211_calc_expected_tx_airtime(struct ieee80211_hw *hw,
+				       struct ieee80211_vif *vif,
+				       struct ieee80211_sta *pubsta,
+				       int len)
+{
+	struct ieee80211_supported_band *sband;
+	struct ieee80211_chanctx_conf *conf;
+	int rateidx, shift = 0;
+	bool cck, short_pream;
+	u32 basic_rates;
+	u8 band = 0;
+	u16 rate;
+
+	len += 38; /* Ethernet header length */
+
+	conf = rcu_dereference(vif->chanctx_conf);
+	if (conf) {
+		band = conf->def.chan->band;
+		shift = ieee80211_chandef_get_shift(&conf->def);
+	}
+
+	if (pubsta) {
+		struct sta_info *sta = container_of(pubsta, struct sta_info,
+						    sta);
+
+		return ieee80211_calc_tx_airtime_rate(hw,
+						      &sta->tx_stats.last_rate,
+						      band, len);
+	}
+
+	if (!conf)
+		return 0;
+
+	/* No station to get latest rate from, so calculate the worst-case
+	 * duration using the lowest configured basic rate.
+	 */
+	sband = hw->wiphy->bands[band];
+
+	basic_rates = vif->bss_conf.basic_rates;
+	short_pream = vif->bss_conf.use_short_preamble;
+
+	rateidx = basic_rates ? ffs(basic_rates) - 1 : 0;
+	rate = sband->bitrates[rateidx].bitrate << shift;
+	cck = (rateidx < CCK_NUM_RATES);
+
+	return ieee80211_calc_legacy_rate_duration(rate, short_pream, cck, len);
+}
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 05406e9c05b3..225ea4e3cd76 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -2249,6 +2249,10 @@ const char *ieee80211_get_reason_code_string(u16 reason_code);
 
 extern const struct ethtool_ops ieee80211_ethtool_ops;
 
+u32 ieee80211_calc_expected_tx_airtime(struct ieee80211_hw *hw,
+				       struct ieee80211_vif *vif,
+				       struct ieee80211_sta *pubsta,
+				       int len);
 #ifdef CONFIG_MAC80211_NOINLINE
 #define debug_noinline noinline
 #else


^ permalink raw reply related	[flat|nested] 43+ messages in thread
* [PATCH v6 2/4] mac80211: Import airtime calculation code from mt76
@ 2019-10-23  9:59   ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-23  9:59 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

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     |   29 +++
 net/mac80211/Makefile      |    3 
 net/mac80211/airtime.c     |  390 ++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h |    4 
 4 files changed, 425 insertions(+), 1 deletion(-)
 create mode 100644 net/mac80211/airtime.c

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 4288ace72c2b..8a3e0544a026 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -6424,4 +6424,33 @@ void ieee80211_nan_func_match(struct ieee80211_vif *vif,
 			      struct cfg80211_nan_match_params *match,
 			      gfp_t gfp);
 
+/**
+ * ieee80211_calc_rx_airtime - calculate estimated transmission airtime for RX.
+ *
+ * This function calculates the estimated airtime usage of a frame based on the
+ * rate information in the RX status struct and the frame length.
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @status: &struct ieee80211_rx_status containing the transmission rate
+ *          information.
+ * @len: frame length in bytes
+ */
+u32 ieee80211_calc_rx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_rx_status *status,
+			      int len);
+
+/**
+ * ieee80211_calc_tx_airtime - calculate estimated transmission airtime for TX.
+ *
+ * This function calculates the estimated airtime usage of a frame based on the
+ * rate information in the TX info struct and the frame length.
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @info: &struct ieee80211_tx_info of the frame.
+ * @len: frame length in bytes
+ */
+u32 ieee80211_calc_tx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_tx_info *info,
+			      int len);
+
 #endif /* MAC80211_H */
diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile
index 4f03ebe732fa..6cbb1286d6c0 100644
--- a/net/mac80211/Makefile
+++ b/net/mac80211/Makefile
@@ -32,7 +32,8 @@ mac80211-y := \
 	chan.o \
 	trace.o mlme.o \
 	tdls.o \
-	ocb.o
+	ocb.o \
+	airtime.o
 
 mac80211-$(CONFIG_MAC80211_LEDS) += led.o
 mac80211-$(CONFIG_MAC80211_DEBUGFS) += \
diff --git a/net/mac80211/airtime.c b/net/mac80211/airtime.c
new file mode 100644
index 000000000000..a521f3730381
--- /dev/null
+++ b/net/mac80211/airtime.c
@@ -0,0 +1,390 @@
+// 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(u16 bitrate, bool short_pre,
+				    bool cck, int len)
+{
+	u32 duration;
+
+	if (cck) {
+		duration = 144 + 48; /* preamble + PLCP */
+		if (short_pre)
+			duration >>= 1;
+
+		duration += 10; /* SIFS */
+	} else {
+		duration = 20 + 16; /* premable + SIFS */
+	}
+
+	len <<= 3;
+	duration += (len * 10) / bitrate;
+
+	return duration;
+}
+
+u32 ieee80211_calc_rx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_rx_status *status,
+			      int len)
+{
+	struct ieee80211_supported_band *sband;
+	const struct ieee80211_rate *rate;
+	bool sgi = status->enc_flags & RX_ENC_FLAG_SHORT_GI;
+	bool sp = status->enc_flags & RX_ENC_FLAG_SHORTPRE;
+	int bw, streams;
+	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->bitrate, sp,
+							   cck, len);
+
+	case RX_ENC_VHT:
+		streams = status->nss;
+		idx = status->rate_idx;
+		group = VHT_GROUP_IDX(streams, sgi, bw);
+		break;
+	case RX_ENC_HT:
+		streams = ((status->rate_idx >> 3) & 3) + 1;
+		idx = status->rate_idx & 7;
+		group = HT_GROUP_IDX(streams, sgi, bw);
+		break;
+	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;
+}
+EXPORT_SYMBOL_GPL(ieee80211_calc_rx_airtime);
+
+static u32 ieee80211_calc_tx_airtime_rate(struct ieee80211_hw *hw,
+					  struct ieee80211_tx_rate *rate,
+					  u8 band, int len)
+{
+	struct ieee80211_rx_status stat = {
+		.band = band,
+	};
+
+	if (rate->idx < 0 || !rate->count)
+		return 0;
+
+	if (rate->flags & IEEE80211_TX_RC_80_MHZ_WIDTH)
+		stat.bw = RATE_INFO_BW_80;
+	else if (rate->flags & IEEE80211_TX_RC_40_MHZ_WIDTH)
+		stat.bw = RATE_INFO_BW_40;
+	else
+		stat.bw = RATE_INFO_BW_20;
+
+	stat.enc_flags = 0;
+	if (rate->flags & IEEE80211_TX_RC_USE_SHORT_PREAMBLE)
+		stat.enc_flags |= RX_ENC_FLAG_SHORTPRE;
+	if (rate->flags & IEEE80211_TX_RC_SHORT_GI)
+		stat.enc_flags |= RX_ENC_FLAG_SHORT_GI;
+
+	stat.rate_idx = rate->idx;
+	if (rate->flags & IEEE80211_TX_RC_VHT_MCS) {
+		stat.encoding = RX_ENC_VHT;
+		stat.rate_idx = ieee80211_rate_get_vht_mcs(rate);
+		stat.nss = ieee80211_rate_get_vht_nss(rate);
+	} else if (rate->flags & IEEE80211_TX_RC_MCS) {
+		stat.encoding = RX_ENC_HT;
+	} else {
+		stat.encoding = RX_ENC_LEGACY;
+	}
+
+	return ieee80211_calc_rx_airtime(hw, &stat, len);
+}
+
+u32 ieee80211_calc_tx_airtime(struct ieee80211_hw *hw,
+			      struct ieee80211_tx_info *info,
+			      int len)
+{
+	u32 duration = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(info->status.rates); i++) {
+		struct ieee80211_tx_rate *rate = &info->status.rates[i];
+		u32 cur_duration;
+
+		cur_duration = ieee80211_calc_tx_airtime_rate(hw, rate,
+							      info->band, len);
+		if (!cur_duration)
+			break;
+
+		duration += cur_duration * rate->count;
+	}
+
+	return duration;
+}
+EXPORT_SYMBOL_GPL(ieee80211_calc_tx_airtime);
+
+u32 ieee80211_calc_expected_tx_airtime(struct ieee80211_hw *hw,
+				       struct ieee80211_vif *vif,
+				       struct ieee80211_sta *pubsta,
+				       int len)
+{
+	struct ieee80211_supported_band *sband;
+	struct ieee80211_chanctx_conf *conf;
+	int rateidx, shift = 0;
+	bool cck, short_pream;
+	u32 basic_rates;
+	u8 band = 0;
+	u16 rate;
+
+	len += 38; /* Ethernet header length */
+
+	conf = rcu_dereference(vif->chanctx_conf);
+	if (conf) {
+		band = conf->def.chan->band;
+		shift = ieee80211_chandef_get_shift(&conf->def);
+	}
+
+	if (pubsta) {
+		struct sta_info *sta = container_of(pubsta, struct sta_info,
+						    sta);
+
+		return ieee80211_calc_tx_airtime_rate(hw,
+						      &sta->tx_stats.last_rate,
+						      band, len);
+	}
+
+	if (!conf)
+		return 0;
+
+	/* No station to get latest rate from, so calculate the worst-case
+	 * duration using the lowest configured basic rate.
+	 */
+	sband = hw->wiphy->bands[band];
+
+	basic_rates = vif->bss_conf.basic_rates;
+	short_pream = vif->bss_conf.use_short_preamble;
+
+	rateidx = basic_rates ? ffs(basic_rates) - 1 : 0;
+	rate = sband->bitrates[rateidx].bitrate << shift;
+	cck = (rateidx < CCK_NUM_RATES);
+
+	return ieee80211_calc_legacy_rate_duration(rate, short_pream, cck, len);
+}
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 05406e9c05b3..225ea4e3cd76 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -2249,6 +2249,10 @@ const char *ieee80211_get_reason_code_string(u16 reason_code);
 
 extern const struct ethtool_ops ieee80211_ethtool_ops;
 
+u32 ieee80211_calc_expected_tx_airtime(struct ieee80211_hw *hw,
+				       struct ieee80211_vif *vif,
+				       struct ieee80211_sta *pubsta,
+				       int len);
 #ifdef CONFIG_MAC80211_NOINLINE
 #define debug_noinline noinline
 #else


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

^ permalink raw reply related	[flat|nested] 43+ messages in thread
* [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-10-23  9:58 ` Toke Høiland-Jørgensen
@ 2019-10-23  9:59   ` Toke Høiland-Jørgensen
  -1 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-23  9:59 UTC (permalink / raw)
  To: Johannes Berg
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

From: Kan Yan <kyan@google.com>

In order for the Fq_CoDel algorithm integrated in mac80211 layer to operate
effectively to control excessive queueing latency, the CoDel algorithm
requires an accurate measure of how long packets stays in the queue, AKA
sojourn time. The sojourn time measured at the mac80211 layer doesn't
include queueing latency in the lower layer (firmware/hardware) and CoDel
expects lower layer to have a short queue. However, most 802.11ac chipsets
offload tasks such TX aggregation to firmware or hardware, thus have a deep
lower layer queue.

Without a mechanism to control the lower layer queue size, packets only
stay in mac80211 layer transiently before being sent to firmware queue.
As a result, the sojourn time measured by CoDel in the mac80211 layer is
almost always lower than the CoDel latency target, hence CoDel does little
to control the latency, even when the lower layer queue causes excessive
latency.

The Byte Queue Limits (BQL) mechanism is commonly used to address the
similar issue with wired network interface. However, this method cannot be
applied directly to the wireless network interface. "Bytes" is not a
suitable measure of queue depth in the wireless network, as the data rate
can vary dramatically from station to station in the same network, from a
few Mbps to over Gbps.

This patch implements an Airtime-based Queue Limit (AQL) to make CoDel work
effectively with wireless drivers that utilized firmware/hardware
offloading. AQL allows each txq to release just enough packets to the lower
layer to form 1-2 large aggregations to keep hardware fully utilized and
retains the rest of the frames in mac80211 layer to be controlled by the
CoDel algorithm.

Signed-off-by: Kan Yan <kyan@google.com>
[ Toke: Keep API to set pending airtime internal, fix nits in commit msg ]
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/cfg80211.h     |    7 ++++
 include/net/mac80211.h     |   12 +++++++
 net/mac80211/debugfs.c     |   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 8a3e0544a026..2bc0f2538a36 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5565,6 +5565,18 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
 void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
 				    u32 tx_airtime, u32 rx_airtime);
 
+/**
+ * ieee80211_txq_airtime_check - check if a txq can send frame to device
+ *
+ * @hw: pointer obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Return true if the AQL's airtime limit has not been reached and the txq can
+ * continue to send more packets to the device. Otherwise return false.
+ */
+bool
+ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
+
 /**
  * ieee80211_iter_keys - iterate keys programmed into the device
  * @hw: pointer obtained from ieee80211_alloc_hw()
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 568b3b276931..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 a16c2f863702..12653d873b8c 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)
 {


^ permalink raw reply related	[flat|nested] 43+ messages in thread
* [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
@ 2019-10-23  9:59   ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-23  9:59 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

From: Kan Yan <kyan@google.com>

In order for the Fq_CoDel algorithm integrated in mac80211 layer to operate
effectively to control excessive queueing latency, the CoDel algorithm
requires an accurate measure of how long packets stays in the queue, AKA
sojourn time. The sojourn time measured at the mac80211 layer doesn't
include queueing latency in the lower layer (firmware/hardware) and CoDel
expects lower layer to have a short queue. However, most 802.11ac chipsets
offload tasks such TX aggregation to firmware or hardware, thus have a deep
lower layer queue.

Without a mechanism to control the lower layer queue size, packets only
stay in mac80211 layer transiently before being sent to firmware queue.
As a result, the sojourn time measured by CoDel in the mac80211 layer is
almost always lower than the CoDel latency target, hence CoDel does little
to control the latency, even when the lower layer queue causes excessive
latency.

The Byte Queue Limits (BQL) mechanism is commonly used to address the
similar issue with wired network interface. However, this method cannot be
applied directly to the wireless network interface. "Bytes" is not a
suitable measure of queue depth in the wireless network, as the data rate
can vary dramatically from station to station in the same network, from a
few Mbps to over Gbps.

This patch implements an Airtime-based Queue Limit (AQL) to make CoDel work
effectively with wireless drivers that utilized firmware/hardware
offloading. AQL allows each txq to release just enough packets to the lower
layer to form 1-2 large aggregations to keep hardware fully utilized and
retains the rest of the frames in mac80211 layer to be controlled by the
CoDel algorithm.

Signed-off-by: Kan Yan <kyan@google.com>
[ Toke: Keep API to set pending airtime internal, fix nits in commit msg ]
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/cfg80211.h     |    7 ++++
 include/net/mac80211.h     |   12 +++++++
 net/mac80211/debugfs.c     |   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 8a3e0544a026..2bc0f2538a36 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5565,6 +5565,18 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
 void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
 				    u32 tx_airtime, u32 rx_airtime);
 
+/**
+ * ieee80211_txq_airtime_check - check if a txq can send frame to device
+ *
+ * @hw: pointer obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Return true if the AQL's airtime limit has not been reached and the txq can
+ * continue to send more packets to the device. Otherwise return false.
+ */
+bool
+ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
+
 /**
  * ieee80211_iter_keys - iterate keys programmed into the device
  * @hw: pointer obtained from ieee80211_alloc_hw()
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 568b3b276931..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 a16c2f863702..12653d873b8c 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

^ permalink raw reply related	[flat|nested] 43+ messages in thread
* [PATCH v6 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
  2019-10-23  9:58 ` Toke Høiland-Jørgensen
@ 2019-10-23  9:59   ` Toke Høiland-Jørgensen
  -1 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-23  9:59 UTC (permalink / raw)
  To: Johannes Berg
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

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 |   33 +++++++++++++++++++++++++++++++++
 net/mac80211/tx.c     |   21 +++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index ab8ba5835ca0..ae15c8fd2421 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -676,6 +676,28 @@ static void ieee80211_report_used_skb(struct ieee80211_local *local,
 	if (dropped)
 		acked = false;
 
+	if (info->tx_time_est) {
+		struct sta_info *sta = NULL, *s;
+		struct rhlist_head *tmp;
+
+		rcu_read_lock();
+
+		for_each_sta_info(local, hdr->addr1, s, tmp) {
+			/* skip wrong virtual interface */
+			if (!ether_addr_equal(hdr->addr2, s->sdata->vif.addr))
+				continue;
+
+			sta = s;
+			break;
+		}
+
+		ieee80211_sta_update_pending_airtime(local, sta,
+						     skb_get_queue_mapping(skb),
+						     info->tx_time_est << 2,
+						     true);
+		rcu_read_unlock();
+	}
+
 	if (info->flags & IEEE80211_TX_INTFL_MLME_CONN_TX) {
 		struct ieee80211_sub_if_data *sdata;
 
@@ -986,6 +1008,17 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
 			ieee80211_sta_register_airtime(&sta->sta, tid,
 						       info->status.tx_time, 0);
 
+		if (info->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,
+							     skb_get_queue_mapping(skb),
+							     info->tx_time_est << 2,
+							     true);
+			info->tx_time_est = 0;
+		}
+
 		if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS)) {
 			if (info->flags & IEEE80211_TX_STAT_ACK) {
 				if (sta->status_stats.lost_packets)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 12653d873b8c..1405304d8994 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3542,6 +3542,9 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 
 	WARN_ON_ONCE(softirq_count() == 0);
 
+	if (!ieee80211_txq_airtime_check(hw, txq))
+		return NULL;
+
 begin:
 	spin_lock_bh(&fq->lock);
 
@@ -3652,6 +3655,24 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	}
 
 	IEEE80211_SKB_CB(skb)->control.vif = vif;
+
+	if (local->airtime_flags & AIRTIME_USE_AQL) {
+		u32 airtime;
+
+		airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
+							     skb->len);
+		if (airtime) {
+			/* We only have 10 bits in tx_time_est, so store airtime
+			 * in increments of 4us and clamp the maximum to 2**12-1
+			 */
+			airtime = min_t(u32, airtime, 4095) & ~3U;
+			info->tx_time_est = airtime >> 2;
+			ieee80211_sta_update_pending_airtime(local, tx.sta,
+							     txq->ac, airtime,
+							     false);
+		}
+	}
+
 	return skb;
 
 out:


^ permalink raw reply related	[flat|nested] 43+ messages in thread
* [PATCH v6 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
@ 2019-10-23  9:59   ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-23  9:59 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

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 |   33 +++++++++++++++++++++++++++++++++
 net/mac80211/tx.c     |   21 +++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index ab8ba5835ca0..ae15c8fd2421 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -676,6 +676,28 @@ static void ieee80211_report_used_skb(struct ieee80211_local *local,
 	if (dropped)
 		acked = false;
 
+	if (info->tx_time_est) {
+		struct sta_info *sta = NULL, *s;
+		struct rhlist_head *tmp;
+
+		rcu_read_lock();
+
+		for_each_sta_info(local, hdr->addr1, s, tmp) {
+			/* skip wrong virtual interface */
+			if (!ether_addr_equal(hdr->addr2, s->sdata->vif.addr))
+				continue;
+
+			sta = s;
+			break;
+		}
+
+		ieee80211_sta_update_pending_airtime(local, sta,
+						     skb_get_queue_mapping(skb),
+						     info->tx_time_est << 2,
+						     true);
+		rcu_read_unlock();
+	}
+
 	if (info->flags & IEEE80211_TX_INTFL_MLME_CONN_TX) {
 		struct ieee80211_sub_if_data *sdata;
 
@@ -986,6 +1008,17 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
 			ieee80211_sta_register_airtime(&sta->sta, tid,
 						       info->status.tx_time, 0);
 
+		if (info->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,
+							     skb_get_queue_mapping(skb),
+							     info->tx_time_est << 2,
+							     true);
+			info->tx_time_est = 0;
+		}
+
 		if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS)) {
 			if (info->flags & IEEE80211_TX_STAT_ACK) {
 				if (sta->status_stats.lost_packets)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 12653d873b8c..1405304d8994 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3542,6 +3542,9 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 
 	WARN_ON_ONCE(softirq_count() == 0);
 
+	if (!ieee80211_txq_airtime_check(hw, txq))
+		return NULL;
+
 begin:
 	spin_lock_bh(&fq->lock);
 
@@ -3652,6 +3655,24 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	}
 
 	IEEE80211_SKB_CB(skb)->control.vif = vif;
+
+	if (local->airtime_flags & AIRTIME_USE_AQL) {
+		u32 airtime;
+
+		airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
+							     skb->len);
+		if (airtime) {
+			/* We only have 10 bits in tx_time_est, so store airtime
+			 * in increments of 4us and clamp the maximum to 2**12-1
+			 */
+			airtime = min_t(u32, airtime, 4095) & ~3U;
+			info->tx_time_est = airtime >> 2;
+			ieee80211_sta_update_pending_airtime(local, tx.sta,
+							     txq->ac, airtime,
+							     false);
+		}
+	}
+
 	return skb;
 
 out:


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

^ permalink raw reply related	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 0/4] Add Airtime Queue Limits (AQL) to mac80211
  2019-10-23  9:58 ` Toke Høiland-Jørgensen
@ 2019-11-07  6:14   ` Kan Yan
  -1 siblings, 0 replies; 43+ messages in thread
From: Kan Yan @ 2019-11-07  6:14 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Johannes Berg, linux-wireless, Make-Wifi-fast, ath10k,
	John Crispin, Lorenzo Bianconi, Felix Fietkau,
	Rajkumar Manoharan, Kevin Hayes

Patchset v6 works for me with ath10k driver.  AQL does its job as
expected and tests show very significant reduction in latency in
congested environment. The txq stuck issue in patchset v4 got fixed.

However, the device's total pending airtime count still underflows
sometimes. Even though it doesn't cause apparent side effect, there is
some issue with the pending airtime update and needs further
debugging.

Regards,
Kan


On Wed, Oct 23, 2019 at 2:59 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> 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-06
>
> Changelog:
>
> v6:
>   - Fix sta lookup in ieee80211_report_used_skb().
>   - Move call to ieee80211_sta_update_pending_airtime() to a bit later in
>     __ieee80211_tx_status()
> v5:
>   - Add missing export of ieee80211_calc_rx_airtime() and make
>     ieee80211_calc_tx_airtime_rate() static (kbuildbot).
>   - Use skb_get_queue_mapping() to get the AC from the skb.
>   - Take basic rate configuration for the BSS into account when calculating
>     multicast rate.
> v4:
>   - Fix calculation that clamps the maximum airtime to fit into 10 bits
>   - Incorporate Rich Brown's nits for the commit message in Kan's patch
>   - Add fewer local variables to ieee80211_tx_dequeue()
> v3:
>   - Move the tx_time_est field so it's shared with ack_frame_id, and use units
>     of 4us for the value stored in it.
>   - Move the addition of the Ethernet header size into ieee80211_calc_expected_tx_airtime()
> v2:
>   - Integrate Kan's approach to airtime throttling.
>   - Hopefully fix the cb struct alignment on big-endian architectures.
>
> ---
>
> Kan Yan (1):
>       mac80211: Implement Airtime-based Queue Limit (AQL)
>
> Toke Høiland-Jørgensen (3):
>       mac80211: Shrink the size of ack_frame_id 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     |   45 +++++
>  net/mac80211/Makefile      |    3
>  net/mac80211/airtime.c     |  390 ++++++++++++++++++++++++++++++++++++++++++++
>  net/mac80211/cfg.c         |    2
>  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      |   33 ++++
>  net/mac80211/tx.c          |   69 +++++++-
>  13 files changed, 709 insertions(+), 18 deletions(-)
>  create mode 100644 net/mac80211/airtime.c
>

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 0/4] Add Airtime Queue Limits (AQL) to mac80211
@ 2019-11-07  6:14   ` Kan Yan
  0 siblings, 0 replies; 43+ messages in thread
From: Kan Yan @ 2019-11-07  6:14 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Rajkumar Manoharan, Kevin Hayes, Make-Wifi-fast, linux-wireless,
	ath10k, John Crispin, Johannes Berg, Lorenzo Bianconi,
	Felix Fietkau

Patchset v6 works for me with ath10k driver.  AQL does its job as
expected and tests show very significant reduction in latency in
congested environment. The txq stuck issue in patchset v4 got fixed.

However, the device's total pending airtime count still underflows
sometimes. Even though it doesn't cause apparent side effect, there is
some issue with the pending airtime update and needs further
debugging.

Regards,
Kan


On Wed, Oct 23, 2019 at 2:59 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> 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-06
>
> Changelog:
>
> v6:
>   - Fix sta lookup in ieee80211_report_used_skb().
>   - Move call to ieee80211_sta_update_pending_airtime() to a bit later in
>     __ieee80211_tx_status()
> v5:
>   - Add missing export of ieee80211_calc_rx_airtime() and make
>     ieee80211_calc_tx_airtime_rate() static (kbuildbot).
>   - Use skb_get_queue_mapping() to get the AC from the skb.
>   - Take basic rate configuration for the BSS into account when calculating
>     multicast rate.
> v4:
>   - Fix calculation that clamps the maximum airtime to fit into 10 bits
>   - Incorporate Rich Brown's nits for the commit message in Kan's patch
>   - Add fewer local variables to ieee80211_tx_dequeue()
> v3:
>   - Move the tx_time_est field so it's shared with ack_frame_id, and use units
>     of 4us for the value stored in it.
>   - Move the addition of the Ethernet header size into ieee80211_calc_expected_tx_airtime()
> v2:
>   - Integrate Kan's approach to airtime throttling.
>   - Hopefully fix the cb struct alignment on big-endian architectures.
>
> ---
>
> Kan Yan (1):
>       mac80211: Implement Airtime-based Queue Limit (AQL)
>
> Toke Høiland-Jørgensen (3):
>       mac80211: Shrink the size of ack_frame_id 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     |   45 +++++
>  net/mac80211/Makefile      |    3
>  net/mac80211/airtime.c     |  390 ++++++++++++++++++++++++++++++++++++++++++++
>  net/mac80211/cfg.c         |    2
>  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      |   33 ++++
>  net/mac80211/tx.c          |   69 +++++++-
>  13 files changed, 709 insertions(+), 18 deletions(-)
>  create mode 100644 net/mac80211/airtime.c
>

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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 0/4] Add Airtime Queue Limits (AQL) to mac80211
  2019-11-07  6:14   ` Kan Yan
@ 2019-11-07 10:55     ` Toke Høiland-Jørgensen
  -1 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-07 10:55 UTC (permalink / raw)
  To: Kan Yan
  Cc: Johannes Berg, linux-wireless, Make-Wifi-fast, ath10k,
	John Crispin, Lorenzo Bianconi, Felix Fietkau,
	Rajkumar Manoharan, Kevin Hayes

Kan Yan <kyan@google.com> writes:

> Patchset v6 works for me with ath10k driver.  AQL does its job as
> expected and tests show very significant reduction in latency in
> congested environment. The txq stuck issue in patchset v4 got fixed.

Awesome! Thank you for testing!

> However, the device's total pending airtime count still underflows
> sometimes. Even though it doesn't cause apparent side effect, there is
> some issue with the pending airtime update and needs further
> debugging.

Huh, it *under*flows? That's... interesting. Cloned SKBs? Or maybe the
tx_time_est field is being set in some other place?

Could I get you to add a dump_stack() to the underflow test so we can
get an idea of where that happens?

-Toke


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 0/4] Add Airtime Queue Limits (AQL) to mac80211
@ 2019-11-07 10:55     ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-07 10:55 UTC (permalink / raw)
  To: Kan Yan
  Cc: Rajkumar Manoharan, Kevin Hayes, Make-Wifi-fast, linux-wireless,
	ath10k, John Crispin, Johannes Berg, Lorenzo Bianconi,
	Felix Fietkau

Kan Yan <kyan@google.com> writes:

> Patchset v6 works for me with ath10k driver.  AQL does its job as
> expected and tests show very significant reduction in latency in
> congested environment. The txq stuck issue in patchset v4 got fixed.

Awesome! Thank you for testing!

> However, the device's total pending airtime count still underflows
> sometimes. Even though it doesn't cause apparent side effect, there is
> some issue with the pending airtime update and needs further
> debugging.

Huh, it *under*flows? That's... interesting. Cloned SKBs? Or maybe the
tx_time_est field is being set in some other place?

Could I get you to add a dump_stack() to the underflow test so we can
get an idea of where that happens?

-Toke


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [Make-wifi-fast] [PATCH v6 0/4] Add Airtime Queue Limits (AQL) to mac80211
  2019-11-07  6:14   ` Kan Yan
  (?)
  (?)
@ 2019-11-07 21:24   ` Dave Taht
  -1 siblings, 0 replies; 43+ messages in thread
From: Dave Taht @ 2019-11-07 21:24 UTC (permalink / raw)
  To: Kan Yan
  Cc: Toke Høiland-Jørgensen, Rajkumar Manoharan,
	Kevin Hayes, Make-Wifi-fast, linux-wireless, ath10k,
	John Crispin, Johannes Berg, Lorenzo Bianconi, Felix Fietkau

Kan Yan <kyan@google.com> writes:

> Patchset v6 works for me with ath10k driver.  AQL does its job as
> expected and tests show very significant reduction in latency in
> congested environment. The txq stuck issue in patchset v4 got fixed.
>
> However, the device's total pending airtime count still underflows
> sometimes. Even though it doesn't cause apparent side effect, there is
> some issue with the pending airtime update and needs further
> debugging.
>
> Regards,
> Kan

That's great to hear! I have been trying to clear some time to get on
this too (I have a HUGE number of fq_codel related updates on top of
this worth testing) but it's still looking a few weeks out.

>
>
> On Wed, Oct 23, 2019 at 2:59 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> 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-06
>>
>> Changelog:
>>
>> v6:
>>   - Fix sta lookup in ieee80211_report_used_skb().
>>   - Move call to ieee80211_sta_update_pending_airtime() to a bit later in
>>     __ieee80211_tx_status()
>> v5:
>>   - Add missing export of ieee80211_calc_rx_airtime() and make
>>     ieee80211_calc_tx_airtime_rate() static (kbuildbot).
>>   - Use skb_get_queue_mapping() to get the AC from the skb.
>>   - Take basic rate configuration for the BSS into account when calculating
>>     multicast rate.
>> v4:
>>   - Fix calculation that clamps the maximum airtime to fit into 10 bits
>>   - Incorporate Rich Brown's nits for the commit message in Kan's patch
>>   - Add fewer local variables to ieee80211_tx_dequeue()
>> v3:
>>   - Move the tx_time_est field so it's shared with ack_frame_id, and use units
>>     of 4us for the value stored in it.
>>   - Move the addition of the Ethernet header size into ieee80211_calc_expected_tx_airtime()
>> v2:
>>   - Integrate Kan's approach to airtime throttling.
>>   - Hopefully fix the cb struct alignment on big-endian architectures.
>>
>> ---
>>
>> Kan Yan (1):
>>       mac80211: Implement Airtime-based Queue Limit (AQL)
>>
>> Toke Høiland-Jørgensen (3):
>>       mac80211: Shrink the size of ack_frame_id 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     |   45 +++++
>>  net/mac80211/Makefile      |    3
>>  net/mac80211/airtime.c     |  390 ++++++++++++++++++++++++++++++++++++++++++++
>>  net/mac80211/cfg.c         |    2
>>  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      |   33 ++++
>>  net/mac80211/tx.c          |   69 +++++++-
>>  13 files changed, 709 insertions(+), 18 deletions(-)
>>  create mode 100644 net/mac80211/airtime.c
>>
> _______________________________________________
> Make-wifi-fast mailing list
> Make-wifi-fast@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/make-wifi-fast

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 1/4] mac80211: Shrink the size of ack_frame_id to make room for tx_time_est
  2019-10-23  9:59   ` Toke Høiland-Jørgensen
@ 2019-11-08 10:03     ` Johannes Berg
  -1 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:03 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen 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.
> 
> To get this, decrease the size of the ack_frame_id field to 6 bits, and
> lower the size of the ID space accordingly. This leaves 10 bits for use for
> tx_time_est, which is enough to store a maximum of 4096 us, if we shift the
> values so they become units of 4us.
> 

I've applied this as preparation.

johannes


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 1/4] mac80211: Shrink the size of ack_frame_id to make room for tx_time_est
@ 2019-11-08 10:03     ` Johannes Berg
  0 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:03 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen 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.
> 
> To get this, decrease the size of the ack_frame_id field to 6 bits, and
> lower the size of the ID space accordingly. This leaves 10 bits for use for
> tx_time_est, which is enough to store a maximum of 4096 us, if we shift the
> values so they become units of 4us.
> 

I've applied this as preparation.

johannes


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 2/4] mac80211: Import airtime calculation code from mt76
  2019-10-23  9:59   ` Toke Høiland-Jørgensen
@ 2019-11-08 10:07     ` Johannes Berg
  -1 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:07 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> Felix recently added code to calculate airtime of packets to the mt76
> driver. Import this into mac80211 so we can use it for airtime queue limit
> calculations 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.

Any way it could be exposed by mac80211 back to the drivers, perhaps, to
share it?

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

This is highly questionable ...

> +	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);

Why not

	cck = rate->flags & IEEE80211_RATE_MANDATORY_B;

I mean .. we know that IEEE80211_RATE_MANDATORY_B rates are exactly the
CCK rates, and that's not really going to change?

Alternatively, we could do

	cck = sband->band == NL80211_BAND_2GHZ &&
	      !(rate->flags & IEEE80211_RATE_ERP_G);

or even

	cck = rate->bitrate == 10 || rate->bitrate == 20 ||
	      rate->bitrate == 55 || rate->bitrate == 110;

:)

> +	default:
> +		WARN_ON_ONCE(1);

You can't do that in mac80211 either. That might be fine for mt76, but
mac80211 already supports HE.

johannes


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 2/4] mac80211: Import airtime calculation code from mt76
@ 2019-11-08 10:07     ` Johannes Berg
  0 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:07 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> Felix recently added code to calculate airtime of packets to the mt76
> driver. Import this into mac80211 so we can use it for airtime queue limit
> calculations 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.

Any way it could be exposed by mac80211 back to the drivers, perhaps, to
share it?

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

This is highly questionable ...

> +	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);

Why not

	cck = rate->flags & IEEE80211_RATE_MANDATORY_B;

I mean .. we know that IEEE80211_RATE_MANDATORY_B rates are exactly the
CCK rates, and that's not really going to change?

Alternatively, we could do

	cck = sband->band == NL80211_BAND_2GHZ &&
	      !(rate->flags & IEEE80211_RATE_ERP_G);

or even

	cck = rate->bitrate == 10 || rate->bitrate == 20 ||
	      rate->bitrate == 55 || rate->bitrate == 110;

:)

> +	default:
> +		WARN_ON_ONCE(1);

You can't do that in mac80211 either. That might be fine for mt76, but
mac80211 already supports HE.

johannes


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-10-23  9:59   ` Toke Høiland-Jørgensen
@ 2019-11-08 10:09     ` Johannes Berg
  -1 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:09 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
> 
>  
> +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))

Maybe add the STA/AC to the message?

johannes


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
@ 2019-11-08 10:09     ` Johannes Berg
  0 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:09 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
> 
>  
> +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))

Maybe add the STA/AC to the message?

johannes


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
  2019-10-23  9:59   ` Toke Høiland-Jørgensen
@ 2019-11-08 10:17     ` Johannes Berg
  -1 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:17 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
> 
> +	if (info->tx_time_est) {
> +		struct sta_info *sta = NULL, *s;
> +		struct rhlist_head *tmp;
> +
> +		rcu_read_lock();
> +
> +		for_each_sta_info(local, hdr->addr1, s, tmp) {
> +			/* skip wrong virtual interface */
> +			if (!ether_addr_equal(hdr->addr2, s->sdata->vif.addr))
> +				continue;
> +
> +			sta = s;
> +			break;
> +		}

I guess that is better than looking up the sdata and then using
sta_info_get(), but I think I'd like to see this wrapped into a function
(even if it's an inline) in sta_info.{c,h}.

> +		airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
> +							     skb->len);
> +		if (airtime) {
> +			/* We only have 10 bits in tx_time_est, so store airtime
> +			 * in increments of 4us and clamp the maximum to 2**12-1
> +			 */
> +			airtime = min_t(u32, airtime, 4095) & ~3U;
> +			info->tx_time_est = airtime >> 2;
> +			ieee80211_sta_update_pending_airtime(local, tx.sta,
> +							     txq->ac, airtime,
> +							     false);

I wonder if it'd be better to pass the shifted value to
ieee80211_sta_update_pending_airtime() to avoid all the shifting in all
places?

You could even store the shifted value in "aql_tx_pending" and
"aql_total_pending_airtime" etc., it's completely equivalent, and only
shift it out for people looking at debugfs.

johannes


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
@ 2019-11-08 10:17     ` Johannes Berg
  0 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:17 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
> 
> +	if (info->tx_time_est) {
> +		struct sta_info *sta = NULL, *s;
> +		struct rhlist_head *tmp;
> +
> +		rcu_read_lock();
> +
> +		for_each_sta_info(local, hdr->addr1, s, tmp) {
> +			/* skip wrong virtual interface */
> +			if (!ether_addr_equal(hdr->addr2, s->sdata->vif.addr))
> +				continue;
> +
> +			sta = s;
> +			break;
> +		}

I guess that is better than looking up the sdata and then using
sta_info_get(), but I think I'd like to see this wrapped into a function
(even if it's an inline) in sta_info.{c,h}.

> +		airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
> +							     skb->len);
> +		if (airtime) {
> +			/* We only have 10 bits in tx_time_est, so store airtime
> +			 * in increments of 4us and clamp the maximum to 2**12-1
> +			 */
> +			airtime = min_t(u32, airtime, 4095) & ~3U;
> +			info->tx_time_est = airtime >> 2;
> +			ieee80211_sta_update_pending_airtime(local, tx.sta,
> +							     txq->ac, airtime,
> +							     false);

I wonder if it'd be better to pass the shifted value to
ieee80211_sta_update_pending_airtime() to avoid all the shifting in all
places?

You could even store the shifted value in "aql_tx_pending" and
"aql_total_pending_airtime" etc., it's completely equivalent, and only
shift it out for people looking at debugfs.

johannes


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 2/4] mac80211: Import airtime calculation code from mt76
  2019-11-08 10:07     ` Johannes Berg
@ 2019-11-08 10:55       ` Toke Høiland-Jørgensen
  -1 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-08 10:55 UTC (permalink / raw)
  To: Johannes Berg
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> Felix recently added code to calculate airtime of packets to the mt76
>> driver. Import this into mac80211 so we can use it for airtime queue limit
>> calculations 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.
>
> Any way it could be exposed by mac80211 back to the drivers, perhaps, to
> share it?

Didn't I already export some of the functions? My intention was to do
that, certainly, and to patch mt76 to switch to using them once the
trees have converged...

>> 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.
>
> This is highly questionable ...
>
>> +	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);

Heh, yeah this did feel like a hack to me as well ;)

> Why not
>
> 	cck = rate->flags & IEEE80211_RATE_MANDATORY_B;
>
> I mean .. we know that IEEE80211_RATE_MANDATORY_B rates are exactly the
> CCK rates, and that's not really going to change?
>
> Alternatively, we could do
>
> 	cck = sband->band == NL80211_BAND_2GHZ &&
> 	      !(rate->flags & IEEE80211_RATE_ERP_G);
>
> or even
>
> 	cck = rate->bitrate == 10 || rate->bitrate == 20 ||
> 	      rate->bitrate == 55 || rate->bitrate == 110;

I am fine with either of those; I just wasn't sure what assumptions I
could actually make here. I guess I'll just pick one :)

>> +	default:
>> +		WARN_ON_ONCE(1);
>
> You can't do that in mac80211 either. That might be fine for mt76, but
> mac80211 already supports HE.

Good point, will fix.

-Toke


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 2/4] mac80211: Import airtime calculation code from mt76
@ 2019-11-08 10:55       ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-08 10:55 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> Felix recently added code to calculate airtime of packets to the mt76
>> driver. Import this into mac80211 so we can use it for airtime queue limit
>> calculations 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.
>
> Any way it could be exposed by mac80211 back to the drivers, perhaps, to
> share it?

Didn't I already export some of the functions? My intention was to do
that, certainly, and to patch mt76 to switch to using them once the
trees have converged...

>> 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.
>
> This is highly questionable ...
>
>> +	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);

Heh, yeah this did feel like a hack to me as well ;)

> Why not
>
> 	cck = rate->flags & IEEE80211_RATE_MANDATORY_B;
>
> I mean .. we know that IEEE80211_RATE_MANDATORY_B rates are exactly the
> CCK rates, and that's not really going to change?
>
> Alternatively, we could do
>
> 	cck = sband->band == NL80211_BAND_2GHZ &&
> 	      !(rate->flags & IEEE80211_RATE_ERP_G);
>
> or even
>
> 	cck = rate->bitrate == 10 || rate->bitrate == 20 ||
> 	      rate->bitrate == 55 || rate->bitrate == 110;

I am fine with either of those; I just wasn't sure what assumptions I
could actually make here. I guess I'll just pick one :)

>> +	default:
>> +		WARN_ON_ONCE(1);
>
> You can't do that in mac80211 either. That might be fine for mt76, but
> mac80211 already supports HE.

Good point, will fix.

-Toke


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-11-08 10:09     ` Johannes Berg
@ 2019-11-08 10:56       ` Toke Høiland-Jørgensen
  -1 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-08 10:56 UTC (permalink / raw)
  To: Johannes Berg
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>> 
>>  
>> +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))
>
> Maybe add the STA/AC to the message?

Can do. Any idea why we might be seeing underflows (as Kan reported)?

-Toke


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
@ 2019-11-08 10:56       ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-08 10:56 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>> 
>>  
>> +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))
>
> Maybe add the STA/AC to the message?

Can do. Any idea why we might be seeing underflows (as Kan reported)?

-Toke


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 2/4] mac80211: Import airtime calculation code from mt76
  2019-11-08 10:55       ` Toke Høiland-Jørgensen
@ 2019-11-08 10:57         ` Johannes Berg
  -1 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:57 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

On Fri, 2019-11-08 at 11:55 +0100, Toke Høiland-Jørgensen wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
> > > From: Toke Høiland-Jørgensen <toke@redhat.com>
> > > 
> > > Felix recently added code to calculate airtime of packets to the mt76
> > > driver. Import this into mac80211 so we can use it for airtime queue limit
> > > calculations 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.
> > 
> > Any way it could be exposed by mac80211 back to the drivers, perhaps, to
> > share it?
> 
> Didn't I already export some of the functions? My intention was to do
> that, certainly, and to patch mt76 to switch to using them once the
> trees have converged...

Yeah, I think you did. Sounds good.

johannes


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 2/4] mac80211: Import airtime calculation code from mt76
@ 2019-11-08 10:57         ` Johannes Berg
  0 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:57 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

On Fri, 2019-11-08 at 11:55 +0100, Toke Høiland-Jørgensen wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
> > > From: Toke Høiland-Jørgensen <toke@redhat.com>
> > > 
> > > Felix recently added code to calculate airtime of packets to the mt76
> > > driver. Import this into mac80211 so we can use it for airtime queue limit
> > > calculations 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.
> > 
> > Any way it could be exposed by mac80211 back to the drivers, perhaps, to
> > share it?
> 
> Didn't I already export some of the functions? My intention was to do
> that, certainly, and to patch mt76 to switch to using them once the
> trees have converged...

Yeah, I think you did. Sounds good.

johannes


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-11-08 10:56       ` Toke Høiland-Jørgensen
@ 2019-11-08 10:59         ` Johannes Berg
  -1 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:59 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

On Fri, 2019-11-08 at 11:56 +0100, Toke Høiland-Jørgensen wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
> > >  
> > > +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))
> > 
> > Maybe add the STA/AC to the message?
> 
> Can do. Any idea why we might be seeing underflows (as Kan reported)?

No, I really have no idea. The shifting looked OK to me, though I didn't
review it carefully enough to say I've really looked at all places ...

johannes


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
@ 2019-11-08 10:59         ` Johannes Berg
  0 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 10:59 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

On Fri, 2019-11-08 at 11:56 +0100, Toke Høiland-Jørgensen wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
> > >  
> > > +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))
> > 
> > Maybe add the STA/AC to the message?
> 
> Can do. Any idea why we might be seeing underflows (as Kan reported)?

No, I really have no idea. The shifting looked OK to me, though I didn't
review it carefully enough to say I've really looked at all places ...

johannes


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
  2019-11-08 10:17     ` Johannes Berg
@ 2019-11-08 11:01       ` Toke Høiland-Jørgensen
  -1 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-08 11:01 UTC (permalink / raw)
  To: Johannes Berg
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>> 
>> +	if (info->tx_time_est) {
>> +		struct sta_info *sta = NULL, *s;
>> +		struct rhlist_head *tmp;
>> +
>> +		rcu_read_lock();
>> +
>> +		for_each_sta_info(local, hdr->addr1, s, tmp) {
>> +			/* skip wrong virtual interface */
>> +			if (!ether_addr_equal(hdr->addr2, s->sdata->vif.addr))
>> +				continue;
>> +
>> +			sta = s;
>> +			break;
>> +		}
>
> I guess that is better than looking up the sdata and then using
> sta_info_get(), but I think I'd like to see this wrapped into a function
> (even if it's an inline) in sta_info.{c,h}.

OK, can do.

>> +		airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>> +							     skb->len);
>> +		if (airtime) {
>> +			/* We only have 10 bits in tx_time_est, so store airtime
>> +			 * in increments of 4us and clamp the maximum to 2**12-1
>> +			 */
>> +			airtime = min_t(u32, airtime, 4095) & ~3U;
>> +			info->tx_time_est = airtime >> 2;
>> +			ieee80211_sta_update_pending_airtime(local, tx.sta,
>> +							     txq->ac, airtime,
>> +							     false);
>
> I wonder if it'd be better to pass the shifted value to
> ieee80211_sta_update_pending_airtime() to avoid all the shifting in all
> places?
>
> You could even store the shifted value in "aql_tx_pending" and
> "aql_total_pending_airtime" etc., it's completely equivalent, and only
> shift it out for people looking at debugfs.

My reasoning for doing it this way was that we have another set of APIs
dealing with airtime which doesn't do any shifting; so keeping the APIs
in the same unit (straight airtime) seemed less confusing.

We could add (inline) setter and getter functions for the tx_time_est
field instead to avoid sprinkling shifts all over the place? :)

-Toke


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
@ 2019-11-08 11:01       ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-08 11:01 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>> 
>> +	if (info->tx_time_est) {
>> +		struct sta_info *sta = NULL, *s;
>> +		struct rhlist_head *tmp;
>> +
>> +		rcu_read_lock();
>> +
>> +		for_each_sta_info(local, hdr->addr1, s, tmp) {
>> +			/* skip wrong virtual interface */
>> +			if (!ether_addr_equal(hdr->addr2, s->sdata->vif.addr))
>> +				continue;
>> +
>> +			sta = s;
>> +			break;
>> +		}
>
> I guess that is better than looking up the sdata and then using
> sta_info_get(), but I think I'd like to see this wrapped into a function
> (even if it's an inline) in sta_info.{c,h}.

OK, can do.

>> +		airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>> +							     skb->len);
>> +		if (airtime) {
>> +			/* We only have 10 bits in tx_time_est, so store airtime
>> +			 * in increments of 4us and clamp the maximum to 2**12-1
>> +			 */
>> +			airtime = min_t(u32, airtime, 4095) & ~3U;
>> +			info->tx_time_est = airtime >> 2;
>> +			ieee80211_sta_update_pending_airtime(local, tx.sta,
>> +							     txq->ac, airtime,
>> +							     false);
>
> I wonder if it'd be better to pass the shifted value to
> ieee80211_sta_update_pending_airtime() to avoid all the shifting in all
> places?
>
> You could even store the shifted value in "aql_tx_pending" and
> "aql_total_pending_airtime" etc., it's completely equivalent, and only
> shift it out for people looking at debugfs.

My reasoning for doing it this way was that we have another set of APIs
dealing with airtime which doesn't do any shifting; so keeping the APIs
in the same unit (straight airtime) seemed less confusing.

We could add (inline) setter and getter functions for the tx_time_est
field instead to avoid sprinkling shifts all over the place? :)

-Toke


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
  2019-11-08 11:01       ` Toke Høiland-Jørgensen
@ 2019-11-08 11:05         ` Johannes Berg
  -1 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 11:05 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

On Fri, 2019-11-08 at 12:01 +0100, Toke Høiland-Jørgensen wrote:
> 
> My reasoning for doing it this way was that we have another set of APIs
> dealing with airtime which doesn't do any shifting; so keeping the APIs
> in the same unit (straight airtime) seemed less confusing.

Fair enough.

> We could add (inline) setter and getter functions for the tx_time_est
> field instead to avoid sprinkling shifts all over the place? :)

It doesn't really bother me that much, but yeah, perhaps that's easier.

johannes


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
@ 2019-11-08 11:05         ` Johannes Berg
  0 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 11:05 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

On Fri, 2019-11-08 at 12:01 +0100, Toke Høiland-Jørgensen wrote:
> 
> My reasoning for doing it this way was that we have another set of APIs
> dealing with airtime which doesn't do any shifting; so keeping the APIs
> in the same unit (straight airtime) seemed less confusing.

Fair enough.

> We could add (inline) setter and getter functions for the tx_time_est
> field instead to avoid sprinkling shifts all over the place? :)

It doesn't really bother me that much, but yeah, perhaps that's easier.

johannes


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-11-08 10:59         ` Johannes Berg
@ 2019-11-08 11:10           ` Toke Høiland-Jørgensen
  -1 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-08 11:10 UTC (permalink / raw)
  To: Johannes Berg
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2019-11-08 at 11:56 +0100, Toke Høiland-Jørgensen wrote:
>> Johannes Berg <johannes@sipsolutions.net> writes:
>> 
>> > On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>> > >  
>> > > +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))
>> > 
>> > Maybe add the STA/AC to the message?
>> 
>> Can do. Any idea why we might be seeing underflows (as Kan reported)?
>
> No, I really have no idea. The shifting looked OK to me, though I didn't
> review it carefully enough to say I've really looked at all places ...

Right, bugger. I was thinking maybe there's a case where skbs can be
cloned (and retain the tx_time_est field) and then released twice? Or
maybe somewhere that steps on the skb->cb field in some other way?
Couldn't find anything obvious on a first perusal of the TX path code,
but maybe you could think of something?

Otherwise I guess we'll be forced to go and do some actual,
old-fashioned debugging ;)

-Toke


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
@ 2019-11-08 11:10           ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-08 11:10 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2019-11-08 at 11:56 +0100, Toke Høiland-Jørgensen wrote:
>> Johannes Berg <johannes@sipsolutions.net> writes:
>> 
>> > On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>> > >  
>> > > +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))
>> > 
>> > Maybe add the STA/AC to the message?
>> 
>> Can do. Any idea why we might be seeing underflows (as Kan reported)?
>
> No, I really have no idea. The shifting looked OK to me, though I didn't
> review it carefully enough to say I've really looked at all places ...

Right, bugger. I was thinking maybe there's a case where skbs can be
cloned (and retain the tx_time_est field) and then released twice? Or
maybe somewhere that steps on the skb->cb field in some other way?
Couldn't find anything obvious on a first perusal of the TX path code,
but maybe you could think of something?

Otherwise I guess we'll be forced to go and do some actual,
old-fashioned debugging ;)

-Toke


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-11-08 11:10           ` Toke Høiland-Jørgensen
@ 2019-11-08 11:17             ` Johannes Berg
  -1 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 11:17 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: linux-wireless, make-wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Kan Yan, Rajkumar Manoharan,
	Kevin Hayes

On Fri, 2019-11-08 at 12:10 +0100, Toke Høiland-Jørgensen wrote:

> Right, bugger. I was thinking maybe there's a case where skbs can be
> cloned (and retain the tx_time_est field) and then released twice? 

They could be cloned, but I don't see how that'd be while *inside* the
stack and then they get reported twice - unless the driver did something
like that?

I mean, TCP surely does that for example, but it's before we even get to
mac80211.

> Or
> maybe somewhere that steps on the skb->cb field in some other way?
> Couldn't find anything obvious on a first perusal of the TX path code,
> but maybe you could think of something?

No, sorry. But I also didn't actually look at the driver at all.

> Otherwise I guess we'll be forced to go and do some actual,
> old-fashioned debugging ;)

:)

johannes


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
@ 2019-11-08 11:17             ` Johannes Berg
  0 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2019-11-08 11:17 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Kan Yan, Rajkumar Manoharan, Kevin Hayes, make-wifi-fast,
	linux-wireless, ath10k, John Crispin, Lorenzo Bianconi,
	Felix Fietkau

On Fri, 2019-11-08 at 12:10 +0100, Toke Høiland-Jørgensen wrote:

> Right, bugger. I was thinking maybe there's a case where skbs can be
> cloned (and retain the tx_time_est field) and then released twice? 

They could be cloned, but I don't see how that'd be while *inside* the
stack and then they get reported twice - unless the driver did something
like that?

I mean, TCP surely does that for example, but it's before we even get to
mac80211.

> Or
> maybe somewhere that steps on the skb->cb field in some other way?
> Couldn't find anything obvious on a first perusal of the TX path code,
> but maybe you could think of something?

No, sorry. But I also didn't actually look at the driver at all.

> Otherwise I guess we'll be forced to go and do some actual,
> old-fashioned debugging ;)

:)

johannes


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-11-08 11:17             ` Johannes Berg
@ 2019-11-09  1:22               ` Kan Yan
  -1 siblings, 0 replies; 43+ messages in thread
From: Kan Yan @ 2019-11-09  1:22 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Toke Høiland-Jørgensen, linux-wireless, Make-Wifi-fast,
	ath10k, John Crispin, Lorenzo Bianconi, Felix Fietkau,
	Rajkumar Manoharan, Kevin Hayes

It is most likely just insufficient locking. active_txq_lock is per
AC, can't protect local->aql_total_pending_airtime against racing
conditions:
void ieee80211_sta_update_pending_airtime(...)
{
        spin_lock_bh(&local->active_txq_lock[ac]);
        ...
        local->aql_total_pending_airtime -= tx_airtime;
        ...
        spin_unlock_bh(&local->active_txq_lock[ac]);
}

After changing it to atomic_t, no more aql_total_pending_airtime
underflow so far :). Using atomic operation should also help reduce
CPU overhead due to lock contention, as
ieee80211_sta_update_pending_airtime() is often called from the tx
completion routine triggered by interrupts, often in a different core
than where __ieee80211_schedule_txq() is running.

I will post a new version a bit later if the test goes well.

Regards,
Kan


On Fri, Nov 8, 2019 at 3:17 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Fri, 2019-11-08 at 12:10 +0100, Toke Høiland-Jørgensen wrote:
>
> > Right, bugger. I was thinking maybe there's a case where skbs can be
> > cloned (and retain the tx_time_est field) and then released twice?
>
> They could be cloned, but I don't see how that'd be while *inside* the
> stack and then they get reported twice - unless the driver did something
> like that?
>
> I mean, TCP surely does that for example, but it's before we even get to
> mac80211.
>
> > Or
> > maybe somewhere that steps on the skb->cb field in some other way?
> > Couldn't find anything obvious on a first perusal of the TX path code,
> > but maybe you could think of something?
>
> No, sorry. But I also didn't actually look at the driver at all.
>
> > Otherwise I guess we'll be forced to go and do some actual,
> > old-fashioned debugging ;)
>
> :)
>
> johannes
>

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
@ 2019-11-09  1:22               ` Kan Yan
  0 siblings, 0 replies; 43+ messages in thread
From: Kan Yan @ 2019-11-09  1:22 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Rajkumar Manoharan, Kevin Hayes, Make-Wifi-fast,
	Toke Høiland-Jørgensen, linux-wireless, ath10k,
	John Crispin, Lorenzo Bianconi, Felix Fietkau

It is most likely just insufficient locking. active_txq_lock is per
AC, can't protect local->aql_total_pending_airtime against racing
conditions:
void ieee80211_sta_update_pending_airtime(...)
{
        spin_lock_bh(&local->active_txq_lock[ac]);
        ...
        local->aql_total_pending_airtime -= tx_airtime;
        ...
        spin_unlock_bh(&local->active_txq_lock[ac]);
}

After changing it to atomic_t, no more aql_total_pending_airtime
underflow so far :). Using atomic operation should also help reduce
CPU overhead due to lock contention, as
ieee80211_sta_update_pending_airtime() is often called from the tx
completion routine triggered by interrupts, often in a different core
than where __ieee80211_schedule_txq() is running.

I will post a new version a bit later if the test goes well.

Regards,
Kan


On Fri, Nov 8, 2019 at 3:17 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Fri, 2019-11-08 at 12:10 +0100, Toke Høiland-Jørgensen wrote:
>
> > Right, bugger. I was thinking maybe there's a case where skbs can be
> > cloned (and retain the tx_time_est field) and then released twice?
>
> They could be cloned, but I don't see how that'd be while *inside* the
> stack and then they get reported twice - unless the driver did something
> like that?
>
> I mean, TCP surely does that for example, but it's before we even get to
> mac80211.
>
> > Or
> > maybe somewhere that steps on the skb->cb field in some other way?
> > Couldn't find anything obvious on a first perusal of the TX path code,
> > but maybe you could think of something?
>
> No, sorry. But I also didn't actually look at the driver at all.
>
> > Otherwise I guess we'll be forced to go and do some actual,
> > old-fashioned debugging ;)
>
> :)
>
> johannes
>

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

^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-11-09  1:22               ` Kan Yan
@ 2019-11-09 11:22                 ` Toke Høiland-Jørgensen
  -1 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-09 11:22 UTC (permalink / raw)
  To: Kan Yan, Johannes Berg
  Cc: linux-wireless, Make-Wifi-fast, ath10k, John Crispin,
	Lorenzo Bianconi, Felix Fietkau, Rajkumar Manoharan, Kevin Hayes

Kan Yan <kyan@google.com> writes:

> It is most likely just insufficient locking. active_txq_lock is per
> AC, can't protect local->aql_total_pending_airtime against racing
> conditions:
> void ieee80211_sta_update_pending_airtime(...)
> {
>         spin_lock_bh(&local->active_txq_lock[ac]);
>         ...
>         local->aql_total_pending_airtime -= tx_airtime;
>         ...
>         spin_unlock_bh(&local->active_txq_lock[ac]);
> }

Ohh, right; didn't even realise those were not per-AC as well...

> After changing it to atomic_t, no more aql_total_pending_airtime
> underflow so far :). Using atomic operation should also help reduce
> CPU overhead due to lock contention, as
> ieee80211_sta_update_pending_airtime() is often called from the tx
> completion routine triggered by interrupts, often in a different core
> than where __ieee80211_schedule_txq() is running.
>
> I will post a new version a bit later if the test goes well.

Awesome! :)

-Toke


^ permalink raw reply	[flat|nested] 43+ messages in thread
* Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
@ 2019-11-09 11:22                 ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 43+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-09 11:22 UTC (permalink / raw)
  To: Kan Yan, Johannes Berg
  Cc: Rajkumar Manoharan, Kevin Hayes, Make-Wifi-fast, linux-wireless,
	ath10k, John Crispin, Lorenzo Bianconi, Felix Fietkau

Kan Yan <kyan@google.com> writes:

> It is most likely just insufficient locking. active_txq_lock is per
> AC, can't protect local->aql_total_pending_airtime against racing
> conditions:
> void ieee80211_sta_update_pending_airtime(...)
> {
>         spin_lock_bh(&local->active_txq_lock[ac]);
>         ...
>         local->aql_total_pending_airtime -= tx_airtime;
>         ...
>         spin_unlock_bh(&local->active_txq_lock[ac]);
> }

Ohh, right; didn't even realise those were not per-AC as well...

> After changing it to atomic_t, no more aql_total_pending_airtime
> underflow so far :). Using atomic operation should also help reduce
> CPU overhead due to lock contention, as
> ieee80211_sta_update_pending_airtime() is often called from the tx
> completion routine triggered by interrupts, often in a different core
> than where __ieee80211_schedule_txq() is running.
>
> I will post a new version a bit later if the test goes well.

Awesome! :)

-Toke


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

^ permalink raw reply	[flat|nested] 43+ messages in thread
end of thread, other threads:[~2019-11-09 11:22 UTC | newest]

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-23  9:58 [PATCH v6 0/4] Add Airtime Queue Limits (AQL) to mac80211 Toke Høiland-Jørgensen
2019-10-23  9:58 ` Toke Høiland-Jørgensen
2019-10-23  9:59 ` [PATCH v6 1/4] mac80211: Shrink the size of ack_frame_id to make room for tx_time_est Toke Høiland-Jørgensen
2019-10-23  9:59   ` Toke Høiland-Jørgensen
2019-11-08 10:03   ` Johannes Berg
2019-11-08 10:03     ` Johannes Berg
2019-10-23  9:59 ` [PATCH v6 2/4] mac80211: Import airtime calculation code from mt76 Toke Høiland-Jørgensen
2019-10-23  9:59   ` Toke Høiland-Jørgensen
2019-11-08 10:07   ` Johannes Berg
2019-11-08 10:07     ` Johannes Berg
2019-11-08 10:55     ` Toke Høiland-Jørgensen
2019-11-08 10:55       ` Toke Høiland-Jørgensen
2019-11-08 10:57       ` Johannes Berg
2019-11-08 10:57         ` Johannes Berg
2019-10-23  9:59 ` [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL) Toke Høiland-Jørgensen
2019-10-23  9:59   ` Toke Høiland-Jørgensen
2019-11-08 10:09   ` Johannes Berg
2019-11-08 10:09     ` Johannes Berg
2019-11-08 10:56     ` Toke Høiland-Jørgensen
2019-11-08 10:56       ` Toke Høiland-Jørgensen
2019-11-08 10:59       ` Johannes Berg
2019-11-08 10:59         ` Johannes Berg
2019-11-08 11:10         ` Toke Høiland-Jørgensen
2019-11-08 11:10           ` Toke Høiland-Jørgensen
2019-11-08 11:17           ` Johannes Berg
2019-11-08 11:17             ` Johannes Berg
2019-11-09  1:22             ` Kan Yan
2019-11-09  1:22               ` Kan Yan
2019-11-09 11:22               ` Toke Høiland-Jørgensen
2019-11-09 11:22                 ` Toke Høiland-Jørgensen
2019-10-23  9:59 ` [PATCH v6 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue Toke Høiland-Jørgensen
2019-10-23  9:59   ` Toke Høiland-Jørgensen
2019-11-08 10:17   ` Johannes Berg
2019-11-08 10:17     ` Johannes Berg
2019-11-08 11:01     ` Toke Høiland-Jørgensen
2019-11-08 11:01       ` Toke Høiland-Jørgensen
2019-11-08 11:05       ` Johannes Berg
2019-11-08 11:05         ` Johannes Berg
2019-11-07  6:14 ` [PATCH v6 0/4] Add Airtime Queue Limits (AQL) to mac80211 Kan Yan
2019-11-07  6:14   ` Kan Yan
2019-11-07 10:55   ` Toke Høiland-Jørgensen
2019-11-07 10:55     ` Toke Høiland-Jørgensen
2019-11-07 21:24   ` [Make-wifi-fast] " Dave Taht

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.