* [PATCH] mac80211: use non-zero TID only for QoS frames
@ 2018-09-05 8:00 Johannes Berg
2018-09-05 8:06 ` Arend van Spriel
2018-09-05 9:47 ` Toke Høiland-Jørgensen
0 siblings, 2 replies; 10+ messages in thread
From: Johannes Berg @ 2018-09-05 8:00 UTC (permalink / raw)
To: linux-wireless
Cc: Felix Fietkau, Toke Høiland-Jørgensen, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
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 <johannes.berg@intel.com>
---
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;
if (!sta->uploaded)
return NULL;
--
2.14.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] mac80211: use non-zero TID only for QoS frames
2018-09-05 8:00 [PATCH] mac80211: use non-zero TID only for QoS frames Johannes Berg
@ 2018-09-05 8:06 ` Arend van Spriel
2018-09-05 8:09 ` Johannes Berg
2018-09-05 9:47 ` Toke Høiland-Jørgensen
1 sibling, 1 reply; 10+ messages in thread
From: Arend van Spriel @ 2018-09-05 8:06 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
Cc: Felix Fietkau, Toke Høiland-Jørgensen, Johannes Berg
On 9/5/2018 10:00 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> 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 <johannes.berg@intel.com>
> ---
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mac80211: use non-zero TID only for QoS frames
2018-09-05 8:06 ` Arend van Spriel
@ 2018-09-05 8:09 ` Johannes Berg
0 siblings, 0 replies; 10+ messages in thread
From: Johannes Berg @ 2018-09-05 8:09 UTC (permalink / raw)
To: Arend van Spriel, linux-wireless
Cc: Felix Fietkau, Toke Høiland-Jørgensen
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mac80211: use non-zero TID only for QoS frames
2018-09-05 8:00 [PATCH] mac80211: use non-zero TID only for QoS frames Johannes Berg
2018-09-05 8:06 ` Arend van Spriel
@ 2018-09-05 9:47 ` Toke Høiland-Jørgensen
2018-09-05 9:50 ` Johannes Berg
1 sibling, 1 reply; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2018-09-05 9:47 UTC (permalink / raw)
To: Johannes Berg, linux-wireless; +Cc: Felix Fietkau, Johannes Berg
Johannes Berg <johannes@sipsolutions.net> writes:
> From: Johannes Berg <johannes.berg@intel.com>
>
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mac80211: use non-zero TID only for QoS frames
2018-09-05 9:47 ` Toke Høiland-Jørgensen
@ 2018-09-05 9:50 ` Johannes Berg
2018-09-05 9:56 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 10+ messages in thread
From: Johannes Berg @ 2018-09-05 9:50 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, linux-wireless; +Cc: Felix Fietkau
On Wed, 2018-09-05 at 11:47 +0200, Toke Høiland-Jørgensen wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
>
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mac80211: use non-zero TID only for QoS frames
2018-09-05 9:50 ` Johannes Berg
@ 2018-09-05 9:56 ` Toke Høiland-Jørgensen
2018-09-05 10:56 ` Johannes Berg
0 siblings, 1 reply; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2018-09-05 9:56 UTC (permalink / raw)
To: Johannes Berg, linux-wireless; +Cc: Felix Fietkau
Johannes Berg <johannes@sipsolutions.net> writes:
> On Wed, 2018-09-05 at 11:47 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Johannes Berg <johannes@sipsolutions.net> writes:
>>=20
>> > From: Johannes Berg <johannes.berg@intel.com>
>> >=20
>> > Some frames may have a non-zero skb->priority assigned by
>> > mac80211 internally, e.g. TDLS setup frames, regardless of
>> > support for QoS.
>> >=20
>> > 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
>> >=20
>> > if (ieee80211_is_data_qos(hdr->frame_control))
>> > qos_tid =3D ieee80211_get_tid(hdr);
>> > else
>> > qos_tid =3D 0;
>> >=20
>> > 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.
>> >=20
>> > 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.
>>=20
>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mac80211: use non-zero TID only for QoS frames
2018-09-05 9:56 ` Toke Høiland-Jørgensen
@ 2018-09-05 10:56 ` Johannes Berg
2018-09-05 11:07 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 10+ messages in thread
From: Johannes Berg @ 2018-09-05 10:56 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, linux-wireless; +Cc: Felix Fietkau
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mac80211: use non-zero TID only for QoS frames
2018-09-05 10:56 ` Johannes Berg
@ 2018-09-05 11:07 ` Toke Høiland-Jørgensen
2018-09-05 11:08 ` Johannes Berg
0 siblings, 1 reply; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2018-09-05 11:07 UTC (permalink / raw)
To: Johannes Berg, linux-wireless; +Cc: Felix Fietkau
Johannes Berg <johannes@sipsolutions.net> writes:
> On Wed, 2018-09-05 at 11:56 +0200, Toke H=C3=B8iland-J=C3=B8rgensen 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.
>>=20
>> 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 =3D '
ath/ath9k/channel.c: skb->priority =3D 7;
broadcom/brcm80211/brcmfmac/core.c: skb->priority =3D cfg80211_classify802=
1d(skb, NULL);
broadcom/brcm80211/brcmutil/utils.c: skb->priority =3D 0;
intel/ipw2x00/libipw_tx.c: skb->priority =3D libipw_classify(skb);
marvell/mwifiex/cfg80211.c: skb->priority =3D LOW_PRIO_TID;
marvell/mwifiex/main.c: skb->priority =3D cfg80211_classify8021d(skb, NULL);
marvell/mwifiex/tdls.c: skb->priority =3D MWIFIEX_PRIO_BK;
marvell/mwifiex/tdls.c: skb->priority =3D MWIFIEX_PRIO_VI;
marvell/mwifiex/tdls.c: skb->priority =3D MWIFIEX_PRIO_VI;
rsi/rsi_91x_core.c: skb->priority =3D q_num;
rsi/rsi_91x_core.c: skb->priority =3D TID_TO_WME_AC(tid);
rsi/rsi_91x_core.c: skb->priority =3D BE_Q;
rsi/rsi_91x_core.c: skb->priority =3D q_num;
rsi/rsi_91x_hal.c: skb->priority =3D VO_Q;
rsi/rsi_91x_mgmt.c: skb->priority =3D MGMT_SOFT_Q;
ti/wlcore/main.c: skb->priority =3D 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mac80211: use non-zero TID only for QoS frames
2018-09-05 11:07 ` Toke Høiland-Jørgensen
@ 2018-09-05 11:08 ` Johannes Berg
2018-09-05 11:12 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 10+ messages in thread
From: Johannes Berg @ 2018-09-05 11:08 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, linux-wireless; +Cc: Felix Fietkau
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mac80211: use non-zero TID only for QoS frames
2018-09-05 11:08 ` Johannes Berg
@ 2018-09-05 11:12 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2018-09-05 11:12 UTC (permalink / raw)
To: Johannes Berg, linux-wireless; +Cc: Felix Fietkau
Johannes Berg <johannes@sipsolutions.net> writes:
> On Wed, 2018-09-05 at 13:07 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>
>> > Felix wasn't really convinced, I think. He also pointed out some drive=
rs
>> > use skb->priority without checking anything, but I'm not sure we can
>> > really squash all the cases of setting skb priority easily?
>>=20
>> ~/build/linux/drivers/net/wireless $ git grep 'skb->priority =3D '
>> ath/ath9k/channel.c: skb->priority =3D 7;
>> broadcom/brcm80211/brcmfmac/core.c: skb->priority =3D cfg80211_classify=
8021d(skb, NULL);
>> broadcom/brcm80211/brcmutil/utils.c: skb->priority =3D 0;
>> intel/ipw2x00/libipw_tx.c: skb->priority =3D libipw_classify(skb);
>> marvell/mwifiex/cfg80211.c: skb->priority =3D LOW_PRIO_TID;
>> marvell/mwifiex/main.c: skb->priority =3D cfg80211_classify8021d(skb, NU=
LL);
>> marvell/mwifiex/tdls.c: skb->priority =3D MWIFIEX_PRIO_BK;
>> marvell/mwifiex/tdls.c: skb->priority =3D MWIFIEX_PRIO_VI;
>> marvell/mwifiex/tdls.c: skb->priority =3D MWIFIEX_PRIO_VI;
>> rsi/rsi_91x_core.c: skb->priority =3D q_num;
>> rsi/rsi_91x_core.c: skb->priority =3D TID_TO_WME_AC(tid);
>> rsi/rsi_91x_core.c: skb->priority =3D BE_Q;
>> rsi/rsi_91x_core.c: skb->priority =3D q_num;
>> rsi/rsi_91x_hal.c: skb->priority =3D VO_Q;
>> rsi/rsi_91x_mgmt.c: skb->priority =3D MGMT_SOFT_Q;
>> ti/wlcore/main.c: skb->priority =3D WL1271_TID_MGMT;
>>=20
>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-09-05 15:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-05 8:00 [PATCH] mac80211: use non-zero TID only for QoS frames Johannes Berg
2018-09-05 8:06 ` Arend van Spriel
2018-09-05 8:09 ` Johannes Berg
2018-09-05 9:47 ` Toke Høiland-Jørgensen
2018-09-05 9:50 ` Johannes Berg
2018-09-05 9:56 ` Toke Høiland-Jørgensen
2018-09-05 10:56 ` Johannes Berg
2018-09-05 11:07 ` Toke Høiland-Jørgensen
2018-09-05 11:08 ` Johannes Berg
2018-09-05 11:12 ` Toke Høiland-Jørgensen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).