LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Lai Jiangshan <jiangshanlai@gmail.com>
To: Tejun Heo <tj@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux API <linux-api@vger.kernel.org>,
	kernel-team <kernel-team@fb.com>, Craig Small <csmall@enc.com.au>
Subject: Re: [PATCH 3/6] workqueue: Make worker_attach/detach_pool() update worker->pool
Date: Sat, 19 May 2018 14:44:39 +0800	[thread overview]
Message-ID: <CAJhGHyC8pZvy3cmT-n6aFy0KQVJ0je6LwUKZf2Z1Rdvq--vFVw@mail.gmail.com> (raw)
In-Reply-To: <20180517043448.3152269-4-tj@kernel.org>

On Thu, May 17, 2018 at 12:34 PM, Tejun Heo <tj@kernel.org> wrote:
> For historical reasons, the worker attach/detach functions don't
> currently manage worker->pool and the callers are manually and
> inconsistently updating it.
>
> This patch moves worker->pool updates into the worker attach/detach
> functions.  This makes worker->pool consistent and clearly defines how
> worker->pool updates are synchronized.
>
> This will help later workqueue visibility improvements by allowing
> safe access to workqueue information from worker->task.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  kernel/workqueue.c          | 16 ++++++++--------
>  kernel/workqueue_internal.h |  2 +-
>  2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 91fe0a6..2fde50f 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1741,6 +1741,7 @@ static void worker_attach_to_pool(struct worker *worker,
>                 worker->flags |= WORKER_UNBOUND;
>
>         list_add_tail(&worker->node, &pool->workers);
> +       worker->pool = pool;
>
>         mutex_unlock(&wq_pool_attach_mutex);
>  }
> @@ -1748,19 +1749,21 @@ static void worker_attach_to_pool(struct worker *worker,
>  /**
>   * worker_detach_from_pool() - detach a worker from its pool
>   * @worker: worker which is attached to its pool
> - * @pool: the pool @worker is attached to
>   *
>   * Undo the attaching which had been done in worker_attach_to_pool().  The
>   * caller worker shouldn't access to the pool after detached except it has
>   * other reference to the pool.
>   */
> -static void worker_detach_from_pool(struct worker *worker,
> -                                   struct worker_pool *pool)
> +static void worker_detach_from_pool(struct worker *worker)
>  {
> +       struct worker_pool *pool = worker->pool;
>         struct completion *detach_completion = NULL;
>
>         mutex_lock(&wq_pool_attach_mutex);
> +
>         list_del(&worker->node);
> +       worker->pool = NULL;
> +
>         if (list_empty(&pool->workers))
>                 detach_completion = pool->detach_completion;
>         mutex_unlock(&wq_pool_attach_mutex);
> @@ -1799,7 +1802,6 @@ static struct worker *create_worker(struct worker_pool *pool)
>         if (!worker)
>                 goto fail;
>
> -       worker->pool = pool;
>         worker->id = id;
>
>         if (pool->cpu >= 0)
> @@ -2236,7 +2238,7 @@ static int worker_thread(void *__worker)
>
>                 set_task_comm(worker->task, "kworker/dying");
>                 ida_simple_remove(&pool->worker_ida, worker->id);
> -               worker_detach_from_pool(worker, pool);
> +               worker_detach_from_pool(worker);
>                 kfree(worker);
>                 return 0;
>         }
> @@ -2367,7 +2369,6 @@ static int rescuer_thread(void *__rescuer)
>                 worker_attach_to_pool(rescuer, pool);
>
>                 spin_lock_irq(&pool->lock);
> -               rescuer->pool = pool;
>
>                 /*
>                  * Slurp in all works issued via this workqueue and
> @@ -2417,10 +2418,9 @@ static int rescuer_thread(void *__rescuer)
>                 if (need_more_worker(pool))
>                         wake_up_worker(pool);
>
> -               rescuer->pool = NULL;
>                 spin_unlock_irq(&pool->lock);
>
> -               worker_detach_from_pool(rescuer, pool);
> +               worker_detach_from_pool(rescuer);
>
>                 spin_lock_irq(&wq_mayday_lock);
>         }
> diff --git a/kernel/workqueue_internal.h b/kernel/workqueue_internal.h
> index d390d1b..4a182e0 100644
> --- a/kernel/workqueue_internal.h
> +++ b/kernel/workqueue_internal.h
> @@ -37,7 +37,7 @@ struct worker {
>         /* 64 bytes boundary on 64bit, 32 on 32bit */
>
>         struct task_struct      *task;          /* I: worker task */
> -       struct worker_pool      *pool;          /* I: the associated pool */
> +       struct worker_pool      *pool;          /* A: the associated pool */
>                                                 /* L: for rescuers */

I guess ` /* L: for rescuers */ ` needed to be removed.

And there are many usages of worker->pool without attached_mutex
heled. The only locking annotation "A:" is not enough. Additional
comment is needed too.

1) called from the worker(including rescuer) task: it is safe because only
    the worker task itself can call the attach()/detach().

2) called from the destroy_worker() (only normal worker): it is safe
    because it is immutable before the worker detaches itself.

>         struct list_head        node;           /* A: anchored at pool->workers */
>                                                 /* A: runs through worker->node */
> --
> 2.9.5
>

  reply	other threads:[~2018-05-19  6:44 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-17  4:34 [PATCHSET] workqueue: Show the latest workqueue name in /proc/PID/{comm,stat,status} Tejun Heo
2018-05-17  4:34 ` [PATCH 1/6] proc: Don't allow empty /proc/PID/cmdline for user tasks Tejun Heo
2018-05-17  4:34 ` [PATCH 2/6] workqueue: Replace pool->attach_mutex with global wq_pool_attach_mutex Tejun Heo
2018-05-17  4:34 ` [PATCH 3/6] workqueue: Make worker_attach/detach_pool() update worker->pool Tejun Heo
2018-05-19  6:44   ` Lai Jiangshan [this message]
2018-05-17  4:34 ` [PATCH 4/6] workqueue: Set worker->desc to workqueue name by default Tejun Heo
2018-05-17  4:34 ` [PATCH 5/6] proc: Consolidate task->comm formatting into proc_task_name() Tejun Heo
2018-05-17  4:34 ` [PATCH 6/6] workqueue: Show the latest workqueue name in /proc/PID/{comm,stat,status} Tejun Heo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJhGHyC8pZvy3cmT-n6aFy0KQVJ0je6LwUKZf2Z1Rdvq--vFVw@mail.gmail.com \
    --to=jiangshanlai@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=csmall@enc.com.au \
    --cc=kernel-team@fb.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tj@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [PATCH 3/6] workqueue: Make worker_attach/detach_pool() update worker->pool' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).