Message-ID: <f356b916-1c10-1565-73fb-34027c6c510a@redhat.com>
Date:   Sun, 5 Feb 2023 00:00:25 -0500
From:   Waiman Long <longman@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Tejun Heo <tj@...nel.org>, Zefan Li <lizefan.x@...edance.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Will Deacon <will@...nel.org>, linux-kernel@...r.kernel.org,
        cgroups@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH] cgroup/cpuset: Don't filter offline CPUs in
 cpuset_cpus_allowed() for top cpuset tasks

On 2/4/23 05:01, Peter Zijlstra wrote:
> On Fri, Feb 03, 2023 at 11:40:40AM -0500, Waiman Long wrote:
>> Since commit 8f9ea86fdf99 ("sched: Always preserve the user
>> requested cpumask"), relax_compatible_cpus_allowed_ptr() is calling
>> __sched_setaffinity() unconditionally. This helps to expose a bug in
>> the current cpuset hotplug code where the cpumasks of the tasks in
>> the top cpuset are not updated at all when some CPUs become online or
>> offline. It is likely caused by the fact that some of the tasks in the
>> top cpuset, like percpu kthreads, cannot have their cpu affinity changed.
>>
>> One way to reproduce this as suggested by Peter is:
>>   - boot machine
>>   - offline all CPUs except one
>>   - taskset -p ffffffff $$
>>   - online all CPUs
>>
>> Fix this by allowing cpuset_cpus_allowed() to return a wider mask that
>> includes offline CPUs for those tasks that are in the top cpuset. For
>> tasks not in the top cpuset, the old rule applies and only online CPUs
>> will be returned in the mask since hotplug events will update their
>> cpumasks accordingly.
> So you get the task_cpu_possible_mask() interaction vs cpusets horribly
> wrong here, but given the very sorry state of task_cpu_possible_mask()
> correctness of cpuset as a whole that might just not matter at this
> point.
>
> I do very much hate how you add exceptions on exceptions instead of
> looking to do something right :-(
>
> Fixing that parition case in my patch is 1 extra line and then I think
> it fundamentally does the right thing and can serve as a basis for
> fixing cpuset as a whole.

I am not saying that your patch is incorrect other than handling the 
partition case. However, it is rather complex and is hard to understand 
especially for those that are not that familiar with the cpuset code. 
 From the maintainability point of view, a simpler solution that is 
easier to understand is better.

If we want to get it into the next merge windows, there isn't much time 
left for linux-next testing. So a lower risk solution is better from 
that perspective too.

>> Fixes: 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")
>> Reported-by: Will Deacon <will@...nel.org>
>> Originally-from: Peter Zijlstra (Intel) <peterz@...radead.org>
>> Signed-off-by: Waiman Long <longman@...hat.com>
>> ---
>>   kernel/cgroup/cpuset.c | 27 +++++++++++++++++++++++++--
>>   1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>> index 207bafdb05e8..11554e5845f7 100644
>> --- a/kernel/cgroup/cpuset.c
>> +++ b/kernel/cgroup/cpuset.c
>> @@ -3707,15 +3707,38 @@ void __init cpuset_init_smp(void)
>>    * Description: Returns the cpumask_var_t cpus_allowed of the cpuset
>>    * attached to the specified @tsk.  Guaranteed to return some non-empty
>>    * subset of cpu_online_mask, even if this means going outside the
>> - * tasks cpuset.
>> + * tasks cpuset, except when the task is in the top cpuset.
>>    **/
>>   
>>   void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask)
>>   {
>>   	unsigned long flags;
>> +	struct cpuset *cs;
>>   
>>   	spin_lock_irqsave(&callback_lock, flags);
>> -	guarantee_online_cpus(tsk, pmask);
>> +	rcu_read_lock();
>> +
>> +	cs = task_cs(tsk);
>> +	if (cs != &top_cpuset)
>> +		guarantee_online_cpus(tsk, pmask);
>> +	/*
>> +	 * TODO: Tasks in the top cpuset won't get update to their cpumasks
>> +	 * when a hotplug online/offline event happens. So we include all
>> +	 * offline cpus in the allowed cpu list.
>> +	 */
> I don't like TODO there, I really don't think CPUSET should update root
> tasks, that means yet another fundamental difference between
> CPUSET={y,n}.
OK, I can remove the "TODO". I have no objection to that.
>
>> +	if ((cs == &top_cpuset) || cpumask_empty(pmask)) {
>> +		const struct cpumask *possible_mask = task_cpu_possible_mask(tsk);
>> +
>> +		/*
>> +		 * We first exclude cpus allocated to partitions. If there is no
>> +		 * allowable online cpu left, we fall back to all possible cpus.
>> +		 */
>> +		cpumask_andnot(pmask, possible_mask, top_cpuset.subparts_cpus);
>> +		if (!cpumask_intersects(pmask, cpu_online_mask))
>> +			cpumask_copy(pmask, possible_mask);
>> +	}
>> +
>> +	rcu_read_unlock();
>>   	spin_unlock_irqrestore(&callback_lock, flags);
>>   }
> I really detest this patch, but if you insist it might just do :-/

If we decide that we should always try to keep possible offline cpus in 
a task's cpumask. We could adopt your solution or we can try to keep 
that information in the cpuset structure itself. At this point, I don't 
see any advantage in doing that except for tasks in the top cpuset 
because the hotplug code won't update their cpumasks. Also inferring 
offline cpus that should be in the cpuset is only possible with cgroup 
v2. It does not work for v1. So it is also not a complete solution. To 
be complete, we may need keep this information in the cpuset.

Cheers,
Longman

Powered by blists - more mailing lists