[PATCH] mac80211: TDLS: fix skb queue/priority assignment

[PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

If the TDLS setup happens over a connection to an AP that
doesn't have QoS, we nevertheless assign a non-zero TID
(skb->priority) and queue mapping, which may confuse us or
drivers later.

Fix it by just assigning the special skb->priority and then
using ieee80211_select_queue() just like other data frames
would go through.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
OK, this addresses the case I was worried about - perhaps
better take this than the txq patch?
---
 net/mac80211/tdls.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/tdls.c b/net/mac80211/tdls.c
index 5cd5e6e5834e..6c647f425e05 100644
--- a/net/mac80211/tdls.c
+++ b/net/mac80211/tdls.c
@@ -16,6 +16,7 @@
 #include "ieee80211_i.h"
 #include "driver-ops.h"
 #include "rate.h"
+#include "wme.h"
 
 /* give usermode some time for retries in setting up the TDLS session */
 #define TDLS_PEER_SETUP_TIMEOUT	(15 * HZ)
@@ -1010,14 +1011,13 @@ ieee80211_tdls_prep_mgmt_packet(struct wiphy *wiphy, struct net_device *dev,
 	switch (action_code) {
 	case WLAN_TDLS_SETUP_REQUEST:
 	case WLAN_TDLS_SETUP_RESPONSE:
-		skb_set_queue_mapping(skb, IEEE80211_AC_BK);
-		skb->priority = 2;
+		skb->priority = 256 + 2;
 		break;
 	default:
-		skb_set_queue_mapping(skb, IEEE80211_AC_VI);
-		skb->priority = 5;
+		skb->priority = 256 + 5;
 		break;
 	}
+	skb_set_queue_mapping(skb, ieee80211_select_queue(sdata, skb));
 
 	/*
 	 * Set the WLAN_TDLS_TEARDOWN flag to indicate a teardown in progress.
-- 
2.14.4

Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Johannes Berg]

On Wed, 2018-09-05 at 13:34 +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> If the TDLS setup happens over a connection to an AP that
> doesn't have QoS, we nevertheless assign a non-zero TID
> (skb->priority) and queue mapping, which may confuse us or
> drivers later.
> 
> Fix it by just assigning the special skb->priority and then
> using ieee80211_select_queue() just like other data frames
> would go through.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
> OK, this addresses the case I was worried about - perhaps
> better take this than the txq patch?

I really only found this. All the other cases aren't doing real data
frames, only null-data and management. So I guess for those we're OK,
and this was the only special case after all.

johannes

Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2018-09-05 at 13:34 +0200, Johannes Berg wrote:
>> From: Johannes Berg <johannes.berg@intel.com>
>> 
>> If the TDLS setup happens over a connection to an AP that
>> doesn't have QoS, we nevertheless assign a non-zero TID
>> (skb->priority) and queue mapping, which may confuse us or
>> drivers later.
>> 
>> Fix it by just assigning the special skb->priority and then
>> using ieee80211_select_queue() just like other data frames
>> would go through.
>> 
>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>> ---
>> OK, this addresses the case I was worried about - perhaps
>> better take this than the txq patch?

(This is just you talking to yourself, right? :))

> I really only found this. All the other cases aren't doing real data
> frames, only null-data and management. So I guess for those we're OK,
> and this was the only special case after all.

SGTM.

Guess we'll have to deal with everything else if we ever move management
frames onto the TXQ path as well...

-Toke

Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Johannes Berg]

On Wed, 2018-09-05 at 13:40 +0200, Toke Høiland-Jørgensen wrote:
> 
> Guess we'll have to deal with everything else if we ever move management
> frames onto the TXQ path as well...

Depends on whether we care for management frame priorities or not ... so
far we haven't really.

johannes

Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Johannes Berg]

On Wed, 2018-09-05 at 13:41 +0200, Johannes Berg wrote:
> On Wed, 2018-09-05 at 13:40 +0200, Toke Høiland-Jørgensen wrote:
> > 
> > Guess we'll have to deal with everything else if we ever move management
> > frames onto the TXQ path as well...
> 
> Depends on whether we care for management frame priorities or not ... so
> far we haven't really.

Actually, for the most part we have implemented that properly. Except
for the TXQ I added for bufferable management ... oh well, I think we're
the only user thereof now.

I'm not sure we want to have a TXQ per TID for management, that seems
overkill. But I'm also not sure how to solve this otherwise ...

johannes

Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2018-09-05 at 13:41 +0200, Johannes Berg wrote:
>> On Wed, 2018-09-05 at 13:40 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrot=
e:
>> >=20
>> > Guess we'll have to deal with everything else if we ever move manageme=
nt
>> > frames onto the TXQ path as well...
>>=20
>> Depends on whether we care for management frame priorities or not ... so
>> far we haven't really.
>
> Actually, for the most part we have implemented that properly. Except
> for the TXQ I added for bufferable management ... oh well, I think we're
> the only user thereof now.
>
> I'm not sure we want to have a TXQ per TID for management, that seems
> overkill. But I'm also not sure how to solve this otherwise ...

Graft it to an existing TXQ, similar to how the fragments queue is used
now? Saves a TXQ at the expense of having to special-case it...

-Toke

Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Johannes Berg]

On Wed, 2018-09-05 at 14:32 +0200, Toke Høiland-Jørgensen wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > On Wed, 2018-09-05 at 13:41 +0200, Johannes Berg wrote:
> > > On Wed, 2018-09-05 at 13:40 +0200, Toke Høiland-Jørgensen wrote:
> > > > 
> > > > Guess we'll have to deal with everything else if we ever move management
> > > > frames onto the TXQ path as well...
> > > 
> > > Depends on whether we care for management frame priorities or not ... so
> > > far we haven't really.
> > 
> > Actually, for the most part we have implemented that properly. Except
> > for the TXQ I added for bufferable management ... oh well, I think we're
> > the only user thereof now.
> > 
> > I'm not sure we want to have a TXQ per TID for management, that seems
> > overkill. But I'm also not sure how to solve this otherwise ...
> 
> Graft it to an existing TXQ, similar to how the fragments queue is used
> now? Saves a TXQ at the expense of having to special-case it...

The problem isn't so much how we handle it in mac80211 for the queueing,
but how we deal with things like A-MSDU and how we present it to the
driver ... for iwlwifi at least we'd really like to have only data
frames so we can map it directly to the hardware queue ...

johannes

Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2018-09-05 at 14:32 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Johannes Berg <johannes@sipsolutions.net> writes:
>>=20
>> > On Wed, 2018-09-05 at 13:41 +0200, Johannes Berg wrote:
>> > > On Wed, 2018-09-05 at 13:40 +0200, Toke H=C3=B8iland-J=C3=B8rgensen =
wrote:
>> > > >=20
>> > > > Guess we'll have to deal with everything else if we ever move mana=
gement
>> > > > frames onto the TXQ path as well...
>> > >=20
>> > > Depends on whether we care for management frame priorities or not ..=
. so
>> > > far we haven't really.
>> >=20
>> > Actually, for the most part we have implemented that properly. Except
>> > for the TXQ I added for bufferable management ... oh well, I think we'=
re
>> > the only user thereof now.
>> >=20
>> > I'm not sure we want to have a TXQ per TID for management, that seems
>> > overkill. But I'm also not sure how to solve this otherwise ...
>>=20
>> Graft it to an existing TXQ, similar to how the fragments queue is used
>> now? Saves a TXQ at the expense of having to special-case it...
>
> The problem isn't so much how we handle it in mac80211 for the queueing,
> but how we deal with things like A-MSDU and how we present it to the
> driver ... for iwlwifi at least we'd really like to have only data
> frames so we can map it directly to the hardware queue ...

Ah, I see. No, then just putting them at the head of a different TXQ
probably won't work...

Are you mapping TXQs to hardware queues dynamically as they empty and
re-fill? Presumably you'll have cases where you don't have enough HWQs?

-Toke

Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Johannes Berg]

On Wed, 2018-09-05 at 14:50 +0200, Toke Høiland-Jørgensen wrote:

(uh, sorry, bit late ...)

> > The problem isn't so much how we handle it in mac80211 for the queueing,
> > but how we deal with things like A-MSDU and how we present it to the
> > driver ... for iwlwifi at least we'd really like to have only data
> > frames so we can map it directly to the hardware queue ...
> 
> Ah, I see. No, then just putting them at the head of a different TXQ
> probably won't work...
> 
> Are you mapping TXQs to hardware queues dynamically as they empty and
> re-fill? Presumably you'll have cases where you don't have enough HWQs?

Depends on the hardware. Newer hardware has basically unlimited HWQs
(something on the order of 512 IIRC).

Older hardware sort of maps them dynamically, but not too dynamic, we
also have 32 (ish) queues there. We only free them if we need a new one
and don't have one, and yes, theoretically we can run out and then we
may have to share a single hardware queue for multiple TXQs, but it
basically never happens in practice.

johannes

Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2018-09-05 at 14:50 +0200, Toke Høiland-Jørgensen wrote:
>
> (uh, sorry, bit late ...)
>
>> > The problem isn't so much how we handle it in mac80211 for the queueing,
>> > but how we deal with things like A-MSDU and how we present it to the
>> > driver ... for iwlwifi at least we'd really like to have only data
>> > frames so we can map it directly to the hardware queue ...
>> 
>> Ah, I see. No, then just putting them at the head of a different TXQ
>> probably won't work...
>> 
>> Are you mapping TXQs to hardware queues dynamically as they empty and
>> re-fill? Presumably you'll have cases where you don't have enough HWQs?
>
> Depends on the hardware. Newer hardware has basically unlimited HWQs
> (something on the order of 512 IIRC).
>
> Older hardware sort of maps them dynamically, but not too dynamic, we
> also have 32 (ish) queues there. We only free them if we need a new one
> and don't have one, and yes, theoretically we can run out and then we
> may have to share a single hardware queue for multiple TXQs, but it
> basically never happens in practice.

Right, gotcha, thanks for the explanation :)

-Toke