diff mbox series

workqueue: Drop the NOT_RUNNING check to flags in worker_{set,clr}_flags

Message ID 20230320092652.16649-1-zbestahu@gmail.com
State New
Headers
Series workqueue: Drop the NOT_RUNNING check to flags in worker_{set,clr}_flags |

Commit Message

Yue Hu March 20, 2023, 9:26 a.m. UTC 
  From: Yue Hu <huyue2@coolpad.com>

We know whether the worker flags are belong to WORKER_NOT_RUNNING or not
when we are setting and clearing them.  So check the flags not running
related is unnecessary for both the cases.

Currently, worker_{set,clr}_flags() are all used for WORKER_NOT_RUNNING
except for clearing WORKER_IDLE.  Let's change to directly clear it
instead.  Also, update the comment a little in worker_clr_flags().

Signed-off-by: Yue Hu <huyue2@coolpad.com>
---
 kernel/workqueue.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Comments

Lai Jiangshan March 25, 2023, 9:20 a.m. UTC | #1 
On Fri, Mar 24, 2023 at 9:58 AM Tejun Heo <tj@kernel.org> wrote:
>
> On Mon, Mar 20, 2023 at 05:26:52PM +0800, Yue Hu wrote:
> > From: Yue Hu <huyue2@coolpad.com>
> >
> > We know whether the worker flags are belong to WORKER_NOT_RUNNING or not
> > when we are setting and clearing them.  So check the flags not running
> > related is unnecessary for both the cases.
> >
> > Currently, worker_{set,clr}_flags() are all used for WORKER_NOT_RUNNING
> > except for clearing WORKER_IDLE.  Let's change to directly clear it
> > instead.  Also, update the comment a little in worker_clr_flags().
>
> I'm not sure this is better. Semantically, the existing code seems clearer
> and less error-prone to me and this isn't gonna make any meaningful perf
> difference. Lai, what do you think?

objdump -DSr kernel/workqueue.o | less

    3275:       00 00
                        3273: R_X86_64_32S      current_task
        WARN_ON_ONCE(worker->task != current);
    3277:       48 39 43 40             cmp    %rax,0x40(%rbx)
    327b:       0f 85 91 00 00 00       jne    3312 <process_one_work+0x3a2>
            !(worker->flags & WORKER_NOT_RUNNING)) {
    3281:       8b 43 68                mov    0x68(%rbx),%eax
        if ((flags & WORKER_NOT_RUNNING) &&
    3284:       a9 c8 01 00 00          test   $0x1c8,%eax
    3289:       75 0b                   jne    3296 <process_one_work+0x326>
        struct worker_pool *pool = worker->pool;
    328b:       48 8b 43 48             mov    0x48(%rbx),%rax
                pool->nr_running--;
    328f:       83 68 20 01             subl   $0x1,0x20(%rax)
    3293:       8b 43 68                mov    0x68(%rbx),%eax
        worker->flags |= flags;
    3296:       83 c8 40                or     $0x40,%eax
    3299:       89 43 68                mov    %eax,0x68(%rbx)

It seems the compiler will do the trick. The clearer existing code
seems better.

Thanks
Lai

>
> Thanks.
>
> --
> tejun
Yue Hu March 27, 2023, 3:07 a.m. UTC | #2 
Hi Tejun, Lai,

On Sat, 25 Mar 2023 17:20:14 +0800
Lai Jiangshan <jiangshanlai@gmail.com> wrote:

> On Fri, Mar 24, 2023 at 9:58 AM Tejun Heo <tj@kernel.org> wrote:
> >
> > On Mon, Mar 20, 2023 at 05:26:52PM +0800, Yue Hu wrote:  
> > > From: Yue Hu <huyue2@coolpad.com>
> > >
> > > We know whether the worker flags are belong to WORKER_NOT_RUNNING or not
> > > when we are setting and clearing them.  So check the flags not running
> > > related is unnecessary for both the cases.
> > >
> > > Currently, worker_{set,clr}_flags() are all used for WORKER_NOT_RUNNING
> > > except for clearing WORKER_IDLE.  Let's change to directly clear it
> > > instead.  Also, update the comment a little in worker_clr_flags().  
> >
> > I'm not sure this is better. Semantically, the existing code seems clearer
> > and less error-prone to me and this isn't gonna make any meaningful perf
> > difference. Lai, what do you think?  
> 
> objdump -DSr kernel/workqueue.o | less
> 
>     3275:       00 00
>                         3273: R_X86_64_32S      current_task
>         WARN_ON_ONCE(worker->task != current);
>     3277:       48 39 43 40             cmp    %rax,0x40(%rbx)
>     327b:       0f 85 91 00 00 00       jne    3312 <process_one_work+0x3a2>
>             !(worker->flags & WORKER_NOT_RUNNING)) {
>     3281:       8b 43 68                mov    0x68(%rbx),%eax
>         if ((flags & WORKER_NOT_RUNNING) &&
>     3284:       a9 c8 01 00 00          test   $0x1c8,%eax
>     3289:       75 0b                   jne    3296 <process_one_work+0x326>
>         struct worker_pool *pool = worker->pool;
>     328b:       48 8b 43 48             mov    0x48(%rbx),%rax
>                 pool->nr_running--;
>     328f:       83 68 20 01             subl   $0x1,0x20(%rax)
>     3293:       8b 43 68                mov    0x68(%rbx),%eax
>         worker->flags |= flags;
>     3296:       83 c8 40                or     $0x40,%eax
>     3299:       89 43 68                mov    %eax,0x68(%rbx)
> 
> It seems the compiler will do the trick. The clearer existing code
> seems better.

Thank you for taking time to review the patch.

The `worker_set_flags` has two behaviors. One is set worker flags unconditionally.
Another is to conditionally adjust `nr_running`. I understand the adjustment should be
only for the flags that we set to NOT_RUNNING. 

And currently the `worker_set_flags()` is not universally used when setting them, such
as in the case of setting WOKER_IDLE. So, I think this helper is not fulfilling its
responsibility, it is actually just only processing `not_running` state. 

If the change is meaningful, maybe it's better to name it as worker_set_not_running_flags()?

Anyway, it is just a minor change.

Thanks.

> 
> Thanks
> Lai
> 
> >
> > Thanks.
> >
> > --
> > tejun
diff mbox series

Patch

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index de4282736999..c56112cb17c3 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -978,10 +978,8 @@  static inline void worker_set_flags(struct worker *worker, unsigned int flags)
 	WARN_ON_ONCE(worker->task != current);
 
 	/* If transitioning into NOT_RUNNING, adjust nr_running. */
-	if ((flags & WORKER_NOT_RUNNING) &&
-	    !(worker->flags & WORKER_NOT_RUNNING)) {
+	if (!(worker->flags & WORKER_NOT_RUNNING))
 		pool->nr_running--;
-	}
 
 	worker->flags |= flags;
 }
@@ -1007,12 +1005,11 @@  static inline void worker_clr_flags(struct worker *worker, unsigned int flags)
 
 	/*
 	 * If transitioning out of NOT_RUNNING, increment nr_running.  Note
-	 * that the nested NOT_RUNNING is not a noop.  NOT_RUNNING is mask
-	 * of multiple flags, not a single flag.
+	 * that NOT_RUNNING is mask of multiple flags, not a single flag.
 	 */
-	if ((flags & WORKER_NOT_RUNNING) && (oflags & WORKER_NOT_RUNNING))
-		if (!(worker->flags & WORKER_NOT_RUNNING))
-			pool->nr_running++;
+	if ((oflags & WORKER_NOT_RUNNING) &&
+	    (!(worker->flags & WORKER_NOT_RUNNING)))
+		pool->nr_running++;
 }
 
 /**
@@ -1835,7 +1832,9 @@  static void worker_leave_idle(struct worker *worker)
 
 	if (WARN_ON_ONCE(!(worker->flags & WORKER_IDLE)))
 		return;
-	worker_clr_flags(worker, WORKER_IDLE);
+	WARN_ON_ONCE(worker->task != current);
+
+	worker->flags &= ~WORKER_IDLE;
 	pool->nr_idle--;
 	list_del_init(&worker->entry);
 }