| Message ID | 20180504175144.12179-3-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
On Fri, May 04, 2018 at 07:51:44PM +0200, Sebastian Andrzej Siewior wrote: > From: Anna-Maria Gleixner <anna-maria@linutronix.de> > > The warning in ieee802154_rx() and ieee80211_rx_napi() is there to ensure > the softirq context for the subsequent netif_receive_skb() call. That's not in fact what it does though; so while that might indeed be the intent that's not what it does.
On 2018-05-04 20:32:49 [+0200], Peter Zijlstra wrote: > On Fri, May 04, 2018 at 07:51:44PM +0200, Sebastian Andrzej Siewior wrote: > > From: Anna-Maria Gleixner <anna-maria@linutronix.de> > > > > The warning in ieee802154_rx() and ieee80211_rx_napi() is there to ensure > > the softirq context for the subsequent netif_receive_skb() call. > > That's not in fact what it does though; so while that might indeed be > the intent that's not what it does. It was introduced in commit d20ef63d3246 ("mac80211: document ieee80211_rx() context requirement"): mac80211: document ieee80211_rx() context requirement ieee80211_rx() must be called with softirqs disabled since the networking stack requires this for netif_rx() and some code in mac80211 can assume that it can not be processing its own tasklet and this call at the same time. It may be possible to remove this requirement after a careful audit of mac80211 and doing any needed locking improvements in it along with disabling softirqs around netif_rx(). An alternative might be to push all packet processing to process context in mac80211, instead of to the tasklet, and add other synchronisation. Sebastian
On Fri, May 04, 2018 at 08:45:39PM +0200, Sebastian Andrzej Siewior wrote: > On 2018-05-04 20:32:49 [+0200], Peter Zijlstra wrote: > > On Fri, May 04, 2018 at 07:51:44PM +0200, Sebastian Andrzej Siewior wrote: > > > From: Anna-Maria Gleixner <anna-maria@linutronix.de> > > > > > > The warning in ieee802154_rx() and ieee80211_rx_napi() is there to ensure > > > the softirq context for the subsequent netif_receive_skb() call. > > > > That's not in fact what it does though; so while that might indeed be > > the intent that's not what it does. > > It was introduced in commit d20ef63d3246 ("mac80211: document > ieee80211_rx() context requirement"): > > mac80211: document ieee80211_rx() context requirement > > ieee80211_rx() must be called with softirqs disabled softirqs disabled, ack that is exactly what it checks. But afaict the assertion you introduced tests that we are _in_ softirq context, which is not the same.
On 2018-05-04 20:51:32 [+0200], Peter Zijlstra wrote: > On Fri, May 04, 2018 at 08:45:39PM +0200, Sebastian Andrzej Siewior wrote: > > On 2018-05-04 20:32:49 [+0200], Peter Zijlstra wrote: > > > On Fri, May 04, 2018 at 07:51:44PM +0200, Sebastian Andrzej Siewior wrote: > > > > From: Anna-Maria Gleixner <anna-maria@linutronix.de> > > > > > > > > The warning in ieee802154_rx() and ieee80211_rx_napi() is there to ensure > > > > the softirq context for the subsequent netif_receive_skb() call. > > > > > > That's not in fact what it does though; so while that might indeed be > > > the intent that's not what it does. > > > > It was introduced in commit d20ef63d3246 ("mac80211: document > > ieee80211_rx() context requirement"): > > > > mac80211: document ieee80211_rx() context requirement > > > > ieee80211_rx() must be called with softirqs disabled > > softirqs disabled, ack that is exactly what it checks. > > But afaict the assertion you introduced tests that we are _in_ softirq > context, which is not the same. indeed, now it clicked. Given what I wrote in the cover letter would you be in favour of (a proper) lockdep_assert_BH_disabled() or the cheaper local_bh_enable() (assuming the network folks don't mind the cheaper version)? Sebastian
On Fri, May 04, 2018 at 09:07:35PM +0200, Sebastian Andrzej Siewior wrote: > On 2018-05-04 20:51:32 [+0200], Peter Zijlstra wrote: > > softirqs disabled, ack that is exactly what it checks. > > > > But afaict the assertion you introduced tests that we are _in_ softirq > > context, which is not the same. > > indeed, now it clicked. Given what I wrote in the cover letter would you > be in favour of (a proper) lockdep_assert_BH_disabled() or the cheaper > local_bh_enable() (assuming the network folks don't mind the cheaper > version)? Depends a bit on what the code wants I suppose. If the code is in fact fine with the stronger in-softirq assertion, that'd be best. Otherwise I don't object to a lockdep_assert_bh_disabled() to accompany the lockdep_assert_irq_disabled() we already have either.
* Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > On 2018-05-04 20:51:32 [+0200], Peter Zijlstra wrote: > > On Fri, May 04, 2018 at 08:45:39PM +0200, Sebastian Andrzej Siewior wrote: > > > On 2018-05-04 20:32:49 [+0200], Peter Zijlstra wrote: > > > > On Fri, May 04, 2018 at 07:51:44PM +0200, Sebastian Andrzej Siewior wrote: > > > > > From: Anna-Maria Gleixner <anna-maria@linutronix.de> > > > > > > > > > > The warning in ieee802154_rx() and ieee80211_rx_napi() is there to ensure > > > > > the softirq context for the subsequent netif_receive_skb() call. > > > > > > > > That's not in fact what it does though; so while that might indeed be > > > > the intent that's not what it does. > > > > > > It was introduced in commit d20ef63d3246 ("mac80211: document > > > ieee80211_rx() context requirement"): > > > > > > mac80211: document ieee80211_rx() context requirement > > > > > > ieee80211_rx() must be called with softirqs disabled > > > > softirqs disabled, ack that is exactly what it checks. > > > > But afaict the assertion you introduced tests that we are _in_ softirq > > context, which is not the same. > > indeed, now it clicked. Given what I wrote in the cover letter would you > be in favour of (a proper) lockdep_assert_BH_disabled() or the cheaper > local_bh_enable() (assuming the network folks don't mind the cheaper > version)? BTW., going by the hardirq variant nomenclature: lockdep_assert_irqs_enabled(); ... the proper name would not be lockdep_assert_BH_disabled(), but: lockdep_assert_softirqs_disabled(); Thanks, Ingo
diff --git a/net/core/dev.c b/net/core/dev.c index af0558b00c6c..7b4b8611cc21 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4750,6 +4750,9 @@ static int netif_receive_skb_internal(struct sk_buff *skb) */ int netif_receive_skb(struct sk_buff *skb) { + lockdep_assert_irqs_enabled(); + lockdep_assert_in_softirq(); + trace_netif_receive_skb_entry(skb); return netif_receive_skb_internal(skb); diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index 03102aff0953..653332d81c17 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -4324,8 +4324,6 @@ void ieee80211_rx_napi(struct ieee80211_hw *hw, struct ieee80211_sta *pubsta, struct ieee80211_supported_band *sband; struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb); - WARN_ON_ONCE(softirq_count() == 0); - if (WARN_ON(status->band >= NUM_NL80211_BANDS)) goto drop; diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c index 4dcf6e18563a..66916c270efc 100644 --- a/net/mac802154/rx.c +++ b/net/mac802154/rx.c @@ -258,8 +258,6 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb) { u16 crc; - WARN_ON_ONCE(softirq_count() == 0); - if (local->suspended) goto drop;