The function ieee80211_rx_napi can be now called
from a thread context as well, with napi context
being NULL.
Hence add the napi context check before giving out
a warning for softirq count being 0.
Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
Signed-off-by: Rakesh Pillai <[email protected]>
---
net/mac80211/rx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index a88ab6f..1e703f1 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -4652,7 +4652,7 @@ 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);
+ WARN_ON_ONCE(napi && softirq_count() == 0);
if (WARN_ON(status->band >= NUM_NL80211_BANDS))
goto drop;
--
2.7.4
On Tue, 2020-07-21 at 22:44 +0530, Rakesh Pillai wrote:
> The function ieee80211_rx_napi can be now called
> from a thread context as well, with napi context
> being NULL.
>
> Hence add the napi context check before giving out
> a warning for softirq count being 0.
>
> Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
>
> Signed-off-by: Rakesh Pillai <[email protected]>
> ---
> net/mac80211/rx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index a88ab6f..1e703f1 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -4652,7 +4652,7 @@ 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);
> + WARN_ON_ONCE(napi && softirq_count() == 0);
FWIW, I'm pretty sure this is incorrect - we make assumptions on
softirqs being disabled in mac80211 for serialization and in place of
some locking, I believe.
johannes
> -----Original Message-----
> From: Johannes Berg <[email protected]>
> Sent: Wednesday, July 22, 2020 6:26 PM
> To: Rakesh Pillai <[email protected]>; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [RFC 1/7] mac80211: Add check for napi handle before
> WARN_ON
>
> On Tue, 2020-07-21 at 22:44 +0530, Rakesh Pillai wrote:
> > The function ieee80211_rx_napi can be now called
> > from a thread context as well, with napi context
> > being NULL.
> >
> > Hence add the napi context check before giving out
> > a warning for softirq count being 0.
> >
> > Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
> >
> > Signed-off-by: Rakesh Pillai <[email protected]>
> > ---
> > net/mac80211/rx.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > index a88ab6f..1e703f1 100644
> > --- a/net/mac80211/rx.c
> > +++ b/net/mac80211/rx.c
> > @@ -4652,7 +4652,7 @@ 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);
> > + WARN_ON_ONCE(napi && softirq_count() == 0);
>
> FWIW, I'm pretty sure this is incorrect - we make assumptions on
> softirqs being disabled in mac80211 for serialization and in place of
> some locking, I believe.
>
I checked this, but let me double confirm.
But after this change, no packet is submitted from driver in a softirq context.
So ideally this should take care of serialization.
> johannes
On Thu, 2020-07-23 at 23:56 +0530, Rakesh Pillai wrote:
> > > - WARN_ON_ONCE(softirq_count() == 0);
> > > + WARN_ON_ONCE(napi && softirq_count() == 0);
> >
> > FWIW, I'm pretty sure this is incorrect - we make assumptions on
> > softirqs being disabled in mac80211 for serialization and in place of
> > some locking, I believe.
> >
>
> I checked this, but let me double confirm.
> But after this change, no packet is submitted from driver in a softirq context.
> So ideally this should take care of serialization.
I'd guess that we have some reliance on BHs already being disabled, for
things like u64 sync updates, or whatnot. I mean, we did "rx_ni()" for a
reason ... Maybe lockdep can help catch some of the issues.
But couldn't you be in a thread and have BHs disabled too?
johannes
> -----Original Message-----
> From: Rakesh Pillai <[email protected]>
> Sent: Friday, July 24, 2020 11:51 AM
> To: 'Johannes Berg' <[email protected]>;
> '[email protected]' <[email protected]>
> Cc: '[email protected]' <[email protected]>;
> '[email protected]' <[email protected]>;
> '[email protected]' <[email protected]>; '[email protected]'
> <[email protected]>; '[email protected]' <[email protected]>;
> '[email protected]' <[email protected]>;
> '[email protected]' <[email protected]>;
> '[email protected]' <[email protected]>
> Subject: RE: [RFC 1/7] mac80211: Add check for napi handle before
> WARN_ON
>
>
>
> > -----Original Message-----
> > From: Johannes Berg <[email protected]>
> > Sent: Friday, July 24, 2020 1:37 AM
> > To: Rakesh Pillai <[email protected]>; [email protected]
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> [email protected]
> > Subject: Re: [RFC 1/7] mac80211: Add check for napi handle before
> > WARN_ON
> >
> > On Thu, 2020-07-23 at 23:56 +0530, Rakesh Pillai wrote:
> >
> > > > > - WARN_ON_ONCE(softirq_count() == 0);
> > > > > + WARN_ON_ONCE(napi && softirq_count() == 0);
> > > >
> > > > FWIW, I'm pretty sure this is incorrect - we make assumptions on
> > > > softirqs being disabled in mac80211 for serialization and in place of
> > > > some locking, I believe.
> > > >
> > >
> > > I checked this, but let me double confirm.
> > > But after this change, no packet is submitted from driver in a softirq
> > context.
> > > So ideally this should take care of serialization.
> >
> > I'd guess that we have some reliance on BHs already being disabled, for
> > things like u64 sync updates, or whatnot. I mean, we did "rx_ni()" for a
> > reason ... Maybe lockdep can help catch some of the issues.
> >
> > But couldn't you be in a thread and have BHs disabled too?
>
> This would ideally beat the purpose and possibly hurt the other subsystems
> running on the same core.
>
Hi Johannes,
We do have the usage of napi_gro_receive and netif_receive_skb in mac80211.
/* deliver to local stack */
if (rx->napi)
napi_gro_receive(rx->napi, skb);
else
netif_receive_skb(skb);
Also all the rx_handlers are called under the " rx->local->rx_path_lock" lock.
Is the BH disable still required ?
> >
> > johannes
On Sun, 2020-07-26 at 21:49 +0530, Rakesh Pillai wrote:
> We do have the usage of napi_gro_receive and netif_receive_skb in mac80211.
> /* deliver to local stack */
> if (rx->napi)
> napi_gro_receive(rx->napi, skb);
> else
> netif_receive_skb(skb);
>
>
> Also all the rx_handlers are called under the " rx->local->rx_path_lock" lock.
> Is the BH disable still required ?
I tend to think so, but you're welcome to show that it's not. The lock
serves a different purpose.
TBH, I don't have all of this in my head at all times either, so please
do your own work and analyse why it may or may not be necessary. But
without any such analysis I'm not going to take patches that change it.
johannes