RE: [External] Re: [PATCH 1/1] workqueue: Remove redundant assignment

[Adrian Huang12]

Hi Jiangshan,

> -----Original Message-----
> From: Lai Jiangshan <jiangshanlai@xxxxxxxxx>
> Sent: Tuesday, November 3, 2020 12:35 PM
> To: Adrian Huang <adrianhuang0701@xxxxxxxxx>
> Cc: Tejun Heo <tj@xxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>; Adrian
> Huang12 <ahuang12@xxxxxxxxxx>
> Subject: [External] Re: [PATCH 1/1] workqueue: Remove redundant assignment
> 
> Hello, Adrian
> 
> I believe the pool->node is being used as a node hint before workqueue_init() for
> allocating memory. It is useful when it is correct.

Thanks for the comments. I had the same concern in my mind before
submitting this patch. My understanding is that the worker_pool.node
member is used to provide a node hint when allocating the 'worker'
structure or allocating the woker_pool structure (for unbound workqueue
only).

-- Bound workqueue --
The worker structure is allocated when invoking create_worker() in
the end of workqueue_init(), so this won't lead to the potential
problem by removing the pool->node assignment in workqueue_init_early().

-- Unbound workqueue --
The worker_pool structure is allocated in get_unbound_pool().
The function gets the corresponding node id by checking the
global variable 'wq_numa_possible_cpumask' (of course, it depends
on if the global variable 'wq_numa_enabled' is true).
This is not related to 'per_cpu worker_pool.node' (global variable
'cpu_worker_pools').

Please correct me if I miss something. Thanks.
 
> 
> I think it is better to init it early unless there is a bug about it in this early stage
> reported (on same archs).

OK, please ignore this patch since the patch is not created for a bug report.

> Thanks
> Lai.-
> 
> On Sun, Nov 1, 2020 at 8:21 PM Adrian Huang <adrianhuang0701@xxxxxxxxx>
> wrote:
> >
> > From: Adrian Huang <ahuang12@xxxxxxxxxx>
> >
> > The member 'node' of worker_pool struct (per_cpu worker_pool) is
> > assigned in workqueue_init_early() and workqueue_init().
> > Commit 2186d9f940b6 ("workqueue: move wq_numa_init() to
> > workqueue_init()") fixes an issue by moving wq_numa_init() to
> > workqueue_init() in order to get the valid 'cpu to node' mapping. So,
> > remove the redundant assignment in workqueue_init_early().
> >
> > Signed-off-by: Adrian Huang <ahuang12@xxxxxxxxxx>
> > ---
> >  kernel/workqueue.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c index
> > 437935e7a199..cf8c0df2410e 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -5937,7 +5937,6 @@ void __init workqueue_init_early(void)
> >                         pool->cpu = cpu;
> >                         cpumask_copy(pool->attrs->cpumask, cpumask_of(cpu));
> >                         pool->attrs->nice = std_nice[i++];
> > -                       pool->node = cpu_to_node(cpu);
> >
> >                         /* alloc pool ID */
> >                         mutex_lock(&wq_pool_mutex);
> > --
> > 2.17.1
> >