Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>
>
> +void ieee80211_sta_update_pending_airtime(struct ieee80211_local *local,
> + struct sta_info *sta, u8 ac,
> + u16 tx_airtime, bool tx_completed)
> +{
> + spin_lock_bh(&local->active_txq_lock[ac]);
> + if (tx_completed) {
> + if (sta) {
> + if (WARN_ONCE(sta->airtime[ac].aql_tx_pending <
> tx_airtime,
> + "TXQ pending airtime underflow: %u, %u",
> + sta->airtime[ac].aql_tx_pending,
> tx_airtime))
Maybe add the STA/AC to the message?
johannes
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
Johannes Berg writes:
> On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>>
>>
>> +void ieee80211_sta_update_pending_airtime(struct ieee80211_local *local,
>> + struct sta_info *sta, u8 ac,
>> + u16 tx_airtime, bool tx_completed)
>> +{
>> +spin_lock_bh(&local->active_txq_lock[ac]);
>> +if (tx_completed) {
>> +if (sta) {
>> +if (WARN_ONCE(sta->airtime[ac].aql_tx_pending <
>> tx_airtime,
>> + "TXQ pending airtime underflow: %u, %u",
>> + sta->airtime[ac].aql_tx_pending,
>> tx_airtime))
>
> Maybe add the STA/AC to the message?
Can do. Any idea why we might be seeing underflows (as Kan reported)?
-Toke
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
On Fri, 2019-11-08 at 11:56 +0100, Toke Høiland-Jørgensen wrote:
> Johannes Berg writes:
>
> > On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
> > >
> > > +void ieee80211_sta_update_pending_airtime(struct ieee80211_local *local,
> > > + struct sta_info *sta, u8 ac,
> > > + u16 tx_airtime, bool tx_completed)
> > > +{
> > > + spin_lock_bh(&local->active_txq_lock[ac]);
> > > + if (tx_completed) {
> > > + if (sta) {
> > > + if (WARN_ONCE(sta->airtime[ac].aql_tx_pending <
> > > tx_airtime,
> > > + "TXQ pending airtime underflow: %u, %u",
> > > + sta->airtime[ac].aql_tx_pending,
> > > tx_airtime))
> >
> > Maybe add the STA/AC to the message?
>
> Can do. Any idea why we might be seeing underflows (as Kan reported)?
No, I really have no idea. The shifting looked OK to me, though I didn't
review it carefully enough to say I've really looked at all places ...
johannes
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
Johannes Berg writes:
> On Fri, 2019-11-08 at 11:56 +0100, Toke Høiland-Jørgensen wrote:
>> Johannes Berg writes:
>>
>> > On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>> > >
>> > > +void ieee80211_sta_update_pending_airtime(struct ieee80211_local *local,
>> > > + struct sta_info *sta, u8 ac,
>> > > + u16 tx_airtime, bool
>> > > tx_completed)
>> > > +{
>> > > +spin_lock_bh(&local->active_txq_lock[ac]);
>> > > +if (tx_completed) {
>> > > +if (sta) {
>> > > +if (WARN_ONCE(sta->airtime[ac].aql_tx_pending <
>> > > tx_airtime,
>> > > + "TXQ pending airtime underflow:
>> > > %u, %u",
>> > > + sta->airtime[ac].aql_tx_pending,
>> > > tx_airtime))
>> >
>> > Maybe add the STA/AC to the message?
>>
>> Can do. Any idea why we might be seeing underflows (as Kan reported)?
>
> No, I really have no idea. The shifting looked OK to me, though I didn't
> review it carefully enough to say I've really looked at all places ...
Right, bugger. I was thinking maybe there's a case where skbs can be
cloned (and retain the tx_time_est field) and then released twice? Or
maybe somewhere that steps on the skb->cb field in some other way?
Couldn't find anything obvious on a first perusal of the TX path code,
but maybe you could think of something?
Otherwise I guess we'll be forced to go and do some actual,
old-fashioned debugging ;)
-Toke
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
On Fri, 2019-11-08 at 12:10 +0100, Toke Høiland-Jørgensen wrote: > Right, bugger. I was thinking maybe there's a case where skbs can be > cloned (and retain the tx_time_est field) and then released twice? They could be cloned, but I don't see how that'd be while *inside* the stack and then they get reported twice - unless the driver did something like that? I mean, TCP surely does that for example, but it's before we even get to mac80211. > Or > maybe somewhere that steps on the skb->cb field in some other way? > Couldn't find anything obvious on a first perusal of the TX path code, > but maybe you could think of something? No, sorry. But I also didn't actually look at the driver at all. > Otherwise I guess we'll be forced to go and do some actual, > old-fashioned debugging ;) :) johannes ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
It is most likely just insufficient locking. active_txq_lock is per
AC, can't protect local->aql_total_pending_airtime against racing
conditions:
void ieee80211_sta_update_pending_airtime(...)
{
spin_lock_bh(&local->active_txq_lock[ac]);
...
local->aql_total_pending_airtime -= tx_airtime;
...
spin_unlock_bh(&local->active_txq_lock[ac]);
}
After changing it to atomic_t, no more aql_total_pending_airtime
underflow so far :). Using atomic operation should also help reduce
CPU overhead due to lock contention, as
ieee80211_sta_update_pending_airtime() is often called from the tx
completion routine triggered by interrupts, often in a different core
than where __ieee80211_schedule_txq() is running.
I will post a new version a bit later if the test goes well.
Regards,
Kan
On Fri, Nov 8, 2019 at 3:17 AM Johannes Berg wrote:
>
> On Fri, 2019-11-08 at 12:10 +0100, Toke Høiland-Jørgensen wrote:
>
> > Right, bugger. I was thinking maybe there's a case where skbs can be
> > cloned (and retain the tx_time_est field) and then released twice?
>
> They could be cloned, but I don't see how that'd be while *inside* the
> stack and then they get reported twice - unless the driver did something
> like that?
>
> I mean, TCP surely does that for example, but it's before we even get to
> mac80211.
>
> > Or
> > maybe somewhere that steps on the skb->cb field in some other way?
> > Couldn't find anything obvious on a first perusal of the TX path code,
> > but maybe you could think of something?
>
> No, sorry. But I also didn't actually look at the driver at all.
>
> > Otherwise I guess we'll be forced to go and do some actual,
> > old-fashioned debugging ;)
>
> :)
>
> johannes
>
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v6 3/4] mac80211: Implement Airtime-based Queue Limit (AQL)
Kan Yan writes:
> It is most likely just insufficient locking. active_txq_lock is per
> AC, can't protect local->aql_total_pending_airtime against racing
> conditions:
> void ieee80211_sta_update_pending_airtime(...)
> {
> spin_lock_bh(&local->active_txq_lock[ac]);
> ...
> local->aql_total_pending_airtime -= tx_airtime;
> ...
> spin_unlock_bh(&local->active_txq_lock[ac]);
> }
Ohh, right; didn't even realise those were not per-AC as well...
> After changing it to atomic_t, no more aql_total_pending_airtime
> underflow so far :). Using atomic operation should also help reduce
> CPU overhead due to lock contention, as
> ieee80211_sta_update_pending_airtime() is often called from the tx
> completion routine triggered by interrupts, often in a different core
> than where __ieee80211_schedule_txq() is running.
>
> I will post a new version a bit later if the test goes well.
Awesome! :)
-Toke
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
