net: Handle napi_schedule() calls from non-interrupt
Subsystems (2)
PATCH v2 1/1 net: Handle napi_schedule() calls from non-interrupt
napi_schedule() is expected to be called either:
* From an interrupt, where raised softirqs are handled on IRQ exit
* From a softirq disabled section, where raised softirqs are handled on the next call to local_bh_enable().
* From a softirq handler, where raised softirqs are handled on the next round in do_softirq(), or further deferred to a dedicated kthread.
Other bare tasks context may end up ignoring the raised NET_RX vector until the next random softirq handling opportunity, which may not happen before a while if the CPU goes idle afterwards with the tick stopped.
Such "misuses" have been detected on several places thanks to messages of the kind:
"NOHZ tick-stop error: local softirq work is pending, handler #08!!!"
For example:
__raise_softirq_irqoff
__napi_schedule
rtl8152_runtime_resume.isra.0
rtl8152_resume
usb_resume_interface.isra.0
usb_resume_both
__rpm_callback
rpm_callback
rpm_resume
__pm_runtime_resume
usb_autoresume_device
usb_remote_wakeup
hub_event
process_one_work
worker_thread
kthread
ret_from_fork
ret_from_fork_asmAnd also:
* drivers/net/usb/r8152.c::rtl_work_func_t
* drivers/net/netdevsim/netdev.c::nsim_start_xmit
There is a long history of issues of this kind:
019edd01d174 ("ath10k: sdio: Add missing BH locking around napi_schdule()")
330068589389 ("idpf: disable local BH when scheduling napi for marker packets")
e3d5d70cb483 ("net: lan78xx: fix "softirq work is pending" error")
e55c27ed9ccf ("mt76: mt7615: add missing bh-disable around rx napi schedule")
c0182aa98570 ("mt76: mt7915: add missing bh-disable around tx napi enable/schedule")
970be1dff26d ("mt76: disable BH around napi_schedule() calls")
019edd01d174 ("ath10k: sdio: Add missing BH locking around napi_schdule()")
30bfec4fec59 ("can: rx-offload: can_rx_offload_threaded_irq_finish(): add new function to be called from threaded interrupt")
e63052a5dd3c ("mlx5e: add add missing BH locking around napi_schdule()")
83a0c6e58901 ("i40e: Invoke softirqs after napi_reschedule")
bd4ce941c8d5 ("mlx4: Invoke softirqs after napi_reschedule")
8cf699ec849f ("mlx4: do not call napi_schedule() without care")
ec13ee80145c ("virtio_net: invoke softirqs after __napi_schedule")This shows that relying on the caller to arrange a proper context for the softirqs to be handled while calling napi_schedule() is very fragile and error prone. Also fixing them can also prove challenging if the caller may be called from different kinds of contexts.
Therefore fix this from napi_schedule() itself with waking up ksoftirqd when softirqs are raised from task contexts.
--- net/core/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/core/dev.c +1 -1
@@ -4693,7 +4693,7 @@ static inline void ____napi_schedule(struct softnet_data *sd,* we have to raise NET_RX_SOFTIRQ. */ if (!sd->in_net_rx_action) -__raise_softirq_irqoff (NET_RX_SOFTIRQ ); +raise_softirq_irqoff (NET_RX_SOFTIRQ ); }#ifdef CONFIG_RPS
Signature · 1 line
2.48.1
Quoted 91 lines · Frederic Weisbecker
napi_schedule()is expected to be called either:* From an interrupt, where raised softirqs are handled on IRQ exit
* From a softirq disabled section, where raised softirqs are handled on the next call to
local_bh_enable().* From a softirq handler, where raised softirqs are handled on the next round in
do_softirq(), or further deferred to a dedicated kthread.Other bare tasks context may end up ignoring the raised
NET_RXvector until the next random softirq handling opportunity, which may not happen before a while if the CPU goes idle afterwards with the tick stopped.Such "misuses" have been detected on several places thanks to messages of the kind:
"NOHZ tick-stop error: local softirq work is pending, handler #08!!!"
For example:
__raise_softirq_irqoff __napi_schedule rtl8152_runtime_resume.isra.0 rtl8152_resume usb_resume_interface.isra.0 usb_resume_both __rpm_callback rpm_callback rpm_resume __pm_runtime_resume usb_autoresume_device usb_remote_wakeup hub_event process_one_work worker_thread kthread ret_from_fork ret_from_fork_asmAnd also:
* drivers/net/usb/r8152.c::rtl_work_func_t
* drivers/net/netdevsim/netdev.c::nsim_start_xmitThere is a long history of issues of this kind:
019edd01d174 ("ath10k: sdio: Add missing BH locking around napi_schdule()") 330068589389 ("idpf: disable local BH when scheduling napi for marker packets") e3d5d70cb483 ("net: lan78xx: fix "softirq work is pending" error") e55c27ed9ccf ("mt76: mt7615: add missing bh-disable around rx napi schedule") c0182aa98570 ("mt76: mt7915: add missing bh-disable around tx napi enable/schedule") 970be1dff26d ("mt76: disable BH around napi_schedule() calls") 019edd01d174 ("ath10k: sdio: Add missing BH locking around napi_schdule()") 30bfec4fec59 ("can: rx-offload: can_rx_offload_threaded_irq_finish(): add new function to be called from threaded interrupt") e63052a5dd3c ("mlx5e: add add missing BH locking around napi_schdule()") 83a0c6e58901 ("i40e: Invoke softirqs after napi_reschedule") bd4ce941c8d5 ("mlx4: Invoke softirqs after napi_reschedule") 8cf699ec849f ("mlx4: do not call napi_schedule() without care") ec13ee80145c ("virtio_net: invoke softirqs after __napi_schedule")This shows that relying on the caller to arrange a proper context for the softirqs to be handled while calling
napi_schedule()is very fragile and error prone. Also fixing them can also prove challenging if the caller may be called from different kinds of contexts.Therefore fix this from
napi_schedule()itself with waking up ksoftirqd when softirqs are raised from task contexts.Reported-by Paul Menzel <[email protected]>Reported-by Jakub Kicinski <[email protected]>Reported-by Francois Romieu <[email protected]>Cc Breno Leitao <[email protected]>Cc Jakub Kicinski <[email protected]>Cc "David S. Miller" <[email protected]>Cc Eric Dumazet <[email protected]>Cc Paolo Abeni <[email protected]>Cc Francois Romieu <[email protected]>Signed-off-by Frederic Weisbecker <[email protected]>--- net/core/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/core/dev.c +0
-0
net/core/dev.c static inline void ____napi_schedule(struct softnet_data *sd, · line 4696
Quoted 5 lines
net/core/dev.c +1
-1@@ -4693,7 +4693,7 @@ static inline void ____napi_schedule(struct softnet_data *sd,* we have to raise NET_RX_SOFTIRQ. */ if (!sd->in_net_rx_action) -__raise_softirq_irqoff (NET_RX_SOFTIRQ ); +raise_softirq_irqoff (NET_RX_SOFTIRQ );
Your patch is fine, but would silence performance bugs.
I would probably add something to let network developers be aware of them.
net/core/dev.c +0 -0
100644
net/core/dev.c +0 -0
@@ -4762,8 +4762,10 @@ static inline void ____napi_schedule(struct
softnet_data *sd,
/* If not called from net_rx_action()
* we have to raise NET_RX_SOFTIRQ.
*/- if (!sd->in_net_rx_action)
- __raise_softirq_irqoff(NET_RX_SOFTIRQ);
+ if (!sd->in_net_rx_action) {
+ raise_softirq_irqoff(NET_RX_SOFTIRQ);
+ DEBUG_NET_WARN_ON_ONCE(!in_interrupt());
+ }
}
#ifdef CONFIG_RPS
Looking at the list of patches, I can see idpf fix was not very good, I will submit another patch.
Quoted 109 lines · Eric Dumazet
Quoted 91 lines · Frederic Weisbecker
napi_schedule()is expected to be called either:* From an interrupt, where raised softirqs are handled on IRQ exit
* From a softirq disabled section, where raised softirqs are handled on the next call to
local_bh_enable().* From a softirq handler, where raised softirqs are handled on the next round in
do_softirq(), or further deferred to a dedicated kthread.Other bare tasks context may end up ignoring the raised
NET_RXvector until the next random softirq handling opportunity, which may not happen before a while if the CPU goes idle afterwards with the tick stopped.Such "misuses" have been detected on several places thanks to messages of the kind:
"NOHZ tick-stop error: local softirq work is pending, handler #08!!!"
For example:
__raise_softirq_irqoff __napi_schedule rtl8152_runtime_resume.isra.0 rtl8152_resume usb_resume_interface.isra.0 usb_resume_both __rpm_callback rpm_callback rpm_resume __pm_runtime_resume usb_autoresume_device usb_remote_wakeup hub_event process_one_work worker_thread kthread ret_from_fork ret_from_fork_asmAnd also:
* drivers/net/usb/r8152.c::rtl_work_func_t
* drivers/net/netdevsim/netdev.c::nsim_start_xmitThere is a long history of issues of this kind:
019edd01d174 ("ath10k: sdio: Add missing BH locking around napi_schdule()") 330068589389 ("idpf: disable local BH when scheduling napi for marker packets") e3d5d70cb483 ("net: lan78xx: fix "softirq work is pending" error") e55c27ed9ccf ("mt76: mt7615: add missing bh-disable around rx napi schedule") c0182aa98570 ("mt76: mt7915: add missing bh-disable around tx napi enable/schedule") 970be1dff26d ("mt76: disable BH around napi_schedule() calls") 019edd01d174 ("ath10k: sdio: Add missing BH locking around napi_schdule()") 30bfec4fec59 ("can: rx-offload: can_rx_offload_threaded_irq_finish(): add new function to be called from threaded interrupt") e63052a5dd3c ("mlx5e: add add missing BH locking around napi_schdule()") 83a0c6e58901 ("i40e: Invoke softirqs after napi_reschedule") bd4ce941c8d5 ("mlx4: Invoke softirqs after napi_reschedule") 8cf699ec849f ("mlx4: do not call napi_schedule() without care") ec13ee80145c ("virtio_net: invoke softirqs after __napi_schedule")This shows that relying on the caller to arrange a proper context for the softirqs to be handled while calling
napi_schedule()is very fragile and error prone. Also fixing them can also prove challenging if the caller may be called from different kinds of contexts.Therefore fix this from
napi_schedule()itself with waking up ksoftirqd when softirqs are raised from task contexts.Reported-by Paul Menzel <[email protected]>Reported-by Jakub Kicinski <[email protected]>Reported-by Francois Romieu <[email protected]>Cc Breno Leitao <[email protected]>Cc Jakub Kicinski <[email protected]>Cc "David S. Miller" <[email protected]>Cc Eric Dumazet <[email protected]>Cc Paolo Abeni <[email protected]>Cc Francois Romieu <[email protected]>Signed-off-by Frederic Weisbecker <[email protected]>--- net/core/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/core/dev.c +0
-0net/core/dev.c static inline void ____napi_schedule(struct softnet_data *sd, · line 4696
Quoted 5 lines
net/core/dev.c +1
-1@@ -4693,7 +4693,7 @@ static inline void ____napi_schedule(struct softnet_data *sd,* we have to raise NET_RX_SOFTIRQ. */ if (!sd->in_net_rx_action) -__raise_softirq_irqoff (NET_RX_SOFTIRQ ); +raise_softirq_irqoff (NET_RX_SOFTIRQ );Your patch is fine, but would silence performance bugs.
I would probably add something to let network developers be aware of them.
net/core/dev.c +0
-0100644
net/core/dev.c static inline void ____napi_schedule(struct · line 4769
Quoted 10 lines
net/core/dev.c +4
-2@@ -4762,8 +4762,10 @@ static inline void ____napi_schedule(struct softnet_data *sd,/* If not called from net_rx_action() * we have to raise NET_RX_SOFTIRQ. */ - if (!sd->in_net_rx_action) -__raise_softirq_irqoff (NET_RX_SOFTIRQ ); + if (!sd->in_net_rx_action) { +raise_softirq_irqoff (NET_RX_SOFTIRQ ); +DEBUG_NET_WARN_ON_ONCE (!in_interrupt ()); + }
That looks good and looks like what I did initially:
https://lore.kernel.org/lkml/[email protected]/
Do you prefer me doing it over DEBUG_NET_WARN_ON_ONCE() or with lockdep like in the link?
Thanks.
Quoted 7 lines
net/core/dev.c +0
-0}#ifdef
CONFIG_RPSLooking at the list of patches, I can see idpf fix was not very good, I will submit another patch.
Eric Dumazet
Reviewed-byQuoted 8 lines · Frederic Weisbecker
That looks good and looks like what I did initially:
https://lore.kernel.org/lkml/[email protected]/
Do you prefer me doing it over
DEBUG_NET_WARN_ON_ONCE()or with lockdep like in the link?
To be clear, I have not tried this thing yet.
Perhaps let your patch as is (for stable backports), and put the debug stuff only after some tests, in net-next.
It is very possible that napi_schedule() in the problematic cases were not on a fast path anyway.
Quoted 7 lines · Eric Dumazet
Quoted 6 lines · Frederic Weisbecker
That looks good and looks like what I did initially:
Quoted 7 lines
Quoted 2 lines
Do you prefer me doing it over
DEBUG_NET_WARN_ON_ONCE()or with lockdep like in the link?To be clear, I have not tried this thing yet.
Perhaps let your patch as is (for stable backports), and put the debug stuff only after some tests, in net-next.
Ok.
Quoted 3 lines
It is very possible that
napi_schedule()in the problematic cases were not on a fast path anyway.
That was my assumption but I must confess I don't know well this realm.
Quoted 2 lines
Reviewed-by Eric Dumazet <[email protected]>
Thanks!
Quoted 10 lines · Eric Dumazet
Quoted 8 lines · Frederic Weisbecker
That looks good and looks like what I did initially:
https://lore.kernel.org/lkml/[email protected]/
Do you prefer me doing it over
DEBUG_NET_WARN_ON_ONCE()or with lockdep like in the link?
Quoted 7 lines
To be clear, I have not tried this thing yet.
Perhaps let your patch as is (for stable backports), and put the debug stuff only after some tests, in net-next.
It is very possible that
napi_schedule()in the problematic cases were not on a fast path anyway.
I got here via Sascha's stable backports. It looks to me that these paths (the reported once) are not widely tested and then we don't have any prints if the BH section is missing while expected.
Would it work for everyone if warnings are added and this patch is reverted? These days ksoftirqd is not blocking any softirqs until it run so chances are that the NAPI softirq kicks in before ksoftirqd gets on the CPU so the damage is probably little.
I am slightly undecided here since real work usually originates in softirq and it is hard to get this wrong but then who knows…
Quoted 1 line
Reviewed-by Eric Dumazet <[email protected]>
Sebastian
This patch was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
Quoted 8 lines · On Sun, 23 Feb 2025 23:17:08 +0100 you wrote:
napi_schedule()is expected to be called either:* From an interrupt, where raised softirqs are handled on IRQ exit
* From a softirq disabled section, where raised softirqs are handled on the next call to
local_bh_enable().[...]
Here is the summary with links:
- [net,v2] net: Handle napi_schedule() calls from non-interrupt
https://git.kernel.org/netdev/net/c/77e45145e303
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html