| Message ID | 1568639388-27291-3-git-send-email-yiboz@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [1/4] mac80211: Switch to a virtual time-based airtime scheduler | expand |
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. > Co-developed-by: Yibo Zhao <yiboz@codeaurora.org> That also seems wrong, should be Toke I guess, unless you intended for a From: Toke to be present? johannes
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 <yiboz@codeaurora.org> > > 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 <toke@toke.dk> Signed-off-by: Yibo Zhao <yiboz@codeaurora.org> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> Am I understanding right? > > johannes
On Tue, 2019-09-17 at 14:36 +0800, Yibo Zhao wrote: > > Do you mean it should be something like: > > Co-developed-by: Toke Høiland-Jørgensen <toke@toke.dk> > Signed-off-by: Yibo Zhao <yiboz@codeaurora.org> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> Yes, I think you mean the right thing. For the record, it seems to me it should be From: A <...> [...] Co-developed-by: B <...> Signed-off-by: B <...> Signed-off-by: A <...> or so. IOW, I think having the same "From:" (which gets preserved in git as "Author") and "Co-developed-by" makes no sense? Your "From" line was implied, but I suppose you did mean that From would be yourself (as it was in the patch) and then the above seems right. Or you can add a "From: Toke ..." to your patch message and leave the "Co-developed-by: yourself" I suppose, the difference is in how git will record it. johannes
Yibo Zhao <yiboz@codeaurora.org> 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 <yiboz@codeaurora.org> >> >> 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 <toke@toke.dk> > Signed-off-by: Yibo Zhao <yiboz@codeaurora.org> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> > > 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
On 2019-09-18 05:12, Toke Høiland-Jørgensen wrote: > Yibo Zhao <yiboz@codeaurora.org> 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 <yiboz@codeaurora.org> >>> >>> 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 <toke@toke.dk> >> Signed-off-by: Yibo Zhao <yiboz@codeaurora.org> >> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> >> >> 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.
Yibo Zhao <yiboz@codeaurora.org> writes: > On 2019-09-18 05:12, Toke Høiland-Jørgensen wrote: >> Yibo Zhao <yiboz@codeaurora.org> 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 <yiboz@codeaurora.org> >>>> >>>> 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 <toke@toke.dk> >>> Signed-off-by: Yibo Zhao <yiboz@codeaurora.org> >>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> >>> >>> 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
On 2019-09-18 18:16, Toke Høiland-Jørgensen wrote: > Yibo Zhao <yiboz@codeaurora.org> writes: > >> On 2019-09-18 05:12, Toke Høiland-Jørgensen wrote: >>> Yibo Zhao <yiboz@codeaurora.org> 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 <yiboz@codeaurora.org> >>>>> >>>>> 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 <toke@toke.dk> >>>> Signed-off-by: Yibo Zhao <yiboz@codeaurora.org> >>>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> >>>> >>>> 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 Sure, thanks.
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]); }