| Message ID | 20200818144610.8094-1-jussi.kivilinna@haltian.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Simon Wunderlich |
| Headers | show |
| Series | batman-adv: bla: use netif_rx_ni when not in interrupt context | expand |
On Tuesday, 18 August 2020 16:46:10 CEST Jussi Kivilinna wrote: > batadv_bla_send_claim() gets called from worker thread context through > batadv_bla_periodic_work(), thus netif_rx_ni needs to be used in that > case. This fixes "NOHZ: local_softirq_pending 08" log messages seen > when batman-adv is enabled. > > Signed-off-by: Jussi Kivilinna <jussi.kivilinna@haltian.com> > --- > net/batman-adv/bridge_loop_avoidance.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) Fixes: a9ce0dc43e2c ("batman-adv: add basic bridge loop avoidance code") Kind regards, Sven
Hi, On 18/08/2020 16:46, Jussi Kivilinna wrote: > batadv_bla_send_claim() gets called from worker thread context through > batadv_bla_periodic_work(), thus netif_rx_ni needs to be used in that > case. This fixes "NOHZ: local_softirq_pending 08" log messages seen > when batman-adv is enabled. > > Signed-off-by: Jussi Kivilinna <jussi.kivilinna@haltian.com> > --- > net/batman-adv/bridge_loop_avoidance.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c > index 5c41cc52bc53..ab6cec3c7586 100644 > --- a/net/batman-adv/bridge_loop_avoidance.c > +++ b/net/batman-adv/bridge_loop_avoidance.c > @@ -437,7 +437,10 @@ static void batadv_bla_send_claim(struct batadv_priv *bat_priv, u8 *mac, > batadv_add_counter(bat_priv, BATADV_CNT_RX_BYTES, > skb->len + ETH_HLEN); > > - netif_rx(skb); > + if (in_interrupt()) > + netif_rx(skb); > + else > + netif_rx_ni(skb); What's the downside in calling netif_rx_ni() all the times? Is there any possible side effect? (consider this call is not along the fast path) On top of that, I just checked the definition of in_interrupt() and I got this comment: * Note: due to the BH disabled confusion: in_softirq(),in_interrupt() really * should not be used in new code. Check https://elixir.bootlin.com/linux/latest/source/include/linux/preempt.h#L85 Is that something we should consider or is the comment bogus? Regards, > out: > if (primary_if) > batadv_hardif_put(primary_if); >
Hello, +CC netdev mailing-list On 18.8.2020 23.12, Antonio Quartulli wrote: > Hi, > > On 18/08/2020 16:46, Jussi Kivilinna wrote: >> batadv_bla_send_claim() gets called from worker thread context through >> batadv_bla_periodic_work(), thus netif_rx_ni needs to be used in that >> case. This fixes "NOHZ: local_softirq_pending 08" log messages seen >> when batman-adv is enabled. >> >> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@haltian.com> >> --- >> net/batman-adv/bridge_loop_avoidance.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c >> index 5c41cc52bc53..ab6cec3c7586 100644 >> --- a/net/batman-adv/bridge_loop_avoidance.c >> +++ b/net/batman-adv/bridge_loop_avoidance.c >> @@ -437,7 +437,10 @@ static void batadv_bla_send_claim(struct batadv_priv *bat_priv, u8 *mac, >> batadv_add_counter(bat_priv, BATADV_CNT_RX_BYTES, >> skb->len + ETH_HLEN); >> >> - netif_rx(skb); >> + if (in_interrupt()) >> + netif_rx(skb); >> + else >> + netif_rx_ni(skb); > > What's the downside in calling netif_rx_ni() all the times? > Is there any possible side effect? > (consider this call is not along the fast path) Good question. I tried to find answer for this but found documentation being lacking on the issue, so I looked for examples and used 'in_interrupt/netif_rx/netif_rx_ni' bit that appears in few other places: https://elixir.bootlin.com/linux/v5.8/source/drivers/net/caif/caif_hsi.c#L469 https://elixir.bootlin.com/linux/v5.8/source/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c#L425 https://elixir.bootlin.com/linux/v5.8/source/drivers/net/wireless/marvell/libertas/rx.c#L153 https://elixir.bootlin.com/linux/v5.8/source/drivers/net/wireless/marvell/mwifiex/uap_txrx.c#L356 https://elixir.bootlin.com/linux/v5.8/source/net/caif/chnl_net.c#L121 Maybe someone on netdev mailing-list could give hint on this matter - should 'in_interrupt()?netif_rx(skb):netif_rx_ni(skb)' be used if context is not known or is just using 'netif_rx_ni(skb)' ok? > > On top of that, I just checked the definition of in_interrupt() and I > got this comment: > > * Note: due to the BH disabled confusion: in_softirq(),in_interrupt() > really > * should not be used in new code. > > > Check > https://elixir.bootlin.com/linux/latest/source/include/linux/preempt.h#L85 > > Is that something we should consider or is the comment bogus? It very well be that the existing code that I looked at may not be the best for reuse today. -Jussi
diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c index 5c41cc52bc53..ab6cec3c7586 100644 --- a/net/batman-adv/bridge_loop_avoidance.c +++ b/net/batman-adv/bridge_loop_avoidance.c @@ -437,7 +437,10 @@ static void batadv_bla_send_claim(struct batadv_priv *bat_priv, u8 *mac, batadv_add_counter(bat_priv, BATADV_CNT_RX_BYTES, skb->len + ETH_HLEN); - netif_rx(skb); + if (in_interrupt()) + netif_rx(skb); + else + netif_rx_ni(skb); out: if (primary_if) batadv_hardif_put(primary_if);
batadv_bla_send_claim() gets called from worker thread context through batadv_bla_periodic_work(), thus netif_rx_ni needs to be used in that case. This fixes "NOHZ: local_softirq_pending 08" log messages seen when batman-adv is enabled. Signed-off-by: Jussi Kivilinna <jussi.kivilinna@haltian.com> --- net/batman-adv/bridge_loop_avoidance.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)