Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220804084135.92425-1-jiangshanlai@gmail.com>
 <20220804084135.92425-3-jiangshanlai@gmail.com> <20220804123520.1660-1-hdanton@sina.com>
In-Reply-To: <20220804123520.1660-1-hdanton@sina.com>
From:   Lai Jiangshan <jiangshanlai@gmail.com>
Date:   Fri, 5 Aug 2022 10:30:10 +0800
Message-ID: <CAJhGHyBaPPRvEMy7pJRKA6wU27tCrCMpO4MfjObhDP-vjQvjfw@mail.gmail.com>
Subject: Re: [RFC PATCH 2/8] workqueue: Make create_worker() safe against
 prematurely wakeups
To:     Hillf Danton <hdanton@sina.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, linux-mm@kvack.org,
        Petr Mladek <pmladek@suse.com>,
        Peter Zijlstra <peterz@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

 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?