Ryder Lee <[email protected]> writes:
> For some drivers and hardware may report faulty airtime, which ends up
> with excessive hold time (~0.9s on mt7915 multiclent tests) impacting
> system performance.
>
> Although issue has been fixed in driver, but it make sense to select txqi
> depends on a reasonable airtime reporting to prevent such a case from
> happening again.
I think I see what you're trying to do with the patch, but this commit
message makes no sense. What, exactly, was the error you were seeing
that this is supposed to fix?
> Tested-by: Jiao Bo <[email protected]>
> Tested-by: Sujuan Chen <[email protected]>
> Signed-off-by: Ryder Lee <[email protected]>
> ---
> net/mac80211/tx.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 6422da6690f7..0b8a8c3600f4 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3770,6 +3770,10 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
> sta->airtime_weight;
>
> if (deficit < 0 || !aql_check) {
> + if (txqi->schedule_round == local->schedule_round[ac])
> + goto out;
> +
> + txqi->schedule_round = local->schedule_round[ac];
I think this change may be worth making anyway, but for a different
reason: Without it, a station that fails aql_check will keep getting
recycled through the list, advancing its deficit. Which could actually
be the reason AQL breaks airtime fairness; did you observe any
difference in fairness with this change?
-Toke
On Fri, 2021-02-05 at 14:29 +0100, Toke Høiland-Jørgensen wrote:
> Ryder Lee <[email protected]> writes:
>
> > For some drivers and hardware may report faulty airtime, which ends up
> > with excessive hold time (~0.9s on mt7915 multiclent tests) impacting
> > system performance.
> >
> > Although issue has been fixed in driver, but it make sense to select txqi
> > depends on a reasonable airtime reporting to prevent such a case from
> > happening again.
>
> I think I see what you're trying to do with the patch, but this commit
> message makes no sense. What, exactly, was the error you were seeing
> that this is supposed to fix?
I will make commit message more straightforward - if a station takes
large amount of airtime and fails the check that will keep getting
recycled through the list along with excessive lock hold time. Add this
patch to avoid breaking fairness.
> > Tested-by: Jiao Bo <[email protected]>
> > Tested-by: Sujuan Chen <[email protected]>
> > Signed-off-by: Ryder Lee <[email protected]>
> > ---
> > net/mac80211/tx.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> > index 6422da6690f7..0b8a8c3600f4 100644
> > --- a/net/mac80211/tx.c
> > +++ b/net/mac80211/tx.c
> > @@ -3770,6 +3770,10 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
> > sta->airtime_weight;
> >
> > if (deficit < 0 || !aql_check) {
> > + if (txqi->schedule_round == local->schedule_round[ac])
> > + goto out;
> > +
> > + txqi->schedule_round = local->schedule_round[ac];
>
> I think this change may be worth making anyway, but for a different
> reason: Without it, a station that fails aql_check will keep getting
> recycled through the list, advancing its deficit. Which could actually
> be the reason AQL breaks airtime fairness; did you observe any
> difference in fairness with this change?
Our case is: mt7915 provides per-peer airtime counters. However, some of
them were not properly configured, so certain stations reported large
amount of airtime which led to deficit < 0, and as you said, ending up
with recycle + very longer lock hold time (0.9s in our tests) and
breaking fairness.
Ryder
On Sun, 2021-02-07 at 10:41 +0800, Ryder Lee wrote:
> On Fri, 2021-02-05 at 14:29 +0100, Toke Høiland-Jørgensen wrote:
> > > @@ -3770,6 +3770,10 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
> > > sta->airtime_weight;
> > >
> > > if (deficit < 0 || !aql_check) {
> > > + if (txqi->schedule_round == local->schedule_round[ac])
> > > + goto out;
> > > +
> > > + txqi->schedule_round = local->schedule_round[ac];
> >
> > I think this change may be worth making anyway, but for a different
> > reason: Without it, a station that fails aql_check will keep getting
> > recycled through the list, advancing its deficit. Which could actually
> > be the reason AQL breaks airtime fairness; did you observe any
> > difference in fairness with this change?
>
> Our case is: mt7915 provides per-peer airtime counters. However, some of
> them were not properly configured, so certain stations reported large
> amount of airtime which led to deficit < 0, and as you said, ending up
> with recycle + very longer lock hold time (0.9s in our tests) and
> breaking fairness.
>
>
Found a problem when we are in low traffic with this patch.This will
increase latency (i.e ping)
So, we have to
if (deficit < 0 || !aql_check) {
if (txqi->schedule_round == local->schedule_round[ac])
// re-schedule
goto out;
....
}
}
if (txqi->schedule_round == local->schedule_round[ac])
// re-schedule
goto out;
Ryder
Ryder Lee <[email protected]> writes:
> On Sun, 2021-02-07 at 10:41 +0800, Ryder Lee wrote:
>> On Fri, 2021-02-05 at 14:29 +0100, Toke Høiland-Jørgensen wrote:
>
>> > > @@ -3770,6 +3770,10 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
>> > > sta->airtime_weight;
>> > >
>> > > if (deficit < 0 || !aql_check) {
>> > > + if (txqi->schedule_round == local->schedule_round[ac])
>> > > + goto out;
>> > > +
>> > > + txqi->schedule_round = local->schedule_round[ac];
>> >
>> > I think this change may be worth making anyway, but for a different
>> > reason: Without it, a station that fails aql_check will keep getting
>> > recycled through the list, advancing its deficit. Which could actually
>> > be the reason AQL breaks airtime fairness; did you observe any
>> > difference in fairness with this change?
>>
>> Our case is: mt7915 provides per-peer airtime counters. However, some of
>> them were not properly configured, so certain stations reported large
>> amount of airtime which led to deficit < 0, and as you said, ending up
>> with recycle + very longer lock hold time (0.9s in our tests) and
>> breaking fairness.
First of all, if the driver reports wrong airtime values, of course it
is going to affect fairness. The right thing in that case is to fix the
driver, or turn off reporting if it can't be fixed.
> Found a problem when we are in low traffic with this patch.This will
> increase latency (i.e ping)
>
>
> So, we have to
>
> if (deficit < 0 || !aql_check) {
> if (txqi->schedule_round == local->schedule_round[ac])
> // re-schedule
You mean, signal the driver to start over? But then you're just undoing
the check you just inserted here...
...and thinking about it a bit more, I don't actually think adding this
check is the right thing to do. As you've just discovered, the deficit
scheduler relies on the "goto begin" below (and thus being able to
keep spinning and increasing the deficit) to make progress. So if you
short-circuit that, you'll get blocking, but if you keep rotating the
queues for other reasons (like AQL does) you no longer get fairness.
Ultimately this comes from using two different sources of airtime:
predicted values (in AQL) and after-the-fact reporting (in the fairness
scheduler). There's a time lag between when these two values are
applied, which leads to the fairness scheduler insisting that a station
should be the next one to transmit even though AQL is blocking it.
Hmm, I wonder what would happen if we just accounted the AQL balance in
the fairness deficit as well? Something like the patch below
(compile-tested only). I'm not sure what the effect of running the
deficit backwards like this is; we may get weird oscillating values when
we subtract the AQL value and the "real" value hasn't been accounted
yet. But it may also turn out to not be a big issue; worth testing,
maybe?
The alternative would be to switch to using only the AQL values for
fairness as well; if the AQL predictions are reasonably accurate this
would likely work well enough. Got any idea how much they are off?
-Toke
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index ec6973ee88ef..86718a6429e6 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1893,12 +1893,10 @@ void ieee80211_sta_set_buffered(struct ieee80211_sta *pubsta,
}
EXPORT_SYMBOL(ieee80211_sta_set_buffered);
-void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
- u32 tx_airtime, u32 rx_airtime)
+static void __ieee80211_sta_register_airtime(struct ieee80211_local *local,
+ struct sta_info *sta, u8 ac,
+ u32 tx_airtime, u32 rx_airtime)
{
- struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
- struct ieee80211_local *local = sta->sdata->local;
- u8 ac = ieee80211_ac_from_tid(tid);
u32 airtime = 0;
if (sta->local->airtime_flags & AIRTIME_USE_TX)
@@ -1912,6 +1910,16 @@ void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
sta->airtime[ac].deficit -= airtime;
spin_unlock_bh(&local->active_txq_lock[ac]);
}
+
+void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 tid,
+ u32 tx_airtime, u32 rx_airtime)
+{
+ struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
+ struct ieee80211_local *local = sta->sdata->local;
+ u8 ac = ieee80211_ac_from_tid(tid);
+
+ __ieee80211_sta_register_airtime(local, sta, ac, tx_airtime, rx_airtime);
+}
EXPORT_SYMBOL(ieee80211_sta_register_airtime);
void ieee80211_sta_update_pending_airtime(struct ieee80211_local *local,
@@ -1924,9 +1932,11 @@ void ieee80211_sta_update_pending_airtime(struct ieee80211_local *local,
return;
if (!tx_completed) {
- if (sta)
+ if (sta) {
atomic_add(tx_airtime,
&sta->airtime[ac].aql_tx_pending);
+ __ieee80211_sta_register_airtime(local, sta, ac, tx_airtime, 0);
+ }
atomic_add(tx_airtime, &local->aql_total_pending_airtime);
return;
@@ -1938,6 +1948,7 @@ void ieee80211_sta_update_pending_airtime(struct ieee80211_local *local,
if (tx_pending < 0)
atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending,
tx_pending, 0);
+ __ieee80211_sta_register_airtime(local, sta, ac, -tx_airtime, 0);
}
tx_pending = atomic_sub_return(tx_airtime,