From: Johannes Berg <[email protected]>
In some cases (depending on the driver, but it's true e.g. for
iwlwifi) we're using an internal TXQ for management packets,
mostly to simplify the code and to have a place to queue them.
However, it appears that in certain cases we can confuse the
code and management frames are dropped, which is certainly not
what we want.
Short-circuit the processing of management frames. To keep the
impact minimal, only put them on the frags queue and check the
tid == management only for doing that and to skip the airtime
fairness checks, if applicable.
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/tx.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 5d06de61047a..b2d09acb9fb0 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -5,7 +5,7 @@
* Copyright 2006-2007 Jiri Benc <[email protected]>
* Copyright 2007 Johannes Berg <[email protected]>
* Copyright 2013-2014 Intel Mobile Communications GmbH
- * Copyright (C) 2018-2020 Intel Corporation
+ * Copyright (C) 2018-2021 Intel Corporation
*
* Transmit and frame generation functions.
*/
@@ -1388,8 +1388,17 @@ static void ieee80211_txq_enqueue(struct ieee80211_local *local,
ieee80211_set_skb_enqueue_time(skb);
spin_lock_bh(&fq->lock);
- fq_tin_enqueue(fq, tin, flow_idx, skb,
- fq_skb_free_func);
+ /*
+ * For management frames, don't really apply codel etc.,
+ * we don't want to apply any shaping or anything we just
+ * want to simplify the driver API by having them on the
+ * txqi.
+ */
+ if (unlikely(txqi->txq.tid == IEEE80211_NUM_TIDS))
+ __skb_queue_tail(&txqi->frags, skb);
+ else
+ fq_tin_enqueue(fq, tin, flow_idx, skb,
+ fq_skb_free_func);
spin_unlock_bh(&fq->lock);
}
@@ -3835,6 +3844,9 @@ bool ieee80211_txq_airtime_check(struct ieee80211_hw *hw,
if (!txq->sta)
return true;
+ if (unlikely(txq->tid == IEEE80211_NUM_TIDS))
+ return true;
+
sta = container_of(txq->sta, struct sta_info, sta);
if (atomic_read(&sta->airtime[txq->ac].aql_tx_pending) <
sta->airtime[txq->ac].aql_limit_low)
--
2.30.2
Johannes Berg <[email protected]> writes:
> From: Johannes Berg <[email protected]>
>
> In some cases (depending on the driver, but it's true e.g. for
> iwlwifi) we're using an internal TXQ for management packets,
> mostly to simplify the code and to have a place to queue them.
> However, it appears that in certain cases we can confuse the
> code and management frames are dropped, which is certainly not
> what we want.
>
> Short-circuit the processing of management frames. To keep the
> impact minimal, only put them on the frags queue and check the
> tid == management only for doing that and to skip the airtime
> fairness checks, if applicable.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> net/mac80211/tx.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 5d06de61047a..b2d09acb9fb0 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -5,7 +5,7 @@
> * Copyright 2006-2007 Jiri Benc <[email protected]>
> * Copyright 2007 Johannes Berg <[email protected]>
> * Copyright 2013-2014 Intel Mobile Communications GmbH
> - * Copyright (C) 2018-2020 Intel Corporation
> + * Copyright (C) 2018-2021 Intel Corporation
> *
> * Transmit and frame generation functions.
> */
> @@ -1388,8 +1388,17 @@ static void ieee80211_txq_enqueue(struct ieee80211_local *local,
> ieee80211_set_skb_enqueue_time(skb);
>
> spin_lock_bh(&fq->lock);
> - fq_tin_enqueue(fq, tin, flow_idx, skb,
> - fq_skb_free_func);
> + /*
> + * For management frames, don't really apply codel etc.,
> + * we don't want to apply any shaping or anything we just
> + * want to simplify the driver API by having them on the
> + * txqi.
> + */
> + if (unlikely(txqi->txq.tid == IEEE80211_NUM_TIDS))
> + __skb_queue_tail(&txqi->frags, skb);
> + else
> + fq_tin_enqueue(fq, tin, flow_idx, skb,
> + fq_skb_free_func);
One consequence of this is that we create a strict priority queue for
management frames. With all the possibilities for badness (such as the
ability of starving all other queues) that carries with it. I guess
that's probably fine for management frames, though, right? As in, there
is some other mechanism that prevents abuse of this?
-Toke
On Sat, 2021-03-20 at 01:13 +0100, Toke Høiland-Jørgensen wrote:
>
> One consequence of this is that we create a strict priority queue for
> management frames.
Yes, for iwlwifi that's actually a change. For everyone else (not
setting BUFF_MMPDU_TXQ or STA_MMPDU_TXQ) it already is since it goes
directly to ->tx() and from there to the hardware queue(s).
> With all the possibilities for badness (such as the
> ability of starving all other queues) that carries with it. I guess
> that's probably fine for management frames, though, right? As in, there
> is some other mechanism that prevents abuse of this?
Well, there's just not that many management frames to start with? And
only wpa_supplicant (or root in general) can create them. So I don't
think we need to worry about that yet. With QoS-mgmt frames we might
eventually want to think about that, but even there I'd say we never
really want to drop them.
johannes
Hi,
On Mon, 2021-03-22 at 10:43 +0100, Maxime Bizon wrote:
> > Short-circuit the processing of management frames. To keep the
> > impact minimal, only put them on the frags queue and check the
> > tid == management only for doing that and to skip the airtime
> > fairness checks, if applicable.
>
> After your patch, what are the actual effects of HW_STA_MMPDU_TXQ and
> HW_BUFF_MMPDU_TXQ ?
Well, my patch doesn't change the effect of those significantly.
The idea for iwlwifi was that it doesn't actually like ->tx() to get
called, but much prefers a TXQ where the frame is, and then it can pull
it whenever it can transmit it.
This was the key requirement here, and it doesn't change: instead of
tx() getting called with the frames, the frames go to the TXQ instead
and wake_tx_queue() is called, and then the driver later pulls the
frames and pushes them to the hardware.
What does change is that management frames are no longer subject to
codel and inter-flow issues, and also note that the hash of a management
frame isn't actually well-defined.
johannes
On Fri, 2021-03-19 at 23:28 +0100, Johannes Berg wrote:
Hello Johannes,
>
> Short-circuit the processing of management frames. To keep the
> impact minimal, only put them on the frags queue and check the
> tid == management only for doing that and to skip the airtime
> fairness checks, if applicable.
After your patch, what are the actual effects of HW_STA_MMPDU_TXQ and
HW_BUFF_MMPDU_TXQ ?
Thanks,
--
Maxime
Johannes Berg <[email protected]> writes:
> On Sat, 2021-03-20 at 01:13 +0100, Toke Høiland-Jørgensen wrote:
>>
>> One consequence of this is that we create a strict priority queue for
>> management frames.
>
> Yes, for iwlwifi that's actually a change. For everyone else (not
> setting BUFF_MMPDU_TXQ or STA_MMPDU_TXQ) it already is since it goes
> directly to ->tx() and from there to the hardware queue(s).
Ah, right, of course; so not much change at all. Cool.
>> With all the possibilities for badness (such as the
>> ability of starving all other queues) that carries with it. I guess
>> that's probably fine for management frames, though, right? As in, there
>> is some other mechanism that prevents abuse of this?
>
> Well, there's just not that many management frames to start with? And
> only wpa_supplicant (or root in general) can create them. So I don't
> think we need to worry about that yet. With QoS-mgmt frames we might
> eventually want to think about that, but even there I'd say we never
> really want to drop them.
Yup, that's what I meant with "some other mechanism to prevent abuse".
Great.
Feel free to add my:
Acked-by: Toke Høiland-Jørgensen <[email protected]>