| Message ID | 20240208004243.1762223-1-alan.brady@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1,iwl-net] idpf: disable local BH when scheduling napi for marker packets | expand |
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>
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
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
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
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
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
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.
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
> -----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>
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); }