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

[Rajkumar Manoharan <rmanohar@codeaurora.org>]

[-- 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;