| Message ID | 883923fa22745a9589e8610962b7dc59df09fb1f.1617981844.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 27f0ad71699de41bae013c367b95a6b319cc46a9 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: fix hangup on napi_disable for threaded napi | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 2 blamed authors not CCed: hannes@stressinduktion.org alexanderduyck@fb.com; 8 maintainers not CCed: daniel@iogearbox.net andriin@fb.com cong.wang@bytedance.com ast@kernel.org ap420073@gmail.com bjorn@kernel.org hannes@stressinduktion.org alexanderduyck@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 10 this patch: 10 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 15 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 10 this patch: 10 |
| netdev/header_inline | success | Link |
On Fri, Apr 9, 2021 at 5:25 PM Paolo Abeni <pabeni@redhat.com> wrote: > > napi_disable() is subject to an hangup, when the threaded > mode is enabled and the napi is under heavy traffic. > > If the relevant napi has been scheduled and the napi_disable() > kicks in before the next napi_threaded_wait() completes - so > that the latter quits due to the napi_disable_pending() condition, > the existing code leaves the NAPI_STATE_SCHED bit set and the > napi_disable() loop waiting for such bit will hang. > > This patch addresses the issue by dropping the NAPI_STATE_DISABLE > bit test in napi_thread_wait(). The later napi_threaded_poll() > iteration will take care of clearing the NAPI_STATE_SCHED. > > This also addresses a related problem reported by Jakub: > before this patch a napi_disable()/napi_enable() pair killed > the napi thread, effectively disabling the threaded mode. > On the patched kernel napi_disable() simply stops scheduling > the relevant thread. > > v1 -> v2: > - let the main napi_thread_poll() loop clear the SCHED bit > > Reported-by: Jakub Kicinski <kuba@kernel.org> > Fixes: 29863d41bb6e ("net: implement threaded-able napi poll loop support") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- Reviewed-by: Eric Dumazet <edumazet@google.com> Thanks !
On 09.04.2021 17:24, Paolo Abeni wrote: > napi_disable() is subject to an hangup, when the threaded > mode is enabled and the napi is under heavy traffic. > > If the relevant napi has been scheduled and the napi_disable() > kicks in before the next napi_threaded_wait() completes - so > that the latter quits due to the napi_disable_pending() condition, > the existing code leaves the NAPI_STATE_SCHED bit set and the > napi_disable() loop waiting for such bit will hang. > > This patch addresses the issue by dropping the NAPI_STATE_DISABLE > bit test in napi_thread_wait(). The later napi_threaded_poll() > iteration will take care of clearing the NAPI_STATE_SCHED. > > This also addresses a related problem reported by Jakub: > before this patch a napi_disable()/napi_enable() pair killed > the napi thread, effectively disabling the threaded mode. > On the patched kernel napi_disable() simply stops scheduling > the relevant thread. > > v1 -> v2: > - let the main napi_thread_poll() loop clear the SCHED bit > > Reported-by: Jakub Kicinski <kuba@kernel.org> > Fixes: 29863d41bb6e ("net: implement threaded-able napi poll loop support") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net/core/dev.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 0f72ff5d34ba..af8c1ea040b9 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -6992,7 +6992,7 @@ static int napi_thread_wait(struct napi_struct *napi) > > set_current_state(TASK_INTERRUPTIBLE); > > - while (!kthread_should_stop() && !napi_disable_pending(napi)) { > + while (!kthread_should_stop()) { > /* Testing SCHED_THREADED bit here to make sure the current > * kthread owns this napi and could poll on this napi. > * Testing SCHED bit is not enough because SCHED bit might be > @@ -7010,6 +7010,7 @@ static int napi_thread_wait(struct napi_struct *napi) > set_current_state(TASK_INTERRUPTIBLE); > } > __set_current_state(TASK_RUNNING); > + > return -1; > } > > Unrelated to the actual issue: I wonder why -1 is returned, that's unusual in kernel. The return value is used as bool, so why not return a bool?
On Fri, Apr 9, 2021 at 5:35 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > > Unrelated to the actual issue: > I wonder why -1 is returned, that's unusual in kernel. The return value > is used as bool, so why not return a bool? This is a leftover of a prior implementation that returned 3 values. No bug here really, this is a matter of taste I suppose.
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Fri, 9 Apr 2021 17:24:17 +0200 you wrote: > napi_disable() is subject to an hangup, when the threaded > mode is enabled and the napi is under heavy traffic. > > If the relevant napi has been scheduled and the napi_disable() > kicks in before the next napi_threaded_wait() completes - so > that the latter quits due to the napi_disable_pending() condition, > the existing code leaves the NAPI_STATE_SCHED bit set and the > napi_disable() loop waiting for such bit will hang. > > [...] Here is the summary with links: - [net,v2] net: fix hangup on napi_disable for threaded napi https://git.kernel.org/netdev/net/c/27f0ad71699d You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/net/core/dev.c b/net/core/dev.c index 0f72ff5d34ba..af8c1ea040b9 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6992,7 +6992,7 @@ static int napi_thread_wait(struct napi_struct *napi) set_current_state(TASK_INTERRUPTIBLE); - while (!kthread_should_stop() && !napi_disable_pending(napi)) { + while (!kthread_should_stop()) { /* Testing SCHED_THREADED bit here to make sure the current * kthread owns this napi and could poll on this napi. * Testing SCHED bit is not enough because SCHED bit might be @@ -7010,6 +7010,7 @@ static int napi_thread_wait(struct napi_struct *napi) set_current_state(TASK_INTERRUPTIBLE); } __set_current_state(TASK_RUNNING); + return -1; }
napi_disable() is subject to an hangup, when the threaded mode is enabled and the napi is under heavy traffic. If the relevant napi has been scheduled and the napi_disable() kicks in before the next napi_threaded_wait() completes - so that the latter quits due to the napi_disable_pending() condition, the existing code leaves the NAPI_STATE_SCHED bit set and the napi_disable() loop waiting for such bit will hang. This patch addresses the issue by dropping the NAPI_STATE_DISABLE bit test in napi_thread_wait(). The later napi_threaded_poll() iteration will take care of clearing the NAPI_STATE_SCHED. This also addresses a related problem reported by Jakub: before this patch a napi_disable()/napi_enable() pair killed the napi thread, effectively disabling the threaded mode. On the patched kernel napi_disable() simply stops scheduling the relevant thread. v1 -> v2: - let the main napi_thread_poll() loop clear the SCHED bit Reported-by: Jakub Kicinski <kuba@kernel.org> Fixes: 29863d41bb6e ("net: implement threaded-able napi poll loop support") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/core/dev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)