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