[PATCH] batman-adv: bla: use netif_rx_ni when not in interrupt context

[PATCH] batman-adv: bla: use netif_rx_ni when not in interrupt context

[Jussi Kivilinna]

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);
 out:
 	if (primary_if)
 		batadv_hardif_put(primary_if);
-- 
2.25.1

Re: [PATCH] batman-adv: bla: use netif_rx_ni when not in interrupt context

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] batman-adv: bla: use netif_rx_ni when not in interrupt context

[Antonio Quartulli]

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);
> 

-- 
Antonio Quartulli

Is netif_rx_ni safe from interrupt context? (Re: [PATCH] batman-adv: bla: use netif_rx_ni when not in interrupt context)

[Jussi Kivilinna]

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