LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Valentin Schneider <valentin.schneider@arm.com>
To: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: linux-kernel@vger.kernel.org,
	Peter Zijlstra <peterz@infradead.org>, Qian Cai <cai@redhat.com>,
	Vincent Donnefort <vincent.donnefort@arm.com>,
	Lai Jiangshan <laijs@linux.alibaba.com>,
	Tejun Heo <tj@kernel.org>
Subject: Re: [PATCH -tip V2 10/10] workqueue: Fix affinity of kworkers when attaching into pool
Date: Fri, 18 Dec 2020 17:59:45 +0000
Message-ID: <jhjpn37ro4u.mognet@arm.com> (raw)
In-Reply-To: <20201218170919.2950-11-jiangshanlai@gmail.com>


On 18/12/20 17:09, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
>
> When worker_attach_to_pool() is called, we should not put the workers
> to pool->attrs->cpumask when there is not CPU online in it.
>
> We have to use wq_online_cpumask in worker_attach_to_pool() to check
> if pool->attrs->cpumask is valid rather than cpu_online_mask or
> cpu_active_mask due to gaps between stages in cpu hot[un]plug.
>
> So for that late-spawned per-CPU kworker case: the outgoing CPU should have
> already been cleared from wq_online_cpumask, so it gets its affinity reset
> to the possible mask and the subsequent wakeup will ensure it's put on an
> active CPU.
>
> To use wq_online_cpumask in worker_attach_to_pool(), we need to protect
> wq_online_cpumask in wq_pool_attach_mutex and we modify workqueue_online_cpu()
> and workqueue_offline_cpu() to enlarge wq_pool_attach_mutex protected
> region. We also put updating wq_online_cpumask and [re|un]bind_workers()
> in the same wq_pool_attach_mutex protected region to make the update
> for percpu workqueue atomically.
>
> Cc: Qian Cai <cai@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Vincent Donnefort <vincent.donnefort@arm.com>
> Link: https://lore.kernel.org/lkml/20201210163830.21514-3-valentin.schneider@arm.com/
> Acked-by: Valentin Schneider <valentin.schneider@arm.com>

So an etiquette thing: I never actually gave an Acked-by. I did say it
looked good to me, and that probably should've been bundled with a
Reviewed-by, but it wasn't (I figured I'd wait for v2). Forging is bad,
m'kay.

When in doubt (e.g. someone says they're ok with your patch but don't give
any Ack/Reviewed-by), just ask via mail or on IRC.

For now, please make this a:

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>

> Acked-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> ---
>  kernel/workqueue.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 65270729454c..eeb726598f80 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -310,7 +310,7 @@ static bool workqueue_freezing;		/* PL: have wqs started freezing? */
>  /* PL: allowable cpus for unbound wqs and work items */
>  static cpumask_var_t wq_unbound_cpumask;
>
> -/* PL: online cpus (cpu_online_mask with the going-down cpu cleared) */
> +/* PL&A: online cpus (cpu_online_mask with the going-down cpu cleared) */
>  static cpumask_var_t wq_online_cpumask;
>
>  /* CPU where unbound work was last round robin scheduled from this CPU */
> @@ -1848,11 +1848,11 @@ static void worker_attach_to_pool(struct worker *worker,
>  {
>       mutex_lock(&wq_pool_attach_mutex);
>
> -	/*
> -	 * set_cpus_allowed_ptr() will fail if the cpumask doesn't have any
> -	 * online CPUs.  It'll be re-applied when any of the CPUs come up.
> -	 */
> -	set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask);
> +	/* Is there any cpu in pool->attrs->cpumask online? */
> +	if (cpumask_intersects(pool->attrs->cpumask, wq_online_cpumask))
> +		WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask) < 0);
> +	else
> +		WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, cpu_possible_mask) < 0);
>
>       /*
>        * The wq_pool_attach_mutex ensures %POOL_DISASSOCIATED remains
> @@ -5081,13 +5081,12 @@ int workqueue_online_cpu(unsigned int cpu)
>       int pi;
>
>       mutex_lock(&wq_pool_mutex);
> -	cpumask_set_cpu(cpu, wq_online_cpumask);
>
> -	for_each_cpu_worker_pool(pool, cpu) {
> -		mutex_lock(&wq_pool_attach_mutex);
> +	mutex_lock(&wq_pool_attach_mutex);
> +	cpumask_set_cpu(cpu, wq_online_cpumask);
> +	for_each_cpu_worker_pool(pool, cpu)
>               rebind_workers(pool);
> -		mutex_unlock(&wq_pool_attach_mutex);
> -	}
> +	mutex_unlock(&wq_pool_attach_mutex);
>
>       /* update CPU affinity of workers of unbound pools */
>       for_each_pool(pool, pi) {
> @@ -5117,14 +5116,13 @@ int workqueue_offline_cpu(unsigned int cpu)
>       if (WARN_ON(cpu != smp_processor_id()))
>               return -1;
>
> -	for_each_cpu_worker_pool(pool, cpu) {
> -		mutex_lock(&wq_pool_attach_mutex);
> -		unbind_workers(pool);
> -		mutex_unlock(&wq_pool_attach_mutex);
> -	}
> -
>       mutex_lock(&wq_pool_mutex);
> +
> +	mutex_lock(&wq_pool_attach_mutex);
>       cpumask_clear_cpu(cpu, wq_online_cpumask);
> +	for_each_cpu_worker_pool(pool, cpu)
> +		unbind_workers(pool);
> +	mutex_unlock(&wq_pool_attach_mutex);
>
>       /* update CPU affinity of workers of unbound pools */
>       for_each_pool(pool, pi) {

  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-18 17:09 [PATCH -tip V2 00/10] workqueue: break affinity initiatively Lai Jiangshan
2020-12-18 17:09 ` [PATCH -tip V2 01/10] workqueue: restore unbound_workers' cpumask correctly Lai Jiangshan
2020-12-18 17:09 ` [PATCH -tip V2 02/10] workqueue: use cpu_possible_mask instead of cpu_active_mask to break affinity Lai Jiangshan
2020-12-18 17:09 ` [PATCH -tip V2 03/10] workqueue: Manually break affinity on pool detachment Lai Jiangshan
2020-12-18 17:09 ` [PATCH -tip V2 04/10] workqueue: don't set the worker's cpumask when kthread_bind_mask() Lai Jiangshan
2020-12-24  6:33   ` [workqueue] 6094661b16: WARNING:at_kernel/sched/core.c:#__set_cpus_allowed_ptr kernel test robot
2020-12-18 17:09 ` [PATCH -tip V2 05/10] workqueue: introduce wq_online_cpumask Lai Jiangshan
2020-12-18 17:09 ` [PATCH -tip V2 06/10] workqueue: use wq_online_cpumask in restore_unbound_workers_cpumask() Lai Jiangshan
2020-12-18 17:09 ` [PATCH -tip V2 07/10] workqueue: Manually break affinity on hotplug for unbound pool Lai Jiangshan
2020-12-18 17:09 ` [PATCH -tip V2 08/10] workqueue: reorganize workqueue_online_cpu() Lai Jiangshan
2020-12-18 17:09 ` [PATCH -tip V2 09/10] workqueue: reorganize workqueue_offline_cpu() unbind_workers() Lai Jiangshan
2020-12-18 17:09 ` [PATCH -tip V2 10/10] workqueue: Fix affinity of kworkers when attaching into pool Lai Jiangshan
2020-12-18 17:59   ` Valentin Schneider [this message]
2020-12-19  1:11     ` Lai Jiangshan
2020-12-22 21:39 ` [PATCH -tip V2 00/10] workqueue: break affinity initiatively Dexuan-Linux Cui
2020-12-23 11:32   ` Lai Jiangshan
2020-12-23 15:01   ` Lai Jiangshan
2020-12-23 20:27     ` Dexuan Cui
2020-12-23 20:39       ` Dexuan Cui
2020-12-23 19:49 ` Paul E. McKenney
     [not found] ` <20201226103421.6616-1-hdanton@sina.com>
2020-12-26 14:52   ` Paul E. McKenney
2020-12-27 14:08     ` Lai Jiangshan
2020-12-27 16:02       ` Paul E. McKenney

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=jhjpn37ro4u.mognet@arm.com \
    --to=valentin.schneider@arm.com \
    --cc=cai@redhat.com \
    --cc=jiangshanlai@gmail.com \
    --cc=laijs@linux.alibaba.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=tj@kernel.org \
    --cc=vincent.donnefort@arm.com \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git