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

[Hillf Danton <hdanton@sina.com>]

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?