Messages in this thread | | | Date | Mon, 27 Mar 2023 11:07:39 +0800 | From | Yue Hu <> | Subject | Re: [PATCH] workqueue: Drop the NOT_RUNNING check to flags in worker_{set,clr}_flags |
| |
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
| |