Re: ath10k: Missing airtime scheduler parts
Sven Eckelmann writes: > Hi, > > I was asked what parts are currently missing for (a better) airtime fairness > with ath10k. I know that the AQL were merged and enabled for ath10k (and > mt76). But there was also the virtual time-based airtime scheduler [1] which > was proposed. I think the development on that one didn't continue since last > year. I did recently rebase that patch to the current mac80211-next, actually. Sent it off to Felix for some testing, but if you wan to give it a go I can post it to the list as well :) > Maybe someone else knows if there were some other parts worked on > which I've am missed and which were not yet merged. I don't think there have been any other patches being actively worked on, but I seem to recall something about AQL (partly?) breaking airtime fairness because the scheduler only keeps fairness between stations that are actually scheduled, and AQL interrupting that... -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: ath10k: Missing airtime scheduler parts
Sven Eckelmann writes: > Hi, > > thanks for the reply. I will forward the information. > > On Tuesday, 1 December 2020 11:56:57 CET Toke Høiland-Jørgensen wrote: >> I did recently rebase that patch to the current mac80211-next, actually. >> Sent it off to Felix for some testing, but if you wan to give it a go I >> can post it to the list as well > > Thank you for the offer. But I doubt that I will find time for it > before next year. Alright, fair enough; I'll give Felix a chance to test it first, then... :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH] PCI: Disallow retraining link for Atheros QCA98xx chips on non-Gen1 PCIe bridges
Pali Rohár writes:
> Atheros QCA9880 and QCA9890 chips do not behave after a bus reset and also
> after retrain link when PCIe bridge is not in GEN1 mode at 2.5 GT/s speed.
> The device will throw a Link Down error and config space is not accessible
> again. Retrain link can be called only when using GEN1 PCIe bridge or when
> PCIe bridge has forced link speed to 2.5 GT/s via PCI_EXP_LNKCTL2 register.
>
> This issue was reproduced with more Compex WLE900VX cards (QCA9880 based)
> on Armada 385 with pci-mvebu.c driver and also on Armada 3720 with
> pci-aardvark.c driver. Also this issue was reproduced with some "noname"
> card with QCA9890 WiFi chip on Armada 3720. All problematic cards with
> these QCA chips have PCI device id 0x003c.
>
> Tests showed that other WiFi cards based on AR93xx (PCI device id 0x0030)
> and AR9287 (PCI device id 0x002e) chips do not have these problems.
>
> To workaround this issue, this change introduces a new PCI quirk called
> PCI_DEV_FLAGS_NO_RETRAIN_LINK_WHEN_NOT_GEN1 for PCI device id 0x003c.
>
> When this quirk is set then kernel disallows triggering PCI_EXP_LNKCTL_RL
> bit in config space of PCIe Bridge in case PCIe Bridge is capable of higher
> speed than 2.5 GT/s and higher speed is already allowed. When PCIe Bridge
> has accessible LNKCTL2 register then kernel tries to force target link
> speed via PCI_EXP_LNKCTL2_TLS* bits to 2.5 GT/s. After this change it is
> possible to trigger PCI_EXP_LNKCTL_RL bit without causing issues on
> problematic Atheros QCA98xx cards.
>
> Currently only PCIe ASPM kernel code triggers this PCI_EXP_LNKCTL_RL bit,
> so quirk check is added only into pcie/aspm.c file.
>
> Signed-off-by: Pali Rohár
> Reported-by: Toke Høiland-Jørgensen
> Tested-by: Marek Behún
> Link: https://lore.kernel.org/linux-pci/87h7l8axqp@toke.dk/
> Cc: sta...@vger.kernel.org # c80851f6ce63a ("PCI: Add
> PCI_EXP_LNKCTL2_TLS* macros")
Thanks!
Tested-by: Toke Høiland-Jørgensen
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH] mac80211: Fix wrong channel bandwidths reported for aggregates
Linus Lüssing writes: > On 19/07/2022 17:03, Adrian Chadd wrote: >> Hi! >> >> It's not a hardware bug. Dating back to the original AR5416 11n chip, >> most flags aren't valid for subframes in an aggregate. Only the final >> frame has valid flags. This was explicitly covered internally way back >> when. > > Ah, thanks for the clarification! I see it in the datasheet for the > QCA9531, too, now. And thanks for the confirmation, that what we are > doing so far is not correct for ath9k. > > Words 0+2 are valid for all RX descriptors, 0+2+11 valid for the last RX > descriptor of each packet and 0-11 for the last RX descriptor of an > aggregate or last RX descriptor of a stand-alone packet. Or in other > words, word 4, which contains the 20 vs. 40 MHz indicator, is invalid > for any aggregate sub-frame other than the last one. I can rename that > in the commit message. > > > Another approach that also came to my mind was introducing more explicit > flags in cfg80211.h's "struct rate_info", like a RATE_INFO_BW_UNKNOWN in > "enum rate_info_bw" and/or RATE_INFO_FLAGS_UNKNOWN in "enum > rate_info_flags". And setting those flags in ath9k_cmn_process_rate(). > > The current approach is smaller though, as it simply uses the already > existing flags. If anyone has any preferences, please let me know. I have no objections to doing it in mac80211 like you're proposing here :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 0/6] Move TXQ scheduling and airtime fairness into mac80211
Rajkumar Manoharan writes: > Toke, > > It is been a while since mac80211 TXQ discussion started. Here is the > consolidated > series of mac80211, ath9k and ath10k changes to move TXQ scheduling and > airtime > fairness into mac80211. The major changes w.r.t 5th RFC version are in > may_transmit() > API. Whenever the driver checks deficit for given TXQ, the list will be > reordered so that driver/firmware RR quickly becomes in sync with mac80211 > list. Also airtime tasklet approach cumbersome and causing regression > in multi client performance. Cool, thanks. I'll look at this in more detail later, but in the meantime, do you have any test results to share? Specifically, how did you measure that the fairness part actually works? :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Rajkumar Manoharan writes:
> From: Toke Høiland-Jørgensen
>
> This adds airtime accounting and scheduling to the mac80211 TXQ
> scheduler. A new callback, ieee80211_sta_register_airtime(), is added
> that drivers can call to report airtime usage for stations.
>
> When airtime information is present, mac80211 will schedule TXQs
> (through ieee80211_next_txq()) in a way that enforces airtime fairness
> between active stations. This scheduling works the same way as the ath9k
> in-driver airtime fairness scheduling. If no airtime usage is reported
> by the driver, the scheduler will default to round-robin scheduling.
>
> For drivers that don't control TXQ scheduling in software, a new API
> function, ieee80211_txq_may_transmit(), is added which the driver can use
> to check if the TXQ is eligible for transmission, or should be throttled to
> enforce fairness. Calls to this function must also be enclosed in
> ieee80211_txq_schedule_{start,end}() calls to ensure proper locking.
>
> The API ieee80211_txq_may_transmit() also ensures that TXQ list will be
> aligned aginst driver's own round-robin scheduler list. i.e it rotates
> the TXQ list till it makes the requested node becomes the first entry
> in TXQ list. Thus both the TXQ list and driver's list are in sync.
>
> Signed-off-by: Toke Høiland-Jørgensen
> Signed-off-by: Rajkumar Manoharan
> ---
> include/net/mac80211.h | 58 ++
> net/mac80211/cfg.c | 3 ++
> net/mac80211/debugfs.c | 3 ++
> net/mac80211/debugfs_sta.c | 50 --
> net/mac80211/ieee80211_i.h | 2 ++
> net/mac80211/main.c| 4 +++
> net/mac80211/sta_info.c| 45 +--
> net/mac80211/sta_info.h| 13 +++
> net/mac80211/status.c | 6
> net/mac80211/tx.c | 90
> +++---
> 10 files changed, 264 insertions(+), 10 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 2f5c0fbd453c..0ced3adb09ac 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -2334,6 +2334,8 @@ enum ieee80211_hw_flags {
> * @tx_sk_pacing_shift: Pacing shift to set on TCP sockets when frames from
> * them are encountered. The default should typically not be changed,
> * unless the driver has good reasons for needing more buffers.
> + *
> + * @airtime_weight: Default airtime weight preferred by driver.
> */
> struct ieee80211_hw {
> struct ieee80211_conf conf;
> @@ -2370,6 +2372,7 @@ struct ieee80211_hw {
> const struct ieee80211_cipher_scheme *cipher_schemes;
> u8 max_nan_de_entries;
> u8 tx_sk_pacing_shift;
> + u32 airtime_weight;
> };
This doesn't make sense. Airtime weights can be set by userspace, so
even if a driver sets another default it is not guaranteed to be
honoured. So what's the point?
> +bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
> + struct ieee80211_txq *txq)
> +{
> + struct ieee80211_local *local = hw_to_local(hw);
> + struct txq_info *iter, *tmp, *txqi = to_txq_info(txq);
> + struct sta_info *sta;
> + u8 ac = txq->ac;
> +
> + lockdep_assert_held(&local->active_txq_lock[ac]);
> +
> + if (!txqi->txq.sta)
> + goto out;
> +
> + if (list_empty(&txqi->schedule_order))
> + goto out;
> +
> + list_for_each_entry_safe(iter, tmp, &local->active_txqs[ac],
> + schedule_order) {
> + if (iter == txqi)
> + break;
> +
> + if (!iter->txq.sta) {
> + list_move_tail(&iter->schedule_order,
> +&local->active_txqs[ac]);
> + continue;
> + }
> + sta = container_of(iter->txq.sta, struct sta_info, sta);
> + if (sta->airtime[ac].deficit < 0)
> + sta->airtime[ac].deficit += sta->airtime_weight;
> + list_move_tail(&iter->schedule_order, &local->active_txqs[ac]);
> + }
So since we're rotating the queue on every call to the function, I'm
wondering if this actually succeeds in throttling the slow station's
airtime usage enough to achieve fairness? So I'll ask again: Did you
test the fairness performance, and how?
Also, taking a step back:
With this, we're doing lots of work to make sure that the hardware's
round-robin scheduling queue lines up with mac80211; so if we do this
anyway, why can't we just control the order directly from mac80211
(which is what we do with the other next_txq() API)?
-Toke
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Rajkumar Manoharan writes: > On 2018-10-26 07:16, Toke Høiland-Jørgensen wrote: >> Rajkumar Manoharan writes: >> >>> From: Toke Høiland-Jørgensen > [...] >>> u8 max_nan_de_entries; >>> u8 tx_sk_pacing_shift; >>> + u32 airtime_weight; >>> }; >> >> This doesn't make sense. Airtime weights can be set by userspace, so >> even if a driver sets another default it is not guaranteed to be >> honoured. So what's the point? >> > The reason for driver specific default is to avoid performance impact > in ath10k when the user is using vanilla ath10k with default airtime. > As I mentioned earlier, mac80211 default (256us) is too low for 11ac > devices especially with driver is bursting aggregation. > > Yes. I do understand the user can change airtime at anytime but It > must be noted that different airtime weight will result in different > throughput. IMHO the defaults should not impact current benchmark. > Otherwise it will be alarmed as regression later. isn't it? My point is that if the user has to know the implementation-specific limitations of each driver before setting a weight, then it's not a particularly friendly API. I think we should be able to do better than that... >> So since we're rotating the queue on every call to the function, I'm >> wondering if this actually succeeds in throttling the slow station's >> airtime usage enough to achieve fairness? So I'll ask again: Did you >> test the fairness performance, and how? >> > We are collecting data of mixed clients (11ac, 11n and legacy) keeping > them at same distance and different distance. So that lower rate > clients will interfere higher MCS rate station. Also configuring > different airtime weight for each stations so that throttling low rate > clients more should help higher performing(better MCS) clients. > > By allowing different airtime for each stations, the user can control > guest network over primary network. Also It helped to improve > performance of preferred station and algo. to control station is given > to cloud or user application. > > As of now, We are testing with 4 11ac clients of same distance. And > collecting the performance data in multiple iteration. Below are > current data of station's performance (Mbps) against airtime weight. > > airtime station1(%airtime) station2 station3 station4 > (Mbps) > > No ATF 182 168 166169 > > 4ms 170 (100%)164 (100%) 185 (100%)175 (100%) > > 4ms 277 (70%) 115 (10%)103 (10%) 105 (10%) > > 4ms 223 (40%) 214 (40%)109 (10%) 94 (10%) > > 4ms 337 (90%) 182 (8%) 23 (1%)30 (1%) So this looks like it's doing *something*, but not like it's succeeding in achieving the set percentages. Did you check if the actual airtime values (in debugfs) corresponds to the configured weights? > > STA1(11ac) STA2 (11n) STA3(11a) > == == = > > No ATF 225 166 3.5 > > ATF (4ms)234 151 3.5 This also shows like ATF has no effect? >> Also, taking a step back: >> >> With this, we're doing lots of work to make sure that the hardware's >> round-robin scheduling queue lines up with mac80211; so if we do this >> anyway, why can't we just control the order directly from mac80211 >> (which is what we do with the other next_txq() API)? >> > The only way to enforce mac80211 ordering in ath10k is to disable pull > mode in firmware and always operates in push mode similar to ath9k. And I assume that is not too likely to happen, or? What is the benefit of pull mode at the firmware level? -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Rajkumar Manoharan writes: > On 2018-10-28 15:01, Rajkumar Manoharan wrote: >> On 2018-10-28 08:48, Toke Høiland-Jørgensen wrote: >>> Rajkumar Manoharan writes: >>> >>>> >>>> 4ms 223 (40%) 214 (40%)109 (10%) 94 (10%) >>>> >>>> 4ms 337 (90%) 182 (8%) 23 (1%)30 (1%) >>> >>> So this looks like it's doing *something*, but not like it's >>> succeeding >>> in achieving the set percentages. Did you check if the actual airtime >>> values (in debugfs) corresponds to the configured weights? >>> >> No. Will check that. >> > Toke, > > From above results, different airtime for each station is reflecting on > output performance. Unfortunately I don't see such tput difference, when > the tx mode is fixed in push-only. Even low weight station is giving > same > performance. Are you also seeing the same behavior in your setup? Could > you please share your results? Sorry, I've been travelling this week; I'll be back in the office next week and can run some tests then. I may also have an idea for a different algorithm that will work better in pull mode, but need to see if it works at all first :) How do I force ath10k into push mode? -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Rajkumar Manoharan writes: > On 2018-11-02 03:30, Toke Høiland-Jørgensen wrote: >> Rajkumar Manoharan writes: >> >>> On 2018-10-28 15:01, Rajkumar Manoharan wrote: >>>> On 2018-10-28 08:48, Toke Høiland-Jørgensen wrote: >>>>> Rajkumar Manoharan writes: >>>>> >>>>>> >>>>>> 4ms 223 (40%) 214 (40%)109 (10%) 94 (10%) >>>>>> >>>>>> 4ms 337 (90%) 182 (8%) 23 (1%)30 (1%) >>>>> >>>>> So this looks like it's doing *something*, but not like it's >>>>> succeeding >>>>> in achieving the set percentages. Did you check if the actual >>>>> airtime >>>>> values (in debugfs) corresponds to the configured weights? >>>>> >>>> No. Will check that. >>>> >>> Toke, >>> >>> From above results, different airtime for each station is reflecting >>> on >>> output performance. Unfortunately I don't see such tput difference, >>> when >>> the tx mode is fixed in push-only. Even low weight station is giving >>> same >>> performance. Are you also seeing the same behavior in your setup? >>> Could >>> you please share your results? >> >> Sorry, I've been travelling this week; I'll be back in the office next >> week and can run some tests then. I may also have an idea for a >> different algorithm that will work better in pull mode, but need to see >> if it works at all first :) >> > Wow... :) > > Meanwhile we did some more experiments with both modes. The experiment > was done in open environment and fixed rate and UDP traffic ran for 60 > seconds. > > Seems like push mode not honoring the configured weight. Always the > airtime was almost same whereas in pull-mode airtime is changing based > on configured weight. Hence I would like to know your results. Right, so I verified that the current version of the patch set still works with ath9k. However, the ath10k card I have doesn't seem to support peer stats, so I can't test ath10k. $ lspci | grep Qualcomm 03:00.0 Network controller: Qualcomm Atheros QCA986x/988x 802.11ac Wireless Network Adapter $ cat /sys/kernel/debug/ieee80211/phy1/ath10k/chip_id 0x043202ff $ cat /sys/kernel/debug/ieee80211/phy1/ath10k/wmi_services | grep PEER WMI_SERVICE_PEER_CACHING - WMI_SERVICE_PEER_STATS - Is there a way to force-enable airtime support, is this a hardware issue? > sta1sta2sta3sta4 > pull-mode 8s(205us) 18s(3.2ms) 8s(205us) 14s(410us) > 12s(256us) 12s(256us) 13s(256us) 12s(256us) > 14s(4ms)13s(4ms)14s(4ms)13s(4ms) > > push-mode 15s(205us) 12s(3.2ms) 16s(205us) 12s(410us) > 15s(256us) 12s(256us) 16s(256us) 12s(256us) > 14s(4ms)13s(4ms)16s(4ms)12s(4ms) Right, so the pull-mode results are encouraging! *Something* is happening, at least, even though the aggregate airtime doesn't quite match the ratios of the configured weights. Are you running the UDP generator on the AP itself, or on a separate device, BTW? If it's on the AP, the userspace socket can get throttled, which will interfere with results, so it's better to have it on a separate device (connected via ethernet). As for push-mode, could this be because of bad buffer management? I.e., because there's a lag between the time airtime is registered, and the time that airtime usage is reported, the driver just pushes a whole bunch of packets to the firmware when it gets the chance, which prevents the scheduler from throttling properly. This could also explain the better, but not quite perfect, results in pull mode, assuming that pull mode results in better firmware buffer management which reduces, but doesn't quite remove, the lag. If this is indeed the reason, the queue limit patches should hopefully be a solution. So guess we need to get those working as well :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Rajkumar Manoharan writes: > On 2018-11-07 06:53, Toke Høiland-Jørgensen wrote: >> Rajkumar Manoharan writes: >> >>> Meanwhile we did some more experiments with both modes. The experiment >>> was done in open environment and fixed rate and UDP traffic ran for 60 >>> seconds. >>> >>> Seems like push mode not honoring the configured weight. Always the >>> airtime was almost same whereas in pull-mode airtime is changing based >>> on configured weight. Hence I would like to know your results. >> >> Right, so I verified that the current version of the patch set still >> works with ath9k. However, the ath10k card I have doesn't seem to >> support peer stats, so I can't test ath10k. >> >> $ lspci | grep Qualcomm >> 03:00.0 Network controller: Qualcomm Atheros QCA986x/988x 802.11ac >> Wireless Network Adapter >> >> $ cat /sys/kernel/debug/ieee80211/phy1/ath10k/chip_id >> 0x043202ff >> >> $ cat /sys/kernel/debug/ieee80211/phy1/ath10k/wmi_services | grep PEER >> WMI_SERVICE_PEER_CACHING - >> WMI_SERVICE_PEER_STATS - >> > > Oops... Yeah 988x firmware (10.2.4) does not have peer stats support. > >> >> Is there a way to force-enable airtime support, is this a hardware >> issue? >> > Unfortunately not. There is one more pending change that handles > airtime report from HTT tx-compl. Again it depends firmware support. > These experiments are taken with this f/w interface. Will post the > change. Right, thought so. Too bad. Guess you are doing all the ath10k testing, then ;) >>> sta1sta2sta3sta4 >>> pull-mode 8s(205us) 18s(3.2ms) 8s(205us) 14s(410us) >>> 12s(256us) 12s(256us) 13s(256us) 12s(256us) >>> 14s(4ms)13s(4ms)14s(4ms)13s(4ms) >>> >>> push-mode 15s(205us) 12s(3.2ms) 16s(205us) 12s(410us) >>> 15s(256us) 12s(256us) 16s(256us) 12s(256us) >>> 14s(4ms)13s(4ms)16s(4ms)12s(4ms) >> >> Right, so the pull-mode results are encouraging! *Something* is >> happening, at least, even though the aggregate airtime doesn't quite >> match the ratios of the configured weights. >> >> Are you running the UDP generator on the AP itself, or on a separate >> device, BTW? If it's on the AP, the userspace socket can get throttled, >> which will interfere with results, so it's better to have it on a >> separate device (connected via ethernet). >> > Traffic b/w wired client (via ethernet) and wireless clients. Cool. >> As for push-mode, could this be because of bad buffer management? I.e., >> because there's a lag between the time airtime is registered, and the >> time that airtime usage is reported, the driver just pushes a whole >> bunch of packets to the firmware when it gets the chance, which >> prevents >> the scheduler from throttling properly. This could also explain the >> better, but not quite perfect, results in pull mode, assuming that pull >> mode results in better firmware buffer management which reduces, but >> doesn't quite remove, the lag. >> > Hmm... I agree that lag in reporting airtime may cause more data push > to hw. Right now both tx and tx-compl are serialized by > active_txq_lock. So once lock acquired by tx path, it will download > all frames. i.e it is even true for ath9k driver. Hence I am wondering > how it is working only with ath9k. If enough packets are dequeued at once that the TXQ empties and is not put back on the scheduling loop, the next_txq() loop is just going to loop through the remaining TXQs and immediately increase their quantum. In ath9k, there's a maximum of two outstanding aggregates below the TXQ level, but I think you mentioned that ath10k can queue thousands in firmware? Your patch removes the 'max 16 packets at a time' check before the call to ath10k_mac_tx_push_txq(); try adding that back and see if it helps? >> If this is indeed the reason, the queue limit patches should hopefully >> be a solution. So guess we need to get those working as well :) >> > I would prefer to baseline the basic infra into upstream first and do > enhancement on top of that. Sure, I'm fine with merging this first and building on top. > I request you to revisit maintaining per driver default. Otherwise > there would be performance impact with 256us. :( Hmm, how about we make it a driver-specified multiplier instead (which could be 4, 8 or 16 for ath10k)? That way it would still work even if userspace changes the weights. It would affect the minimum possible granularity, but that is probably acceptable; and we would be free to change the mechanism later, without affecting the userspace API. -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 1/6] mac80211: Add TXQ scheduling API
On 9 November 2018 13:00:04 CET, Johannes Berg wrote: >On Sat, 2018-10-20 at 04:05 -0700, Rajkumar Manoharan wrote: >> >> + * @txq: pointer obtained from station or virtual interface, or from >> + * ieee80211_next_txq() > >nit: please just indent by a single tab for continuation, trying to >line >it all up doesn't really make it more readable Not sure I agree about the "not more readable" part, but OK. >I see you're still discussing all this, so I'm going to assume you'll >resend this series eventually :-) Yup. Think we're getting close to something that might be mergeable. Will send another version then :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v3 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Felix Fietkau writes:
> On 2018-11-12 23:51, Rajkumar Manoharan wrote:
>> From: Toke Høiland-Jørgensen
>>
>> This adds airtime accounting and scheduling to the mac80211 TXQ
>> scheduler. A new callback, ieee80211_sta_register_airtime(), is added
>> that drivers can call to report airtime usage for stations.
>>
>> When airtime information is present, mac80211 will schedule TXQs
>> (through ieee80211_next_txq()) in a way that enforces airtime fairness
>> between active stations. This scheduling works the same way as the ath9k
>> in-driver airtime fairness scheduling. If no airtime usage is reported
>> by the driver, the scheduler will default to round-robin scheduling.
>>
>> For drivers that don't control TXQ scheduling in software, a new API
>> function, ieee80211_txq_may_transmit(), is added which the driver can use
>> to check if the TXQ is eligible for transmission, or should be throttled to
>> enforce fairness. Calls to this function must also be enclosed in
>> ieee80211_txq_schedule_{start,end}() calls to ensure proper locking.
>>
>> The API ieee80211_txq_may_transmit() also ensures that TXQ list will be
>> aligned aginst driver's own round-robin scheduler list. i.e it rotates
>> the TXQ list till it makes the requested node becomes the first entry
>> in TXQ list. Thus both the TXQ list and driver's list are in sync.
>>
>> Co-Developed-by: Rajkumar Manoharan
>> Signed-off-by: Toke Høiland-Jørgensen
>> Signed-off-by: Rajkumar Manoharan
>> ---
>> include/net/mac80211.h | 59 ++
>> net/mac80211/cfg.c | 3 ++
>> net/mac80211/debugfs.c | 3 ++
>> net/mac80211/debugfs_sta.c | 50 --
>> net/mac80211/ieee80211_i.h | 2 ++
>> net/mac80211/main.c| 4 +++
>> net/mac80211/sta_info.c| 44 +--
>> net/mac80211/sta_info.h| 13 +++
>> net/mac80211/status.c | 6
>> net/mac80211/tx.c | 90
>> +++---
>> 10 files changed, 264 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
>> index aa4afbf0abaf..a1f1256448f5 100644
>> --- a/net/mac80211/status.c
>> +++ b/net/mac80211/status.c
>> @@ -818,6 +818,12 @@ static void __ieee80211_tx_status(struct ieee80211_hw
>> *hw,
>> ieee80211_sta_tx_notify(sta->sdata, (void *) skb->data,
>> acked, info->status.tx_time);
>>
>> +if (info->status.tx_time &&
>> +wiphy_ext_feature_isset(local->hw.wiphy,
>> +
>> NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
>> +ieee80211_sta_register_airtime(&sta->sta, tid,
>> + info->status.tx_time, 0);
>> +
>> if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS)) {
>> if (info->flags & IEEE80211_TX_STAT_ACK) {
>> if (sta->status_stats.lost_packets)
> I think the same is needed in ieee80211_tx_status_ext.
Right, good point.
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index 305965283506..3f417e80e041 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -3660,12 +3680,74 @@ void ieee80211_return_txq(struct ieee80211_hw *hw,
>> lockdep_assert_held(&local->active_txq_lock[txq->ac]);
>>
>> if (list_empty(&txqi->schedule_order) &&
>> -(!skb_queue_empty(&txqi->frags) || txqi->tin.backlog_packets))
>> -list_add_tail(&txqi->schedule_order,
>> - &local->active_txqs[txq->ac]);
>> +(!skb_queue_empty(&txqi->frags) || txqi->tin.backlog_packets)) {
>> +/* If airtime accounting is active, always enqueue STAs at the
>> + * head of the list to ensure that they only get moved to the
>> + * back by the airtime DRR scheduler once they have a negative
>> + * deficit. A station that already has a negative deficit will
>> + * get immediately moved to the back of the list on the next
>> + * call to ieee80211_next_txq().
>> + */
>> +if (txqi->txq.sta &&
>> +wiphy_ext_feature_isset(local->hw.wiphy,
>> +
>> NL80211_EXT_FEATURE_AI
Re: [Make-wifi-fast] [PATCH v3 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Louie Lu writes: > Hi Rajkumar, Toke, > > I found the series (v3,4/6) remove the debugfs remove reset station's > airtime method, and didn't added at here. > > Not sure how to help this kind of situation, do I need a separate > patch to fix this, or posting the patch here is fine? This is fine; we can fold it into the next version. Thanks :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v3 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Felix Fietkau writes: > On 2018-11-14 18:40, Toke Høiland-Jørgensen wrote: >>> This part doesn't really make much sense to me, but maybe I'm >>> misunderstanding how the code works. >>> Let's assume we have a driver like ath9k or mt76, which tries to keep a >>> number of aggregates in the hardware queue, and the hardware queue is >>> currently empty. >>> If the current txq entry is kept at the head of the schedule list, >>> wouldn't the code just pull from that one over and over again, until >>> enough packets are transmitted by the hardware and their tx status >>> processed? >>> It seems to me that while fairness is still preserved in the long run, >>> this could lead to rather bursty scheduling, which may not be >>> particularly latency friendly. >> >> Yes, it'll be a bit more bursty when the hardware queue is completely >> empty. However, when a TX completion comes back, that will adjust the >> deficit of that sta and cause it to be rotated on the next dequeue. This >> obviously relies on the fact that the lower-level hardware queue is >> sufficiently shallow to not add a lot of latency. But we want that to be >> the case anyway. In practice, it works quite well for ath9k, but not so >> well for ath10k because it has a large buffer in firmware. >> >> If we requeue the TXQ at the end of the list, a station that is taking >> up too much airtime will fail to be throttled properly, so the >> queue-at-head is kinda needed to ensure fairness... > Thanks for the explanation, that makes sense to me. I have an idea on > how to mitigate the burstiness within the driver. I'll write it down in > pseudocode, please let me know if you think that'll work. I don't think it will, unfortunately. For example, consider the case where there are two stations queued; one with a large negative deficit (say, -10ms), and one with a positive deficit. In this case, we really need to throttle the station with a negative deficit. But if the driver loops and caches txqs, we'll get something like the following: - First driver loop iteration: returns TXQ with positive deficit. - Second driver loop iteration: Only the negative-deficit TXQ is in the mac80211 list, so it will loop until that TXQ's deficit turns positive and return it. Because of this, the negative-deficit station won't be throttled, and we won't get fairness. How many frames will mt76 queue up below the driver point? I.e., how much burstiness are you expecting this will introduce on that driver? Taking a step back, it's clear that it would be good to be able to dequeue packets to multiple STAs at once (we need that for MU-MIMO on ath10k as well). However, I don't think we can do that with the round-robin fairness scheduler; so we are going to need a different algorithm. I *think* it may be possible to do this with a virtual-time scheduler, but I haven't sat down and worked out the details yet... -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [Make-wifi-fast] [PATCH v3 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Dave Taht writes: > Toke Høiland-Jørgensen writes: > >> Felix Fietkau writes: >> >>> On 2018-11-14 18:40, Toke Høiland-Jørgensen wrote: >>>>> This part doesn't really make much sense to me, but maybe I'm >>>>> misunderstanding how the code works. >>>>> Let's assume we have a driver like ath9k or mt76, which tries to keep a >>>>> number of aggregates in the hardware queue, and the hardware queue is >>>>> currently empty. >>>>> If the current txq entry is kept at the head of the schedule list, >>>>> wouldn't the code just pull from that one over and over again, until >>>>> enough packets are transmitted by the hardware and their tx status >>>>> processed? >>>>> It seems to me that while fairness is still preserved in the long run, >>>>> this could lead to rather bursty scheduling, which may not be >>>>> particularly latency friendly. >>>> >>>> Yes, it'll be a bit more bursty when the hardware queue is completely >>>> empty. However, when a TX completion comes back, that will adjust the >>>> deficit of that sta and cause it to be rotated on the next dequeue. This >>>> obviously relies on the fact that the lower-level hardware queue is >>>> sufficiently shallow to not add a lot of latency. But we want that to be >>>> the case anyway. In practice, it works quite well for ath9k, but not so >>>> well for ath10k because it has a large buffer in firmware. >>>> >>>> If we requeue the TXQ at the end of the list, a station that is taking >>>> up too much airtime will fail to be throttled properly, so the >>>> queue-at-head is kinda needed to ensure fairness... >>> Thanks for the explanation, that makes sense to me. I have an idea on >>> how to mitigate the burstiness within the driver. I'll write it down in >>> pseudocode, please let me know if you think that'll work. >> >> I don't think it will, unfortunately. For example, consider the case >> where there are two stations queued; one with a large negative deficit >> (say, -10ms), and one with a positive deficit. > > Perhaps a flag for one way or the other? > > if(driver->has_absurd_hardware_queue_depth) doitthisway(); else > doitabetterway(); Well, there's going to be a BQL-like queue limit (but for airtime) on top, which drivers can opt-in to if the hardware has too much queueing. >> In this case, we really need to throttle the station with a negative >> deficit. But if the driver loops and caches txqs, we'll get something >> like the following: >> >> - First driver loop iteration: returns TXQ with positive deficit. >> - Second driver loop iteration: Only the negative-deficit TXQ is in the >> mac80211 list, so it will loop until that TXQ's deficit turns positive >> and return it. >> >> Because of this, the negative-deficit station won't be throttled, and we >> won't get fairness. >> >> How many frames will mt76 queue up below the driver point? I.e., how >> much burstiness are you expecting this will introduce on that driver? >> >> Taking a step back, it's clear that it would be good to be able to >> dequeue packets to multiple STAs at once (we need that for MU-MIMO on >> ath10k as well). However, I don't think we can do that with the >> round-robin fairness scheduler; so we are going to need a different >> algorithm. I *think* it may be possible to do this with a virtual-time >> scheduler, but I haven't sat down and worked out the details yet... > > The answer to which did not fit on the margins of your thesis. :) > > I too have been trying to come up with a better means of gang > scheduling... for about 2 years now. In terms of bitmaps it looks a bit > like QFQ, but honestly... It's not the gang scheduling we need, deciding which devices to send to at once is generally done in firmware anyway. We just need to be able to dequeue packets for more than one station when possible. I don't think we need the fancy bitmap stuff from QFQ since we don't have that many stations to schedule at once; so we can probably live with O(log(n)) in the number of active stations. > Is there going to be some point where whatever we have here is > significantly better than what we had? Or not significantly worse? Or > handwavy enough to fix the rest once enlightenment arrives? > > The perfect is the enemy of the good. Well, what we have now works for ath9k, works reasonably well for ath10k in pull mode, not so well for ath10k in push mode, and then there's Felix' comments in this thread... > I'd rather like the intel folk to be weighing in on this stuff, too, > trying to get an API right requires use cases. Johannes has already reviewed a previous version, and I do believe he said he'd review it again once we have converged on something :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v3 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Hi Felix
Thinking a bit more about this, I think that rather than having the
driver work around the API as in your example...
> do {
> struct ieee80211_txq *pending_txq[4];
> int n_pending_txq = 0;
> int i;
>
> if (hwq->pending < 4)
> break;p
>
> nframes = 0;
>
> ieee80211_txq_schedule_start(hw, ac)
> do {
> bool requeue = false;
>
> struct ieee80211_txq *txq;
>
> txq = ieee80211_next_txq(hw, ac);
> if (!txq)
> break;
>
> nframes += schedule_txq(txq, &requeue);
> if (requeue)
> pending_txq[n_pending_txq++] = txq;
>
> } while (n_pending_txq < ARRAY_SIZE(pending_txq));
>
> for (i = n_pending_txq; i > 0; i--)
> ieee80211_return_txq(hw, pending_txq[i - 1]);
>
> ieee80211_txq_schedule_end(hw, ac)
> } while (nframes);
... really what we want is that the driver can just do this:
ieee80211_txq_schedule_start(hw, ac);
while ((txq = ieee80211_next_txq(hw, ac)) {
schedule_txq(txq, &requeue);
return_txq(txq);
}
ieee80211_txq_schedule_end(hw, ac);
and expect so get through all eligible TXQs. Note that there will be
cases where there is only a single eligible TXQ (such as the example I
gave in the other email); in which case the current version is fine. But
there is (probably) also going to be cases where more than one TXQ is
eligible at the same time, which we cannot handle with the current RR
scheduler.
However, I think that assuming we can get the scheduler to guarantee
that it will return all eligible TXQs between each pair of calls to
schedule_start()/schedule_end(), we should be fine with the current API.
Do you agree with this?
-Toke
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v3 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Felix Fietkau writes:
>> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
>> index aa4afbf0abaf..a1f1256448f5 100644
>> --- a/net/mac80211/status.c
>> +++ b/net/mac80211/status.c
>> @@ -818,6 +818,12 @@ static void __ieee80211_tx_status(struct ieee80211_hw
>> *hw,
>> ieee80211_sta_tx_notify(sta->sdata, (void *) skb->data,
>> acked, info->status.tx_time);
>>
>> +if (info->status.tx_time &&
>> +wiphy_ext_feature_isset(local->hw.wiphy,
>> +
>> NL80211_EXT_FEATURE_AIRTIME_FAIRNESS))
>> +ieee80211_sta_register_airtime(&sta->sta, tid,
>> + info->status.tx_time, 0);
>> +
>> if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS)) {
>> if (info->flags & IEEE80211_TX_STAT_ACK) {
>> if (sta->status_stats.lost_packets)
> I think the same is needed in ieee80211_tx_status_ext.
So finally circled back to this. In ieee80211_tx_status_ext() we don't
have an skb, so we don't know which TID the packet was sent to; what
airtime information would the driver actually provide in this case? Is
it an aggregate of all ACs, or?
-Toke
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [Make-wifi-fast] [PATCH v3 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Johannes Berg writes: > On Thu, 2018-11-15 at 09:10 -0800, Toke Høiland-Jørgensen wrote: >> Louie Lu writes: >> >> > Hi Rajkumar, Toke, >> > >> > I found the series (v3,4/6) remove the debugfs remove reset station's >> > airtime method, and didn't added at here. >> > >> > Not sure how to help this kind of situation, do I need a separate >> > patch to fix this, or posting the patch here is fine? >> >> This is fine; we can fold it into the next version. Thanks :) > > Just FYI - I'm going to assume, given this comment and the long > discussion, that there will be a next version :) Yes. Got caught up in moving before I managed to get another version out. Will get around to it eventually, promise! :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v5 0/6] Move TXQ scheduling and airtime fairness into mac80211
Rajkumar Manoharan writes: > All, > > Sorry for the long delay. Here is the consolidated series of mac80211, > ath9k and ath10k changes for moving TXQ scheduling and airtime fairness > into mac80211 and support airtime fairness. Thanks for taking care of the respin! Johannes, I think it's safe for you to review this version. I have started working on another approach for the scheduler algorithm itself, but it is fine to do as an incremental patch on top of this as it shouldn't impact the API. Having a separate patch also makes testing easier anyway. I'll see if I can't get it to a working state over the holidays... -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v4 6/6] ath10k: reporting estimated tx airtime for fairness
Rajkumar Manoharan writes: > From: Kan Yan > > Transmit airtime will be estimated from last tx rate used. > Firmware report tx rate by peer stats. Airtime is computed > on tx path and the same will be reported to mac80211 upon > tx completion. FYI, it seems that this last patch in the series no longer applies cleanly on top of mac80211-next. May need a refresh. The mac80211 parts were merged into mac80211-next (thanks, Johannes!); I guess the driver updates are waiting for drivers-next to catch up to that? Patches 4 and 5 in the series are fine... -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v4 6/6] ath10k: reporting estimated tx airtime for fairness
Toke Høiland-Jørgensen writes: > Rajkumar Manoharan writes: > >> From: Kan Yan >> >> Transmit airtime will be estimated from last tx rate used. >> Firmware report tx rate by peer stats. Airtime is computed >> on tx path and the same will be reported to mac80211 upon >> tx completion. > > FYI, it seems that this last patch in the series no longer applies > cleanly on top of mac80211-next. May need a refresh. > > The mac80211 parts were merged into mac80211-next (thanks, Johannes!); I > guess the driver updates are waiting for drivers-next to catch up to > that? > > Patches 4 and 5 in the series are fine... This was meant to be a reply to v5 of the patch set (which is where I had the issue); apologies for picking the wrong one to reply to... -Toke > > -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v5 4/6] ath9k: Switch to mac80211 TXQ scheduling and airtime APIs
Just discovered this while working on my follow-up:
> void ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid)
> {
> - struct ath_vif *avp = (struct ath_vif *) tid->an->vif->drv_priv;
> - struct ath_chanctx *ctx = avp->chanctx;
> - struct ath_acq *acq;
> + struct ieee80211_txq *queue =
> + container_of((void *)tid, struct ieee80211_txq, drv_priv);
>
> - if (!ctx || !list_empty(&tid->list))
> - return;
> -
> - acq = &ctx->acq[TID_TO_WME_AC(tid->tidno)];
> - spin_lock_bh(&acq->lock);
> - __ath_tx_queue_tid(sc, tid);
> - spin_unlock_bh(&acq->lock);
> + ieee80211_return_txq(sc->hw, queue);
> }
After we evolved the API, this is now wrong, as ieee80211_return_txq()
should only be called while holding the right lock. I'll post a fixed
version tomorrow.
-Toke
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
[PATCH v6 0/4] Switch ath9k and ath10k to mac80211 airtime framework
This is an updated resend of the driver part of the previous patch set that moves airtime fairness scheduling into mac80211 and enables it for ath10k as well. This version is just a refresh of the driver code, along with a small fix for the issue I noticed yesterday where ath9k was calling ieee80211_return_txq() without proper logging. Kan Yan (1): ath10k: reporting estimated tx airtime for fairness Toke Høiland-Jørgensen (3): mac80211: Expose ieee80211_schedule_txq() function ath9k: Switch to mac80211 TXQ scheduling and airtime APIs ath10k: migrate to mac80211 txq scheduling drivers/net/wireless/ath/ath10k/core.c | 2 - drivers/net/wireless/ath/ath10k/core.h | 8 +- drivers/net/wireless/ath/ath10k/htc.h | 1 - drivers/net/wireless/ath/ath10k/htt_rx.c | 9 + drivers/net/wireless/ath/ath10k/mac.c | 155 - drivers/net/wireless/ath/ath10k/txrx.c | 4 + drivers/net/wireless/ath/ath9k/ath9k.h | 14 -- drivers/net/wireless/ath/ath9k/debug.c | 3 - drivers/net/wireless/ath/ath9k/debug.h | 8 - drivers/net/wireless/ath/ath9k/debug_sta.c | 70 -- drivers/net/wireless/ath/ath9k/init.c | 3 +- drivers/net/wireless/ath/ath9k/recv.c | 9 +- drivers/net/wireless/ath/ath9k/xmit.c | 244 ++--- include/net/mac80211.h | 13 ++ net/mac80211/driver-ops.h | 4 +- net/mac80211/tx.c | 13 ++ 16 files changed, 217 insertions(+), 343 deletions(-) -- 2.20.1 ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
[PATCH v7 1/4] mac80211: Expose ieee80211_schedule_txq() function
Since we reworked ieee80211_return_txq() so it assumes that the caller
takes care of logging, we need another function that can be called without
holding any locks. Introduce ieee80211_schedule_txq() which serves this
purpose.
Signed-off-by: Toke Høiland-Jørgensen
---
include/net/mac80211.h| 13 +
net/mac80211/driver-ops.h | 4 +---
net/mac80211/tx.c | 13 +
3 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index a285c2bfd14e..294a8a36012a 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -6209,6 +6209,19 @@ void ieee80211_txq_schedule_start(struct ieee80211_hw
*hw, u8 ac)
void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac)
__releases(txq_lock);
+/**
+ * ieee80211_schedule_txq - schedule a TXQ for transmission
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Schedules a TXQ for transmission if it is not already scheduled. Takes a
+ * lock, which means it must *not* be called between
+ * ieee80211_txq_schedule_start() and ieee80211_txq_schedule_end()
+ */
+void ieee80211_schedule_txq(struct ieee80211_hw *hw, struct ieee80211_txq *txq)
+ __acquires(txq_lock) __releases(txq_lock);
+
/**
* ieee80211_txq_may_transmit - check whether TXQ is allowed to transmit
*
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 1aab1734b26f..ba3c07b10cd0 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1176,9 +1176,7 @@ static inline void drv_wake_tx_queue(struct
ieee80211_local *local,
static inline void schedule_and_wake_txq(struct ieee80211_local *local,
struct txq_info *txqi)
{
- spin_lock_bh(&local->active_txq_lock[txqi->txq.ac]);
- ieee80211_return_txq(&local->hw, &txqi->txq);
- spin_unlock_bh(&local->active_txq_lock[txqi->txq.ac]);
+ ieee80211_schedule_txq(&local->hw, &txqi->txq);
drv_wake_tx_queue(local, txqi);
}
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index f46d8d822f86..17744be84b34 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3703,6 +3703,19 @@ void ieee80211_return_txq(struct ieee80211_hw *hw,
}
EXPORT_SYMBOL(ieee80211_return_txq);
+void ieee80211_schedule_txq(struct ieee80211_hw *hw,
+ struct ieee80211_txq *txq)
+ __acquires(txq_lock) __releases(txq_lock)
+{
+ struct ieee80211_local *local = hw_to_local(hw);
+ struct txq_info *txqi = to_txq_info(txq);
+
+ spin_lock_bh(&local->active_txq_lock[txq->ac]);
+ ieee80211_return_txq(hw, txq);
+ spin_unlock_bh(&local->active_txq_lock[ac]);
+}
+EXPORT_SYMBOL(ieee80211_schedule_txq);
+
bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
struct ieee80211_txq *txq)
{
--
2.20.1
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
[PATCH v6 4/4] ath10k: reporting estimated tx airtime for fairness
From: Kan Yan
Transmit airtime will be estimated from last tx rate used.
Firmware report tx rate by peer stats. Airtime is computed
on tx path and the same will be reported to mac80211 upon
tx completion.
This change is based on Kan's orginal commit in Chromium tree
("CHROMIUM: ath10k: Implementing airtime fairness based TX scheduler")
ref: https://chromium-review.googlesource.com/588190
Tested on QCA4019 with firmware version 10.4-3.2.1.1-00015
Tested on QCA9984 with firmware version 10.4-3.9.0.1-5
Signed-off-by: Kan Yan
[rmano...@codeaurora.org: ported only the airtime computation]
Signed-off-by: Rajkumar Manoharan
[t...@redhat.com: Rebase to mac80211-next, add test note]
Signed-off-by: Toke Høiland-Jørgensen
---
Rajkumar / Kan: Please check that I didn't mess anything up while
rebasing this onto the current mac80211-next branch!
drivers/net/wireless/ath/ath10k/core.h | 2 +
drivers/net/wireless/ath/ath10k/htt_rx.c | 1 +
drivers/net/wireless/ath/ath10k/mac.c| 57 ++--
drivers/net/wireless/ath/ath10k/txrx.c | 4 ++
4 files changed, 61 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.h
b/drivers/net/wireless/ath/ath10k/core.h
index 28cecf7375f8..4528a6e47b8d 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -126,6 +126,7 @@ struct ath10k_skb_cb {
u8 flags;
u8 eid;
u16 msdu_id;
+ u16 airtime_est;
struct ieee80211_vif *vif;
struct ieee80211_txq *txq;
} __packed;
@@ -498,6 +499,7 @@ struct ath10k_sta {
u16 peer_id;
struct rate_info txrate;
struct ieee80211_tx_info tx_info;
+ u32 last_tx_bitrate;
struct work_struct update_wk;
u64 rx_duration;
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c
b/drivers/net/wireless/ath/ath10k/htt_rx.c
index dc9895f2eb9d..6a93b57900ae 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -3078,6 +3078,7 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar,
arsta->txrate.nss = txrate.nss;
arsta->txrate.bw = ath10k_bw_to_mac80211_bw(txrate.bw);
+ arsta->last_tx_bitrate = cfg80211_calculate_bitrate(&arsta->txrate);
if (sgi)
arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c
b/drivers/net/wireless/ath/ath10k/mac.c
index cf7fdf72e990..b13dcb4533cb 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3544,7 +3544,7 @@ static void ath10k_tx_h_add_p2p_noa_ie(struct ath10k *ar,
static void ath10k_mac_tx_h_fill_cb(struct ath10k *ar,
struct ieee80211_vif *vif,
struct ieee80211_txq *txq,
- struct sk_buff *skb)
+ struct sk_buff *skb, u16 airtime)
{
struct ieee80211_hdr *hdr = (void *)skb->data;
struct ath10k_skb_cb *cb = ATH10K_SKB_CB(skb);
@@ -3561,6 +3561,7 @@ static void ath10k_mac_tx_h_fill_cb(struct ath10k *ar,
cb->vif = vif;
cb->txq = txq;
+ cb->airtime_est = airtime;
}
bool ath10k_mac_tx_frm_has_freq(struct ath10k *ar)
@@ -3948,6 +3949,49 @@ static bool ath10k_mac_tx_can_push(struct ieee80211_hw
*hw,
return false;
}
+/* Return estimated airtime in microsecond, which is calculated using last
+ * reported TX rate. This is just a rough estimation because host driver has no
+ * knowledge of the actual transmit rate, retries or aggregation. If actual
+ * airtime can be reported by firmware, then delta between estimated and actual
+ * airtime can be adjusted from deficit.
+ */
+#define IEEE80211_ATF_OVERHEAD 100 /* IFS + some slot time */
+#define IEEE80211_ATF_OVERHEAD_IFS 16 /* IFS only */
+static u16 ath10k_mac_update_airtime(struct ath10k *ar,
+struct ieee80211_txq *txq,
+struct sk_buff *skb)
+{
+ struct ath10k_sta *arsta;
+ u32 pktlen;
+ u16 airtime = 0;
+
+ if (!txq || !txq->sta)
+ return airtime;
+
+ spin_lock_bh(&ar->data_lock);
+ arsta = (struct ath10k_sta *)txq->sta->drv_priv;
+
+ pktlen = skb->len + 38; /* Assume MAC header 30, SNAP 8 for most case */
+ if (arsta->last_tx_bitrate) {
+ /* airtime in us, last_tx_bitrate in 100kbps */
+ airtime = (pktlen * 8 * (1000 / 100))
+ / arsta->last_tx_bitrate;
+ /* overhead for media access time and IFS */
+ airtime += IEEE80211_ATF_OVERHEAD_IFS;
+ } else {
+ /* This is mostly for throttle excessive BC/MC frames, and the
+* airtime/rate doesn't need be exact. Airtime of BC/M
[PATCH v6 2/4] ath9k: Switch to mac80211 TXQ scheduling and airtime APIs
From: Toke Høiland-Jørgensen
This moves the ath9k driver to use the mac80211 TXQ scheduling and
airtime accounting APIs, removing the corresponding state tracking
inside the driver.
Signed-off-by: Toke Høiland-Jørgensen
[rmano...@codeaurora.org: fixed checkpatch error and warnings]
Signed-off-by: Rajkumar Manoharan
---
v6:
- Use the ieee80211_schedule_txq() function introduced in the previous patch
instead of calling ieee80211_return_txq() with no locking.
drivers/net/wireless/ath/ath9k/ath9k.h | 14 --
drivers/net/wireless/ath/ath9k/debug.c | 3 -
drivers/net/wireless/ath/ath9k/debug.h | 8 -
drivers/net/wireless/ath/ath9k/debug_sta.c | 70 --
drivers/net/wireless/ath/ath9k/init.c | 3 +-
drivers/net/wireless/ath/ath9k/recv.c | 9 +-
drivers/net/wireless/ath/ath9k/xmit.c | 244 ++---
7 files changed, 75 insertions(+), 276 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h
b/drivers/net/wireless/ath/ath9k/ath9k.h
index 0fca44e91a71..a412b352182c 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -112,8 +112,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct
ath_descdma *dd,
#define ATH_TXFIFO_DEPTH 8
#define ATH_TX_ERROR 0x01
-#define ATH_AIRTIME_QUANTUM300 /* usec */
-
/* Stop tx traffic 1ms before the GO goes away */
#define ATH_P2P_PS_STOP_TIME 1000
@@ -246,10 +244,8 @@ struct ath_atx_tid {
s8 bar_index;
bool active;
bool clear_ps_filter;
- bool has_queued;
};
-void __ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
void ath_tx_queue_tid(struct ath_softc *sc, struct ath_atx_tid *tid);
struct ath_node {
@@ -263,12 +259,9 @@ struct ath_node {
bool sleeping;
bool no_ps_filter;
- s64 airtime_deficit[IEEE80211_NUM_ACS];
- u32 airtime_rx_start;
#ifdef CONFIG_ATH9K_STATION_STATISTICS
struct ath_rx_rate_stats rx_rate_stats;
- struct ath_airtime_stats airtime_stats;
#endif
u8 key_idx[4];
@@ -986,11 +979,6 @@ void ath_ant_comb_scan(struct ath_softc *sc, struct
ath_rx_status *rs);
#define ATH9K_NUM_CHANCTX 2 /* supports 2 operating channels */
-#define AIRTIME_USE_TX BIT(0)
-#define AIRTIME_USE_RX BIT(1)
-#define AIRTIME_USE_NEW_QUEUES BIT(2)
-#define AIRTIME_ACTIVE(flags) (!!(flags & (AIRTIME_USE_TX|AIRTIME_USE_RX)))
-
struct ath_softc {
struct ieee80211_hw *hw;
struct device *dev;
@@ -1034,8 +1022,6 @@ struct ath_softc {
short nbcnvifs;
unsigned long ps_usecount;
- u16 airtime_flags; /* AIRTIME_* */
-
struct ath_rx rx;
struct ath_tx tx;
struct ath_beacon beacon;
diff --git a/drivers/net/wireless/ath/ath9k/debug.c
b/drivers/net/wireless/ath/ath9k/debug.c
index 4399e9ad058f..0dfea5d6e949 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -1443,9 +1443,6 @@ int ath9k_init_debug(struct ath_hw *ah)
#endif
debugfs_create_file("tpc", 0600, sc->debug.debugfs_phy, sc, &fops_tpc);
- debugfs_create_u16("airtime_flags", 0600,
- sc->debug.debugfs_phy, &sc->airtime_flags);
-
debugfs_create_file("nf_override", 0600,
sc->debug.debugfs_phy, sc, &fops_nf_override);
diff --git a/drivers/net/wireless/ath/ath9k/debug.h
b/drivers/net/wireless/ath/ath9k/debug.h
index 79607db14387..33826aa13687 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -319,20 +319,12 @@ ath9k_debug_sync_cause(struct ath_softc *sc, u32
sync_cause)
void ath_debug_rate_stats(struct ath_softc *sc,
struct ath_rx_status *rs,
struct sk_buff *skb);
-void ath_debug_airtime(struct ath_softc *sc,
- struct ath_node *an,
- u32 rx, u32 tx);
#else
static inline void ath_debug_rate_stats(struct ath_softc *sc,
struct ath_rx_status *rs,
struct sk_buff *skb)
{
}
-static inline void ath_debug_airtime(struct ath_softc *sc,
- struct ath_node *an,
- u32 rx, u32 tx)
-{
-}
#endif /* CONFIG_ATH9K_STATION_STATISTICS */
#endif /* DEBUG_H */
diff --git a/drivers/net/wireless/ath/ath9k/debug_sta.c
b/drivers/net/wireless/ath/ath9k/debug_sta.c
index e8fcd3e1c470..d95cabddce33 100644
--- a/drivers/net/wireless/ath/ath9k/debug_sta.c
+++ b/drivers/net/wireless/ath/ath9k/debug_sta.c
@@ -242,75 +242,6 @@ static const struct file_operations fops_node_recv = {
.llseek = default_llseek,
};
-void ath_debug_airtime(struct ath_softc *sc,
- struct ath_node *an,
- u32 rx,
- u32 tx)
-{
-
[PATCH v6 3/4] ath10k: migrate to mac80211 txq scheduling
From: Toke Høiland-Jørgensen
ath10k maintains common txqs list for all stations. This txq
management can be removed by migrating to mac80211 txq APIs
and let mac80211 handle txqs reordering based on reported airtime.
By doing this, txq fairness maintained in ath10k i.e processing
N frames per txq is removed. By adapting to mac80211 APIs,
ath10k will support mac80211 based airtime fairness algorithm.
Tested on QCA4019 with firmware version 10.4-3.2.1.1-00015
Tested on QCA9984 with firmware version 10.4-3.9.0.1-5
Tested-by: Venkateswara Naralasetty
Co-developed-by: Rajkumar Manoharan
Signed-off-by: Rajkumar Manoharan
Signed-off-by: Toke Høiland-Jørgensen
---
drivers/net/wireless/ath/ath10k/core.c | 2 -
drivers/net/wireless/ath/ath10k/core.h | 6 +-
drivers/net/wireless/ath/ath10k/htc.h| 1 -
drivers/net/wireless/ath/ath10k/htt_rx.c | 8 ++
drivers/net/wireless/ath/ath10k/mac.c| 98 +++-
5 files changed, 54 insertions(+), 61 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.c
b/drivers/net/wireless/ath/ath10k/core.c
index 399b501f3c3c..c7327900316e 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -3098,9 +3098,7 @@ struct ath10k *ath10k_core_create(size_t priv_size,
struct device *dev,
mutex_init(&ar->conf_mutex);
spin_lock_init(&ar->data_lock);
- spin_lock_init(&ar->txqs_lock);
- INIT_LIST_HEAD(&ar->txqs);
INIT_LIST_HEAD(&ar->peers);
init_waitqueue_head(&ar->peer_mapping_wq);
init_waitqueue_head(&ar->htt.empty_tx_wq);
diff --git a/drivers/net/wireless/ath/ath10k/core.h
b/drivers/net/wireless/ath/ath10k/core.h
index 46e9c8c97a4d..28cecf7375f8 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -90,6 +90,9 @@
/* The magic used by QCA spec */
#define ATH10K_SMBIOS_BDF_EXT_MAGIC "BDF_"
+/* Default Airtime weight multipler (Tuned for multiclient performance) */
+#define ATH10K_AIRTIME_WEIGHT_MULTIPLIER 4
+
struct ath10k;
static inline const char *ath10k_bus_str(enum ath10k_bus bus)
@@ -1068,10 +1071,7 @@ struct ath10k {
/* protects shared structure data */
spinlock_t data_lock;
- /* protects: ar->txqs, artxq->list */
- spinlock_t txqs_lock;
- struct list_head txqs;
struct list_head arvifs;
struct list_head peers;
struct ath10k_peer *peer_map[ATH10K_MAX_NUM_PEER_IDS];
diff --git a/drivers/net/wireless/ath/ath10k/htc.h
b/drivers/net/wireless/ath/ath10k/htc.h
index 51fda6c23f69..cb30add7dd33 100644
--- a/drivers/net/wireless/ath/ath10k/htc.h
+++ b/drivers/net/wireless/ath/ath10k/htc.h
@@ -51,7 +51,6 @@ struct ath10k;
*/
#define HTC_HOST_MAX_MSG_PER_RX_BUNDLE8
-#define HTC_HOST_MAX_MSG_PER_TX_BUNDLE16
enum ath10k_htc_tx_flags {
ATH10K_HTC_FLAG_NEED_CREDIT_UPDATE = 0x01,
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c
b/drivers/net/wireless/ath/ath10k/htt_rx.c
index f42bac204ef8..dc9895f2eb9d 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -2596,6 +2596,7 @@ static void ath10k_htt_rx_tx_fetch_ind(struct ath10k *ar,
struct sk_buff *skb)
u8 tid;
int ret;
int i;
+ bool may_tx;
ath10k_dbg(ar, ATH10K_DBG_HTT, "htt rx tx fetch ind\n");
@@ -2668,8 +2669,13 @@ static void ath10k_htt_rx_tx_fetch_ind(struct ath10k
*ar, struct sk_buff *skb)
num_msdus = 0;
num_bytes = 0;
+ ieee80211_txq_schedule_start(hw, txq->ac);
+ may_tx = ieee80211_txq_may_transmit(hw, txq);
while (num_msdus < max_num_msdus &&
num_bytes < max_num_bytes) {
+ if (!may_tx)
+ break;
+
ret = ath10k_mac_tx_push_txq(hw, txq);
if (ret < 0)
break;
@@ -2677,6 +2683,8 @@ static void ath10k_htt_rx_tx_fetch_ind(struct ath10k *ar,
struct sk_buff *skb)
num_msdus++;
num_bytes += ret;
}
+ ieee80211_return_txq(hw, txq);
+ ieee80211_txq_schedule_end(hw, txq->ac);
record->num_msdus = cpu_to_le16(num_msdus);
record->num_bytes = cpu_to_le32(num_bytes);
diff --git a/drivers/net/wireless/ath/ath10k/mac.c
b/drivers/net/wireless/ath/ath10k/mac.c
index 49758490eaba..cf7fdf72e990 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3890,7 +3890,6 @@ static void ath10k_mac_txq_init(struct ieee80211_txq *txq)
static void ath10k_mac_txq_unref(struct ath10k *ar, struct ieee80211_txq *txq)
{
- struct ath10k_txq *artxq;
struct ath10k_skb_cb *cb;
Re: [PATCH v7 1/4] mac80211: Expose ieee80211_schedule_txq() function
Toke Høiland-Jørgensen writes: > Since we reworked ieee80211_return_txq() so it assumes that the caller > takes care of logging, we need another function that can be called without > holding any locks. Introduce ieee80211_schedule_txq() which serves this > purpose. Hmm, messed up the version number on this one, sorry about that. It was supposed to be v6 along with the rest... -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v6 4/4] ath10k: reporting estimated tx airtime for fairness
Kan Yan writes: > Hi Toke, > > This patch looks good to me. Great job on getting the airtime fairness > scheduling framework done! Great, thanks for checking! Have another patch to test on top of this series, then we should look into getting the airtime queue limit patch into mac80211 as well :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v7 1/4] mac80211: Expose ieee80211_schedule_txq() function
Johannes Berg writes:
>> +void ieee80211_schedule_txq(struct ieee80211_hw *hw,
>> +struct ieee80211_txq *txq)
>> +__acquires(txq_lock) __releases(txq_lock)
>> +{
>> +struct ieee80211_local *local = hw_to_local(hw);
>> +struct txq_info *txqi = to_txq_info(txq);
>> +
>> +spin_lock_bh(&local->active_txq_lock[txq->ac]);
>> +ieee80211_return_txq(hw, txq);
>> +spin_unlock_bh(&local->active_txq_lock[ac]);
>>
> Maybe v6 had txq->ac here instead of just ac which doesn't compile ;-)
>
> I fixed it up, but I hope you tested a compiling version :P
Ah, right, thanks for fixing it! I think this was an artifact of moving
things around while rebasing for submission (I have another patch on top
of this series that I need to test first).
So yeah, I definitely have a version in my tree somewhere that actually
compiles ;)
-Toke
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 2/2] ath10k: Set sk_pacing_shift to 6 for 11AC WiFi chips
Grant Grundler writes: > On Thu, Sep 6, 2018 at 3:18 AM Toke Høiland-Jørgensen wrote: >> >> Grant Grundler writes: >> >> >> And, well, Grant's data is from a single test in a noisy >> >> environment where the time series graph shows that throughput is all over >> >> the place for the duration of the test; so it's hard to draw solid >> >> conclusions from (for instance, for the 5-stream test, the average >> >> throughput for 6 is 331 and 379 Mbps for the two repetitions, and for 7 >> >> it's 326 and 371 Mbps) . Unfortunately I don't have the same hardware >> >> used in this test, so I can't go verify it myself; so the only thing I >> >> can do is grumble about it here... :) >> > >> > It's a fair complaint and I agree with it. My counter argument is the >> > opposite is true too: most ideal benchmarks don't measure what most >> > users see. While the data wgong provided are way more noisy than I >> > like, my overall "confidence" in the "conclusion" I offered is still >> > positive. >> >> Right. I guess I would just prefer a slightly more comprehensive >> evaluation to base a 4x increase in buffer size on... > > Kalle, is this why you didn't accept this patch? Other reasons? > > Toke, what else would you like to see evaluated? > > I generally want to see three things measured when "benchmarking" > technologies: throughput, latency, cpu utilization > We've covered those three I think "reasonably". Hmm, going back and looking at this (I'd completely forgotten about this patch), I think I had two main concerns: 1. What happens in a degraded signal situation, where the throughput is limited by the signal conditions, or by contention with other devices. Both of these happen regularly, and I worry that latency will be badly affected under those conditions. 2. What happens with old hardware that has worse buffer management in the driver->firmware path (especially drivers without push/pull mode support)? For these, the lower-level queueing structure is less effective at controlling queueing latency. Getting the queue size limit patches from ChromeOS ported would alleviate point 2. I do believe Kan said he'd look into that once the airtime patches were merged. So Kan, any progress on that front? :) > What does a "4x increase in memory" mean here? Wen, how much more > memory does this cause ath10k to use? I didn't say "memory", I said "buffer size"... :) I.e., it's the latency impact of the increased buffering I'm worried about (see above), not the system memory usage. -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 2/2] ath10k: Set sk_pacing_shift to 6 for 11AC WiFi chips
Kalle Valo writes: > Toke Høiland-Jørgensen writes: > >> Grant Grundler writes: >> >>> On Thu, Sep 6, 2018 at 3:18 AM Toke Høiland-Jørgensen wrote: >>>> >>>> Grant Grundler writes: >>>> >>>> >> And, well, Grant's data is from a single test in a noisy >>>> >> environment where the time series graph shows that throughput is all >>>> >> over >>>> >> the place for the duration of the test; so it's hard to draw solid >>>> >> conclusions from (for instance, for the 5-stream test, the average >>>> >> throughput for 6 is 331 and 379 Mbps for the two repetitions, and for 7 >>>> >> it's 326 and 371 Mbps) . Unfortunately I don't have the same hardware >>>> >> used in this test, so I can't go verify it myself; so the only thing I >>>> >> can do is grumble about it here... :) >>>> > >>>> > It's a fair complaint and I agree with it. My counter argument is the >>>> > opposite is true too: most ideal benchmarks don't measure what most >>>> > users see. While the data wgong provided are way more noisy than I >>>> > like, my overall "confidence" in the "conclusion" I offered is still >>>> > positive. >>>> >>>> Right. I guess I would just prefer a slightly more comprehensive >>>> evaluation to base a 4x increase in buffer size on... >>> >>> Kalle, is this why you didn't accept this patch? Other reasons? >>> >>> Toke, what else would you like to see evaluated? >>> >>> I generally want to see three things measured when "benchmarking" >>> technologies: throughput, latency, cpu utilization >>> We've covered those three I think "reasonably". >> >> Hmm, going back and looking at this (I'd completely forgotten about this >> patch), I think I had two main concerns: >> >> 1. What happens in a degraded signal situation, where the throughput is >>limited by the signal conditions, or by contention with other devices. >>Both of these happen regularly, and I worry that latency will be >>badly affected under those conditions. >> >> 2. What happens with old hardware that has worse buffer management in >>the driver->firmware path (especially drivers without push/pull mode >>support)? For these, the lower-level queueing structure is less >>effective at controlling queueing latency. > > Do note that this patch changes behaviour _only_ for QCA6174 and QCA9377 > PCI devices, which IIRC do not even support push/pull mode. All the > rest, including QCA988X and QCA9984 are unaffected. Ah, right; I did not go all the way back and look at the actual patch, so missed that :) But in that case, why are the latency results that low? Were these tests done with the ChromeOS queue limit patches? -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 2/2] ath10k: Set sk_pacing_shift to 6 for 11AC WiFi chips
Ben Greear writes: > On 2/21/19 8:10 AM, Kalle Valo wrote: >> Toke Høiland-Jørgensen writes: >> >>> Grant Grundler writes: >>> >>>> On Thu, Sep 6, 2018 at 3:18 AM Toke Høiland-Jørgensen wrote: >>>>> >>>>> Grant Grundler writes: >>>>> >>>>>>> And, well, Grant's data is from a single test in a noisy >>>>>>> environment where the time series graph shows that throughput is all >>>>>>> over >>>>>>> the place for the duration of the test; so it's hard to draw solid >>>>>>> conclusions from (for instance, for the 5-stream test, the average >>>>>>> throughput for 6 is 331 and 379 Mbps for the two repetitions, and for 7 >>>>>>> it's 326 and 371 Mbps) . Unfortunately I don't have the same hardware >>>>>>> used in this test, so I can't go verify it myself; so the only thing I >>>>>>> can do is grumble about it here... :) >>>>>> >>>>>> It's a fair complaint and I agree with it. My counter argument is the >>>>>> opposite is true too: most ideal benchmarks don't measure what most >>>>>> users see. While the data wgong provided are way more noisy than I >>>>>> like, my overall "confidence" in the "conclusion" I offered is still >>>>>> positive. >>>>> >>>>> Right. I guess I would just prefer a slightly more comprehensive >>>>> evaluation to base a 4x increase in buffer size on... >>>> >>>> Kalle, is this why you didn't accept this patch? Other reasons? >>>> >>>> Toke, what else would you like to see evaluated? >>>> >>>> I generally want to see three things measured when "benchmarking" >>>> technologies: throughput, latency, cpu utilization >>>> We've covered those three I think "reasonably". >>> >>> Hmm, going back and looking at this (I'd completely forgotten about this >>> patch), I think I had two main concerns: >>> >>> 1. What happens in a degraded signal situation, where the throughput is >>> limited by the signal conditions, or by contention with other devices. >>> Both of these happen regularly, and I worry that latency will be >>> badly affected under those conditions. >>> >>> 2. What happens with old hardware that has worse buffer management in >>> the driver->firmware path (especially drivers without push/pull mode >>> support)? For these, the lower-level queueing structure is less >>> effective at controlling queueing latency. >> >> Do note that this patch changes behaviour _only_ for QCA6174 and QCA9377 >> PCI devices, which IIRC do not even support push/pull mode. All the >> rest, including QCA988X and QCA9984 are unaffected. > > Just as a note, at least kernels such as 4.14.whatever perform poorly when > running ath10k on 9984 when acting as TCP endpoints. This makes them not > really usable for stuff like serving video to lots of clients. > > Tweaking TCP (I do it a bit differently, but either way) can significantly > improve performance. Differently how? Did you have to do more than fiddle with the pacing_shift? > Recently I helped a user that could get barely 70 stations streaming > at 1Mbps on stock kernel (using one wave1 on 2.4, one wave-2 on 5Ghz), > and we got 110 working with a tweaked TCP stack. These were /n > stations too. > > I think it is lame that it _still_ requires out of tree patches to > make TCP work well on ath10k...even if you want to default to current > behaviour, you should allow users to tweak it to work with their use > case. Well if TCP is broken to the point of being unusable I do think we should fix it; but I think "just provide a configuration knob" should be the last resort... -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 2/2] ath10k: Set sk_pacing_shift to 6 for 11AC WiFi chips
Ben Greear writes: > On 2/21/19 8:37 AM, Toke Høiland-Jørgensen wrote: >> Ben Greear writes: >> >>> On 2/21/19 8:10 AM, Kalle Valo wrote: >>>> Toke Høiland-Jørgensen writes: >>>> >>>>> Grant Grundler writes: >>>>> >>>>>> On Thu, Sep 6, 2018 at 3:18 AM Toke Høiland-Jørgensen >>>>>> wrote: >>>>>>> >>>>>>> Grant Grundler writes: >>>>>>> >>>>>>>>> And, well, Grant's data is from a single test in a noisy >>>>>>>>> environment where the time series graph shows that throughput is all >>>>>>>>> over >>>>>>>>> the place for the duration of the test; so it's hard to draw solid >>>>>>>>> conclusions from (for instance, for the 5-stream test, the average >>>>>>>>> throughput for 6 is 331 and 379 Mbps for the two repetitions, and for >>>>>>>>> 7 >>>>>>>>> it's 326 and 371 Mbps) . Unfortunately I don't have the same hardware >>>>>>>>> used in this test, so I can't go verify it myself; so the only thing I >>>>>>>>> can do is grumble about it here... :) >>>>>>>> >>>>>>>> It's a fair complaint and I agree with it. My counter argument is the >>>>>>>> opposite is true too: most ideal benchmarks don't measure what most >>>>>>>> users see. While the data wgong provided are way more noisy than I >>>>>>>> like, my overall "confidence" in the "conclusion" I offered is still >>>>>>>> positive. >>>>>>> >>>>>>> Right. I guess I would just prefer a slightly more comprehensive >>>>>>> evaluation to base a 4x increase in buffer size on... >>>>>> >>>>>> Kalle, is this why you didn't accept this patch? Other reasons? >>>>>> >>>>>> Toke, what else would you like to see evaluated? >>>>>> >>>>>> I generally want to see three things measured when "benchmarking" >>>>>> technologies: throughput, latency, cpu utilization >>>>>> We've covered those three I think "reasonably". >>>>> >>>>> Hmm, going back and looking at this (I'd completely forgotten about this >>>>> patch), I think I had two main concerns: >>>>> >>>>> 1. What happens in a degraded signal situation, where the throughput is >>>>> limited by the signal conditions, or by contention with other >>>>> devices. >>>>> Both of these happen regularly, and I worry that latency will be >>>>> badly affected under those conditions. >>>>> >>>>> 2. What happens with old hardware that has worse buffer management in >>>>> the driver->firmware path (especially drivers without push/pull mode >>>>> support)? For these, the lower-level queueing structure is less >>>>> effective at controlling queueing latency. >>>> >>>> Do note that this patch changes behaviour _only_ for QCA6174 and QCA9377 >>>> PCI devices, which IIRC do not even support push/pull mode. All the >>>> rest, including QCA988X and QCA9984 are unaffected. >>> >>> Just as a note, at least kernels such as 4.14.whatever perform poorly when >>> running ath10k on 9984 when acting as TCP endpoints. This makes them not >>> really usable for stuff like serving video to lots of clients. >>> >>> Tweaking TCP (I do it a bit differently, but either way) can significantly >>> improve performance. >> >> Differently how? Did you have to do more than fiddle with the pacing_shift? > > This one, or a slightly tweaked version that applies to different kernels: > > https://github.com/greearb/linux-ct-4.16/commit/3e14e8491a5b31ce994fb2752347145e6ab7eaf5 Right; but the current mac80211 default (pacing shift 8) corresponds to setting your sysctl to 4... >>> Recently I helped a user that could get barely 70 stations streaming >>> at 1Mbps on stock kernel (using one wave1 on 2.4, one wave-2 on 5Ghz), >>> and we got 110 working with a tweaked TCP stack. These were /n >>> stations too. >>> >>> I think it is lame that it _still_ requires out of tree patches to >>> make TCP work well on ath10k...even if you want to default to current >>> behaviour, you should allow users to tweak it to work with their use >>> case. >> >> Well if TCP is broken to the point of being unusable I do think we >> should fix it; but I think "just provide a configuration knob" should be >> the last resort... > > So, it has been broken for years, and waiting for a perfect solution > has not gotten the problem fixed. Well, the current default should at least be closer to something that works well. I do think I may have erred on the wrong side of the optimum when I submitted the original patch to set the default to 8; that should probably have been 7 (i.e., 8 ms; the optimum in the evaluation we did was around 6 ms, which is sadly not a power of two). Maybe changing that default is actually better than having to redo the testing for all the different devices as we're discussing in the context of this patch. Maybe I should just send a patch to do that... -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
[PATCH] mac80211: Change default tx_sk_pacing_shift to 7
When we did the original tests for the optimal value of sk_pacing_shift, we came up with 6 ms of buffering as the default. Sadly, 6 is not a power of two, so when picking the shift value I erred on the size of less buffering and picked 4 ms instead of 8. This was probably wrong; those 2 ms of extra buffering makes a larger difference than I thought. So, change the default pacing shift to 7, which corresponds to 8 ms of buffering. The point of diminishing returns really kicks in after 8 ms, and so having this as a default should cut down on the need for extensive per-device testing and overrides needed in the drivers. Signed-off-by: Toke Høiland-Jørgensen --- net/mac80211/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 5055aeba5c5a..800e67615e2a 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -617,13 +617,13 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len, * We need a bit of data queued to build aggregates properly, so * instruct the TCP stack to allow more than a single ms of data * to be queued in the stack. The value is a bit-shift of 1 -* second, so 8 is ~4ms of queued data. Only affects local TCP +* second, so 7 is ~8ms of queued data. Only affects local TCP * sockets. * This is the default, anyhow - drivers may need to override it * for local reasons (longer buffers, longer completion time, or * similar). */ - local->hw.tx_sk_pacing_shift = 8; + local->hw.tx_sk_pacing_shift = 7; /* set up some defaults */ local->hw.queues = 1; -- 2.20.1 ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 2/2] ath10k: Set sk_pacing_shift to 6 for 11AC WiFi chips
Ben Greear writes: > On 2/21/19 9:15 AM, Toke Høiland-Jørgensen wrote: >> Ben Greear writes: >> >>> On 2/21/19 8:37 AM, Toke Høiland-Jørgensen wrote: >>>> Ben Greear writes: >>>> >>>>> On 2/21/19 8:10 AM, Kalle Valo wrote: >>>>>> Toke Høiland-Jørgensen writes: >>>>>> >>>>>>> Grant Grundler writes: >>>>>>> >>>>>>>> On Thu, Sep 6, 2018 at 3:18 AM Toke Høiland-Jørgensen >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> Grant Grundler writes: >>>>>>>>> >>>>>>>>>>> And, well, Grant's data is from a single test in a noisy >>>>>>>>>>> environment where the time series graph shows that throughput is >>>>>>>>>>> all over >>>>>>>>>>> the place for the duration of the test; so it's hard to draw solid >>>>>>>>>>> conclusions from (for instance, for the 5-stream test, the average >>>>>>>>>>> throughput for 6 is 331 and 379 Mbps for the two repetitions, and >>>>>>>>>>> for 7 >>>>>>>>>>> it's 326 and 371 Mbps) . Unfortunately I don't have the same >>>>>>>>>>> hardware >>>>>>>>>>> used in this test, so I can't go verify it myself; so the only >>>>>>>>>>> thing I >>>>>>>>>>> can do is grumble about it here... :) >>>>>>>>>> >>>>>>>>>> It's a fair complaint and I agree with it. My counter argument is the >>>>>>>>>> opposite is true too: most ideal benchmarks don't measure what most >>>>>>>>>> users see. While the data wgong provided are way more noisy than I >>>>>>>>>> like, my overall "confidence" in the "conclusion" I offered is still >>>>>>>>>> positive. >>>>>>>>> >>>>>>>>> Right. I guess I would just prefer a slightly more comprehensive >>>>>>>>> evaluation to base a 4x increase in buffer size on... >>>>>>>> >>>>>>>> Kalle, is this why you didn't accept this patch? Other reasons? >>>>>>>> >>>>>>>> Toke, what else would you like to see evaluated? >>>>>>>> >>>>>>>> I generally want to see three things measured when "benchmarking" >>>>>>>> technologies: throughput, latency, cpu utilization >>>>>>>> We've covered those three I think "reasonably". >>>>>>> >>>>>>> Hmm, going back and looking at this (I'd completely forgotten about this >>>>>>> patch), I think I had two main concerns: >>>>>>> >>>>>>> 1. What happens in a degraded signal situation, where the throughput is >>>>>>> limited by the signal conditions, or by contention with other >>>>>>> devices. >>>>>>> Both of these happen regularly, and I worry that latency will be >>>>>>> badly affected under those conditions. >>>>>>> >>>>>>> 2. What happens with old hardware that has worse buffer management in >>>>>>> the driver->firmware path (especially drivers without push/pull >>>>>>> mode >>>>>>> support)? For these, the lower-level queueing structure is less >>>>>>> effective at controlling queueing latency. >>>>>> >>>>>> Do note that this patch changes behaviour _only_ for QCA6174 and QCA9377 >>>>>> PCI devices, which IIRC do not even support push/pull mode. All the >>>>>> rest, including QCA988X and QCA9984 are unaffected. >>>>> >>>>> Just as a note, at least kernels such as 4.14.whatever perform poorly when >>>>> running ath10k on 9984 when acting as TCP endpoints. This makes them not >>>>> really usable for stuff like serving video to lots of clients. >>>>> >>>>> Tweaking TCP (I do it a bit differently, but either way) can significantly >>>>> improve perform
Re: [PATCH] mac80211: Change default tx_sk_pacing_shift to 7
Johannes Berg writes: > Toke Høiland-Jørgensen wrote: > >> When we did the original tests for the optimal value of sk_pacing_shift, we >> came up with 6 ms of buffering as the default. Sadly, 6 is not a power of >> two, so when picking the shift value I erred on the size of less buffering >> and picked 4 ms instead of 8. This was probably wrong; those 2 ms of extra >> buffering makes a larger difference than I thought. >> >> So, change the default pacing shift to 7, which corresponds to 8 ms of >> buffering. The point of diminishing returns really kicks in after 8 ms, and >> so having this as a default should cut down on the need for extensive >> per-device testing and overrides needed in the drivers. >> >> Signed-off-by: Toke Høiland-Jørgensen > > Patch applied to wireless.git, thanks. > > a41f56b9a17a (HEAD -> mac80211) mac80211: Change default > tx_sk_pacing_shift to 7 Cool, thanks! What's the easiest way to backport this? I figure it's easier to just update sk_pacing_shift_update() in tx.c for 4.19 (which predates the addition of the driver override hook); shall I just send a separate patch to stable for that? Or do we need to backport the driver override hook as well? -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH] mac80211: Change default tx_sk_pacing_shift to 7
Johannes Berg writes: > On Fri, 2019-02-22 at 14:06 +0100, Toke Høiland-Jørgensen wrote: >> Johannes Berg writes: >> >> > Toke Høiland-Jørgensen wrote: >> > >> > > When we did the original tests for the optimal value of sk_pacing_shift, >> > > we >> > > came up with 6 ms of buffering as the default. Sadly, 6 is not a power of >> > > two, so when picking the shift value I erred on the size of less >> > > buffering >> > > and picked 4 ms instead of 8. This was probably wrong; those 2 ms of >> > > extra >> > > buffering makes a larger difference than I thought. >> > > >> > > So, change the default pacing shift to 7, which corresponds to 8 ms of >> > > buffering. The point of diminishing returns really kicks in after 8 ms, >> > > and >> > > so having this as a default should cut down on the need for extensive >> > > per-device testing and overrides needed in the drivers. >> > > >> > > Signed-off-by: Toke Høiland-Jørgensen >> > >> > Patch applied to wireless.git, thanks. >> > >> > a41f56b9a17a (HEAD -> mac80211) mac80211: Change default >> > tx_sk_pacing_shift to 7 > > This mess came from Kalle's tool btw, so I can't really use it yet :-) > >> Cool, thanks! What's the easiest way to backport this? I figure it's >> easier to just update sk_pacing_shift_update() in tx.c for 4.19 (which >> predates the addition of the driver override hook); shall I just send a >> separate patch to stable for that? Or do we need to backport the driver >> override hook as well? > > Just update the value, no need to backport the hook. And send it directly to stable, or does it need to go through you? -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH] mac80211: Change default tx_sk_pacing_shift to 7
Johannes Berg writes: > On Fri, 2019-02-22 at 14:40 +0100, Toke Høiland-Jørgensen wrote: >> >> And send it directly to stable, or does it need to go through you? > > I added a Cc: stable tag. So all you really need to do is wait for the > emails telling you it failed to apply, and handle accordingly :) Cool, that I can do; thanks! :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH] ath10k: remove iteration in wake_tx_queue
Erik Stromdahl writes: > Iterating the TX queue and thereby dequeuing all available packets in the > queue could result in performance penalties on some SMP systems. > > The reason for this is most likely that the per-ac lock (active_txq_lock) > in mac80211 will be held by the CPU iterating the current queue. > > This will lock up other CPUs trying to push new messages on the TX > queue. > > Instead of iterating the queue we fetch just one packet at the time, > resulting in minimal starvation of the other CPUs. Did you test this with Felix' patches reducing the time the lock is held in mac80211? -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH] mac80211: Fix kernel panic due to use of txq after free
Bhagavathi Perumal S writes:
> The txq of vif is added to active_txqs list for ATF TXQ scheduling
> in the function ieee80211_queue_skb(), but it was not properly removed
> before freeing the txq object. It was causing use after free of the txq
> objects from the active_txqs list, result was kernel panic
> due to invalid memory access.
>
> Fix kernel invalid memory access by properly removing txq object
> from active_txqs list before free the object.
>
> Signed-off-by: Bhagavathi Perumal S
Nice catch, thanks!
Acked-by: Toke Høiland-Jørgensen
This should probably have a fixes tag:
Fixes: 1866760096bf ("mac80211: Add TXQ scheduling API")
-Toke
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH] ath10k: remove iteration in wake_tx_queue
Erik Stromdahl writes: > On 4/1/19 1:05 PM, Toke Høiland-Jørgensen wrote: >> Erik Stromdahl writes: >> >>> Iterating the TX queue and thereby dequeuing all available packets in the >>> queue could result in performance penalties on some SMP systems. >>> >>> The reason for this is most likely that the per-ac lock (active_txq_lock) >>> in mac80211 will be held by the CPU iterating the current queue. >>> >>> This will lock up other CPUs trying to push new messages on the TX >>> queue. >>> >>> Instead of iterating the queue we fetch just one packet at the time, >>> resulting in minimal starvation of the other CPUs. >> >> Did you test this with Felix' patches reducing the time the lock is held >> in mac80211? >> >> -Toke >> > Hi Toke, > > I am not aware of these patches. Can you please point them out for me? They've already been merged. Commits dcec1d9bc8a7 and 7ef769459f14 in mac80211-next :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2] ath10k: fix different tx duration output
Lei Wang writes: > TX duration output of tx_stats in debugfs and station dump had big > difference because they got tx duration value from different statistic > data. We should use the same statistic data. So are you sure you picked the most accurate one of the two? :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2] ath10k: fix different tx duration output
le...@codeaurora.org writes: > On 2019-04-17 17:26, Toke Høiland-Jørgensen wrote: >> Lei Wang writes: >> >>> TX duration output of tx_stats in debugfs and station dump had big >>> difference because they got tx duration value from different statistic >>> data. We should use the same statistic data. >> >> So are you sure you picked the most accurate one of the two? :) >> >> -Toke > > Hi Toke, > > Yes. > Now for ath10k, there are two ways to get tx duration output. > One is got from tx_stats in debugfs reported by firmware. It is a total > value including all the frames which created by host and firmware sent > to the peer. > And the second is calculated from > ath10k_htt_rx_tx_compl_ind()-->ieee80211_sta_register_airtime(), here > the tx duration just includes the data frames sent from host to the > peer. So the difference is that the former includes control frames as well? Is that the only difference? And what exactly is a "big difference" (from the commit message)? > So the first value is preferable for station dump. Hmm, I'm not sure if I agree with this. I specifically added the tx_duration to the station dump to be able to get the values used by the airtime scheduler. This breaks with this patch. -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2] ath10k: fix different tx duration output
le...@codeaurora.org writes: > On 2019-04-18 16:07, Toke Høiland-Jørgensen wrote: >> le...@codeaurora.org writes: >> >>> On 2019-04-17 17:26, Toke Høiland-Jørgensen wrote: >>>> Lei Wang writes: >>>> >>>>> TX duration output of tx_stats in debugfs and station dump had big >>>>> difference because they got tx duration value from different >>>>> statistic >>>>> data. We should use the same statistic data. >>>> >>>> So are you sure you picked the most accurate one of the two? :) >>>> >>>> -Toke >>> >>> Hi Toke, >>> >>> Yes. >>> Now for ath10k, there are two ways to get tx duration output. >>> One is got from tx_stats in debugfs reported by firmware. It is a >>> total >>> value including all the frames which created by host and firmware sent >>> to the peer. >>> And the second is calculated from >>> ath10k_htt_rx_tx_compl_ind()-->ieee80211_sta_register_airtime(), here >>> the tx duration just includes the data frames sent from host to the >>> peer. >> >> So the difference is that the former includes control frames as well? >> Is >> that the only difference? And what exactly is a "big difference" (from >> the commit message)? >> > Yes,it adds the duration time of receiving ACK frames. > From my test,TX from AP to station with iperf UDP test in > 10s,tx_stats->tx_duration:5496623us, > and another value is 3934327us. Hmm, that's quite a big difference. Is this really only ACKs, or is it also a question of whether retries are accounted? If so, it may actually be that what we should do is change which value is passed to ieee80211_sta_register_airtime()? >>> So the first value is preferable for station dump. >> >> Hmm, I'm not sure if I agree with this. I specifically added the >> tx_duration to the station dump to be able to get the values used by >> the >> airtime scheduler. This breaks with this patch. >> >> -Toke > From our internal discussing, we will revert this change. Cool, but see above :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 2/7] ath10k: change max RX bundle size from 8 to 32 for sdio
Wen Gong writes: > The max bundle size support by firmware is 32, change it from 8 to 32 > will help performance. This results in significant performance > improvement on RX path. What happens when the hardware doesn't have enough data to fill a bundle? Does it send a smaller one, or does it wait until it can fill it? -Toke
Re: [PATCH 4/7] ath10k: disable TX complete indication of htt for sdio
Wen Gong writes: > Tx complete message from firmware cost bus bandwidth of sdio, and bus > bandwidth is the bollteneck of throughput, it will effect the bandwidth > occupancy of data packet of TX and RX. > > This patch disable TX complete indication from firmware for htt data > packet, it results in significant performance improvement on TX path. Wait, how does that work? Am I understanding it correctly that this replaces a per-packet TX completion with a periodic one sent out of band? And could you explain what the credits thing is for, please? :) -Toke
RE: [PATCH 2/7] ath10k: change max RX bundle size from 8 to 32 for sdio
Wen Gong writes: >> -Original Message- >> From: ath10k On Behalf Of Toke >> Høiland-Jørgensen >> Sent: Tuesday, August 20, 2019 8:23 PM >> To: Wen Gong ; ath10k@lists.infradead.org >> Cc: linux-wirel...@vger.kernel.org >> Subject: [EXT] Re: [PATCH 2/7] ath10k: change max RX bundle size from 8 to >> 32 for sdio >> >> Wen Gong writes: >> >> > The max bundle size support by firmware is 32, change it from 8 to 32 >> > will help performance. This results in significant performance >> > improvement on RX path. >> >> What happens when the hardware doesn't have enough data to fill a >> bundle? Does it send a smaller one, or does it wait until it can fill >> it? >> > > The bundle is filled by firmware. > It will not wait until it can fill it. > For example, if you do ping per second, it will have only 1 ping packet > In the bundle. Right, cool. -Toke
RE: [PATCH 4/7] ath10k: disable TX complete indication of htt for sdio
Wen Gong writes: >> -Original Message- >> From: ath10k On Behalf Of Toke >> Høiland-Jørgensen >> Sent: Tuesday, August 20, 2019 8:24 PM >> To: Wen Gong ; ath10k@lists.infradead.org >> Cc: linux-wirel...@vger.kernel.org >> Subject: [EXT] Re: [PATCH 4/7] ath10k: disable TX complete indication of htt >> for sdio >> >> Wen Gong writes: >> >> > Tx complete message from firmware cost bus bandwidth of sdio, and bus >> > bandwidth is the bollteneck of throughput, it will effect the bandwidth >> > occupancy of data packet of TX and RX. >> > >> > This patch disable TX complete indication from firmware for htt data >> > packet, it results in significant performance improvement on TX path. >> >> Wait, how does that work? Am I understanding it correctly that this >> replaces a per-packet TX completion with a periodic one sent out of >> band? > When this patch applied, firmware will not indicate tx complete for tx > Data, it only indicate HTT_T2H_MSG_TYPE_TX_CREDIT_UPDATE_IND, > This htt msg will tell how many data tx complete without status(status maybe > success/fail). Ah, so this is basically a counter of how much data is currently queued in the firmware? >> And could you explain what the credits thing is for, please? :) > For high latency bus chip, all the tx data's content(include ip/udp/tcp header > and payload) will be transfer to firmware's memory via bus. > And firmware has limited memory for tx data, the tx data's content must > Saved in firmware memory before it tx complete, if ath10k transfer tx > data more than the limit, firmware will occur error. The credit is used > to avoid ath10k exceed the limit. What's a typical limit in the firmware? -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
RE: [PATCH 4/7] ath10k: disable TX complete indication of htt for sdio
Wen Gong writes: >> -Original Message- >> From: Toke Høiland-Jørgensen >> Sent: Wednesday, August 21, 2019 6:10 PM >> To: Wen Gong ; Wen Gong >> ; ath10k@lists.infradead.org >> Cc: linux-wirel...@vger.kernel.org >> Subject: [EXT] RE: [PATCH 4/7] ath10k: disable TX complete indication of htt >> for sdio >> >> Wen Gong writes: >> >> >> -Original Message- >> >> From: ath10k On Behalf Of Toke >> >> Høiland-Jørgensen >> >> Sent: Tuesday, August 20, 2019 8:24 PM >> >> To: Wen Gong ; ath10k@lists.infradead.org >> >> Cc: linux-wirel...@vger.kernel.org >> >> Subject: [EXT] Re: [PATCH 4/7] ath10k: disable TX complete indication of >> htt >> > When this patch applied, firmware will not indicate tx complete for tx >> > Data, it only indicate HTT_T2H_MSG_TYPE_TX_CREDIT_UPDATE_IND, >> > This htt msg will tell how many data tx complete without status(status >> maybe success/fail). >> >> Ah, so this is basically a counter of how much data is currently queued >> in the firmware? > Yes. >> >> >> And could you explain what the credits thing is for, please? :) >> > For high latency bus chip, all the tx data's content(include ip/udp/tcp >> header >> > and payload) will be transfer to firmware's memory via bus. >> > And firmware has limited memory for tx data, the tx data's content must >> > Saved in firmware memory before it tx complete, if ath10k transfer tx >> > data more than the limit, firmware will occur error. The credit is used >> > to avoid ath10k exceed the limit. >> >> What's a typical limit in the firmware? > It is 96 data packet in my test. It can changed in firmware code. Right, I see. Is this counter available in all ath10k firmware, or is it SDIO only? I'm asking because this could also be a way of implementing something like Byte Queue Limits (though I'm not sure how effective it would be). -Toke
RE: [PATCH 4/7] ath10k: disable TX complete indication of htt for sdio
Wen Gong writes: >> -Original Message- >> From: Toke Høiland-Jørgensen >> Sent: Thursday, August 22, 2019 8:12 PM >> To: Wen Gong ; Wen Gong >> ; ath10k@lists.infradead.org >> Cc: linux-wirel...@vger.kernel.org >> Subject: [EXT] RE: [PATCH 4/7] ath10k: disable TX complete indication of htt >> for sdio >> >> What's a typical limit in the firmware? >> > It is 96 data packet in my test. It can changed in firmware code. >> >> Right, I see. Is this counter available in all ath10k firmware, or is it >> SDIO only? >> > It is only for SDIO. > For PCIE, it does not have credit limit, firmware memory only need to save > The tx descriptor, not need for palyload. OK, too bad. Thanks for the explanations :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 1/4] mac80211: Switch to a virtual time-based airtime scheduler
Yibo Zhao writes: > From: Toke Høiland-Jørgensen > > This switches the airtime scheduler in mac80211 to use a virtual time-based > scheduler instead of the round-robin scheduler used before. This has a > couple of advantages: Thank you for keeping at this! I'll take a look at the series in detail tomorrow. While you're testing things related to this, I've also prototyped a port of the "airtime queue limit" feature from chromeos into mainline. It's currently in my tree here: https://git.kernel.org/pub/scm/linux/kernel/git/toke/linux.git/log/?h=mac80211-aql-01 If you have time to test it at some point, that would be awesome. I'm planning to submit it as an RFC, but it needs a bit more work first. Also, it's completely untested, but it does compile :) -Toke
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
Yibo Zhao writes:
> In a loop txqs dequeue scenario, if the first txq in the rbtree gets
> removed from rbtree immediately in the ieee80211_return_txq(), the
> loop will break soon in the ieee80211_next_txq() due to schedule_pos
> not leading to the second txq in the rbtree. Thus, defering the
> removal right before the end of this schedule round.
>
> Co-developed-by: Yibo Zhao
> Signed-off-by: Yibo Zhao
> Signed-off-by: Toke Høiland-Jørgensen
I didn't write this patch, so please don't use my sign-off. I'll add
ack or review tags as appropriate in reply; but a few comments first:
> ---
> include/net/mac80211.h | 16 ++--
> net/mac80211/ieee80211_i.h | 3 +++
> net/mac80211/main.c| 6 +
> net/mac80211/tx.c | 63
> +++---
> 4 files changed, 83 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index ac2ed8e..ba5a345 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -925,6 +925,8 @@ struct ieee80211_tx_rate {
>
> #define IEEE80211_MAX_TX_RETRY 31
>
> +#define IEEE80211_AIRTIME_TXQ_RM_CHK_INTV_IN_MS 100
> +
> static inline void ieee80211_rate_set_vht(struct ieee80211_tx_rate *rate,
> u8 mcs, u8 nss)
> {
> @@ -6232,7 +6234,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct
> ieee80211_hw *hw,
> * @ac: AC number to return packets from.
> *
> * Should only be called between calls to ieee80211_txq_schedule_start()
> - * and ieee80211_txq_schedule_end().
> + * and ieee80211_txq_schedule_end(). If the txq is empty, it will be added
> + * to a remove list and get removed later.
> * Returns the next txq if successful, %NULL if no queue is eligible. If a
> txq
> * is returned, it should be returned with ieee80211_return_txq() after the
> * driver has finished scheduling it.
> @@ -6268,7 +6271,8 @@ void ieee80211_txq_schedule_start(struct ieee80211_hw
> *hw, u8 ac)
> * @hw: pointer as obtained from ieee80211_alloc_hw()
> * @ac: AC number to acquire locks for
> *
> - * Release locks previously acquired by ieee80211_txq_schedule_end().
> + * Release locks previously acquired by ieee80211_txq_schedule_end(). Check
> + * and remove the empty txq from rb-tree.
> */
> void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac)
> __releases(txq_lock);
> @@ -6287,6 +6291,14 @@ void ieee80211_schedule_txq(struct ieee80211_hw *hw,
> struct ieee80211_txq *txq)
> __acquires(txq_lock) __releases(txq_lock);
>
> /**
> + * ieee80211_txqs_check - Check txqs waiting for removal
> + *
> + * @tmr: pointer as obtained from local
> + *
> + */
> +void ieee80211_txqs_check(struct timer_list *tmr);
> +
> +/**
> * ieee80211_txq_may_transmit - check whether TXQ is allowed to transmit
> *
> * This function is used to check whether given txq is allowed to transmit by
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index a4556f9..49aa143e 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -847,6 +847,7 @@ struct txq_info {
> struct codel_stats cstats;
> struct sk_buff_head frags;
> struct rb_node schedule_order;
> + struct list_head candidate;
> unsigned long flags;
>
> /* keep last! */
> @@ -1145,6 +1146,8 @@ struct ieee80211_local {
> u64 airtime_v_t[IEEE80211_NUM_ACS];
> u64 airtime_weight_sum[IEEE80211_NUM_ACS];
>
> + struct list_head remove_list[IEEE80211_NUM_ACS];
> + struct timer_list remove_timer;
> u16 airtime_flags;
>
> const struct ieee80211_ops *ops;
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index e9ffa8e..78fe24a 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -667,10 +667,15 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t
> priv_data_len,
>
> for (i = 0; i < IEEE80211_NUM_ACS; i++) {
> local->active_txqs[i] = RB_ROOT_CACHED;
> + INIT_LIST_HEAD(&local->remove_list[i]);
> spin_lock_init(&local->active_txq_lock[i]);
> }
> local->airtime_flags = AIRTIME_USE_TX | AIRTIME_USE_RX;
>
> + timer_setup(&local->remove_timer, ieee80211_txqs_check, 0);
> + mod_timer(&local->remove_timer,
> + jiffies +
> msecs_to_jiffies(IEEE80211_AIRTIME_TXQ_RM_CHK_INTV_IN_MS));
> +
> INIT_LIST_HEAD(&local->chanctx_list);
> mutex_init(&local->chanctx_mtx);
>
> @@ -1305,6 +1310,7 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
>
Re: [PATCH 3/4] mac80211: fix low throughput in push pull mode
Yibo Zhao writes: > On 2019-09-16 23:27, Johannes Berg wrote: >> Without really looking at the code - >> >>> If station is ineligible for transmission in >>> ieee80211_txq_may_transmit(), >>> no packet will be delivered to FW. During the tests in push-pull mode >>> with >>> many clients, after several seconds, not a single station is an >>> eligible >>> candidate for transmission since global time is smaller than all the >>> station's virtual airtime. As a consequence, the Tx has been blocked >>> and >>> throughput is quite low. >> >> You should rewrite this to be, erm, a bit more understandable in >> mac80211 context. I assume you're speaking (mostly?) about ath10k, but >> I >> have very little context there. "push pull mode"? "firmware"? These >> things are not something mac80211 knows about. > Hi Johannes, > > Thanks for your kindly reminder. Will rewrite the commit log. > >> >>> Co-developed-by: Yibo Zhao >> >> That also seems wrong, should be Toke I guess, unless you intended for >> a >> From: Toke to be present? > Do you mean it should be something like: > > Co-developed-by: Toke Høiland-Jørgensen > Signed-off-by: Yibo Zhao > Signed-off-by: Toke Høiland-Jørgensen > > Am I understanding right? I think the right thing here, as with the previous patch, is to just drop my sign-off; you're writing this patch, and I'll add ack/reviews as appropriate. And in that case, well, no need to have co-developed-by yourself when your name is on the patch as author :) -Toke
Re: [PATCH 4/4] mac80211: Sync airtime weight sum with per AC synced sta airtime weight together
Yibo Zhao writes:
> Global airtime weight sum is updated only when txq is added/removed
> from rbtree. If upper layer configures sta weight during high load,
> airtime weight sum will not be updated since txq is most likely on the
> tree. It could a little late for upper layer to reconfigure sta weight
> when txq is already in the rbtree. And thus, incorrect airtime weight sum
> will lead to incorrect global virtual time calculation as well as global
> airtime weight sum overflow of airtime weight sum during txq removed.
>
> Hence, need to update airtime weight sum upon receiving event for
> configuring sta weight once sta's txq is on the rbtree.
>
> Besides, if airtime weight sum of ACs and sta weight is synced under the
> same per AC lock protection, there can be a very short window causing
> incorrct airtime weight sum calculation as below:
>
> active_txq_lock_VO .
> VO weight sum is syncd.
> sta airtime weight sum is synced .
> active_txq_unlock_VO .
> . .
> active_txq_lock_VI.
> VI weight sum is syncd.
> sta airtime weight sumactive_txq_lock_BE
> active_txq_unlock_VIRemove txq and thus sum
> . is calculated with synced
> . sta airtime weight
> . active_txq_unlock_BE
>
> So introduce a per ac synced station airtime weight synced with per
> AC synced weight sum together. And the per-AC station airtime weight
> is used to calculate weight sum.
>
> Signed-off-by: Yibo Zhao
> ---
> net/mac80211/cfg.c | 27 +--
> net/mac80211/sta_info.c | 6 --
> net/mac80211/sta_info.h | 3 +++
> net/mac80211/tx.c | 4 ++--
> 4 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index d65aa01..4b420bb 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -1284,7 +1284,8 @@ static int sta_apply_parameters(struct ieee80211_local
> *local,
> int ret = 0;
> struct ieee80211_supported_band *sband;
> struct ieee80211_sub_if_data *sdata = sta->sdata;
> - u32 mask, set;
> + u32 mask, set, tid, ac;
> + struct txq_info *txqi;
>
> sband = ieee80211_get_sband(sdata);
> if (!sband)
> @@ -1452,8 +1453,30 @@ static int sta_apply_parameters(struct ieee80211_local
> *local,
> if (ieee80211_vif_is_mesh(&sdata->vif))
> sta_apply_mesh_params(local, sta, params);
>
> - if (params->airtime_weight)
> + if (params->airtime_weight &&
> + params->airtime_weight != sta->airtime_weight) {
> + for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
> + spin_lock_bh(&local->active_txq_lock[ac]);
> + for (tid = 0; tid < IEEE80211_NUM_TIDS + 1; tid++) {
> + if (!sta->sta.txq[tid] ||
> + ac != ieee80211_ac_from_tid(tid))
> + continue;
> +
> + sta->airtime_weight_synced[ac] =
> + params->airtime_weight;
> +
> + txqi = to_txq_info(sta->sta.txq[tid]);
> + if (RB_EMPTY_NODE(&txqi->schedule_order))
> + continue;
> +
> + local->airtime_weight_sum[ac] =
> local->airtime_weight_sum[ac] +
> +
> params->airtime_weight -
> +
> sta->airtime_weight;
> + }
> + spin_unlock_bh(&local->active_txq_lock[ac]);
> + }
> sta->airtime_weight = params->airtime_weight;
With this, airtime_weight is basically only used to return to and from
userspace, right? I.e., after the above loop has run, it will match the
contents of airtime_weight_synced; so why not just turn airtime_weight
into a per-ac array? You could just use airtime_weight[0] as the value
to return to userspace...
-Toke
Re: [PATCH 1/4] mac80211: Switch to a virtual time-based airtime scheduler
Yibo Zhao writes: > From: Toke Høiland-Jørgensen > > This switches the airtime scheduler in mac80211 to use a virtual time-based > scheduler instead of the round-robin scheduler used before. This has a > couple of advantages: > > - No need to sync up the round-robin scheduler in firmware/hardware with > the round-robin airtime scheduler. > > - If several stations are eligible for transmission we can schedule both of > them; no need to hard-block the scheduling rotation until the head of the > queue has used up its quantum. > > - The check of whether a station is eligible for transmission becomes > simpler (in ieee80211_txq_may_transmit()). > > The drawback is that scheduling becomes slightly more expensive, as we need > to maintain an rbtree of TXQs sorted by virtual time. This means that > ieee80211_register_airtime() becomes O(logN) in the number of currently > scheduled TXQs. However, hopefully this number rarely grows too big (it's > only TXQs currently backlogged, not all associated stations), so it > shouldn't be too big of an issue. > > Signed-off-by: Toke Høiland-Jørgensen I'll note that this patch still has the two issues that Felix pointed out when I posted the RFC version. Namely: - The use of divisions in the fast path. I guess I need to go write some reciprocal-calculation code, since that is also an issue with the AQL patches I linked to before. - The fact that we don't count the airtime usage of multicast traffic, which with this series means that the vif TXQ will get priority over the others. I think we agreed to fix this by just adding an airtime v_t to the vif as well and use that for scheduling the TXQ. Does ath10k report airtime usage for multicast as well, or only for stations? -Toke
Re: [PATCH V2 3/4] mac80211: fix low throughput in multi-clients situation
Yibo Zhao writes: > Not long after the start of multi-clients test, not a single station is > an eligible candidate for transmission since global virtual time(g_vt) is > smaller than the virtual airtime(s_vt) of all the stations. As a result, > the Tx has been blocked and throughput is quite low. > > This may mainly due to sync mechanism and accumulative deviation from the > devision calculation of g_vt. > > For example: > Suppose we have 50 clients in first round. > Round 1: > STA weight Tx_time_round wt_sum s_vtg_vt valid_for_next_Tx > . . . . . > . . . . . > . . . . . > > After this round, all the stations are not valid for next transmission due > to accumulative deviation. > > And if we add a new #51, > Round 2: > STA weight Tx_time_round wt_sum s_vtg_vt valid_for_next_Tx > . . . . . > . . . . . > . . . . . > > Sync is done by: > max(g_vt of last round - grace period, s_vt) > and s_vt of #51 = max(2000 - 500, 0) + 1024 = 2524, and it is more than the > final g_vt of this round. > > After this round, no more station is valid for transmission. I'm not sure I understand this. Was there supposed to be numbers in those tables above? -Toke
Re: [PATCH 3/4] mac80211: fix low throughput in push pull mode
Yibo Zhao writes: > On 2019-09-18 05:12, Toke Høiland-Jørgensen wrote: >> Yibo Zhao writes: >> >>> On 2019-09-16 23:27, Johannes Berg wrote: >>>> Without really looking at the code - >>>> >>>>> If station is ineligible for transmission in >>>>> ieee80211_txq_may_transmit(), >>>>> no packet will be delivered to FW. During the tests in push-pull >>>>> mode >>>>> with >>>>> many clients, after several seconds, not a single station is an >>>>> eligible >>>>> candidate for transmission since global time is smaller than all the >>>>> station's virtual airtime. As a consequence, the Tx has been blocked >>>>> and >>>>> throughput is quite low. >>>> >>>> You should rewrite this to be, erm, a bit more understandable in >>>> mac80211 context. I assume you're speaking (mostly?) about ath10k, >>>> but >>>> I >>>> have very little context there. "push pull mode"? "firmware"? These >>>> things are not something mac80211 knows about. >>> Hi Johannes, >>> >>> Thanks for your kindly reminder. Will rewrite the commit log. >>> >>>> >>>>> Co-developed-by: Yibo Zhao >>>> >>>> That also seems wrong, should be Toke I guess, unless you intended >>>> for >>>> a >>>> From: Toke to be present? >>> Do you mean it should be something like: >>> >>> Co-developed-by: Toke Høiland-Jørgensen >>> Signed-off-by: Yibo Zhao >>> Signed-off-by: Toke Høiland-Jørgensen >>> >>> Am I understanding right? >> >> I think the right thing here, as with the previous patch, is to just >> drop my sign-off; you're writing this patch, and I'll add ack/reviews >> as >> appropriate. And in that case, well, no need to have co-developed-by >> yourself when your name is on the patch as author :) >> >> -Toke > Sorry, I think I have missed checking your reply, please ignore the > wrong signed-off in PATCH-V2. While you're re-spinning, could you please add a changelog for the changes you make? Makes it easier to keep track :) You can add a cover-letter with a full changelog instead of having a separate changelog for each patch; that's what I usually do... -Toke
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
Yibo Zhao writes:
> On 2019-09-18 05:10, Toke Høiland-Jørgensen wrote:
>> Yibo Zhao writes:
>>
>>> In a loop txqs dequeue scenario, if the first txq in the rbtree gets
>>> removed from rbtree immediately in the ieee80211_return_txq(), the
>>> loop will break soon in the ieee80211_next_txq() due to schedule_pos
>>> not leading to the second txq in the rbtree. Thus, defering the
>>> removal right before the end of this schedule round.
>>>
>>> Co-developed-by: Yibo Zhao
>>> Signed-off-by: Yibo Zhao
>>> Signed-off-by: Toke Høiland-Jørgensen
>>
>> I didn't write this patch, so please don't use my sign-off. I'll add
>> ack or review tags as appropriate in reply; but a few comments first:
>>
>>> ---
>>> include/net/mac80211.h | 16 ++--
>>> net/mac80211/ieee80211_i.h | 3 +++
>>> net/mac80211/main.c| 6 +
>>> net/mac80211/tx.c | 63
>>> +++---
>>> 4 files changed, 83 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>>> index ac2ed8e..ba5a345 100644
>>> --- a/include/net/mac80211.h
>>> +++ b/include/net/mac80211.h
>>> @@ -925,6 +925,8 @@ struct ieee80211_tx_rate {
>>>
>>> #define IEEE80211_MAX_TX_RETRY 31
>>>
>>> +#define IEEE80211_AIRTIME_TXQ_RM_CHK_INTV_IN_MS 100
>>> +
>>> static inline void ieee80211_rate_set_vht(struct ieee80211_tx_rate
>>> *rate,
>>> u8 mcs, u8 nss)
>>> {
>>> @@ -6232,7 +6234,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct
>>> ieee80211_hw *hw,
>>> * @ac: AC number to return packets from.
>>> *
>>> * Should only be called between calls to
>>> ieee80211_txq_schedule_start()
>>> - * and ieee80211_txq_schedule_end().
>>> + * and ieee80211_txq_schedule_end(). If the txq is empty, it will be
>>> added
>>> + * to a remove list and get removed later.
>>> * Returns the next txq if successful, %NULL if no queue is eligible.
>>> If a txq
>>> * is returned, it should be returned with ieee80211_return_txq()
>>> after the
>>> * driver has finished scheduling it.
>>> @@ -6268,7 +6271,8 @@ void ieee80211_txq_schedule_start(struct
>>> ieee80211_hw *hw, u8 ac)
>>> * @hw: pointer as obtained from ieee80211_alloc_hw()
>>> * @ac: AC number to acquire locks for
>>> *
>>> - * Release locks previously acquired by ieee80211_txq_schedule_end().
>>> + * Release locks previously acquired by ieee80211_txq_schedule_end().
>>> Check
>>> + * and remove the empty txq from rb-tree.
>>> */
>>> void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac)
>>> __releases(txq_lock);
>>> @@ -6287,6 +6291,14 @@ void ieee80211_schedule_txq(struct ieee80211_hw
>>> *hw, struct ieee80211_txq *txq)
>>> __acquires(txq_lock) __releases(txq_lock);
>>>
>>> /**
>>> + * ieee80211_txqs_check - Check txqs waiting for removal
>>> + *
>>> + * @tmr: pointer as obtained from local
>>> + *
>>> + */
>>> +void ieee80211_txqs_check(struct timer_list *tmr);
>>> +
>>> +/**
>>> * ieee80211_txq_may_transmit - check whether TXQ is allowed to
>>> transmit
>>> *
>>> * This function is used to check whether given txq is allowed to
>>> transmit by
>>> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
>>> index a4556f9..49aa143e 100644
>>> --- a/net/mac80211/ieee80211_i.h
>>> +++ b/net/mac80211/ieee80211_i.h
>>> @@ -847,6 +847,7 @@ struct txq_info {
>>> struct codel_stats cstats;
>>> struct sk_buff_head frags;
>>> struct rb_node schedule_order;
>>> + struct list_head candidate;
>>> unsigned long flags;
>>>
>>> /* keep last! */
>>> @@ -1145,6 +1146,8 @@ struct ieee80211_local {
>>> u64 airtime_v_t[IEEE80211_NUM_ACS];
>>> u64 airtime_weight_sum[IEEE80211_NUM_ACS];
>>>
>>> + struct list_head remove_list[IEEE80211_NUM_ACS];
>>> + struct timer_list remove_timer;
>>> u16 airtime_flags;
>>>
>>> const struct ieee80211_ops *ops;
>>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>>> index e9ffa8e..78fe24a 100644
>>> ---
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
Yibo Zhao writes:
> On 2019-09-18 19:23, Toke Høiland-Jørgensen wrote:
>> Yibo Zhao writes:
>>
>>> On 2019-09-18 05:10, Toke Høiland-Jørgensen wrote:
>>>> Yibo Zhao writes:
>>>>
>>>>> In a loop txqs dequeue scenario, if the first txq in the rbtree gets
>>>>> removed from rbtree immediately in the ieee80211_return_txq(), the
>>>>> loop will break soon in the ieee80211_next_txq() due to schedule_pos
>>>>> not leading to the second txq in the rbtree. Thus, defering the
>>>>> removal right before the end of this schedule round.
>>>>>
>>>>> Co-developed-by: Yibo Zhao
>>>>> Signed-off-by: Yibo Zhao
>>>>> Signed-off-by: Toke Høiland-Jørgensen
>>>>
>>>> I didn't write this patch, so please don't use my sign-off. I'll add
>>>> ack or review tags as appropriate in reply; but a few comments first:
>>>>
>>>>> ---
>>>>> include/net/mac80211.h | 16 ++--
>>>>> net/mac80211/ieee80211_i.h | 3 +++
>>>>> net/mac80211/main.c| 6 +
>>>>> net/mac80211/tx.c | 63
>>>>> +++---
>>>>> 4 files changed, 83 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>>>>> index ac2ed8e..ba5a345 100644
>>>>> --- a/include/net/mac80211.h
>>>>> +++ b/include/net/mac80211.h
>>>>> @@ -925,6 +925,8 @@ struct ieee80211_tx_rate {
>>>>>
>>>>> #define IEEE80211_MAX_TX_RETRY 31
>>>>>
>>>>> +#define IEEE80211_AIRTIME_TXQ_RM_CHK_INTV_IN_MS 100
>>>>> +
>>>>> static inline void ieee80211_rate_set_vht(struct ieee80211_tx_rate
>>>>> *rate,
>>>>> u8 mcs, u8 nss)
>>>>> {
>>>>> @@ -6232,7 +6234,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct
>>>>> ieee80211_hw *hw,
>>>>> * @ac: AC number to return packets from.
>>>>> *
>>>>> * Should only be called between calls to
>>>>> ieee80211_txq_schedule_start()
>>>>> - * and ieee80211_txq_schedule_end().
>>>>> + * and ieee80211_txq_schedule_end(). If the txq is empty, it will
>>>>> be
>>>>> added
>>>>> + * to a remove list and get removed later.
>>>>> * Returns the next txq if successful, %NULL if no queue is
>>>>> eligible.
>>>>> If a txq
>>>>> * is returned, it should be returned with ieee80211_return_txq()
>>>>> after the
>>>>> * driver has finished scheduling it.
>>>>> @@ -6268,7 +6271,8 @@ void ieee80211_txq_schedule_start(struct
>>>>> ieee80211_hw *hw, u8 ac)
>>>>> * @hw: pointer as obtained from ieee80211_alloc_hw()
>>>>> * @ac: AC number to acquire locks for
>>>>> *
>>>>> - * Release locks previously acquired by
>>>>> ieee80211_txq_schedule_end().
>>>>> + * Release locks previously acquired by
>>>>> ieee80211_txq_schedule_end().
>>>>> Check
>>>>> + * and remove the empty txq from rb-tree.
>>>>> */
>>>>> void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac)
>>>>> __releases(txq_lock);
>>>>> @@ -6287,6 +6291,14 @@ void ieee80211_schedule_txq(struct
>>>>> ieee80211_hw
>>>>> *hw, struct ieee80211_txq *txq)
>>>>> __acquires(txq_lock) __releases(txq_lock);
>>>>>
>>>>> /**
>>>>> + * ieee80211_txqs_check - Check txqs waiting for removal
>>>>> + *
>>>>> + * @tmr: pointer as obtained from local
>>>>> + *
>>>>> + */
>>>>> +void ieee80211_txqs_check(struct timer_list *tmr);
>>>>> +
>>>>> +/**
>>>>> * ieee80211_txq_may_transmit - check whether TXQ is allowed to
>>>>> transmit
>>>>> *
>>>>> * This function is used to check whether given txq is allowed to
>>>>> transmit by
>>>>> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
>>>>> index a4556f9..49aa143e 100644
>>>>> --- a/net/mac80211/ieee80211_i.h
>>>&
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
Yibo Zhao writes:
> On 2019-09-19 18:37, Toke Høiland-Jørgensen wrote:
>> Yibo Zhao writes:
>>
>>> On 2019-09-18 19:23, Toke Høiland-Jørgensen wrote:
>>>> Yibo Zhao writes:
>>>>
>>>>> On 2019-09-18 05:10, Toke Høiland-Jørgensen wrote:
>>>>>> Yibo Zhao writes:
>>>>>>
>>>>>>> In a loop txqs dequeue scenario, if the first txq in the rbtree
>>>>>>> gets
>>>>>>> removed from rbtree immediately in the ieee80211_return_txq(), the
>>>>>>> loop will break soon in the ieee80211_next_txq() due to
>>>>>>> schedule_pos
>>>>>>> not leading to the second txq in the rbtree. Thus, defering the
>>>>>>> removal right before the end of this schedule round.
>>>>>>>
>>>>>>> Co-developed-by: Yibo Zhao
>>>>>>> Signed-off-by: Yibo Zhao
>>>>>>> Signed-off-by: Toke Høiland-Jørgensen
>>>>>>
>>>>>> I didn't write this patch, so please don't use my sign-off. I'll
>>>>>> add
>>>>>> ack or review tags as appropriate in reply; but a few comments
>>>>>> first:
>>>>>>
>>>>>>> ---
>>>>>>> include/net/mac80211.h | 16 ++--
>>>>>>> net/mac80211/ieee80211_i.h | 3 +++
>>>>>>> net/mac80211/main.c| 6 +
>>>>>>> net/mac80211/tx.c | 63
>>>>>>> +++---
>>>>>>> 4 files changed, 83 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>>>>>>> index ac2ed8e..ba5a345 100644
>>>>>>> --- a/include/net/mac80211.h
>>>>>>> +++ b/include/net/mac80211.h
>>>>>>> @@ -925,6 +925,8 @@ struct ieee80211_tx_rate {
>>>>>>>
>>>>>>> #define IEEE80211_MAX_TX_RETRY 31
>>>>>>>
>>>>>>> +#define IEEE80211_AIRTIME_TXQ_RM_CHK_INTV_IN_MS 100
>>>>>>> +
>>>>>>> static inline void ieee80211_rate_set_vht(struct
>>>>>>> ieee80211_tx_rate
>>>>>>> *rate,
>>>>>>> u8 mcs, u8 nss)
>>>>>>> {
>>>>>>> @@ -6232,7 +6234,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct
>>>>>>> ieee80211_hw *hw,
>>>>>>> * @ac: AC number to return packets from.
>>>>>>> *
>>>>>>> * Should only be called between calls to
>>>>>>> ieee80211_txq_schedule_start()
>>>>>>> - * and ieee80211_txq_schedule_end().
>>>>>>> + * and ieee80211_txq_schedule_end(). If the txq is empty, it will
>>>>>>> be
>>>>>>> added
>>>>>>> + * to a remove list and get removed later.
>>>>>>> * Returns the next txq if successful, %NULL if no queue is
>>>>>>> eligible.
>>>>>>> If a txq
>>>>>>> * is returned, it should be returned with ieee80211_return_txq()
>>>>>>> after the
>>>>>>> * driver has finished scheduling it.
>>>>>>> @@ -6268,7 +6271,8 @@ void ieee80211_txq_schedule_start(struct
>>>>>>> ieee80211_hw *hw, u8 ac)
>>>>>>> * @hw: pointer as obtained from ieee80211_alloc_hw()
>>>>>>> * @ac: AC number to acquire locks for
>>>>>>> *
>>>>>>> - * Release locks previously acquired by
>>>>>>> ieee80211_txq_schedule_end().
>>>>>>> + * Release locks previously acquired by
>>>>>>> ieee80211_txq_schedule_end().
>>>>>>> Check
>>>>>>> + * and remove the empty txq from rb-tree.
>>>>>>> */
>>>>>>> void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac)
>>>>>>> __releases(txq_lock);
>>>>>>> @@ -6287,6 +6291,14 @@ void ieee80211_schedule_txq(struct
>>>>>>> ieee80211_hw
>>>>>>> *hw, struct ieee80211_txq *txq)
>>>>>>> __acquires(txq_lock) __releases(txq_lock);
>&g
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
Yibo Zhao writes:
> On 2019-09-20 17:15, Toke Høiland-Jørgensen wrote:
>> Yibo Zhao writes:
>>
>>> On 2019-09-19 18:37, Toke Høiland-Jørgensen wrote:
>>>> Yibo Zhao writes:
>>>>
>>>>> On 2019-09-18 19:23, Toke Høiland-Jørgensen wrote:
>>>>>> Yibo Zhao writes:
>>>>>>
>>>>>>> On 2019-09-18 05:10, Toke Høiland-Jørgensen wrote:
>>>>>>>> Yibo Zhao writes:
>>>>>>>>
>>>>>>>>> In a loop txqs dequeue scenario, if the first txq in the rbtree
>>>>>>>>> gets
>>>>>>>>> removed from rbtree immediately in the ieee80211_return_txq(),
>>>>>>>>> the
>>>>>>>>> loop will break soon in the ieee80211_next_txq() due to
>>>>>>>>> schedule_pos
>>>>>>>>> not leading to the second txq in the rbtree. Thus, defering the
>>>>>>>>> removal right before the end of this schedule round.
>>>>>>>>>
>>>>>>>>> Co-developed-by: Yibo Zhao
>>>>>>>>> Signed-off-by: Yibo Zhao
>>>>>>>>> Signed-off-by: Toke Høiland-Jørgensen
>>>>>>>>
>>>>>>>> I didn't write this patch, so please don't use my sign-off. I'll
>>>>>>>> add
>>>>>>>> ack or review tags as appropriate in reply; but a few comments
>>>>>>>> first:
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> include/net/mac80211.h | 16 ++--
>>>>>>>>> net/mac80211/ieee80211_i.h | 3 +++
>>>>>>>>> net/mac80211/main.c| 6 +
>>>>>>>>> net/mac80211/tx.c | 63
>>>>>>>>> +++---
>>>>>>>>> 4 files changed, 83 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>>>>>>>>> index ac2ed8e..ba5a345 100644
>>>>>>>>> --- a/include/net/mac80211.h
>>>>>>>>> +++ b/include/net/mac80211.h
>>>>>>>>> @@ -925,6 +925,8 @@ struct ieee80211_tx_rate {
>>>>>>>>>
>>>>>>>>> #define IEEE80211_MAX_TX_RETRY 31
>>>>>>>>>
>>>>>>>>> +#define IEEE80211_AIRTIME_TXQ_RM_CHK_INTV_IN_MS 100
>>>>>>>>> +
>>>>>>>>> static inline void ieee80211_rate_set_vht(struct
>>>>>>>>> ieee80211_tx_rate
>>>>>>>>> *rate,
>>>>>>>>> u8 mcs, u8 nss)
>>>>>>>>> {
>>>>>>>>> @@ -6232,7 +6234,8 @@ struct sk_buff
>>>>>>>>> *ieee80211_tx_dequeue(struct
>>>>>>>>> ieee80211_hw *hw,
>>>>>>>>> * @ac: AC number to return packets from.
>>>>>>>>> *
>>>>>>>>> * Should only be called between calls to
>>>>>>>>> ieee80211_txq_schedule_start()
>>>>>>>>> - * and ieee80211_txq_schedule_end().
>>>>>>>>> + * and ieee80211_txq_schedule_end(). If the txq is empty, it
>>>>>>>>> will
>>>>>>>>> be
>>>>>>>>> added
>>>>>>>>> + * to a remove list and get removed later.
>>>>>>>>> * Returns the next txq if successful, %NULL if no queue is
>>>>>>>>> eligible.
>>>>>>>>> If a txq
>>>>>>>>> * is returned, it should be returned with
>>>>>>>>> ieee80211_return_txq()
>>>>>>>>> after the
>>>>>>>>> * driver has finished scheduling it.
>>>>>>>>> @@ -6268,7 +6271,8 @@ void ieee80211_txq_schedule_start(struct
>>>>>>>>> ieee80211_hw *hw, u8 ac)
>>>>>>>>> * @hw: pointer as obtained from ieee80211_alloc_hw()
>>>>>>>>> * @ac: AC number to acquire locks for
>>>>>>>>> *
>>>>>&
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
Yibo Zhao writes:
> On 2019-09-21 19:27, Toke Høiland-Jørgensen wrote:
>> Yibo Zhao writes:
>>
>>> On 2019-09-20 17:15, Toke Høiland-Jørgensen wrote:
>>>> Yibo Zhao writes:
>>>>
>>>>> On 2019-09-19 18:37, Toke Høiland-Jørgensen wrote:
>>>>>> Yibo Zhao writes:
>>>>>>
>>>>>>> On 2019-09-18 19:23, Toke Høiland-Jørgensen wrote:
>>>>>>>> Yibo Zhao writes:
>>>>>>>>
>>>>>>>>> On 2019-09-18 05:10, Toke Høiland-Jørgensen wrote:
>>>>>>>>>> Yibo Zhao writes:
>>>>>>>>>>
>>>>>>>>>>> In a loop txqs dequeue scenario, if the first txq in the
>>>>>>>>>>> rbtree
>>>>>>>>>>> gets
>>>>>>>>>>> removed from rbtree immediately in the ieee80211_return_txq(),
>>>>>>>>>>> the
>>>>>>>>>>> loop will break soon in the ieee80211_next_txq() due to
>>>>>>>>>>> schedule_pos
>>>>>>>>>>> not leading to the second txq in the rbtree. Thus, defering
>>>>>>>>>>> the
>>>>>>>>>>> removal right before the end of this schedule round.
>>>>>>>>>>>
>>>>>>>>>>> Co-developed-by: Yibo Zhao
>>>>>>>>>>> Signed-off-by: Yibo Zhao
>>>>>>>>>>> Signed-off-by: Toke Høiland-Jørgensen
>>>>>>>>>>
>>>>>>>>>> I didn't write this patch, so please don't use my sign-off.
>>>>>>>>>> I'll
>>>>>>>>>> add
>>>>>>>>>> ack or review tags as appropriate in reply; but a few comments
>>>>>>>>>> first:
>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>> include/net/mac80211.h | 16 ++--
>>>>>>>>>>> net/mac80211/ieee80211_i.h | 3 +++
>>>>>>>>>>> net/mac80211/main.c| 6 +
>>>>>>>>>>> net/mac80211/tx.c | 63
>>>>>>>>>>> +++---
>>>>>>>>>>> 4 files changed, 83 insertions(+), 5 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>>>>>>>>>>> index ac2ed8e..ba5a345 100644
>>>>>>>>>>> --- a/include/net/mac80211.h
>>>>>>>>>>> +++ b/include/net/mac80211.h
>>>>>>>>>>> @@ -925,6 +925,8 @@ struct ieee80211_tx_rate {
>>>>>>>>>>>
>>>>>>>>>>> #define IEEE80211_MAX_TX_RETRY 31
>>>>>>>>>>>
>>>>>>>>>>> +#define IEEE80211_AIRTIME_TXQ_RM_CHK_INTV_IN_MS 100
>>>>>>>>>>> +
>>>>>>>>>>> static inline void ieee80211_rate_set_vht(struct
>>>>>>>>>>> ieee80211_tx_rate
>>>>>>>>>>> *rate,
>>>>>>>>>>> u8 mcs, u8 nss)
>>>>>>>>>>> {
>>>>>>>>>>> @@ -6232,7 +6234,8 @@ struct sk_buff
>>>>>>>>>>> *ieee80211_tx_dequeue(struct
>>>>>>>>>>> ieee80211_hw *hw,
>>>>>>>>>>> * @ac: AC number to return packets from.
>>>>>>>>>>> *
>>>>>>>>>>> * Should only be called between calls to
>>>>>>>>>>> ieee80211_txq_schedule_start()
>>>>>>>>>>> - * and ieee80211_txq_schedule_end().
>>>>>>>>>>> + * and ieee80211_txq_schedule_end(). If the txq is empty, it
>>>>>>>>>>> will
>>>>>>>>>>> be
>>>>>>>>>>> added
>>>>>>>>>>> + * to a remove list and get removed later.
>>>>>>>>>>> * Returns the next txq if successful, %
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
Yibo Zhao writes:
> On 2019-09-21 21:02, Toke Høiland-Jørgensen wrote:
>> Yibo Zhao writes:
>>
>>> On 2019-09-21 19:27, Toke Høiland-Jørgensen wrote:
>>>> Yibo Zhao writes:
>>>>
>>>>> On 2019-09-20 17:15, Toke Høiland-Jørgensen wrote:
>>>>>> Yibo Zhao writes:
>>>>>>
>>>>>>> On 2019-09-19 18:37, Toke Høiland-Jørgensen wrote:
>>>>>>>> Yibo Zhao writes:
>>>>>>>>
>>>>>>>>> On 2019-09-18 19:23, Toke Høiland-Jørgensen wrote:
>>>>>>>>>> Yibo Zhao writes:
>>>>>>>>>>
>>>>>>>>>>> On 2019-09-18 05:10, Toke Høiland-Jørgensen wrote:
>>>>>>>>>>>> Yibo Zhao writes:
>>>>>>>>>>>>
>>>>>>>>>>>>> In a loop txqs dequeue scenario, if the first txq in the
>>>>>>>>>>>>> rbtree
>>>>>>>>>>>>> gets
>>>>>>>>>>>>> removed from rbtree immediately in the
>>>>>>>>>>>>> ieee80211_return_txq(),
>>>>>>>>>>>>> the
>>>>>>>>>>>>> loop will break soon in the ieee80211_next_txq() due to
>>>>>>>>>>>>> schedule_pos
>>>>>>>>>>>>> not leading to the second txq in the rbtree. Thus, defering
>>>>>>>>>>>>> the
>>>>>>>>>>>>> removal right before the end of this schedule round.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Co-developed-by: Yibo Zhao
>>>>>>>>>>>>> Signed-off-by: Yibo Zhao
>>>>>>>>>>>>> Signed-off-by: Toke Høiland-Jørgensen
>>>>>>>>>>>>
>>>>>>>>>>>> I didn't write this patch, so please don't use my sign-off.
>>>>>>>>>>>> I'll
>>>>>>>>>>>> add
>>>>>>>>>>>> ack or review tags as appropriate in reply; but a few
>>>>>>>>>>>> comments
>>>>>>>>>>>> first:
>>>>>>>>>>>>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> include/net/mac80211.h | 16 ++--
>>>>>>>>>>>>> net/mac80211/ieee80211_i.h | 3 +++
>>>>>>>>>>>>> net/mac80211/main.c| 6 +
>>>>>>>>>>>>> net/mac80211/tx.c | 63
>>>>>>>>>>>>> +++---
>>>>>>>>>>>>> 4 files changed, 83 insertions(+), 5 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>>>>>>>>>>>>> index ac2ed8e..ba5a345 100644
>>>>>>>>>>>>> --- a/include/net/mac80211.h
>>>>>>>>>>>>> +++ b/include/net/mac80211.h
>>>>>>>>>>>>> @@ -925,6 +925,8 @@ struct ieee80211_tx_rate {
>>>>>>>>>>>>>
>>>>>>>>>>>>> #define IEEE80211_MAX_TX_RETRY 31
>>>>>>>>>>>>>
>>>>>>>>>>>>> +#define IEEE80211_AIRTIME_TXQ_RM_CHK_INTV_IN_MS 100
>>>>>>>>>>>>> +
>>>>>>>>>>>>> static inline void ieee80211_rate_set_vht(struct
>>>>>>>>>>>>> ieee80211_tx_rate
>>>>>>>>>>>>> *rate,
>>>>>>>>>>>>> u8 mcs, u8 nss)
>>>>>>>>>>>>> {
>>>>>>>>>>>>> @@ -6232,7 +6234,8 @@ struct sk_buff
>>>>>>>>>>>>> *ieee80211_tx_dequeue(struct
>>>>>>>>>>>>> ieee80211_hw *hw,
>>>>>>>>>>>>> * @ac: AC number to return packets from.
>>>>>>>>>>>&g
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
Yibo Zhao writes:
> On 2019-09-21 22:00, Toke Høiland-Jørgensen wrote:
>> Yibo Zhao writes:
>>
>>> On 2019-09-21 21:02, Toke Høiland-Jørgensen wrote:
>>>> Yibo Zhao writes:
>>>>
>>>>> On 2019-09-21 19:27, Toke Høiland-Jørgensen wrote:
>>>>>> Yibo Zhao writes:
>>>>>>
>>>>>>> On 2019-09-20 17:15, Toke Høiland-Jørgensen wrote:
>>>>>>>> Yibo Zhao writes:
>>>>>>>>
>>>>>>>>> On 2019-09-19 18:37, Toke Høiland-Jørgensen wrote:
>>>>>>>>>> Yibo Zhao writes:
>>>>>>>>>>
>>>>>>>>>>> On 2019-09-18 19:23, Toke Høiland-Jørgensen wrote:
>>>>>>>>>>>> Yibo Zhao writes:
>>>>>>>>>>>>
>>>>>>>>>>>>> On 2019-09-18 05:10, Toke Høiland-Jørgensen wrote:
>>>>>>>>>>>>>> Yibo Zhao writes:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> In a loop txqs dequeue scenario, if the first txq in the
>>>>>>>>>>>>>>> rbtree
>>>>>>>>>>>>>>> gets
>>>>>>>>>>>>>>> removed from rbtree immediately in the
>>>>>>>>>>>>>>> ieee80211_return_txq(),
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> loop will break soon in the ieee80211_next_txq() due to
>>>>>>>>>>>>>>> schedule_pos
>>>>>>>>>>>>>>> not leading to the second txq in the rbtree. Thus,
>>>>>>>>>>>>>>> defering
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> removal right before the end of this schedule round.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Co-developed-by: Yibo Zhao
>>>>>>>>>>>>>>> Signed-off-by: Yibo Zhao
>>>>>>>>>>>>>>> Signed-off-by: Toke Høiland-Jørgensen
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I didn't write this patch, so please don't use my sign-off.
>>>>>>>>>>>>>> I'll
>>>>>>>>>>>>>> add
>>>>>>>>>>>>>> ack or review tags as appropriate in reply; but a few
>>>>>>>>>>>>>> comments
>>>>>>>>>>>>>> first:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>> include/net/mac80211.h | 16 ++--
>>>>>>>>>>>>>>> net/mac80211/ieee80211_i.h | 3 +++
>>>>>>>>>>>>>>> net/mac80211/main.c| 6 +
>>>>>>>>>>>>>>> net/mac80211/tx.c | 63
>>>>>>>>>>>>>>> +++---
>>>>>>>>>>>>>>> 4 files changed, 83 insertions(+), 5 deletions(-)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/include/net/mac80211.h
>>>>>>>>>>>>>>> b/include/net/mac80211.h
>>>>>>>>>>>>>>> index ac2ed8e..ba5a345 100644
>>>>>>>>>>>>>>> --- a/include/net/mac80211.h
>>>>>>>>>>>>>>> +++ b/include/net/mac80211.h
>>>>>>>>>>>>>>> @@ -925,6 +925,8 @@ struct ieee80211_tx_rate {
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> #define IEEE80211_MAX_TX_RETRY 31
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +#define IEEE80211_AIRTIME_TXQ_RM_CHK_INTV_IN_MS 100
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> static inline void ieee80211_rat
Re: [PATCH V3 3/4] mac80211: fix low throughput in multi-clients situation
Yibo Zhao writes: > Not long after the start of multi-clients test, not a single station is > an eligible candidate for transmission since global virtual time(g_vt) is > smaller than the virtual airtime(s_vt) of all the stations. As a result, > the Tx has been blocked and throughput is quite low. > > This may mainly due to sync mechanism and accumulative deviation from the > devision calculation of g_vt. > > For example: > Suppose we have 50 clients in first round. > Round 1: > STA weight Tx_time_round wt_sum s_vtg_vt valid_for_next_Tx > 1 256 204812800 20482000N > 2 256 20482048N > . . . . . > . . . . . > . . . . . > 50256 20482048N > > After this round, all the stations are not valid for next transmission due to > accumulative deviation. > > And if we add a new #51, > STA weight Tx_time_round wt_sum s_vtg_vt valid_for_next_Tx > 1 256 204813056 20482020N > 2 256 20482048N > . . . . > . . . . > . . . . > 50256 20482048N > 51256 10242524N That's better :) > Sync is done by: > max(g_vt of last round - grace period, s_vt) > and s_vt of #51 = max(2000 - 500, 0) + 1024 = 2524, and it is more than the > final > g_vt of this round. > > After this round, no more station is valid for transmission. > > The real situation can be more complicate, above is one of the extremely case. > > To avoid this situation to occur, the new proposal is: > > - Increase the airtime grace period a little more to reduce the > unexpected sync > > - If global virtual time is less than the virtual airtime of any station, > sync it to the airtime of first station in the red-black tree > > - Round the division result I can see why we need the second part (basically, this happens because I forgot to add a check for "no eligible stations" in may_transmit(), like the one in next_txq()). And rounding up the division result doesn't hurt, I guess. But why does it help to change the grace period if we're doing all the other stuff? -Toke
Re: [PATCH V3 2/4] mac80211: defer txqs removal from rbtree
Yibo Zhao writes: > In a loop txqs dequeue scenario, if the first txq in the rbtree gets > removed from rbtree immediately in the ieee80211_return_txq(), the > loop will break soon in the ieee80211_next_txq() due to schedule_pos > not leading to the second txq in the rbtree. Thus, defering the > removal right before the end of this schedule round. Didn't we agree that we could fix this by making __unschedule_txq() aware of schedule_pos instead of this deferred removal mechanism? -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH V3 4/4] mac80211: Sync airtime weight sum with per AC synced sta airtime weight together
Yibo Zhao writes:
> Global airtime weight sum is updated only when txq is added/removed
> from rbtree. If upper layer configures sta weight during high load,
> airtime weight sum will not be updated since txq is most likely on the
> tree. It could a little late for upper layer to reconfigure sta weight
> when txq is already in the rbtree. And thus, incorrect airtime weight sum
> will lead to incorrect global virtual time calculation as well as overflow
> of airtime weight sum during txq removed.
>
> Hence, need to update airtime weight sum upon receiving event for
> configuring sta weight once sta's txq is on the rbtree.
>
> Besides, if airtime weight sum of ACs and sta weight is synced under the
> same per AC lock protection, there can be a very short window causing
> incorrct airtime weight sum calculation as below:
>
> active_txq_lock_VO .
> VO weight sum is syncd.
> sta airtime weight sum is synced .
> active_txq_unlock_VO .
> . .
> active_txq_lock_VI.
> VI weight sum is syncd.
> sta airtime weight sumactive_txq_lock_BE
> active_txq_unlock_VIRemove txq and thus sum
> . is calculated with synced
> . sta airtime weight
> . active_txq_unlock_BE
>
> So introduce a per ac synced station airtime weight synced with per
> AC synced weight sum together. And the per-AC station airtime weight
> is used to calculate weight sum.
>
> Signed-off-by: Yibo Zhao
> ---
> net/mac80211/cfg.c | 29 ++---
> net/mac80211/debugfs_sta.c | 2 +-
> net/mac80211/sta_info.c| 9 -
> net/mac80211/sta_info.h| 5 +++--
> net/mac80211/tx.c | 4 ++--
> 5 files changed, 36 insertions(+), 13 deletions(-)
>
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index d65aa01..c8a0683 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -1284,7 +1284,8 @@ static int sta_apply_parameters(struct ieee80211_local
> *local,
> int ret = 0;
> struct ieee80211_supported_band *sband;
> struct ieee80211_sub_if_data *sdata = sta->sdata;
> - u32 mask, set;
> + u32 mask, set, tid, ac, pre_weight;
> + struct txq_info *txqi;
>
> sband = ieee80211_get_sband(sdata);
> if (!sband)
> @@ -1452,8 +1453,30 @@ static int sta_apply_parameters(struct ieee80211_local
> *local,
> if (ieee80211_vif_is_mesh(&sdata->vif))
> sta_apply_mesh_params(local, sta, params);
>
> - if (params->airtime_weight)
> - sta->airtime_weight = params->airtime_weight;
> + if (params->airtime_weight &&
> + params->airtime_weight != sta->airtime_weight) {
This check doesn't work I think? You're not using the array-based
sta->airtime_weight[], and there are locking issues by just checking
like this; so maybe just keep the if() on params->airtime_weight, and do
the checking inside the loop while holding the lock?
Or could we just turn the weights into atomics to avoid the locking
entirely?
> + for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
> + spin_lock_bh(&local->active_txq_lock[ac]);
> + for (tid = 0; tid < IEEE80211_NUM_TIDS + 1; tid++) {
> + if (!sta->sta.txq[tid] ||
> + ac != ieee80211_ac_from_tid(tid))
> + continue;
> +
> + pre_weight = sta->airtime_weight[ac];
> + sta->airtime_weight[ac] =
> + params->airtime_weight;
> +
> + txqi = to_txq_info(sta->sta.txq[tid]);
> + if (RB_EMPTY_NODE(&txqi->schedule_order))
> + continue;
> +
> + local->airtime_weight_sum[ac] =
> local->airtime_weight_sum[ac] +
> +
> params->airtime_weight -
> + pre_weight;
> + }
> + spin_unlock_bh(&local->active_txq_lock[ac]);
> + }
> + }
>
> /* set the STA state after all sta info from usermode has been set */
> if (test_sta_flag(sta, WLAN_STA_TDLS_PEER) ||
> diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
> index 80028da..43a7e6a 100644
> --- a/net/mac80211/debugfs_sta.c
> +++ b/net/mac80211/debugfs_sta.c
> @@ -223,7 +223,7 @@ static ssize_t sta_airtime_read(struct file *file, char
> __user *userbuf,
> "Virt-T: VO: %lld us VI: %lld us BE: %lld us BK: %lld us\n",
> rx_airtime,
> tx_airtime,
> - sta->airti
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
Kalle Valo writes: > Toke Høiland-Jørgensen writes: > >> Yibo Zhao writes: >> >>> On 2019-09-21 22:00, Toke Høiland-Jørgensen wrote: >>>> Yibo Zhao writes: >>>> >>>>> On 2019-09-21 21:02, Toke Høiland-Jørgensen wrote: >>>>>> Yibo Zhao writes: >>>>>> >>>>>>> On 2019-09-21 19:27, Toke Høiland-Jørgensen wrote: >>>>>>>> Yibo Zhao writes: >>>>>>>> >>>>>>>>> On 2019-09-20 17:15, Toke Høiland-Jørgensen wrote: >>>>>>>>>> Yibo Zhao writes: >>>>>>>>>> >>>>>>>>>>> On 2019-09-19 18:37, Toke Høiland-Jørgensen wrote: >>>>>>>>>>>> Yibo Zhao writes: >>>>>>>>>>>> >>>>>>>>>>>>> On 2019-09-18 19:23, Toke Høiland-Jørgensen wrote: >>>>>>>>>>>>>> Yibo Zhao writes: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 2019-09-18 05:10, Toke Høiland-Jørgensen wrote: >>>>>>>>>>>>>>>> Yibo Zhao writes: > > Guys, PLEASE please consider us poor maintainers drowning in email and > edit your quotes :) This style of discussion makes patchwork unusable: > > https://patchwork.kernel.org/patch/11147019/ Heh, oops, didn't realise you were following the discussion from patchwork; sorry, will be sure to cut things in the future. The quote marks do make a very nice (reverse) christmas tree, though ;) -Toke
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
Kalle Valo writes: > Toke Høiland-Jørgensen writes: > >> Kalle Valo writes: >> >>> Toke Høiland-Jørgensen writes: >>> >>>> Yibo Zhao writes: >>>> >>>>> On 2019-09-21 22:00, Toke Høiland-Jørgensen wrote: >>>>>> Yibo Zhao writes: >>>>>> >>>>>>> On 2019-09-21 21:02, Toke Høiland-Jørgensen wrote: >>>>>>>> Yibo Zhao writes: >>>>>>>> >>>>>>>>> On 2019-09-21 19:27, Toke Høiland-Jørgensen wrote: >>>>>>>>>> Yibo Zhao writes: >>>>>>>>>> >>>>>>>>>>> On 2019-09-20 17:15, Toke Høiland-Jørgensen wrote: >>>>>>>>>>>> Yibo Zhao writes: >>>>>>>>>>>> >>>>>>>>>>>>> On 2019-09-19 18:37, Toke Høiland-Jørgensen wrote: >>>>>>>>>>>>>> Yibo Zhao writes: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 2019-09-18 19:23, Toke Høiland-Jørgensen wrote: >>>>>>>>>>>>>>>> Yibo Zhao writes: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On 2019-09-18 05:10, Toke Høiland-Jørgensen wrote: >>>>>>>>>>>>>>>>>> Yibo Zhao writes: >>> >>> Guys, PLEASE please consider us poor maintainers drowning in email and >>> edit your quotes :) This style of discussion makes patchwork unusable: >>> >>> https://patchwork.kernel.org/patch/11147019/ >> >> Heh, oops, didn't realise you were following the discussion from >> patchwork; sorry, will be sure to cut things in the future. > > To be honest, I'm not sure how much Johannes uses patchwork. But I > check everything from patchwork 95% of the time and try to keep my > email boxes clean. Noted. I'll try to be nice to patchwork, then :) >> The quote marks do make a very nice (reverse) christmas tree, though ;) > > It did! I had to include that to my rant :) :D -Toke
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
>> Hmm, yeah, I guess we could end up with a loop like that as well. >> Keeping the schedule_round would be a way to fix it, but I'm not sure >> we >> should just skip that station; maybe we should just end the round >> instead? > I am not sure. I believe, in some cases, the rest of the nodes which > could be most of the nodes in the tree will not have the chance to be > scheduled in this round. My guess would be that it doesn't really matter, because in most cases each schedule round will only actually end up queueing packets from one or two stations; as the driver will pull multiple packets from that one station which will often fill up the firmware queues (especially once we start throttling that with the AQL stuff). So I guess we can just skip TXQs that we've already seen this scheduling round, and let the v_t compare determine transmit eligibility :) -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH V3 4/4] mac80211: Sync airtime weight sum with per AC synced sta airtime weight together
Yibo Zhao writes:
> On 2019-09-23 19:00, Toke Høiland-Jørgensen wrote:
>
>>> - if (params->airtime_weight)
>>> - sta->airtime_weight = params->airtime_weight;
>>> + if (params->airtime_weight &&
>>> + params->airtime_weight != sta->airtime_weight) {
>>
>> This check doesn't work I think? You're not using the array-based
>> sta->airtime_weight[], and there are locking issues by just checking
>> like this; so maybe just keep the if() on params->airtime_weight, and
>> do
>> the checking inside the loop while holding the lock?
>
> It should be array-based sta->airtime_weight[] and I am missing that
> part during the porting. But you are right about we should check it
> inside the loop with the lock.
>
>>
>> Or could we just turn the weights into atomics to avoid the locking
>> entirely?
>
> Actually, we still need the active txq locking to make sure the txq is
> on the rbtree. Otherwise, no need to change airtime weight sum.
True. Just moving the check inside the locking will be the right thing
to do, then.
-Toke
Re: [PATCH 2/4] mac80211: defer txqs removal from rbtree
Yibo Zhao writes: > On 2019-09-24 15:26, Toke Høiland-Jørgensen wrote: >>>> Hmm, yeah, I guess we could end up with a loop like that as well. >>>> Keeping the schedule_round would be a way to fix it, but I'm not sure >>>> we >>>> should just skip that station; maybe we should just end the round >>>> instead? >>> I am not sure. I believe, in some cases, the rest of the nodes which >>> could be most of the nodes in the tree will not have the chance to be >>> scheduled in this round. >> >> My guess would be that it doesn't really matter, because in most cases >> each schedule round will only actually end up queueing packets from one >> or two stations; as the driver will pull multiple packets from that one >> station which will often fill up the firmware queues (especially once >> we >> start throttling that with the AQL stuff). >> >> So I guess we can just skip TXQs that we've already seen this >> scheduling >> round, and let the v_t compare determine transmit eligibility :) > > I am a little confused. So do you mean it is fine for you to skip the > TXQs we met in this round before and continue the loop until the end or > vt comparison failure? Yeah. In most cases it won't make any difference; but it'll make sure we visit all eligible TXQs in all cases, so we might as well do that if we're tracking the scheduling round anyway. -Toke
Re: [PATCH V3 3/4] mac80211: fix low throughput in multi-clients situation
Yibo Zhao writes: >> I can see why we need the second part (basically, this happens because >> I >> forgot to add a check for "no eligible stations" in may_transmit(), >> like >> the one in next_txq()). And rounding up the division result doesn't >> hurt, I guess. But why does it help to change the grace period if we're >> doing all the other stuff? > In multi-clients case, it is possible a TXQ sometimes gets drained due > to FW has deep queue and few packets in TXQ at that time. So the TXQ is > removed from the rbtree after dequeuing. When it is about to added back > very soon after the removal, the g_vt might have gone a little far away > from sta vt where sync is needed. With this sync, the station is forced > to catch up with the g_vt, however, its chance for transmission has been > reduced. I think 500us is quite a short period in multi-clients case. That's a good point, actually: Having the grace period be too small will allow stations that leave and re-enter the queue to "skip ahead" and use more than its share. However, I think it's a separate issue from what this patch is about; so how about I just increase the grace period in the next version of the base patch? -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH] mac80211: fix txq null pointer dereference
Miaoqing Pan writes: > If the interface type is P2P_DEVICE or NAN, read the file of > '/sys/kernel/debug/ieee80211/phyx/netdev:wlanx/aqm' will get a > NULL pointer dereference. As for those interface type, the > pointer sdata->vif.txq is NULL. Heh. Oops! > Unable to handle kernel NULL pointer dereference at virtual address 0011 > CPU: 1 PID: 30936 Comm: cat Not tainted 4.14.104 #1 > task: ffc0337e4880 task.stack: ff800cd2 > PC is at ieee80211_if_fmt_aqm+0x34/0xa0 [mac80211] > LR is at ieee80211_if_fmt_aqm+0x34/0xa0 [mac80211] > pc : [] lr : [] pstate: 6145 > sp : ff800cd23bb0 > x29: ff800cd23c00 x28: ffc0337e4880 > x27: ff8008a04000 x26: 0003 > x25: 018e x24: ff80090f9d30 > x23: ffc034b62000 x22: > x21: ffc0335008e0 x20: ff800cd23c10 > x19: 00c8 x18: > x17: x16: ff80081ef454 > x15: x14: f45c1d27 > x13: ffab6710 x12: 0003 > x11: 0006 x10: > x9 : 0001 x8 : ffc0337e4880 > x7 : 0003 x6 : f4498000 > x5 : x4 : ff8000b7 > x3 : ff800cd23e80 x2 : 00c8 > x1 : ff800cd23c10 x0 : ffc0335008e0 > Process cat (pid: 30936, stack limit = 0xff800cd2) > [Call trace: > Exception stack(0xff800cd23a70 to 0xff800cd23bb0) > 3a60: ffc0335008e0 ff800cd23c10 > 3a80: 00c8 ff800cd23e80 ff8000b7 > 3aa0: f4498000 0003 ffc0337e4880 0001 > 3ac0: 0006 0003 ffab6710 > 3ae0: f45c1d27 ff80081ef454 > 3b00: 00c8 ff800cd23c10 ffc0335008e0 > 3b20: ffc034b62000 ff80090f9d30 018e > 3b40: 0003 ff8008a04000 ffc0337e4880 ff800cd23c00 > 3b60: ff8000b7cd00 ff800cd23bb0 ff8000b7cd00 6145 > 3b80: ff800cd23ba0 ff80088e16e4 007f ff800cd23c40 > 3ba0: ff800cd23c00 ff8000b7cd00 > [] ieee80211_if_fmt_aqm+0x34/0xa0 [mac80211] > [] ieee80211_if_read+0x60/0xbc [mac80211] > [] ieee80211_if_read_aqm+0x28/0x30 [mac80211] > [] full_proxy_read+0x2c/0x48 > [] __vfs_read+0x2c/0xd4 > [] vfs_read+0x8c/0x108 > [] SyS_read+0x40/0x7c > > Tested HW: QCA9984 > Tested FW: 10.4-3.10-00047 > > Signed-off-by: Miaoqing Pan Acked-by: Toke Høiland-Jørgensen
Re: [PATCH V3 0/4] Enable virtual time-based airtime scheduler support on ath10k
Yibo Zhao writes: > On 2019-10-01 18:19, Johannes Berg wrote: >> On Mon, 2019-09-23 at 15:19 +0800, Yibo Zhao wrote: >>> This series fix some issues when enabling virtual time-based airtime >>> scheduler on ath10k. >>> >> Given the lengthy discussion here and also over in the related thread >> about AQL, I'm assuming you're going to repost this eventually. > > Yes, will repost once Toke have updated "mac80211: Switch to a virtual > time-based airtime scheduler" with his new ideas. Which in turn is waiting for the AQL stuff... :) -Toke
Re: [PATCH v3 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
Johannes Berg writes: > Hi, > > A couple of points... > > First, I'd like Toke to review & ack this if possible :-) Sure, I'll look at it. I'm away the rest of this week, but should hopefully get some more time next week. It may be that it will take the form of another submission that integrates this with the previous patch I sent that put more of the calculation into mac80211 itself... -Toke
[PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
From: Toke Høiland-Jørgensen
To implement airtime queue limiting, we need to keep a running account of
the estimated airtime of all skbs queued into the device. Do to this
correctly, we need to store the airtime estimate into the skb so we can
decrease the outstanding balance when the skb is freed. This means that the
time estimate must be stored somewhere that will survive for the lifetime
of the skb.
Fortunately, we had a couple of bytes left in the 'status' field in the
ieee80211_tx_info; and since we only plan to calculate the airtime estimate
after the skb is dequeued from the FQ structure, on the control side we can
share the space with the codel enqueue time. And by rearranging the order
of elements it is possible to have the position of the new tx_time_est line
up between the control and status structs, so the value will survive from
when mac80211 hands the packet to the driver, and until the driver either
frees it, or hands it back through TX status.
Signed-off-by: Toke Høiland-Jørgensen
---
include/net/mac80211.h | 26 +++---
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d69081c38788..49f8ea0af5f8 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -975,20 +975,23 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate
*rate)
* @control.short_preamble: use short preamble (CCK only)
* @control.skip_table: skip externally configured rate table
* @control.jiffies: timestamp for expiry on powersave clients
+ * @control.enqueue_time: enqueue time (for iTXQs)
+ * @control.tx_time_est: estimated airtime usage (shared with @status)
+ * @control.reserved: unused field to ensure alignment of data structure
+ * @control.flags: control flags, see &enum mac80211_tx_control_flags
* @control.vif: virtual interface (may be NULL)
* @control.hw_key: key to encrypt with (may be NULL)
- * @control.flags: control flags, see &enum mac80211_tx_control_flags
- * @control.enqueue_time: enqueue time (for iTXQs)
* @driver_rates: alias to @control.rates to reserve space
* @pad: padding
* @rate_driver_data: driver use area if driver needs @control.rates
* @status: union part for status data
* @status.rates: attempted rates
* @status.ack_signal: ACK signal
+ * @status.tx_time_est: estimated airtime of skb (shared with @control)
+ * @status.tx_time: actual airtime consumed for transmission
* @status.ampdu_ack_len: AMPDU ack length
* @status.ampdu_len: AMPDU length
* @status.antenna: (legacy, kept only for iwlegacy)
- * @status.tx_time: airtime consumed for transmission
* @status.is_valid_ack_signal: ACK signal is valid
* @status.status_driver_data: driver use area
* @ack: union part for pure ACK data
@@ -1026,11 +1029,17 @@ struct ieee80211_tx_info {
/* only needed before rate control */
unsigned long jiffies;
};
+ union {
+ codel_time_t enqueue_time;
+ struct {
+ u16 tx_time_est; /* shared with status
*/
+ u16 reserved; /* padding for alignment
*/
+ };
+ };
+ u32 flags;
/* NB: vif can be NULL for injected frames */
struct ieee80211_vif *vif;
struct ieee80211_key_conf *hw_key;
- u32 flags;
- codel_time_t enqueue_time;
} control;
struct {
u64 cookie;
@@ -1038,12 +1047,13 @@ struct ieee80211_tx_info {
struct {
struct ieee80211_tx_rate rates[IEEE80211_TX_MAX_RATES];
s32 ack_signal;
+ u16 tx_time_est; /* shared with control */
+ u16 tx_time;
u8 ampdu_ack_len;
u8 ampdu_len;
u8 antenna;
- u16 tx_time;
bool is_valid_ack_signal;
- void *status_driver_data[19 / sizeof(void *)];
+ void *status_driver_data[16 / sizeof(void *)];
} status;
struct {
struct ieee80211_tx_rate driver_rates[
@@ -1126,6 +1136,8 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info
*info)
offsetof(struct ieee80211_tx_info, control.rates));
BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
offsetof(struct ieee80211_tx_info, driver_rates));
+ BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, control.tx_time_est) !=
+offsetof(struct ieee80211_tx_info, status.tx_time_est));
BUILD_BUG_
[PATCH v2 0/4] Add Airtime Queue Limits (AQL) to mac80211
This series is a first attempt at porting the Airtime Queue Limits concept from the out-of-tree ath10k implementation[0] to mac80211. This version takes Kan's patch to do the throttling in mac80211, and replaces the driver API with the mechanism from the previous version of my series, which instead calculated the expected airtime at dequeue time inside mac80211, storing it in the SKB cb field. This series also imports Felix' airtime calculation code from mt76 into mac80211, adjusting the API so it can be used from TX dequeue, by extracting the latest TX rate from the tx_stats structure kept for each station. As before, I've only compile tested this (lacking the proper hardware to do more testing). So I'm hoping someone with a proper testing setup can take the whole thing for a spin... :) The series is also available in my git repo here: https://git.kernel.org/pub/scm/linux/kernel/git/toke/linux.git/log/?h=mac80211-aql-02 Changelog: v2: - Integrate Kan's approach to airtime throttling. - Hopefully fix the cb struct alignment on big-endian architectures. --- Kan Yan (1): mac80211: Implement Airtime-based Queue Limit (AQL) Toke Høiland-Jørgensen (3): mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est mac80211: Import airtime calculation code from mt76 mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue include/net/cfg80211.h |7 + include/net/mac80211.h | 52 +- net/mac80211/Makefile |3 net/mac80211/airtime.c | 375 net/mac80211/debugfs.c | 78 + net/mac80211/debugfs_sta.c | 43 - net/mac80211/ieee80211_i.h |8 + net/mac80211/main.c|9 + net/mac80211/sta_info.c| 32 net/mac80211/sta_info.h|8 + net/mac80211/status.c | 38 net/mac80211/tx.c | 62 +++ 12 files changed, 693 insertions(+), 22 deletions(-) create mode 100644 net/mac80211/airtime.c ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
[PATCH v2 2/4] mac80211: Import airtime calculation code from mt76
From: Toke Høiland-Jørgensen Felix recently added code to calculate airtime of packets to the mt76 driver. Import this into mac80211 so we can use it for airtime queue limit calculations later. The airtime.c file is copied verbatim from the mt76 driver, and adjusted to use mac80211 data structures instead (which is fairly straight forward). The per-rate TX rate calculation is split out to its own function (ieee80211_calc_tx_airtime_rate()) so it can be used directly for the AQL calculations added in a subsequent patch. The only thing that it was not possible to port directly was the bit that read the internal driver flags of struct ieee80211_rate to determine whether a rate is using CCK or OFDM encoding. Instead, just look at the rate index, since at least mt76 and ath10k both seem to have the same number of CCK rates (4) in their tables. Signed-off-by: Toke Høiland-Jørgensen --- include/net/mac80211.h | 14 ++ net/mac80211/Makefile |3 net/mac80211/airtime.c | 375 net/mac80211/ieee80211_i.h |4 4 files changed, 395 insertions(+), 1 deletion(-) create mode 100644 net/mac80211/airtime.c diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 49f8ea0af5f8..7619eb74d612 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -6434,4 +6434,18 @@ void ieee80211_nan_func_match(struct ieee80211_vif *vif, struct cfg80211_nan_match_params *match, gfp_t gfp); +/** + * ieee80211_calc_tx_airtime - calculate estimated transmission airtime. + * + * This function calculates the estimated airtime usage of a frame based on the + * rate information in the TX info struct and the frame length. + * + * @hw: pointer as obtained from ieee80211_alloc_hw() + * @info: &struct ieee80211_tx_info of the frame. + * @len: frame length in bytes + */ +u32 ieee80211_calc_tx_airtime(struct ieee80211_hw *hw, + struct ieee80211_tx_info *info, + int len); + #endif /* MAC80211_H */ diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile index 4f03ebe732fa..6cbb1286d6c0 100644 --- a/net/mac80211/Makefile +++ b/net/mac80211/Makefile @@ -32,7 +32,8 @@ mac80211-y := \ chan.o \ trace.o mlme.o \ tdls.o \ - ocb.o + ocb.o \ + airtime.o mac80211-$(CONFIG_MAC80211_LEDS) += led.o mac80211-$(CONFIG_MAC80211_DEBUGFS) += \ diff --git a/net/mac80211/airtime.c b/net/mac80211/airtime.c new file mode 100644 index ..7a18d5405756 --- /dev/null +++ b/net/mac80211/airtime.c @@ -0,0 +1,375 @@ +// SPDX-License-Identifier: ISC +/* + * Copyright (C) 2019 Felix Fietkau + */ + +#include +#include "ieee80211_i.h" +#include "sta_info.h" + +#define AVG_PKT_SIZE 1024 + +/* Number of bits for an average sized packet */ +#define MCS_NBITS (AVG_PKT_SIZE << 3) + +/* Number of symbols for a packet with (bps) bits per symbol */ +#define MCS_NSYMS(bps) DIV_ROUND_UP(MCS_NBITS, (bps)) + +/* Transmission time (1024 usec) for a packet containing (syms) * symbols */ +#define MCS_SYMBOL_TIME(sgi, syms) \ + (sgi ? \ + ((syms) * 18 * 1024 + 4 * 1024) / 5 : /* syms * 3.6 us */ \ + ((syms) * 1024) << 2 /* syms * 4 us */ \ + ) + +/* Transmit duration for the raw data part of an average sized packet */ +#define MCS_DURATION(streams, sgi, bps) \ + MCS_SYMBOL_TIME(sgi, MCS_NSYMS((streams) * (bps))) + +#define BW_20 0 +#define BW_40 1 +#define BW_80 2 + +/* + * Define group sort order: HT40 -> SGI -> #streams + */ +#define IEEE80211_MAX_STREAMS 4 +#define IEEE80211_HT_STREAM_GROUPS 4 /* BW(=2) * SGI(=2) */ +#define IEEE80211_VHT_STREAM_GROUPS6 /* BW(=3) * SGI(=2) */ + +#define IEEE80211_HT_GROUPS_NB (IEEE80211_MAX_STREAMS *\ +IEEE80211_HT_STREAM_GROUPS) +#define IEEE80211_VHT_GROUPS_NB(IEEE80211_MAX_STREAMS *\ +IEEE80211_VHT_STREAM_GROUPS) +#define IEEE80211_GROUPS_NB(IEEE80211_HT_GROUPS_NB + \ +IEEE80211_VHT_GROUPS_NB) + +#define IEEE80211_HT_GROUP_0 0 +#define IEEE80211_VHT_GROUP_0 (IEEE80211_HT_GROUP_0 + IEEE80211_HT_GROUPS_NB) + +#define MCS_GROUP_RATES10 +#define CCK_NUM_RATES 4 + +#define HT_GROUP_IDX(_streams, _sgi, _ht40)\ + IEEE80211_HT_GROUP_0 + \ + IEEE80211_MAX_STREAMS * 2 * _ht40 + \ + IEEE80211_MAX_STREAMS * _sgi + \ + _streams - 1 + +#define _MAX(a, b) (((a)>(b))?(a):(b)) + +#define GROUP_SHIFT(duration) \ + _MAX(0, 16 - __builtin_clz(duration)) + +/* MCS
[PATCH v2 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
From: Kan Yan
In order for the Fq_CoDel integrated in mac80211 layer operates effectively
to control excessive queueing latency, the CoDel algorithm requires an
accurate measure of how long the packets stays in the queue, aka sojourn
time. The sojourn time measured at mac80211 layer doesn't include queueing
latency in lower layer (firmware/hardware) and CoDel expects lower layer to
have a short queue. However, most 802.11ac chipsets offload tasks such TX
aggregation to firmware or hardware, thus have a deep lower layer queue.
Without a mechanism to control the lower layer queue size, packets only
stays in mac80211 layer transiently before being sent to firmware queue.
As a result, the sojourn time measured by CoDel in the mac80211 layer is
almost always lower than the CoDel latency target, hence CoDel does little
to control the latency, even when the lower layer queue causes excessive
latency.
Byte Queue limits (BQL) is commonly used to address the similar issue with
wired network interface. However, this method cannot be applied directly
to the wireless network interface. Byte is not a suitable measure of queue
depth in the wireless network, as the data rate can vary dramatically from
station to station in the same network, from a few Mbps to over Gbps.
This patch implemented an Airtime-based Queue Limit (AQL) to make CoDel
works effectively with wireless drivers that utilized firmware/hardware
offloading. AQL only allows each txq to release just enough packets to the
lower layer to form 1-2 large aggregations to keep hardware fully utilized
and keep the rest of frames in mac80211 layer to be controlled by the CoDel
algorithm.
Signed-off-by: Kan Yan
[ Toke: Get rid of the driver API to set pending airtime ]
Signed-off-by: Toke Høiland-Jørgensen
---
include/net/cfg80211.h |7
include/net/mac80211.h | 12 +++
net/mac80211/debugfs.c | 78
net/mac80211/debugfs_sta.c | 43 +++-
net/mac80211/ieee80211_i.h |4 ++
net/mac80211/main.c|9 +
net/mac80211/sta_info.c| 32 ++
net/mac80211/sta_info.h|8 +
net/mac80211/tx.c | 46 --
9 files changed, 225 insertions(+), 14 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ff45c3e1abff..8d50c0a60dbd 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2602,6 +2602,13 @@ enum wiphy_params_flags {
#define IEEE80211_DEFAULT_AIRTIME_WEIGHT 256
+/* The per TXQ device queue limit in airtime */
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L 4000
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H 8000
+
+/* The per interface airtime threshold to switch to lower queue limit */
+#define IEEE80211_AQL_THRESHOLD24000
+
/**
* struct cfg80211_pmksa - PMK Security Association
*
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 7619eb74d612..b5727a20754c 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5575,6 +5575,18 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta
*pubsta, int tid);
void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
u32 tx_airtime, u32 rx_airtime);
+/**
+ * ieee80211_txq_airtime_check - check if a txq can send frame to device
+ *
+ * @hw: pointer obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Return true if the AQL's airtime limit has not been reached and the txq can
+ * continue to send more packets to the device. Otherwise return false.
+ */
+bool
+ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq
*txq);
+
/**
* ieee80211_iter_keys - iterate keys programmed into the device
* @hw: pointer obtained from ieee80211_alloc_hw()
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 568b3b276931..d77ea0e51c1d 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -148,6 +148,80 @@ static const struct file_operations aqm_ops = {
.llseek = default_llseek,
};
+static ssize_t aql_txq_limit_read(struct file *file,
+ char __user *user_buf,
+ size_t count,
+ loff_t *ppos)
+{
+ struct ieee80211_local *local = file->private_data;
+ char buf[400];
+ int len = 0;
+
+ len = scnprintf(buf, sizeof(buf),
+ "AC AQL limit low AQL limit high\n"
+ "VO %u %u\n"
+ "VI %u %u\n"
+ "BE %u %u\n"
+ "BK %u %u\n",
+ local->aql_txq_limit_low[IEEE80211_AC_VO],
+ local->aql_txq_limit_hi
[PATCH v2 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
From: Toke Høiland-Jørgensen
The previous commit added the ability to throttle stations when they queue
too much airtime in the hardware. This commit enables the functionality by
calculating the expected airtime usage of each packet that is dequeued from
the TXQs in mac80211, and accounting that as pending airtime.
The estimated airtime for each skb is stored in the tx_info, so we can
subtract the same amount from the running total when the skb is freed or
recycled. The throttling mechanism relies on this accounting to be
accurate (i.e., that we are not freeing skbs without subtracting any
airtime they were accounted for), so we put the subtraction into
ieee80211_report_used_skb(). As an optimisation, we also subtract the
airtime on regular TX completion, zeroing out the value stored in the
packet afterwards, to avoid having to do an expensive lookup of the
station from the packet data on every packet.
This patch does *not* include any mechanism to wake a throttled TXQ again,
on the assumption that this will happen anyway as a side effect of whatever
freed the skb (most commonly a TX completion).
Signed-off-by: Toke Høiland-Jørgensen
---
net/mac80211/status.c | 38 ++
net/mac80211/tx.c | 16
2 files changed, 54 insertions(+)
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index ab8ba5835ca0..ce990a1e9043 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -676,6 +676,33 @@ static void ieee80211_report_used_skb(struct
ieee80211_local *local,
if (dropped)
acked = false;
+ if (info->status.tx_time_est) {
+ struct ieee80211_sub_if_data *sdata;
+ struct sta_info *sta = NULL;
+ u8 *qc, ac;
+ int tid;
+
+ rcu_read_lock();
+
+ sdata = ieee80211_sdata_from_skb(local, skb);
+ if (sdata)
+ sta = sta_info_get_bss(sdata, skb_mac_header(skb));
+
+ if (ieee80211_is_data_qos(hdr->frame_control)) {
+ qc = ieee80211_get_qos_ctl(hdr);
+ tid = qc[0] & 0xf;
+ ac = ieee80211_ac_from_tid(tid);
+ } else {
+ ac = IEEE80211_AC_BE;
+ }
+
+ ieee80211_sta_update_pending_airtime(local, sta, ac,
+info->status.tx_time_est,
+true);
+ rcu_read_unlock();
+
+ }
+
if (info->flags & IEEE80211_TX_INTFL_MLME_CONN_TX) {
struct ieee80211_sub_if_data *sdata;
@@ -930,6 +957,17 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
tid = qc[0] & 0xf;
}
+ if (info->status.tx_time_est) {
+ /* Do this here to avoid the expensive lookup of the sta
+* in ieee80211_report_used_skb().
+*/
+ ieee80211_sta_update_pending_airtime(local, sta,
+
ieee80211_ac_from_tid(tid),
+
info->status.tx_time_est,
+true);
+ info->status.tx_time_est = 0;
+ }
+
if (!acked && ieee80211_is_back_req(fc)) {
u16 control;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 405f622b3fe0..b6b47171b340 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3539,9 +3539,14 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw
*hw,
struct ieee80211_tx_data tx;
ieee80211_tx_result r;
struct ieee80211_vif *vif = txq->vif;
+ u8 ac = txq->ac;
+ u32 airtime;
WARN_ON_ONCE(softirq_count() == 0);
+ if (!ieee80211_txq_airtime_check(hw, txq))
+ return NULL;
+
begin:
spin_lock_bh(&fq->lock);
@@ -3652,6 +3657,17 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw
*hw,
}
IEEE80211_SKB_CB(skb)->control.vif = vif;
+
+ if (local->airtime_flags & AIRTIME_USE_AQL) {
+ airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
+skb->len + 38);
+ if (airtime) {
+ info->control.tx_time_est = airtime;
+ ieee80211_sta_update_pending_airtime(local, tx.sta, ac,
+airtime, false);
+ }
+ }
+
return skb;
out:
Re: [PATCH v2 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
Kan Yan writes:
> Hi Toke,
>
> Thanks for getting this done! I will give it a try in the next few
> days. A few comments:
>
>> The estimated airtime for each skb is stored in the tx_info, so we can
>> subtract the same amount from the running total when the skb is freed or
>> recycled.
>
> Looks like ath10k driver zero out the info->status before calling
> ieee80211_tx_status(...):
> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
> const struct htt_tx_done *tx_done)
> {
> ...
> info = IEEE80211_SKB_CB(msdu);
> memset(&info->status, 0, sizeof(info->status));
> ...
> ieee80211_tx_status(htt->ar->hw, msdu);
> }
Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
it's only ath10k and iwl that do this, though, so it's probably
manageable to just fix this. I think the simplest solution is just to
restore the field after clearing, no?
> We need either restore the info->status.tx_time_est or calling
> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
> get erased.
>
>> + if (local->airtime_flags & AIRTIME_USE_AQL) {
>> + airtime = ieee80211_calc_expected_tx_airtime(hw, vif,
>> txq->sta,
>> +skb->len + 38);
>
> I think it is better to put the "+ 38" that takes care of the header
> overhead inside ieee80211_calc_expected_tx_airtime().
Hmm, no strong opinion about this; but yeah, since we have a dedicated
function for this use I guess there's no harm in adding it there :)
-Toke
Re: [Make-wifi-fast] [PATCH v2 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
Sebastian Moeller writes:
>> On Oct 17, 2019, at 11:44, Toke Høiland-Jørgensen wrote:
>>
>> Kan Yan writes:
>>
>>> Hi Toke,
>>>
>>> Thanks for getting this done! I will give it a try in the next few
>>> days. A few comments:
>>>
>>>> The estimated airtime for each skb is stored in the tx_info, so we can
>>>> subtract the same amount from the running total when the skb is freed or
>>>> recycled.
>>>
>>> Looks like ath10k driver zero out the info->status before calling
>>> ieee80211_tx_status(...):
>>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>> const struct htt_tx_done *tx_done)
>>> {
>>> ...
>>>info = IEEE80211_SKB_CB(msdu);
>>>memset(&info->status, 0, sizeof(info->status));
>>> ...
>>>ieee80211_tx_status(htt->ar->hw, msdu);
>>> }
>>
>> Ah, bugger; I was afraid we'd run into this. A quick grep indicates that
>> it's only ath10k and iwl that do this, though, so it's probably
>> manageable to just fix this. I think the simplest solution is just to
>> restore the field after clearing, no?
>>
>>> We need either restore the info->status.tx_time_est or calling
>>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
>>> get erased.
>>>
>>>> + if (local->airtime_flags & AIRTIME_USE_AQL) {
>>>> + airtime = ieee80211_calc_expected_tx_airtime(hw, vif,
>>>> txq->sta,
>>>> +skb->len +
>>>> 38);
>>>
>>> I think it is better to put the "+ 38" that takes care of the header
>>> overhead inside ieee80211_calc_expected_tx_airtime().
>>
>> Hmm, no strong opinion about this; but yeah, since we have a dedicated
>> function for this use I guess there's no harm in adding it there :)
>>
>
> Silly question, is this Overhead guaranteed to be 38 Bytes for all
> eternity? Otherwise a variable or a preprocessor constant might be
> more future proof?
Well, yeah, as long as we're sending Ethernet packets. Which is kinda
baked into the WiFi standard :)
-Toke
Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
Kan Yan writes:
> The "tx_time_est" field, shared by control and status, is not able to
> survive until the skb returns to the mac80211 layer in some
> architectures. The same space is defined as driver_data and some
> wireless drivers use it for other purposes, as the cb in the sk_buff
> is free to be used by any layer.
>
> In the case of ath10k, the tx_time_est get clobbered by
> struct ath10k_skb_cb {
> dma_addr_t paddr;
> u8 flags;
> u8 eid;
> u16 msdu_id;
> u16 airtime_est;
> struct ieee80211_vif *vif;
> struct ieee80211_txq *txq;
> } __packed;
Ah, bugger, of course the driver that actually needs this is using the
full driver_data space :P
> Do you think shrink driver_data by 2 bytes and use that space for
> tx_time_est to make it persistent across mac80211 and wireless driver
> layer an acceptable solution?
Hmm, the driver_data field is defined as an array of pointers, so we can
only shrink it in increments of sizeof(void *). I think it may be
feasible to shrink it (as in, I don't think any drivers are actually
using the full 40 bytes), but doing this in a way that will gain us a
2-byte space that is also usable in the case driver_data is *not* used
(i.e., it needs be able to align with a field in .control and .status as
well) would require some serious surgery of the whole ieee80211_tx_info...
However, there's a nice juicy 'u16 ack_frame_id' at the start of
ieee80211_tx_info. Could we potentially use that? We could use the top
bit as a disambiguation flag; I think we're fine with 15 bits for the TX
time itself (a single packet won't exceed 8ms or TX time), so if we can
live with 15 bits of ACK frame ID space, that could be a way forward?
Johannes, what do you think?
-Toke
Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
Johannes Berg writes: > On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote: > >> However, there's a nice juicy 'u16 ack_frame_id' at the start of >> ieee80211_tx_info. Could we potentially use that? We could use the top >> bit as a disambiguation flag; I think we're fine with 15 bits for the TX >> time itself (a single packet won't exceed 8ms or TX time), so if we can >> live with 15 bits of ACK frame ID space, that could be a way forward? > > I was going to say that should work as we only ever have a handful of > ACK frame IDs, but ... you still need the airtime even for a frame that > userspace wants to know the ACK status of, no? Oh, right. Well, let's try to do the actual math... A full-size (1538 bytes) packet takes ~2050 microseconds to transmit at 6 Mbps. Adding in overhead, it's certainly still less that 4096 us, so 12 bits is plenty. That leaves four bits for the ACK status ID if we just split the u16; if we only ever have "a handful", that should be enough, no? We could also split 5/11. That would support up to 32 ACK IDs, and we can just truncate the airtime at 2048 us, which is not a big deal I'd say. We could even split 6/10 and only need to truncate the TX time at rates below 13 Mbps... Or we could sacrifice a bit to the flag and only truncate if the ACK status flag is set. Think it mostly depends on what is the smallest ID space for ACK IDs we can live with? :) -Toke
Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
Johannes Berg writes: > On Fri, 2019-10-18 at 15:31 +0200, Toke Høiland-Jørgensen wrote: > >> Well, let's try to do the actual math... A full-size (1538 bytes) packet >> takes ~2050 microseconds to transmit at 6 Mbps. Adding in overhead, it's >> certainly still less that 4096 us, so 12 bits is plenty. > > What about A-MSDUs? But I guess maximum continous transmissions are at > most 4ms anyway, so a single packet should never be longer. Ah yeah, those could be a bit bigger, but yeah, 4ms should at least be enough. >> That leaves >> four bits for the ACK status ID if we just split the u16; if we only >> ever have "a handful", that should be enough, no? > > It's how many are in flight at a time, 16 doesn't seem likely to happen, > but I don't really know what applications are doing with it now. > Probably only wpa_s for the EAPOL TX status. Right. Well in that case, let's try it. As long as we fail in a reasonable way, we can just see if we run into anything that breaks? I guess in this case that means rejecting requests from userspace if we run out of IDs rather than silently wrapping and returning wrong data :) >> We could also split 5/11. That would support up to 32 ACK IDs, and we >> can just truncate the airtime at 2048 us, which is not a big deal I'd >> say. > > We can also play with the units of the airtime, e.g. making that a > multiple of 2 or 4 us? Seems unlikely to matter much? Sure, that's a good point! Increments of 4us means we can fit 4ms is 10 bits, leaving plenty of space for ACK IDs (hopefully). I'll rework the series to use that instead :) -Toke
Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
Johannes Berg writes: > On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote: > >> Right. Well in that case, let's try it. As long as we fail in a >> reasonable way, we can just see if we run into anything that breaks? I >> guess in this case that means rejecting requests from userspace if we >> run out of IDs rather than silently wrapping and returning wrong data :) > > We can't reject due to how this works, but if the idr_alloc() fails then > we'll just not give a status back to userspace later. OK, I guess someone will notice if that suddenly starts happening all the time ;) >> > > We could also split 5/11. That would support up to 32 ACK IDs, and we >> > > can just truncate the airtime at 2048 us, which is not a big deal I'd >> > > say. >> > >> > We can also play with the units of the airtime, e.g. making that a >> > multiple of 2 or 4 us? Seems unlikely to matter much? >> >> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10 >> bits, leaving plenty of space for ACK IDs (hopefully). >> >> I'll rework the series to use that instead :) > > OK. > > There are two places that call idr_alloc() with a hardcoded limit of > 0x1, you'll have to fix those to have the right limit according to > the bits you leave for the ACK id. Yup, found those. Will send a new version of the series in a bit. -Toke
Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
Johannes Berg writes: > On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote: >> >> > We can also play with the units of the airtime, e.g. making that a >> > multiple of 2 or 4 us? Seems unlikely to matter much? >> >> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10 >> bits, leaving plenty of space for ACK IDs (hopefully). > > If you do need more bits (e.g. to be on the safe side and have space for > 8ms) you could also steal bits out of 'band' (we only need 3 I think) > and 'hw_queue' (not sure what the limit really is, but there aren't many > users, seems like only iwlwifi/dvm and hwsim care, and those certainly > don't need >32 queues). > > Of course if you leave more bits for later that's good too ;-) Yeah, let's leave that for later. After all, with the limits we currently have configured, if a single packet takes up 4096 us, that will trigger the per-station queue throttling in itself, so I think we'll be fine (famous last words). -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
[PATCH v3 2/4] mac80211: Import airtime calculation code from mt76
From: Toke Høiland-Jørgensen Felix recently added code to calculate airtime of packets to the mt76 driver. Import this into mac80211 so we can use it for airtime queue limit calculations later. The airtime.c file is copied verbatim from the mt76 driver, and adjusted to use mac80211 data structures instead (which is fairly straight forward). The per-rate TX rate calculation is split out to its own function (ieee80211_calc_tx_airtime_rate()) so it can be used directly for the AQL calculations added in a subsequent patch. The only thing that it was not possible to port directly was the bit that read the internal driver flags of struct ieee80211_rate to determine whether a rate is using CCK or OFDM encoding. Instead, just look at the rate index, since at least mt76 and ath10k both seem to have the same number of CCK rates (4) in their tables. Signed-off-by: Toke Høiland-Jørgensen --- include/net/mac80211.h | 14 ++ net/mac80211/Makefile |3 net/mac80211/airtime.c | 377 net/mac80211/ieee80211_i.h |4 4 files changed, 397 insertions(+), 1 deletion(-) create mode 100644 net/mac80211/airtime.c diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 4288ace72c2b..f058386e3fef 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -6424,4 +6424,18 @@ void ieee80211_nan_func_match(struct ieee80211_vif *vif, struct cfg80211_nan_match_params *match, gfp_t gfp); +/** + * ieee80211_calc_tx_airtime - calculate estimated transmission airtime. + * + * This function calculates the estimated airtime usage of a frame based on the + * rate information in the TX info struct and the frame length. + * + * @hw: pointer as obtained from ieee80211_alloc_hw() + * @info: &struct ieee80211_tx_info of the frame. + * @len: frame length in bytes + */ +u32 ieee80211_calc_tx_airtime(struct ieee80211_hw *hw, + struct ieee80211_tx_info *info, + int len); + #endif /* MAC80211_H */ diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile index 4f03ebe732fa..6cbb1286d6c0 100644 --- a/net/mac80211/Makefile +++ b/net/mac80211/Makefile @@ -32,7 +32,8 @@ mac80211-y := \ chan.o \ trace.o mlme.o \ tdls.o \ - ocb.o + ocb.o \ + airtime.o mac80211-$(CONFIG_MAC80211_LEDS) += led.o mac80211-$(CONFIG_MAC80211_DEBUGFS) += \ diff --git a/net/mac80211/airtime.c b/net/mac80211/airtime.c new file mode 100644 index ..c8d0cee61366 --- /dev/null +++ b/net/mac80211/airtime.c @@ -0,0 +1,377 @@ +// SPDX-License-Identifier: ISC +/* + * Copyright (C) 2019 Felix Fietkau + */ + +#include +#include "ieee80211_i.h" +#include "sta_info.h" + +#define AVG_PKT_SIZE 1024 + +/* Number of bits for an average sized packet */ +#define MCS_NBITS (AVG_PKT_SIZE << 3) + +/* Number of symbols for a packet with (bps) bits per symbol */ +#define MCS_NSYMS(bps) DIV_ROUND_UP(MCS_NBITS, (bps)) + +/* Transmission time (1024 usec) for a packet containing (syms) * symbols */ +#define MCS_SYMBOL_TIME(sgi, syms) \ + (sgi ? \ + ((syms) * 18 * 1024 + 4 * 1024) / 5 : /* syms * 3.6 us */ \ + ((syms) * 1024) << 2 /* syms * 4 us */ \ + ) + +/* Transmit duration for the raw data part of an average sized packet */ +#define MCS_DURATION(streams, sgi, bps) \ + MCS_SYMBOL_TIME(sgi, MCS_NSYMS((streams) * (bps))) + +#define BW_20 0 +#define BW_40 1 +#define BW_80 2 + +/* + * Define group sort order: HT40 -> SGI -> #streams + */ +#define IEEE80211_MAX_STREAMS 4 +#define IEEE80211_HT_STREAM_GROUPS 4 /* BW(=2) * SGI(=2) */ +#define IEEE80211_VHT_STREAM_GROUPS6 /* BW(=3) * SGI(=2) */ + +#define IEEE80211_HT_GROUPS_NB (IEEE80211_MAX_STREAMS *\ +IEEE80211_HT_STREAM_GROUPS) +#define IEEE80211_VHT_GROUPS_NB(IEEE80211_MAX_STREAMS *\ +IEEE80211_VHT_STREAM_GROUPS) +#define IEEE80211_GROUPS_NB(IEEE80211_HT_GROUPS_NB + \ +IEEE80211_VHT_GROUPS_NB) + +#define IEEE80211_HT_GROUP_0 0 +#define IEEE80211_VHT_GROUP_0 (IEEE80211_HT_GROUP_0 + IEEE80211_HT_GROUPS_NB) + +#define MCS_GROUP_RATES10 +#define CCK_NUM_RATES 4 + +#define HT_GROUP_IDX(_streams, _sgi, _ht40)\ + IEEE80211_HT_GROUP_0 + \ + IEEE80211_MAX_STREAMS * 2 * _ht40 + \ + IEEE80211_MAX_STREAMS * _sgi + \ + _streams - 1 + +#define _MAX(a, b) (((a)>(b))?(a):(b)) + +#define GROUP_SHIFT(duration) \ + _MAX(0, 16 - __builtin_clz(duration)) + +/* MCS
[PATCH v3 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
From: Kan Yan
In order for the Fq_CoDel integrated in mac80211 layer operates effectively
to control excessive queueing latency, the CoDel algorithm requires an
accurate measure of how long the packets stays in the queue, aka sojourn
time. The sojourn time measured at mac80211 layer doesn't include queueing
latency in lower layer (firmware/hardware) and CoDel expects lower layer to
have a short queue. However, most 802.11ac chipsets offload tasks such TX
aggregation to firmware or hardware, thus have a deep lower layer queue.
Without a mechanism to control the lower layer queue size, packets only
stays in mac80211 layer transiently before being sent to firmware queue.
As a result, the sojourn time measured by CoDel in the mac80211 layer is
almost always lower than the CoDel latency target, hence CoDel does little
to control the latency, even when the lower layer queue causes excessive
latency.
Byte Queue limits (BQL) is commonly used to address the similar issue with
wired network interface. However, this method cannot be applied directly
to the wireless network interface. Byte is not a suitable measure of queue
depth in the wireless network, as the data rate can vary dramatically from
station to station in the same network, from a few Mbps to over Gbps.
This patch implemented an Airtime-based Queue Limit (AQL) to make CoDel
works effectively with wireless drivers that utilized firmware/hardware
offloading. AQL only allows each txq to release just enough packets to the
lower layer to form 1-2 large aggregations to keep hardware fully utilized
and keep the rest of frames in mac80211 layer to be controlled by the CoDel
algorithm.
Signed-off-by: Kan Yan
[ Toke: Get rid of the driver API to set pending airtime ]
Signed-off-by: Toke Høiland-Jørgensen
---
include/net/cfg80211.h |7
include/net/mac80211.h | 12 +++
net/mac80211/debugfs.c | 78
net/mac80211/debugfs_sta.c | 43 +++-
net/mac80211/ieee80211_i.h |4 ++
net/mac80211/main.c|9 +
net/mac80211/sta_info.c| 32 ++
net/mac80211/sta_info.h|8 +
net/mac80211/tx.c | 46 --
9 files changed, 225 insertions(+), 14 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ff45c3e1abff..8d50c0a60dbd 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2602,6 +2602,13 @@ enum wiphy_params_flags {
#define IEEE80211_DEFAULT_AIRTIME_WEIGHT 256
+/* The per TXQ device queue limit in airtime */
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L 4000
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H 8000
+
+/* The per interface airtime threshold to switch to lower queue limit */
+#define IEEE80211_AQL_THRESHOLD24000
+
/**
* struct cfg80211_pmksa - PMK Security Association
*
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index f058386e3fef..fdfa5805e1cf 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5565,6 +5565,18 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta
*pubsta, int tid);
void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
u32 tx_airtime, u32 rx_airtime);
+/**
+ * ieee80211_txq_airtime_check - check if a txq can send frame to device
+ *
+ * @hw: pointer obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Return true if the AQL's airtime limit has not been reached and the txq can
+ * continue to send more packets to the device. Otherwise return false.
+ */
+bool
+ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq
*txq);
+
/**
* ieee80211_iter_keys - iterate keys programmed into the device
* @hw: pointer obtained from ieee80211_alloc_hw()
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 568b3b276931..d77ea0e51c1d 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -148,6 +148,80 @@ static const struct file_operations aqm_ops = {
.llseek = default_llseek,
};
+static ssize_t aql_txq_limit_read(struct file *file,
+ char __user *user_buf,
+ size_t count,
+ loff_t *ppos)
+{
+ struct ieee80211_local *local = file->private_data;
+ char buf[400];
+ int len = 0;
+
+ len = scnprintf(buf, sizeof(buf),
+ "AC AQL limit low AQL limit high\n"
+ "VO %u %u\n"
+ "VI %u %u\n"
+ "BE %u %u\n"
+ "BK %u %u\n",
+ local->aql_txq_limit_low[IEEE80211_AC_VO],
+ local->aql_txq_limit_hi
[PATCH v3 1/4] mac80211: Shrink the size of ack_frame_id to make room for tx_time_est
From: Toke Høiland-Jørgensen
To implement airtime queue limiting, we need to keep a running account of
the estimated airtime of all skbs queued into the device. Do to this
correctly, we need to store the airtime estimate into the skb so we can
decrease the outstanding balance when the skb is freed. This means that the
time estimate must be stored somewhere that will survive for the lifetime
of the skb.
To get this, decrease the size of the ack_frame_id field to 6 bits, and
lower the size of the ID space accordingly. This leaves 10 bits for use for
tx_time_est, which is enough to store a maximum of 4096 us, if we shift the
values so they become units of 4us.
Signed-off-by: Toke Høiland-Jørgensen
---
include/net/mac80211.h |4 +++-
net/mac80211/cfg.c |2 +-
net/mac80211/tx.c |2 +-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d69081c38788..4288ace72c2b 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -967,6 +967,7 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate
*rate)
* @band: the band to transmit on (use for checking for races)
* @hw_queue: HW queue to put the frame on, skb_get_queue_mapping() gives the
AC
* @ack_frame_id: internal frame ID for TX status, used internally
+ * @tx_time_est: TX time estimate in units of 4us, used internally
* @control: union part for control data
* @control.rates: TX rates array to try
* @control.rts_cts_rate_idx: rate for RTS or CTS
@@ -1007,7 +1008,8 @@ struct ieee80211_tx_info {
u8 hw_queue;
- u16 ack_frame_id;
+ u16 ack_frame_id:6;
+ u16 tx_time_est:10;
union {
struct {
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 70739e746c13..4fb7f1f12109 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3428,7 +3428,7 @@ int ieee80211_attach_ack_skb(struct ieee80211_local
*local, struct sk_buff *skb,
spin_lock_irqsave(&local->ack_status_lock, spin_flags);
id = idr_alloc(&local->ack_status_frames, ack_skb,
- 1, 0x1, GFP_ATOMIC);
+ 1, 0x40, GFP_ATOMIC);
spin_unlock_irqrestore(&local->ack_status_lock, spin_flags);
if (id < 0) {
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 938c10f7955b..a16c2f863702 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2719,7 +2719,7 @@ static struct sk_buff *ieee80211_build_hdr(struct
ieee80211_sub_if_data *sdata,
spin_lock_irqsave(&local->ack_status_lock, flags);
id = idr_alloc(&local->ack_status_frames, ack_skb,
- 1, 0x1, GFP_ATOMIC);
+ 1, 0x40, GFP_ATOMIC);
spin_unlock_irqrestore(&local->ack_status_lock, flags);
if (id >= 0) {
[PATCH v3 0/4] Add Airtime Queue Limits (AQL) to mac80211
This series is a first attempt at porting the Airtime Queue Limits concept from the out-of-tree ath10k implementation[0] to mac80211. This version takes Kan's patch to do the throttling in mac80211, and replaces the driver API with the mechanism from the previous version of my series, which instead calculated the expected airtime at dequeue time inside mac80211, storing it in the SKB cb field. This series also imports Felix' airtime calculation code from mt76 into mac80211, adjusting the API so it can be used from TX dequeue, by extracting the latest TX rate from the tx_stats structure kept for each station. As before, I've only compile tested this (lacking the proper hardware to do more testing). So I'm hoping someone with a proper testing setup can take the whole thing for a spin... :) The series is also available in my git repo here: https://git.kernel.org/pub/scm/linux/kernel/git/toke/linux.git/log/?h=mac80211-aql-02 Changelog: v3: - Move the tx_time_est field so it's shared with ack_frame_id, and use units of 4us for the value stored in it. - Move the addition of the Ethernet header size into ieee80211_calc_expected_tx_airtime() v2: - Integrate Kan's approach to airtime throttling. - Hopefully fix the cb struct alignment on big-endian architectures. --- Kan Yan (1): mac80211: Implement Airtime-based Queue Limit (AQL) Toke Høiland-Jørgensen (3): mac80211: Shrink the size of ack_frame_id to make room for tx_time_est mac80211: Import airtime calculation code from mt76 mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue include/net/cfg80211.h |7 + include/net/mac80211.h | 30 +++- net/mac80211/Makefile |3 net/mac80211/airtime.c | 377 net/mac80211/cfg.c |2 net/mac80211/debugfs.c | 78 + net/mac80211/debugfs_sta.c | 43 - net/mac80211/ieee80211_i.h |8 + net/mac80211/main.c|9 + net/mac80211/sta_info.c| 32 net/mac80211/sta_info.h|8 + net/mac80211/status.c | 38 net/mac80211/tx.c | 67 +++- 13 files changed, 684 insertions(+), 18 deletions(-) create mode 100644 net/mac80211/airtime.c
[PATCH v3 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
From: Toke Høiland-Jørgensen
The previous commit added the ability to throttle stations when they queue
too much airtime in the hardware. This commit enables the functionality by
calculating the expected airtime usage of each packet that is dequeued from
the TXQs in mac80211, and accounting that as pending airtime.
The estimated airtime for each skb is stored in the tx_info, so we can
subtract the same amount from the running total when the skb is freed or
recycled. The throttling mechanism relies on this accounting to be
accurate (i.e., that we are not freeing skbs without subtracting any
airtime they were accounted for), so we put the subtraction into
ieee80211_report_used_skb(). As an optimisation, we also subtract the
airtime on regular TX completion, zeroing out the value stored in the
packet afterwards, to avoid having to do an expensive lookup of the
station from the packet data on every packet.
This patch does *not* include any mechanism to wake a throttled TXQ again,
on the assumption that this will happen anyway as a side effect of whatever
freed the skb (most commonly a TX completion).
Signed-off-by: Toke Høiland-Jørgensen
---
net/mac80211/status.c | 38 ++
net/mac80211/tx.c | 19 +++
2 files changed, 57 insertions(+)
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index ab8ba5835ca0..905b4afd457d 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -676,6 +676,33 @@ static void ieee80211_report_used_skb(struct
ieee80211_local *local,
if (dropped)
acked = false;
+ if (info->tx_time_est) {
+ struct ieee80211_sub_if_data *sdata;
+ struct sta_info *sta = NULL;
+ u8 *qc, ac;
+ int tid;
+
+ rcu_read_lock();
+
+ sdata = ieee80211_sdata_from_skb(local, skb);
+ if (sdata)
+ sta = sta_info_get_bss(sdata, skb_mac_header(skb));
+
+ if (ieee80211_is_data_qos(hdr->frame_control)) {
+ qc = ieee80211_get_qos_ctl(hdr);
+ tid = qc[0] & 0xf;
+ ac = ieee80211_ac_from_tid(tid);
+ } else {
+ ac = IEEE80211_AC_BE;
+ }
+
+ ieee80211_sta_update_pending_airtime(local, sta, ac,
+info->tx_time_est << 2,
+true);
+ rcu_read_unlock();
+
+ }
+
if (info->flags & IEEE80211_TX_INTFL_MLME_CONN_TX) {
struct ieee80211_sub_if_data *sdata;
@@ -930,6 +957,17 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
tid = qc[0] & 0xf;
}
+ if (info->tx_time_est) {
+ /* Do this here to avoid the expensive lookup of the sta
+* in ieee80211_report_used_skb().
+*/
+ ieee80211_sta_update_pending_airtime(local, sta,
+
ieee80211_ac_from_tid(tid),
+info->tx_time_est
<< 2,
+true);
+ info->tx_time_est = 0;
+ }
+
if (!acked && ieee80211_is_back_req(fc)) {
u16 control;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 12653d873b8c..bd8284f04361 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3539,9 +3539,14 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw
*hw,
struct ieee80211_tx_data tx;
ieee80211_tx_result r;
struct ieee80211_vif *vif = txq->vif;
+ u8 ac = txq->ac;
+ u32 airtime;
WARN_ON_ONCE(softirq_count() == 0);
+ if (!ieee80211_txq_airtime_check(hw, txq))
+ return NULL;
+
begin:
spin_lock_bh(&fq->lock);
@@ -3652,6 +3657,20 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw
*hw,
}
IEEE80211_SKB_CB(skb)->control.vif = vif;
+
+ if (local->airtime_flags & AIRTIME_USE_AQL) {
+ airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
+skb->len);
+ if (airtime) {
+ /* We only have 10 bits in tx_time_est, so store airtime
+* in increments of 4 us and clamp that to 2**10.
+*/
+ info->tx_time_est = min_t(u32, airtime >> 2, 1 << 10);
+ ieee80211_sta_update_pending_airtime(local, tx.sta, ac,
+
Re: [PATCH v3 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
Kan Yan writes:
>> + if (local->airtime_flags & AIRTIME_USE_AQL) {
>> + airtime = ieee80211_calc_expected_tx_airtime(hw, vif,
>> txq->sta,
>> +skb->len);
>> + if (airtime) {
>> + /* We only have 10 bits in tx_time_est, so store
>> airtime
>> +* in increments of 4 us and clamp that to 2**10.
>> +*/
>> + info->tx_time_est = min_t(u32, airtime >> 2, 1 <<
>> 10);
>> + ieee80211_sta_update_pending_airtime(local, tx.sta,
>> ac,
>> +airtime, false);
>> + }
>> + }
>> +
>
> It should be:
> ieee80211_sta_update_pending_airtime(local, tx.sta,
> ac,
>
> info->tx_time_est << 2, false);
>
> The airtime rounded to 4us (info->tx_time_est << 2), instead of the
> original airtime should be used when registering the pending airtime,
> to keep it consistent with airtime subtracted when the skb is freed.
Yes, I realised that last night as well. The rounding is also off (max
is 2**10-1, not 2**10. Will send a v4 :)
-Toke
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
[PATCH v4 2/4] mac80211: Import airtime calculation code from mt76
From: Toke Høiland-Jørgensen Felix recently added code to calculate airtime of packets to the mt76 driver. Import this into mac80211 so we can use it for airtime queue limit calculations later. The airtime.c file is copied verbatim from the mt76 driver, and adjusted to use mac80211 data structures instead (which is fairly straight forward). The per-rate TX rate calculation is split out to its own function (ieee80211_calc_tx_airtime_rate()) so it can be used directly for the AQL calculations added in a subsequent patch. The only thing that it was not possible to port directly was the bit that read the internal driver flags of struct ieee80211_rate to determine whether a rate is using CCK or OFDM encoding. Instead, just look at the rate index, since at least mt76 and ath10k both seem to have the same number of CCK rates (4) in their tables. Signed-off-by: Toke Høiland-Jørgensen --- include/net/mac80211.h | 14 ++ net/mac80211/Makefile |3 net/mac80211/airtime.c | 377 net/mac80211/ieee80211_i.h |4 4 files changed, 397 insertions(+), 1 deletion(-) create mode 100644 net/mac80211/airtime.c diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 4288ace72c2b..f058386e3fef 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -6424,4 +6424,18 @@ void ieee80211_nan_func_match(struct ieee80211_vif *vif, struct cfg80211_nan_match_params *match, gfp_t gfp); +/** + * ieee80211_calc_tx_airtime - calculate estimated transmission airtime. + * + * This function calculates the estimated airtime usage of a frame based on the + * rate information in the TX info struct and the frame length. + * + * @hw: pointer as obtained from ieee80211_alloc_hw() + * @info: &struct ieee80211_tx_info of the frame. + * @len: frame length in bytes + */ +u32 ieee80211_calc_tx_airtime(struct ieee80211_hw *hw, + struct ieee80211_tx_info *info, + int len); + #endif /* MAC80211_H */ diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile index 4f03ebe732fa..6cbb1286d6c0 100644 --- a/net/mac80211/Makefile +++ b/net/mac80211/Makefile @@ -32,7 +32,8 @@ mac80211-y := \ chan.o \ trace.o mlme.o \ tdls.o \ - ocb.o + ocb.o \ + airtime.o mac80211-$(CONFIG_MAC80211_LEDS) += led.o mac80211-$(CONFIG_MAC80211_DEBUGFS) += \ diff --git a/net/mac80211/airtime.c b/net/mac80211/airtime.c new file mode 100644 index ..c8d0cee61366 --- /dev/null +++ b/net/mac80211/airtime.c @@ -0,0 +1,377 @@ +// SPDX-License-Identifier: ISC +/* + * Copyright (C) 2019 Felix Fietkau + */ + +#include +#include "ieee80211_i.h" +#include "sta_info.h" + +#define AVG_PKT_SIZE 1024 + +/* Number of bits for an average sized packet */ +#define MCS_NBITS (AVG_PKT_SIZE << 3) + +/* Number of symbols for a packet with (bps) bits per symbol */ +#define MCS_NSYMS(bps) DIV_ROUND_UP(MCS_NBITS, (bps)) + +/* Transmission time (1024 usec) for a packet containing (syms) * symbols */ +#define MCS_SYMBOL_TIME(sgi, syms) \ + (sgi ? \ + ((syms) * 18 * 1024 + 4 * 1024) / 5 : /* syms * 3.6 us */ \ + ((syms) * 1024) << 2 /* syms * 4 us */ \ + ) + +/* Transmit duration for the raw data part of an average sized packet */ +#define MCS_DURATION(streams, sgi, bps) \ + MCS_SYMBOL_TIME(sgi, MCS_NSYMS((streams) * (bps))) + +#define BW_20 0 +#define BW_40 1 +#define BW_80 2 + +/* + * Define group sort order: HT40 -> SGI -> #streams + */ +#define IEEE80211_MAX_STREAMS 4 +#define IEEE80211_HT_STREAM_GROUPS 4 /* BW(=2) * SGI(=2) */ +#define IEEE80211_VHT_STREAM_GROUPS6 /* BW(=3) * SGI(=2) */ + +#define IEEE80211_HT_GROUPS_NB (IEEE80211_MAX_STREAMS *\ +IEEE80211_HT_STREAM_GROUPS) +#define IEEE80211_VHT_GROUPS_NB(IEEE80211_MAX_STREAMS *\ +IEEE80211_VHT_STREAM_GROUPS) +#define IEEE80211_GROUPS_NB(IEEE80211_HT_GROUPS_NB + \ +IEEE80211_VHT_GROUPS_NB) + +#define IEEE80211_HT_GROUP_0 0 +#define IEEE80211_VHT_GROUP_0 (IEEE80211_HT_GROUP_0 + IEEE80211_HT_GROUPS_NB) + +#define MCS_GROUP_RATES10 +#define CCK_NUM_RATES 4 + +#define HT_GROUP_IDX(_streams, _sgi, _ht40)\ + IEEE80211_HT_GROUP_0 + \ + IEEE80211_MAX_STREAMS * 2 * _ht40 + \ + IEEE80211_MAX_STREAMS * _sgi + \ + _streams - 1 + +#define _MAX(a, b) (((a)>(b))?(a):(b)) + +#define GROUP_SHIFT(duration) \ + _MAX(0, 16 - __builtin_clz(duration)) + +/* MCS
[PATCH v4 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
From: Toke Høiland-Jørgensen
The previous commit added the ability to throttle stations when they queue
too much airtime in the hardware. This commit enables the functionality by
calculating the expected airtime usage of each packet that is dequeued from
the TXQs in mac80211, and accounting that as pending airtime.
The estimated airtime for each skb is stored in the tx_info, so we can
subtract the same amount from the running total when the skb is freed or
recycled. The throttling mechanism relies on this accounting to be
accurate (i.e., that we are not freeing skbs without subtracting any
airtime they were accounted for), so we put the subtraction into
ieee80211_report_used_skb(). As an optimisation, we also subtract the
airtime on regular TX completion, zeroing out the value stored in the
packet afterwards, to avoid having to do an expensive lookup of the
station from the packet data on every packet.
This patch does *not* include any mechanism to wake a throttled TXQ again,
on the assumption that this will happen anyway as a side effect of whatever
freed the skb (most commonly a TX completion).
Signed-off-by: Toke Høiland-Jørgensen
---
net/mac80211/status.c | 38 ++
net/mac80211/tx.c | 21 +
2 files changed, 59 insertions(+)
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index ab8ba5835ca0..905b4afd457d 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -676,6 +676,33 @@ static void ieee80211_report_used_skb(struct
ieee80211_local *local,
if (dropped)
acked = false;
+ if (info->tx_time_est) {
+ struct ieee80211_sub_if_data *sdata;
+ struct sta_info *sta = NULL;
+ u8 *qc, ac;
+ int tid;
+
+ rcu_read_lock();
+
+ sdata = ieee80211_sdata_from_skb(local, skb);
+ if (sdata)
+ sta = sta_info_get_bss(sdata, skb_mac_header(skb));
+
+ if (ieee80211_is_data_qos(hdr->frame_control)) {
+ qc = ieee80211_get_qos_ctl(hdr);
+ tid = qc[0] & 0xf;
+ ac = ieee80211_ac_from_tid(tid);
+ } else {
+ ac = IEEE80211_AC_BE;
+ }
+
+ ieee80211_sta_update_pending_airtime(local, sta, ac,
+info->tx_time_est << 2,
+true);
+ rcu_read_unlock();
+
+ }
+
if (info->flags & IEEE80211_TX_INTFL_MLME_CONN_TX) {
struct ieee80211_sub_if_data *sdata;
@@ -930,6 +957,17 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
tid = qc[0] & 0xf;
}
+ if (info->tx_time_est) {
+ /* Do this here to avoid the expensive lookup of the sta
+* in ieee80211_report_used_skb().
+*/
+ ieee80211_sta_update_pending_airtime(local, sta,
+
ieee80211_ac_from_tid(tid),
+info->tx_time_est
<< 2,
+true);
+ info->tx_time_est = 0;
+ }
+
if (!acked && ieee80211_is_back_req(fc)) {
u16 control;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 12653d873b8c..b8ff56d1d661 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3542,6 +3542,9 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw
*hw,
WARN_ON_ONCE(softirq_count() == 0);
+ if (!ieee80211_txq_airtime_check(hw, txq))
+ return NULL;
+
begin:
spin_lock_bh(&fq->lock);
@@ -3652,6 +3655,24 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw
*hw,
}
IEEE80211_SKB_CB(skb)->control.vif = vif;
+
+ if (local->airtime_flags & AIRTIME_USE_AQL) {
+ u32 airtime;
+
+ airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
+skb->len);
+ if (airtime) {
+ /* We only have 10 bits in tx_time_est, so store airtime
+* in increments of 4 us and clamp the maximum to
2**12-1
+*/
+ airtime = min_t(u32, airtime, 4095) & ~3U;
+ info->tx_time_est = airtime >> 2;
+ ieee80211_sta_update_pending_airtime(local, tx.sta,
+txq->ac, airtime,
+false);
+ }
+ }
+
return skb;
out:
[PATCH v4 1/4] mac80211: Shrink the size of ack_frame_id to make room for tx_time_est
From: Toke Høiland-Jørgensen
To implement airtime queue limiting, we need to keep a running account of
the estimated airtime of all skbs queued into the device. Do to this
correctly, we need to store the airtime estimate into the skb so we can
decrease the outstanding balance when the skb is freed. This means that the
time estimate must be stored somewhere that will survive for the lifetime
of the skb.
To get this, decrease the size of the ack_frame_id field to 6 bits, and
lower the size of the ID space accordingly. This leaves 10 bits for use for
tx_time_est, which is enough to store a maximum of 4096 us, if we shift the
values so they become units of 4us.
Signed-off-by: Toke Høiland-Jørgensen
---
include/net/mac80211.h |4 +++-
net/mac80211/cfg.c |2 +-
net/mac80211/tx.c |2 +-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d69081c38788..4288ace72c2b 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -967,6 +967,7 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate
*rate)
* @band: the band to transmit on (use for checking for races)
* @hw_queue: HW queue to put the frame on, skb_get_queue_mapping() gives the
AC
* @ack_frame_id: internal frame ID for TX status, used internally
+ * @tx_time_est: TX time estimate in units of 4us, used internally
* @control: union part for control data
* @control.rates: TX rates array to try
* @control.rts_cts_rate_idx: rate for RTS or CTS
@@ -1007,7 +1008,8 @@ struct ieee80211_tx_info {
u8 hw_queue;
- u16 ack_frame_id;
+ u16 ack_frame_id:6;
+ u16 tx_time_est:10;
union {
struct {
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 70739e746c13..4fb7f1f12109 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3428,7 +3428,7 @@ int ieee80211_attach_ack_skb(struct ieee80211_local
*local, struct sk_buff *skb,
spin_lock_irqsave(&local->ack_status_lock, spin_flags);
id = idr_alloc(&local->ack_status_frames, ack_skb,
- 1, 0x1, GFP_ATOMIC);
+ 1, 0x40, GFP_ATOMIC);
spin_unlock_irqrestore(&local->ack_status_lock, spin_flags);
if (id < 0) {
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 938c10f7955b..a16c2f863702 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2719,7 +2719,7 @@ static struct sk_buff *ieee80211_build_hdr(struct
ieee80211_sub_if_data *sdata,
spin_lock_irqsave(&local->ack_status_lock, flags);
id = idr_alloc(&local->ack_status_frames, ack_skb,
- 1, 0x1, GFP_ATOMIC);
+ 1, 0x40, GFP_ATOMIC);
spin_unlock_irqrestore(&local->ack_status_lock, flags);
if (id >= 0) {
[PATCH v4 0/4] Add Airtime Queue Limits (AQL) to mac80211
This series is a first attempt at porting the Airtime Queue Limits concept from the out-of-tree ath10k implementation[0] to mac80211. This version takes Kan's patch to do the throttling in mac80211, and replaces the driver API with the mechanism from the previous version of my series, which instead calculated the expected airtime at dequeue time inside mac80211, storing it in the SKB cb field. This series also imports Felix' airtime calculation code from mt76 into mac80211, adjusting the API so it can be used from TX dequeue, by extracting the latest TX rate from the tx_stats structure kept for each station. As before, I've only compile tested this (lacking the proper hardware to do more testing). So I'm hoping someone with a proper testing setup can take the whole thing for a spin... :) The series is also available in my git repo here: https://git.kernel.org/pub/scm/linux/kernel/git/toke/linux.git/log/?h=mac80211-aql-02 Changelog: v4: - Fix calculation that clamps the maximum airtime to fit into 10 bits - Incorporate Rich Brown's nits for the commit message in Kan's patch - Add fewer local variables to ieee80211_tx_dequeue() v3: - Move the tx_time_est field so it's shared with ack_frame_id, and use units of 4us for the value stored in it. - Move the addition of the Ethernet header size into ieee80211_calc_expected_tx_airtime() v2: - Integrate Kan's approach to airtime throttling. - Hopefully fix the cb struct alignment on big-endian architectures. --- Kan Yan (1): mac80211: Implement Airtime-based Queue Limit (AQL) Toke Høiland-Jørgensen (3): mac80211: Shrink the size of ack_frame_id to make room for tx_time_est mac80211: Import airtime calculation code from mt76 mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue include/net/cfg80211.h |7 + include/net/mac80211.h | 30 +++- net/mac80211/Makefile |3 net/mac80211/airtime.c | 377 net/mac80211/cfg.c |2 net/mac80211/debugfs.c | 78 + net/mac80211/debugfs_sta.c | 43 - net/mac80211/ieee80211_i.h |8 + net/mac80211/main.c|9 + net/mac80211/sta_info.c| 32 net/mac80211/sta_info.h|8 + net/mac80211/status.c | 38 net/mac80211/tx.c | 69 13 files changed, 686 insertions(+), 18 deletions(-) create mode 100644 net/mac80211/airtime.c
[PATCH v4 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
From: Kan Yan
In order for the Fq_CoDel algorithm integrated in mac80211 layer to operate
effectively to control excessive queueing latency, the CoDel algorithm
requires an accurate measure of how long packets stays in the queue, AKA
sojourn time. The sojourn time measured at the mac80211 layer doesn't
include queueing latency in the lower layer (firmware/hardware) and CoDel
expects lower layer to have a short queue. However, most 802.11ac chipsets
offload tasks such TX aggregation to firmware or hardware, thus have a deep
lower layer queue.
Without a mechanism to control the lower layer queue size, packets only
stay in mac80211 layer transiently before being sent to firmware queue.
As a result, the sojourn time measured by CoDel in the mac80211 layer is
almost always lower than the CoDel latency target, hence CoDel does little
to control the latency, even when the lower layer queue causes excessive
latency.
The Byte Queue Limits (BQL) mechanism is commonly used to address the
similar issue with wired network interface. However, this method cannot be
applied directly to the wireless network interface. "Bytes" is not a
suitable measure of queue depth in the wireless network, as the data rate
can vary dramatically from station to station in the same network, from a
few Mbps to over Gbps.
This patch implements an Airtime-based Queue Limit (AQL) to make CoDel work
effectively with wireless drivers that utilized firmware/hardware
offloading. AQL allows each txq to release just enough packets to the lower
layer to form 1-2 large aggregations to keep hardware fully utilized and
retains the rest of the frames in mac80211 layer to be controlled by the
CoDel algorithm.
Signed-off-by: Kan Yan
[ Toke: Keep API to set pending airtime internal, fix nits in commit msg ]
Signed-off-by: Toke Høiland-Jørgensen
---
include/net/cfg80211.h |7
include/net/mac80211.h | 12 +++
net/mac80211/debugfs.c | 78
net/mac80211/debugfs_sta.c | 43 +++-
net/mac80211/ieee80211_i.h |4 ++
net/mac80211/main.c|9 +
net/mac80211/sta_info.c| 32 ++
net/mac80211/sta_info.h|8 +
net/mac80211/tx.c | 46 --
9 files changed, 225 insertions(+), 14 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ff45c3e1abff..8d50c0a60dbd 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2602,6 +2602,13 @@ enum wiphy_params_flags {
#define IEEE80211_DEFAULT_AIRTIME_WEIGHT 256
+/* The per TXQ device queue limit in airtime */
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L 4000
+#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H 8000
+
+/* The per interface airtime threshold to switch to lower queue limit */
+#define IEEE80211_AQL_THRESHOLD24000
+
/**
* struct cfg80211_pmksa - PMK Security Association
*
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index f058386e3fef..fdfa5805e1cf 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5565,6 +5565,18 @@ void ieee80211_send_eosp_nullfunc(struct ieee80211_sta
*pubsta, int tid);
void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
u32 tx_airtime, u32 rx_airtime);
+/**
+ * ieee80211_txq_airtime_check - check if a txq can send frame to device
+ *
+ * @hw: pointer obtained from ieee80211_alloc_hw()
+ * @txq: pointer obtained from station or virtual interface
+ *
+ * Return true if the AQL's airtime limit has not been reached and the txq can
+ * continue to send more packets to the device. Otherwise return false.
+ */
+bool
+ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_txq
*txq);
+
/**
* ieee80211_iter_keys - iterate keys programmed into the device
* @hw: pointer obtained from ieee80211_alloc_hw()
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 568b3b276931..d77ea0e51c1d 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -148,6 +148,80 @@ static const struct file_operations aqm_ops = {
.llseek = default_llseek,
};
+static ssize_t aql_txq_limit_read(struct file *file,
+ char __user *user_buf,
+ size_t count,
+ loff_t *ppos)
+{
+ struct ieee80211_local *local = file->private_data;
+ char buf[400];
+ int len = 0;
+
+ len = scnprintf(buf, sizeof(buf),
+ "AC AQL limit low AQL limit high\n"
+ "VO %u %u\n"
+ "VI %u %u\n"
+ "BE %u %u\n"
+ "BK %u %u\n",
+ local->aql_txq_limit_low[IEEE802
