[PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets

[PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets

[Alan Brady]

From: Emil Tantilov <emil.s.tantilov@intel.com>

Fix softirq's not being handled during napi_schedule() call when
receiving marker packets for queue disable by disabling local bottom
half.

The issue can be seen on ifdown:
NOHZ tick-stop error: Non-RCU local softirq work is pending, handler #08!!!

Using ftrace to catch the failing scenario:
ifconfig   [003] d.... 22739.830624: softirq_raise: vec=3 [action=NET_RX]
<idle>-0   [003] ..s.. 22739.831357: softirq_entry: vec=3 [action=NET_RX]

No interrupt and CPU is idle.

After the patch, with BH locks:
ifconfig   [003] d.... 22993.928336: softirq_raise: vec=3 [action=NET_RX]
ifconfig   [003] ..s1. 22993.928337: softirq_entry: vec=3 [action=NET_RX]

Fixes: c2d548cad150 ("idpf: add TX splitq napi poll support")
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Signed-off-by: Alan Brady <alan.brady@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index d0cdd63b3d5b..390977a76de2 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -2087,8 +2087,10 @@ int idpf_send_disable_queues_msg(struct idpf_vport *vport)
 		set_bit(__IDPF_Q_POLL_MODE, vport->txqs[i]->flags);
 
 	/* schedule the napi to receive all the marker packets */
+	local_bh_disable();
 	for (i = 0; i < vport->num_q_vectors; i++)
 		napi_schedule(&vport->q_vectors[i].napi);
+	local_bh_enable();
 
 	return idpf_wait_for_marker_event(vport);
 }
-- 
2.43.0

Re: [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets

[Simon Horman]

On Wed, Feb 07, 2024 at 04:42:43PM -0800, Alan Brady wrote:
> From: Emil Tantilov <emil.s.tantilov@intel.com>
> 
> Fix softirq's not being handled during napi_schedule() call when
> receiving marker packets for queue disable by disabling local bottom
> half.
> 
> The issue can be seen on ifdown:
> NOHZ tick-stop error: Non-RCU local softirq work is pending, handler #08!!!
> 
> Using ftrace to catch the failing scenario:
> ifconfig   [003] d.... 22739.830624: softirq_raise: vec=3 [action=NET_RX]
> <idle>-0   [003] ..s.. 22739.831357: softirq_entry: vec=3 [action=NET_RX]
> 
> No interrupt and CPU is idle.
> 
> After the patch, with BH locks:
> ifconfig   [003] d.... 22993.928336: softirq_raise: vec=3 [action=NET_RX]
> ifconfig   [003] ..s1. 22993.928337: softirq_entry: vec=3 [action=NET_RX]
> 
> Fixes: c2d548cad150 ("idpf: add TX splitq napi poll support")
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Signed-off-by: Alan Brady <alan.brady@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets

[Alexander Lobakin]

From: Alan Brady <alan.brady@intel.com>
Date: Wed,  7 Feb 2024 16:42:43 -0800

> From: Emil Tantilov <emil.s.tantilov@intel.com>
> 
> Fix softirq's not being handled during napi_schedule() call when
> receiving marker packets for queue disable by disabling local bottom
> half.
> 
> The issue can be seen on ifdown:
> NOHZ tick-stop error: Non-RCU local softirq work is pending, handler #08!!!
> 
> Using ftrace to catch the failing scenario:
> ifconfig   [003] d.... 22739.830624: softirq_raise: vec=3 [action=NET_RX]
> <idle>-0   [003] ..s.. 22739.831357: softirq_entry: vec=3 [action=NET_RX]
> 
> No interrupt and CPU is idle.
> 
> After the patch, with BH locks:

Minor: local_bh_{en,dis}able() are not "BH locks", it's BH
enabling/disabling. It doesn't lock/unlock anything.

> ifconfig   [003] d.... 22993.928336: softirq_raise: vec=3 [action=NET_RX]
> ifconfig   [003] ..s1. 22993.928337: softirq_entry: vec=3 [action=NET_RX]
> 
> Fixes: c2d548cad150 ("idpf: add TX splitq napi poll support")
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Signed-off-by: Alan Brady <alan.brady@intel.com>

Thanks,
Olek

Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets

[Tantilov, Emil S]

On 2/12/2024 6:41 AM, Alexander Lobakin wrote:
> From: Alan Brady <alan.brady@intel.com>
> Date: Wed,  7 Feb 2024 16:42:43 -0800
> 
>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>>
>> Fix softirq's not being handled during napi_schedule() call when
>> receiving marker packets for queue disable by disabling local bottom
>> half.
>>
>> The issue can be seen on ifdown:
>> NOHZ tick-stop error: Non-RCU local softirq work is pending, handler #08!!!
>>
>> Using ftrace to catch the failing scenario:
>> ifconfig   [003] d.... 22739.830624: softirq_raise: vec=3 [action=NET_RX]
>> <idle>-0   [003] ..s.. 22739.831357: softirq_entry: vec=3 [action=NET_RX]
>>
>> No interrupt and CPU is idle.
>>
>> After the patch, with BH locks:
> 
> Minor: local_bh_{en,dis}able() are not "BH locks", it's BH
> enabling/disabling. It doesn't lock/unlock anything.

Good catch, we can change it to:
"After the patch when disabling local BH before calling napi_schedule:"

> 
>> ifconfig   [003] d.... 22993.928336: softirq_raise: vec=3 [action=NET_RX]
>> ifconfig   [003] ..s1. 22993.928337: softirq_entry: vec=3 [action=NET_RX]
>>
>> Fixes: c2d548cad150 ("idpf: add TX splitq napi poll support")
>> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
>> Signed-off-by: Alan Brady <alan.brady@intel.com>
> 
> Thanks,
> Olek

Thanks,
Emil

Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets

[Alexander Lobakin]

From: Alan Brady <alan.brady@intel.com>
Date: Wed,  7 Feb 2024 16:42:43 -0800

> From: Emil Tantilov <emil.s.tantilov@intel.com>
> 
> Fix softirq's not being handled during napi_schedule() call when
> receiving marker packets for queue disable by disabling local bottom
> half.

BTW, how exactly does this help?

__napi_schedule() already disables interrupts (local_irq_save()).
napi_schedule_prep() only has READ_ONCE() and other atomic read/write
helpers.

It's always been safe to call napi_schedule() with enabled BH, so I
don't really understand how this works.

> 
> The issue can be seen on ifdown:
> NOHZ tick-stop error: Non-RCU local softirq work is pending, handler #08!!!
> 
> Using ftrace to catch the failing scenario:
> ifconfig   [003] d.... 22739.830624: softirq_raise: vec=3 [action=NET_RX]
> <idle>-0   [003] ..s.. 22739.831357: softirq_entry: vec=3 [action=NET_RX]
> 
> No interrupt and CPU is idle.
> 
> After the patch, with BH locks:
> ifconfig   [003] d.... 22993.928336: softirq_raise: vec=3 [action=NET_RX]
> ifconfig   [003] ..s1. 22993.928337: softirq_entry: vec=3 [action=NET_RX]
> 
> Fixes: c2d548cad150 ("idpf: add TX splitq napi poll support")
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Signed-off-by: Alan Brady <alan.brady@intel.com>
> ---
>  drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index d0cdd63b3d5b..390977a76de2 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -2087,8 +2087,10 @@ int idpf_send_disable_queues_msg(struct idpf_vport *vport)
>  		set_bit(__IDPF_Q_POLL_MODE, vport->txqs[i]->flags);
>  
>  	/* schedule the napi to receive all the marker packets */
> +	local_bh_disable();
>  	for (i = 0; i < vport->num_q_vectors; i++)
>  		napi_schedule(&vport->q_vectors[i].napi);
> +	local_bh_enable();
>  
>  	return idpf_wait_for_marker_event(vport);
>  }

Thanks,
Olek

Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets

[Alexander Lobakin]

From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: Tue, 13 Feb 2024 14:16:47 +0100

> From: Alan Brady <alan.brady@intel.com>
> Date: Wed,  7 Feb 2024 16:42:43 -0800
> 
>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>>
>> Fix softirq's not being handled during napi_schedule() call when
>> receiving marker packets for queue disable by disabling local bottom
>> half.
> 
> BTW, how exactly does this help?
> 
> __napi_schedule() already disables interrupts (local_irq_save()).
> napi_schedule_prep() only has READ_ONCE() and other atomic read/write
> helpers.
> 
> It's always been safe to call napi_schedule() with enabled BH, so I
> don't really understand how this works.

This also needs to be dropped from the fixes queue until investigated.
For now, it looks like a cheap hack (without the explanation how exactly
it does help), not a proper fix.

Thanks,
Olek

Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets

[Tantilov, Emil S]

On 2/14/2024 6:54 AM, Alexander Lobakin wrote:
> From: Alexander Lobakin <aleksander.lobakin@intel.com>
> Date: Tue, 13 Feb 2024 14:16:47 +0100
> 
>> From: Alan Brady <alan.brady@intel.com>
>> Date: Wed,  7 Feb 2024 16:42:43 -0800
>>
>>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>>>
>>> Fix softirq's not being handled during napi_schedule() call when
>>> receiving marker packets for queue disable by disabling local bottom
>>> half.
>>
>> BTW, how exactly does this help?
>>
>> __napi_schedule() already disables interrupts (local_irq_save()).
>> napi_schedule_prep() only has READ_ONCE() and other atomic read/write
>> helpers.
>>
>> It's always been safe to call napi_schedule() with enabled BH, so I
>> don't really understand how this works.

It's been a while since I debugged this, I'll have to take a look again, 
but its not so much about being safe as it is about making sure the 
marker packets are received in those cases - like ifdown in the trace.

> This also needs to be dropped from the fixes queue until investigated.
> For now, it looks like a cheap hack (without the explanation how exactly
> it does help), not a proper fix.

It does fix the issue at hand. Looking at the kernel code I see multiple 
examples of napi_schedule() being wrapped with local_bh_disable/enable, 
so this appears to be a common (not uncommon?) technique.

Thanks,
Emil

> 
> Thanks,
> Olek

Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets

[Alexander Lobakin]

From: Tantilov, Emil S <emil.s.tantilov@intel.com>
Date: Wed, 14 Feb 2024 07:39:53 -0800

> 
> 
> On 2/14/2024 6:54 AM, Alexander Lobakin wrote:
>> From: Alexander Lobakin <aleksander.lobakin@intel.com>
>> Date: Tue, 13 Feb 2024 14:16:47 +0100
>>
>>> From: Alan Brady <alan.brady@intel.com>
>>> Date: Wed,  7 Feb 2024 16:42:43 -0800
>>>
>>>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>>>>
>>>> Fix softirq's not being handled during napi_schedule() call when
>>>> receiving marker packets for queue disable by disabling local bottom
>>>> half.
>>>
>>> BTW, how exactly does this help?
>>>
>>> __napi_schedule() already disables interrupts (local_irq_save()).
>>> napi_schedule_prep() only has READ_ONCE() and other atomic read/write
>>> helpers.
>>>
>>> It's always been safe to call napi_schedule() with enabled BH, so I
>>> don't really understand how this works.
> 
> It's been a while since I debugged this, I'll have to take a look again,
> but its not so much about being safe as it is about making sure the
> marker packets are received in those cases - like ifdown in the trace.
> 
>> This also needs to be dropped from the fixes queue until investigated.
>> For now, it looks like a cheap hack (without the explanation how exactly
>> it does help), not a proper fix.
> 
> It does fix the issue at hand. Looking at the kernel code I see multiple

Sometimes adding debug prints fixes bugs, fixing something doesn't mean
it's the correct way.

> examples of napi_schedule() being wrapped with local_bh_disable/enable,

"Everybody do that" doesn't prove anything until explained how exactly
this helps.

> so this appears to be a common (not uncommon?) technique.
> 
> Thanks,
> Emil
> 
>>
>> Thanks,
>> Olek

Thanks,
Olek

Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets

[Jakub Kicinski]

On Tue, 13 Feb 2024 14:16:47 +0100 Alexander Lobakin wrote:
> > Fix softirq's not being handled during napi_schedule() call when
> > receiving marker packets for queue disable by disabling local bottom
> > half.  
> 
> BTW, how exactly does this help?
> 
> __napi_schedule() already disables interrupts (local_irq_save()).
> napi_schedule_prep() only has READ_ONCE() and other atomic read/write
> helpers.
> 
> It's always been safe to call napi_schedule() with enabled BH, so I
> don't really understand how this works.

Sorry for late reply. IIRC the problem isn't a race but the fact that
local_irq_restore() does not have a hook for BH. IOW we may just set 
the bit that the BH is pending but never call into softirq.c to run it.

RE: [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets

[Singh, Krishneil K]

> -----Original Message-----
> From: Alan Brady <alan.brady@intel.com>
> Sent: Wednesday, February 7, 2024 4:43 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Tantilov, Emil S <emil.s.tantilov@intel.com>;
> Brandeburg, Jesse <jesse.brandeburg@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Brady, Alan <alan.brady@intel.com>
> Subject: [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for
> marker packets
> 
> From: Emil Tantilov <emil.s.tantilov@intel.com>
> 
> Fix softirq's not being handled during napi_schedule() call when
> receiving marker packets for queue disable by disabling local bottom
> half.
> 
> The issue can be seen on ifdown:
> NOHZ tick-stop error: Non-RCU local softirq work is pending, handler #08!!!
> 
> Using ftrace to catch the failing scenario:
> ifconfig   [003] d.... 22739.830624: softirq_raise: vec=3 [action=NET_RX]
> <idle>-0   [003] ..s.. 22739.831357: softirq_entry: vec=3 [action=NET_RX]
> 
> No interrupt and CPU is idle.
> 
> After the patch, with BH locks:
> ifconfig   [003] d.... 22993.928336: softirq_raise: vec=3 [action=NET_RX]
> ifconfig   [003] ..s1. 22993.928337: softirq_entry: vec=3 [action=NET_RX]
> 
> Fixes: c2d548cad150 ("idpf: add TX splitq napi poll support")
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Signed-off-by: Alan Brady <alan.brady@intel.com>
> ---
>  drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index d0cdd63b3d5b..390977a76de2 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c

Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>