All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v7 0/2] Implement Airtime-based Queue Limit (AQL)
@ 2019-11-12  2:11 Kan Yan
  2019-11-12  2:11 ` [PATCH v7 1/2] mac80211: " Kan Yan
  2019-11-12  2:11 ` [PATCH v7 2/2] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue Kan Yan
  0 siblings, 2 replies; 9+ messages in thread
From: Kan Yan @ 2019-11-12  2:11 UTC (permalink / raw)
  To: johannes
  Cc: linux-wireless, make-wifi-fast, toke, nbd, yiboz, john, lorenzo,
	rmanohar, kevinhayes, Kan Yan

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

This patch series has been tested on QCA9984 plaform.

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

Changelog:

v7:
  - Fix aql_total_pending_airtime underflow due to insufficient locking.

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

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

Toke Høiland-Jørgensen (1):
  mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue

 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        | 10 ++++-
 net/mac80211/sta_info.c    | 40 +++++++++++++++++++
 net/mac80211/sta_info.h    |  8 ++++
 net/mac80211/status.c      | 33 ++++++++++++++++
 net/mac80211/tx.c          | 68 +++++++++++++++++++++++++++++++--
 10 files changed, 289 insertions(+), 14 deletions(-)

-- 
2.24.0.rc1.363.gb1bccd3e3d-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH v7 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-11-12  2:11 [PATCH v7 0/2] Implement Airtime-based Queue Limit (AQL) Kan Yan
@ 2019-11-12  2:11 ` Kan Yan
  2019-11-12 11:47   ` Toke Høiland-Jørgensen
  2019-11-12  2:11 ` [PATCH v7 2/2] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue Kan Yan
  1 sibling, 1 reply; 9+ messages in thread
From: Kan Yan @ 2019-11-12  2:11 UTC (permalink / raw)
  To: johannes
  Cc: linux-wireless, make-wifi-fast, toke, nbd, yiboz, john, lorenzo,
	rmanohar, kevinhayes, Kan Yan

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

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

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

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

Signed-off-by: Kan Yan <kyan@google.com>
[ Toke: Keep API to set pending airtime internal, fix nits in commit msg ]
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/cfg80211.h     |  7 ++++
 include/net/mac80211.h     | 12 ++++++
 net/mac80211/debugfs.c     | 78 ++++++++++++++++++++++++++++++++++++++
 net/mac80211/debugfs_sta.c | 43 ++++++++++++++++-----
 net/mac80211/ieee80211_i.h |  4 ++
 net/mac80211/main.c        | 10 ++++-
 net/mac80211/sta_info.c    | 40 +++++++++++++++++++
 net/mac80211/sta_info.h    |  8 ++++
 net/mac80211/tx.c          | 47 +++++++++++++++++++++--
 9 files changed, 235 insertions(+), 14 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 4ab2c49423dc..15f9f04de149 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2602,6 +2602,13 @@ enum wiphy_params_flags {
 
 #define IEEE80211_DEFAULT_AIRTIME_WEIGHT	256
 
+/* The per TXQ device queue limit in airtime */
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L	4000
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H	8000
+
+/* The per interface airtime threshold to switch to lower queue limit */
+#define IEEE80211_AQL_THRESHOLD			24000
+
 /**
  * struct cfg80211_pmksa - PMK Security Association
  *
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 4319596ef472..269be699e89c 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5558,6 +5558,18 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
 void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
 				    u32 tx_airtime, u32 rx_airtime);
 
+/**
+ * ieee80211_txq_airtime_check - check if a txq can send frame to device
+ *
+ * @hw: pointer obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Return true if the AQL's airtime limit has not been reached and the txq can
+ * continue to send more packets to the device. Otherwise return false.
+ */
+bool
+ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
+
 /**
  * ieee80211_iter_keys - iterate keys programmed into the device
  * @hw: pointer obtained from ieee80211_alloc_hw()
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 568b3b276931..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..0185e6e5e5d1 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -197,10 +197,12 @@ static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
 {
 	struct sta_info *sta = file->private_data;
 	struct ieee80211_local *local = sta->sdata->local;
-	size_t bufsz = 200;
+	size_t bufsz = 400;
 	char *buf = kzalloc(bufsz, GFP_KERNEL), *p = buf;
 	u64 rx_airtime = 0, tx_airtime = 0;
 	s64 deficit[IEEE80211_NUM_ACS];
+	u32 q_depth[IEEE80211_NUM_ACS];
+	u32 q_limit_l[IEEE80211_NUM_ACS], q_limit_h[IEEE80211_NUM_ACS];
 	ssize_t rv;
 	int ac;
 
@@ -212,19 +214,22 @@ static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
 		rx_airtime += sta->airtime[ac].rx_airtime;
 		tx_airtime += sta->airtime[ac].tx_airtime;
 		deficit[ac] = sta->airtime[ac].deficit;
+		q_limit_l[ac] = sta->airtime[ac].aql_limit_low;
+		q_limit_h[ac] = sta->airtime[ac].aql_limit_high;
 		spin_unlock_bh(&local->active_txq_lock[ac]);
+		q_depth[ac] = atomic_read(&sta->airtime[ac].aql_tx_pending);
 	}
 
 	p += scnprintf(p, bufsz + buf - p,
 		"RX: %llu us\nTX: %llu us\nWeight: %u\n"
-		"Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n",
-		rx_airtime,
-		tx_airtime,
-		sta->airtime_weight,
-		deficit[0],
-		deficit[1],
-		deficit[2],
-		deficit[3]);
+		"Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n"
+		"Q depth: VO: %u us VI: %u us BE: %u us BK: %u us\n"
+		"Q limit[low/high]: VO: %u/%u VI: %u/%u BE: %u/%u BK: %u/%u\n",
+		rx_airtime, tx_airtime, sta->airtime_weight,
+		deficit[0], deficit[1], deficit[2], deficit[3],
+		q_depth[0], q_depth[1], q_depth[2], q_depth[3],
+		q_limit_l[0], q_limit_h[0], q_limit_l[1], q_limit_h[1],
+		q_limit_l[2], q_limit_h[2], q_limit_l[3], q_limit_h[3]),
 
 	rv = simple_read_from_buffer(userbuf, count, ppos, buf, p - buf);
 	kfree(buf);
@@ -236,7 +241,25 @@ static ssize_t sta_airtime_write(struct file *file, const char __user *userbuf,
 {
 	struct sta_info *sta = file->private_data;
 	struct ieee80211_local *local = sta->sdata->local;
-	int ac;
+	u32 ac, q_limit_l, q_limit_h;
+	char _buf[100] = {}, *buf = _buf;
+
+	if (count > sizeof(_buf))
+		return -EINVAL;
+
+	if (copy_from_user(buf, userbuf, count))
+		return -EFAULT;
+
+	buf[sizeof(_buf) - 1] = '\0';
+	if (sscanf(buf, "queue limit %u %u %u", &ac, &q_limit_l, &q_limit_h)
+	    != 3)
+		return -EINVAL;
+
+	if (ac >= IEEE80211_NUM_ACS)
+		return -EINVAL;
+
+	sta->airtime[ac].aql_limit_low = q_limit_l;
+	sta->airtime[ac].aql_limit_high = q_limit_h;
 
 	for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
 		spin_lock_bh(&local->active_txq_lock[ac]);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 225ea4e3cd76..ad15b3be8bb3 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1142,6 +1142,10 @@ struct ieee80211_local {
 	u16 schedule_round[IEEE80211_NUM_ACS];
 
 	u16 airtime_flags;
+	u32 aql_txq_limit_low[IEEE80211_NUM_ACS];
+	u32 aql_txq_limit_high[IEEE80211_NUM_ACS];
+	u32 aql_threshold;
+	atomic_t aql_total_pending_airtime;
 
 	const struct ieee80211_ops *ops;
 
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index aba094b4ccfc..071ea92a3748 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -667,8 +667,16 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
 		INIT_LIST_HEAD(&local->active_txqs[i]);
 		spin_lock_init(&local->active_txq_lock[i]);
+		local->aql_txq_limit_low[i] = IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L;
+		local->aql_txq_limit_high[i] =
+			IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H;
 	}
-	local->airtime_flags = AIRTIME_USE_TX | AIRTIME_USE_RX;
+
+	local->airtime_flags = AIRTIME_USE_TX |
+			       AIRTIME_USE_RX |
+			       AIRTIME_USE_AQL;
+	local->aql_threshold = IEEE80211_AQL_THRESHOLD;
+	atomic_set(&local->aql_total_pending_airtime, 0);
 
 	INIT_LIST_HEAD(&local->chanctx_list);
 	mutex_init(&local->chanctx_mtx);
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index bd11fef2139f..a76e935a543a 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -396,6 +396,9 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 		skb_queue_head_init(&sta->ps_tx_buf[i]);
 		skb_queue_head_init(&sta->tx_filtered[i]);
 		sta->airtime[i].deficit = sta->airtime_weight;
+		atomic_set(&sta->airtime[i].aql_tx_pending, 0);
+		sta->airtime[i].aql_limit_low = local->aql_txq_limit_low[i];
+		sta->airtime[i].aql_limit_high = local->aql_txq_limit_high[i];
 	}
 
 	for (i = 0; i < IEEE80211_NUM_TIDS; i++)
@@ -1893,6 +1896,43 @@ 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)
+{
+	if (!tx_completed) {
+		if (sta)
+			atomic_add(tx_airtime,
+				   &sta->airtime[ac].aql_tx_pending);
+
+		atomic_add(tx_airtime, &local->aql_total_pending_airtime);
+		return;
+	}
+
+	if (sta) {
+		if (WARN_ONCE(atomic_read(&sta->airtime[ac].aql_tx_pending) <
+			      tx_airtime,
+			      "STA %pM AC %d txq pending airtime underflow: %u, %u",
+			      sta->addr, ac,
+			      atomic_read(&sta->airtime[ac].aql_tx_pending),
+			      tx_airtime))
+			atomic_set(&sta->airtime[ac].aql_tx_pending, 0);
+		else
+			atomic_sub(tx_airtime,
+				   &sta->airtime[ac].aql_tx_pending);
+	}
+
+	if (WARN_ONCE(atomic_read(&local->aql_total_pending_airtime) <
+		      tx_airtime,
+		      "Device %s AC %d pending airtime underflow: %u, %u",
+		      wiphy_name(local->hw.wiphy), ac,
+		      atomic_read(&local->aql_total_pending_airtime),
+		      tx_airtime))
+		atomic_set(&local->aql_total_pending_airtime, 0);
+	else
+		atomic_sub(tx_airtime, &local->aql_total_pending_airtime);
+}
+
 int sta_info_move_state(struct sta_info *sta,
 			enum ieee80211_sta_state new_state)
 {
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 369c2dddce52..c8823bd0e55f 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -127,13 +127,21 @@ enum ieee80211_agg_stop_reason {
 /* Debugfs flags to enable/disable use of RX/TX airtime in scheduler */
 #define AIRTIME_USE_TX		BIT(0)
 #define AIRTIME_USE_RX		BIT(1)
+#define AIRTIME_USE_AQL		BIT(2)
 
 struct airtime_info {
 	u64 rx_airtime;
 	u64 tx_airtime;
 	s64 deficit;
+	atomic_t aql_tx_pending; /* Estimated airtime for frames pending */
+	u32 aql_limit_low;
+	u32 aql_limit_high;
 };
 
+void ieee80211_sta_update_pending_airtime(struct ieee80211_local *local,
+					  struct sta_info *sta, u8 ac,
+					  u16 tx_airtime, bool tx_completed);
+
 struct sta_info;
 
 /**
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 238c47be5fe4..f53d56ef535a 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3672,7 +3672,8 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
 	struct ieee80211_txq *ret = NULL;
-	struct txq_info *txqi = NULL;
+	struct txq_info *txqi = NULL, *head = NULL;
+	bool found_eligible_txq = false;
 
 	spin_lock_bh(&local->active_txq_lock[ac]);
 
@@ -3683,13 +3684,26 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
 	if (!txqi)
 		goto out;
 
+	if (txqi == head && !found_eligible_txq)
+		goto out;
+
+	if (!head)
+		head = txqi;
+
 	if (txqi->txq.sta) {
 		struct sta_info *sta = container_of(txqi->txq.sta,
-						struct sta_info, sta);
+						    struct sta_info, sta);
+		bool aql_check = ieee80211_txq_airtime_check(hw, &txqi->txq);
+		s64 deficit = sta->airtime[txqi->txq.ac].deficit;
+
+		if (aql_check)
+			found_eligible_txq = true;
 
-		if (sta->airtime[txqi->txq.ac].deficit < 0) {
+		if (deficit < 0)
 			sta->airtime[txqi->txq.ac].deficit +=
 				sta->airtime_weight;
+
+		if (deficit < 0 || !aql_check) {
 			list_move_tail(&txqi->schedule_order,
 				       &local->active_txqs[txqi->txq.ac]);
 			goto begin;
@@ -3743,6 +3757,33 @@ void __ieee80211_schedule_txq(struct ieee80211_hw *hw,
 }
 EXPORT_SYMBOL(__ieee80211_schedule_txq);
 
+bool ieee80211_txq_airtime_check(struct ieee80211_hw *hw,
+				 struct ieee80211_txq *txq)
+{
+	struct sta_info *sta;
+	struct ieee80211_local *local = hw_to_local(hw);
+
+	if (!(local->airtime_flags & AIRTIME_USE_AQL))
+		return true;
+
+	if (!txq->sta)
+		return true;
+
+	sta = container_of(txq->sta, struct sta_info, sta);
+	if (atomic_read(&sta->airtime[txq->ac].aql_tx_pending) <
+	    sta->airtime[txq->ac].aql_limit_low)
+		return true;
+
+	if (atomic_read(&local->aql_total_pending_airtime) <
+	    local->aql_threshold &&
+	    atomic_read(&sta->airtime[txq->ac].aql_tx_pending) <
+	    sta->airtime[txq->ac].aql_limit_high)
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL(ieee80211_txq_airtime_check);
+
 bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
 				struct ieee80211_txq *txq)
 {
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH v7 2/2] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
  2019-11-12  2:11 [PATCH v7 0/2] Implement Airtime-based Queue Limit (AQL) Kan Yan
  2019-11-12  2:11 ` [PATCH v7 1/2] mac80211: " Kan Yan
@ 2019-11-12  2:11 ` Kan Yan
  2019-11-12 22:42     ` kbuild test robot
  1 sibling, 1 reply; 9+ messages in thread
From: Kan Yan @ 2019-11-12  2:11 UTC (permalink / raw)
  To: johannes
  Cc: linux-wireless, make-wifi-fast, toke, nbd, yiboz, john, lorenzo,
	rmanohar, kevinhayes

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 f53d56ef535a..583211ca670d 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3549,6 +3549,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);
 
@@ -3659,6 +3662,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:
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-11-12  2:11 ` [PATCH v7 1/2] mac80211: " Kan Yan
@ 2019-11-12 11:47   ` Toke Høiland-Jørgensen
  2019-11-13  2:27     ` Kan Yan
  0 siblings, 1 reply; 9+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-12 11:47 UTC (permalink / raw)
  To: Kan Yan, johannes
  Cc: linux-wireless, make-wifi-fast, nbd, yiboz, john, lorenzo,
	rmanohar, kevinhayes, Kan Yan

Kan Yan <kyan@google.com> writes:

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

Thanks for the update! A few comments below. I'll also send you and
updated version of my patch following this one, with fixes for Johannes'
comments. You can include that in your next version...

> ---
>  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        | 10 ++++-
>  net/mac80211/sta_info.c    | 40 +++++++++++++++++++
>  net/mac80211/sta_info.h    |  8 ++++
>  net/mac80211/tx.c          | 47 +++++++++++++++++++++--
>  9 files changed, 235 insertions(+), 14 deletions(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 4ab2c49423dc..15f9f04de149 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -2602,6 +2602,13 @@ enum wiphy_params_flags {
>  
>  #define IEEE80211_DEFAULT_AIRTIME_WEIGHT	256
>  
> +/* The per TXQ device queue limit in airtime */
> +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L	4000
> +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H	8000
> +
> +/* The per interface airtime threshold to switch to lower queue limit */
> +#define IEEE80211_AQL_THRESHOLD			24000
> +
>  /**
>   * struct cfg80211_pmksa - PMK Security Association
>   *
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 4319596ef472..269be699e89c 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -5558,6 +5558,18 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
>  void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
>  				    u32 tx_airtime, u32 rx_airtime);
>  
> +/**
> + * ieee80211_txq_airtime_check - check if a txq can send frame to device
> + *
> + * @hw: pointer obtained from ieee80211_alloc_hw()
> + * @txq: pointer obtained from station or virtual interface
> + *
> + * Return true if the AQL's airtime limit has not been reached and the txq can
> + * continue to send more packets to the device. Otherwise return false.
> + */
> +bool
> +ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
> +
>  /**
>   * ieee80211_iter_keys - iterate keys programmed into the device
>   * @hw: pointer obtained from ieee80211_alloc_hw()
> diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
> index 568b3b276931..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;
> +	}

Why is this setting sta and device limits to the same value?

Also, are you sure we won't risk write tearing when writing 32-bit
values without locking on some architectures?

> +	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..0185e6e5e5d1 100644
> --- a/net/mac80211/debugfs_sta.c
> +++ b/net/mac80211/debugfs_sta.c
> @@ -197,10 +197,12 @@ static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
>  {
>  	struct sta_info *sta = file->private_data;
>  	struct ieee80211_local *local = sta->sdata->local;
> -	size_t bufsz = 200;
> +	size_t bufsz = 400;
>  	char *buf = kzalloc(bufsz, GFP_KERNEL), *p = buf;
>  	u64 rx_airtime = 0, tx_airtime = 0;
>  	s64 deficit[IEEE80211_NUM_ACS];
> +	u32 q_depth[IEEE80211_NUM_ACS];
> +	u32 q_limit_l[IEEE80211_NUM_ACS], q_limit_h[IEEE80211_NUM_ACS];
>  	ssize_t rv;
>  	int ac;
>  
> @@ -212,19 +214,22 @@ static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
>  		rx_airtime += sta->airtime[ac].rx_airtime;
>  		tx_airtime += sta->airtime[ac].tx_airtime;
>  		deficit[ac] = sta->airtime[ac].deficit;
> +		q_limit_l[ac] = sta->airtime[ac].aql_limit_low;
> +		q_limit_h[ac] = sta->airtime[ac].aql_limit_high;
>  		spin_unlock_bh(&local->active_txq_lock[ac]);
> +		q_depth[ac] = atomic_read(&sta->airtime[ac].aql_tx_pending);
>  	}
>  
>  	p += scnprintf(p, bufsz + buf - p,
>  		"RX: %llu us\nTX: %llu us\nWeight: %u\n"
> -		"Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n",
> -		rx_airtime,
> -		tx_airtime,
> -		sta->airtime_weight,
> -		deficit[0],
> -		deficit[1],
> -		deficit[2],
> -		deficit[3]);
> +		"Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n"
> +		"Q depth: VO: %u us VI: %u us BE: %u us BK: %u us\n"
> +		"Q limit[low/high]: VO: %u/%u VI: %u/%u BE: %u/%u BK: %u/%u\n",
> +		rx_airtime, tx_airtime, sta->airtime_weight,
> +		deficit[0], deficit[1], deficit[2], deficit[3],
> +		q_depth[0], q_depth[1], q_depth[2], q_depth[3],
> +		q_limit_l[0], q_limit_h[0], q_limit_l[1], q_limit_h[1],
> +		q_limit_l[2], q_limit_h[2], q_limit_l[3], q_limit_h[3]),
>  
>  	rv = simple_read_from_buffer(userbuf, count, ppos, buf, p - buf);
>  	kfree(buf);
> @@ -236,7 +241,25 @@ static ssize_t sta_airtime_write(struct file *file, const char __user *userbuf,
>  {
>  	struct sta_info *sta = file->private_data;
>  	struct ieee80211_local *local = sta->sdata->local;
> -	int ac;
> +	u32 ac, q_limit_l, q_limit_h;
> +	char _buf[100] = {}, *buf = _buf;
> +
> +	if (count > sizeof(_buf))
> +		return -EINVAL;
> +
> +	if (copy_from_user(buf, userbuf, count))
> +		return -EFAULT;
> +
> +	buf[sizeof(_buf) - 1] = '\0';
> +	if (sscanf(buf, "queue limit %u %u %u", &ac, &q_limit_l, &q_limit_h)
> +	    != 3)
> +		return -EINVAL;
> +
> +	if (ac >= IEEE80211_NUM_ACS)
> +		return -EINVAL;
> +
> +	sta->airtime[ac].aql_limit_low = q_limit_l;
> +	sta->airtime[ac].aql_limit_high = q_limit_h;

Same concern as above re: unprotected writes.

>  	for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
>  		spin_lock_bh(&local->active_txq_lock[ac]);
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index 225ea4e3cd76..ad15b3be8bb3 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -1142,6 +1142,10 @@ struct ieee80211_local {
>  	u16 schedule_round[IEEE80211_NUM_ACS];
>  
>  	u16 airtime_flags;
> +	u32 aql_txq_limit_low[IEEE80211_NUM_ACS];
> +	u32 aql_txq_limit_high[IEEE80211_NUM_ACS];
> +	u32 aql_threshold;
> +	atomic_t aql_total_pending_airtime;
>  
>  	const struct ieee80211_ops *ops;
>  
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index aba094b4ccfc..071ea92a3748 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -667,8 +667,16 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
>  	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
>  		INIT_LIST_HEAD(&local->active_txqs[i]);
>  		spin_lock_init(&local->active_txq_lock[i]);
> +		local->aql_txq_limit_low[i] = IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L;
> +		local->aql_txq_limit_high[i] =
> +			IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H;
>  	}
> -	local->airtime_flags = AIRTIME_USE_TX | AIRTIME_USE_RX;
> +
> +	local->airtime_flags = AIRTIME_USE_TX |
> +			       AIRTIME_USE_RX |
> +			       AIRTIME_USE_AQL;
> +	local->aql_threshold = IEEE80211_AQL_THRESHOLD;
> +	atomic_set(&local->aql_total_pending_airtime, 0);
>  
>  	INIT_LIST_HEAD(&local->chanctx_list);
>  	mutex_init(&local->chanctx_mtx);
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index bd11fef2139f..a76e935a543a 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -396,6 +396,9 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>  		skb_queue_head_init(&sta->ps_tx_buf[i]);
>  		skb_queue_head_init(&sta->tx_filtered[i]);
>  		sta->airtime[i].deficit = sta->airtime_weight;
> +		atomic_set(&sta->airtime[i].aql_tx_pending, 0);
> +		sta->airtime[i].aql_limit_low = local->aql_txq_limit_low[i];
> +		sta->airtime[i].aql_limit_high = local->aql_txq_limit_high[i];
>  	}
>  
>  	for (i = 0; i < IEEE80211_NUM_TIDS; i++)
> @@ -1893,6 +1896,43 @@ 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)
> +{
> +	if (!tx_completed) {
> +		if (sta)
> +			atomic_add(tx_airtime,
> +				   &sta->airtime[ac].aql_tx_pending);
> +
> +		atomic_add(tx_airtime, &local->aql_total_pending_airtime);
> +		return;
> +	}
> +
> +	if (sta) {
> +		if (WARN_ONCE(atomic_read(&sta->airtime[ac].aql_tx_pending) <
> +			      tx_airtime,
> +			      "STA %pM AC %d txq pending airtime underflow: %u, %u",
> +			      sta->addr, ac,
> +			      atomic_read(&sta->airtime[ac].aql_tx_pending),
> +			      tx_airtime))
> +			atomic_set(&sta->airtime[ac].aql_tx_pending, 0);

I don't think this is right; another thread could do atomic_inc()
between the atomic_read() and atomic_set() here, in which case this
would clobber the other value.

I think to get this right the logic would need to be something like
this:

retry:
  old = atomic_read(&sta->airtime[ac].aql_tx_pending);
  if (warn_once(tx_airtime > old))
     new = 0;
  else
     new = old - tx_airtime;

  if (atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending, old, new) != old)
     goto retry;


(or use an equivalent do/while).


-Toke


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v7 2/2] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
  2019-11-12  2:11 ` [PATCH v7 2/2] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue Kan Yan
@ 2019-11-12 22:42     ` kbuild test robot
  0 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2019-11-12 22:42 UTC (permalink / raw)
  To: Kan Yan
  Cc: kbuild-all, johannes, linux-wireless, make-wifi-fast, toke, nbd,
	yiboz, john, lorenzo, rmanohar, kevinhayes

[-- Attachment #1: Type: text/plain, Size: 6632 bytes --]

Hi Kan,

I love your patch! Yet something to improve:

[auto build test ERROR on mac80211-next/master]
[cannot apply to v5.4-rc7 next-20191112]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Kan-Yan/Implement-Airtime-based-Queue-Limit-AQL/20191113-055705
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/mac80211/tx.c: In function 'ieee80211_tx_dequeue':
>> net/mac80211/tx.c:3671:13: error: implicit declaration of function 'ieee80211_calc_expected_tx_airtime'; did you mean 'ieee80211_sta_register_airtime'? [-Werror=implicit-function-declaration]
      airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                ieee80211_sta_register_airtime
   cc1: some warnings being treated as errors

vim +3671 net/mac80211/tx.c

  3537	
  3538	struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
  3539					     struct ieee80211_txq *txq)
  3540	{
  3541		struct ieee80211_local *local = hw_to_local(hw);
  3542		struct txq_info *txqi = container_of(txq, struct txq_info, txq);
  3543		struct ieee80211_hdr *hdr;
  3544		struct sk_buff *skb = NULL;
  3545		struct fq *fq = &local->fq;
  3546		struct fq_tin *tin = &txqi->tin;
  3547		struct ieee80211_tx_info *info;
  3548		struct ieee80211_tx_data tx;
  3549		ieee80211_tx_result r;
  3550		struct ieee80211_vif *vif = txq->vif;
  3551	
  3552		WARN_ON_ONCE(softirq_count() == 0);
  3553	
  3554		if (!ieee80211_txq_airtime_check(hw, txq))
  3555			return NULL;
  3556	
  3557	begin:
  3558		spin_lock_bh(&fq->lock);
  3559	
  3560		if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ||
  3561		    test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags))
  3562			goto out;
  3563	
  3564		if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) {
  3565			set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags);
  3566			goto out;
  3567		}
  3568	
  3569		/* Make sure fragments stay together. */
  3570		skb = __skb_dequeue(&txqi->frags);
  3571		if (skb)
  3572			goto out;
  3573	
  3574		skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func);
  3575		if (!skb)
  3576			goto out;
  3577	
  3578		spin_unlock_bh(&fq->lock);
  3579	
  3580		hdr = (struct ieee80211_hdr *)skb->data;
  3581		info = IEEE80211_SKB_CB(skb);
  3582	
  3583		memset(&tx, 0, sizeof(tx));
  3584		__skb_queue_head_init(&tx.skbs);
  3585		tx.local = local;
  3586		tx.skb = skb;
  3587		tx.sdata = vif_to_sdata(info->control.vif);
  3588	
  3589		if (txq->sta)
  3590			tx.sta = container_of(txq->sta, struct sta_info, sta);
  3591	
  3592		/*
  3593		 * The key can be removed while the packet was queued, so need to call
  3594		 * this here to get the current key.
  3595		 */
  3596		r = ieee80211_tx_h_select_key(&tx);
  3597		if (r != TX_CONTINUE) {
  3598			ieee80211_free_txskb(&local->hw, skb);
  3599			goto begin;
  3600		}
  3601	
  3602		if (test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags))
  3603			info->flags |= IEEE80211_TX_CTL_AMPDU;
  3604		else
  3605			info->flags &= ~IEEE80211_TX_CTL_AMPDU;
  3606	
  3607		if (info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {
  3608			struct sta_info *sta = container_of(txq->sta, struct sta_info,
  3609							    sta);
  3610			u8 pn_offs = 0;
  3611	
  3612			if (tx.key &&
  3613			    (tx.key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV))
  3614				pn_offs = ieee80211_hdrlen(hdr->frame_control);
  3615	
  3616			ieee80211_xmit_fast_finish(sta->sdata, sta, pn_offs,
  3617						   tx.key, skb);
  3618		} else {
  3619			if (invoke_tx_handlers_late(&tx))
  3620				goto begin;
  3621	
  3622			skb = __skb_dequeue(&tx.skbs);
  3623	
  3624			if (!skb_queue_empty(&tx.skbs)) {
  3625				spin_lock_bh(&fq->lock);
  3626				skb_queue_splice_tail(&tx.skbs, &txqi->frags);
  3627				spin_unlock_bh(&fq->lock);
  3628			}
  3629		}
  3630	
  3631		if (skb_has_frag_list(skb) &&
  3632		    !ieee80211_hw_check(&local->hw, TX_FRAG_LIST)) {
  3633			if (skb_linearize(skb)) {
  3634				ieee80211_free_txskb(&local->hw, skb);
  3635				goto begin;
  3636			}
  3637		}
  3638	
  3639		switch (tx.sdata->vif.type) {
  3640		case NL80211_IFTYPE_MONITOR:
  3641			if (tx.sdata->u.mntr.flags & MONITOR_FLAG_ACTIVE) {
  3642				vif = &tx.sdata->vif;
  3643				break;
  3644			}
  3645			tx.sdata = rcu_dereference(local->monitor_sdata);
  3646			if (tx.sdata) {
  3647				vif = &tx.sdata->vif;
  3648				info->hw_queue =
  3649					vif->hw_queue[skb_get_queue_mapping(skb)];
  3650			} else if (ieee80211_hw_check(&local->hw, QUEUE_CONTROL)) {
  3651				ieee80211_free_txskb(&local->hw, skb);
  3652				goto begin;
  3653			} else {
  3654				vif = NULL;
  3655			}
  3656			break;
  3657		case NL80211_IFTYPE_AP_VLAN:
  3658			tx.sdata = container_of(tx.sdata->bss,
  3659						struct ieee80211_sub_if_data, u.ap);
  3660			/* fall through */
  3661		default:
  3662			vif = &tx.sdata->vif;
  3663			break;
  3664		}
  3665	
  3666		IEEE80211_SKB_CB(skb)->control.vif = vif;
  3667	
  3668		if (local->airtime_flags & AIRTIME_USE_AQL) {
  3669			u32 airtime;
  3670	
> 3671			airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
  3672								     skb->len);
  3673			if (airtime) {
  3674				/* We only have 10 bits in tx_time_est, so store airtime
  3675				 * in increments of 4us and clamp the maximum to 2**12-1
  3676				 */
  3677				airtime = min_t(u32, airtime, 4095) & ~3U;
  3678				info->tx_time_est = airtime >> 2;
  3679				ieee80211_sta_update_pending_airtime(local, tx.sta,
  3680								     txq->ac, airtime,
  3681								     false);
  3682			}
  3683		}
  3684	
  3685		return skb;
  3686	
  3687	out:
  3688		spin_unlock_bh(&fq->lock);
  3689	
  3690		return skb;
  3691	}
  3692	EXPORT_SYMBOL(ieee80211_tx_dequeue);
  3693	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59098 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v7 2/2] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
@ 2019-11-12 22:42     ` kbuild test robot
  0 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2019-11-12 22:42 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 6829 bytes --]

Hi Kan,

I love your patch! Yet something to improve:

[auto build test ERROR on mac80211-next/master]
[cannot apply to v5.4-rc7 next-20191112]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Kan-Yan/Implement-Airtime-based-Queue-Limit-AQL/20191113-055705
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/mac80211/tx.c: In function 'ieee80211_tx_dequeue':
>> net/mac80211/tx.c:3671:13: error: implicit declaration of function 'ieee80211_calc_expected_tx_airtime'; did you mean 'ieee80211_sta_register_airtime'? [-Werror=implicit-function-declaration]
      airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                ieee80211_sta_register_airtime
   cc1: some warnings being treated as errors

vim +3671 net/mac80211/tx.c

  3537	
  3538	struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
  3539					     struct ieee80211_txq *txq)
  3540	{
  3541		struct ieee80211_local *local = hw_to_local(hw);
  3542		struct txq_info *txqi = container_of(txq, struct txq_info, txq);
  3543		struct ieee80211_hdr *hdr;
  3544		struct sk_buff *skb = NULL;
  3545		struct fq *fq = &local->fq;
  3546		struct fq_tin *tin = &txqi->tin;
  3547		struct ieee80211_tx_info *info;
  3548		struct ieee80211_tx_data tx;
  3549		ieee80211_tx_result r;
  3550		struct ieee80211_vif *vif = txq->vif;
  3551	
  3552		WARN_ON_ONCE(softirq_count() == 0);
  3553	
  3554		if (!ieee80211_txq_airtime_check(hw, txq))
  3555			return NULL;
  3556	
  3557	begin:
  3558		spin_lock_bh(&fq->lock);
  3559	
  3560		if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ||
  3561		    test_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags))
  3562			goto out;
  3563	
  3564		if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) {
  3565			set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txqi->flags);
  3566			goto out;
  3567		}
  3568	
  3569		/* Make sure fragments stay together. */
  3570		skb = __skb_dequeue(&txqi->frags);
  3571		if (skb)
  3572			goto out;
  3573	
  3574		skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func);
  3575		if (!skb)
  3576			goto out;
  3577	
  3578		spin_unlock_bh(&fq->lock);
  3579	
  3580		hdr = (struct ieee80211_hdr *)skb->data;
  3581		info = IEEE80211_SKB_CB(skb);
  3582	
  3583		memset(&tx, 0, sizeof(tx));
  3584		__skb_queue_head_init(&tx.skbs);
  3585		tx.local = local;
  3586		tx.skb = skb;
  3587		tx.sdata = vif_to_sdata(info->control.vif);
  3588	
  3589		if (txq->sta)
  3590			tx.sta = container_of(txq->sta, struct sta_info, sta);
  3591	
  3592		/*
  3593		 * The key can be removed while the packet was queued, so need to call
  3594		 * this here to get the current key.
  3595		 */
  3596		r = ieee80211_tx_h_select_key(&tx);
  3597		if (r != TX_CONTINUE) {
  3598			ieee80211_free_txskb(&local->hw, skb);
  3599			goto begin;
  3600		}
  3601	
  3602		if (test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags))
  3603			info->flags |= IEEE80211_TX_CTL_AMPDU;
  3604		else
  3605			info->flags &= ~IEEE80211_TX_CTL_AMPDU;
  3606	
  3607		if (info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {
  3608			struct sta_info *sta = container_of(txq->sta, struct sta_info,
  3609							    sta);
  3610			u8 pn_offs = 0;
  3611	
  3612			if (tx.key &&
  3613			    (tx.key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV))
  3614				pn_offs = ieee80211_hdrlen(hdr->frame_control);
  3615	
  3616			ieee80211_xmit_fast_finish(sta->sdata, sta, pn_offs,
  3617						   tx.key, skb);
  3618		} else {
  3619			if (invoke_tx_handlers_late(&tx))
  3620				goto begin;
  3621	
  3622			skb = __skb_dequeue(&tx.skbs);
  3623	
  3624			if (!skb_queue_empty(&tx.skbs)) {
  3625				spin_lock_bh(&fq->lock);
  3626				skb_queue_splice_tail(&tx.skbs, &txqi->frags);
  3627				spin_unlock_bh(&fq->lock);
  3628			}
  3629		}
  3630	
  3631		if (skb_has_frag_list(skb) &&
  3632		    !ieee80211_hw_check(&local->hw, TX_FRAG_LIST)) {
  3633			if (skb_linearize(skb)) {
  3634				ieee80211_free_txskb(&local->hw, skb);
  3635				goto begin;
  3636			}
  3637		}
  3638	
  3639		switch (tx.sdata->vif.type) {
  3640		case NL80211_IFTYPE_MONITOR:
  3641			if (tx.sdata->u.mntr.flags & MONITOR_FLAG_ACTIVE) {
  3642				vif = &tx.sdata->vif;
  3643				break;
  3644			}
  3645			tx.sdata = rcu_dereference(local->monitor_sdata);
  3646			if (tx.sdata) {
  3647				vif = &tx.sdata->vif;
  3648				info->hw_queue =
  3649					vif->hw_queue[skb_get_queue_mapping(skb)];
  3650			} else if (ieee80211_hw_check(&local->hw, QUEUE_CONTROL)) {
  3651				ieee80211_free_txskb(&local->hw, skb);
  3652				goto begin;
  3653			} else {
  3654				vif = NULL;
  3655			}
  3656			break;
  3657		case NL80211_IFTYPE_AP_VLAN:
  3658			tx.sdata = container_of(tx.sdata->bss,
  3659						struct ieee80211_sub_if_data, u.ap);
  3660			/* fall through */
  3661		default:
  3662			vif = &tx.sdata->vif;
  3663			break;
  3664		}
  3665	
  3666		IEEE80211_SKB_CB(skb)->control.vif = vif;
  3667	
  3668		if (local->airtime_flags & AIRTIME_USE_AQL) {
  3669			u32 airtime;
  3670	
> 3671			airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
  3672								     skb->len);
  3673			if (airtime) {
  3674				/* We only have 10 bits in tx_time_est, so store airtime
  3675				 * in increments of 4us and clamp the maximum to 2**12-1
  3676				 */
  3677				airtime = min_t(u32, airtime, 4095) & ~3U;
  3678				info->tx_time_est = airtime >> 2;
  3679				ieee80211_sta_update_pending_airtime(local, tx.sta,
  3680								     txq->ac, airtime,
  3681								     false);
  3682			}
  3683		}
  3684	
  3685		return skb;
  3686	
  3687	out:
  3688		spin_unlock_bh(&fq->lock);
  3689	
  3690		return skb;
  3691	}
  3692	EXPORT_SYMBOL(ieee80211_tx_dequeue);
  3693	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 59098 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-11-12 11:47   ` Toke Høiland-Jørgensen
@ 2019-11-13  2:27     ` Kan Yan
  2019-11-13 14:02       ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 9+ messages in thread
From: Kan Yan @ 2019-11-13  2:27 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Johannes Berg, linux-wireless, Make-Wifi-fast, Felix Fietkau,
	Yibo Zhao, John Crispin, Lorenzo Bianconi, Rajkumar Manoharan,
	Kevin Hayes

Thanks for the review. I will pick up your new patches and give it a
try tomorrow.

> Why is this setting sta and device limits to the same value?

local->aql_txq_limit_low is not the per device limit, but the default
txq_limit for all STAs. Individual stations can be configured with
non-default value via debugfs entry
"netdev:interface_name_x/stations/mac_addr_x/airtime". "aql_threshold"
is the device limit for switching between the lower and higher per
station queue limit.

> Also, are you sure we won't risk write tearing when writing 32-bit
> values without locking on some architectures?

Does mac80211 ever runs in any 16-bit architectures? Even in an
architecture that write to 32-bit value is not atomic, I don't think
there is any side-effect for queue limit get wrong transiently in rare
occasions. Besides, the practical value of those queue limits should
always fit into 16 bits.

> I don't think this is right; another thread could do atomic_inc()
> between the atomic_read() and atomic_set() here, in which case this
> would clobber the other value.
> I think to get this right the logic would need to be something like
> this:
> retry:
>   old = atomic_read(&sta->airtime[ac].aql_tx_pending);
>   if (warn_once(tx_airtime > old))
>      new = 0;
>   else
>      new = old - tx_airtime;
>   if (atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending, old, new) != old)
>      goto retry;
> (or use an equivalent do/while).

That's indeed not right. However, if a potential aql_tx_pending
underflow case is detected here (It should never happen), reset it to
0 maybe not the best remedy anyway. I think it is better  just
WARN_ONCE() and skip updating aql_tx_pending all together, so the
retry or loop can be avoided here. What do you think?


On Tue, Nov 12, 2019 at 3:48 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Kan Yan <kyan@google.com> writes:
>
> > In order for the Fq_CoDel algorithm integrated in mac80211 layer to operate
> > effectively to control excessive queueing latency, the CoDel algorithm
> > requires an accurate measure of how long packets stays in the queue, AKA
> > sojourn time. The sojourn time measured at the mac80211 layer doesn't
> > include queueing latency in the lower layer (firmware/hardware) and CoDel
> > expects lower layer to have a short queue. However, most 802.11ac chipsets
> > offload tasks such TX aggregation to firmware or hardware, thus have a deep
> > lower layer queue.
> >
> > Without a mechanism to control the lower layer queue size, packets only
> > stay in mac80211 layer transiently before being sent to firmware queue.
> > As a result, the sojourn time measured by CoDel in the mac80211 layer is
> > almost always lower than the CoDel latency target, hence CoDel does little
> > to control the latency, even when the lower layer queue causes excessive
> > latency.
> >
> > The Byte Queue Limits (BQL) mechanism is commonly used to address the
> > similar issue with wired network interface. However, this method cannot be
> > applied directly to the wireless network interface. "Bytes" is not a
> > suitable measure of queue depth in the wireless network, as the data rate
> > can vary dramatically from station to station in the same network, from a
> > few Mbps to over Gbps.
> >
> > This patch implements an Airtime-based Queue Limit (AQL) to make CoDel work
> > effectively with wireless drivers that utilized firmware/hardware
> > offloading. AQL allows each txq to release just enough packets to the lower
> > layer to form 1-2 large aggregations to keep hardware fully utilized and
> > retains the rest of the frames in mac80211 layer to be controlled by the
> > CoDel algorithm.
> >
> > Signed-off-by: Kan Yan <kyan@google.com>
> > [ Toke: Keep API to set pending airtime internal, fix nits in commit msg ]
> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Thanks for the update! A few comments below. I'll also send you and
> updated version of my patch following this one, with fixes for Johannes'
> comments. You can include that in your next version...
>
> > ---
> >  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        | 10 ++++-
> >  net/mac80211/sta_info.c    | 40 +++++++++++++++++++
> >  net/mac80211/sta_info.h    |  8 ++++
> >  net/mac80211/tx.c          | 47 +++++++++++++++++++++--
> >  9 files changed, 235 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> > index 4ab2c49423dc..15f9f04de149 100644
> > --- a/include/net/cfg80211.h
> > +++ b/include/net/cfg80211.h
> > @@ -2602,6 +2602,13 @@ enum wiphy_params_flags {
> >
> >  #define IEEE80211_DEFAULT_AIRTIME_WEIGHT     256
> >
> > +/* The per TXQ device queue limit in airtime */
> > +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L    4000
> > +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H    8000
> > +
> > +/* The per interface airtime threshold to switch to lower queue limit */
> > +#define IEEE80211_AQL_THRESHOLD                      24000
> > +
> >  /**
> >   * struct cfg80211_pmksa - PMK Security Association
> >   *
> > diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> > index 4319596ef472..269be699e89c 100644
> > --- a/include/net/mac80211.h
> > +++ b/include/net/mac80211.h
> > @@ -5558,6 +5558,18 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
> >  void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
> >                                   u32 tx_airtime, u32 rx_airtime);
> >
> > +/**
> > + * ieee80211_txq_airtime_check - check if a txq can send frame to device
> > + *
> > + * @hw: pointer obtained from ieee80211_alloc_hw()
> > + * @txq: pointer obtained from station or virtual interface
> > + *
> > + * Return true if the AQL's airtime limit has not been reached and the txq can
> > + * continue to send more packets to the device. Otherwise return false.
> > + */
> > +bool
> > +ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
> > +
> >  /**
> >   * ieee80211_iter_keys - iterate keys programmed into the device
> >   * @hw: pointer obtained from ieee80211_alloc_hw()
> > diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
> > index 568b3b276931..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;
> > +     }
>
> Why is this setting sta and device limits to the same value?
>
> Also, are you sure we won't risk write tearing when writing 32-bit
> values without locking on some architectures?
>
> > +     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..0185e6e5e5d1 100644
> > --- a/net/mac80211/debugfs_sta.c
> > +++ b/net/mac80211/debugfs_sta.c
> > @@ -197,10 +197,12 @@ static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
> >  {
> >       struct sta_info *sta = file->private_data;
> >       struct ieee80211_local *local = sta->sdata->local;
> > -     size_t bufsz = 200;
> > +     size_t bufsz = 400;
> >       char *buf = kzalloc(bufsz, GFP_KERNEL), *p = buf;
> >       u64 rx_airtime = 0, tx_airtime = 0;
> >       s64 deficit[IEEE80211_NUM_ACS];
> > +     u32 q_depth[IEEE80211_NUM_ACS];
> > +     u32 q_limit_l[IEEE80211_NUM_ACS], q_limit_h[IEEE80211_NUM_ACS];
> >       ssize_t rv;
> >       int ac;
> >
> > @@ -212,19 +214,22 @@ static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
> >               rx_airtime += sta->airtime[ac].rx_airtime;
> >               tx_airtime += sta->airtime[ac].tx_airtime;
> >               deficit[ac] = sta->airtime[ac].deficit;
> > +             q_limit_l[ac] = sta->airtime[ac].aql_limit_low;
> > +             q_limit_h[ac] = sta->airtime[ac].aql_limit_high;
> >               spin_unlock_bh(&local->active_txq_lock[ac]);
> > +             q_depth[ac] = atomic_read(&sta->airtime[ac].aql_tx_pending);
> >       }
> >
> >       p += scnprintf(p, bufsz + buf - p,
> >               "RX: %llu us\nTX: %llu us\nWeight: %u\n"
> > -             "Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n",
> > -             rx_airtime,
> > -             tx_airtime,
> > -             sta->airtime_weight,
> > -             deficit[0],
> > -             deficit[1],
> > -             deficit[2],
> > -             deficit[3]);
> > +             "Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n"
> > +             "Q depth: VO: %u us VI: %u us BE: %u us BK: %u us\n"
> > +             "Q limit[low/high]: VO: %u/%u VI: %u/%u BE: %u/%u BK: %u/%u\n",
> > +             rx_airtime, tx_airtime, sta->airtime_weight,
> > +             deficit[0], deficit[1], deficit[2], deficit[3],
> > +             q_depth[0], q_depth[1], q_depth[2], q_depth[3],
> > +             q_limit_l[0], q_limit_h[0], q_limit_l[1], q_limit_h[1],
> > +             q_limit_l[2], q_limit_h[2], q_limit_l[3], q_limit_h[3]),
> >
> >       rv = simple_read_from_buffer(userbuf, count, ppos, buf, p - buf);
> >       kfree(buf);
> > @@ -236,7 +241,25 @@ static ssize_t sta_airtime_write(struct file *file, const char __user *userbuf,
> >  {
> >       struct sta_info *sta = file->private_data;
> >       struct ieee80211_local *local = sta->sdata->local;
> > -     int ac;
> > +     u32 ac, q_limit_l, q_limit_h;
> > +     char _buf[100] = {}, *buf = _buf;
> > +
> > +     if (count > sizeof(_buf))
> > +             return -EINVAL;
> > +
> > +     if (copy_from_user(buf, userbuf, count))
> > +             return -EFAULT;
> > +
> > +     buf[sizeof(_buf) - 1] = '\0';
> > +     if (sscanf(buf, "queue limit %u %u %u", &ac, &q_limit_l, &q_limit_h)
> > +         != 3)
> > +             return -EINVAL;
> > +
> > +     if (ac >= IEEE80211_NUM_ACS)
> > +             return -EINVAL;
> > +
> > +     sta->airtime[ac].aql_limit_low = q_limit_l;
> > +     sta->airtime[ac].aql_limit_high = q_limit_h;
>
> Same concern as above re: unprotected writes.
>
> >       for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
> >               spin_lock_bh(&local->active_txq_lock[ac]);
> > diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> > index 225ea4e3cd76..ad15b3be8bb3 100644
> > --- a/net/mac80211/ieee80211_i.h
> > +++ b/net/mac80211/ieee80211_i.h
> > @@ -1142,6 +1142,10 @@ struct ieee80211_local {
> >       u16 schedule_round[IEEE80211_NUM_ACS];
> >
> >       u16 airtime_flags;
> > +     u32 aql_txq_limit_low[IEEE80211_NUM_ACS];
> > +     u32 aql_txq_limit_high[IEEE80211_NUM_ACS];
> > +     u32 aql_threshold;
> > +     atomic_t aql_total_pending_airtime;
> >
> >       const struct ieee80211_ops *ops;
> >
> > diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> > index aba094b4ccfc..071ea92a3748 100644
> > --- a/net/mac80211/main.c
> > +++ b/net/mac80211/main.c
> > @@ -667,8 +667,16 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
> >       for (i = 0; i < IEEE80211_NUM_ACS; i++) {
> >               INIT_LIST_HEAD(&local->active_txqs[i]);
> >               spin_lock_init(&local->active_txq_lock[i]);
> > +             local->aql_txq_limit_low[i] = IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L;
> > +             local->aql_txq_limit_high[i] =
> > +                     IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H;
> >       }
> > -     local->airtime_flags = AIRTIME_USE_TX | AIRTIME_USE_RX;
> > +
> > +     local->airtime_flags = AIRTIME_USE_TX |
> > +                            AIRTIME_USE_RX |
> > +                            AIRTIME_USE_AQL;
> > +     local->aql_threshold = IEEE80211_AQL_THRESHOLD;
> > +     atomic_set(&local->aql_total_pending_airtime, 0);
> >
> >       INIT_LIST_HEAD(&local->chanctx_list);
> >       mutex_init(&local->chanctx_mtx);
> > diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> > index bd11fef2139f..a76e935a543a 100644
> > --- a/net/mac80211/sta_info.c
> > +++ b/net/mac80211/sta_info.c
> > @@ -396,6 +396,9 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
> >               skb_queue_head_init(&sta->ps_tx_buf[i]);
> >               skb_queue_head_init(&sta->tx_filtered[i]);
> >               sta->airtime[i].deficit = sta->airtime_weight;
> > +             atomic_set(&sta->airtime[i].aql_tx_pending, 0);
> > +             sta->airtime[i].aql_limit_low = local->aql_txq_limit_low[i];
> > +             sta->airtime[i].aql_limit_high = local->aql_txq_limit_high[i];
> >       }
> >
> >       for (i = 0; i < IEEE80211_NUM_TIDS; i++)
> > @@ -1893,6 +1896,43 @@ 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)
> > +{
> > +     if (!tx_completed) {
> > +             if (sta)
> > +                     atomic_add(tx_airtime,
> > +                                &sta->airtime[ac].aql_tx_pending);
> > +
> > +             atomic_add(tx_airtime, &local->aql_total_pending_airtime);
> > +             return;
> > +     }
> > +
> > +     if (sta) {
> > +             if (WARN_ONCE(atomic_read(&sta->airtime[ac].aql_tx_pending) <
> > +                           tx_airtime,
> > +                           "STA %pM AC %d txq pending airtime underflow: %u, %u",
> > +                           sta->addr, ac,
> > +                           atomic_read(&sta->airtime[ac].aql_tx_pending),
> > +                           tx_airtime))
> > +                     atomic_set(&sta->airtime[ac].aql_tx_pending, 0);
>
> I don't think this is right; another thread could do atomic_inc()
> between the atomic_read() and atomic_set() here, in which case this
> would clobber the other value.
>
> I think to get this right the logic would need to be something like
> this:
>
> retry:
>   old = atomic_read(&sta->airtime[ac].aql_tx_pending);
>   if (warn_once(tx_airtime > old))
>      new = 0;
>   else
>      new = old - tx_airtime;
>
>   if (atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending, old, new) != old)
>      goto retry;
>
>
> (or use an equivalent do/while).
>
>
> -Toke
>

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
  2019-11-13  2:27     ` Kan Yan
@ 2019-11-13 14:02       ` Toke Høiland-Jørgensen
  2019-11-14  2:26         ` Kan Yan
  0 siblings, 1 reply; 9+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-11-13 14:02 UTC (permalink / raw)
  To: Kan Yan
  Cc: Johannes Berg, linux-wireless, Make-Wifi-fast, Felix Fietkau,
	Yibo Zhao, John Crispin, Lorenzo Bianconi, Rajkumar Manoharan,
	Kevin Hayes

Kan Yan <kyan@google.com> writes:

> Thanks for the review. I will pick up your new patches and give it a
> try tomorrow.
>
>> Why is this setting sta and device limits to the same value?
>
> local->aql_txq_limit_low is not the per device limit, but the default
> txq_limit for all STAs. Individual stations can be configured with
> non-default value via debugfs entry
> "netdev:interface_name_x/stations/mac_addr_x/airtime". "aql_threshold"
> is the device limit for switching between the lower and higher per
> station queue limit.

Oh, right, I see. But in that case, should writing the default really
stomp on all the per-station values? If I set the value of a station, I
wouldn't expect it to change just because I changed the default value
afterwards?

>> Also, are you sure we won't risk write tearing when writing 32-bit
>> values without locking on some architectures?
>
> Does mac80211 ever runs in any 16-bit architectures? Even in an
> architecture that write to 32-bit value is not atomic, I don't think
> there is any side-effect for queue limit get wrong transiently in rare
> occasions. Besides, the practical value of those queue limits should
> always fit into 16 bits.

I'm not sure about the platform characteristics of all the weird tiny
MIPS boxes that run OpenWrt; which is why I'm vary of making any
assumptions that it is safe :)

But yeah, I suppose you're right that since we're just setting the
limit, it is not going to be a huge concern here...

>> I don't think this is right; another thread could do atomic_inc()
>> between the atomic_read() and atomic_set() here, in which case this
>> would clobber the other value.
>> I think to get this right the logic would need to be something like
>> this:
>> retry:
>>   old = atomic_read(&sta->airtime[ac].aql_tx_pending);
>>   if (warn_once(tx_airtime > old))
>>      new = 0;
>>   else
>>      new = old - tx_airtime;
>>   if (atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending, old, new) != old)
>>      goto retry;
>> (or use an equivalent do/while).
>
> That's indeed not right. However, if a potential aql_tx_pending
> underflow case is detected here (It should never happen), reset it to
> 0 maybe not the best remedy anyway. I think it is better  just
> WARN_ONCE() and skip updating aql_tx_pending all together, so the
> retry or loop can be avoided here. What do you think?

If we don't reset the value to zero may end up with a device that is
unable to transmit. Better to reset it I think, even if this is never
supposed to happen...

-Toke


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

> Oh, right, I see. But in that case, should writing the default really
> stomp on all the per-station values? If I set the value of a station, I
> wouldn't expect it to change just because I changed the default value
> afterwards?

Will persevere the value for stations with customized queue limit in
the next version.

> > That's indeed not right. However, if a potential aql_tx_pending
> > underflow case is detected here (It should never happen), reset it to
> > 0 maybe not the best remedy anyway. I think it is better  just
> > WARN_ONCE() and skip updating aql_tx_pending all together, so the
> > retry or loop can be avoided here. What do you think?
> If we don't reset the value to zero may end up with a device that is
> unable to transmit. Better to reset it I think, even if this is never
> supposed to happen...

I mean not updating the pending airtime to prevent it from going
negative when the tx_airtime is larger than aql_tx_pending.
Will reset it to 0 in next version, which is simpler and cleaner.



On Wed, Nov 13, 2019 at 6:02 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Kan Yan <kyan@google.com> writes:
>
> > Thanks for the review. I will pick up your new patches and give it a
> > try tomorrow.
> >
> >> Why is this setting sta and device limits to the same value?
> >
> > local->aql_txq_limit_low is not the per device limit, but the default
> > txq_limit for all STAs. Individual stations can be configured with
> > non-default value via debugfs entry
> > "netdev:interface_name_x/stations/mac_addr_x/airtime". "aql_threshold"
> > is the device limit for switching between the lower and higher per
> > station queue limit.
>
> Oh, right, I see. But in that case, should writing the default really
> stomp on all the per-station values? If I set the value of a station, I
> wouldn't expect it to change just because I changed the default value
> afterwards?
>
> >> Also, are you sure we won't risk write tearing when writing 32-bit
> >> values without locking on some architectures?
> >
> > Does mac80211 ever runs in any 16-bit architectures? Even in an
> > architecture that write to 32-bit value is not atomic, I don't think
> > there is any side-effect for queue limit get wrong transiently in rare
> > occasions. Besides, the practical value of those queue limits should
> > always fit into 16 bits.
>
> I'm not sure about the platform characteristics of all the weird tiny
> MIPS boxes that run OpenWrt; which is why I'm vary of making any
> assumptions that it is safe :)
>
> But yeah, I suppose you're right that since we're just setting the
> limit, it is not going to be a huge concern here...
>
> >> I don't think this is right; another thread could do atomic_inc()
> >> between the atomic_read() and atomic_set() here, in which case this
> >> would clobber the other value.
> >> I think to get this right the logic would need to be something like
> >> this:
> >> retry:
> >>   old = atomic_read(&sta->airtime[ac].aql_tx_pending);
> >>   if (warn_once(tx_airtime > old))
> >>      new = 0;
> >>   else
> >>      new = old - tx_airtime;
> >>   if (atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending, old, new) != old)
> >>      goto retry;
> >> (or use an equivalent do/while).
> >
> > That's indeed not right. However, if a potential aql_tx_pending
> > underflow case is detected here (It should never happen), reset it to
> > 0 maybe not the best remedy anyway. I think it is better  just
> > WARN_ONCE() and skip updating aql_tx_pending all together, so the
> > retry or loop can be avoided here. What do you think?
>
> If we don't reset the value to zero may end up with a device that is
> unable to transmit. Better to reset it I think, even if this is never
> supposed to happen...
>
> -Toke
>

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-14  2:26 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-12  2:11 [PATCH v7 0/2] Implement Airtime-based Queue Limit (AQL) Kan Yan
2019-11-12  2:11 ` [PATCH v7 1/2] mac80211: " Kan Yan
2019-11-12 11:47   ` Toke Høiland-Jørgensen
2019-11-13  2:27     ` Kan Yan
2019-11-13 14:02       ` Toke Høiland-Jørgensen
2019-11-14  2:26         ` Kan Yan
2019-11-12  2:11 ` [PATCH v7 2/2] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue Kan Yan
2019-11-12 22:42   ` kbuild test robot
2019-11-12 22:42     ` kbuild test robot

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.