diff mbox series

mac80211: Simplify queue selection

Message ID 20220510155828.9406-1-alexander@wetzel-home.de
State New
Headers show
Series mac80211: Simplify queue selection | expand

Commit Message

Alexander Wetzel May 10, 2022, 3:58 p.m. UTC 
Let iTXQ drivers also register four queues in netdev and move queue
assignment to ndo_select_queue(), like it's done for other drivers.

This gets rid of a special case in mac80211 and also increases the
chance that when we call skb_get_hash() the skb is still hot in the CPU
buffers.

Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
---
This patch is currently mostly an attempt to review our queueing
procedure. It's also assuming the patch offered in
https://lore.kernel.org/linux-wireless/20220507083706.384513-1-alexander@wetzel-home.de/
has been merged. Without that there will be one merge conflict in
ieee80211_tx_control_port(). But it only drops skb_get_hash() there,
which is also in the function without the patch.

I've only a basic understanding about QoS and qdisc and I may miss
some obvious reason why we can't use ndo_select_queue() with iTXQ
drivers. Looking at the code I just started wondering why we have
all the special handling when there seems to be no pressing need.
Now the additional sta lookup may make it a bit slower. But on the other
side we move skb_get_hash() up in the call chain, increasing the change
the skb is still in the CPU buffers. (Which according to our discussion
in https://lore.kernel.org/linux-wireless/875ymf263a.fsf@toke.dk/ seems
to be the only reason for the call.)

'1974da8b31e6 ("mac80211: when using iTXQ, select the queue in
ieee80211_subif_start_xmit")' from Felix claims "mac80211 is using its
internal queues anyway". But following that logic we can always bypass
ndo_select_queue(). Or even stop registering the handler at all.
(Which is something I also consider.)

I still don't understand why we don't want to use qdisc with the iTXQ
drivers. I now just made sure we don't start using qdiscs with this
patch to start with the least invasive approach. Anyone able to shed some
light on that?

I've also test this patch with an iwlmvm card and it looks like it's just
doing what I expect:
netdev is indeed accepting IEEE80211_AC_BE (2) as queue number and the
warning from netdev_cap_txqueue() is not triggered.

The card also looks unchanged when I check it with "tc qdisc show":
	qdisc noqueue 0: dev wlp0s12f0 root refcnt 2

Alexander

 net/mac80211/iface.c | 10 +++++-----
 net/mac80211/tx.c    | 13 -------------
 net/mac80211/wme.c   |  5 ++---
 3 files changed, 7 insertions(+), 21 deletions(-)

Comments

Alexander Wetzel May 15, 2022, 11:10 a.m. UTC | #1 
On 10.05.22 18:22, Johannes Berg wrote:
> On Tue, 2022-05-10 at 18:21 +0200, Felix Fietkau wrote:
>>
>> To fix this properly, we'd need to move to iTXQ for all drivers (by
>> having mac80211 push packets via drv_tx calls after pulling from iTXQ).
>> This can probably be done without having to modify the drivers.
>>
> 
> I looked at this, and I kind of really want to do that, but it's not
> _entirely_ trivial... If somebody's sufficiently motivated I'd
> definitely be happy :)

No promise I'll see that trough... But I'll start to look into that.
While I don't have much time for coding at the moment this sounds 
worthwhile and instructive at the same time.

Also thanks to all for the other feedback's. I'm starting to see the 
woods and not only the trees:-)


Alexander
diff mbox series

Patch

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 41531478437c..98b499197c41 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -2009,13 +2009,13 @@  int ieee80211_if_add(struct ieee80211_local *local, const char *name,
 			txq_size += sizeof(struct txq_info) +
 				    local->hw.txq_data_size;
 
-		if (local->ops->wake_tx_queue) {
+		if (local->ops->wake_tx_queue)
 			if_setup = ieee80211_if_setup_no_queue;
-		} else {
+		else
 			if_setup = ieee80211_if_setup;
-			if (local->hw.queues >= IEEE80211_NUM_ACS)
-				txqs = IEEE80211_NUM_ACS;
-		}
+
+		if (local->hw.queues >= IEEE80211_NUM_ACS)
+			txqs = IEEE80211_NUM_ACS;
 
 		ndev = alloc_netdev_mqs(size + txq_size,
 					name, name_assign_type,
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 2fabf6c4547c..2298e2e1a4ce 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -4181,12 +4181,6 @@  void __ieee80211_subif_start_xmit(struct sk_buff *skb,
 	if (IS_ERR(sta))
 		sta = NULL;
 
-	if (local->ops->wake_tx_queue) {
-		u16 queue = __ieee80211_select_queue(sdata, sta, skb);
-		skb_set_queue_mapping(skb, queue);
-		skb_get_hash(skb);
-	}
-
 	ieee80211_aggr_check(sdata, sta, skb);
 
 	sk_pacing_shift_update(skb->sk, sdata->local->hw.tx_sk_pacing_shift);
@@ -4442,12 +4436,6 @@  static void ieee80211_8023_xmit(struct ieee80211_sub_if_data *sdata,
 	struct tid_ampdu_tx *tid_tx;
 	u8 tid;
 
-	if (local->ops->wake_tx_queue) {
-		u16 queue = __ieee80211_select_queue(sdata, sta, skb);
-		skb_set_queue_mapping(skb, queue);
-		skb_get_hash(skb);
-	}
-
 	if (unlikely(test_bit(SCAN_SW_SCANNING, &local->scanning)) &&
 	    test_bit(SDATA_STATE_OFFCHANNEL, &sdata->state))
 		goto out_free;
@@ -5772,7 +5760,6 @@  int ieee80211_tx_control_port(struct wiphy *wiphy, struct net_device *dev,
 
 	/* skb bypassed queue selection in net/core/dev.c, do it now */
 	skb_set_queue_mapping(skb, ieee80211_select_queue(sdata, skb));
-	skb_get_hash(skb);
 
 	/* mutex lock is only needed for incrementing the cookie counter */
 	mutex_lock(&local->mtx);
diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c
index 774afefbe0b0..5047faee6974 100644
--- a/net/mac80211/wme.c
+++ b/net/mac80211/wme.c
@@ -186,9 +186,8 @@  u16 ieee80211_select_queue(struct ieee80211_sub_if_data *sdata,
 	const u8 *ra = NULL;
 	u16 ret;
 
-	/* when using iTXQ, we can do this later */
-	if (local->ops->wake_tx_queue)
-		return 0;
+	/* Calculate hash early, hopefully it's still in the CPU buffer */
+	skb_get_hash(skb);
 
 	if (local->hw.queues < IEEE80211_NUM_ACS || skb->len < 6) {
 		skb->priority = 0; /* required for correct WPA/11i MIC */