| Message ID | 20210319232800.0e876c800866.Id2b66eb5a17f3869b776c39b5ca713272ea09d5d@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: don't apply flow control on management frames | expand |
Johannes Berg <johannes@sipsolutions.net> writes: > From: Johannes Berg <johannes.berg@intel.com> > > 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 <johannes.berg@intel.com> > --- > 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 <jbenc@suse.cz> > * Copyright 2007 Johannes Berg <johannes@sipsolutions.net> > * 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
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,
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
Johannes Berg <johannes@sipsolutions.net> 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 <toke@redhat.com>
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 <jbenc@suse.cz> * Copyright 2007 Johannes Berg <johannes@sipsolutions.net> * 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)