* [PATCH] mac80211: don't put null-data frames on the normal TXQ @ 2018-07-03 12:47 Johannes Berg 2018-07-03 14:31 ` Toke Høiland-Jørgensen 2018-07-03 20:40 ` Peter Oh 0 siblings, 2 replies; 11+ messages in thread From: Johannes Berg @ 2018-07-03 12:47 UTC (permalink / raw) To: linux-wireless Cc: Toke Høiland-Jørgensen, Felix Fietkau, Johannes Berg From: Johannes Berg <johannes.berg@intel.com> Since (QoS) NDP frames shouldn't be put into aggregation nor are assigned real sequence numbers, etc. it's better to treat them as non-data packets and not put them on the normal TXQs, for example when building A-MPDUs they need to be treated specially, and they are more used for management (e.g. to see if the station is alive) anyway. Signed-off-by: Johannes Berg <johannes.berg@intel.com> --- net/mac80211/tx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index fa1f1e63a264..5a60832052dc 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -1247,7 +1247,7 @@ static struct txq_info *ieee80211_get_txq(struct ieee80211_local *local, (info->control.flags & IEEE80211_TX_CTRL_PS_RESPONSE)) return NULL; - if (!ieee80211_is_data(hdr->frame_control)) + if (!ieee80211_is_data_present(hdr->frame_control)) return NULL; if (sta) { -- 2.14.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ 2018-07-03 12:47 [PATCH] mac80211: don't put null-data frames on the normal TXQ Johannes Berg @ 2018-07-03 14:31 ` Toke Høiland-Jørgensen 2018-07-03 14:32 ` Johannes Berg 2018-07-03 20:40 ` Peter Oh 1 sibling, 1 reply; 11+ messages in thread From: Toke Høiland-Jørgensen @ 2018-07-03 14:31 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> > > Since (QoS) NDP frames shouldn't be put into aggregation nor are > assigned real sequence numbers, etc. it's better to treat them as > non-data packets and not put them on the normal TXQs, for example > when building A-MPDUs they need to be treated specially, and they > are more used for management (e.g. to see if the station is alive) > anyway. No objections to this per se; but didn't we want to move towards everything going through the TXQs? Any progress on that front? :) -Toke ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ 2018-07-03 14:31 ` Toke Høiland-Jørgensen @ 2018-07-03 14:32 ` Johannes Berg 2018-07-03 14:36 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 11+ messages in thread From: Johannes Berg @ 2018-07-03 14:32 UTC (permalink / raw) To: Toke Høiland-Jørgensen, linux-wireless; +Cc: Felix Fietkau On Tue, 2018-07-03 at 16:31 +0200, Toke Høiland-Jørgensen wrote: > Johannes Berg <johannes@sipsolutions.net> writes: > > > From: Johannes Berg <johannes.berg@intel.com> > > > > Since (QoS) NDP frames shouldn't be put into aggregation nor are > > assigned real sequence numbers, etc. it's better to treat them as > > non-data packets and not put them on the normal TXQs, for example > > when building A-MPDUs they need to be treated specially, and they > > are more used for management (e.g. to see if the station is alive) > > anyway. > > No objections to this per se; :-) > but didn't we want to move towards > everything going through the TXQs? Any progress on that front? :) Not really. Yes, I wanted to, but it's some massive surgery. Right now I'm working on converting iwlwifi, perhaps I'll learn about it more and can then do the mac80211 surgery better later. johannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ 2018-07-03 14:32 ` Johannes Berg @ 2018-07-03 14:36 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 11+ messages in thread From: Toke Høiland-Jørgensen @ 2018-07-03 14:36 UTC (permalink / raw) To: Johannes Berg, linux-wireless; +Cc: Felix Fietkau Johannes Berg <johannes@sipsolutions.net> writes: > On Tue, 2018-07-03 at 16:31 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> Johannes Berg <johannes@sipsolutions.net> writes: >>=20 >> > From: Johannes Berg <johannes.berg@intel.com> >> >=20 >> > Since (QoS) NDP frames shouldn't be put into aggregation nor are >> > assigned real sequence numbers, etc. it's better to treat them as >> > non-data packets and not put them on the normal TXQs, for example >> > when building A-MPDUs they need to be treated specially, and they >> > are more used for management (e.g. to see if the station is alive) >> > anyway. >>=20 >> No objections to this per se; > > :-) > >> but didn't we want to move towards >> everything going through the TXQs? Any progress on that front? :) > > Not really. Yes, I wanted to, but it's some massive surgery. Right now > I'm working on converting iwlwifi, perhaps I'll learn about it more > and can then do the mac80211 surgery better later. Right, sounds good! Looking forward to the iwlwifi conversion :) -Toke ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ 2018-07-03 12:47 [PATCH] mac80211: don't put null-data frames on the normal TXQ Johannes Berg 2018-07-03 14:31 ` Toke Høiland-Jørgensen @ 2018-07-03 20:40 ` Peter Oh 2018-07-03 23:48 ` Johannes Berg 1 sibling, 1 reply; 11+ messages in thread From: Peter Oh @ 2018-07-03 20:40 UTC (permalink / raw) To: Johannes Berg, linux-wireless Cc: Toke Høiland-Jørgensen, Felix Fietkau, Johannes Berg On 07/03/2018 05:47 AM, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > Since (QoS) NDP frames shouldn't be put into aggregation nor are > assigned real sequence numbers, etc. it's better to treat them as > non-data packets and not put them on the normal TXQs, for example > when building A-MPDUs they need to be treated specially, To be treated specially at which layer, mac80211 or drivers? Are you seeing any issues other than complexity of handling NDP or is it just improvement? Thanks, Peter ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ 2018-07-03 20:40 ` Peter Oh @ 2018-07-03 23:48 ` Johannes Berg 2018-07-04 4:24 ` Ben Greear 2018-07-04 7:26 ` Toke Høiland-Jørgensen 0 siblings, 2 replies; 11+ messages in thread From: Johannes Berg @ 2018-07-03 23:48 UTC (permalink / raw) To: Peter Oh, linux-wireless; +Cc: Toke Høiland-Jørgensen, Felix Fietkau On Tue, 2018-07-03 at 13:40 -0700, Peter Oh wrote: > > On 07/03/2018 05:47 AM, Johannes Berg wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > > > Since (QoS) NDP frames shouldn't be put into aggregation nor are > > assigned real sequence numbers, etc. it's better to treat them as > > non-data packets and not put them on the normal TXQs, for example > > when building A-MPDUs they need to be treated specially, > > To be treated specially at which layer, mac80211 or drivers? They cannot be put into an A-MPDU, so you need to skip them when building A-MPDUs. > Are you seeing any issues other than complexity of handling NDP or is it > just improvement? I'm not actually running any hardware on my development setup that would use TXQs today. However, I'm starting to work on converting iwlwifi to it, and if we put the NDPs on there it means we no longer can schedule the TXQ to a single hardware queue. Similarly, I think for other drivers it would be a complexity reduction and possibly performance improvement with aggregation because you no longer need to check if the next frame is an NDP and if yes, finish the open A-MPDU and put both frames on the HW queue. That said, I hadn't looked much at the drivers. Seems the situation is worse than I thought, with those not doing it so well. ath10k appears to not do aggregation in the host, and I guess the data isn't split over multiple queues so the firmware has to determine/buffer it some other way. No idea how that would work. I see no evidence of ath9k doing this correctly, so this might fix a bug there, but I may have missed it. mt76 also appears to behave erroneously: if the txq is marked with aggregation it will even update the mtxq->agg_ssn to 0x10 for QoS NDPs, because those always have seqno 0; in mt76_check_agg_ssn: mtxq->agg_ssn = le16_to_cpu(hdr->seq_ctrl) + 0x10; johannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ 2018-07-03 23:48 ` Johannes Berg @ 2018-07-04 4:24 ` Ben Greear 2018-07-04 7:12 ` Johannes Berg 2018-07-04 7:26 ` Toke Høiland-Jørgensen 1 sibling, 1 reply; 11+ messages in thread From: Ben Greear @ 2018-07-04 4:24 UTC (permalink / raw) To: Johannes Berg, Peter Oh, linux-wireless Cc: Toke Høiland-Jørgensen, Felix Fietkau On 07/03/2018 04:48 PM, Johannes Berg wrote: > On Tue, 2018-07-03 at 13:40 -0700, Peter Oh wrote: >> >> On 07/03/2018 05:47 AM, Johannes Berg wrote: >>> From: Johannes Berg <johannes.berg@intel.com> >>> >>> Since (QoS) NDP frames shouldn't be put into aggregation nor are >>> assigned real sequence numbers, etc. it's better to treat them as >>> non-data packets and not put them on the normal TXQs, for example >>> when building A-MPDUs they need to be treated specially, >> >> To be treated specially at which layer, mac80211 or drivers? > > They cannot be put into an A-MPDU, so you need to skip them when > building A-MPDUs. > >> Are you seeing any issues other than complexity of handling NDP or is it >> just improvement? > > I'm not actually running any hardware on my development setup that would > use TXQs today. However, I'm starting to work on converting iwlwifi to > it, and if we put the NDPs on there it means we no longer can schedule > the TXQ to a single hardware queue. > > Similarly, I think for other drivers it would be a complexity reduction > and possibly performance improvement with aggregation because you no > longer need to check if the next frame is an NDP and if yes, finish the > open A-MPDU and put both frames on the HW queue. > > That said, I hadn't looked much at the drivers. Seems the situation is > worse than I thought, with those not doing it so well. > > ath10k appears to not do aggregation in the host, and I guess the data > isn't split over multiple queues so the firmware has to determine/buffer > it some other way. No idea how that would work. This is my current understanding of ath10k..hope it helps. ath10k firmware does handle the aggregation. It doesn't pay much attention to the driver's txqueues. For wave-2 firmware, the firmware will attempt to fetch frames for peers in a fair/optimal way, and that should indirectly take the txqueues into account. Wave-1 does not do any of the prefetch logic as far as I know. Stock firmware sends mgt frames through an entirely different tx path, while my ath10k-ct firmware can send all frames through the same 'htt' transmit path. Either way, the firmware has final control over what goes to what tid and what is aggregated. That said, there are memory use-after-free and other bugs related to txq in ath10k. Hopefully it will be fixed by the stop-txqueue patch that has been in recent discussion. Thanks, Ben > I see no evidence of ath9k doing this correctly, so this might fix a bug > there, but I may have missed it. > > mt76 also appears to behave erroneously: if the txq is marked with > aggregation it will even update the mtxq->agg_ssn to 0x10 for QoS NDPs, > because those always have seqno 0; in mt76_check_agg_ssn: > mtxq->agg_ssn = le16_to_cpu(hdr->seq_ctrl) + 0x10; > > johannes > -- Ben Greear <greearb@candelatech.com> Candela Technologies Inc http://www.candelatech.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ 2018-07-04 4:24 ` Ben Greear @ 2018-07-04 7:12 ` Johannes Berg 0 siblings, 0 replies; 11+ messages in thread From: Johannes Berg @ 2018-07-04 7:12 UTC (permalink / raw) To: Ben Greear, Peter Oh, linux-wireless Cc: Toke Høiland-Jørgensen, Felix Fietkau On Tue, 2018-07-03 at 21:24 -0700, Ben Greear wrote: > > ath10k appears to not do aggregation in the host, and I guess the data > > isn't split over multiple queues so the firmware has to determine/buffer > > it some other way. No idea how that would work. > > This is my current understanding of ath10k..hope it helps. [snip] Thanks Ben. I guess we can't really know whether or not it would be buggy with (QoS-)NDP, but it shouldn't be since all the frames are expected (by firmware) to go through the same path anyhow and it's responsible for putting them together into aggregates. And, unlike Intel firmware/hardware, it can't necessarily assume that one HW queue consists only of packets allowed for aggregation. johannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ 2018-07-03 23:48 ` Johannes Berg 2018-07-04 4:24 ` Ben Greear @ 2018-07-04 7:26 ` Toke Høiland-Jørgensen 2018-07-04 7:54 ` Johannes Berg 1 sibling, 1 reply; 11+ messages in thread From: Toke Høiland-Jørgensen @ 2018-07-04 7:26 UTC (permalink / raw) To: Johannes Berg, Peter Oh, linux-wireless; +Cc: Felix Fietkau Johannes Berg <johannes@sipsolutions.net> writes: > I see no evidence of ath9k doing this correctly, so this might fix a > bug there, but I may have missed it. ath9k does check for this, in ath_tx_sched_aggr() and in ath_tx_form_aggr(); it'll check for the IEEE80211_TX_CTL_AMPDU flag, and stop building the current aggregate if the flag is not set. -Toke ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ 2018-07-04 7:26 ` Toke Høiland-Jørgensen @ 2018-07-04 7:54 ` Johannes Berg 2018-07-04 8:25 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 11+ messages in thread From: Johannes Berg @ 2018-07-04 7:54 UTC (permalink / raw) To: Toke Høiland-Jørgensen, Peter Oh, linux-wireless; +Cc: Felix Fietkau On Wed, 2018-07-04 at 09:26 +0200, Toke Høiland-Jørgensen wrote: > Johannes Berg <johannes@sipsolutions.net> writes: > > > I see no evidence of ath9k doing this correctly, so this might fix a > > bug there, but I may have missed it. > > ath9k does check for this, in ath_tx_sched_aggr() and in > ath_tx_form_aggr(); it'll check for the IEEE80211_TX_CTL_AMPDU flag, and > stop building the current aggregate if the flag is not set. Ok, thanks. Nevertheless, I guess it's more efficient to not stop the aggregate on encountering a (QoS-)NDP :-) johannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mac80211: don't put null-data frames on the normal TXQ 2018-07-04 7:54 ` Johannes Berg @ 2018-07-04 8:25 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 11+ messages in thread From: Toke Høiland-Jørgensen @ 2018-07-04 8:25 UTC (permalink / raw) To: Johannes Berg, Peter Oh, linux-wireless; +Cc: Felix Fietkau Johannes Berg <johannes@sipsolutions.net> writes: > On Wed, 2018-07-04 at 09:26 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> Johannes Berg <johannes@sipsolutions.net> writes: >>=20 >> > I see no evidence of ath9k doing this correctly, so this might fix a >> > bug there, but I may have missed it. >>=20 >> ath9k does check for this, in ath_tx_sched_aggr() and in >> ath_tx_form_aggr(); it'll check for the IEEE80211_TX_CTL_AMPDU flag, and >> stop building the current aggregate if the flag is not set. > > Ok, thanks. Nevertheless, I guess it's more efficient to not stop the > aggregate on encountering a (QoS-)NDP :-) Oh, absolutely! :) -Toke ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-07-04 8:25 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-03 12:47 [PATCH] mac80211: don't put null-data frames on the normal TXQ Johannes Berg 2018-07-03 14:31 ` Toke Høiland-Jørgensen 2018-07-03 14:32 ` Johannes Berg 2018-07-03 14:36 ` Toke Høiland-Jørgensen 2018-07-03 20:40 ` Peter Oh 2018-07-03 23:48 ` Johannes Berg 2018-07-04 4:24 ` Ben Greear 2018-07-04 7:12 ` Johannes Berg 2018-07-04 7:26 ` Toke Høiland-Jørgensen 2018-07-04 7:54 ` Johannes Berg 2018-07-04 8:25 ` Toke Høiland-Jørgensen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.