| Message ID | 1569223201-1490-4-git-send-email-yiboz@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Enable virtual time-based airtime scheduler support on ath10k | expand |
Yibo Zhao <yiboz@codeaurora.org> 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_vt g_vt valid_for_next_Tx > 1 256 2048 12800 2048 2000 N > 2 256 2048 2048 N > . . . . . > . . . . . > . . . . . > 50 256 2048 2048 N > > 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_vt g_vt valid_for_next_Tx > 1 256 2048 13056 2048 2020 N > 2 256 2048 2048 N > . . . . > . . . . > . . . . > 50 256 2048 2048 N > 51 256 1024 2524 N 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
On 2019-09-23 18:55, Toke Høiland-Jørgensen wrote: > Yibo Zhao <yiboz@codeaurora.org> 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_vt g_vt valid_for_next_Tx >> 1 256 2048 12800 2048 2000 N >> 2 256 2048 2048 N >> . . . . . >> . . . . . >> . . . . . >> 50 256 2048 2048 N >> >> 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_vt g_vt valid_for_next_Tx >> 1 256 2048 13056 2048 2020 N >> 2 256 2048 2048 N >> . . . . >> . . . . >> . . . . >> 50 256 2048 2048 N >> 51 256 1024 2524 N > > 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? 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. > > -Toke
Yibo Zhao <yiboz@codeaurora.org> 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
On 2019-09-24 16:48, Toke Høiland-Jørgensen wrote: > Yibo Zhao <yiboz@codeaurora.org> 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? Sure, no problem. :) > > -Toke
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 9d01fdd..feac975 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -1852,7 +1852,8 @@ void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid, weight_sum = local->airtime_weight_sum[ac] ?: sta->airtime_weight; - local->airtime_v_t[ac] += airtime / weight_sum; + /* Round the calculation of global vt */ + local->airtime_v_t[ac] += (airtime + (weight_sum >> 1)) / weight_sum; sta->airtime[ac].v_t += airtime / sta->airtime_weight; ieee80211_resort_txq(&local->hw, txq); diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index 5c1cac9..5055f94 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -130,7 +130,7 @@ enum ieee80211_agg_stop_reason { /* Debugfs flags to enable/disable use of RX/TX airtime in scheduler */ #define AIRTIME_USE_TX BIT(0) #define AIRTIME_USE_RX BIT(1) -#define AIRTIME_GRACE 500 /* usec of grace period before reset */ +#define AIRTIME_GRACE 2000 /* usec of grace period before reset */ struct airtime_info { u64 rx_airtime; diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 42ca010..60cf569 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -3867,15 +3867,29 @@ bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw, struct ieee80211_txq *txq) { struct ieee80211_local *local = hw_to_local(hw); - struct txq_info *txqi = to_txq_info(txq); + struct txq_info *first_txqi, *txqi = to_txq_info(txq); + struct rb_node *node = NULL; struct sta_info *sta; u8 ac = txq->ac; + first_txqi = NULL; lockdep_assert_held(&local->active_txq_lock[ac]); if (!txqi->txq.sta) return true; + node = rb_first_cached(&local->active_txqs[ac]); + if (node) { + first_txqi = container_of(node, struct txq_info, + schedule_order); + if (first_txqi->txq.sta) { + sta = container_of(first_txqi->txq.sta, + struct sta_info, sta); + if (local->airtime_v_t[ac] < sta->airtime[ac].v_t) + local->airtime_v_t[ac] = sta->airtime[ac].v_t; + } + } + sta = container_of(txqi->txq.sta, struct sta_info, sta); return (sta->airtime[ac].v_t <= local->airtime_v_t[ac]); }
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_vt g_vt valid_for_next_Tx 1 256 2048 12800 2048 2000 N 2 256 2048 2048 N . . . . . . . . . . . . . . . 50 256 2048 2048 N 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_vt g_vt valid_for_next_Tx 1 256 2048 13056 2048 2020 N 2 256 2048 2048 N . . . . . . . . . . . . 50 256 2048 2048 N 51 256 1024 2524 N 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 Signed-off-by: Yibo Zhao <yiboz@codeaurora.org> --- net/mac80211/sta_info.c | 3 ++- net/mac80211/sta_info.h | 2 +- net/mac80211/tx.c | 16 +++++++++++++++- 3 files changed, 18 insertions(+), 3 deletions(-)