| Message ID | 1569924485-I3e8838c5ecad878e59d4a94eb069a90f6641461a@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: keep BHs disabled while calling drv_tx_wake_queue() | expand |
Johannes Berg <johannes@sipsolutions.net> writes: > From: Johannes Berg <johannes.berg@intel.com> > > Drivers typically expect this, as it's the case for almost all cases > where this is called (i.e. from the TX path). Also, the code in mac80211 > itself (if the driver calls ieee80211_tx_dequeue()) expects this as it > uses this_cpu_ptr() without additional protection. > > This should fix various reports of the problem: > https://bugzilla.kernel.org/show_bug.cgi?id=204127 > https://lore.kernel.org/linux-wireless/CAN5HydrWb3o_FE6A1XDnP1E+xS66d5kiEuhHfiGKkLNQokx13Q@mail.gmail.com/ > https://lore.kernel.org/lkml/nycvar.YFH.7.76.1909111238470.473@cbobk.fhfr.pm/ > > Reported-by: Jiri Kosina <jikos@kernel.org> > Reported-by: Aaron Hill <aa1ronham@gmail.com> > Reported-by: Lukas Redlinger <rel+kernel@agilox.net> > Reported-by: Oleksii Shevchuk <alxchk@gmail.com> > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > net/mac80211/util.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/mac80211/util.c b/net/mac80211/util.c > index 051a02ddcb85..ad1e88958da2 100644 > --- a/net/mac80211/util.c > +++ b/net/mac80211/util.c > @@ -273,9 +273,9 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac) > &txqi->flags)) > continue; > > - spin_unlock_bh(&fq->lock); > + spin_unlock(&fq->lock); > drv_wake_tx_queue(local, txqi); > - spin_lock_bh(&fq->lock); > + spin_lock(&fq->lock); Okay, so this will mean that the drv_wake_tx_queue() entry point will be called with bhs disabled. But there are lots of uses of spin_{,un}lock_bh() in tx.c: $ git grep lock_bh tx.c tx.c: spin_lock_bh(&fq->lock); tx.c: spin_unlock_bh(&fq->lock); tx.c: spin_lock_bh(&fq->lock); tx.c: spin_unlock_bh(&fq->lock); tx.c: spin_lock_bh(&fq->lock); tx.c: spin_unlock_bh(&fq->lock); tx.c: spin_lock_bh(&local->active_txq_lock[txqi->txq.ac]); tx.c: spin_unlock_bh(&local->active_txq_lock[txqi->txq.ac]); tx.c: spin_lock_bh(&fq->lock); tx.c: spin_unlock_bh(&fq->lock); tx.c: spin_lock_bh(&fq->lock); tx.c: spin_unlock_bh(&fq->lock); tx.c: spin_lock_bh(&sta->lock); tx.c: spin_unlock_bh(&sta->lock); tx.c: spin_lock_bh(&sta->lock); tx.c: spin_unlock_bh(&sta->lock); tx.c: spin_lock_bh(&fq->lock); tx.c: spin_unlock_bh(&fq->lock); tx.c: spin_lock_bh(&fq->lock); tx.c: spin_unlock_bh(&fq->lock); tx.c: spin_lock_bh(&fq->lock); tx.c: spin_unlock_bh(&fq->lock); tx.c: spin_unlock_bh(&fq->lock); tx.c: spin_lock_bh(&local->active_txq_lock[ac]); tx.c: spin_unlock_bh(&local->active_txq_lock[ac]); tx.c: spin_lock_bh(&local->active_txq_lock[txq->ac]); tx.c: spin_unlock_bh(&local->active_txq_lock[txq->ac]); tx.c: spin_lock_bh(&local->active_txq_lock[ac]); tx.c: spin_unlock_bh(&local->active_txq_lock[ac]); tx.c: spin_unlock_bh(&local->active_txq_lock[ac]); tx.c: spin_lock_bh(&local->active_txq_lock[ac]); tx.c: spin_unlock_bh(&local->active_txq_lock[ac]); tx.c: spin_lock_bh(&local->tim_lock); tx.c: spin_unlock_bh(&local->tim_lock); so won't that mean that the driver still gets bhs re-enabled after (for instance) the first call to ieee80211_tx_dequeue()? I must admit that I'm a bit fuzzy on this whole bh/not-bh thing; I've mostly been cargo-culting the _bh variant of the locking... :) -Toke
On Tue, 2019-10-01 at 12:53 +0200, Toke Høiland-Jørgensen wrote: > > > - spin_unlock_bh(&fq->lock); > > + spin_unlock(&fq->lock); > > drv_wake_tx_queue(local, txqi); > > - spin_lock_bh(&fq->lock); > > + spin_lock(&fq->lock); > > Okay, so this will mean that the drv_wake_tx_queue() entry point will be > called with bhs disabled. Right. > But there are lots of uses of > spin_{,un}lock_bh() in tx.c: [snip] > so won't that mean that the driver still gets bhs re-enabled after (for > instance) the first call to ieee80211_tx_dequeue()? No, local_bh_disable()/local_bh_enable() is re-entrant and nests fine. johannes
On Tue, 1 Oct 2019, Toke Høiland-Jørgensen wrote: > > - spin_unlock_bh(&fq->lock); > > + spin_unlock(&fq->lock); > > drv_wake_tx_queue(local, txqi); > > - spin_lock_bh(&fq->lock); > > + spin_lock(&fq->lock); > > Okay, so this will mean that the drv_wake_tx_queue() entry point will be > called with bhs disabled. But there are lots of uses of > spin_{,un}lock_bh() in tx.c: I am fine with whatever fix for this you guys settle on :) Just for the record, I proposed this back then: http://lore.kernel.org/r/nycvar.YFH.7.76.1904151300160.9803@cbobk.fhfr.pm maybe it could be resurected, as I believe it'd fix this one as well? But again, I have absolutely no preference either way, only that it'd be nice to have this fixed :) Thanks!
On Tue, 2019-10-01 at 12:56 +0200, Jiri Kosina wrote: > On Tue, 1 Oct 2019, Toke Høiland-Jørgensen wrote: > > > > - spin_unlock_bh(&fq->lock); > > > + spin_unlock(&fq->lock); > > > drv_wake_tx_queue(local, txqi); > > > - spin_lock_bh(&fq->lock); > > > + spin_lock(&fq->lock); > > > > Okay, so this will mean that the drv_wake_tx_queue() entry point will be > > called with bhs disabled. But there are lots of uses of > > spin_{,un}lock_bh() in tx.c: > > I am fine with whatever fix for this you guys settle on :) Just for the > record, I proposed this back then: > > http://lore.kernel.org/r/nycvar.YFH.7.76.1904151300160.9803@cbobk.fhfr.pm > > maybe it could be resurected, as I believe it'd fix this one as well? Yes, it would, but it wouldn't address any other driver that likely has the same issue :) johannes
Johannes Berg <johannes@sipsolutions.net> writes: > On Tue, 2019-10-01 at 12:53 +0200, Toke Høiland-Jørgensen wrote: >> >> > - spin_unlock_bh(&fq->lock); >> > + spin_unlock(&fq->lock); >> > drv_wake_tx_queue(local, txqi); >> > - spin_lock_bh(&fq->lock); >> > + spin_lock(&fq->lock); >> >> Okay, so this will mean that the drv_wake_tx_queue() entry point will be >> called with bhs disabled. > > Right. > >> But there are lots of uses of >> spin_{,un}lock_bh() in tx.c: > > [snip] > >> so won't that mean that the driver still gets bhs re-enabled after (for >> instance) the first call to ieee80211_tx_dequeue()? > > No, local_bh_disable()/local_bh_enable() is re-entrant and nests fine. Ah, right, gotcha. Hmm, in that case, would it be more clear to just add an outer local_bh_disable()/local_bh_enable() to ___ieee80211_wake_txqs(). With this patch we're relying on the mismatched use of _bh/non-_bh variants of the locking to ensure the bhs stay off. Isn't that prone to breaking in the future? Oh, and also, with just this patch, the additional drv_wake_tx_queue() call for the vif TXQ at the bottom of __ieee80211_wake_txqs() will still happen without bhs disabled, won't it? -Toke
diff --git a/net/mac80211/util.c b/net/mac80211/util.c index 051a02ddcb85..ad1e88958da2 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -273,9 +273,9 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac) &txqi->flags)) continue; - spin_unlock_bh(&fq->lock); + spin_unlock(&fq->lock); drv_wake_tx_queue(local, txqi); - spin_lock_bh(&fq->lock); + spin_lock(&fq->lock); } }