[PATCH] mac80211: use non-zero TID only for QoS frames

[PATCH] mac80211: use non-zero TID only for QoS frames

[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

Re: [PATCH] mac80211: use non-zero TID only for QoS frames

[Arend van Spriel]

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

Re: [PATCH] mac80211: use non-zero TID only for QoS frames

[Johannes Berg]

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

[Toke Høiland-Jørgensen]

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

Re: [PATCH] mac80211: use non-zero TID only for QoS frames

[Johannes Berg]

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

Re: [PATCH] mac80211: use non-zero TID only for QoS frames

[Toke Høiland-Jørgensen]

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

Re: [PATCH] mac80211: use non-zero TID only for QoS frames

[Johannes Berg]

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

[Toke Høiland-Jørgensen]

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

Re: [PATCH] mac80211: use non-zero TID only for QoS frames

[Johannes Berg]

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

[Toke Høiland-Jørgensen]

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