| Message ID | 153635803319.14170.10011969968767927187.stgit@alrua-x1 (mailing list archive) |
|---|---|
| Headers | show |
| Series | Move TXQ scheduling into mac80211 | expand |
Hi Toke, It is great to see finally there will be a general airtime fairness support in mac80211. I feel this patch set could still use some improvements to make it works better with 11ac drivers like ath10k. I did a version of airtime fairness in the ath10k driver that used in Google Wifi and I'd like to share a few things I learned from this experience. Airtime fairness is a cool feature, but that's not the motivation for me to implement that. The goal is to solve the bufferbloat problem at wireless interface. Fq_CoDel has been proved to be very effective in fixing bufferbloat for wired interface, and it has been integrated with mac802.11 for more than 2 years. However, it still does not work well with 11ac drivers like ath10k, which relies heavily on firmware to do most of transmit scheduling, and hence has deep firmware queue. FQ_CoDel expects a thin driver layer queue so it can get an accurate measurement of sojourn time and use the sojourn time to control latency. However, the queue in ath10k firmware is so deep, that packets only stay in mac80211 layer queue transiently. The sojourn time is almost always below target, and hence CoDel doesn't do much to control the latency. The key to make Fq_Codel works effectively with 11ac wireless driver is to limit the queue depth in lower layer. It looks to me this patch set only enforce fairness among stations (airtime wise), but did not limit how many packets can be released to firmware/hardware. When a txq run out of its airtime deficit, the txq is put to the back of the list but will get a refill of airtime deficit in next round. It doesn't keep counts of the total pending airtime released to hardware or firmware. Packets will be released as long as wireless driver keeps calling the dequeue function. This maybe not an issue for ath9k, because the number of available tx descriptors is more limited. However, for 11ac drivers like ath10k, it routinely queues thousands packets when the bandwidth is oversubscribed in my test. The version I did is to use pending airtime to restrict the firmware queue depth. Another difference is it does not use airtime reported from firmware, which is "past" or spent airtime, but use "future" airtime, the estimated airtime calculated using last reported tx rate for the inflight packets pending in the firmware queue. Host driver keeps counts of total pending airtime for each TxQ (per station, per AC). It tries to release up to 4-8 ms worth of frames for each TxQ. The idea is to use airtime accounting to give firmware just enough frames to keep it busy and form good aggregations, and keeps the rest of frames in mac80211 layer queues so Fq_Codel can work on it to control latency. Here are some of the patches from the version I did in ath10k for kernel in ChromeOs. CHROMIUM: net: mac80211: Recording last tx bitrate. https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/588189 CHROMIUM: ath10k: Implementing airtime fairness based TX scheduler. https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/588190/13 CHROMIUM: ath10k: use frame count as defict for fq_codel. https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/588192/13 I had some discussions with Rajkumar on how to adapt some methods from my patch and make it fit with the new TXQ scheduling API proposed here. The major issue is the lack of pending airtime accounting. It could be done in individual wireless drivers, such as in ath10k, or even in firmware, but implement it in mac80211 could be a better choice than do it one driver at a time. Thanks, Kan On Fri, Sep 7, 2018 at 3:22 PM, Toke Høiland-Jørgensen <toke@toke.dk> wrote: > This is an updated version of the patch series to move TXQ scheduling and > airtime fairness scheduling into mac80211. I've only compile tested this > version, but thought it was better to get the conversation moving instead > of being blocked on me. > > This addresses most of the comments from the last round. Specifically: > > - It folds in Rajkumar's patches to keep per-AC TXQ lists, and to add an > API that ath10k can use to check if a TXQ may transmit according even > when not using get_next_txq(). I changed a few names and descriptions, > but otherwise it's mostly the same. After the discussions we had in the > last series, I *think* it will work this way, but I'm not entirely sure. > > - I got rid of any mention of seqno in next_txq() and schedule_txq() - and > removed the third parameter to schedule_txq() entirely, so drivers can no > longer signal that a TXQ should be allowed to re-appear in a scheduling > round. We can add that back if needed. > > - Added a helper function to schedule and wake TXQs in a single call, for > internal mac80211 use. > > - Changed the default station weight to 256 and got rid of the per-phy > quantum. This makes it possible to lower station weights without having > to change the weights of every other station. > > A few things that were discussed in the last round that I did *not* change: > > - I did not add any locking around next_txq(); the driver is still supposed > to maintain a lock that prevents two threads from trying to schedule the > same AC at the same time. This is what drivers already do, so I figured it > was easier to just keep it that way rather than do it in mac80211. > > - I didn't get rid of the register_airtime() callback. As far as I can tell, > only iwlwifi uses the tx_time field in the struct tx_info. Which means that > we *could* probably use it for this and just make the other drivers set it; > but I'm not sure what effects that would have in relation to WMM-AC for > those drivers, so I chickened out. Will have to try it out, I guess; but it > also depends on whether ath10k needs to be able to report airtime > asynchronously anyway. So I'll hold off on that for a bit more. > > -Toke > > --- > > Toke Høiland-Jørgensen (4): > mac80211: Add TXQ scheduling API > mac80211: Add airtime accounting and scheduling to TXQs > cfg80211: Add airtime statistics and settings > ath9k: Switch to mac80211 TXQ scheduling and airtime APIs > > > 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 | 54 ------ > drivers/net/wireless/ath/ath9k/init.c | 3 > drivers/net/wireless/ath/ath9k/recv.c | 9 - > drivers/net/wireless/ath/ath9k/xmit.c | 245 ++++++++-------------------- > include/net/cfg80211.h | 12 + > include/net/mac80211.h | 97 +++++++++++ > include/uapi/linux/nl80211.h | 15 ++ > net/mac80211/agg-tx.c | 2 > net/mac80211/cfg.c | 3 > net/mac80211/debugfs.c | 3 > net/mac80211/debugfs_sta.c | 42 ++++- > net/mac80211/driver-ops.h | 7 + > net/mac80211/ieee80211_i.h | 11 + > net/mac80211/main.c | 5 + > net/mac80211/sta_info.c | 49 +++++- > net/mac80211/sta_info.h | 13 + > net/mac80211/tx.c | 125 ++++++++++++++ > net/wireless/nl80211.c | 25 +++ > 21 files changed, 471 insertions(+), 274 deletions(-) > > X-Clacks-Overhead: GNU Terry Pratchett
On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: > - I didn't get rid of the register_airtime() callback. As far as I can tell, > only iwlwifi uses the tx_time field in the struct tx_info. Which means that > we *could* probably use it for this and just make the other drivers set it; > but I'm not sure what effects that would have in relation to WMM-AC for > those drivers, so I chickened out. Will have to try it out, I guess; but it > also depends on whether ath10k needs to be able to report airtime > asynchronously anyway. So I'll hold off on that for a bit more. I don't think you need to be concerned, the reporting through this has no immediate effect as the driver would also have to set the feature flag (NL80211_FEATURE_SUPPORTS_WMM_ADMISSION) for userspace to be able to use WMM admission TSPECs, and getting tx_tspec->admitted_time to be non-zero in ieee80211_sta_tx_wmm_ac_notify(). I just think that this may be desirable to drivers eventually, and/or maybe iwlwifi wants to take advantage of the airtime scheduling eventually, so having two APIs overlapping seems a bit strange. johannes
On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: > > A few things that were discussed in the last round that I did *not* change: Thanks for this list btw. > - I did not add any locking around next_txq(); the driver is still supposed > to maintain a lock that prevents two threads from trying to schedule the > same AC at the same time. This is what drivers already do, so I figured it > was easier to just keep it that way rather than do it in mac80211. I'll look at this in the code, but from a maintainer perspective I'm somewhat worried that this will lead to issues that are really the driver's fault, but surface in mac80211. I don't know how easy it would be to catch that. Might be fine, but something to keep in mind. johannes
Kan Yan <kyan@google.com> writes: > Hi Toke, > > It is great to see finally there will be a general airtime fairness > support in mac80211. I feel this patch set could still use some > improvements to make it works better with 11ac drivers like ath10k. I > did a version of airtime fairness in the ath10k driver that used in > Google Wifi and I'd like to share a few things I learned from this > experience. Hi Kan Thanks for weighing in! > The idea is to use airtime accounting to give firmware just enough > frames to keep it busy and form good aggregations, and keeps the rest > of frames in mac80211 layer queues so Fq_Codel can work on it to > control latency. I think this is a great approach, that we should definitely adopt in mac80211. However, I'm not sure the outstanding airtime limiting should be integrated into the fairness scheduling, since there are drivers such as ath9k that don't need it. Rather, I'd propose that we figure out the API for fairness scheduling first, and add the BQL-like limiter as a separate layer. They can be integrated in the sense that the limiter's estimate of airtime can be used for fairness scheduling as well in the case where the driver/firmware doesn't have a better source of airtime usage. Does that make sense? -Toke
Johannes Berg <johannes@sipsolutions.net> writes: > On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: > >> - I didn't get rid of the register_airtime() callback. As far as I can tell, >> only iwlwifi uses the tx_time field in the struct tx_info. Which means that >> we *could* probably use it for this and just make the other drivers set it; >> but I'm not sure what effects that would have in relation to WMM-AC for >> those drivers, so I chickened out. Will have to try it out, I guess; but it >> also depends on whether ath10k needs to be able to report airtime >> asynchronously anyway. So I'll hold off on that for a bit more. > > I don't think you need to be concerned, the reporting through this has > no immediate effect as the driver would also have to set the feature > flag (NL80211_FEATURE_SUPPORTS_WMM_ADMISSION) for userspace to be able > to use WMM admission TSPECs, and getting tx_tspec->admitted_time to be > non-zero in ieee80211_sta_tx_wmm_ac_notify(). Great! In that case I'll try moving the reporting go through the tx_info struct and check that it works for ath9k :) -Toke
Johannes Berg <johannes@sipsolutions.net> writes: > On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: >> >> A few things that were discussed in the last round that I did *not* change: > > Thanks for this list btw. > >> - I did not add any locking around next_txq(); the driver is still supposed >> to maintain a lock that prevents two threads from trying to schedule the >> same AC at the same time. This is what drivers already do, so I figured it >> was easier to just keep it that way rather than do it in mac80211. > > I'll look at this in the code, but from a maintainer perspective I'm > somewhat worried that this will lead to issues that are really the > driver's fault, but surface in mac80211. I don't know how easy it > would be to catch that. Yeah, I get what you mean. The alternative would be to have a ieee80211_start_schedule(ac) and ieee80211_end_schedule(ac), which basically just takes a lock. Would mean we could get rid of the 'first' parameter for next_txq(), so might not be such a bad idea; and if the driver has its own locking the extra locking in mac80211 would just be an always-uncontested spinlock, which shouldn't be much overhead, right? -Toke
On Mon, 2018-09-10 at 13:16 +0200, Toke Høiland-Jørgensen wrote: > > > - I didn't get rid of the register_airtime() callback. As far as I can tell, > > > only iwlwifi uses the tx_time field in the struct tx_info. Which means that > > > we *could* probably use it for this and just make the other drivers set it; > > > but I'm not sure what effects that would have in relation to WMM-AC for > > > those drivers, so I chickened out. Will have to try it out, I guess; but it > > > also depends on whether ath10k needs to be able to report airtime > > > asynchronously anyway. So I'll hold off on that for a bit more. > > > > I don't think you need to be concerned, the reporting through this has > > no immediate effect as the driver would also have to set the feature > > flag (NL80211_FEATURE_SUPPORTS_WMM_ADMISSION) for userspace to be able > > to use WMM admission TSPECs, and getting tx_tspec->admitted_time to be > > non-zero in ieee80211_sta_tx_wmm_ac_notify(). > > Great! In that case I'll try moving the reporting go through the tx_info > struct and check that it works for ath9k :) I'm not sure it would work for everyone (though I guess it should for ath9k), so it may well be necessary to report it out-of-band from frames. My objection wasn't so much to the out-of-band mechanism, but to the fact that we have two reporting mechanisms that are each tied to one feature, while reporting the same thing. So if you figure out that you need to keep the reporting out-of-band I'm perfectly fine with that, but I guess I'd argue the two should cross over: reporting in-band should feed back to this, reporting out-of-band should feed the admission mechanism. johannes
On Mon, 2018-09-10 at 13:17 +0200, Toke Høiland-Jørgensen wrote: > > > - I did not add any locking around next_txq(); the driver is still supposed > > > to maintain a lock that prevents two threads from trying to schedule the > > > same AC at the same time. This is what drivers already do, so I figured it > > > was easier to just keep it that way rather than do it in mac80211. > > > > I'll look at this in the code, but from a maintainer perspective I'm > > somewhat worried that this will lead to issues that are really the > > driver's fault, but surface in mac80211. I don't know how easy it > > would be to catch that. > > Yeah, I get what you mean. The alternative would be to have a > ieee80211_start_schedule(ac) and ieee80211_end_schedule(ac), which > basically just takes a lock. And I guess start would increment the schedule number, which is now dependent on first > Would mean we could get rid of the 'first' > parameter for next_txq(), so might not be such a bad idea; Right, that's what I meant. > and if the > driver has its own locking the extra locking in mac80211 would just be > an always-uncontested spinlock, which shouldn't be much overhead, right? It may still bounce around CPUs if you call this from other places, but I suspect that wouldn't be the biggest issue. There are a lot of calculations going on too... johannes
> I think this is a great approach, that we should definitely adopt in > mac80211. However, I'm not sure the outstanding airtime limiting should > be integrated into the fairness scheduling, since there are drivers such > as ath9k that don't need it. > > Rather, I'd propose that we figure out the API for fairness scheduling > first, and add the BQL-like limiter as a separate layer. They can be > integrated in the sense that the limiter's estimate of airtime can be > used for fairness scheduling as well in the case where the > driver/firmware doesn't have a better source of airtime usage. > > Does that make sense? Sure that make sense. Yes, the airtime based BQL like queue limiter is not needed for drivers such as ath9k. We could leave the outstanding airtime accounting and queue depth limit to another subject and get airtime fairness scheduling API done first. On Mon, Sep 10, 2018 at 4:26 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2018-09-10 at 13:17 +0200, Toke Høiland-Jørgensen wrote: > >> > > - I did not add any locking around next_txq(); the driver is still supposed >> > > to maintain a lock that prevents two threads from trying to schedule the >> > > same AC at the same time. This is what drivers already do, so I figured it >> > > was easier to just keep it that way rather than do it in mac80211. >> > >> > I'll look at this in the code, but from a maintainer perspective I'm >> > somewhat worried that this will lead to issues that are really the >> > driver's fault, but surface in mac80211. I don't know how easy it >> > would be to catch that. >> >> Yeah, I get what you mean. The alternative would be to have a >> ieee80211_start_schedule(ac) and ieee80211_end_schedule(ac), which >> basically just takes a lock. > > And I guess start would increment the schedule number, which is now > dependent on first > >> Would mean we could get rid of the 'first' >> parameter for next_txq(), so might not be such a bad idea; > > Right, that's what I meant. > >> and if the >> driver has its own locking the extra locking in mac80211 would just be >> an always-uncontested spinlock, which shouldn't be much overhead, right? > > It may still bounce around CPUs if you call this from other places, but > I suspect that wouldn't be the biggest issue. There are a lot of > calculations going on too... > > johannes
Kan Yan <kyan@google.com> writes: >> I think this is a great approach, that we should definitely adopt in >> mac80211. However, I'm not sure the outstanding airtime limiting should >> be integrated into the fairness scheduling, since there are drivers such >> as ath9k that don't need it. >> >> Rather, I'd propose that we figure out the API for fairness scheduling >> first, and add the BQL-like limiter as a separate layer. They can be >> integrated in the sense that the limiter's estimate of airtime can be >> used for fairness scheduling as well in the case where the >> driver/firmware doesn't have a better source of airtime usage. >> >> Does that make sense? > Sure that make sense. Yes, the airtime based BQL like queue limiter is > not needed for drivers such as ath9k. We could leave the outstanding > airtime accounting and queue depth limit to another subject and get > airtime fairness scheduling API done first. Cool! I was thinking I would stub out an untested PoC in a separate patch on my next submission, to show how I think this could be incorporated. Have some other stuff to finish up this week, but hopefully I'll have time to do the next version over the weekend :) -Toke
This is an updated version of the patch series to move TXQ scheduling and airtime fairness scheduling into mac80211. I've only compile tested this version, but thought it was better to get the conversation moving instead of being blocked on me. This addresses most of the comments from the last round. Specifically: - It folds in Rajkumar's patches to keep per-AC TXQ lists, and to add an API that ath10k can use to check if a TXQ may transmit according even when not using get_next_txq(). I changed a few names and descriptions, but otherwise it's mostly the same. After the discussions we had in the last series, I *think* it will work this way, but I'm not entirely sure. - I got rid of any mention of seqno in next_txq() and schedule_txq() - and removed the third parameter to schedule_txq() entirely, so drivers can no longer signal that a TXQ should be allowed to re-appear in a scheduling round. We can add that back if needed. - Added a helper function to schedule and wake TXQs in a single call, for internal mac80211 use. - Changed the default station weight to 256 and got rid of the per-phy quantum. This makes it possible to lower station weights without having to change the weights of every other station. A few things that were discussed in the last round that I did *not* change: - I did not add any locking around next_txq(); the driver is still supposed to maintain a lock that prevents two threads from trying to schedule the same AC at the same time. This is what drivers already do, so I figured it was easier to just keep it that way rather than do it in mac80211. - I didn't get rid of the register_airtime() callback. As far as I can tell, only iwlwifi uses the tx_time field in the struct tx_info. Which means that we *could* probably use it for this and just make the other drivers set it; but I'm not sure what effects that would have in relation to WMM-AC for those drivers, so I chickened out. Will have to try it out, I guess; but it also depends on whether ath10k needs to be able to report airtime asynchronously anyway. So I'll hold off on that for a bit more. -Toke --- Toke Høiland-Jørgensen (4): mac80211: Add TXQ scheduling API mac80211: Add airtime accounting and scheduling to TXQs cfg80211: Add airtime statistics and settings ath9k: Switch to mac80211 TXQ scheduling and airtime APIs 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 | 54 ------ drivers/net/wireless/ath/ath9k/init.c | 3 drivers/net/wireless/ath/ath9k/recv.c | 9 - drivers/net/wireless/ath/ath9k/xmit.c | 245 ++++++++-------------------- include/net/cfg80211.h | 12 + include/net/mac80211.h | 97 +++++++++++ include/uapi/linux/nl80211.h | 15 ++ net/mac80211/agg-tx.c | 2 net/mac80211/cfg.c | 3 net/mac80211/debugfs.c | 3 net/mac80211/debugfs_sta.c | 42 ++++- net/mac80211/driver-ops.h | 7 + net/mac80211/ieee80211_i.h | 11 + net/mac80211/main.c | 5 + net/mac80211/sta_info.c | 49 +++++- net/mac80211/sta_info.h | 13 + net/mac80211/tx.c | 125 ++++++++++++++ net/wireless/nl80211.c | 25 +++ 21 files changed, 471 insertions(+), 274 deletions(-) X-Clacks-Overhead: GNU Terry Pratchett