| Message ID | 20230110091409.2962-1-sensor1010@163.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1] net/dev.c : Remove redundant state settings after waking up | expand |
On Tue, Jan 10, 2023 at 10:15 AM 李哲 <sensor1010@163.com> wrote: > > the task status has been set to TASK_RUNNING in shcedule(), > no need to set again here Changelog is rather confusing, this does not match the patch, which removes one set_current_state(TASK_INTERRUPTIBLE); TASK_INTERRUPTIBLE != TASK_RUNNING Patch itself looks okay (but has nothing to do with thread state after schedule()), you should have CC Wei Wang because she authored commit cb038357937e net: fix race between napi kthread mode and busy poll > > Signed-off-by: 李哲 <sensor1010@163.com> > --- > net/core/dev.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index b76fb37b381e..4bd2d4b954c9 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -6580,7 +6580,6 @@ static int napi_thread_wait(struct napi_struct *napi) > schedule(); > /* woken being true indicates this thread owns this napi. */ > woken = true; > - set_current_state(TASK_INTERRUPTIBLE); > } > __set_current_state(TASK_RUNNING); > > -- > 2.17.1 >
On Tue, 10 Jan 2023 10:29:20 +0100 Eric Dumazet wrote: > > the task status has been set to TASK_RUNNING in shcedule(), > > no need to set again here > > Changelog is rather confusing, this does not match the patch, which > removes one set_current_state(TASK_INTERRUPTIBLE); > > TASK_INTERRUPTIBLE != TASK_RUNNING > > Patch itself looks okay (but has nothing to do with thread state after > schedule()), > you should have CC Wei Wang because she > authored commit cb038357937e net: fix race between napi kthread mode > and busy poll AFAIU this is the semi-idiomatic way of handling wait loops. It's not schedule() that may set the task state to TASK_RUNNING, it's whoever wakes the process and makes the "wait condition" true. In this case - test_bit(NAPI_STATE_SCHED, &napi->state) I vote to not futz with this logic.
I was not able to see the entire changelog, but I don't think > - set_current_state(TASK_INTERRUPTIBLE); is redundant. It makes sure that if the previous if statement: if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) is false, this napi thread yields the CPU to other threads waiting to be run by calling schedule(). And the napi thread gets into running state again after the next wake_up_process() is called from ____napi_schedule(). On Tue, Jan 10, 2023 at 4:30 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 10 Jan 2023 10:29:20 +0100 Eric Dumazet wrote: > > > the task status has been set to TASK_RUNNING in shcedule(), > > > no need to set again here > > > > Changelog is rather confusing, this does not match the patch, which > > removes one set_current_state(TASK_INTERRUPTIBLE); > > > > TASK_INTERRUPTIBLE != TASK_RUNNING > > > > Patch itself looks okay (but has nothing to do with thread state after > > schedule()), > > you should have CC Wei Wang because she > > authored commit cb038357937e net: fix race between napi kthread mode > > and busy poll > > AFAIU this is the semi-idiomatic way of handling wait loops. > It's not schedule() that may set the task state to TASK_RUNNING, > it's whoever wakes the process and makes the "wait condition" true. > In this case - test_bit(NAPI_STATE_SCHED, &napi->state) > > I vote to not futz with this logic.
On 2023-01-10 16:42:56 [-0800], Wei Wang wrote: > I was not able to see the entire changelog, but I don't think > > - set_current_state(TASK_INTERRUPTIBLE); > is redundant. > > It makes sure that if the previous if statement: > if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) > is false, this napi thread yields the CPU to other threads waiting to > be run by calling schedule(). It made sense in the beginning but now the suggested patch is a clean up. First the `woken' parameter was added in commit cb038357937ee ("net: fix race between napi kthread mode and busy poll") and then the `napi_disable_pending' check was removed in commit 27f0ad71699de ("net: fix hangup on napi_disable for threaded napi") which renders the code to: | while (!kthread_should_stop()) { | if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) { | WARN_ON(!list_empty(&napi->poll_list)); | __set_current_state(TASK_RUNNING); | return 0; | } | | schedule(); | /* woken being true indicates this thread owns this napi. */ | woken = true; | set_current_state(TASK_INTERRUPTIBLE); | } | __set_current_state(TASK_RUNNING); so when you get out of schedule() woken is set and even if NAPI_STATE_SCHED_THREADED is not set, the while() loop is left due to `woken = true'. So changing state to TASK_INTERRUPTIBLE makes no sense since it will be set back to TASK_RUNNING cycles later. Sebastian
On Wed, 11 Jan 2023 08:32:42 +0100 Sebastian Andrzej Siewior wrote: > It made sense in the beginning but now the suggested patch is a clean > up. First the `woken' parameter was added in commit > cb038357937ee ("net: fix race between napi kthread mode and busy poll") > > and then the `napi_disable_pending' check was removed in commit > 27f0ad71699de ("net: fix hangup on napi_disable for threaded napi") > > which renders the code to: > | while (!kthread_should_stop()) { > | if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) { > | WARN_ON(!list_empty(&napi->poll_list)); > | __set_current_state(TASK_RUNNING); > | return 0; > | } > | > | schedule(); > | /* woken being true indicates this thread owns this napi. */ > | woken = true; > | set_current_state(TASK_INTERRUPTIBLE); > | } > | __set_current_state(TASK_RUNNING); > > so when you get out of schedule() woken is set and even if > NAPI_STATE_SCHED_THREADED is not set, the while() loop is left due to > `woken = true'. So changing state to TASK_INTERRUPTIBLE makes no sense > since it will be set back to TASK_RUNNING cycles later. Ah, fair point, forgot about the woken optimization.
On Wed, Jan 11, 2023 at 10:21 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 11 Jan 2023 08:32:42 +0100 Sebastian Andrzej Siewior wrote: > > It made sense in the beginning but now the suggested patch is a clean > > up. First the `woken' parameter was added in commit > > cb038357937ee ("net: fix race between napi kthread mode and busy poll") > > > > and then the `napi_disable_pending' check was removed in commit > > 27f0ad71699de ("net: fix hangup on napi_disable for threaded napi") > > > > which renders the code to: > > | while (!kthread_should_stop()) { > > | if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken) { > > | WARN_ON(!list_empty(&napi->poll_list)); > > | __set_current_state(TASK_RUNNING); > > | return 0; > > | } > > | > > | schedule(); > > | /* woken being true indicates this thread owns this napi. */ > > | woken = true; > > | set_current_state(TASK_INTERRUPTIBLE); > > | } > > | __set_current_state(TASK_RUNNING); > > > > so when you get out of schedule() woken is set and even if > > NAPI_STATE_SCHED_THREADED is not set, the while() loop is left due to > > `woken = true'. So changing state to TASK_INTERRUPTIBLE makes no sense > > since it will be set back to TASK_RUNNING cycles later. > > Ah, fair point, forgot about the woken optimization. Agree. I think it is OK to remove this, since woken is set, and this function will set TASK_RUNNING and return 0. And the next time, it will enter with TASK_INTERRUPTIBLE and woken = false.
On Wed, 1 Mar 2023 23:32:50 +0800 (CST) lizhe wrote: > HI : > if want to merge this patch into the main line, what should i do ? Stop sending HTML emails please, the list only accepts plain text. Read this: https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html Improve the commit message and the subject line (look at other related commits to find examples). Repost when appropriate (net-next is closed).
diff --git a/net/core/dev.c b/net/core/dev.c index b76fb37b381e..4bd2d4b954c9 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6580,7 +6580,6 @@ static int napi_thread_wait(struct napi_struct *napi) schedule(); /* woken being true indicates this thread owns this napi. */ woken = true; - set_current_state(TASK_INTERRUPTIBLE); } __set_current_state(TASK_RUNNING);
the task status has been set to TASK_RUNNING in shcedule(), no need to set again here Signed-off-by: 李哲 <sensor1010@163.com> --- net/core/dev.c | 1 - 1 file changed, 1 deletion(-)