| Message ID | 1534202011-13101-4-git-send-email-rmanohar@codeaurora.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: handle push-pull path in ATF | expand |
Rajkumar Manoharan <rmanohar@codeaurora.org> writes: > This allows the driver to refill airtime fairness deficit > where the driver will not access txqs by ieee80211_next_txq. > In tx push mode data path, high priority txqs will be scheduled > for data transmission by ieee80211_next_txq and driver will not > prioritize txqs whereas in push-pull mode, the drivers can > prioritize txqs and access them directly. In such mode, airtime > deficit can not filled by ieee80211_next_txq. > > Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org> > --- > include/net/mac80211.h | 15 +++++++++++++ > net/mac80211/tx.c | 59 +++++++++++++++++++++++++++++++++++--------------- > 2 files changed, 57 insertions(+), 17 deletions(-) > > diff --git a/include/net/mac80211.h b/include/net/mac80211.h > index cc16847bd52d..a2f0b6800100 100644 > --- a/include/net/mac80211.h > +++ b/include/net/mac80211.h > @@ -6033,6 +6033,21 @@ bool ieee80211_schedule_txq(struct ieee80211_hw *hw, > struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac); > > /** > + * ieee80211_reorder_txq - change txq position in scheduling loop > + * > + * This function is used to reorder txq's position in scheduling loop. > + * The txq fairness deficit will be refilled. The drivers calling this > + * function should ensure the txq won't be accessed by ieee80211_next_txq > + * in the same path. > + * > + * @hw: pointer as obtained from ieee80211_alloc_hw() > + * @txq: pointer obtained from station or virtual interface > + * > + */ > +void ieee80211_reorder_txq(struct ieee80211_hw *hw, > + struct ieee80211_txq *txq); > + > +/** > * ieee80211_txq_get_depth - get pending frame/byte count of given txq > * > * The values are not guaranteed to be coherent with regard to each other, i.e. > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c > index 0af35c08e0d9..b7b2f93152f8 100644 > --- a/net/mac80211/tx.c > +++ b/net/mac80211/tx.c > @@ -3634,10 +3634,38 @@ static inline struct txq_info *find_txqi(struct ieee80211_local *local, s8 ac) > return txqi; > } > > +static bool ieee80211_txq_refill_deficit(struct ieee80211_local *local, > + struct txq_info *txqi) > +{ > + struct fq *fq = &local->fq; > + struct sta_info *sta; > + > + lockdep_assert_held(&local->active_txq_lock); > + > + if (!txqi->txq.sta) > + return false; > + > + sta = container_of(txqi->txq.sta, struct sta_info, sta); > + > + if (sta->airtime.deficit[txqi->txq.ac] > 0) > + return false; > + > + sta->airtime.deficit[txqi->txq.ac] += > + IEEE80211_AIRTIME_QUANTUM * sta->airtime.weight; > + list_move_tail(&txqi->schedule_order, > + &local->active_txqs[txqi->txq.ac]); This needs to check that the txq is currently at the head of local->active_txqs[txqi->txq.ac]; otherwise fairness enforcement doesn't work. And with this check I'm not sure the reorder function is terribly useful for what you want to use it for? -Toke
On 2018-08-21 05:24, Toke Høiland-Jørgensen wrote: > Rajkumar Manoharan <rmanohar@codeaurora.org> writes: > >> This allows the driver to refill airtime fairness deficit >> where the driver will not access txqs by ieee80211_next_txq. >> In tx push mode data path, high priority txqs will be scheduled >> for data transmission by ieee80211_next_txq and driver will not >> prioritize txqs whereas in push-pull mode, the drivers can >> prioritize txqs and access them directly. In such mode, airtime >> deficit can not filled by ieee80211_next_txq. >> [...] > > This needs to check that the txq is currently at the head of > local->active_txqs[txqi->txq.ac]; otherwise fairness enforcement > doesn't > work. And with this check I'm not sure the reorder function is terribly > useful for what you want to use it for? > Hmm.. I tried to keep the same policy of next_txq(). i.e do not allow serving txqs when the deficit is negative. reorder_txq() limits over serving the same txq by driver. So that driver is given chance to serve other txqs. As mentioned earlier, next_txq() can not be used for fetching txq directly. So reorder_txq() needs to take care of refilling txq after serving them. -Rajkumar
Rajkumar Manoharan <rmanohar@codeaurora.org> writes: > On 2018-08-21 05:24, Toke Høiland-Jørgensen wrote: >> Rajkumar Manoharan <rmanohar@codeaurora.org> writes: >> >>> This allows the driver to refill airtime fairness deficit >>> where the driver will not access txqs by ieee80211_next_txq. >>> In tx push mode data path, high priority txqs will be scheduled >>> for data transmission by ieee80211_next_txq and driver will not >>> prioritize txqs whereas in push-pull mode, the drivers can >>> prioritize txqs and access them directly. In such mode, airtime >>> deficit can not filled by ieee80211_next_txq. >>> > [...] >> >> This needs to check that the txq is currently at the head of >> local->active_txqs[txqi->txq.ac]; otherwise fairness enforcement >> doesn't >> work. And with this check I'm not sure the reorder function is terribly >> useful for what you want to use it for? >> > Hmm.. I tried to keep the same policy of next_txq(). i.e do not allow > serving txqs when the deficit is negative. reorder_txq() limits over > serving the same txq by driver. So that driver is given chance to > serve other txqs. Yeah, but the fairness comes from all TXQs being given the *same amount* of deficit increase. I.e., with reorder_txq() there needs to be a guarantee that it is called the same number of times for all active TXQs. Which is what the round-robin scheduling ensures in next_txq(). > As mentioned earlier, next_txq() can not be used for fetching txq > directly. So reorder_txq() needs to take care of refilling txq after > serving them. Yeah, I got that; but see above. Unless there's a guarantee that the push/pull mechanism will be round-robin scheduled (which as I'm reading the code there isn't), just increasing the deficit on every call to reorder_txq() is not going to ensure fairness (it'll probably help some, but it won't be completely fair). However, if we add the check to reorder_txq() so the deficit increase + rotation is only done if the TXQ is at the head of the list, we'll get round-robin-equivalent behaviour since a TXQ will only get to continue when it happens to have rotated to the head of the queue. As I mentioned previously it will break MIMO, but I we could fix that later once we've verified that the basic mechanism works. Alternatively, if we keep it the way you've written it in this patch, we can iterate on the driver API and make sure that airtime reporting actually works for ath10k, and then change the scheduler once everything else is in place... -Toke
On 2018-08-22 04:10, Toke Høiland-Jørgensen wrote: > Rajkumar Manoharan <rmanohar@codeaurora.org> writes: > >> On 2018-08-21 05:24, Toke Høiland-Jørgensen wrote: >>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes: >>> [...] > Yeah, but the fairness comes from all TXQs being given the *same > amount* > of deficit increase. I.e., with reorder_txq() there needs to be a > guarantee that it is called the same number of times for all active > TXQs. Which is what the round-robin scheduling ensures in next_txq(). > Understood. >> As mentioned earlier, next_txq() can not be used for fetching txq >> directly. So reorder_txq() needs to take care of refilling txq after >> serving them. > > Yeah, I got that; but see above. Unless there's a guarantee that the > push/pull mechanism will be round-robin scheduled (which as I'm reading > the code there isn't), just increasing the deficit on every call to > reorder_txq() is not going to ensure fairness (it'll probably help > some, > but it won't be completely fair). > > However, if we add the check to reorder_txq() so the deficit increase + > rotation is only done if the TXQ is at the head of the list, we'll get > round-robin-equivalent behaviour since a TXQ will only get to continue > when it happens to have rotated to the head of the queue. As I > mentioned > previously it will break MIMO, but I we could fix that later once we've > verified that the basic mechanism works. > Hmm... The pull mechanism operates in round-robin fashion. Agree that accessing only head node is right way of ensuring fairness. Will fix it and rename as ieee80211_txq_can_transmit(). Thanks Toke for your feedback. -Rajkumar
Rajkumar Manoharan <rmanohar@codeaurora.org> writes: >>> As mentioned earlier, next_txq() can not be used for fetching txq >>> directly. So reorder_txq() needs to take care of refilling txq after >>> serving them. >> >> Yeah, I got that; but see above. Unless there's a guarantee that the >> push/pull mechanism will be round-robin scheduled (which as I'm reading >> the code there isn't), just increasing the deficit on every call to >> reorder_txq() is not going to ensure fairness (it'll probably help >> some, >> but it won't be completely fair). >> >> However, if we add the check to reorder_txq() so the deficit increase + >> rotation is only done if the TXQ is at the head of the list, we'll get >> round-robin-equivalent behaviour since a TXQ will only get to continue >> when it happens to have rotated to the head of the queue. As I >> mentioned >> previously it will break MIMO, but I we could fix that later once we've >> verified that the basic mechanism works. >> > Hmm... The pull mechanism operates in round-robin fashion. Agree that > accessing only head node is right way of ensuring fairness. Ah, excellent; then it should be no problem to enforce the head node access; worst case the pull mechanism will be at a different point in the round-robin, which will sync up quickly. > Will fix it and rename as ieee80211_txq_can_transmit(). Cool. If you send an updated patchset I can fold it into an updated version of my RFC and send it as a proper patch once I've verified everything works with ath9k :) > Thanks Toke for your feedback. You're welcome! And thanks for working on this :) -Toke
On 2018-08-28 03:20, Toke Høiland-Jørgensen wrote: > Rajkumar Manoharan <rmanohar@codeaurora.org> writes: > > Ah, excellent; then it should be no problem to enforce the head node > access; worst case the pull mechanism will be at a different point in > the round-robin, which will sync up quickly. > >> Will fix it and rename as ieee80211_txq_can_transmit(). > > Cool. If you send an updated patchset I can fold it into an updated > version of my RFC and send it as a proper patch once I've verified > everything works with ath9k :) > Sure. Found an issue in txq pause change. The drivers assume that ieee80211_tx_dequeue() returns NULL only when txq is empty. Adding a IEEE80211_TXQ_PAUSE check in tx_dequeue() breaks this assumption. Moreover once txq is paused, it should be queued to tail not at head. Will send updated version ASAP. -Rajkumar
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index cc16847bd52d..a2f0b6800100 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -6033,6 +6033,21 @@ bool ieee80211_schedule_txq(struct ieee80211_hw *hw, struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac); /** + * ieee80211_reorder_txq - change txq position in scheduling loop + * + * This function is used to reorder txq's position in scheduling loop. + * The txq fairness deficit will be refilled. The drivers calling this + * function should ensure the txq won't be accessed by ieee80211_next_txq + * in the same path. + * + * @hw: pointer as obtained from ieee80211_alloc_hw() + * @txq: pointer obtained from station or virtual interface + * + */ +void ieee80211_reorder_txq(struct ieee80211_hw *hw, + struct ieee80211_txq *txq); + +/** * ieee80211_txq_get_depth - get pending frame/byte count of given txq * * The values are not guaranteed to be coherent with regard to each other, i.e. diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 0af35c08e0d9..b7b2f93152f8 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -3634,10 +3634,38 @@ static inline struct txq_info *find_txqi(struct ieee80211_local *local, s8 ac) return txqi; } +static bool ieee80211_txq_refill_deficit(struct ieee80211_local *local, + struct txq_info *txqi) +{ + struct fq *fq = &local->fq; + struct sta_info *sta; + + lockdep_assert_held(&local->active_txq_lock); + + if (!txqi->txq.sta) + return false; + + sta = container_of(txqi->txq.sta, struct sta_info, sta); + + if (sta->airtime.deficit[txqi->txq.ac] > 0) + return false; + + sta->airtime.deficit[txqi->txq.ac] += + IEEE80211_AIRTIME_QUANTUM * sta->airtime.weight; + list_move_tail(&txqi->schedule_order, + &local->active_txqs[txqi->txq.ac]); + + if (sta->airtime.deficit[txqi->txq.ac] > 0) { + spin_lock_bh(&fq->lock); + clear_bit(IEEE80211_TXQ_PAUSE, &txqi->flags); + spin_unlock_bh(&fq->lock); + } + return true; +} + struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac) { struct ieee80211_local *local = hw_to_local(hw); - struct fq *fq = &local->fq; struct txq_info *txqi = NULL; spin_lock_bh(&local->active_txq_lock); @@ -3647,22 +3675,8 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac) if (!txqi) goto out; - if (txqi->txq.sta) { - struct sta_info *sta = container_of(txqi->txq.sta, - struct sta_info, sta); - - if (sta->airtime.deficit[txqi->txq.ac] < 0) { - sta->airtime.deficit[txqi->txq.ac] += IEEE80211_AIRTIME_QUANTUM * sta->airtime.weight; - list_move_tail(&txqi->schedule_order, - &local->active_txqs[txqi->txq.ac]); - if (sta->airtime.deficit[txqi->txq.ac] > 0) { - spin_lock_bh(&fq->lock); - clear_bit(IEEE80211_TXQ_PAUSE, &txqi->flags); - spin_unlock_bh(&fq->lock); - } - goto begin; - } - } + if (ieee80211_txq_refill_deficit(local, txqi)) + goto begin; list_del_init(&txqi->schedule_order); @@ -3676,6 +3690,17 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac) } EXPORT_SYMBOL(ieee80211_next_txq); +void ieee80211_reorder_txq(struct ieee80211_hw *hw, struct ieee80211_txq *txq) +{ + struct ieee80211_local *local = hw_to_local(hw); + struct txq_info *txqi = to_txq_info(txq); + + spin_lock_bh(&local->active_txq_lock); + ieee80211_txq_refill_deficit(local, txqi); + spin_unlock_bh(&local->active_txq_lock); +} +EXPORT_SYMBOL(ieee80211_reorder_txq); + void __ieee80211_subif_start_xmit(struct sk_buff *skb, struct net_device *dev, u32 info_flags)
This allows the driver to refill airtime fairness deficit where the driver will not access txqs by ieee80211_next_txq. In tx push mode data path, high priority txqs will be scheduled for data transmission by ieee80211_next_txq and driver will not prioritize txqs whereas in push-pull mode, the drivers can prioritize txqs and access them directly. In such mode, airtime deficit can not filled by ieee80211_next_txq. Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org> --- include/net/mac80211.h | 15 +++++++++++++ net/mac80211/tx.c | 59 +++++++++++++++++++++++++++++++++++--------------- 2 files changed, 57 insertions(+), 17 deletions(-)