* [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
@ 2024-02-08 0:42 ` Alan Brady
0 siblings, 0 replies; 20+ messages in thread
From: Alan Brady @ 2024-02-08 0:42 UTC (permalink / raw)
To: intel-wired-lan
Cc: netdev, Emil Tantilov, Jesse Brandeburg, Przemek Kitszel, 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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
@ 2024-02-08 0:42 ` Alan Brady
0 siblings, 0 replies; 20+ messages in thread
From: Alan Brady @ 2024-02-08 0:42 UTC (permalink / raw)
To: intel-wired-lan
Cc: Przemek Kitszel, netdev, Emil Tantilov, Alan Brady, Jesse Brandeburg
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
2024-02-08 0:42 ` [Intel-wired-lan] " Alan Brady
@ 2024-02-09 10:34 ` Simon Horman
-1 siblings, 0 replies; 20+ messages in thread
From: Simon Horman @ 2024-02-09 10:34 UTC (permalink / raw)
To: Alan Brady
Cc: Przemek Kitszel, netdev, intel-wired-lan, Emil Tantilov,
Jesse Brandeburg
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>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
@ 2024-02-09 10:34 ` Simon Horman
0 siblings, 0 replies; 20+ messages in thread
From: Simon Horman @ 2024-02-09 10:34 UTC (permalink / raw)
To: Alan Brady
Cc: intel-wired-lan, netdev, Emil Tantilov, Jesse Brandeburg,
Przemek Kitszel
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>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
2024-02-08 0:42 ` [Intel-wired-lan] " Alan Brady
@ 2024-02-12 14:41 ` Alexander Lobakin
-1 siblings, 0 replies; 20+ messages in thread
From: Alexander Lobakin @ 2024-02-12 14:41 UTC (permalink / raw)
To: Alan Brady
Cc: Jesse Brandeburg, Przemek Kitszel, intel-wired-lan,
Emil Tantilov, netdev
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
@ 2024-02-12 14:41 ` Alexander Lobakin
0 siblings, 0 replies; 20+ messages in thread
From: Alexander Lobakin @ 2024-02-12 14:41 UTC (permalink / raw)
To: Alan Brady
Cc: intel-wired-lan, Przemek Kitszel, netdev, Emil Tantilov,
Jesse Brandeburg
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
2024-02-12 14:41 ` Alexander Lobakin
@ 2024-02-12 17:43 ` Tantilov, Emil S
-1 siblings, 0 replies; 20+ messages in thread
From: Tantilov, Emil S @ 2024-02-12 17:43 UTC (permalink / raw)
To: Alexander Lobakin, Alan Brady
Cc: intel-wired-lan, Przemek Kitszel, netdev, Jesse Brandeburg
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
@ 2024-02-12 17:43 ` Tantilov, Emil S
0 siblings, 0 replies; 20+ messages in thread
From: Tantilov, Emil S @ 2024-02-12 17:43 UTC (permalink / raw)
To: Alexander Lobakin, Alan Brady
Cc: Przemek Kitszel, intel-wired-lan, Jesse Brandeburg, netdev
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
2024-02-08 0:42 ` [Intel-wired-lan] " Alan Brady
@ 2024-02-13 13:16 ` Alexander Lobakin
-1 siblings, 0 replies; 20+ messages in thread
From: Alexander Lobakin @ 2024-02-13 13:16 UTC (permalink / raw)
To: Alan Brady
Cc: intel-wired-lan, Przemek Kitszel, netdev, Emil Tantilov,
Jesse Brandeburg
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
@ 2024-02-13 13:16 ` Alexander Lobakin
0 siblings, 0 replies; 20+ messages in thread
From: Alexander Lobakin @ 2024-02-13 13:16 UTC (permalink / raw)
To: Alan Brady
Cc: Jesse Brandeburg, Przemek Kitszel, intel-wired-lan,
Emil Tantilov, netdev
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
2024-02-13 13:16 ` Alexander Lobakin
@ 2024-02-14 14:54 ` Alexander Lobakin
-1 siblings, 0 replies; 20+ messages in thread
From: Alexander Lobakin @ 2024-02-14 14:54 UTC (permalink / raw)
To: Alan Brady
Cc: Przemek Kitszel, netdev, intel-wired-lan, Jesse Brandeburg,
Emil Tantilov
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
@ 2024-02-14 14:54 ` Alexander Lobakin
0 siblings, 0 replies; 20+ messages in thread
From: Alexander Lobakin @ 2024-02-14 14:54 UTC (permalink / raw)
To: Alan Brady
Cc: Jesse Brandeburg, Przemek Kitszel, intel-wired-lan,
Emil Tantilov, netdev
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
2024-02-14 14:54 ` Alexander Lobakin
@ 2024-02-14 15:39 ` Tantilov, Emil S
-1 siblings, 0 replies; 20+ messages in thread
From: Tantilov, Emil S @ 2024-02-14 15:39 UTC (permalink / raw)
To: Alexander Lobakin, Alan Brady
Cc: Przemek Kitszel, intel-wired-lan, Jesse Brandeburg, netdev
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
@ 2024-02-14 15:39 ` Tantilov, Emil S
0 siblings, 0 replies; 20+ messages in thread
From: Tantilov, Emil S @ 2024-02-14 15:39 UTC (permalink / raw)
To: Alexander Lobakin, Alan Brady
Cc: Jesse Brandeburg, Przemek Kitszel, intel-wired-lan, netdev
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
2024-02-13 13:16 ` Alexander Lobakin
@ 2024-02-15 0:36 ` Jakub Kicinski
-1 siblings, 0 replies; 20+ messages in thread
From: Jakub Kicinski @ 2024-02-15 0:36 UTC (permalink / raw)
To: Alexander Lobakin
Cc: Alan Brady, intel-wired-lan, Przemek Kitszel, netdev,
Emil Tantilov, Jesse Brandeburg
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
@ 2024-02-15 0:36 ` Jakub Kicinski
0 siblings, 0 replies; 20+ messages in thread
From: Jakub Kicinski @ 2024-02-15 0:36 UTC (permalink / raw)
To: Alexander Lobakin
Cc: Emil Tantilov, Przemek Kitszel, Jesse Brandeburg,
intel-wired-lan, Alan Brady, netdev
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
2024-02-14 15:39 ` Tantilov, Emil S
@ 2024-02-15 13:28 ` Alexander Lobakin
-1 siblings, 0 replies; 20+ messages in thread
From: Alexander Lobakin @ 2024-02-15 13:28 UTC (permalink / raw)
To: Tantilov, Emil S
Cc: Alan Brady, Jesse Brandeburg, Przemek Kitszel, intel-wired-lan, netdev
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
@ 2024-02-15 13:28 ` Alexander Lobakin
0 siblings, 0 replies; 20+ messages in thread
From: Alexander Lobakin @ 2024-02-15 13:28 UTC (permalink / raw)
To: Tantilov, Emil S
Cc: Przemek Kitszel, netdev, intel-wired-lan, Alan Brady, Jesse Brandeburg
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
2024-02-08 0:42 ` [Intel-wired-lan] " Alan Brady
@ 2024-03-05 0:44 ` Singh, Krishneil K
-1 siblings, 0 replies; 20+ messages in thread
From: Singh, Krishneil K @ 2024-03-05 0:44 UTC (permalink / raw)
To: Brady, Alan, intel-wired-lan
Cc: netdev, Tantilov, Emil S, Brandeburg, Jesse, Kitszel, Przemyslaw,
Brady, Alan
> -----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>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-wired-lan] [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets
@ 2024-03-05 0:44 ` Singh, Krishneil K
0 siblings, 0 replies; 20+ messages in thread
From: Singh, Krishneil K @ 2024-03-05 0:44 UTC (permalink / raw)
To: Brady, Alan, intel-wired-lan
Cc: Kitszel, Przemyslaw, netdev, Tantilov, Emil S, Brady, Alan
> -----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>
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2024-03-05 0:44 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-08 0:42 [PATCH 1/1 iwl-net] idpf: disable local BH when scheduling napi for marker packets Alan Brady
2024-02-08 0:42 ` [Intel-wired-lan] " Alan Brady
2024-02-09 10:34 ` Simon Horman
2024-02-09 10:34 ` Simon Horman
2024-02-12 14:41 ` [Intel-wired-lan] " Alexander Lobakin
2024-02-12 14:41 ` Alexander Lobakin
2024-02-12 17:43 ` Tantilov, Emil S
2024-02-12 17:43 ` Tantilov, Emil S
2024-02-13 13:16 ` Alexander Lobakin
2024-02-13 13:16 ` Alexander Lobakin
2024-02-14 14:54 ` Alexander Lobakin
2024-02-14 14:54 ` Alexander Lobakin
2024-02-14 15:39 ` Tantilov, Emil S
2024-02-14 15:39 ` Tantilov, Emil S
2024-02-15 13:28 ` Alexander Lobakin
2024-02-15 13:28 ` Alexander Lobakin
2024-02-15 0:36 ` Jakub Kicinski
2024-02-15 0:36 ` Jakub Kicinski
2024-03-05 0:44 ` Singh, Krishneil K
2024-03-05 0:44 ` [Intel-wired-lan] " Singh, Krishneil K
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.