Re: [PATCH] mac80211: use non-zero TID only for QoS frames
Johannes Berg writes: > On Wed, 2018-09-05 at 13:07 +0200, Toke Høiland-Jørgensen wrote: > >> > Felix wasn't really convinced, I think. He also pointed out some drivers >> > use skb->priority without checking anything, but I'm not sure we can >> > really squash all the cases of setting skb priority easily? >> >> ~/build/linux/drivers/net/wireless $ git grep 'skb->priority = ' >> ath/ath9k/channel.c: skb->priority = 7; >> broadcom/brcm80211/brcmfmac/core.c: skb->priority = >> cfg80211_classify8021d(skb, NULL); >> broadcom/brcm80211/brcmutil/utils.c: skb->priority = 0; >> intel/ipw2x00/libipw_tx.c: skb->priority = libipw_classify(skb); >> marvell/mwifiex/cfg80211.c: skb->priority = LOW_PRIO_TID; >> marvell/mwifiex/main.c: skb->priority = cfg80211_classify8021d(skb, >> NULL); >> marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_BK; >> marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; >> marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; >> rsi/rsi_91x_core.c: skb->priority = q_num; >> rsi/rsi_91x_core.c: skb->priority = TID_TO_WME_AC(tid); >> rsi/rsi_91x_core.c: skb->priority = BE_Q; >> rsi/rsi_91x_core.c: skb->priority = q_num; >> rsi/rsi_91x_hal.c: skb->priority = VO_Q; >> rsi/rsi_91x_mgmt.c: skb->priority = MGMT_SOFT_Q; >> ti/wlcore/main.c:skb->priority = WL1271_TID_MGMT; >> >> Doesn't seem *that* excessive? Obviously there could be other cases, and >> I haven't looked closer at any of those... > > That's assignments. For assignments, I guess you'd have to look at > net/mac80211/. It's not that excessive either, but it's not in all > places trivial to determine ... Ah, sorry, I read that as "some drivers *set* skb->priority without checking"... -Toke
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
On Wed, 2018-09-05 at 13:07 +0200, Toke Høiland-Jørgensen wrote: > > Felix wasn't really convinced, I think. He also pointed out some drivers > > use skb->priority without checking anything, but I'm not sure we can > > really squash all the cases of setting skb priority easily? > > ~/build/linux/drivers/net/wireless $ git grep 'skb->priority = ' > ath/ath9k/channel.c: skb->priority = 7; > broadcom/brcm80211/brcmfmac/core.c: skb->priority = > cfg80211_classify8021d(skb, NULL); > broadcom/brcm80211/brcmutil/utils.c: skb->priority = 0; > intel/ipw2x00/libipw_tx.c:skb->priority = libipw_classify(skb); > marvell/mwifiex/cfg80211.c: skb->priority = LOW_PRIO_TID; > marvell/mwifiex/main.c: skb->priority = cfg80211_classify8021d(skb, > NULL); > marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_BK; > marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; > marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; > rsi/rsi_91x_core.c: skb->priority = q_num; > rsi/rsi_91x_core.c: skb->priority = TID_TO_WME_AC(tid); > rsi/rsi_91x_core.c: skb->priority = BE_Q; > rsi/rsi_91x_core.c: skb->priority = q_num; > rsi/rsi_91x_hal.c:skb->priority = VO_Q; > rsi/rsi_91x_mgmt.c: skb->priority = MGMT_SOFT_Q; > ti/wlcore/main.c: skb->priority = WL1271_TID_MGMT; > > Doesn't seem *that* excessive? Obviously there could be other cases, and > I haven't looked closer at any of those... That's assignments. For assignments, I guess you'd have to look at net/mac80211/. It's not that excessive either, but it's not in all places trivial to determine ... Whatever, I'll just try, give me a minute :) > Does it matter for the drivers that don't use TXQs? Probably. johannes
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
Johannes Berg writes: > On Wed, 2018-09-05 at 11:56 +0200, Toke Høiland-Jørgensen wrote: > >> > So basically this gets rid of a corner case that we shouldn't have. >> > Either we should decide that using different TXQs is *always* correct >> > for non-QoS, or - what I thought - that this isn't worth it, and then we >> > should *never* do it. >> >> Yeah, I agree that this is not worth it. The queue is already >> FQ-CoDel'ed, which gives us most of the benefit of QoS anyway :) > > So do I read that as a tentative ack? :) Yeah, guess so :) > Felix wasn't really convinced, I think. He also pointed out some drivers > use skb->priority without checking anything, but I'm not sure we can > really squash all the cases of setting skb priority easily? ~/build/linux/drivers/net/wireless $ git grep 'skb->priority = ' ath/ath9k/channel.c:skb->priority = 7; broadcom/brcm80211/brcmfmac/core.c: skb->priority = cfg80211_classify8021d(skb, NULL); broadcom/brcm80211/brcmutil/utils.c:skb->priority = 0; intel/ipw2x00/libipw_tx.c: skb->priority = libipw_classify(skb); marvell/mwifiex/cfg80211.c: skb->priority = LOW_PRIO_TID; marvell/mwifiex/main.c: skb->priority = cfg80211_classify8021d(skb, NULL); marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_BK; marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; rsi/rsi_91x_core.c: skb->priority = q_num; rsi/rsi_91x_core.c: skb->priority = TID_TO_WME_AC(tid); rsi/rsi_91x_core.c: skb->priority = BE_Q; rsi/rsi_91x_core.c: skb->priority = q_num; rsi/rsi_91x_hal.c: skb->priority = VO_Q; rsi/rsi_91x_mgmt.c: skb->priority = MGMT_SOFT_Q; ti/wlcore/main.c: skb->priority = WL1271_TID_MGMT; Doesn't seem *that* excessive? Obviously there could be other cases, and I haven't looked closer at any of those... Does it matter for the drivers that don't use TXQs? -Toke
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
On Wed, 2018-09-05 at 11:56 +0200, Toke Høiland-Jørgensen wrote: > > So basically this gets rid of a corner case that we shouldn't have. > > Either we should decide that using different TXQs is *always* correct > > for non-QoS, or - what I thought - that this isn't worth it, and then we > > should *never* do it. > > Yeah, I agree that this is not worth it. The queue is already > FQ-CoDel'ed, which gives us most of the benefit of QoS anyway :) So do I read that as a tentative ack? :) Felix wasn't really convinced, I think. He also pointed out some drivers use skb->priority without checking anything, but I'm not sure we can really squash all the cases of setting skb priority easily? johannes
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
Johannes Berg writes: > On Wed, 2018-09-05 at 11:47 +0200, Toke Høiland-Jørgensen wrote: >> Johannes Berg writes: >> >> > From: Johannes Berg >> > >> > Some frames may have a non-zero skb->priority assigned by >> > mac80211 internally, e.g. TDLS setup frames, regardless of >> > support for QoS. >> > >> > Currently, we set skb->priority to 0 for all data frames. >> > Note that there's a comment that this is "required for >> > correct WPA/11i MIC", but that doesn't seem true as we use >> > >> > if (ieee80211_is_data_qos(hdr->frame_control)) >> > qos_tid = ieee80211_get_tid(hdr); >> > else >> > qos_tid = 0; >> > >> > in the code there. We could therefore reconsider this, but >> > it seems like unnecessary complexity for the unlikely (and >> > not very useful) case of not having QoS on the connection. >> > >> > This situation then causes something strange - most data >> > frames will go on TXQ for TID 0 for non-QoS connections, >> > but very few exceptions that are internally generated will >> > go on another TXQ, possibly causing confusion. >> >> What kind of confusion are you seeing? Reordering issues, or something >> else? > > I haven't actually been able to test this... > > But with the iwlwifi work we're doing, at the very least we'd waste a > hardware queue for the case that basically never happens, since you'd > end up putting these frames (that are very few) on a separate TXQ and > thus hardware queue. Ah, right, you're doing 1-to-1 TXQ-to-HWQ mapping. Gotcha. > You could argue we should explicitly _not_ do this, but then we should > also set skb->priority to be non-zero for non-QoS stations. Then we > could benefit from some form of QoS (between the TXQs) for non-QoS > connections, but that seems pretty complex and doesn't seem worth it > since all connections that want anything from HT/11n and newer need QoS > anyway. > > So basically this gets rid of a corner case that we shouldn't have. > Either we should decide that using different TXQs is *always* correct > for non-QoS, or - what I thought - that this isn't worth it, and then we > should *never* do it. Yeah, I agree that this is not worth it. The queue is already FQ-CoDel'ed, which gives us most of the benefit of QoS anyway :) -Toke
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
On Wed, 2018-09-05 at 11:47 +0200, Toke Høiland-Jørgensen wrote: > Johannes Berg writes: > > > From: Johannes Berg > > > > Some frames may have a non-zero skb->priority assigned by > > mac80211 internally, e.g. TDLS setup frames, regardless of > > support for QoS. > > > > Currently, we set skb->priority to 0 for all data frames. > > Note that there's a comment that this is "required for > > correct WPA/11i MIC", but that doesn't seem true as we use > > > > if (ieee80211_is_data_qos(hdr->frame_control)) > > qos_tid = ieee80211_get_tid(hdr); > > else > > qos_tid = 0; > > > > in the code there. We could therefore reconsider this, but > > it seems like unnecessary complexity for the unlikely (and > > not very useful) case of not having QoS on the connection. > > > > This situation then causes something strange - most data > > frames will go on TXQ for TID 0 for non-QoS connections, > > but very few exceptions that are internally generated will > > go on another TXQ, possibly causing confusion. > > What kind of confusion are you seeing? Reordering issues, or something > else? I haven't actually been able to test this... But with the iwlwifi work we're doing, at the very least we'd waste a hardware queue for the case that basically never happens, since you'd end up putting these frames (that are very few) on a separate TXQ and thus hardware queue. You could argue we should explicitly _not_ do this, but then we should also set skb->priority to be non-zero for non-QoS stations. Then we could benefit from some form of QoS (between the TXQs) for non-QoS connections, but that seems pretty complex and doesn't seem worth it since all connections that want anything from HT/11n and newer need QoS anyway. So basically this gets rid of a corner case that we shouldn't have. Either we should decide that using different TXQs is *always* correct for non-QoS, or - what I thought - that this isn't worth it, and then we should *never* do it. johannes
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
Johannes Berg writes: > From: Johannes Berg > > Some frames may have a non-zero skb->priority assigned by > mac80211 internally, e.g. TDLS setup frames, regardless of > support for QoS. > > Currently, we set skb->priority to 0 for all data frames. > Note that there's a comment that this is "required for > correct WPA/11i MIC", but that doesn't seem true as we use > > if (ieee80211_is_data_qos(hdr->frame_control)) > qos_tid = ieee80211_get_tid(hdr); > else > qos_tid = 0; > > in the code there. We could therefore reconsider this, but > it seems like unnecessary complexity for the unlikely (and > not very useful) case of not having QoS on the connection. > > This situation then causes something strange - most data > frames will go on TXQ for TID 0 for non-QoS connections, > but very few exceptions that are internally generated will > go on another TXQ, possibly causing confusion. What kind of confusion are you seeing? Reordering issues, or something else? -Toke
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
On Wed, 2018-09-05 at 10:06 +0200, Arend van Spriel wrote:
>
> > +++ b/net/mac80211/tx.c
> > @@ -1260,7 +1260,10 @@ static struct txq_info *ieee80211_get_txq(struct
> > ieee80211_local *local,
> > txq = sta->sta.txq[IEEE80211_NUM_TIDS];
> > }
> > } else if (sta) {
> > - u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK;
> > + u8 tid = 0;
> > +
> > + if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA))
> > + tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
>
> Is the use of different mask intentional here? Just a quick glance so
> did not look into it further.
Ah, I forgot to mention that in the commit log. That just aligns it with
most of the other code, but since we never have values other than 0-7 it
doesn't actually matter.
johannes
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
On 9/5/2018 10:00 AM, Johannes Berg wrote:
From: Johannes Berg
Some frames may have a non-zero skb->priority assigned by
mac80211 internally, e.g. TDLS setup frames, regardless of
support for QoS.
Currently, we set skb->priority to 0 for all data frames.
Note that there's a comment that this is "required for
correct WPA/11i MIC", but that doesn't seem true as we use
if (ieee80211_is_data_qos(hdr->frame_control))
qos_tid = ieee80211_get_tid(hdr);
else
qos_tid = 0;
in the code there. We could therefore reconsider this, but
it seems like unnecessary complexity for the unlikely (and
not very useful) case of not having QoS on the connection.
This situation then causes something strange - most data
frames will go on TXQ for TID 0 for non-QoS connections,
but very few exceptions that are internally generated will
go on another TXQ, possibly causing confusion.
Avoid this confusion by putting non-QoS data frames into
TID 0 regardless of the skb->priority.
Signed-off-by: Johannes Berg
---
net/mac80211/tx.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 5b6e06ad35ff..6540595addd1 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1260,7 +1260,10 @@ static struct txq_info *ieee80211_get_txq(struct
ieee80211_local *local,
txq = sta->sta.txq[IEEE80211_NUM_TIDS];
}
} else if (sta) {
- u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK;
+ u8 tid = 0;
+
+ if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA))
+ tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
Is the use of different mask intentional here? Just a quick glance so
did not look into it further.
Gr. AvS
