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;
References: <20220707090501.55483-1-schspa@gmail.com>
 <0320c5f9-cbda-1652-1f97-24d1a22fb298@gmail.com>
User-agent: mu4e 1.6.10; emacs 27.2
From:   Schspa Shi <schspa@gmail.com>
To:     Lai Jiangshan <jiangshanlai@gmail.com>
Cc:     tj@kernel.org, linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH] workqueue: Use active mask for new worker when pool is
 DISASSOCIATED
Date:   Wed, 13 Jul 2022 19:22:06 +0800
In-reply-to: <0320c5f9-cbda-1652-1f97-24d1a22fb298@gmail.com>
Message-ID: <m2y1wxi5qh.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


Lai Jiangshan <jiangshanlai@gmail.com> writes:

> CC Peter.
> Peter has changed the CPU binding code in workqueue.c.
>
> I'm not understanding the problem enough, if kthread_bind_mask() is buggy
> in workqueue.c, it would be buggy in other places too.
>

It's not the bug of to use kthread_bind_mask(), other than we set the
bad pool->attrs->cpumask to this kthread.

>
> On 2022/7/7 17:05, Schspa Shi wrote:
>
>>   -	if (worker->rescue_wq)
>> -		set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask);
>> +	if (worker->rescue_wq) {
>> +		if (pool->flags & POOL_DISASSOCIATED)
>> +			set_cpus_allowed_ptr(worker->task, cpu_active_mask);
>> +		else
>> +			set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask);
>> +	}
>>   
>
> For unbound pools (which also has POOL_DISASSOCIATED), pool->attrs->cpumask
> should be used if pool->attrs->cpumask has active cpu.
>

In this case pool->attrs->cpumask have no active cpu, the cpu for this
pool have offlined already.

The bug will occurs when the cpu have called the workqueue_offline_cpu
form cpu unplug, and create a new worker which will running on a offline
cpu.

>
>> +
>> +	mutex_lock(&wq_pool_attach_mutex);
>> +	if ((pool->flags & POOL_DISASSOCIATED)) {
>> +		/* We can't call get_online_cpus, there will be deadlock
>> +		 * cpu_active_mask will no change, because we have
>> +		 * wq_pool_attach_mutex hold.
>> +		 **/
>> +		kthread_bind_mask(worker->task, cpu_active_mask);
>> +	} else {
>> +		kthread_bind_mask(worker->task, pool->attrs->cpumask);
>> +	}
>> +	mutex_unlock(&wq_pool_attach_mutex);
>
>
> For unbound pools, pool->attrs->cpumask should be used if pool->attrs->cpumask
> has active cpu.
>
> wq_pool_attach_mutex is held here and in worker_attach_to_pool() which smells bad.
>

Yes, this will be changed, I have make a new patch , to move the
thread bind to worker_attach_to_pool, via set_cpus_allowed_ptr.

>
>
> The change is complex.  And if kthread_bind_mask() can't work as expected here,
> the change I prefer would be:
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 4056f2a3f9d5..1ad8aef5fe98 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1862,6 +1862,12 @@ 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);
> +

This will succeed in this case, set_cpus_allowed_ptr will use
cpu_online_mask to verify the cpumask is valid, but in this case,
the cpu_state is between cpu_online and cpu_active. And this
modification doesn't solve the problem I'm having.

>  	/*
>  	 * The wq_pool_attach_mutex ensures %POOL_DISASSOCIATED remains
>  	 * stable across this function.  See the comments above the flag
> @@ -1872,9 +1877,6 @@ static void worker_attach_to_pool(struct worker *worker,
>  	else
>  		kthread_set_per_cpu(worker->task, pool->cpu);
>
> -	if (worker->rescue_wq)
> -		set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask);
> -
>  	list_add_tail(&worker->node, &pool->workers);
>  	worker->pool = pool;
>
> @@ -1952,7 +1954,7 @@ static struct worker *create_worker(struct worker_pool *pool)
>  		goto fail;
>
>  	set_user_nice(worker->task, pool->attrs->nice);
> -	kthread_bind_mask(worker->task, pool->attrs->cpumask);
> +	worker->flags |= PF_NO_SETAFFINITY;
>
>  	/* successful, attach the worker to the pool */
>  	worker_attach_to_pool(worker, pool);
> @@ -4270,7 +4272,7 @@ static int init_rescuer(struct workqueue_struct *wq)
>  	}
>
>  	wq->rescuer = rescuer;
> -	kthread_bind_mask(rescuer->task, cpu_possible_mask);
> +	rescuer->flags |= PF_NO_SETAFFINITY;
>  	wake_up_process(rescuer->task);
>
>  	return 0;
>
>
> It is untested.  It effectively reverts the commit 640f17c82460e
> ("workqueue: Restrict affinity change to rescuer").
> It avoids using kthread_bind_mask().

I will upload a new patchset to remove the extra &wq_pool_attach_mutex
and add a timing diagram to make this question clearer.

-- 
BRs
Schspa Shi