| Message ID | 1595351666-28193-2-git-send-email-pillair@codeaurora.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Add support to process rx packets in thread | expand |
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 <pillair@codeaurora.org> > --- > 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 <johannes@sipsolutions.net> > Sent: Wednesday, July 22, 2020 6:26 PM > To: Rakesh Pillai <pillair@codeaurora.org>; ath10k@lists.infradead.org > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > kvalo@codeaurora.org; davem@davemloft.net; kuba@kernel.org; > netdev@vger.kernel.org; dianders@chromium.org; evgreen@chromium.org > 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 <pillair@codeaurora.org> > > --- > > 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: Johannes Berg <johannes@sipsolutions.net> > Sent: Friday, July 24, 2020 1:37 AM > To: Rakesh Pillai <pillair@codeaurora.org>; ath10k@lists.infradead.org > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > kvalo@codeaurora.org; davem@davemloft.net; kuba@kernel.org; > netdev@vger.kernel.org; dianders@chromium.org; evgreen@chromium.org > 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. > > johannes
> -----Original Message----- > From: Rakesh Pillai <pillair@codeaurora.org> > Sent: Friday, July 24, 2020 11:51 AM > To: 'Johannes Berg' <johannes@sipsolutions.net>; > 'ath10k@lists.infradead.org' <ath10k@lists.infradead.org> > Cc: 'linux-wireless@vger.kernel.org' <linux-wireless@vger.kernel.org>; > 'linux-kernel@vger.kernel.org' <linux-kernel@vger.kernel.org>; > 'kvalo@codeaurora.org' <kvalo@codeaurora.org>; 'davem@davemloft.net' > <davem@davemloft.net>; 'kuba@kernel.org' <kuba@kernel.org>; > 'netdev@vger.kernel.org' <netdev@vger.kernel.org>; > 'dianders@chromium.org' <dianders@chromium.org>; > 'evgreen@chromium.org' <evgreen@chromium.org> > Subject: RE: [RFC 1/7] mac80211: Add check for napi handle before > WARN_ON > > > > > -----Original Message----- > > From: Johannes Berg <johannes@sipsolutions.net> > > Sent: Friday, July 24, 2020 1:37 AM > > To: Rakesh Pillai <pillair@codeaurora.org>; ath10k@lists.infradead.org > > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > > kvalo@codeaurora.org; davem@davemloft.net; kuba@kernel.org; > > netdev@vger.kernel.org; dianders@chromium.org; > evgreen@chromium.org > > 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
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;
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 <pillair@codeaurora.org> --- net/mac80211/rx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)