[PATCH RFC v4 0/4] Move TXQ scheduling into mac80211

[PATCH RFC v4 0/4] Move TXQ scheduling into mac80211

[Toke Høiland-Jørgensen]

Another update, addressing most of the concerns raised in the last round:

- Added schedule_start()/end() functions that adds locking around the
  whole scheduling operation, which means we can get rid of the 'first'
  parameter to ieee80211_next_txq().

- Adds a callback in the wake_txqs tasklet which will ensure that any
  TXQs throttled by ieee80211_txq_may_transmit() will get woken up
  again. This also makes it possible to ensure all TXQs' deficits are
  increased in the case where the rotation in may_transmit is not
  effective because TXQs are not scheduled in round-robin order by the
  hardware. As part of this, bring back the flag that marks a TXQ as
  throttled.

- Rename ieee80211_schedule_txq() to ieee80211_return_txq() and add a
  check of empty TXQs inside it, so the driver can just call it
  unconditionally.

- Add a call to ieee80211_sta_register_airtime() from the existing
  tx_status path if tx_time is set in the tx_info status field.

- Reorder the patches to the cfg80211 airtime changes come before the
  changes to mac80211.

I didn't port over Kan's "airtime queue limits" stuff yet, partly
because I ran out of time, and partly because I wasn't use if he wanted
to do it himself :)

-Toke

---

Toke Høiland-Jørgensen (4):
      mac80211: Add TXQ scheduling API
      cfg80211: Add airtime statistics and settings
      mac80211: Add airtime accounting and scheduling to TXQs
      ath9k: Switch to mac80211 TXQ scheduling and airtime APIs


 drivers/net/wireless/ath/ath9k/ath9k.h     |   14 --
 drivers/net/wireless/ath/ath9k/debug.c     |    3 
 drivers/net/wireless/ath/ath9k/debug.h     |    8 -
 drivers/net/wireless/ath/ath9k/debug_sta.c |   54 ------
 drivers/net/wireless/ath/ath9k/init.c      |    3 
 drivers/net/wireless/ath/ath9k/recv.c      |    9 -
 drivers/net/wireless/ath/ath9k/xmit.c      |  244 ++++++++--------------------
 include/net/cfg80211.h                     |   10 +
 include/net/mac80211.h                     |  114 +++++++++++++
 include/uapi/linux/nl80211.h               |   15 ++
 net/mac80211/agg-tx.c                      |    2 
 net/mac80211/cfg.c                         |    3 
 net/mac80211/debugfs.c                     |    3 
 net/mac80211/debugfs_sta.c                 |   51 ++++++
 net/mac80211/driver-ops.h                  |    9 +
 net/mac80211/ieee80211_i.h                 |   12 +
 net/mac80211/main.c                        |    6 +
 net/mac80211/sta_info.c                    |   51 ++++++
 net/mac80211/sta_info.h                    |   13 +
 net/mac80211/status.c                      |    6 +
 net/mac80211/tx.c                          |  134 +++++++++++++++
 net/mac80211/util.c                        |   55 ++++++
 net/wireless/nl80211.c                     |   29 +++
 23 files changed, 575 insertions(+), 273 deletions(-)

X-Clacks-Overhead: GNU Terry Pratchett

[PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

This adds airtime accounting and scheduling to the mac80211 TXQ
scheduler. A new callback, ieee80211_sta_register_airtime(), is added
that drivers can call to report airtime usage for stations.

When airtime information is present, mac80211 will schedule TXQs
(through ieee80211_next_txq()) in a way that enforces airtime fairness
between active stations. This scheduling works the same way as the ath9k
in-driver airtime fairness scheduling. If no airtime usage is reported
by the driver, the scheduler will default to round-robin scheduling.

For drivers that don't control TXQ scheduling in software, a new API
function, ieee80211_txq_may_transmit(), is added which the driver can use
to check if the TXQ is eligible for transmission, or should be throttled to
enforce fairness. Calls to this function must also be enclosed in
ieee80211_txq_schedule_{start,end}() calls to ensure proper locking. TXQs
that are throttled by ieee802111_txq_may_transmit() will be woken up again
by a check added to the ieee80211_wake_txqs() tasklet.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 include/net/mac80211.h     |   52 ++++++++++++++++++++++++++++
 net/mac80211/cfg.c         |    3 ++
 net/mac80211/debugfs.c     |    3 ++
 net/mac80211/debugfs_sta.c |   51 +++++++++++++++++++++++++--
 net/mac80211/ieee80211_i.h |    3 ++
 net/mac80211/main.c        |    1 +
 net/mac80211/sta_info.c    |   49 ++++++++++++++++++++++++++
 net/mac80211/sta_info.h    |   13 +++++++
 net/mac80211/status.c      |    6 +++
 net/mac80211/tx.c          |   83 ++++++++++++++++++++++++++++++++++++++++++--
 net/mac80211/util.c        |   55 +++++++++++++++++++++++++++++
 11 files changed, 312 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5ca1484cba58..1e3ee0a2667b 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5350,6 +5350,34 @@ void ieee80211_sta_eosp(struct ieee80211_sta *pubsta);
  */
 void ieee80211_send_eosp_nullfunc(struct ieee80211_sta *pubsta, int tid);
 
+/**
+ * ieee80211_sta_register_airtime - register airtime usage for a sta/tid
+ *
+ * Register airtime usage for a given sta on a given tid. The driver can call
+ * this function to notify mac80211 that a station used a certain amount of
+ * airtime. This information will be used by the TXQ scheduler to schedule
+ * stations in a way that ensures airtime fairness.
+ *
+ * The reported airtime should as a minimum include all time that is spent
+ * transmitting to the remote station, including overhead and padding, but not
+ * including time spent waiting for a TXOP. If the time is not reported by the
+ * hardware it can in some cases be calculated from the rate and known frame
+ * composition. When possible, the time should include any failed transmission
+ * attempts.
+ *
+ * The driver can either call this function synchronously for every packet or
+ * aggregate, or asynchronously as airtime usage information becomes available.
+ * TX and RX airtime can be reported together, or separately by setting one of
+ * them to 0.
+ *
+ * @pubsta: the station
+ * @tid: the TID to register airtime for
+ * @tx_airtime: airtime used during TX (in usec)
+ * @rx_airtime: airtime used during RX (in usec)
+ */
+void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
+				    u32 tx_airtime, u32 rx_airtime);
+
 /**
  * ieee80211_iter_keys - iterate keys programmed into the device
  * @hw: pointer obtained from ieee80211_alloc_hw()
@@ -6106,6 +6134,30 @@ void ieee80211_txq_schedule_start(struct ieee80211_hw *hw, u8 ac);
  */
 void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac);
 
+/**
+ * ieee80211_txq_may_transmit - check whether TXQ is allowed to transmit
+ *
+ * This function is used to check whether given txq is allowed to transmit by
+ * the airtime scheduler, and can be used by drivers to access the airtime
+ * fairness accounting without going using the scheduling order enfored by
+ * next_txq().
+ *
+ * Returns %true if the airtime scheduler thinks the TXQ should be allowed to
+ * transmit, and %false if it should be throttled. This function can also have
+ * the side effect of rotating the TXQ in the scheduler rotation, which will
+ * eventually bring the deficit to positive and allow the station to transmit
+ * again. If a TXQ is throttled, it will be marked and eventually woken up again
+ * through drv_wake_tx_queue().
+ *
+ * If this function returns %true, the driver is expected to schedule packets
+ * for transmission, and then return the TXQ through ieee80211_return_txq().
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ */
+bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
+				struct ieee80211_txq *txq);
+
 /**
  * ieee80211_txq_get_depth - get pending frame/byte count of given txq
  *
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 504627e2117f..c640d3ee5f04 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1388,6 +1388,9 @@ static int sta_apply_parameters(struct ieee80211_local *local,
 	if (ieee80211_vif_is_mesh(&sdata->vif))
 		sta_apply_mesh_params(local, sta, params);
 
+	if (params->airtime_weight)
+		sta->airtime_weight = params->airtime_weight;
+
 	/* set the STA state after all sta info from usermode has been set */
 	if (test_sta_flag(sta, WLAN_STA_TDLS_PEER) ||
 	    set & BIT(NL80211_STA_FLAG_ASSOCIATED)) {
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 3fe541e358f3..81c5fec2eae7 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -383,6 +383,9 @@ void debugfs_hw_add(struct ieee80211_local *local)
 	if (local->ops->wake_tx_queue)
 		DEBUGFS_ADD_MODE(aqm, 0600);
 
+	debugfs_create_u16("airtime_flags", 0600,
+			   phyd, &local->airtime_flags);
+
 	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 af5185a836e5..29e4641cfff9 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -181,9 +181,10 @@ static ssize_t sta_aqm_read(struct file *file, char __user *userbuf,
 			       txqi->tin.tx_bytes,
 			       txqi->tin.tx_packets,
 			       txqi->flags,
-			       txqi->flags & (1<<IEEE80211_TXQ_STOP) ? "STOP" : "RUN",
-			       txqi->flags & (1<<IEEE80211_TXQ_AMPDU) ? " AMPDU" : "",
-			       txqi->flags & (1<<IEEE80211_TXQ_NO_AMSDU) ? " NO-AMSDU" : "");
+			       test_bit(IEEE80211_TXQ_STOP, &txqi->flags) ? "STOP" :
+ 			         test_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags) ? "THROTTLE" : "RUN",
+			       test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags) ? " AMPDU" : "",
+			       test_bit(IEEE80211_TXQ_NO_AMSDU, &txqi->flags) ? " NO-AMSDU" : "");
 	}
 
 	rcu_read_unlock();
@@ -195,6 +196,46 @@ static ssize_t sta_aqm_read(struct file *file, char __user *userbuf,
 }
 STA_OPS(aqm);
 
+static ssize_t sta_airtime_read(struct file *file, char __user *userbuf,
+				size_t count, loff_t *ppos)
+{
+	struct sta_info *sta = file->private_data;
+	struct ieee80211_local *local = sta->sdata->local;
+	size_t bufsz = 200;
+	char *buf = kzalloc(bufsz, GFP_KERNEL), *p = buf;
+	u64 rx_airtime = 0, tx_airtime = 0;
+	s64 deficit[IEEE80211_NUM_ACS];
+	ssize_t rv;
+	int ac;
+
+	if (!buf)
+		return -ENOMEM;
+
+	for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
+		spin_lock_bh(&local->active_txq_lock[ac]);
+		rx_airtime += sta->airtime[ac].rx_airtime;
+		tx_airtime += sta->airtime[ac].tx_airtime;
+		deficit[ac] = sta->airtime[ac].deficit;
+		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]);
+
+	rv = simple_read_from_buffer(userbuf, count, ppos, buf, p - buf);
+	kfree(buf);
+	return rv;
+}
+STA_OPS(airtime);
+
 static ssize_t sta_agg_status_read(struct file *file, char __user *userbuf,
 					size_t count, loff_t *ppos)
 {
@@ -906,6 +947,10 @@ void ieee80211_sta_debugfs_add(struct sta_info *sta)
 	if (local->ops->wake_tx_queue)
 		DEBUGFS_ADD(aqm);
 
+	if (wiphy_ext_feature_isset(local->hw.wiphy,
+				    NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
+		DEBUGFS_ADD(airtime);
+
 	if (sizeof(sta->driver_buffered_tids) == sizeof(u32))
 		debugfs_create_x32("driver_buffered_tids", 0400,
 				   sta->debugfs_dir,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 976531717902..206f248e82fe 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -819,6 +819,7 @@ enum txq_info_flags {
 	IEEE80211_TXQ_AMPDU,
 	IEEE80211_TXQ_NO_AMSDU,
 	IEEE80211_TXQ_STOP_NETIF_TX,
+	IEEE80211_TXQ_AIRTIME_THROTTLE,
 };
 
 /**
@@ -1136,6 +1137,8 @@ struct ieee80211_local {
 	struct list_head active_txqs[IEEE80211_NUM_ACS];
 	u16 schedule_round[IEEE80211_NUM_ACS];
 
+	u16 airtime_flags;
+
 	const struct ieee80211_ops *ops;
 
 	/*
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index d9315de90b48..f8d0a196fbe8 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -667,6 +667,7 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 		INIT_LIST_HEAD(&local->active_txqs[i]);
 		spin_lock_init(&local->active_txq_lock[i]);
 	}
+	local->airtime_flags = AIRTIME_USE_TX | AIRTIME_USE_RX;
 
 	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 c2f5cb7df54f..6da407364279 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -387,9 +387,12 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 	if (sta_prepare_rate_control(local, sta, gfp))
 		goto free_txq;
 
+	sta->airtime_weight = IEEE80211_DEFAULT_AIRTIME_WEIGHT;
+
 	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
 		skb_queue_head_init(&sta->ps_tx_buf[i]);
 		skb_queue_head_init(&sta->tx_filtered[i]);
+		sta->airtime[i].deficit = sta->airtime_weight;
 	}
 
 	for (i = 0; i < IEEE80211_NUM_TIDS; i++)
@@ -1826,6 +1829,35 @@ void ieee80211_sta_set_buffered(struct ieee80211_sta *pubsta,
 }
 EXPORT_SYMBOL(ieee80211_sta_set_buffered);
 
+void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
+				    u32 tx_airtime, u32 rx_airtime)
+{
+	struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
+	struct ieee80211_local *local = sta->sdata->local;
+	struct txq_info *txqi;
+	u8 ac = ieee80211_ac_from_tid(tid);
+	u32 airtime = 0;
+
+	if (sta->local->airtime_flags & AIRTIME_USE_TX)
+		airtime += tx_airtime;
+	if (sta->local->airtime_flags & AIRTIME_USE_RX)
+		airtime += rx_airtime;
+
+	spin_lock_bh(&local->active_txq_lock[ac]);
+	sta->airtime[ac].tx_airtime += tx_airtime;
+	sta->airtime[ac].rx_airtime += rx_airtime;
+	sta->airtime[ac].deficit -= airtime;
+
+	if (sta->airtime[ac].deficit < 0) {
+		txqi = to_txq_info(pubsta->txq[tid]);
+		if (list_empty(&txqi->schedule_order))
+			list_add_tail(&txqi->schedule_order,
+				      &local->active_txqs[ac]);
+	}
+	spin_unlock_bh(&local->active_txq_lock[ac]);
+}
+EXPORT_SYMBOL(ieee80211_sta_register_airtime);
+
 int sta_info_move_state(struct sta_info *sta,
 			enum ieee80211_sta_state new_state)
 {
@@ -2188,6 +2220,23 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
 		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_FAILED);
 	}
 
+	if (!(sinfo->filled & BIT(NL80211_STA_INFO_RX_DURATION))) {
+		for (ac = 0; ac < IEEE80211_NUM_ACS; ac++)
+			sinfo->rx_duration += sta->airtime[ac].rx_airtime;
+		sinfo->filled |= BIT(NL80211_STA_INFO_RX_DURATION);
+	}
+
+	if (!(sinfo->filled & BIT(NL80211_STA_INFO_TX_DURATION))) {
+		for (ac = 0; ac < IEEE80211_NUM_ACS; ac++)
+			sinfo->tx_duration += sta->airtime[ac].tx_airtime;
+		sinfo->filled |= BIT(NL80211_STA_INFO_TX_DURATION);
+	}
+
+	if (!(sinfo->filled & BIT(NL80211_STA_INFO_AIRTIME_WEIGHT))) {
+		sinfo->airtime_weight = sta->airtime_weight;
+		sinfo->filled |= BIT(NL80211_STA_INFO_AIRTIME_WEIGHT);
+	}
+
 	sinfo->rx_dropped_misc = sta->rx_stats.dropped;
 	if (sta->pcpu_rx_stats) {
 		for_each_possible_cpu(cpu) {
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 9a04327d71d1..b1b0fd6a2e21 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -127,6 +127,16 @@ enum ieee80211_agg_stop_reason {
 	AGG_STOP_DESTROY_STA,
 };
 
+/* Debugfs flags to enable/disable use of RX/TX airtime in scheduler */
+#define AIRTIME_USE_TX		BIT(0)
+#define AIRTIME_USE_RX		BIT(1)
+
+struct airtime_info {
+	u64 rx_airtime;
+	u64 tx_airtime;
+	s64 deficit;
+};
+
 struct sta_info;
 
 /**
@@ -563,6 +573,9 @@ struct sta_info {
 	} tx_stats;
 	u16 tid_seq[IEEE80211_QOS_CTL_TID_MASK + 1];
 
+	struct airtime_info airtime[IEEE80211_NUM_ACS];
+	u16 airtime_weight;
+
 	/*
 	 * Aggregation information, locked with lock.
 	 */
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 9a6d7208bf4f..664379797c46 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -821,6 +821,12 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
 			ieee80211_sta_tx_notify(sta->sdata, (void *) skb->data,
 						acked, info->status.tx_time);
 
+		if (info->status.tx_time &&
+		    wiphy_ext_feature_isset(local->hw.wiphy,
+					    NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
+			ieee80211_sta_register_airtime(&sta->sta, tid,
+						       info->status.tx_time, 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 1e071121cb44..7e383b992bdb 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3634,11 +3634,27 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
 
 	lockdep_assert_held(&local->active_txq_lock[ac]);
 
+ begin:
 	txqi = list_first_entry_or_null(&local->active_txqs[ac],
 					struct txq_info,
 					schedule_order);
+	if (!txqi)
+		return NULL;
+
+	if (txqi->txq.sta) {
+		struct sta_info *sta = container_of(txqi->txq.sta,
+						struct sta_info, sta);
+
+		if (sta->airtime[txqi->txq.ac].deficit < 0) {
+			sta->airtime[txqi->txq.ac].deficit += sta->airtime_weight;
+			list_move_tail(&txqi->schedule_order,
+				       &local->active_txqs[txqi->txq.ac]);
+			goto begin;
+		}
+	}
+
 
-	if (!txqi || txqi->schedule_round == local->schedule_round[ac])
+	if (txqi->schedule_round == local->schedule_round[ac])
 		return NULL;
 
 	list_del_init(&txqi->schedule_order);
@@ -3656,13 +3672,71 @@ void ieee80211_return_txq(struct ieee80211_hw *hw,
 	lockdep_assert_held(&local->active_txq_lock[txq->ac]);
 
 	if (list_empty(&txqi->schedule_order) &&
-	    (!skb_queue_empty(&txqi->frags) || txqi->tin.backlog_packets))
-		list_add_tail(&txqi->schedule_order,
-			      &local->active_txqs[txq->ac]);
+	    (!skb_queue_empty(&txqi->frags) || txqi->tin.backlog_packets)) {
+		/* If airtime accounting is active, always enqueue STAs at the
+		 * head of the list to ensure that they only get moved to the
+		 * back by the airtime DRR scheduler once they have a negative
+		 * deficit. A station that already has a negative deficit will
+		 * get immediately moved to the back of the list on the next
+		 * call to ieee80211_next_txq().
+		 */
+		if (wiphy_ext_feature_isset(local->hw.wiphy,
+					    NL80211_EXT_FEATURE_AIRTIME_FAIRNESS)
+		    && txqi->txq.sta)
+			list_add(&txqi->schedule_order,
+				 &local->active_txqs[txq->ac]);
+		else
+			list_add_tail(&txqi->schedule_order,
+				      &local->active_txqs[txq->ac]);
+	}
 
 }
 EXPORT_SYMBOL(ieee80211_return_txq);
 
+bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
+				struct ieee80211_txq *txq)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+	struct txq_info *txqi = to_txq_info(txq);
+	struct sta_info *sta;
+	bool ret = false;
+
+	lockdep_assert_held(&local->active_txq_lock[txqi->txq.ac]);
+
+	if (!txqi->txq.sta) {
+		ret = true;
+		goto out;
+	}
+
+	sta = container_of(txqi->txq.sta, struct sta_info, sta);
+	if (sta->airtime[txqi->txq.ac].deficit >= 0) {
+		ret = true;
+		goto out;
+	}
+
+	/* Not currently eligible, but if the txq is first in the scheduler
+	 * queue, increase deficit and rotate queues so it may be eligible
+	 * next time.
+	 */
+	if (txqi == list_first_entry(&local->active_txqs[txqi->txq.ac],
+				     struct txq_info,
+				     schedule_order)) {
+		sta->airtime[txqi->txq.ac].deficit += sta->airtime_weight;
+		list_move_tail(&txqi->schedule_order,
+			       &local->active_txqs[txqi->txq.ac]);
+	}
+
+ out:
+	if (ret) {
+		clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
+		list_del_init(&txqi->schedule_order);
+	} else
+		set_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
+
+	return ret;
+}
+EXPORT_SYMBOL(ieee80211_txq_may_transmit);
+
 void ieee80211_txq_schedule_start(struct ieee80211_hw *hw, u8 ac)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
@@ -3677,6 +3751,7 @@ void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac)
 	struct ieee80211_local *local = hw_to_local(hw);
 
 	spin_unlock_bh(&local->active_txq_lock[ac]);
+	tasklet_schedule(&local->wake_txqs_tasklet);
 }
 EXPORT_SYMBOL(ieee80211_txq_schedule_end);
 
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 36a3c2ada515..9c889da48ef0 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -240,6 +240,57 @@ __le16 ieee80211_ctstoself_duration(struct ieee80211_hw *hw,
 }
 EXPORT_SYMBOL(ieee80211_ctstoself_duration);
 
+static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
+{
+	bool seen_eligible = false;
+	struct txq_info *txqi;
+	struct sta_info *sta;
+
+	spin_lock_bh(&local->active_txq_lock[ac]);
+
+ begin:
+	if (list_empty(&local->active_txqs[ac]))
+		goto out;
+
+	list_for_each_entry(txqi, &local->active_txqs[ac], schedule_order) {
+		if (!txqi->txq.sta)
+			continue;
+
+		sta = container_of(txqi->txq.sta, struct sta_info, sta);
+
+		if (sta->airtime[ac].deficit >= 0) {
+			seen_eligible = true;
+
+			if (!test_and_clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE,
+						&txqi->flags))
+				continue;
+
+			spin_unlock_bh(&local->active_txq_lock[ac]);
+			drv_wake_tx_queue(local, txqi);
+			spin_lock_bh(&local->active_txq_lock[ac]);
+		}
+	}
+
+	/* No active queues have positive deficit, which means TX is stuck;
+	 * increase all deficits to get things unstuck.
+	 */
+	if (!seen_eligible) {
+		list_for_each_entry(txqi, &local->active_txqs[ac], schedule_order) {
+			if (!txqi->txq.sta)
+				continue;
+
+			sta = container_of(txqi->txq.sta, struct sta_info, sta);
+
+			sta->airtime[ac].deficit += sta->airtime_weight;
+		}
+
+		goto begin;
+	}
+ out:
+	spin_unlock_bh(&local->active_txq_lock[ac]);
+
+}
+
 static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac)
 {
 	struct ieee80211_local *local = sdata->local;
@@ -250,6 +301,10 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac)
 	struct sta_info *sta;
 	int i;
 
+	if (wiphy_ext_feature_isset(local->hw.wiphy,
+				    NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
+	    __ieee80211_kick_airtime(local, ac);
+
 	spin_lock_bh(&fq->lock);
 
 	if (sdata->vif.type == NL80211_IFTYPE_AP)

Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

On 2018-09-16 10:42, Toke Høiland-Jørgensen wrote:
> This adds airtime accounting and scheduling to the mac80211 TXQ
> scheduler. A new callback, ieee80211_sta_register_airtime(), is added
> that drivers can call to report airtime usage for stations.
> 
> +bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> +				struct ieee80211_txq *txq)
> +{

[...]

> +	if (ret) {
> +		clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
> +		list_del_init(&txqi->schedule_order);
> +	} else
> +		set_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
> +
> 
This looks wrong to me. txqi->flags are protected by fq->lock but here
it is by active_txq_lock. no?

> @@ -3677,6 +3751,7 @@ void ieee80211_txq_schedule_end(struct
> ieee80211_hw *hw, u8 ac)
>  	struct ieee80211_local *local = hw_to_local(hw);
> 
>  	spin_unlock_bh(&local->active_txq_lock[ac]);
> +	tasklet_schedule(&local->wake_txqs_tasklet);
>  }
> 
It is an overload to schedule wake_txqs_tasklet for each txq unlock.
Instead I would prefer to call __ieee80211_kick_airtime from 
schedule_end.
Thoughts?

-Rajkumar

Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> On 2018-09-16 10:42, Toke Høiland-Jørgensen wrote:
>> This adds airtime accounting and scheduling to the mac80211 TXQ
>> scheduler. A new callback, ieee80211_sta_register_airtime(), is added
>> that drivers can call to report airtime usage for stations.
>> 
>> +bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
>> +				struct ieee80211_txq *txq)
>> +{
>
> [...]
>
>> +	if (ret) {
>> +		clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
>> +		list_del_init(&txqi->schedule_order);
>> +	} else
>> +		set_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
>> +
>> 
> This looks wrong to me. txqi->flags are protected by fq->lock but here
> it is by active_txq_lock. no?

Both set_bit() and clear_bit() are atomic operations, so they don't need
separate locking. See Documentation/atomic_bitops.txt

>> @@ -3677,6 +3751,7 @@ void ieee80211_txq_schedule_end(struct
>> ieee80211_hw *hw, u8 ac)
>>  	struct ieee80211_local *local = hw_to_local(hw);
>> 
>>  	spin_unlock_bh(&local->active_txq_lock[ac]);
>> +	tasklet_schedule(&local->wake_txqs_tasklet);
>>  }
>> 
> It is an overload to schedule wake_txqs_tasklet for each txq unlock.
> Instead I would prefer to call __ieee80211_kick_airtime from 
> schedule_end.
> Thoughts?

Yeah, I realise scheduling the whole wake_txqs_tasklet is maybe a bit
heavy-handed here. However just calling into __ieee80211_kick_airtime()
means we'll end up recursing back to the same place, which is not good
either (we could in theory flip-flop between two queues until we run out
of stack space).

My "backup plan" if the wake_txqs_tasklet turns out to be too heavy was
to define a new tasklet just for this use; but wanted to see if this
actually turned out to be a problem first...

-Toke

Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

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

On 2018-09-26 02:22, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
> 
>> On 2018-09-16 10:42, Toke Høiland-Jørgensen wrote:
>>> +	if (ret) {
>>> +		clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
>>> +		list_del_init(&txqi->schedule_order);
>>> +	} else
>>> +		set_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
>>> +
>>> 
>> This looks wrong to me. txqi->flags are protected by fq->lock but here
>> it is by active_txq_lock. no?
> 
> Both set_bit() and clear_bit() are atomic operations, so they don't 
> need
> separate locking. See Documentation/atomic_bitops.txt
> 
:( Yeah... I got confused with attached soft lockup in ARM platform.

>>> @@ -3677,6 +3751,7 @@ void ieee80211_txq_schedule_end(struct
>>> ieee80211_hw *hw, u8 ac)
>>>  	struct ieee80211_local *local = hw_to_local(hw);
>>> 
>>>  	spin_unlock_bh(&local->active_txq_lock[ac]);
>>> +	tasklet_schedule(&local->wake_txqs_tasklet);
>>>  }
>>> 
>> It is an overload to schedule wake_txqs_tasklet for each txq unlock.
>> Instead I would prefer to call __ieee80211_kick_airtime from
>> schedule_end.
>> Thoughts?
> 
> Yeah, I realise scheduling the whole wake_txqs_tasklet is maybe a bit
> heavy-handed here. However just calling into __ieee80211_kick_airtime()
> means we'll end up recursing back to the same place, which is not good
> either (we could in theory flip-flop between two queues until we run 
> out
> of stack space).
> 
IMHO schedule_start/end is needed for tracking first node to break the 
loop.
It is not applicable when the driver calls may_transmit(). It would be 
better
if active_txq_lock is moved within may_transmit.

> My "backup plan" if the wake_txqs_tasklet turns out to be too heavy was
> to define a new tasklet just for this use; but wanted to see if this
> actually turned out to be a problem first...
> 
Instead of adding new tasklet, we can introduce new API to iterate 
through
throttled txqs. Driver that make use of may_transmit, have to call this 
new API
at the end of irq handler i.e after processing tx/rx.

-Rajkumar

[-- Attachment #2: crash.txt --]
[-- Type: text/plain, Size: 13331 bytes --]

BUG: soft lockup - CPU#1 stuck for 22s! [swapper/1:0]
[ 1944.211170] Modules linked in: ath10k_pci ath10k_core ath mac80211 cfg80211 compat iptable_nat nf_nat_pptp nf_nat_ipv4 nf_nat_amanda nf_conntrack_pptp nf_conntrack_ipv6 nf_conntrack_ipv4 nf_conntrack_amanda xt_time xt_tcpudp xt_tcpmss xt_string xt_statistic xt_state xt_recent xt_quota xt_policy xt_pkttype xt_physdev xt_owner xt_nat xt_multiport xt_mark xt_mac xt_limit xt_length xt_hl xt_helper xt_esp xt_ecn xt_dscp xt_conntrack xt_connmark xt_connlimit xt_connbytes xt_comment xt_addrtype xt_TCPMSS xt_REDIRECT xt_LOG xt_HL xt_DSCP xt_CT xt_CLASSIFY usbnet ts_kmp ts_fsm ts_bm r8152 pppoe ppp_mppe ppp_async nf_nat_tftp nf_nat_snmp_basic nf_nat_sip nf_nat_rtsp nf_nat_proto_gre nf_nat_irc nf_nat_h323 nf_nat_ftp nf_defrag_ipv6 nf_defrag_ipv4 nf_conntrack_tftp nf_conntrack_snmp nf_conntrack_sip nf_conntrack_rtsp nf_conntrack_proto_gre nf_conntrack_irc nf_conntrack_h323 nf_conntrack_ftp nf_conntrack_broadcast l2tp_ppp iptable_raw iptable_mangle iptable_filter ipt_ah ipt_REJECT ipt_MASQUERADE ipt_ECN ip_tables crc_ccitt arptable_filter arpt_mangle arp_tables sch_teql sch_tbf sch_sfq sch_red sch_prio sch_pie sch_htb sch_gred sch_fq sch_dsmark sch_codel em_text em_nbyte em_meta em_cmp cls_basic act_police act_ipt act_connmark act_skbedit act_mirred em_u32 cls_u32 cls_tcindex cls_flow cls_route cls_fw sch_hfsc sch_ingress qca_nss_ipsec qca_nss_cfi_ocf qca_nss_tunipip6 qca_nss_tun6rd qca_nss_ipsecmgr qca_nss_cfi_cryptoapi qca_nss_qdisc qca_nss_macsec qca_nss_crypto_tool qca_nss_crypto qca_nss_pptp pptp pppox qca_nss_map_t qca_nss_lag_mgr qca_nss_l2tpv2 ppp_generic slhc qca_nss_gre hyfi_bridging nf_nat_proto_sctp nf_nat libcrc32c ledtrig_usbdev nf_conntrack_proto_sctp essedma ip6t_REJECT ip6table_raw ip6table_mangle ip6table_filter ip6_tables x_tables qca_mcs qca_85xx_sw msdos bonding ip6_gre ip_gre gre ifb nat46 sit qca_nss_drv l2tp_netlink l2tp_core ipcomp6 xfrm6_tunnel xfrm6_mode_tunnel xfrm6_mode_transport xfrm6_mode_beet esp6 ah6 ipcomp xfrm4_tunnel xfrm4_mode_tunnel xfrm4_mode_transport xfrm4_mode_beet esp4 ah4 ip6_tunnel qca_nss_gmac tunnel6 tunnel4 ip_tunnel snd_pcm_oss snd_mixer_oss snd_rawmidi snd_seq_device qca_ssdk af_key xfrm_user xfrm_ipcomp xfrm_algo vfat fat ntfs qrfs nf_conntrack nls_iso8859_1 nls_cp437 shortcut_fe_drv aq_phy shortcut_fe_ipv6 shortcut_fe sha1_generic qcrypto cryptosoft cryptodev ocf md5 hmac ecb des_generic cbc authenc usb_storage leds_gpio bootconfig xhci_hcd dwc3 udc_core dwc3_qcom dwc3_ipq40xx phy_qcom_ssusb phy_qcom_hsusb phy_qca_uniphy phy_qca_baldur sd_mod ahci_platform gpio_button_hotplug button_hotplug input_core usbcore nls_base usb_common mii [last unloaded: ecm]
[ 1944.440683]
[ 1944.442163] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.14.77 #5
[ 1944.448151] task: dd47bf40 ti: dd49c000 task.ti: dd49c000
[ 1944.453541] PC is at _test_and_clear_bit+0xc/0x48
[ 1944.458283] LR is at ieee80211_wake_txqs+0x150/0x434 [mac80211]
[ 1944.464124] pc : [<c020caf4>]    lr : [<bfb63ef8>]    psr: 60000113
[ 1944.464124] sp : dd49dec0  ip : 00000000  fp : 00000030
[ 1944.475581] r10: d8b58c30  r9 : d63ab000  r8 : d8b58d18
[ 1944.480788] r7 : d8b58d00  r6 : 00000002  r5 : d6650500  r4 : d8b58c20
[ 1944.487299] r3 : 00000000  r2 : 00000001  r1 : d63ab0a0  r0 : 00000004
[ 1944.493811] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
[ 1944.501100] Control: 10c5787d  Table: 5801406a  DAC: 00000015
[ 1944.506830] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.14.77 #5
[ 1944.512832] [<c021db64>] (unwind_backtrace) from [<c021abf8>] (show_stack+0x10/0x14)
[ 1944.520555] [<c021abf8>] (show_stack) from [<c03c93d4>] (dump_stack+0x80/0xa0)
[ 1944.527761] [<c03c93d4>] (dump_stack) from [<c0285e68>] (watchdog_timer_fn+0x10c/0x160)
[ 1944.535744] [<c0285e68>] (watchdog_timer_fn) from [<c024a64c>] (__run_hrtimer+0x50/0xc8)
[ 1944.543815] [<c024a64c>] (__run_hrtimer) from [<c024ae30>] (hrtimer_interrupt+0x130/0x27c)
[ 1944.552064] [<c024ae30>] (hrtimer_interrupt) from [<c051afa0>] (msm_timer_interrupt+0x38/0x44)
[ 1944.560658] [<c051afa0>] (msm_timer_interrupt) from [<c026c9b8>] (handle_percpu_devid_irq+0x68/0x84)
[ 1944.569774] [<c026c9b8>] (handle_percpu_devid_irq) from [<c026932c>] (generic_handle_irq+0x20/0x30)
[ 1944.578802] [<c026932c>] (generic_handle_irq) from [<c02181dc>] (handle_IRQ+0x68/0x90)
[ 1944.586697] [<c02181dc>] (handle_IRQ) from [<c02084e0>] (gic_handle_irq+0x3c/0x60)
[ 1944.594248] [<c02084e0>] (gic_handle_irq) from [<c02095c0>] (__irq_svc+0x40/0x70)
[ 1944.601710] Exception stack(0xdd49de78 to 0xdd49dec0)
[ 1944.606744] de60:                                                       00000004 d63ab0a0
[ 1944.614904] de80: 00000001 00000000 d8b58c20 d6650500 00000002 d8b58d00 d8b58d18 d63ab000
[ 1944.623064] dea0: d8b58c30 00000030 00000000 dd49dec0 bfb63ef8 c020caf4 60000113 ffffffff
[ 1944.631229] [<c02095c0>] (__irq_svc) from [<c020caf4>] (_test_and_clear_bit+0xc/0x48)
[ 1944.639066] [<c020caf4>] (_test_and_clear_bit) from [<bfb63ef8>] (ieee80211_wake_txqs+0x150/0x434 [mac80211])
[ 1944.649040] [<bfb63ef8>] (ieee80211_wake_txqs [mac80211]) from [<c0232f88>] (tasklet_action+0x8c/0xec)
[ 1944.658224] [<c0232f88>] (tasklet_action) from [<c0232580>] (__do_softirq+0x104/0x294)
[ 1944.666121] [<c0232580>] (__do_softirq) from [<c02329fc>] (irq_exit+0x9c/0x11c)
[ 1944.673415] [<c02329fc>] (irq_exit) from [<c02181e0>] (handle_IRQ+0x6c/0x90)
[ 1944.680445] [<c02181e0>] (handle_IRQ) from [<c02084e0>] (gic_handle_irq+0x3c/0x60)
[ 1944.687995] [<c02084e0>] (gic_handle_irq) from [<c02095c0>] (__irq_svc+0x40/0x70)
[ 1944.695458] Exception stack(0xdd49dfa0 to 0xdd49dfe8)
[ 1944.700494] dfa0: ffffffed 00000000 1d3c4000 c020a040 dd49c000 dd49c030 10c0387d c08ba2c8
[ 1944.708656] dfc0: 4220406a 512f04d0 00000000 00000000 00000000 dd49dfe8 c02184a0 c02184a4
[ 1944.716812] dfe0: 60000013 ffffffff
[ 1944.720290] [<c02095c0>] (__irq_svc) from [<c02184a4>] (arch_cpu_idle+0x38/0x5c)
[ 1944.727670] [<c02184a4>] (arch_cpu_idle) from [<c0269298>] (cpu_startup_entry+0xa4/0x108)
[ 1944.735859] [<c0269298>] (cpu_startup_entry) from [<422085a4>] (0x422085a4)
[ 1948.126113] BUG: soft lockup - CPU#0 stuck for 23s! [swapper/0:0]
[ 1948.131163] Modules linked in: ath10k_pci ath10k_core ath mac80211 cfg80211 compat iptable_nat nf_nat_pptp nf_nat_ipv4 nf_nat_amanda nf_conntrack_pptp nf_conntrack_ipv6 nf_conntrack_ipv4 nf_conntrack_amanda xt_time xt_tcpudp xt_tcpmss xt_string xt_statistic xt_state xt_recent xt_quota xt_policy xt_pkttype xt_physdev xt_owner xt_nat xt_multiport xt_mark xt_mac xt_limit xt_length xt_hl xt_helper xt_esp xt_ecn xt_dscp xt_conntrack xt_connmark xt_connlimit xt_connbytes xt_comment xt_addrtype xt_TCPMSS xt_REDIRECT xt_LOG xt_HL xt_DSCP xt_CT xt_CLASSIFY usbnet ts_kmp ts_fsm ts_bm r8152 pppoe ppp_mppe ppp_async nf_nat_tftp nf_nat_snmp_basic nf_nat_sip nf_nat_rtsp nf_nat_proto_gre nf_nat_irc nf_nat_h323 nf_nat_ftp nf_defrag_ipv6 nf_defrag_ipv4 nf_conntrack_tftp nf_conntrack_snmp nf_conntrack_sip nf_conntrack_rtsp nf_conntrack_proto_gre nf_conntrack_irc nf_conntrack_h323 nf_conntrack_ftp nf_conntrack_broadcast l2tp_ppp iptable_raw iptable_mangle iptable_filter ipt_ah ipt_REJECT ipt_MASQUERADE ipt_ECN ip_tables crc_ccitt arptable_filter arpt_mangle arp_tables sch_teql sch_tbf sch_sfq sch_red sch_prio sch_pie sch_htb sch_gred sch_fq sch_dsmark sch_codel em_text em_nbyte em_meta em_cmp cls_basic act_police act_ipt act_connmark act_skbedit act_mirred em_u32 cls_u32 cls_tcindex cls_flow cls_route cls_fw sch_hfsc sch_ingress qca_nss_ipsec qca_nss_cfi_ocf qca_nss_tunipip6 qca_nss_tun6rd qca_nss_ipsecmgr qca_nss_cfi_cryptoapi qca_nss_qdisc qca_nss_macsec qca_nss_crypto_tool qca_nss_crypto qca_nss_pptp pptp pppox qca_nss_map_t qca_nss_lag_mgr qca_nss_l2tpv2 ppp_generic slhc qca_nss_gre hyfi_bridging nf_nat_proto_sctp nf_nat libcrc32c ledtrig_usbdev nf_conntrack_proto_sctp essedma ip6t_REJECT ip6table_raw ip6table_mangle ip6table_filter ip6_tables x_tables qca_mcs qca_85xx_sw msdos bonding ip6_gre ip_gre gre ifb nat46 sit qca_nss_drv l2tp_netlink l2tp_core ipcomp6 xfrm6_tunnel xfrm6_mode_tunnel xfrm6_mode_transport xfrm6_mode_beet esp6 ah6 ipcomp xfrm4_tunnel xfrm4_mode_tunnel xfrm4_mode_transport xfrm4_mode_beet esp4 ah4 ip6_tunnel qca_nss_gmac tunnel6 tunnel4 ip_tunnel snd_pcm_oss snd_mixer_oss snd_rawmidi snd_seq_device qca_ssdk af_key xfrm_user xfrm_ipcomp xfrm_algo vfat fat ntfs qrfs nf_conntrack nls_iso8859_1 nls_cp437 shortcut_fe_drv aq_phy shortcut_fe_ipv6 shortcut_fe sha1_generic qcrypto cryptosoft cryptodev ocf md5 hmac ecb des_generic cbc authenc usb_storage leds_gpio bootconfig xhci_hcd dwc3 udc_core dwc3_qcom dwc3_ipq40xx phy_qcom_ssusb phy_qcom_hsusb phy_qca_uniphy phy_qca_baldur sd_mod ahci_platform gpio_button_hotplug button_hotplug input_core usbcore nls_base usb_common mii [last unloaded: ecm]
[ 1948.360675]
[ 1948.362153] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.77 #5
[ 1948.368143] task: c086e790 ti: c0862000 task.ti: c0862000
[ 1948.373527] PC is at _raw_spin_lock_bh+0x48/0x5c
[ 1948.378152] LR is at ieee80211_tx_dequeue+0x6e8/0x8ec [mac80211]
[ 1948.384115] pc : [<c0212144>]    lr : [<bfb5e3c0>]    psr: 20000113
[ 1948.384115] sp : c0863c68  ip : d4f4dfa8  fp : d6956000
[ 1948.395573] r10: d8b58cb4  r9 : d8b58ca8  r8 : d8b58cac
[ 1948.400781] r7 : 00000000  r6 : d6d38c00  r5 : d63ab0a4  r4 : d8b58c20
[ 1948.407291] r3 : 00003abd  r2 : 00003abe  r1 : 00000000  r0 : d8b58d00
[ 1948.413802] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
[ 1948.421093] Control: 10c5787d  Table: 5806806a  DAC: 00000015
[ 1948.426823] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.77 #5
[ 1948.432819] [<c021db64>] (unwind_backtrace) from [<c021abf8>] (show_stack+0x10/0x14)
[ 1948.440543] [<c021abf8>] (show_stack) from [<c03c93d4>] (dump_stack+0x80/0xa0)
[ 1948.447748] [<c03c93d4>] (dump_stack) from [<c0285e68>] (watchdog_timer_fn+0x10c/0x160)
[ 1948.455733] [<c0285e68>] (watchdog_timer_fn) from [<c024a64c>] (__run_hrtimer+0x50/0xc8)
[ 1948.463805] [<c024a64c>] (__run_hrtimer) from [<c024ae30>] (hrtimer_interrupt+0x130/0x27c)
[ 1948.472053] [<c024ae30>] (hrtimer_interrupt) from [<c051afa0>] (msm_timer_interrupt+0x38/0x44)
[ 1948.480646] [<c051afa0>] (msm_timer_interrupt) from [<c026c9b8>] (handle_percpu_devid_irq+0x68/0x84)
[ 1948.489761] [<c026c9b8>] (handle_percpu_devid_irq) from [<c026932c>] (generic_handle_irq+0x20/0x30)
[ 1948.498790] [<c026932c>] (generic_handle_irq) from [<c02181dc>] (handle_IRQ+0x68/0x90)
[ 1948.506688] [<c02181dc>] (handle_IRQ) from [<c02084e0>] (gic_handle_irq+0x3c/0x60)
[ 1948.514239] [<c02084e0>] (gic_handle_irq) from [<c02095c0>] (__irq_svc+0x40/0x70)
[ 1948.521701] Exception stack(0xc0863c20 to 0xc0863c68)
[ 1948.526737] 3c20: d8b58d00 00000000 00003abe 00003abd d8b58c20 d63ab0a4 d6d38c00 00000000
[ 1948.534898] 3c40: d8b58cac d8b58ca8 d8b58cb4 d6956000 d4f4dfa8 c0863c68 bfb5e3c0 c0212144
[ 1948.543056] 3c60: 20000113 ffffffff
[ 1948.546531] [<c02095c0>] (__irq_svc) from [<c0212144>] (_raw_spin_lock_bh+0x48/0x5c)
[ 1948.554284] [<c0212144>] (_raw_spin_lock_bh) from [<bfb5e3c0>] (ieee80211_tx_dequeue+0x6e8/0x8ec [mac80211])
[ 1948.564122] [<bfb5e3c0>] (ieee80211_tx_dequeue [mac80211]) from [<bfb606a0>] (__ieee80211_subif_start_xmit+0x7e4/0x870 [mac80211])
[ 1948.575838] [<bfb606a0>] (__ieee80211_subif_start_xmit [mac80211]) from [<bfb60a7c>] (ieee80211_subif_start_xmit+0x104/0x110 [mac80211])
[ 1948.588055] [<bfb60a7c>] (ieee80211_subif_start_xmit [mac80211]) from [<c059f9b0>] (dev_hard_start_xmit+0x320/0x454)
[ 1948.598529] [<c059f9b0>] (dev_hard_start_xmit) from [<c059fe5c>] (__dev_queue_xmit+0x378/0x454)
[ 1948.607212] [<c059fe5c>] (__dev_queue_xmit) from [<c0659850>] (br_dev_queue_push_xmit+0xbc/0xc4)
[ 1948.615979] [<c0659850>] (br_dev_queue_push_xmit) from [<c065ae98>] (br_handle_frame_finish+0x508/0x54c)
[ 1948.625439] [<c065ae98>] (br_handle_frame_finish) from [<c065b1b4>] (br_handle_frame+0x2d8/0x340)
[ 1948.634293] [<c065b1b4>] (br_handle_frame) from [<c059c5e0>] (__netif_receive_skb_core+0x4e0/0x6e0)
[ 1948.643321] [<c059c5e0>] (__netif_receive_skb_core) from [<c059e1d8>] (process_backlog+0x90/0x158)
[ 1948.652262] [<c059e1d8>] (process_backlog) from [<c059db70>] (net_rx_action+0xac/0x160)
[ 1948.660247] [<c059db70>] (net_rx_action) from [<c0232580>] (__do_softirq+0x104/0x294)
[ 1948.668059] [<c0232580>] (__do_softirq) from [<c02329fc>] (irq_exit+0x9c/0x11c)
[ 1948.675352] [<c02329fc>] (irq_exit) from [<c02181e0>] (handle_IRQ+0x6c/0x90)
[ 1948.682381] [<c02181e0>] (handle_IRQ) from [<c02084e0>] (gic_handle_irq+0x3c/0x60)
[ 1948.689933] [<c02084e0>] (gic_handle_irq) from [<c02095c0>] (__irq_svc+0x40/0x70)
[ 1948.697396] Exception stack(0xc0863f60 to 0xc0863fa8)
[ 1948.702432] 3f60: ffffffed 00000000 1d3bc000 c020a040 c0862000 c0862030 ffffffff c08ba014
[ 1948.710592] 3f80: c086a3c0 c0858ba8 ddffcd40 c08ba010 c0873548 c0863fa8 c02184a0 c02184a4
[ 1948.718751] 3fa0: 60000013 ffffffff
[ 1948.722226] [<c02095c0>] (__irq_svc) from [<c02184a4>] (arch_cpu_idle+0x38/0x5c)
[ 1948.729608] [<c02184a4>] (arch_cpu_idle) from [<c0269298>] (cpu_startup_entry+0xa4/0x108)
[ 1948.737766] [<c0269298>] (cpu_startup_entry) from [<c0837af0>] (start_kernel+0x358/0x3cc)

Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

On 2018-09-26 17:09, Rajkumar Manoharan wrote:
> On 2018-09-26 02:22, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> :( Yeah... I got confused with attached soft lockup in ARM platform.
> 
Toke,

Cause for the soft lockup exposed in multi client scenario is due to
mixed order of fq_lock and active_txqs_lock. In wake_tx_queue or 
push_pending
case, driver acquires active_txq_lock first by schedule_start and 
followed by
fq_lock in tx_dequeue. The same order should be maintained in sta 
cleanup.
Below change fixed the issue.

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 4814710a64f3..2029d600663b 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -114,9 +114,7 @@ static void __cleanup_single_sta(struct sta_info 
*sta)
                 for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
                         struct txq_info *txqi = 
to_txq_info(sta->sta.txq[i]);

-                       spin_lock_bh(&fq->lock);
                         ieee80211_txq_purge(local, txqi);
-                       spin_unlock_bh(&fq->lock);
                 }
         }

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 884d9ce5ce4b..5d70f64a205c 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1467,8 +1467,10 @@ void ieee80211_txq_purge(struct ieee80211_local 
*local,
         struct fq *fq = &local->fq;
         struct fq_tin *tin = &txqi->tin;

+       spin_lock_bh(&fq->lock);
         fq_tin_reset(fq, tin, fq_skb_free_func);
         ieee80211_purge_tx_queue(&local->hw, &txqi->frags);
+       spin_unlock_bh(&fq->lock);
         spin_lock_bh(&local->active_txq_lock[txqi->txq.ac]);
         list_del_init(&txqi->schedule_order);
         spin_unlock_bh(&local->active_txq_lock[txqi->txq.ac]);

-Rajkumar

Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

On 28 September 2018 07:29:03 CEST, Rajkumar Manoharan <rmanohar@codeaurora.org> wrote:
>On 2018-09-26 17:09, Rajkumar Manoharan wrote:
>> On 2018-09-26 02:22, Toke Høiland-Jørgensen wrote:
>>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>
>> :( Yeah... I got confused with attached soft lockup in ARM platform.
>> 
>Toke,
>
>Cause for the soft lockup exposed in multi client scenario is due to
>mixed order of fq_lock and active_txqs_lock. In wake_tx_queue or 
>push_pending
>case, driver acquires active_txq_lock first by schedule_start and 
>followed by
>fq_lock in tx_dequeue. The same order should be maintained in sta 
>cleanup.
>Below change fixed the issue.

Ah, great find! I'll fold this into the next version, thanks!

-Toke

Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

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

On 2018-09-28 00:51, Toke Høiland-Jørgensen wrote:
> On 28 September 2018 07:29:03 CEST, Rajkumar Manoharan
> <rmanohar@codeaurora.org> wrote:
>> On 2018-09-26 17:09, Rajkumar Manoharan wrote:
>>> On 2018-09-26 02:22, Toke Høiland-Jørgensen wrote:
>>>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>> 
>>> :( Yeah... I got confused with attached soft lockup in ARM platform.
>>> 
>> Toke,
>> 
>> Cause for the soft lockup exposed in multi client scenario is due to
>> mixed order of fq_lock and active_txqs_lock. In wake_tx_queue or
>> push_pending
>> case, driver acquires active_txq_lock first by schedule_start and
>> followed by
>> fq_lock in tx_dequeue. The same order should be maintained in sta
>> cleanup.
>> Below change fixed the issue.
> 
> Ah, great find! I'll fold this into the next version, thanks!
> 

One more thing. As I mentioned earlier, scheduling wake_txqs_tasklet
is heavy load and causing random rcu stall issue. Hence I added
another API to schedule throttled txqs once for all. Also I did
a cleanup in kick_airtime by traversing list only once. With these
changes I don't see rcu stall issue. Please review and fold them as 
well.

-Rajkumar


single_iter - clean up kick_airtime
sched_throttle - new API and separate tasklet for throttled txqs

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: single_iter.patch --]
[-- Type: text/x-diff; name=single_iter.patch, Size: 1700 bytes --]

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 404c5e82e4ca..023bc81bd4a0 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -242,13 +242,11 @@ EXPORT_SYMBOL(ieee80211_ctstoself_duration);
 
 static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
 {
-	bool seen_eligible = false;
 	struct txq_info *txqi;
 	struct sta_info *sta;
 
 	spin_lock_bh(&local->active_txq_lock[ac]);
 
- begin:
 	if (list_empty(&local->active_txqs[ac]))
 		goto out;
 
@@ -258,12 +256,12 @@ static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
 
 		sta = container_of(txqi->txq.sta, struct sta_info, sta);
 
-		if (sta->airtime[ac].deficit >= 0) {
-			seen_eligible = true;
-
-			if (!test_and_clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE,
-						&txqi->flags))
+		if (test_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags)) {
+			clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
+			if (sta->airtime[ac].deficit < 0) {
+				sta->airtime[ac].deficit += sta->airtime_weight;
 				continue;
+			}
 
 			spin_unlock_bh(&local->active_txq_lock[ac]);
 			drv_wake_tx_queue(local, txqi);
@@ -271,21 +269,6 @@ static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
 		}
 	}
 
-	/* No active queues have positive deficit, which means TX is stuck;
-	 * increase all deficits to get things unstuck.
-	 */
-	if (!seen_eligible) {
-		list_for_each_entry(txqi, &local->active_txqs[ac], schedule_order) {
-			if (!txqi->txq.sta)
-				continue;
-
-			sta = container_of(txqi->txq.sta, struct sta_info, sta);
-
-			sta->airtime[ac].deficit += sta->airtime_weight;
-		}
-
-		goto begin;
-	}
  out:
 	spin_unlock_bh(&local->active_txq_lock[ac]);
 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: sched_throttle.patch --]
[-- Type: text/x-diff; name=sched_throttle.patch, Size: 4772 bytes --]

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 625a4ab37ea0..b1bb369b0971 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -2362,6 +2362,7 @@ static void ath10k_htt_rx_tx_fetch_ind(struct ath10k *ar, struct sk_buff *skb)
 	}
 
 	rcu_read_unlock();
+	ieee80211_txq_schedule_throttled(hw);
 
 	resp_ids = ath10k_htt_get_tx_fetch_ind_resp_ids(&resp->tx_fetch_ind);
 	ath10k_htt_rx_tx_fetch_resp_id_confirm(ar, resp_ids, num_resp_ids);
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 50c1082c86d3..8101936a9c9c 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -6124,6 +6124,7 @@ void ieee80211_txq_schedule_start(struct ieee80211_hw *hw, u8 ac);
  */
 void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac);
 
+void ieee80211_txq_schedule_throttled(struct ieee80211_hw *hw);
 /**
  * ieee80211_txq_may_transmit - check whether TXQ is allowed to transmit
  *
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c138b9d47453..329ebd2e1594 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1254,6 +1254,7 @@ struct ieee80211_local {
 	struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
 	struct tasklet_struct tx_pending_tasklet;
 	struct tasklet_struct wake_txqs_tasklet;
+	struct tasklet_struct kick_airtime;
 
 	atomic_t agg_queue_stop[IEEE80211_MAX_QUEUES];
 
@@ -2068,6 +2069,7 @@ void ieee80211_txq_purge(struct ieee80211_local *local,
 void ieee80211_txq_remove_vlan(struct ieee80211_local *local,
 			       struct ieee80211_sub_if_data *sdata);
 void ieee80211_wake_txqs(unsigned long data);
+void ieee80211_kick_airtime(unsigned long data);
 void ieee80211_send_auth(struct ieee80211_sub_if_data *sdata,
 			 u16 transaction, u16 auth_alg, u16 status,
 			 const u8 *extra, size_t extra_len, const u8 *bssid,
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index c4c3feaea336..f2772d15a541 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -675,9 +675,12 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 	tasklet_init(&local->tx_pending_tasklet, ieee80211_tx_pending,
 		     (unsigned long)local);
 
-	if (ops->wake_tx_queue)
+	if (ops->wake_tx_queue) {
 		tasklet_init(&local->wake_txqs_tasklet, ieee80211_wake_txqs,
 			     (unsigned long)local);
+		tasklet_init(&local->kick_airtime, ieee80211_kick_airtime,
+			     (unsigned long)local);
+	}
 
 	tasklet_init(&local->tasklet,
 		     ieee80211_tasklet_handler,
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 5d70f64a205c..73287f5eedd8 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3696,6 +3696,14 @@ bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
 }
 EXPORT_SYMBOL(ieee80211_txq_may_transmit);
 
+void ieee80211_txq_schedule_throttled(struct ieee80211_hw *hw)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+
+	tasklet_schedule(&local->wake_txqs_tasklet);
+}
+EXPORT_SYMBOL(ieee80211_txq_schedule_throttled);
+
 void ieee80211_txq_schedule_start(struct ieee80211_hw *hw, u8 ac)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
@@ -3710,7 +3718,6 @@ void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac)
 	struct ieee80211_local *local = hw_to_local(hw);
 
 	spin_unlock_bh(&local->active_txq_lock[ac]);
-	tasklet_schedule(&local->wake_txqs_tasklet);
 }
 EXPORT_SYMBOL(ieee80211_txq_schedule_end);
 
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 023bc81bd4a0..69fc3c723add 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -246,6 +246,7 @@ static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
 	struct sta_info *sta;
 
 	spin_lock_bh(&local->active_txq_lock[ac]);
+	rcu_read_lock();
 
 	if (list_empty(&local->active_txqs[ac]))
 		goto out;
@@ -270,6 +271,7 @@ static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
 	}
 
  out:
+	rcu_read_unlock();
 	spin_unlock_bh(&local->active_txq_lock[ac]);
 
 }
@@ -284,10 +286,6 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac)
 	struct sta_info *sta;
 	int i;
 
-	if (wiphy_ext_feature_isset(local->hw.wiphy,
-				    NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
-	    __ieee80211_kick_airtime(local, ac);
-
 	spin_lock_bh(&fq->lock);
 
 	if (sdata->vif.type == NL80211_IFTYPE_AP)
@@ -334,6 +332,15 @@ out:
 	spin_unlock_bh(&fq->lock);
 }
 
+void ieee80211_kick_airtime(unsigned long data)
+{
+	struct ieee80211_local *local = (struct ieee80211_local *)data;
+	int i;
+
+	for (i = 0; i < IEEE80211_NUM_ACS; i++)
+		__ieee80211_kick_airtime(local, i);
+}
+
 void ieee80211_wake_txqs(unsigned long data)
 {
 	struct ieee80211_local *local = (struct ieee80211_local *)data;

Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

On 2018-09-28 02:27, Rajkumar Manoharan wrote:
> On 2018-09-28 00:51, Toke Høiland-Jørgensen wrote:
>> On 28 September 2018 07:29:03 CEST, Rajkumar Manoharan
>> <rmanohar@codeaurora.org> wrote:
>>> On 2018-09-26 17:09, Rajkumar Manoharan wrote:
>>>> On 2018-09-26 02:22, Toke Høiland-Jørgensen wrote:
>>>>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>>> 
>>>> :( Yeah... I got confused with attached soft lockup in ARM platform.
>>>> 
>>> Toke,
>>> 
>>> Cause for the soft lockup exposed in multi client scenario is due to
>>> mixed order of fq_lock and active_txqs_lock. In wake_tx_queue or
>>> push_pending
>>> case, driver acquires active_txq_lock first by schedule_start and
>>> followed by
>>> fq_lock in tx_dequeue. The same order should be maintained in sta
>>> cleanup.
>>> Below change fixed the issue.
>> 
>> Ah, great find! I'll fold this into the next version, thanks!
>> 
> 
> One more thing. As I mentioned earlier, scheduling wake_txqs_tasklet
> is heavy load and causing random rcu stall issue. Hence I added
> another API to schedule throttled txqs once for all. Also I did
> a cleanup in kick_airtime by traversing list only once. With these
> changes I don't see rcu stall issue. Please review and fold them as 
> well.
> 
Correction in patch. new API schedule kick_aitime tasklet not 
wake_txq_tasklet.

+void ieee80211_txq_schedule_throttled(struct ieee80211_hw *hw)
+{
+       struct ieee80211_local *local = hw_to_local(hw);
+
+       tasklet_schedule(&local->kick_airtime);
+}

-Rajkumar

Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> On 2018-09-28 00:51, Toke Høiland-Jørgensen wrote:
>> On 28 September 2018 07:29:03 CEST, Rajkumar Manoharan
>> <rmanohar@codeaurora.org> wrote:
>>> On 2018-09-26 17:09, Rajkumar Manoharan wrote:
>>>> On 2018-09-26 02:22, Toke Høiland-Jørgensen wrote:
>>>>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>>> 
>>>> :( Yeah... I got confused with attached soft lockup in ARM platform.
>>>> 
>>> Toke,
>>> 
>>> Cause for the soft lockup exposed in multi client scenario is due to
>>> mixed order of fq_lock and active_txqs_lock. In wake_tx_queue or
>>> push_pending
>>> case, driver acquires active_txq_lock first by schedule_start and
>>> followed by
>>> fq_lock in tx_dequeue. The same order should be maintained in sta
>>> cleanup.
>>> Below change fixed the issue.
>> 
>> Ah, great find! I'll fold this into the next version, thanks!
>> 
>
> One more thing. As I mentioned earlier, scheduling wake_txqs_tasklet
> is heavy load and causing random rcu stall issue. Hence I added
> another API to schedule throttled txqs once for all. Also I did
> a cleanup in kick_airtime by traversing list only once. With these
> changes I don't see rcu stall issue. Please review and fold them as 
> well.
>
> -Rajkumar
>
>
> single_iter - clean up kick_airtime
> sched_throttle - new API and separate tasklet for throttled txqs
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index 404c5e82e4ca..023bc81bd4a0 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -242,13 +242,11 @@ EXPORT_SYMBOL(ieee80211_ctstoself_duration);
>  
>  static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
>  {
> -	bool seen_eligible = false;
>  	struct txq_info *txqi;
>  	struct sta_info *sta;
>  
>  	spin_lock_bh(&local->active_txq_lock[ac]);
>  
> - begin:
>  	if (list_empty(&local->active_txqs[ac]))
>  		goto out;
>  
> @@ -258,12 +256,12 @@ static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
>  
>  		sta = container_of(txqi->txq.sta, struct sta_info, sta);
>  
> -		if (sta->airtime[ac].deficit >= 0) {
> -			seen_eligible = true;
> -
> -			if (!test_and_clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE,
> -						&txqi->flags))
> +		if (test_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags)) {
> +			clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
> +			if (sta->airtime[ac].deficit < 0) {
> +				sta->airtime[ac].deficit += sta->airtime_weight;
>  				continue;
> +			}

This is going to break fairness; we only want to increase deficits when
all stations' deficits are negative. Hence the two loops. Did you see
any problems with those specifically?

-Toke

Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

On 2018-09-28 02:58, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
> 
>> On 2018-09-28 00:51, Toke Høiland-Jørgensen wrote:
>>> On 28 September 2018 07:29:03 CEST, Rajkumar Manoharan
>>> <rmanohar@codeaurora.org> wrote:
>>>> On 2018-09-26 17:09, Rajkumar Manoharan wrote:
>>>>> On 2018-09-26 02:22, Toke Høiland-Jørgensen wrote:
>>>>>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>>>> 
>>>>> :( Yeah... I got confused with attached soft lockup in ARM 
>>>>> platform.
>>>>> 
>>>> Toke,
>>>> 
>>>> Cause for the soft lockup exposed in multi client scenario is due to
>>>> mixed order of fq_lock and active_txqs_lock. In wake_tx_queue or
>>>> push_pending
>>>> case, driver acquires active_txq_lock first by schedule_start and
>>>> followed by
>>>> fq_lock in tx_dequeue. The same order should be maintained in sta
>>>> cleanup.
>>>> Below change fixed the issue.
>>> 
>>> Ah, great find! I'll fold this into the next version, thanks!
>>> 
>> 
>> One more thing. As I mentioned earlier, scheduling wake_txqs_tasklet
>> is heavy load and causing random rcu stall issue. Hence I added
>> another API to schedule throttled txqs once for all. Also I did
>> a cleanup in kick_airtime by traversing list only once. With these
>> changes I don't see rcu stall issue. Please review and fold them as
>> well.
>> 
>> -Rajkumar
>> 
>> 
>> single_iter - clean up kick_airtime
>> sched_throttle - new API and separate tasklet for throttled txqs
>> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
>> index 404c5e82e4ca..023bc81bd4a0 100644
>> --- a/net/mac80211/util.c
>> +++ b/net/mac80211/util.c
>> @@ -242,13 +242,11 @@ EXPORT_SYMBOL(ieee80211_ctstoself_duration);
>> 
>>  static void __ieee80211_kick_airtime(struct ieee80211_local *local, 
>> int ac)
>>  {
>> -	bool seen_eligible = false;
>>  	struct txq_info *txqi;
>>  	struct sta_info *sta;
>> 
>>  	spin_lock_bh(&local->active_txq_lock[ac]);
>> 
>> - begin:
>>  	if (list_empty(&local->active_txqs[ac]))
>>  		goto out;
>> 
>> @@ -258,12 +256,12 @@ static void __ieee80211_kick_airtime(struct 
>> ieee80211_local *local, int ac)
>> 
>>  		sta = container_of(txqi->txq.sta, struct sta_info, sta);
>> 
>> -		if (sta->airtime[ac].deficit >= 0) {
>> -			seen_eligible = true;
>> -
>> -			if (!test_and_clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE,
>> -						&txqi->flags))
>> +		if (test_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags)) {
>> +			clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags);
>> +			if (sta->airtime[ac].deficit < 0) {
>> +				sta->airtime[ac].deficit += sta->airtime_weight;
>>  				continue;
>> +			}
> 
> This is going to break fairness; we only want to increase deficits when
> all stations' deficits are negative. Hence the two loops. Did you see
> any problems with those specifically?
> 

No. I didn't see any issue but the loop won't exit until one txq becomes 
positive.
Till then the lock won't be released. Hence I converged into single 
iteration.

-Rajkumar

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Jonathan Morton]

> On 28 Sep, 2018, at 1:19 pm, Rajkumar Manoharan <rmanohar@codeaurora.org> wrote:
> 
>> This is going to break fairness; we only want to increase deficits when
>> all stations' deficits are negative. Hence the two loops. Did you see
>> any problems with those specifically?
> 
> No. I didn't see any issue but the loop won't exit until one txq becomes positive.
> Till then the lock won't be released. Hence I converged into single iteration.

The problem is that the fairness behaviour is changed when there are already positive txqs, but the first one happens to not be positive.  That's why there were two loops.

 - Jonathan Morton

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

On 2018-09-28 03:35, Jonathan Morton wrote:
>> On 28 Sep, 2018, at 1:19 pm, Rajkumar Manoharan 
>> <rmanohar@codeaurora.org> wrote:
>> 
>>> This is going to break fairness; we only want to increase deficits 
>>> when
>>> all stations' deficits are negative. Hence the two loops. Did you see
>>> any problems with those specifically?
>> 
>> No. I didn't see any issue but the loop won't exit until one txq 
>> becomes positive.
>> Till then the lock won't be released. Hence I converged into single 
>> iteration.
> 
> The problem is that the fairness behaviour is changed when there are
> already positive txqs, but the first one happens to not be positive.
> That's why there were two loops.
> 
Yeah.. Understood. we can ignore the cleanup change.

-Rajkumar

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

On 28 September 2018 12:47:51 CEST, Rajkumar Manoharan <rmanohar@codeaurora.org> wrote:
>On 2018-09-28 03:35, Jonathan Morton wrote:
>>> On 28 Sep, 2018, at 1:19 pm, Rajkumar Manoharan 
>>> <rmanohar@codeaurora.org> wrote:
>>> 
>>>> This is going to break fairness; we only want to increase deficits 
>>>> when
>>>> all stations' deficits are negative. Hence the two loops. Did you
>see
>>>> any problems with those specifically?
>>> 
>>> No. I didn't see any issue but the loop won't exit until one txq 
>>> becomes positive.
>>> Till then the lock won't be released. Hence I converged into single 
>>> iteration.
>> 
>> The problem is that the fairness behaviour is changed when there are
>> already positive txqs, but the first one happens to not be positive.
>> That's why there were two loops.
>> 
>Yeah.. Understood. we can ignore the cleanup change.

Great! I'll fold in the rest, test it with ath9k and submit as a proper patch :)

-Toke

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

On 2018-09-28 04:02, Toke Høiland-Jørgensen wrote:
> On 28 September 2018 12:47:51 CEST, Rajkumar Manoharan
> <rmanohar@codeaurora.org> wrote:
>> On 2018-09-28 03:35, Jonathan Morton wrote:
>>>> On 28 Sep, 2018, at 1:19 pm, Rajkumar Manoharan
>>>> <rmanohar@codeaurora.org> wrote:
>>>> 
>>>>> This is going to break fairness; we only want to increase deficits
>>>>> when
>>>>> all stations' deficits are negative. Hence the two loops. Did you
>> see
>>>>> any problems with those specifically?
>>>> 
>>>> No. I didn't see any issue but the loop won't exit until one txq
>>>> becomes positive.
>>>> Till then the lock won't be released. Hence I converged into single
>>>> iteration.
>>> 
>>> The problem is that the fairness behaviour is changed when there are
>>> already positive txqs, but the first one happens to not be positive.
>>> That's why there were two loops.
>>> 
>> Yeah.. Understood. we can ignore the cleanup change.
> 
> Great! I'll fold in the rest, test it with ath9k and submit as a proper 
> patch :)
> 
Thanks. I am seeing higher CPU load. Did you observe similar behavior 
with ath9k.

-Rajkumar

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

> Great! I'll fold in the rest, test it with ath9k and submit as a proper 
> patch :)
> 
Toke,

I noticed a race condition b/w sta cleanup and kick_airtime tasklet. How 
do you
plan to exit kick_airtime gracefully during sta_cleanup?

-Rajkumar

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

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

On 2018-10-01 23:58, Rajkumar Manoharan wrote:
>> Great! I'll fold in the rest, test it with ath9k and submit as a 
>> proper patch :)
>> 
> Toke,
> 
> I noticed a race condition b/w sta cleanup and kick_airtime tasklet. 
> How do you
> plan to exit kick_airtime gracefully during sta_cleanup?
> 
If kick_airtime tasklet is only used for adjusting deficit for all 
throttled txq,
then below rcu lock issue is not observed. I am testing with 50 clients 
and the
crash happens only during sta cleanup. Releasing active_txq_lock from 
tasklet is
yielding handle to txq_purge(). I am thinking of get rid of tasklet and 
handle
adjustment directly in API.


diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 0bb590928dd0..277dbf8e0a4b 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -261,14 +261,7 @@ static void __ieee80211_kick_airtime(struct 
ieee80211_local *local, int ac)

                 if (sta->airtime[ac].deficit >= 0) {
                         seen_eligible = true;
-
-                       if 
(!test_and_clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE,
-                                               &txqi->flags))
-                               continue;
-
-                       spin_unlock_bh(&local->active_txq_lock[ac]);
-                       drv_wake_tx_queue(local, txqi);
-                       spin_lock_bh(&local->active_txq_lock[ac]);
+                       clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, 
&txqi->flags);

-Rajkumar

[-- Attachment #2: rcu_race.txt --]
[-- Type: text/plain, Size: 2571 bytes --]



root@OpenWrt:/# [  363.094702] INFO: rcu_preempt self-detected stall on CPU { 0}
  (t=2101 jiffies g=2679 c=2678 q=134)
[  363.102709] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.77 #9
[  363.104717] INFO: rcu_preempt detected stalls on CPUs/tasks: { 0} (detected b
y 1, t=2102 jiffies, g=2679, c=2678, q=134)
[  363.104723] Task dump for CPU 0:
[  363.104734] swapper/0       R running      0     0      0 0x00001002
[  363.104763] [<c020ec20>] (__schedule) from [<c0218304>] (arch_cpu_idle+0x38/0
x5c)
[  363.104852] [<c0218304>] (arch_cpu_idle) from [<ffffffff>] (0xffffffff)
[  363.143167] [<c021d9ac>] (unwind_backtrace) from [<c021aa40>] (show_stack+0x1
0/0x14)
[  363.150892] [<c021aa40>] (show_stack) from [<c03c59d4>] (dump_stack+0x80/0xa0
)
[  363.158099] [<c03c59d4>] (dump_stack) from [<c026e1dc>] (rcu_check_callbacks+
0x230/0x6a8)
[  363.166258] [<c026e1dc>] (rcu_check_callbacks) from [<c0238814>] (update_proc
ess_times+0x38/0x58)
[  363.175110] [<c0238814>] (update_process_times) from [<c0276780>] (tick_sched
_timer+0x44/0x74)
[  363.183702] [<c0276780>] (tick_sched_timer) from [<c024a828>] (__run_hrtimer+
0x50/0xc8)
[  363.191686] [<c024a828>] (__run_hrtimer) from [<c024aff8>] (hrtimer_interrupt
+0x130/0x27c)
[  363.199936] [<c024aff8>] (hrtimer_interrupt) from [<c05175ac>] (msm_timer_int
errupt+0x38/0x44)
[  363.208528] [<c05175ac>] (msm_timer_interrupt) from [<c0268a6c>] (handle_perc
pu_devid_irq+0x68/0x84)
[  363.217645] [<c0268a6c>] (handle_percpu_devid_irq) from [<c02653e0>] (generic
_handle_irq+0x20/0x30)
[  363.226671] [<c02653e0>] (generic_handle_irq) from [<c021803c>] (handle_IRQ+0
x68/0x90)
[  363.234567] [<c021803c>] (handle_IRQ) from [<c02084e0>] (gic_handle_irq+0x3c/
0x60)
[  363.242120] [<c02084e0>] (gic_handle_irq) from [<c02095c0>] (__irq_svc+0x40/0
x70)
[  363.249581] Exception stack(0xc085de68 to 0xc085deb0)
[  363.254616] de60:                   00000004 db7e50a0 0000009d 00000000 d8bb8
d18 d8bb8d00
[  363.262777] de80: d8bb8c20 00000002 00000030 00000018 db7e5000 00000006 00000
000 c085deb0
[  363.270935] dea0: bfb68c98 c020cae8 60000113 ffffffff
[  363.275977] [<c02095c0>] (__irq_svc) from [<c020cae8>] (_test_and_clear_bit+0
x0/0x48)
[  363.283850] [<c020cae8>] (_test_and_clear_bit) from [<bfb68c98>] (ieee80211_k
ick_airtime+0x88/0x188 [mac80211])
[  363.293963] [<bfb68c98>] (ieee80211_kick_airtime [mac80211]) from [<c02331ec>
] (tasklet_action+0x8c/0xec)
[  363.303406] [<c02331ec>] (tasklet_action) from [<c02327e4>] (__do_softirq+0x1
04/0x294)

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

>> Great! I'll fold in the rest, test it with ath9k and submit as a proper 
>> patch :)
>> 
> Toke,
>
> I noticed a race condition b/w sta cleanup and kick_airtime tasklet.
> How do you plan to exit kick_airtime gracefully during sta_cleanup?

Ah, right, there's a lot of stuff going on before we get to purge_txq.
Hmm, I guess we should either make sure we remove the station from
active_txqs earlier in the sta cleanup process, or maybe it'd enough to
just check the removed flag in the tasklet?

Does the below patch fix the issue?

-Toke


diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 9c889da48ef0..8fa3c09d041c 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -258,6 +258,9 @@ static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
 
 		sta = container_of(txqi->txq.sta, struct sta_info, sta);
 
+		if (sta->removed)
+			continue;
+
 		if (sta->airtime[ac].deficit >= 0) {
 			seen_eligible = true;
 

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

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

On 2018-10-02 01:22, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
> 
>>> Great! I'll fold in the rest, test it with ath9k and submit as a 
>>> proper
>>> patch :)
>>> 
>> Toke,
>> 
>> I noticed a race condition b/w sta cleanup and kick_airtime tasklet.
>> How do you plan to exit kick_airtime gracefully during sta_cleanup?
> 
> Ah, right, there's a lot of stuff going on before we get to purge_txq.
> Hmm, I guess we should either make sure we remove the station from
> active_txqs earlier in the sta cleanup process, or maybe it'd enough to
> just check the removed flag in the tasklet?
> 
> Does the below patch fix the issue?
> 

No. Attaching backtrace. Any clue?

-Rajkumar

[-- Attachment #2: sta_remove.txt --]
[-- Type: text/plain, Size: 16773 bytes --]

[  612.084685] INFO: rcu_preempt self-detected stall on CPU { 0}  (t=2101 jiffie
s g=3643 c=3642 q=107)
[  612.092689] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.77 #9
[  612.094702] INFO: rcu_preempt detected stalls on CPUs/tasks: { 0} (detected b
y 1, t=2102 jiffies, g=3643, c=3642, q=107)
[  612.094705] Task dump for CPU 0:
[  612.094716] swapper/0       R running      0     0      0 0x00001002
[  612.094745] [<c020ec20>] (__schedule) from [<c0218304>] (arch_cpu_idle+0x38/0
x5c)
[  612.094834] [<c0218304>] (arch_cpu_idle) from [<ffffffff>] (0xffffffff)
[  612.133149] [<c021d9ac>] (unwind_backtrace) from [<c021aa40>] (show_stack+0x1
0/0x14)
[  612.140874] [<c021aa40>] (show_stack) from [<c03c59d4>] (dump_stack+0x80/0xa0
)
[  612.148079] [<c03c59d4>] (dump_stack) from [<c026e1dc>] (rcu_check_callbacks+
0x230/0x6a8)
[  612.156239] [<c026e1dc>] (rcu_check_callbacks) from [<c0238814>] (update_proc
ess_times+0x38/0x58)
[  612.165092] [<c0238814>] (update_process_times) from [<c0276780>] (tick_sched
_timer+0x44/0x74)
[  612.173683] [<c0276780>] (tick_sched_timer) from [<c024a828>] (__run_hrtimer+
0x50/0xc8)
[  612.181668] [<c024a828>] (__run_hrtimer) from [<c024aff8>] (hrtimer_interrupt
+0x130/0x27c)
[  612.189917] [<c024aff8>] (hrtimer_interrupt) from [<c05175ac>] (msm_timer_int
errupt+0x38/0x44)
[  612.198510] [<c05175ac>] (msm_timer_interrupt) from [<c0268a6c>] (handle_perc
pu_devid_irq+0x68/0x84)
[  612.207625] [<c0268a6c>] (handle_percpu_devid_irq) from [<c02653e0>] (generic
_handle_irq+0x20/0x30)
[  612.216653] [<c02653e0>] (generic_handle_irq) from [<c021803c>] (handle_IRQ+0
x68/0x90)
[  612.224548] [<c021803c>] (handle_IRQ) from [<c02084e0>] (gic_handle_irq+0x3c/
0x60)
[  612.232101] [<c02084e0>] (gic_handle_irq) from [<c02095c0>] (__irq_svc+0x40/0
x70)
[  612.239562] Exception stack(0xc085de68 to 0xc085deb0)
[  612.244597] de60:                   00000000 d65490a0 00000000 00000010 d4d20
d18 d4d20d00
[  612.252758] de80: d4d20c20 00000002 00000030 00000018 d6549000 00000006 00000
000 c085deb0
[  612.260917] dea0: bf84bc94 c020cb24 60000113 ffffffff
[  612.265959] [<c02095c0>] (__irq_svc) from [<c020cb24>] (_test_and_clear_bit+0
x3c/0x48)
[  612.273915] [<c020cb24>] (_test_and_clear_bit) from [<bf84bc94>] (ieee80211_k
ick_airtime+0x90/0x1a8 [mac80211])
[  612.284028] [<bf84bc94>] (ieee80211_kick_airtime [mac80211]) from [<c02331ec>
] (tasklet_action+0x8c/0xec)
[  612.293474] [<c02331ec>] (tasklet_action) from [<c02327e4>] (__do_softirq+0x1
04/0x294)
[  612.301370] [<c02327e4>] (__do_softirq) from [<c0232c60>] (irq_exit+0x9c/0x11
c)
[  612.308663] [<c0232c60>] (irq_exit) from [<c0218040>] (handle_IRQ+0x6c/0x90)
[  612.315695] [<c0218040>] (handle_IRQ) from [<c02084e0>] (gic_handle_irq+0x3c/
0x60)
[  612.323244] [<c02084e0>] (gic_handle_irq) from [<c02095c0>] (__irq_svc+0x40/0
x70)
[  612.330706] Exception stack(0xc085df60 to 0xc085dfa8)
[  612.335744] df60: ffffffed 00000000 1d3c2000 c020a040 c085c000 c085c030 fffff
fff c08b3014
[  612.343903] df80: c08643c0 c08539c8 ddffcc80 c08b3010 c086d288 c085dfa8 c0218
300 c0218304
[  612.352061] dfa0: 60000013 ffffffff
[  612.355539] [<c02095c0>] (__irq_svc) from [<c0218304>] (arch_cpu_idle+0x38/0x
5c)
[  612.362918] [<c0218304>] (arch_cpu_idle) from [<c026534c>] (cpu_startup_entry
+0xa4/0x108)
[  612.371084] [<c026534c>] (cpu_startup_entry) from [<c0832af0>] (start_kernel+
0x358/0x3cc)
[  632.204687] BUG: soft lockup - CPU#1 stuck for 22s! [nmbd:1458]
[  632.209569] Modules linked in: ath10k_pci ath10k_core ath mac80211 cfg80211 c
ompat ecm iptable_nat nf_nat_pptp nf_nat_ipv4 nf_nat_amanda nf_conntrack_pptp nf
_conntrack_ipv6 nf_conntrack_ipv4 nf_conntrack_amanda xt_time xt_tcpudp xt_tcpms
s xt_string xt_statistic xt_state xt_recent xt_quota xt_policy xt_pkttype xt_phy
sdev xt_owner xt_nat xt_multiport xt_mark xt_mac xt_limit xt_length xt_hl xt_hel
per xt_esp xt_ecn xt_dscp xt_conntrack xt_connmark xt_connlimit xt_connbytes xt_
comment xt_addrtype xt_TCPMSS xt_REDIRECT xt_LOG xt_HL xt_DSCP xt_CT xt_CLASSIFY
 usbnet ts_kmp ts_fsm ts_bm r8152 pppoe ppp_mppe ppp_async nf_nat_tftp nf_nat_sn
mp_basic nf_nat_sip nf_nat_rtsp nf_nat_proto_gre nf_nat_irc nf_nat_h323 nf_nat_f
tp nf_defrag_ipv6 nf_defrag_ipv4 nf_conntrack_tftp nf_conntrack_snmp nf_conntrac
k_sip nf_conntrack_rtsp nf_conntrack_proto_gre nf_conntrack_irc nf_conntrack_h32
3 nf_conntrack_ftp nf_conntrack_broadcast l2tp_ppp iptable_raw iptable_mangle ip
table_filter ipt_ah ipt_REJECT ipt_MASQUERADE ipt_ECN ip_tables crc_ccitt arptab
le_filter arpt_mangle arp_tables sch_teql sch_tbf sch_sfq sch_red sch_prio sch_p
ie sch_htb sch_gred sch_fq sch_dsmark sch_codel em_text em_nbyte em_meta em_cmp
cls_basic act_police act_ipt act_connmark act_skbedit act_mirred em_u32 cls_u32
cls_tcindex cls_flow cls_route cls_fw sch_hfsc sch_ingress qca_nss_ipsec qca_nss
_cfi_ocf qca_nss_tunipip6 qca_nss_tun6rd qca_nss_ipsecmgr qca_nss_cfi_cryptoapi
qca_nss_qdisc qca_nss_macsec qca_nss_crypto_tool qca_nss_crypto qca_nss_pptp ppt
p pppox qca_nss_map_t qca_nss_lag_mgr qca_nss_l2tpv2 ppp_generic slhc qca_nss_gr
e hyfi_bridging nf_nat_proto_sctp nf_nat libcrc32c ledtrig_usbdev nf_conntrack_p
roto_sctp essedma ip6t_REJECT ip6table_raw ip6table_mangle ip6table_filter ip6_t
ables x_tables qca_mcs qca_85xx_sw msdos bonding ip6_gre ip_gre gre ifb nat46 si
t qca_nss_drv l2tp_netlink l2tp_core ipcomp6 xfrm6_tunnel xfrm6_mode_tunnel xfrm
6_mode_transport xfrm6_mode_beet esp6 ah6 ipcomp xfrm4_tunnel xfrm4_mode_tunnel
xfrm4_mode_transport xfrm4_mode_beet esp4 ah4 ip6_tunnel qca_nss_gmac tunnel6 tu
nnel4 ip_tunnel snd_pcm_oss snd_mixer_oss snd_rawmidi snd_seq_device qca_ssdk af
_key xfrm_user xfrm_ipcomp xfrm_algo vfat fat ntfs qrfs nf_conntrack nls_iso8859
_1 nls_cp437 shortcut_fe_drv aq_phy shortcut_fe_ipv6 shortcut_fe sha1_generic qc
rypto cryptosoft cryptodev ocf md5 hmac ecb des_generic cbc authenc usb_storage
leds_gpio bootconfig xhci_hcd dwc3 udc_core dwc3_qcom dwc3_ipq40xx phy_qcom_ssus
b phy_qcom_hsusb phy_qca_uniphy phy_qca_baldur sd_mod ahci_platform gpio_button_
hotplug button_hotplug input_core usbcore nls_base usb_common mii [last unloaded
: compat]
[  632.439690]
[  632.441168] CPU: 1 PID: 1458 Comm: nmbd Not tainted 3.14.77 #9
[  632.446985] task: dcf5dc80 ti: d5ce4000 task.ti: d5ce4000
[  632.452369] PC is at _raw_spin_lock_bh+0x48/0x5c
[  632.457012] LR is at ieee80211_tx_dequeue+0x6e8/0x8ec [mac80211]
[  632.462956] pc : [<c0211fb4>]    lr : [<bf846224>]    psr: 20000013
[  632.462956] sp : d5ce5b20  ip : d4314038  fp : 00000000
[  632.474412] r10: d4d20cb4  r9 : d4d20ca8  r8 : d4d20cac
[  632.479622] r7 : 00000000  r6 : d6d17a80  r5 : d6997478  r4 : d4d20c20
[  632.486132] r3 : 0000d769  r2 : 0000d76a  r1 : 00000000  r0 : d4d20d00
[  632.492644] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
[  632.499760] Control: 10c5787d  Table: 57c4c06a  DAC: 00000015
[  632.505490] CPU: 1 PID: 1458 Comm: nmbd Not tainted 3.14.77 #9
[  632.511317] [<c021d9ac>] (unwind_backtrace) from [<c021aa40>] (show_stack+0x1
0/0x14)
[  632.519040] [<c021aa40>] (show_stack) from [<c03c59d4>] (dump_stack+0x80/0xa0
)
[  632.526250] [<c03c59d4>] (dump_stack) from [<c028269c>] (watchdog_timer_fn+0x
10c/0x160)
[  632.534231] [<c028269c>] (watchdog_timer_fn) from [<c024a828>] (__run_hrtimer
+0x50/0xc8)
[  632.542301] [<c024a828>] (__run_hrtimer) from [<c024aff8>] (hrtimer_interrupt
+0x130/0x27c)
[  632.550550] [<c024aff8>] (hrtimer_interrupt) from [<c05175ac>] (msm_timer_int
errupt+0x38/0x44)
[  632.559143] [<c05175ac>] (msm_timer_interrupt) from [<c0268a6c>] (handle_perc
pu_devid_irq+0x68/0x84)
[  632.568259] [<c0268a6c>] (handle_percpu_devid_irq) from [<c02653e0>] (generic
_handle_irq+0x20/0x30)
[  632.577287] [<c02653e0>] (generic_handle_irq) from [<c021803c>] (handle_IRQ+0
x68/0x90)
[  632.585184] [<c021803c>] (handle_IRQ) from [<c02084e0>] (gic_handle_irq+0x3c/
0x60)
[  632.592734] [<c02084e0>] (gic_handle_irq) from [<c02095c0>] (__irq_svc+0x40/0
x70)
[  632.600193] Exception stack(0xd5ce5ad8 to 0xd5ce5b20)
[  632.605231] 5ac0:                                                       d4d20
d00 00000000
[  632.613392] 5ae0: 0000d76a 0000d769 d4d20c20 d6997478 d6d17a80 00000000 d4d20
cac d4d20ca8
[  632.621552] 5b00: d4d20cb4 00000000 d4314038 d5ce5b20 bf846224 c0211fb4 20000
013 ffffffff
[  632.629715] [<c02095c0>] (__irq_svc) from [<c0211fb4>] (_raw_spin_lock_bh+0x4
8/0x5c)
[  632.637487] [<c0211fb4>] (_raw_spin_lock_bh) from [<bf846224>] (ieee80211_tx_
dequeue+0x6e8/0x8ec [mac80211])
[  632.647339] [<bf846224>] (ieee80211_tx_dequeue [mac80211]) from [<bf846350>]
(ieee80211_tx_dequeue+0x814/0x8ec [mac80211])
[  632.658361] [<bf846350>] (ieee80211_tx_dequeue [mac80211]) from [<bf848c28>]
(ieee80211_subif_8023_start_xmit+0x314/0x35c [mac80211])
[  632.670302] [<bf848c28>] (ieee80211_subif_8023_start_xmit [mac80211]) from [<
c059bf40>] (dev_hard_start_xmit+0x320/0x454)
[  632.681192] [<c059bf40>] (dev_hard_start_xmit) from [<c059c3ec>] (__dev_queue
_xmit+0x378/0x454)
[  632.689877] [<c059c3ec>] (__dev_queue_xmit) from [<c0655eb8>] (br_dev_queue_p
ush_xmit+0xbc/0xc4)
[  632.698640] [<c0655eb8>] (br_dev_queue_push_xmit) from [<c0655df4>] (deliver_
clone+0x40/0x48)
[  632.707145] [<c0655df4>] (deliver_clone) from [<c06561e8>] (maybe_deliver+0x5
c/0x6c)
[  632.714870] [<c06561e8>] (maybe_deliver) from [<c0656284>] (br_flood+0x8c/0xf
c)
[  632.722162] [<c0656284>] (br_flood) from [<c06563e8>] (br_flood_deliver+0x18/
0x24)
[  632.729716] [<c06563e8>] (br_flood_deliver) from [<c06543c0>] (br_dev_xmit+0x
2a8/0x2c4)
[  632.737702] [<c06543c0>] (br_dev_xmit) from [<c059bf40>] (dev_hard_start_xmit
+0x320/0x454)
[  632.745947] [<c059bf40>] (dev_hard_start_xmit) from [<c059c3ec>] (__dev_queue
_xmit+0x378/0x454)
[  632.754635] [<c059c3ec>] (__dev_queue_xmit) from [<c05c9ef8>] (ip_finish_outp
ut+0x434/0x4a4)
[  632.763054] [<c05c9ef8>] (ip_finish_output) from [<c05cbdfc>] (ip_send_skb+0x
10/0x98)
[  632.770865] [<c05cbdfc>] (ip_send_skb) from [<c05ebe44>] (udp_send_skb+0x184/
0x234)
[  632.778503] [<c05ebe44>] (udp_send_skb) from [<c05ed464>] (udp_sendmsg+0x6bc/
0x6e4)
[  632.786143] [<c05ed464>] (udp_sendmsg) from [<c0584420>] (sock_sendmsg+0x70/0
x84)
[  632.793607] [<c0584420>] (sock_sendmsg) from [<c05867a4>] (SyS_sendto+0xe4/0x
128)
[  632.801070] [<c05867a4>] (SyS_sendto) from [<c0208c80>] (ret_fast_syscall+0x0
/0x44)
[  636.124678] BUG: soft lockup - CPU#0 stuck for 22s! [swapper/0:0]
[  636.129726] Modules linked in: ath10k_pci ath10k_core ath mac80211 cfg80211 c
ompat ecm iptable_nat nf_nat_pptp nf_nat_ipv4 nf_nat_amanda nf_conntrack_pptp nf
_conntrack_ipv6 nf_conntrack_ipv4 nf_conntrack_amanda xt_time xt_tcpudp xt_tcpms
s xt_string xt_statistic xt_state xt_recent xt_quota xt_policy xt_pkttype xt_phy
sdev xt_owner xt_nat xt_multiport xt_mark xt_mac xt_limit xt_length xt_hl xt_hel
per xt_esp xt_ecn xt_dscp xt_conntrack xt_connmark xt_connlimit xt_connbytes xt_
comment xt_addrtype xt_TCPMSS xt_REDIRECT xt_LOG xt_HL xt_DSCP xt_CT xt_CLASSIFY
 usbnet ts_kmp ts_fsm ts_bm r8152 pppoe ppp_mppe ppp_async nf_nat_tftp nf_nat_sn
mp_basic nf_nat_sip nf_nat_rtsp nf_nat_proto_gre nf_nat_irc nf_nat_h323 nf_nat_f
tp nf_defrag_ipv6 nf_defrag_ipv4 nf_conntrack_tftp nf_conntrack_snmp nf_conntrac
k_sip nf_conntrack_rtsp nf_conntrack_proto_gre nf_conntrack_irc nf_conntrack_h32
3 nf_conntrack_ftp nf_conntrack_broadcast l2tp_ppp iptable_raw iptable_mangle ip
table_filter ipt_ah ipt_REJECT ipt_MASQUERADE ipt_ECN ip_tables crc_ccitt arptab
le_filter arpt_mangle arp_tables sch_teql sch_tbf sch_sfq sch_red sch_prio sch_p
ie sch_htb sch_gred sch_fq sch_dsmark sch_codel em_text em_nbyte em_meta em_cmp
cls_basic act_police act_ipt act_connmark act_skbedit act_mirred em_u32 cls_u32
cls_tcindex cls_flow cls_route cls_fw sch_hfsc sch_ingress qca_nss_ipsec qca_nss
_cfi_ocf qca_nss_tunipip6 qca_nss_tun6rd qca_nss_ipsecmgr qca_nss_cfi_cryptoapi
qca_nss_qdisc qca_nss_macsec qca_nss_crypto_tool qca_nss_crypto qca_nss_pptp ppt
p pppox qca_nss_map_t qca_nss_lag_mgr qca_nss_l2tpv2 ppp_generic slhc qca_nss_gr
e hyfi_bridging nf_nat_proto_sctp nf_nat libcrc32c ledtrig_usbdev nf_conntrack_p
roto_sctp essedma ip6t_REJECT ip6table_raw ip6table_mangle ip6table_filter ip6_t
ables x_tables qca_mcs qca_85xx_sw msdos bonding ip6_gre ip_gre gre ifb nat46 si
t qca_nss_drv l2tp_netlink l2tp_core ipcomp6 xfrm6_tunnel xfrm6_mode_tunnel xfrm
6_mode_transport xfrm6_mode_beet esp6 ah6 ipcomp xfrm4_tunnel xfrm4_mode_tunnel
xfrm4_mode_transport xfrm4_mode_beet esp4 ah4 ip6_tunnel qca_nss_gmac tunnel6 tu
nnel4 ip_tunnel snd_pcm_oss snd_mixer_oss snd_rawmidi snd_seq_device qca_ssdk af
_key xfrm_user xfrm_ipcomp xfrm_algo vfat fat ntfs qrfs nf_conntrack nls_iso8859
_1 nls_cp437 shortcut_fe_drv aq_phy shortcut_fe_ipv6 shortcut_fe sha1_generic qc
rypto cryptosoft cryptodev ocf md5 hmac ecb des_generic cbc authenc usb_storage
leds_gpio bootconfig xhci_hcd dwc3 udc_core dwc3_qcom dwc3_ipq40xx phy_qcom_ssus
b phy_qcom_hsusb phy_qca_uniphy phy_qca_baldur sd_mod ahci_platform gpio_button_
hotplug button_hotplug input_core usbcore nls_base usb_common mii [last unloaded
: compat]
[  636.359847]
[  636.361324] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.77 #9
[  636.367314] task: c0868750 ti: c085c000 task.ti: c085c000
[  636.372723] PC is at ieee80211_kick_airtime+0x7c/0x1a8 [mac80211]
[  636.378798] LR is at ieee80211_kick_airtime+0x90/0x1a8 [mac80211]
[  636.384848] pc : [<bf84bc80>]    lr : [<bf84bc94>]    psr: 20000113
[  636.384848] sp : c085deb0  ip : 00000000  fp : 00000006
[  636.396306] r10: d6549000  r9 : 00000018  r8 : 00000030
[  636.401514] r7 : 00000002  r6 : d4d20c20  r5 : d4d20d00  r4 : d4d20d18
[  636.408025] r3 : 00000000  r2 : 000000b9  r1 : 00000001  r0 : 00000000
[  636.414536] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kern
el
[  636.421827] Control: 10c5787d  Table: 586cc06a  DAC: 00000015
[  636.427557] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.77 #9
[  636.433552] [<c021d9ac>] (unwind_backtrace) from [<c021aa40>] (show_stack+0x1
0/0x14)
[  636.441276] [<c021aa40>] (show_stack) from [<c03c59d4>] (dump_stack+0x80/0xa0
)
[  636.448482] [<c03c59d4>] (dump_stack) from [<c028269c>] (watchdog_timer_fn+0x
10c/0x160)
[  636.456468] [<c028269c>] (watchdog_timer_fn) from [<c024a828>] (__run_hrtimer
+0x50/0xc8)
[  636.464539] [<c024a828>] (__run_hrtimer) from [<c024aff8>] (hrtimer_interrupt
+0x130/0x27c)
[  636.472787] [<c024aff8>] (hrtimer_interrupt) from [<c05175ac>] (msm_timer_int
errupt+0x38/0x44)
[  636.481380] [<c05175ac>] (msm_timer_interrupt) from [<c0268a6c>] (handle_perc
pu_devid_irq+0x68/0x84)
[  636.490495] [<c0268a6c>] (handle_percpu_devid_irq) from [<c02653e0>] (generic
_handle_irq+0x20/0x30)
[  636.499524] [<c02653e0>] (generic_handle_irq) from [<c021803c>] (handle_IRQ+0
x68/0x90)
[  636.507422] [<c021803c>] (handle_IRQ) from [<c02084e0>] (gic_handle_irq+0x3c/
0x60)
[  636.514972] [<c02084e0>] (gic_handle_irq) from [<c02095c0>] (__irq_svc+0x40/0
x70)
[  636.522436] Exception stack(0xc085de68 to 0xc085deb0)
[  636.527471] de60:                   00000000 00000001 000000b9 00000000 d4d20
d18 d4d20d00
[  636.535631] de80: d4d20c20 00000002 00000030 00000018 d6549000 00000006 00000
000 c085deb0
[  636.543791] dea0: bf84bc94 bf84bc80 20000113 ffffffff
[  636.548854] [<c02095c0>] (__irq_svc) from [<bf84bc80>] (ieee80211_kick_airtim
e+0x7c/0x1a8 [mac80211])
[  636.558056] [<bf84bc80>] (ieee80211_kick_airtime [mac80211]) from [<c02331ec>
] (tasklet_action+0x8c/0xec)
[  636.567578] [<c02331ec>] (tasklet_action) from [<c02327e4>] (__do_softirq+0x1
04/0x294)
[  636.575477] [<c02327e4>] (__do_softirq) from [<c0232c60>] (irq_exit+0x9c/0x11
c)
[  636.582770] [<c0232c60>] (irq_exit) from [<c0218040>] (handle_IRQ+0x6c/0x90)
[  636.589800] [<c0218040>] (handle_IRQ) from [<c02084e0>] (gic_handle_irq+0x3c/
0x60)
[  636.597351] [<c02084e0>] (gic_handle_irq) from [<c02095c0>] (__irq_svc+0x40/0
x70)
[  636.604813] Exception stack(0xc085df60 to 0xc085dfa8)
[  636.609851] df60: ffffffed 00000000 1d3c2000 c020a040 c085c000 c085c030 fffff
fff c08b3014
[  636.618009] df80: c08643c0 c08539c8 ddffcc80 c08b3010 c086d288 c085dfa8 c0218
300 c0218304
[  636.626167] dfa0: 60000013 ffffffff
[  636.629644] [<c02095c0>] (__irq_svc) from [<c0218304>] (arch_cpu_idle+0x38/0x
5c)
[  636.637024] [<c0218304>] (arch_cpu_idle) from [<c026534c>] (cpu_startup_entry
+0xa4/0x108)
[  636.645184] [<c026534c>] (cpu_startup_entry) from [<c0832af0>] (start_kernel+
0x358/0x3cc)

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> On 2018-10-02 01:22, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>> 
>>>> Great! I'll fold in the rest, test it with ath9k and submit as a 
>>>> proper
>>>> patch :)
>>>> 
>>> Toke,
>>> 
>>> I noticed a race condition b/w sta cleanup and kick_airtime tasklet.
>>> How do you plan to exit kick_airtime gracefully during sta_cleanup?
>> 
>> Ah, right, there's a lot of stuff going on before we get to purge_txq.
>> Hmm, I guess we should either make sure we remove the station from
>> active_txqs earlier in the sta cleanup process, or maybe it'd enough to
>> just check the removed flag in the tasklet?
>> 
>> Does the below patch fix the issue?
>> 
>
> No. Attaching backtrace. Any clue?

Ah, that's my bad. Just having a 'continue' there can make the function
loop forever. Oops. Try something like this instead?

-Toke


diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index eb77cf588d69..b30a4fac1d60 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -258,6 +258,9 @@ static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
 
 		sta = container_of(txqi->txq.sta, struct sta_info, sta);
 
+		if (sta->removed)
+			goto out_reschedule;
+
 		if (sta->airtime[ac].deficit >= 0) {
 			seen_eligible = true;
 
@@ -288,7 +291,13 @@ static void __ieee80211_kick_airtime(struct ieee80211_local *local, int ac)
 	}
  out:
 	rcu_read_unlock();
 	spin_unlock_bh(&local->active_txq_lock[ac]);
+	return;
+
+ out_reschedule:
+	rcu_read_unlock();
+	spin_unlock_bh(&local->active_txq_lock[ac]);
+	tasklet_schedule(&local->airtime_tasklet);
 }
 
 void ieee80211_kick_airtime(unsigned long data)

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

On 2018-10-02 12:00, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>> I noticed a race condition b/w sta cleanup and kick_airtime tasklet.
>>>> How do you plan to exit kick_airtime gracefully during sta_cleanup?
>>> 
>>> Ah, right, there's a lot of stuff going on before we get to 
>>> purge_txq.
>>> Hmm, I guess we should either make sure we remove the station from
>>> active_txqs earlier in the sta cleanup process, or maybe it'd enough 
>>> to
>>> just check the removed flag in the tasklet?
>>> 
>>> Does the below patch fix the issue?
>>> 
>> 
>> No. Attaching backtrace. Any clue?
> 
> Ah, that's my bad. Just having a 'continue' there can make the function
> loop forever. Oops. Try something like this instead?
> 

But 'continue' also used in other places. Will give a try but I think 
that
calling drv_wake_tx_queue within iteration is dangerous as it alters the 
list. no?

-Rajkumar

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

On 2018-10-02 16:07, Rajkumar Manoharan wrote:
> On 2018-10-02 12:00, Toke Høiland-Jørgensen wrote:
>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>>> I noticed a race condition b/w sta cleanup and kick_airtime tasklet.
>>>>> How do you plan to exit kick_airtime gracefully during sta_cleanup?
>>>> 
>>>> Ah, right, there's a lot of stuff going on before we get to 
>>>> purge_txq.
>>>> Hmm, I guess we should either make sure we remove the station from
>>>> active_txqs earlier in the sta cleanup process, or maybe it'd enough 
>>>> to
>>>> just check the removed flag in the tasklet?
>>>> 
>>>> Does the below patch fix the issue?
>>>> 
>>> 
>>> No. Attaching backtrace. Any clue?
>> 
>> Ah, that's my bad. Just having a 'continue' there can make the 
>> function
>> loop forever. Oops. Try something like this instead?
>> 
> 
> But 'continue' also used in other places. Will give a try but I think 
> that
> calling drv_wake_tx_queue within iteration is dangerous as it alters
> the list. no?
> 
How about below change? Just schedule first txq and remaining will be
scheduled later by driver upon tx-compl.

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 0bb590928dd0..2dbfd1d8eb5f 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -242,6 +242,7 @@ EXPORT_SYMBOL(ieee80211_ctstoself_duration);

  static void __ieee80211_kick_airtime(struct ieee80211_local *local, int 
ac)
  {
+       struct ieee80211_txq *txq;
         bool seen_eligible = false;
         struct txq_info *txqi;
         struct sta_info *sta;
@@ -261,14 +262,7 @@ static void __ieee80211_kick_airtime(struct 
ieee80211_local *local, int ac)

                 if (sta->airtime[ac].deficit >= 0) {
                         seen_eligible = true;
-
-                       if 
(!test_and_clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE,
-                                               &txqi->flags))
-                               continue;
-
-                       spin_unlock_bh(&local->active_txq_lock[ac]);
-                       drv_wake_tx_queue(local, txqi);
-                       spin_lock_bh(&local->active_txq_lock[ac]);
+                       clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, 
&txqi->flags);
                 }
         }

@@ -289,8 +283,10 @@ static void __ieee80211_kick_airtime(struct 
ieee80211_local *local, int ac)
         }
   out:
         rcu_read_unlock();
+       txq = ieee80211_next_txq(&local->hw, ac);
         spin_unlock_bh(&local->active_txq_lock[ac]);
-
+       if (txq)
+               drv_wake_tx_queue(local, to_txq_info(txq));
  }

-Rajkumar

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Rajkumar Manoharan]

On 2018-10-02 22:53, Rajkumar Manoharan wrote:

Shouldn't have to call next_txq(). It should be as below.
Anyway drv_wake_tx_queue is just an indication to driver and
driver will always dequeue first node from list.

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 2dbfd1d8eb5f..74ac0b19cd54 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -242,7 +242,7 @@ EXPORT_SYMBOL(ieee80211_ctstoself_duration);

  static void __ieee80211_kick_airtime(struct ieee80211_local *local, int 
ac)
  {
-       struct ieee80211_txq *txq;
+       struct ieee80211_txq *txq = NULL;
         bool seen_eligible = false;
         struct txq_info *txqi;
         struct sta_info *sta;
@@ -263,6 +263,7 @@ static void __ieee80211_kick_airtime(struct 
ieee80211_local *local, int ac)
                 if (sta->airtime[ac].deficit >= 0) {
                         seen_eligible = true;
                         clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, 
&txqi->flags);
+                       txq = &txqi->txq;
                 }
         }

@@ -283,7 +284,6 @@ static void __ieee80211_kick_airtime(struct 
ieee80211_local *local, int ac)
         }
   out:
         rcu_read_unlock();
-       txq = ieee80211_next_txq(&local->hw, ac);
         spin_unlock_bh(&local->active_txq_lock[ac]);

-Rajkumar

Re: [Make-wifi-fast] [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> On 2018-10-02 16:07, Rajkumar Manoharan wrote:
>> On 2018-10-02 12:00, Toke Høiland-Jørgensen wrote:
>>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>>>> I noticed a race condition b/w sta cleanup and kick_airtime tasklet.
>>>>>> How do you plan to exit kick_airtime gracefully during sta_cleanup?
>>>>> 
>>>>> Ah, right, there's a lot of stuff going on before we get to 
>>>>> purge_txq.
>>>>> Hmm, I guess we should either make sure we remove the station from
>>>>> active_txqs earlier in the sta cleanup process, or maybe it'd enough 
>>>>> to
>>>>> just check the removed flag in the tasklet?
>>>>> 
>>>>> Does the below patch fix the issue?
>>>>> 
>>>> 
>>>> No. Attaching backtrace. Any clue?
>>> 
>>> Ah, that's my bad. Just having a 'continue' there can make the 
>>> function
>>> loop forever. Oops. Try something like this instead?
>>> 
>> 
>> But 'continue' also used in other places. Will give a try but I think 
>> that
>> calling drv_wake_tx_queue within iteration is dangerous as it alters
>> the list. no?
>> 
> How about below change? Just schedule first txq and remaining will be
> scheduled later by driver upon tx-compl.

Your mail client seems to be mangling the patch somewhat, but I think I
see what your intention is. And yeah, just waking a single TXQ and
letting TX-completion do the rest is a good idea; will fold that into
the next version :)

-Toke

[PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

This adds an API to mac80211 to handle scheduling of TXQs. The interface
between driver and mac80211 for TXQ handling is changed by adding two new
functions: ieee80211_next_txq(), which will return the next TXQ to schedule
in the current round-robin rotation, and ieee80211_return_txq(), which the
driver uses to indicate that it has finished scheduling a TXQ (which will
then be put back in the scheduling rotation if it isn't empty).

The driver must call ieee80211_txq_schedule_start() at the start of each
scheduling session, and ieee80211_txq_schedule_end() at the end. The API
then guarantees that the same TXQ is not returned twice in the same
session (so a driver can loop on ieee80211_next_txq() without worrying
about breaking the loop.

Usage of the new API is optional, so drivers can be ported one at a time.
In this patch, the actual scheduling performed by mac80211 is simple
round-robin, but a subsequent commit adds airtime fairness awareness to the
scheduler.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 include/net/mac80211.h     |   62 +++++++++++++++++++++++++++++++++++++++++---
 net/mac80211/agg-tx.c      |    2 +
 net/mac80211/driver-ops.h  |    9 ++++++
 net/mac80211/ieee80211_i.h |    9 ++++++
 net/mac80211/main.c        |    5 ++++
 net/mac80211/sta_info.c    |    2 +
 net/mac80211/tx.c          |   59 +++++++++++++++++++++++++++++++++++++++++-
 7 files changed, 141 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index c4fadbafbf21..5ca1484cba58 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -108,9 +108,16 @@
  * The driver is expected to initialize its private per-queue data for stations
  * and interfaces in the .add_interface and .sta_add ops.
  *
- * The driver can't access the queue directly. To dequeue a frame, it calls
- * ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to a queue, it
- * calls the .wake_tx_queue driver op.
+ * The driver can't access the queue directly. To dequeue a frame from a
+ * txq, it calls ieee80211_tx_dequeue(). Whenever mac80211 adds a new frame to a
+ * queue, it calls the .wake_tx_queue driver op.
+ *
+ * Drivers can optionally delegate responsibility for scheduling queues to
+ * mac80211, to take advantage of airtime fairness accounting. In this case, to
+ * obtain the next queue to pull frames from, the driver calls
+ * ieee80211_next_txq(). The driver is then expected to re-schedule the txq
+ * using ieee80211_schedule_txq() if it is still active after the driver has
+ * finished pulling packets from it.
  *
  * For AP powersave TIM handling, the driver only needs to indicate if it has
  * buffered packets in the driver specific data structures by calling
@@ -6045,13 +6052,60 @@ void ieee80211_unreserve_tid(struct ieee80211_sta *sta, u8 tid);
  * ieee80211_tx_dequeue - dequeue a packet from a software tx queue
  *
  * @hw: pointer as obtained from ieee80211_alloc_hw()
- * @txq: pointer obtained from station or virtual interface
+ * @txq: pointer obtained from station or virtual interface, or from
+ *       ieee80211_next_txq()
  *
  * Returns the skb if successful, %NULL if no frame was available.
  */
 struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 				     struct ieee80211_txq *txq);
 
+/**
+ * ieee80211_next_txq - get next tx queue to pull packets from
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @ac: AC number to return packets from.
+ *
+ * Should only be called between calls to ieee80211_txq_schedule_start()
+ * and ieee80211_txq_schedule_end().
+ * Returns the next txq if successful, %NULL if no queue is eligible. If a txq
+ * is returned, it should be returned with ieee80211_return_txq() after the
+ * driver has finished scheduling it.
+ */
+struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac);
+
+/**
+ * ieee80211_return_txq - return a TXQ previously acquired by ieee80211_next_txq()
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Should only be called between calls to ieee80211_txq_schedule_start()
+ * and ieee80211_txq_schedule_end().
+ */
+void ieee80211_return_txq(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
+
+/**
+ * ieee80211_txq_schedule_start - acquire locks for safe scheduling of an AC
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @ac: AC number to acquire locks for
+ *
+ * Acquire locks needed to schedule TXQs from the given AC. Should be called
+ * before ieee80211_next_txq() or ieee80211_schedule_txq().
+ */
+void ieee80211_txq_schedule_start(struct ieee80211_hw *hw, u8 ac);
+
+/**
+ * ieee80211_txq_schedule_end - release locks for safe scheduling of an AC
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @ac: AC number to acquire locks for
+ *
+ * Release locks previously acquired by ieee80211_txq_schedule_end().
+ */
+void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac);
+
 /**
  * ieee80211_txq_get_depth - get pending frame/byte count of given txq
  *
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 69e831bc317b..e94b1a0407af 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -229,7 +229,7 @@ ieee80211_agg_start_txq(struct sta_info *sta, int tid, bool enable)
 	clear_bit(IEEE80211_TXQ_STOP, &txqi->flags);
 	local_bh_disable();
 	rcu_read_lock();
-	drv_wake_tx_queue(sta->sdata->local, txqi);
+	schedule_and_wake_txq(sta->sdata->local, txqi);
 	rcu_read_unlock();
 	local_bh_enable();
 }
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index e42c641b6190..0d47dadee747 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1173,6 +1173,15 @@ static inline void drv_wake_tx_queue(struct ieee80211_local *local,
 	local->ops->wake_tx_queue(&local->hw, &txq->txq);
 }
 
+static inline void schedule_and_wake_txq(struct ieee80211_local *local,
+					 struct txq_info *txqi)
+{
+	spin_lock_bh(&local->active_txq_lock[txqi->txq.ac]);
+	ieee80211_return_txq(&local->hw, &txqi->txq);
+	spin_unlock_bh(&local->active_txq_lock[txqi->txq.ac]);
+	drv_wake_tx_queue(local, txqi);
+}
+
 static inline int drv_can_aggregate_in_amsdu(struct ieee80211_local *local,
 					     struct sk_buff *head,
 					     struct sk_buff *skb)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index f40a2167935f..976531717902 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -829,6 +829,8 @@ enum txq_info_flags {
  *	a fq_flow which is already owned by a different tin
  * @def_cvars: codel vars for @def_flow
  * @frags: used to keep fragments created after dequeue
+ * @schedule_order: used with ieee80211_local->active_txqs
+ * @schedule_round: counter to prevent infinite loops on TXQ scheduling
  */
 struct txq_info {
 	struct fq_tin tin;
@@ -836,6 +838,8 @@ struct txq_info {
 	struct codel_vars def_cvars;
 	struct codel_stats cstats;
 	struct sk_buff_head frags;
+	struct list_head schedule_order;
+	u16 schedule_round;
 	unsigned long flags;
 
 	/* keep last! */
@@ -1127,6 +1131,11 @@ struct ieee80211_local {
 	struct codel_vars *cvars;
 	struct codel_params cparams;
 
+	/* protects active_txqs and txqi->schedule_order */
+	spinlock_t active_txq_lock[IEEE80211_NUM_ACS];
+	struct list_head active_txqs[IEEE80211_NUM_ACS];
+	u16 schedule_round[IEEE80211_NUM_ACS];
+
 	const struct ieee80211_ops *ops;
 
 	/*
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 77381017bac7..d9315de90b48 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -663,6 +663,11 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 	spin_lock_init(&local->rx_path_lock);
 	spin_lock_init(&local->queue_stop_reason_lock);
 
+	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
+		INIT_LIST_HEAD(&local->active_txqs[i]);
+		spin_lock_init(&local->active_txq_lock[i]);
+	}
+
 	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 fb8c2252ac0e..c2f5cb7df54f 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1249,7 +1249,7 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
 		if (!sta->sta.txq[i] || !txq_has_queue(sta->sta.txq[i]))
 			continue;
 
-		drv_wake_tx_queue(local, to_txq_info(sta->sta.txq[i]));
+		schedule_and_wake_txq(local, to_txq_info(sta->sta.txq[i]));
 	}
 
 	skb_queue_head_init(&pending);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index c42bfa1dcd2c..1e071121cb44 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1445,6 +1445,7 @@ void ieee80211_txq_init(struct ieee80211_sub_if_data *sdata,
 	codel_vars_init(&txqi->def_cvars);
 	codel_stats_init(&txqi->cstats);
 	__skb_queue_head_init(&txqi->frags);
+	INIT_LIST_HEAD(&txqi->schedule_order);
 
 	txqi->txq.vif = &sdata->vif;
 
@@ -1485,6 +1486,9 @@ void ieee80211_txq_purge(struct ieee80211_local *local,
 
 	fq_tin_reset(fq, tin, fq_skb_free_func);
 	ieee80211_purge_tx_queue(&local->hw, &txqi->frags);
+	spin_lock_bh(&local->active_txq_lock[txqi->txq.ac]);
+	list_del_init(&txqi->schedule_order);
+	spin_unlock_bh(&local->active_txq_lock[txqi->txq.ac]);
 }
 
 void ieee80211_txq_set_params(struct ieee80211_local *local)
@@ -1601,7 +1605,7 @@ static bool ieee80211_queue_skb(struct ieee80211_local *local,
 	ieee80211_txq_enqueue(local, txqi, skb);
 	spin_unlock_bh(&fq->lock);
 
-	drv_wake_tx_queue(local, txqi);
+	schedule_and_wake_txq(local, txqi);
 
 	return true;
 }
@@ -3623,6 +3627,59 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 }
 EXPORT_SYMBOL(ieee80211_tx_dequeue);
 
+struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+	struct txq_info *txqi = NULL;
+
+	lockdep_assert_held(&local->active_txq_lock[ac]);
+
+	txqi = list_first_entry_or_null(&local->active_txqs[ac],
+					struct txq_info,
+					schedule_order);
+
+	if (!txqi || txqi->schedule_round == local->schedule_round[ac])
+		return NULL;
+
+	list_del_init(&txqi->schedule_order);
+	txqi->schedule_round = local->schedule_round[ac];
+	return &txqi->txq;
+}
+EXPORT_SYMBOL(ieee80211_next_txq);
+
+void ieee80211_return_txq(struct ieee80211_hw *hw,
+			    struct ieee80211_txq *txq)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+	struct txq_info *txqi = to_txq_info(txq);
+
+	lockdep_assert_held(&local->active_txq_lock[txq->ac]);
+
+	if (list_empty(&txqi->schedule_order) &&
+	    (!skb_queue_empty(&txqi->frags) || txqi->tin.backlog_packets))
+		list_add_tail(&txqi->schedule_order,
+			      &local->active_txqs[txq->ac]);
+
+}
+EXPORT_SYMBOL(ieee80211_return_txq);
+
+void ieee80211_txq_schedule_start(struct ieee80211_hw *hw, u8 ac)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+
+	spin_lock_bh(&local->active_txq_lock[ac]);
+	local->schedule_round[ac]++;
+}
+EXPORT_SYMBOL(ieee80211_txq_schedule_start);
+
+void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+
+	spin_unlock_bh(&local->active_txq_lock[ac]);
+}
+EXPORT_SYMBOL(ieee80211_txq_schedule_end);
+
 void __ieee80211_subif_start_xmit(struct sk_buff *skb,
 				  struct net_device *dev,
 				  u32 info_flags)

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Rajkumar Manoharan]

On 2018-09-16 10:42, Toke Høiland-Jørgensen wrote:
> +/**
> + * ieee80211_return_txq - return a TXQ previously acquired by
> ieee80211_next_txq()
> + *
> + * @hw: pointer as obtained from ieee80211_alloc_hw()
> + * @txq: pointer obtained from station or virtual interface
> + *
> + * Should only be called between calls to 
> ieee80211_txq_schedule_start()
> + * and ieee80211_txq_schedule_end().
> + */
> +void ieee80211_return_txq(struct ieee80211_hw *hw, struct 
> ieee80211_txq *txq);
> +
> 
return_txq() should return a bool to inform the driver that whether txq 
is
queued back or not. Otherwise the same txq will be served indefinitely
until txq becomes empty. This problem occurs when the driver is running 
out
of hw descriptors or driver sends only N frames (< backlog_packets).

Also an option to add the node at head or tail would be preferred. If 
return_txq
adds node at head of list, then it is forcing the driver to serve same 
txq until it
becomes empty. Also this will not allow the driver to send N frames from 
each txqs.

> +/**
> + * ieee80211_txq_schedule_start - acquire locks for safe scheduling of 
> an AC
> + *
> + * @hw: pointer as obtained from ieee80211_alloc_hw()
> + * @ac: AC number to acquire locks for
> + *
> + * Acquire locks needed to schedule TXQs from the given AC. Should be 
> called
> + * before ieee80211_next_txq() or ieee80211_schedule_txq().
> + */
Typo error. s/schedule_txq()/return_txq()/.

-Rajkumar

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> On 2018-09-16 10:42, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> +/**
>> + * ieee80211_return_txq - return a TXQ previously acquired by
>> ieee80211_next_txq()
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @txq: pointer obtained from station or virtual interface
>> + *
>> + * Should only be called between calls to=20
>> ieee80211_txq_schedule_start()
>> + * and ieee80211_txq_schedule_end().
>> + */
>> +void ieee80211_return_txq(struct ieee80211_hw *hw, struct=20
>> ieee80211_txq *txq);
>> +
>>=20
> return_txq() should return a bool to inform the driver that whether
> txq is queued back or not.

What would the driver do with that return value, exactly?

> Otherwise the same txq will be served indefinitely until txq becomes
> empty. This problem occurs when the driver is running out of hw
> descriptors or driver sends only N frames (< backlog_packets).

No, if it's using next_txq(), the API guarantees that the same TXQ will
not be returned more than once between a set of calls to
schedule_start()/schedule_end() (by way of the seqno mechanism). I
didn't add the same check to may_transmit(), because I assumed the
driver would not be looping in this case. Is that not correct?

> Also an option to add the node at head or tail would be preferred. If
> return_txq adds node at head of list, then it is forcing the driver to
> serve same txq until it becomes empty. Also this will not allow the
> driver to send N frames from each txqs.

The whole point of this patch set is to move those kinds of decisions
out of the driver and into mac80211. The airtime scheduler won't achieve
fairness if it allows queues to be queued to the end of the rotation
before its deficit turns negative. And obviously there's some lag in
this since we're using after-the-fact airtime information.

For ath9k this has not really been a problem in my tests; if the lag
turns out to be too great for ath10k (which I suppose is a possibility
since we don't get airtime information on every TX-compl), I figure we
can use the same estimated airtime value that is used for throttling the
queues to adjust the deficit immediately...

>> +/**
>> + * ieee80211_txq_schedule_start - acquire locks for safe scheduling of=
=20
>> an AC
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @ac: AC number to acquire locks for
>> + *
>> + * Acquire locks needed to schedule TXQs from the given AC. Should be=20
>> called
>> + * before ieee80211_next_txq() or ieee80211_schedule_txq().
>> + */
> Typo error. s/schedule_txq()/return_txq()/.

Yup, will fix :)

-Toke

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Rajkumar Manoharan]

On 2018-09-18 03:29, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
> 
>> On 2018-09-16 10:42, Toke Høiland-Jørgensen wrote:
>> return_txq() should return a bool to inform the driver that whether
>> txq is queued back or not.
> 
> What would the driver do with that return value, exactly?
> 
never mind.. got lost with earlier schedule_txq API.

>> Otherwise the same txq will be served indefinitely until txq becomes
>> empty. This problem occurs when the driver is running out of hw
>> descriptors or driver sends only N frames (< backlog_packets).
> 
> No, if it's using next_txq(), the API guarantees that the same TXQ will
> not be returned more than once between a set of calls to
> schedule_start()/schedule_end() (by way of the seqno mechanism). I
> didn't add the same check to may_transmit(), because I assumed the
> driver would not be looping in this case. Is that not correct?
> 
Yeah.. you are correct. sorry for the noise.

>> Also an option to add the node at head or tail would be preferred. If
>> return_txq adds node at head of list, then it is forcing the driver to
>> serve same txq until it becomes empty. Also this will not allow the
>> driver to send N frames from each txqs.
> 
> The whole point of this patch set is to move those kinds of decisions
> out of the driver and into mac80211. The airtime scheduler won't 
> achieve
> fairness if it allows queues to be queued to the end of the rotation
> before its deficit turns negative. And obviously there's some lag in
> this since we're using after-the-fact airtime information.
> 
Hmm.. As you know ath10k kind of doing fairness by serving fixed frames
from each txq. This approach will be removed from ath10k.

> For ath9k this has not really been a problem in my tests; if the lag
> turns out to be too great for ath10k (which I suppose is a possibility
> since we don't get airtime information on every TX-compl), I figure we
> can use the same estimated airtime value that is used for throttling 
> the
> queues to adjust the deficit immediately...
> 
Thats true. I am porting Kan's changes of airtime estimation for each 
msdu
for firmware that does not report airtime.

-Rajkumar

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

>>> Also an option to add the node at head or tail would be preferred. If
>>> return_txq adds node at head of list, then it is forcing the driver to
>>> serve same txq until it becomes empty. Also this will not allow the
>>> driver to send N frames from each txqs.
>> 
>> The whole point of this patch set is to move those kinds of decisions
>> out of the driver and into mac80211. The airtime scheduler won't 
>> achieve
>> fairness if it allows queues to be queued to the end of the rotation
>> before its deficit turns negative. And obviously there's some lag in
>> this since we're using after-the-fact airtime information.
>> 
> Hmm.. As you know ath10k kind of doing fairness by serving fixed frames
> from each txq. This approach will be removed from ath10k.
>
>> For ath9k this has not really been a problem in my tests; if the lag
>> turns out to be too great for ath10k (which I suppose is a possibility
>> since we don't get airtime information on every TX-compl), I figure we
>> can use the same estimated airtime value that is used for throttling 
>> the
>> queues to adjust the deficit immediately...
>> 
> Thats true. I am porting Kan's changes of airtime estimation for each
> msdu for firmware that does not report airtime.

Right. My thinking with this was that we could put the per-frame airtime
estimation into ieee80211_tx_dequeue(), which could track the
outstanding airtime and just return NULL if it goes over the threshold.
I think this is fairly straight-forward to do on its own; the biggest
problem is probably finding the space in the mac80211 cb?

Is this what you are working on porting? Because then I'll wait for your
patch rather than starting to write this code myself :)

This mechanism on its own will get us the queue limiting and latency
reduction goodness for firmwares with deep queues. And for that it can
be completely independent of the airtime fairness scheduler, which can
use the after-tx-compl airtime information to presumably get more
accurate fairness which includes retransmissions etc.

Now, we could *also* use the ahead-of-time airtime estimation for
fairness; either just as a fallback for drivers that can't get actual
airtime usage information for the hardware, or as an alternative in
cases where it works better for other reasons. But I think that
separating the two in the initial implementation makes more sense; that
will make it easier to experiment with different combinations of the
two.

Does that make sense? :)

-Toke

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Rajkumar Manoharan]

On 2018-09-18 13:41, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
> 
>>>> Also an option to add the node at head or tail would be preferred. 
>>>> If
>>>> return_txq adds node at head of list, then it is forcing the driver 
>>>> to
>>>> serve same txq until it becomes empty. Also this will not allow the
>>>> driver to send N frames from each txqs.
>>> 
>>> The whole point of this patch set is to move those kinds of decisions
>>> out of the driver and into mac80211. The airtime scheduler won't
>>> achieve
>>> fairness if it allows queues to be queued to the end of the rotation
>>> before its deficit turns negative. And obviously there's some lag in
>>> this since we're using after-the-fact airtime information.
>>> 
>> Hmm.. As you know ath10k kind of doing fairness by serving fixed 
>> frames
>> from each txq. This approach will be removed from ath10k.
>> 
>>> For ath9k this has not really been a problem in my tests; if the lag
>>> turns out to be too great for ath10k (which I suppose is a 
>>> possibility
>>> since we don't get airtime information on every TX-compl), I figure 
>>> we
>>> can use the same estimated airtime value that is used for throttling
>>> the
>>> queues to adjust the deficit immediately...
>>> 
>> Thats true. I am porting Kan's changes of airtime estimation for each
>> msdu for firmware that does not report airtime.
> 
> Right. My thinking with this was that we could put the per-frame 
> airtime
> estimation into ieee80211_tx_dequeue(), which could track the
> outstanding airtime and just return NULL if it goes over the threshold.
> I think this is fairly straight-forward to do on its own; the biggest
> problem is probably finding the space in the mac80211 cb?
> 
> Is this what you are working on porting? Because then I'll wait for 
> your
> patch rather than starting to write this code myself :)
> 
Kind of.. something like below.

tx_dequeue(){
     compute airtime_est from last_tx_rate
     if (sta->airtime[ac].deficit < airtime_est)
         return NULL;
     dequeue skb and store airtime_est in cb
}

Unfortunately ath10k is not reporting last_tx_rate in tx_status(). So I
also applied this "ath10k: report tx rate using ieee80211_tx_status" 
change.

> This mechanism on its own will get us the queue limiting and latency
> reduction goodness for firmwares with deep queues. And for that it can
> be completely independent of the airtime fairness scheduler, which can
> use the after-tx-compl airtime information to presumably get more
> accurate fairness which includes retransmissions etc.
> 
> Now, we could *also* use the ahead-of-time airtime estimation for
> fairness; either just as a fallback for drivers that can't get actual
> airtime usage information for the hardware, or as an alternative in
> cases where it works better for other reasons. But I think that
> separating the two in the initial implementation makes more sense; that
> will make it easier to experiment with different combinations of the
> two.
> 
> Does that make sense? :)
> 
Completely agree. I was thinking of using this as fallback for devices
that does not report airtime but tx rate.

-Rajkumar

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> On 2018-09-18 13:41, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>>=20
>>>>> Also an option to add the node at head or tail would be preferred.=20
>>>>> If
>>>>> return_txq adds node at head of list, then it is forcing the driver=20
>>>>> to
>>>>> serve same txq until it becomes empty. Also this will not allow the
>>>>> driver to send N frames from each txqs.
>>>>=20
>>>> The whole point of this patch set is to move those kinds of decisions
>>>> out of the driver and into mac80211. The airtime scheduler won't
>>>> achieve
>>>> fairness if it allows queues to be queued to the end of the rotation
>>>> before its deficit turns negative. And obviously there's some lag in
>>>> this since we're using after-the-fact airtime information.
>>>>=20
>>> Hmm.. As you know ath10k kind of doing fairness by serving fixed=20
>>> frames
>>> from each txq. This approach will be removed from ath10k.
>>>=20
>>>> For ath9k this has not really been a problem in my tests; if the lag
>>>> turns out to be too great for ath10k (which I suppose is a=20
>>>> possibility
>>>> since we don't get airtime information on every TX-compl), I figure=20
>>>> we
>>>> can use the same estimated airtime value that is used for throttling
>>>> the
>>>> queues to adjust the deficit immediately...
>>>>=20
>>> Thats true. I am porting Kan's changes of airtime estimation for each
>>> msdu for firmware that does not report airtime.
>>=20
>> Right. My thinking with this was that we could put the per-frame=20
>> airtime
>> estimation into ieee80211_tx_dequeue(), which could track the
>> outstanding airtime and just return NULL if it goes over the threshold.
>> I think this is fairly straight-forward to do on its own; the biggest
>> problem is probably finding the space in the mac80211 cb?
>>=20
>> Is this what you are working on porting? Because then I'll wait for=20
>> your
>> patch rather than starting to write this code myself :)
>>=20
> Kind of.. something like below.
>
> tx_dequeue(){
>      compute airtime_est from last_tx_rate
>      if (sta->airtime[ac].deficit < airtime_est)
>          return NULL;
>      dequeue skb and store airtime_est in cb
> }

I think I would decouple it further and not use the deficit. But rather:

 tx_dequeue(){
      if (sta->airtime[ac].outstanding > AIRTIME_OUTSTANDING_MAX)
        return NULL
      compute airtime_est from last_tx_rate
      dequeue skb and store airtime_est in cb
      sta->airtime[ac].outstanding +=3D airtime_est;
 }

> Unfortunately ath10k is not reporting last_tx_rate in tx_status(). So
> I also applied this "ath10k: report tx rate using ieee80211_tx_status"
> change.

Yeah, that and the patch that computes the last used rate will probably
be necessary; but they can be pretty much applied as-is, right?

>> This mechanism on its own will get us the queue limiting and latency
>> reduction goodness for firmwares with deep queues. And for that it can
>> be completely independent of the airtime fairness scheduler, which can
>> use the after-tx-compl airtime information to presumably get more
>> accurate fairness which includes retransmissions etc.
>>=20
>> Now, we could *also* use the ahead-of-time airtime estimation for
>> fairness; either just as a fallback for drivers that can't get actual
>> airtime usage information for the hardware, or as an alternative in
>> cases where it works better for other reasons. But I think that
>> separating the two in the initial implementation makes more sense; that
>> will make it easier to experiment with different combinations of the
>> two.
>>=20
>> Does that make sense? :)
>>=20
> Completely agree. I was thinking of using this as fallback for devices
> that does not report airtime but tx rate.

Great! Seems we are converging on a workable solution, then :)

-Toke

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Kalle Valo]

Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:

>> Unfortunately ath10k is not reporting last_tx_rate in tx_status(). So
>> I also applied this "ath10k: report tx rate using ieee80211_tx_status"
>> change.
>
> Yeah, that and the patch that computes the last used rate will probably
> be necessary; but they can be pretty much applied as-is, right?

Unfortunately not. I think the plan is now to follow Johannes' proposal:

   "I'd recommend against doing this and disentangling the necessary
    code in mac80211, e.g. with ieee80211_tx_status_ext() or adding
    similar APIs."

   https://patchwork.kernel.org/patch/10353959/

--=20
Kalle Valo

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Toke Høiland-Jørgensen]

Kalle Valo <kvalo@codeaurora.org> writes:

> Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk> writes:
>
>>> Unfortunately ath10k is not reporting last_tx_rate in tx_status(). So
>>> I also applied this "ath10k: report tx rate using ieee80211_tx_status"
>>> change.
>>
>> Yeah, that and the patch that computes the last used rate will probably
>> be necessary; but they can be pretty much applied as-is, right?
>
> Unfortunately not. I think the plan is now to follow Johannes' proposal:
>
>    "I'd recommend against doing this and disentangling the necessary
>     code in mac80211, e.g. with ieee80211_tx_status_ext() or adding
>     similar APIs."
>
>    https://patchwork.kernel.org/patch/10353959/

Ahh, right... *that* patch :)

Was thinking on this one with the "as-is" comment:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/=
588189

-Toke

Re: [PATCH RFC v4 1/4] mac80211: Add TXQ scheduling API

[Rajkumar Manoharan]

On 2018-09-19 07:50, Toke Høiland-Jørgensen wrote:
> Kalle Valo <kvalo@codeaurora.org> writes:
> 
>> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>> 
>>>> Unfortunately ath10k is not reporting last_tx_rate in tx_status(). 
>>>> So
>>>> I also applied this "ath10k: report tx rate using 
>>>> ieee80211_tx_status"
>>>> change.
>>> 
>>> Yeah, that and the patch that computes the last used rate will 
>>> probably
>>> be necessary; but they can be pretty much applied as-is, right?
>> 
>> Unfortunately not. I think the plan is now to follow Johannes' 
>> proposal:
>> 
>>    "I'd recommend against doing this and disentangling the necessary
>>     code in mac80211, e.g. with ieee80211_tx_status_ext() or adding
>>     similar APIs."
>> 
>>    https://patchwork.kernel.org/patch/10353959/
> 
> Ahh, right... *that* patch :)
> 
> Was thinking on this one with the "as-is" comment:
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/588189
> 
It is useful only when the driver calls tx_status_noskb(). It was 
recommended not to
call tx_status() and tx_status_noskb() APIs from same driver. Hence Anil 
was trying
to piggyback tx rate report by tx_status itself.

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/1e034d84bd444fd29b7f902c5e033a8c737a58b2%5E%21/
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/2a8da427fc9dfb527516e7ac395b1e6af73bff84%5E%21/

-Rajkumar

[PATCH RFC v4 4/4] ath9k: Switch to mac80211 TXQ scheduling and airtime APIs

[Toke Høiland-Jørgensen]

This moves the ath9k driver to use the mac80211 TXQ scheduling and
airtime accounting APIs, removing the corresponding state tracking
inside the driver.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 drivers/net/wireless/ath/ath9k/ath9k.h     |   14 --
 drivers/net/wireless/ath/ath9k/debug.c     |    3 
 drivers/net/wireless/ath/ath9k/debug.h     |    8 -
 drivers/net/wireless/ath/ath9k/debug_sta.c |   54 ------
 drivers/net/wireless/ath/ath9k/init.c      |    3 
 drivers/net/wireless/ath/ath9k/recv.c      |    9 -
 drivers/net/wireless/ath/ath9k/xmit.c      |  244 ++++++++--------------------
 7 files changed, 73 insertions(+), 262 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 21ba20981a80..90b56766dab1 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -112,8 +112,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct ath_descdma *dd,
 #define ATH_TXFIFO_DEPTH           8
 #define ATH_TX_ERROR               0x01
 
-#define ATH_AIRTIME_QUANTUM        300 /* usec */
-
 /* Stop tx traffic 1ms before the GO goes away */
 #define ATH_P2P_PS_STOP_TIME       1000
 
@@ -246,10 +244,8 @@ struct ath_atx_tid {
 	s8 bar_index;
 	bool active;
 	bool clear_ps_filter;
-	bool has_queued;
 };
 
-void __ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
 void ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
 
 struct ath_node {
@@ -263,12 +259,9 @@ struct ath_node {
 
 	bool sleeping;
 	bool no_ps_filter;
-	s64 airtime_deficit[IEEE80211_NUM_ACS];
-	u32 airtime_rx_start;
 
 #ifdef CONFIG_ATH9K_STATION_STATISTICS
 	struct ath_rx_rate_stats rx_rate_stats;
-	struct ath_airtime_stats airtime_stats;
 #endif
 	u8 key_idx[4];
 
@@ -986,11 +979,6 @@ void ath_ant_comb_scan(struct ath_softc *sc, struct ath_rx_status *rs);
 
 #define ATH9K_NUM_CHANCTX  2 /* supports 2 operating channels */
 
-#define AIRTIME_USE_TX		BIT(0)
-#define AIRTIME_USE_RX		BIT(1)
-#define AIRTIME_USE_NEW_QUEUES	BIT(2)
-#define AIRTIME_ACTIVE(flags) (!!(flags & (AIRTIME_USE_TX|AIRTIME_USE_RX)))
-
 struct ath_softc {
 	struct ieee80211_hw *hw;
 	struct device *dev;
@@ -1034,8 +1022,6 @@ struct ath_softc {
 	short nbcnvifs;
 	unsigned long ps_usecount;
 
-	u16 airtime_flags; /* AIRTIME_* */
-
 	struct ath_rx rx;
 	struct ath_tx tx;
 	struct ath_beacon beacon;
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index 0a6eb8a8c1ed..f928d3a07caa 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -1456,9 +1456,6 @@ int ath9k_init_debug(struct ath_hw *ah)
 #endif
 	debugfs_create_file("tpc", 0600, sc->debug.debugfs_phy, sc, &fops_tpc);
 
-	debugfs_create_u16("airtime_flags", 0600,
-			   sc->debug.debugfs_phy, &sc->airtime_flags);
-
 	debugfs_create_file("nf_override", 0600,
 			    sc->debug.debugfs_phy, sc, &fops_nf_override);
 
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 249f8141cd00..559d9628f280 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -319,20 +319,12 @@ ath9k_debug_sync_cause(struct ath_softc *sc, u32 sync_cause)
 void ath_debug_rate_stats(struct ath_softc *sc,
 			  struct ath_rx_status *rs,
 			  struct sk_buff *skb);
-void ath_debug_airtime(struct ath_softc *sc,
-		       struct ath_node *an,
-		       u32 rx, u32 tx);
 #else
 static inline void ath_debug_rate_stats(struct ath_softc *sc,
 					struct ath_rx_status *rs,
 					struct sk_buff *skb)
 {
 }
-static inline void ath_debug_airtime(struct ath_softc *sc,
-			      struct ath_node *an,
-			      u32 rx, u32 tx)
-{
-}
 #endif /* CONFIG_ATH9K_STATION_STATISTICS */
 
 #endif /* DEBUG_H */
diff --git a/drivers/net/wireless/ath/ath9k/debug_sta.c b/drivers/net/wireless/ath/ath9k/debug_sta.c
index a6f45f1bb5bb..bb6f3250aa30 100644
--- a/drivers/net/wireless/ath/ath9k/debug_sta.c
+++ b/drivers/net/wireless/ath/ath9k/debug_sta.c
@@ -242,59 +242,6 @@ static const struct file_operations fops_node_recv = {
 	.llseek = default_llseek,
 };
 
-void ath_debug_airtime(struct ath_softc *sc,
-		struct ath_node *an,
-		u32 rx,
-		u32 tx)
-{
-	struct ath_airtime_stats *astats = &an->airtime_stats;
-
-	astats->rx_airtime += rx;
-	astats->tx_airtime += tx;
-}
-
-static ssize_t read_airtime(struct file *file, char __user *user_buf,
-			size_t count, loff_t *ppos)
-{
-	struct ath_node *an = file->private_data;
-	struct ath_airtime_stats *astats;
-	static const char *qname[4] = {
-		"VO", "VI", "BE", "BK"
-	};
-	u32 len = 0, size = 256;
-	char *buf;
-	size_t retval;
-	int i;
-
-	buf = kzalloc(size, GFP_KERNEL);
-	if (buf == NULL)
-		return -ENOMEM;
-
-	astats = &an->airtime_stats;
-
-	len += scnprintf(buf + len, size - len, "RX: %u us\n", astats->rx_airtime);
-	len += scnprintf(buf + len, size - len, "TX: %u us\n", astats->tx_airtime);
-	len += scnprintf(buf + len, size - len, "Deficit: ");
-	for (i = 0; i < 4; i++)
-		len += scnprintf(buf+len, size - len, "%s: %lld us ", qname[i], an->airtime_deficit[i]);
-	if (len < size)
-		buf[len++] = '\n';
-
-	retval = simple_read_from_buffer(user_buf, count, ppos, buf, len);
-	kfree(buf);
-
-	return retval;
-}
-
-
-static const struct file_operations fops_airtime = {
-	.read = read_airtime,
-	.open = simple_open,
-	.owner = THIS_MODULE,
-	.llseek = default_llseek,
-};
-
-
 void ath9k_sta_add_debugfs(struct ieee80211_hw *hw,
 			   struct ieee80211_vif *vif,
 			   struct ieee80211_sta *sta,
@@ -304,5 +251,4 @@ void ath9k_sta_add_debugfs(struct ieee80211_hw *hw,
 
 	debugfs_create_file("node_aggr", 0444, dir, an, &fops_node_aggr);
 	debugfs_create_file("node_recv", 0444, dir, an, &fops_node_recv);
-	debugfs_create_file("airtime", 0444, dir, an, &fops_airtime);
 }
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index c070a9e51ebf..45a31f8a1514 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -676,8 +676,6 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 
 	/* Will be cleared in ath9k_start() */
 	set_bit(ATH_OP_INVALID, &common->op_flags);
-	sc->airtime_flags = (AIRTIME_USE_TX | AIRTIME_USE_RX |
-			     AIRTIME_USE_NEW_QUEUES);
 
 	sc->sc_ah = ah;
 	sc->dfs_detector = dfs_pattern_detector_init(common, NL80211_DFS_UNSET);
@@ -1013,6 +1011,7 @@ static void ath9k_set_hw_capab(struct ath_softc *sc, struct ieee80211_hw *hw)
 	SET_IEEE80211_PERM_ADDR(hw, common->macaddr);
 
 	wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST);
+	wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_AIRTIME_FAIRNESS);
 }
 
 int ath9k_init_device(u16 devid, struct ath_softc *sc,
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index a8ac42c96d71..fef8fbe12f45 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1054,14 +1054,7 @@ static void ath_rx_count_airtime(struct ath_softc *sc,
 						len, rxs->rate_idx, is_sp);
 	}
 
- 	if (!!(sc->airtime_flags & AIRTIME_USE_RX)) {
-		spin_lock_bh(&acq->lock);
-		an->airtime_deficit[acno] -= airtime;
-		if (an->airtime_deficit[acno] <= 0)
-			__ath_tx_queue_tid(sc, ATH_AN_2_TID(an, tidno));
-		spin_unlock_bh(&acq->lock);
-	}
-	ath_debug_airtime(sc, an, airtime, 0);
+	ieee80211_sta_register_airtime(sta, tidno, 0, airtime);
 exit:
 	rcu_read_unlock();
 }
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 43b6c8508e49..ce6763c3c3ee 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -113,44 +113,14 @@ void ath_txq_unlock_complete(struct ath_softc *sc, struct ath_txq *txq)
 		ath_tx_status(hw, skb);
 }
 
-void __ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid)
-{
-	struct ath_vif *avp = (struct ath_vif *) tid->an->vif->drv_priv;
-	struct ath_chanctx *ctx = avp->chanctx;
-	struct ath_acq *acq;
-	struct list_head *tid_list;
-	u8 acno = TID_TO_WME_AC(tid->tidno);
-
-	if (!ctx || !list_empty(&tid->list))
-		return;
-
-
-	acq = &ctx->acq[acno];
-	if ((sc->airtime_flags & AIRTIME_USE_NEW_QUEUES) &&
-	    tid->an->airtime_deficit[acno] > 0)
-		tid_list = &acq->acq_new;
-	else
-		tid_list = &acq->acq_old;
-
-	list_add_tail(&tid->list, tid_list);
-}
-
 void ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid)
 {
-	struct ath_vif *avp = (struct ath_vif *) tid->an->vif->drv_priv;
-	struct ath_chanctx *ctx = avp->chanctx;
-	struct ath_acq *acq;
+	struct ieee80211_txq *queue = container_of(
+		(void *)tid, struct ieee80211_txq, drv_priv);
 
-	if (!ctx || !list_empty(&tid->list))
-		return;
-
-	acq = &ctx->acq[TID_TO_WME_AC(tid->tidno)];
-	spin_lock_bh(&acq->lock);
-	__ath_tx_queue_tid(sc, tid);
-	spin_unlock_bh(&acq->lock);
+	ieee80211_return_txq(sc->hw, queue);
 }
 
-
 void ath9k_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *queue)
 {
 	struct ath_softc *sc = hw->priv;
@@ -163,11 +133,7 @@ void ath9k_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *queue)
 		tid->tidno);
 
 	ath_txq_lock(sc, txq);
-
-	tid->has_queued = true;
-	ath_tx_queue_tid(sc, tid);
 	ath_txq_schedule(sc, txq);
-
 	ath_txq_unlock(sc, txq);
 }
 
@@ -217,8 +183,8 @@ ath_get_skb_tid(struct ath_softc *sc, struct ath_node *an, struct sk_buff *skb)
 	return ATH_AN_2_TID(an, tidno);
 }
 
-static struct sk_buff *
-ath_tid_pull(struct ath_atx_tid *tid)
+static int
+ath_tid_pull(struct ath_atx_tid *tid, struct sk_buff **skbuf)
 {
 	struct ieee80211_txq *txq = container_of((void*)tid, struct ieee80211_txq, drv_priv);
 	struct ath_softc *sc = tid->an->sc;
@@ -229,20 +195,15 @@ ath_tid_pull(struct ath_atx_tid *tid)
 	};
 	struct sk_buff *skb;
 	struct ath_frame_info *fi;
-	int q;
-
-	if (!tid->has_queued)
-		return NULL;
+	int q, ret;
 
 	skb = ieee80211_tx_dequeue(hw, txq);
-	if (!skb) {
-		tid->has_queued = false;
-		return NULL;
-	}
+	if (!skb)
+		return -ENOENT;
 
-	if (ath_tx_prepare(hw, skb, &txctl)) {
+	if ((ret = ath_tx_prepare(hw, skb, &txctl))) {
 		ieee80211_free_txskb(hw, skb);
-		return NULL;
+		return ret;
 	}
 
 	q = skb_get_queue_mapping(skb);
@@ -252,24 +213,19 @@ ath_tid_pull(struct ath_atx_tid *tid)
 		++tid->txq->pending_frames;
 	}
 
-	return skb;
-}
-
-
-static bool ath_tid_has_buffered(struct ath_atx_tid *tid)
-{
-	return !skb_queue_empty(&tid->retry_q) || tid->has_queued;
-}
+	*skbuf = skb;
+	return 0;
+ }
 
-static struct sk_buff *ath_tid_dequeue(struct ath_atx_tid *tid)
+static int ath_tid_dequeue(struct ath_atx_tid *tid,
+			   struct sk_buff **skb)
 {
-	struct sk_buff *skb;
+	int ret = 0;
+	*skb = __skb_dequeue(&tid->retry_q);
+	if (!*skb)
+		ret = ath_tid_pull(tid, skb);
 
-	skb = __skb_dequeue(&tid->retry_q);
-	if (!skb)
-		skb = ath_tid_pull(tid);
-
-	return skb;
+	return ret;
 }
 
 static void ath_tx_flush_tid(struct ath_softc *sc, struct ath_atx_tid *tid)
@@ -365,11 +321,12 @@ static void ath_tid_drain(struct ath_softc *sc, struct ath_txq *txq,
 	struct list_head bf_head;
 	struct ath_tx_status ts;
 	struct ath_frame_info *fi;
+	int ret;
 
 	memset(&ts, 0, sizeof(ts));
 	INIT_LIST_HEAD(&bf_head);
 
-	while ((skb = ath_tid_dequeue(tid))) {
+	while ((ret = ath_tid_dequeue(tid, &skb)) == 0) {
 		fi = get_frame_info(skb);
 		bf = fi->bf;
 
@@ -681,7 +638,6 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
 		skb_queue_splice_tail(&bf_pending, &tid->retry_q);
 		if (!an->sleeping) {
 			ath_tx_queue_tid(sc, tid);
-
 			if (ts->ts_status & (ATH9K_TXERR_FILT | ATH9K_TXERR_XRETRY))
 				tid->clear_ps_filter = true;
 		}
@@ -708,11 +664,9 @@ static bool bf_is_ampdu_not_probing(struct ath_buf *bf)
     return bf_isampdu(bf) && !(info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE);
 }
 
-static void ath_tx_count_airtime(struct ath_softc *sc, struct ath_node *an,
-				 struct ath_atx_tid *tid, struct ath_buf *bf,
-				 struct ath_tx_status *ts)
+static void ath_tx_count_airtime(struct ath_softc *sc, struct ieee80211_sta *sta,
+				 struct ath_buf *bf, struct ath_tx_status *ts)
 {
-	struct ath_txq *txq = tid->txq;
 	u32 airtime = 0;
 	int i;
 
@@ -722,17 +676,7 @@ static void ath_tx_count_airtime(struct ath_softc *sc, struct ath_node *an,
 		airtime += rate_dur * bf->rates[i].count;
 	}
 
-	if (sc->airtime_flags & AIRTIME_USE_TX) {
-		int q = txq->mac80211_qnum;
-		struct ath_acq *acq = &sc->cur_chan->acq[q];
-
-		spin_lock_bh(&acq->lock);
-		an->airtime_deficit[q] -= airtime;
-		if (an->airtime_deficit[q] <= 0)
-			__ath_tx_queue_tid(sc, tid);
-		spin_unlock_bh(&acq->lock);
-	}
-	ath_debug_airtime(sc, an, 0, airtime);
+	ieee80211_sta_register_airtime(sta, ts->tid, airtime, 0);
 }
 
 static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
@@ -762,7 +706,7 @@ static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
 	if (sta) {
 		struct ath_node *an = (struct ath_node *)sta->drv_priv;
 		tid = ath_get_skb_tid(sc, an, bf->bf_mpdu);
-		ath_tx_count_airtime(sc, an, tid, bf, ts);
+		ath_tx_count_airtime(sc, sta, bf, ts);
 		if (ts->ts_status & (ATH9K_TXERR_FILT | ATH9K_TXERR_XRETRY))
 			tid->clear_ps_filter = true;
 	}
@@ -946,20 +890,21 @@ static int ath_compute_num_delims(struct ath_softc *sc, struct ath_atx_tid *tid,
 	return ndelim;
 }
 
-static struct ath_buf *
+static int
 ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
-			struct ath_atx_tid *tid)
+			struct ath_atx_tid *tid, struct ath_buf **buf)
 {
 	struct ieee80211_tx_info *tx_info;
 	struct ath_frame_info *fi;
-	struct sk_buff *skb, *first_skb = NULL;
 	struct ath_buf *bf;
+	struct sk_buff *skb, *first_skb = NULL;
 	u16 seqno;
+	int ret;
 
 	while (1) {
-		skb = ath_tid_dequeue(tid);
-		if (!skb)
-			break;
+		ret = ath_tid_dequeue(tid, &skb);
+		if (ret < 0)
+			return ret;
 
 		fi = get_frame_info(skb);
 		bf = fi->bf;
@@ -991,7 +936,7 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
 
 		if (!(tx_info->flags & IEEE80211_TX_CTL_AMPDU)) {
 			bf->bf_state.bf_type = 0;
-			return bf;
+			break;
 		}
 
 		bf->bf_state.bf_type = BUF_AMPDU | BUF_AGGR;
@@ -1010,7 +955,7 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
 					first_skb = skb;
 				continue;
 			}
-			break;
+			return -EINPROGRESS;
 		}
 
 		if (tid->bar_index > ATH_BA_INDEX(tid->seq_start, seqno)) {
@@ -1027,10 +972,11 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
 		if (bf_isampdu(bf))
 			ath_tx_addto_baw(sc, tid, bf);
 
-		return bf;
+		break;
 	}
 
-	return NULL;
+	*buf = bf;
+	return 0;
 }
 
 static int
@@ -1040,7 +986,7 @@ ath_tx_form_aggr(struct ath_softc *sc, struct ath_txq *txq,
 {
 #define PADBYTES(_len) ((4 - ((_len) % 4)) % 4)
 	struct ath_buf *bf = bf_first, *bf_prev = NULL;
-	int nframes = 0, ndelim;
+	int nframes = 0, ndelim, ret;
 	u16 aggr_limit = 0, al = 0, bpad = 0,
 	    al_delta, h_baw = tid->baw_size / 2;
 	struct ieee80211_tx_info *tx_info;
@@ -1092,7 +1038,9 @@ ath_tx_form_aggr(struct ath_softc *sc, struct ath_txq *txq,
 
 		bf_prev = bf;
 
-		bf = ath_tx_get_tid_subframe(sc, txq, tid);
+		ret = ath_tx_get_tid_subframe(sc, txq, tid, &bf);
+		if (ret < 0)
+			break;
 	}
 	goto finish;
 stop:
@@ -1489,7 +1437,7 @@ ath_tx_form_burst(struct ath_softc *sc, struct ath_txq *txq,
 		  struct ath_buf *bf_first)
 {
 	struct ath_buf *bf = bf_first, *bf_prev = NULL;
-	int nframes = 0;
+	int nframes = 0, ret;
 
 	do {
 		struct ieee80211_tx_info *tx_info;
@@ -1503,8 +1451,8 @@ ath_tx_form_burst(struct ath_softc *sc, struct ath_txq *txq,
 		if (nframes >= 2)
 			break;
 
-		bf = ath_tx_get_tid_subframe(sc, txq, tid);
-		if (!bf)
+		ret = ath_tx_get_tid_subframe(sc, txq, tid, &bf);
+		if (ret < 0)
 			break;
 
 		tx_info = IEEE80211_SKB_CB(bf->bf_mpdu);
@@ -1517,30 +1465,27 @@ ath_tx_form_burst(struct ath_softc *sc, struct ath_txq *txq,
 	} while (1);
 }
 
-static bool ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
-			      struct ath_atx_tid *tid)
+static int ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
+			     struct ath_atx_tid *tid)
 {
-	struct ath_buf *bf;
+	struct ath_buf *bf = NULL;
 	struct ieee80211_tx_info *tx_info;
 	struct list_head bf_q;
-	int aggr_len = 0;
+	int aggr_len = 0, ret;
 	bool aggr;
 
-	if (!ath_tid_has_buffered(tid))
-		return false;
-
 	INIT_LIST_HEAD(&bf_q);
 
-	bf = ath_tx_get_tid_subframe(sc, txq, tid);
-	if (!bf)
-		return false;
+	ret = ath_tx_get_tid_subframe(sc, txq, tid, &bf);
+	if (ret < 0)
+		return ret;
 
 	tx_info = IEEE80211_SKB_CB(bf->bf_mpdu);
 	aggr = !!(tx_info->flags & IEEE80211_TX_CTL_AMPDU);
 	if ((aggr && txq->axq_ampdu_depth >= ATH_AGGR_MIN_QDEPTH) ||
 	    (!aggr && txq->axq_depth >= ATH_NON_AGGR_MIN_QDEPTH)) {
 		__skb_queue_tail(&tid->retry_q, bf->bf_mpdu);
-		return false;
+		return -EBUSY;
 	}
 
 	ath_set_rates(tid->an->vif, tid->an->sta, bf);
@@ -1550,7 +1495,7 @@ static bool ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
 		ath_tx_form_burst(sc, txq, tid, &bf_q, bf);
 
 	if (list_empty(&bf_q))
-		return false;
+		return -EAGAIN;
 
 	if (tid->clear_ps_filter || tid->an->no_ps_filter) {
 		tid->clear_ps_filter = false;
@@ -1559,7 +1504,7 @@ static bool ath_tx_sched_aggr(struct ath_softc *sc, struct ath_txq *txq,
 
 	ath_tx_fill_desc(sc, bf, txq, aggr_len);
 	ath_tx_txqaddbuf(sc, txq, &bf_q, false);
-	return true;
+	return 0;
 }
 
 int ath_tx_aggr_start(struct ath_softc *sc, struct ieee80211_sta *sta,
@@ -1622,28 +1567,16 @@ void ath_tx_aggr_sleep(struct ieee80211_sta *sta, struct ath_softc *sc,
 {
 	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
 	struct ath_atx_tid *tid;
-	struct ath_txq *txq;
 	int tidno;
 
 	ath_dbg(common, XMIT, "%s called\n", __func__);
 
 	for (tidno = 0; tidno < IEEE80211_NUM_TIDS; tidno++) {
 		tid = ath_node_to_tid(an, tidno);
-		txq = tid->txq;
-
-		ath_txq_lock(sc, txq);
-
-		if (list_empty(&tid->list)) {
-			ath_txq_unlock(sc, txq);
-			continue;
-		}
 
 		if (!skb_queue_empty(&tid->retry_q))
 			ieee80211_sta_set_buffered(sta, tid->tidno, true);
 
-		list_del_init(&tid->list);
-
-		ath_txq_unlock(sc, txq);
 	}
 }
 
@@ -1662,11 +1595,12 @@ void ath_tx_aggr_wakeup(struct ath_softc *sc, struct ath_node *an)
 
 		ath_txq_lock(sc, txq);
 		tid->clear_ps_filter = true;
-		if (ath_tid_has_buffered(tid)) {
+		if (!skb_queue_empty(&tid->retry_q)) {
 			ath_tx_queue_tid(sc, tid);
 			ath_txq_schedule(sc, txq);
 		}
 		ath_txq_unlock_complete(sc, txq);
+
 	}
 }
 
@@ -1697,9 +1631,9 @@ void ath9k_release_buffered_frames(struct ieee80211_hw *hw,
 	struct ath_txq *txq = sc->tx.uapsdq;
 	struct ieee80211_tx_info *info;
 	struct list_head bf_q;
-	struct ath_buf *bf_tail = NULL, *bf;
+	struct ath_buf *bf_tail = NULL, *bf = NULL;
 	int sent = 0;
-	int i;
+	int i, ret;
 
 	INIT_LIST_HEAD(&bf_q);
 	for (i = 0; tids && nframes; i++, tids >>= 1) {
@@ -1712,8 +1646,8 @@ void ath9k_release_buffered_frames(struct ieee80211_hw *hw,
 
 		ath_txq_lock(sc, tid->txq);
 		while (nframes > 0) {
-			bf = ath_tx_get_tid_subframe(sc, sc->tx.uapsdq, tid);
-			if (!bf)
+			ret = ath_tx_get_tid_subframe(sc, sc->tx.uapsdq, tid, &bf);
+			if (ret < 0)
 				break;
 
 			ath9k_set_moredata(sc, bf, true);
@@ -1979,11 +1913,11 @@ void ath_tx_cleanupq(struct ath_softc *sc, struct ath_txq *txq)
  */
 void ath_txq_schedule(struct ath_softc *sc, struct ath_txq *txq)
 {
+	struct ieee80211_hw *hw = sc->hw;
 	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	struct ieee80211_txq *queue;
 	struct ath_atx_tid *tid;
-	struct list_head *tid_list;
-	struct ath_acq *acq;
-	bool active = AIRTIME_ACTIVE(sc->airtime_flags);
+	int ret;
 
 	if (txq->mac80211_qnum < 0)
 		return;
@@ -1991,58 +1925,26 @@ void ath_txq_schedule(struct ath_softc *sc, struct ath_txq *txq)
 	if (test_bit(ATH_OP_HW_RESET, &common->op_flags))
 		return;
 
+	ieee80211_txq_schedule_start(hw, txq->mac80211_qnum);
 	spin_lock_bh(&sc->chan_lock);
 	rcu_read_lock();
-	acq = &sc->cur_chan->acq[txq->mac80211_qnum];
 
 	if (sc->cur_chan->stopped)
 		goto out;
 
-begin:
-	tid_list = &acq->acq_new;
-	if (list_empty(tid_list)) {
-		tid_list = &acq->acq_old;
-		if (list_empty(tid_list))
-			goto out;
-	}
-	tid = list_first_entry(tid_list, struct ath_atx_tid, list);
-
-	if (active && tid->an->airtime_deficit[txq->mac80211_qnum] <= 0) {
-		spin_lock_bh(&acq->lock);
-		tid->an->airtime_deficit[txq->mac80211_qnum] += ATH_AIRTIME_QUANTUM;
-		list_move_tail(&tid->list, &acq->acq_old);
-		spin_unlock_bh(&acq->lock);
-		goto begin;
-	}
-
-	if (!ath_tid_has_buffered(tid)) {
-		spin_lock_bh(&acq->lock);
-		if ((tid_list == &acq->acq_new) && !list_empty(&acq->acq_old))
-			list_move_tail(&tid->list, &acq->acq_old);
-		else {
-			list_del_init(&tid->list);
-		}
-		spin_unlock_bh(&acq->lock);
-		goto begin;
-	}
+	while ((queue = ieee80211_next_txq(hw, txq->mac80211_qnum))) {
+		tid = (struct ath_atx_tid *) queue->drv_priv;
 
+		ret = ath_tx_sched_aggr(sc, txq, tid);
+		ath_dbg(common, QUEUE, "ath_tx_sched_aggr returned %d\n", ret);
 
-	/*
-	 * If we succeed in scheduling something, immediately restart to make
-	 * sure we keep the HW busy.
-	 */
-	if(ath_tx_sched_aggr(sc, txq, tid)) {
-		if (!active) {
-			spin_lock_bh(&acq->lock);
-			list_move_tail(&tid->list, &acq->acq_old);
-			spin_unlock_bh(&acq->lock);
-		}
-		goto begin;
+		ieee80211_return_txq(hw, queue);
 	}
 
 out:
 	rcu_read_unlock();
 	spin_unlock_bh(&sc->chan_lock);
+	ieee80211_txq_schedule_end(hw, txq->mac80211_qnum);
 }
 
 void ath_txq_schedule_all(struct ath_softc *sc)
@@ -2886,9 +2788,6 @@ void ath_tx_node_init(struct ath_softc *sc, struct ath_node *an)
 	struct ath_atx_tid *tid;
 	int tidno, acno;
 
-	for (acno = 0; acno < IEEE80211_NUM_ACS; acno++)
-		an->airtime_deficit[acno] = ATH_AIRTIME_QUANTUM;
-
 	for (tidno = 0; tidno < IEEE80211_NUM_TIDS; tidno++) {
 		tid = ath_node_to_tid(an, tidno);
 		tid->an        = an;
@@ -2898,7 +2797,6 @@ void ath_tx_node_init(struct ath_softc *sc, struct ath_node *an)
 		tid->baw_head  = tid->baw_tail = 0;
 		tid->active	   = false;
 		tid->clear_ps_filter = true;
-		tid->has_queued  = false;
 		__skb_queue_head_init(&tid->retry_q);
 		INIT_LIST_HEAD(&tid->list);
 		acno = TID_TO_WME_AC(tidno);

[PATCH RFC v4 2/4] cfg80211: Add airtime statistics and settings

[Toke Høiland-Jørgensen]

This adds TX airtime statistics to the cfg80211 station dump (to go along
with the RX info already present), and adds a new parameter to set the
airtime weight of each station. The latter allows userspace to implement
policies for different stations by varying their weights.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 include/net/cfg80211.h       |   10 +++++++++-
 include/uapi/linux/nl80211.h |   15 +++++++++++++++
 net/wireless/nl80211.c       |   29 +++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9f3ed79c39d7..91d6fc222263 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -990,6 +990,7 @@ enum station_parameters_apply_mask {
  * @support_p2p_ps: information if station supports P2P PS mechanism
  * @he_capa: HE capabilities of station
  * @he_capa_len: the length of the HE capabilities
+ * @airtime_weight: airtime scheduler weight for this station
  */
 struct station_parameters {
 	const u8 *supported_rates;
@@ -1019,6 +1020,7 @@ struct station_parameters {
 	int support_p2p_ps;
 	const struct ieee80211_he_cap_elem *he_capa;
 	u8 he_capa_len;
+	u16 airtime_weight;
 };
 
 /**
@@ -1286,6 +1288,8 @@ struct cfg80211_tid_stats {
  * @rx_beacon_signal_avg: signal strength average (in dBm) for beacons received
  *	from this peer
  * @rx_duration: aggregate PPDU duration(usecs) for all the frames from a peer
+ * @tx_duration: aggregate PPDU duration(usecs) for all the frames to a peer
+ * @airtime_weight: current airtime scheduling weight
  * @pertid: per-TID statistics, see &struct cfg80211_tid_stats, using the last
  *	(IEEE80211_NUM_TIDS) index for MSDUs not encapsulated in QoS-MPDUs.
  *	Note that this doesn't use the @filled bit, but is used if non-NULL.
@@ -1332,10 +1336,12 @@ struct station_info {
 
 	u32 expected_throughput;
 
-	u64 rx_beacon;
+	u64 tx_duration;
 	u64 rx_duration;
+	u64 rx_beacon;
 	u8 rx_beacon_signal_avg;
 	struct cfg80211_tid_stats *pertid;
+	u16 airtime_weight;
 	s8 ack_signal;
 	s8 avg_ack_signal;
 };
@@ -2363,6 +2369,8 @@ enum wiphy_params_flags {
 	WIPHY_PARAM_TXQ_QUANTUM		= 1 << 8,
 };
 
+#define IEEE80211_DEFAULT_AIRTIME_WEIGHT	256
+
 /**
  * struct cfg80211_pmksa - PMK Security Association
  *
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index cfc94178d608..3664bdc7c8c1 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2241,6 +2241,9 @@ enum nl80211_commands {
  *	association request when used with NL80211_CMD_NEW_STATION). Can be set
  *	only if %NL80211_STA_FLAG_WME is set.
  *
+ * @NL80211_ATTR_AIRTIME_WEIGHT: Station's weight when scheduled by the airtime
+ *      scheduler.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2682,6 +2685,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_HE_CAPABILITY,
 
+	NL80211_ATTR_AIRTIME_WEIGHT,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -3051,6 +3056,9 @@ enum nl80211_sta_bss_param {
  * @NL80211_STA_INFO_PAD: attribute used for padding for 64-bit alignment
  * @NL80211_STA_INFO_ACK_SIGNAL: signal strength of the last ACK frame(u8, dBm)
  * @NL80211_STA_INFO_ACK_SIGNAL_AVG: avg signal strength of ACK frames (s8, dBm)
+ * @NL80211_STA_INFO_TX_DURATION: aggregate PPDU duration for all frames
+ *	sent to the station (u64, usec)
+ * @NL80211_STA_INFO_AIRTIME_WEIGHT: current airtime weight for station (u16)
  * @__NL80211_STA_INFO_AFTER_LAST: internal
  * @NL80211_STA_INFO_MAX: highest possible station info attribute
  */
@@ -3091,6 +3099,8 @@ enum nl80211_sta_info {
 	NL80211_STA_INFO_PAD,
 	NL80211_STA_INFO_ACK_SIGNAL,
 	NL80211_STA_INFO_ACK_SIGNAL_AVG,
+	NL80211_STA_INFO_TX_DURATION,
+	NL80211_STA_INFO_AIRTIME_WEIGHT,
 
 	/* keep last */
 	__NL80211_STA_INFO_AFTER_LAST,
@@ -5231,6 +5241,10 @@ enum nl80211_feature_flags {
  *      if this flag is not set. Ignoring this can leak clear text packets and/or
  *      freeze the connection.
  *
+ * @NL80211_EXT_FEATURE_AIRTIME_FAIRNESS: Driver supports getting airtime
+ *      fairness for transmitted packets and has enabled airtime fairness
+ *      scheduling.
+ *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
  */
@@ -5269,6 +5283,7 @@ enum nl80211_ext_feature_index {
 	NL80211_EXT_FEATURE_SCAN_RANDOM_SN,
 	NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT,
 	NL80211_EXT_FEATURE_CAN_REPLACE_PTK0,
+	NL80211_EXT_FEATURE_AIRTIME_FAIRNESS,
 
 	/* add new features before the definition below */
 	NUM_NL80211_EXT_FEATURES,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d5f9b5235cdd..9db19ae57716 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -430,6 +430,8 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[NL80211_ATTR_TXQ_QUANTUM] = { .type = NLA_U32 },
 	[NL80211_ATTR_HE_CAPABILITY] = { .type = NLA_BINARY,
 					 .len = NL80211_HE_MAX_CAPABILITY_LEN },
+
+	[NL80211_ATTR_AIRTIME_WEIGHT] = { .type = NLA_U16 },
 };
 
 /* policy for the key attributes */
@@ -4656,6 +4658,11 @@ static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid,
 	PUT_SINFO(PLID, plid, u16);
 	PUT_SINFO(PLINK_STATE, plink_state, u8);
 	PUT_SINFO_U64(RX_DURATION, rx_duration);
+	PUT_SINFO_U64(TX_DURATION, tx_duration);
+
+	if (wiphy_ext_feature_isset(&rdev->wiphy,
+				    NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
+		PUT_SINFO(AIRTIME_WEIGHT, airtime_weight, u16);
 
 	switch (rdev->wiphy.signal_type) {
 	case CFG80211_SIGNAL_TYPE_MBM:
@@ -5293,6 +5300,17 @@ static int nl80211_set_station(struct sk_buff *skb, struct genl_info *info)
 			nla_get_u8(info->attrs[NL80211_ATTR_OPMODE_NOTIF]);
 	}
 
+	if (info->attrs[NL80211_ATTR_AIRTIME_WEIGHT]) {
+		if (!wiphy_ext_feature_isset(&rdev->wiphy,
+					     NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
+			return -EOPNOTSUPP;
+
+		params.airtime_weight =
+			nla_get_u16(info->attrs[NL80211_ATTR_AIRTIME_WEIGHT]);
+		if (!params.airtime_weight)
+			return -EINVAL;
+	}
+
 	/* Include parameters for TDLS peer (will check later) */
 	err = nl80211_set_station_tdls(info, &params);
 	if (err)
@@ -5431,6 +5449,17 @@ static int nl80211_new_station(struct sk_buff *skb, struct genl_info *info)
 			return -EINVAL;
 	}
 
+	if (info->attrs[NL80211_ATTR_AIRTIME_WEIGHT]) {
+		if (!wiphy_ext_feature_isset(&rdev->wiphy,
+					     NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
+			return -EOPNOTSUPP;
+
+		params.airtime_weight =
+			nla_get_u16(info->attrs[NL80211_ATTR_AIRTIME_WEIGHT]);
+		if (!params.airtime_weight)
+			return -EINVAL;
+	}
+
 	err = nl80211_parse_sta_channel_info(info, &params);
 	if (err)
 		return err;

Re: [PATCH RFC v4 0/4] Move TXQ scheduling into mac80211

[Rajkumar Manoharan]

On 2018-09-16 10:42, Toke Høiland-Jørgensen wrote:
> Another update, addressing most of the concerns raised in the last 
> round:
> 
> - Added schedule_start()/end() functions that adds locking around the
>   whole scheduling operation, which means we can get rid of the 'first'
>   parameter to ieee80211_next_txq().
> 
Toke,

Wouldn't this start()/end() APIs cause deadlock if mac80211 tries to 
acquire
active_txq_lock[ac] again? Or am I missing?

schedule_start()
while (next_txq()) {
   push_txq -> tx_dequeue()
   return_txq()
}
schedule_end()

tx_dequeue()
  ieee80211_free_txskb
   -> ieee80211_report_used_skb
    -> ieee80211_tdls_td_tx_handle
     -> ieee80211_subif_start_xmit
     -> __ieee80211_subif_start_xmit
       -> ieee80211_xmit_fast
         -> ieee80211_queue_skb
          -> schedule_and_wake_txq

-Rajkumar

Re: [PATCH RFC v4 0/4] Move TXQ scheduling into mac80211

[Toke Høiland-Jørgensen]

Rajkumar Manoharan <rmanohar@codeaurora.org> writes:

> On 2018-09-16 10:42, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Another update, addressing most of the concerns raised in the last=20
>> round:
>>=20
>> - Added schedule_start()/end() functions that adds locking around the
>>   whole scheduling operation, which means we can get rid of the 'first'
>>   parameter to ieee80211_next_txq().
>>=20
> Toke,
>
> Wouldn't this start()/end() APIs cause deadlock if mac80211 tries to=20
> acquire
> active_txq_lock[ac] again? Or am I missing?
>
> schedule_start()
> while (next_txq()) {
>    push_txq -> tx_dequeue()
>    return_txq()
> }
> schedule_end()
>
> tx_dequeue()
>   ieee80211_free_txskb
>    -> ieee80211_report_used_skb
>     -> ieee80211_tdls_td_tx_handle
>      -> ieee80211_subif_start_xmit
>      -> __ieee80211_subif_start_xmit
>        -> ieee80211_xmit_fast
>          -> ieee80211_queue_skb
>           -> schedule_and_wake_txq

Hmm, yeah, that call sequence would certainly deadlock. But earlier, I
think; ieee80211_queue_skb() already takes fq->lock, which is being held
by tx_dequeue(), so this would deadlock on that?

I guess retransmits of TDLS teardown packets don't happen so often?
Otherwise someone would have run into this by now? Or are those frames
not being transmitted through the TXQs at all?

In any case it does seem a bit dangerous to have a possible path where
ieee80211_free_txskb() will result in a call to
ieee80211_subif_start_xmit(). So we should probably fix that in any
case?

-Toke