[RFC PATCH 2/8] workqueue: Make create_worker() safe against prematurely wakeups

[RFC PATCH 2/8] workqueue: Make create_worker() safe against prematurely wakeups

[Hillf Danton]

On Thu,  4 Aug 2022 16:41:29 +0800 Lai Jiangshan wrote:
>  
> @@ -1942,6 +1943,7 @@ static struct worker *create_worker(struct worker_pool *pool)
>  		goto fail;
>  
>  	worker->id = id;
> +	worker->pool = pool;
>  
>  	if (pool->cpu >= 0)
>  		snprintf(id_buf, sizeof(id_buf), "%d:%d%s", pool->cpu, id,
> @@ -1949,6 +1951,7 @@ static struct worker *create_worker(struct worker_pool *pool)
>  	else
>  		snprintf(id_buf, sizeof(id_buf), "u%d:%d", pool->id, id);
>  
> +	reinit_completion(&pool->created);
>  	worker->task = kthread_create_on_node(worker_thread, worker, pool->node,
>  					      "kworker/%s", id_buf);
>  	if (IS_ERR(worker->task))
> @@ -1957,15 +1960,9 @@ static struct worker *create_worker(struct worker_pool *pool)
>  	set_user_nice(worker->task, pool->attrs->nice);
>  	kthread_bind_mask(worker->task, pool->attrs->cpumask);
>  
> -	/* successful, attach the worker to the pool */
> -	worker_attach_to_pool(worker, pool);
> -
>  	/* start the newly created worker */
> -	raw_spin_lock_irq(&pool->lock);
> -	worker->pool->nr_workers++;
> -	worker_enter_idle(worker);
>  	wake_up_process(worker->task);
> -	raw_spin_unlock_irq(&pool->lock);
> +	wait_for_completion(&pool->created);
>  
>  	return worker;

	cpu0	cpu1		cpu2
	===	===		===
		complete

	reinit_completion
				wait_for_completion

Any chance for race above?

Re: [RFC PATCH 2/8] workqueue: Make create_worker() safe against prematurely wakeups

[Lai Jiangshan]

 i

On Thu, Aug 4, 2022 at 8:35 PM Hillf Danton <hdanton@sina.com> wrote:
>
> On Thu,  4 Aug 2022 16:41:29 +0800 Lai Jiangshan wrote:
> >
> > @@ -1942,6 +1943,7 @@ static struct worker *create_worker(struct worker_pool *pool)
> >               goto fail;
> >
> >       worker->id = id;
> > +     worker->pool = pool;
> >
> >       if (pool->cpu >= 0)
> >               snprintf(id_buf, sizeof(id_buf), "%d:%d%s", pool->cpu, id,
> > @@ -1949,6 +1951,7 @@ static struct worker *create_worker(struct worker_pool *pool)
> >       else
> >               snprintf(id_buf, sizeof(id_buf), "u%d:%d", pool->id, id);
> >
> > +     reinit_completion(&pool->created);
> >       worker->task = kthread_create_on_node(worker_thread, worker, pool->node,
> >                                             "kworker/%s", id_buf);
> >       if (IS_ERR(worker->task))
> > @@ -1957,15 +1960,9 @@ static struct worker *create_worker(struct worker_pool *pool)
> >       set_user_nice(worker->task, pool->attrs->nice);
> >       kthread_bind_mask(worker->task, pool->attrs->cpumask);
> >
> > -     /* successful, attach the worker to the pool */
> > -     worker_attach_to_pool(worker, pool);
> > -
> >       /* start the newly created worker */
> > -     raw_spin_lock_irq(&pool->lock);
> > -     worker->pool->nr_workers++;
> > -     worker_enter_idle(worker);
> >       wake_up_process(worker->task);
> > -     raw_spin_unlock_irq(&pool->lock);
> > +     wait_for_completion(&pool->created);
> >
> >       return worker;
>
>         cpu0    cpu1            cpu2
>         ===     ===             ===
>                 complete
>
>         reinit_completion
>                                 wait_for_completion

reinit_completion() and wait_for_completion() are both in
create_worker().  create_worker() itself is mutually exclusive
which means no two create_worker()s running at the same time
for the same pool.

No work item can be added before the first initial create_worker()
returns for a new or first-online per-cpu pool, so there would be no
manager for the pool during the first initial create_worker().

The manager is the only worker who can call create_worker() for a pool
except the first initial create_worker().

And there is always only one manager after the first initial
create_worker().

The document style in some of workqueue code is:
"/* locking rule: what it is */"

For example:
struct list_head        worklist;       /* L: list of pending works */
which means it is protected by pool->lock.

And for
struct completion       created;        /* create_worker(): worker created */
it means it is protected by the exclusive create_worker().

>
> Any chance for race above?

Re: [RFC PATCH 2/8] workqueue: Make create_worker() safe against prematurely wakeups

[Hillf Danton]

On Fri, 5 Aug 2022 10:30:10 +0800 Lai Jiangshan wrote:
> On Thu, Aug 4, 2022 at 8:35 PM Hillf Danton <hdanton@sina.com> wrote:
> > >
> > > @@ -1942,6 +1943,7 @@ static struct worker *create_worker(struct worker_pool *pool)
> > >               goto fail;
> > >
> > >       worker->id = id;
> > > +     worker->pool = pool;
> > >
> > >       if (pool->cpu >= 0)
> > >               snprintf(id_buf, sizeof(id_buf), "%d:%d%s", pool->cpu, id,
> > > @@ -1949,6 +1951,7 @@ static struct worker *create_worker(struct worker_pool *pool)
> > >       else
> > >               snprintf(id_buf, sizeof(id_buf), "u%d:%d", pool->id, id);
> > >
> > > +     reinit_completion(&pool->created);
> > >       worker->task = kthread_create_on_node(worker_thread, worker, pool->node,
> > >                                             "kworker/%s", id_buf);
> > >       if (IS_ERR(worker->task))
> > > @@ -1957,15 +1960,9 @@ static struct worker *create_worker(struct worker_pool *pool)
> > >       set_user_nice(worker->task, pool->attrs->nice);
> > >       kthread_bind_mask(worker->task, pool->attrs->cpumask);
> > >
> > > -     /* successful, attach the worker to the pool */
> > > -     worker_attach_to_pool(worker, pool);
> > > -
> > >       /* start the newly created worker */
> > > -     raw_spin_lock_irq(&pool->lock);
> > > -     worker->pool->nr_workers++;
> > > -     worker_enter_idle(worker);
> > >       wake_up_process(worker->task);
> > > -     raw_spin_unlock_irq(&pool->lock);
> > > +     wait_for_completion(&pool->created);
> > >
> > >       return worker;
> >
> >         cpu0    cpu1            cpu2
> >         ===     ===             ===
> >                 complete
> >
> >         reinit_completion
> >                                 wait_for_completion
> 
> reinit_completion() and wait_for_completion() are both in
> create_worker().  create_worker() itself is mutually exclusive
> which means no two create_worker()s running at the same time
> for the same pool.

Then want to know the reasons why complete() in combination with
wait_for_completion() OTOH fails to work for you without reinit.

> 
> No work item can be added before the first initial create_worker()
> returns for a new or first-online per-cpu pool, so there would be no
> manager for the pool during the first initial create_worker().
> 
> The manager is the only worker who can call create_worker() for a pool
> except the first initial create_worker().
> 
> And there is always only one manager after the first initial
> create_worker().
> 
> The document style in some of workqueue code is:
> "/* locking rule: what it is */"
> 
> For example:
> struct list_head        worklist;       /* L: list of pending works */
> which means it is protected by pool->lock.
> 
> And for
> struct completion       created;        /* create_worker(): worker created */
> it means it is protected by the exclusive create_worker().
> 
> >
> > Any chance for race above?