Re: [RFC PATCH 2/2] net: mac808211: mac802154: use lockdep_assert_in_softirq() instead own warning
* Sebastian Andrzej Siewior 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
> > > > >
> > > > > 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
Re: [RFC PATCH 2/2] net: mac808211: mac802154: use lockdep_assert_in_softirq() instead own warning
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.
Re: [RFC PATCH 2/2] net: mac808211: mac802154: use lockdep_assert_in_softirq() instead own warning
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
> > > >
> > > > 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
Re: [RFC PATCH 2/2] net: mac808211: mac802154: use lockdep_assert_in_softirq() instead own warning
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
> > >
> > > 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.
Re: [RFC PATCH 2/2] net: mac808211: mac802154: use lockdep_assert_in_softirq() instead own warning
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
> >
> > 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
Re: [RFC PATCH 2/2] net: mac808211: mac802154: use lockdep_assert_in_softirq() instead own warning
On Fri, May 04, 2018 at 07:51:44PM +0200, Sebastian Andrzej Siewior wrote: > From: Anna-Maria Gleixner > > 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.
[RFC PATCH 2/2] net: mac808211: mac802154: use lockdep_assert_in_softirq() instead own warning
From: Anna-Maria Gleixner
The warning in ieee802154_rx() and ieee80211_rx_napi() is there to ensure
the softirq context for the subsequent netif_receive_skb() call. The check
could be moved into the netif_receive_skb() function to prevent all calling
functions implement the checks on their own. Use the lockdep variant for
softirq context check. While at it, add a lockdep based check for irq
enabled as mentioned in the comment above netif_receive_skb().
Signed-off-by: Anna-Maria Gleixner
Signed-off-by: Sebastian Andrzej Siewior
---
net/core/dev.c | 3 +++
net/mac80211/rx.c | 2 --
net/mac802154/rx.c | 2 --
3 files changed, 3 insertions(+), 4 deletions(-)
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;
--
2.17.0
