[PATCH] cgroup/cpuset: Don't filter offline CPUs in cpuset_cpus_allowed() for top cpuset tasks

[PATCH] cgroup/cpuset: Don't filter offline CPUs in cpuset_cpus_allowed() for top cpuset tasks

[Waiman Long]

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.

Fixes: 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")
Reported-by: Will Deacon <will@kernel.org>
Originally-from: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Waiman Long <longman@redhat.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.
+	 */
+	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);
 }
 
-- 
2.31.1

Re: [PATCH] cgroup/cpuset: Don't filter offline CPUs in cpuset_cpus_allowed() for top cpuset tasks

[Tejun Heo]

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.
> 
> Fixes: 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")
> Reported-by: Will Deacon <will@kernel.org>
> Originally-from: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Waiman Long <longman@redhat.com>

So, this is the replacement for the first patch[1] Will posted, right?

>  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.
> +	 */
> +	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);

and the differences are that

* It's only applied to the root cgroup.

* Cpus taken up by partitions are excluded.

Is my understanding correct?

> +		if (!cpumask_intersects(pmask, cpu_online_mask))
> +			cpumask_copy(pmask, possible_mask);
> +	}
> +
> +	rcu_read_unlock();
>  	spin_unlock_irqrestore(&callback_lock, flags);

So, I suppose you're suggesting applying this patch instead of the one Will
Deacon posted[1] and we need Will's second patch[2] on top, right?

[1] http://lkml.kernel.org/r/20230131221719.3176-3-will@kernel.org
[2] http://lkml.kernel.org/r/20230131221719.3176-3-will@kernel.org

Thanks.

-- 
tejun

Re: [PATCH] cgroup/cpuset: Don't filter offline CPUs in cpuset_cpus_allowed() for top cpuset tasks

[Waiman Long]

On 2/3/23 16:00, Tejun Heo 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.
>>
>> Fixes: 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")
>> Reported-by: Will Deacon <will@kernel.org>
>> Originally-from: Peter Zijlstra (Intel) <peterz@infradead.org>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> So, this is the replacement for the first patch[1] Will posted, right?

Yes, if Will and Peter has no objection. I think it is less risky and 
handle the partition case better.

With v1, Will's patch should get similar result as the existing 
guarantee_online_cpus() function since we can infer offline cpus from 
cpus_allowed. With v2, it does include offline cpus correctly, I 
believe, as long as no partition is enabled. However, the hotplug code 
is able to update the cpumasks when a CPU is onlined. So the presence of 
offline CPUs is nice to have, but not essential.

>
>>   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.
>> +	 */
>> +	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);
> and the differences are that
>
> * It's only applied to the root cgroup.
>
> * Cpus taken up by partitions are excluded.
>
> Is my understanding correct?
Yes, that is correct.
>
>> +		if (!cpumask_intersects(pmask, cpu_online_mask))
>> +			cpumask_copy(pmask, possible_mask);
>> +	}
>> +
>> +	rcu_read_unlock();
>>   	spin_unlock_irqrestore(&callback_lock, flags);
> So, I suppose you're suggesting applying this patch instead of the one Will
> Deacon posted[1] and we need Will's second patch[2] on top, right?
Right. Let hear if Will and Peter agree with this plan. I have tested 
this patch and it passed Peter's reproducer test correctly. During 
testing, I uncovered another bug in the cpu affinity code which results 
in a separate scheduler patch to fix it.
>
> [1] http://lkml.kernel.org/r/20230131221719.3176-3-will@kernel.org
> [2] http://lkml.kernel.org/r/20230131221719.3176-3-will@kernel.org
>
> Thanks.
Cheers,
Longman

Re: [PATCH] cgroup/cpuset: Don't filter offline CPUs in cpuset_cpus_allowed() for top cpuset tasks

[Peter Zijlstra]

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.

> Fixes: 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")
> Reported-by: Will Deacon <will@kernel.org>
> Originally-from: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Waiman Long <longman@redhat.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}.

> +	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 :-/

Re: [PATCH] cgroup/cpuset: Don't filter offline CPUs in cpuset_cpus_allowed() for top cpuset tasks

[Will Deacon]

On Fri, Feb 03, 2023 at 11:00:24AM -1000, Tejun Heo 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.
> > 
> > Fixes: 8f9ea86fdf99 ("sched: Always preserve the user requested cpumask")
> > Reported-by: Will Deacon <will@kernel.org>
> > Originally-from: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Signed-off-by: Waiman Long <longman@redhat.com>
> 
> So, this is the replacement for the first patch[1] Will posted, right?
> 
> >  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.
> > +	 */
> > +	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);
> 
> and the differences are that
> 
> * It's only applied to the root cgroup.
> 
> * Cpus taken up by partitions are excluded.
> 
> Is my understanding correct?
> 
> > +		if (!cpumask_intersects(pmask, cpu_online_mask))
> > +			cpumask_copy(pmask, possible_mask);
> > +	}
> > +
> > +	rcu_read_unlock();
> >  	spin_unlock_irqrestore(&callback_lock, flags);
> 
> So, I suppose you're suggesting applying this patch instead of the one Will
> Deacon posted[1] and we need Will's second patch[2] on top, right?
> 
> [1] http://lkml.kernel.org/r/20230131221719.3176-3-will@kernel.org
> [2] http://lkml.kernel.org/r/20230131221719.3176-3-will@kernel.org

FWIW, although I tend to share Peter's sentiments in this thread, I took
this (+ my second patch) for a spin and my tests are giving the same
results when compared with Peter's patch.

Tested-by: Will Deacon <will@kernel.org>

Will

Re: [PATCH] cgroup/cpuset: Don't filter offline CPUs in cpuset_cpus_allowed() for top cpuset tasks

[Waiman Long]

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@kernel.org>
>> Originally-from: Peter Zijlstra (Intel) <peterz@infradead.org>
>> Signed-off-by: Waiman Long <longman@redhat.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

Re: [PATCH] cgroup/cpuset: Don't filter offline CPUs in cpuset_cpus_allowed() for top cpuset tasks

[Will Deacon]

On Sun, Feb 05, 2023 at 12:00:25AM -0500, Waiman Long wrote:
> 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.

This needs to land for 6.2 to fix the regression. The next merge window is
too late. That's why I cooked the reverts [1] as an alternative.

Will

[1] https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=ssa-reverts

Re: [PATCH] cgroup/cpuset: Don't filter offline CPUs in cpuset_cpus_allowed() for top cpuset tasks

[Tejun Heo]

Hello,

On Mon, Feb 06, 2023 at 11:05:41AM +0000, Will Deacon wrote:
> > 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.
> 
> This needs to land for 6.2 to fix the regression. The next merge window is
> too late. That's why I cooked the reverts [1] as an alternative.

Yeah, I think Waiman meant before the coming merge window. We have at least
one more week so let's try the two patches and see how that goes. We can
always revert if it doesn't work out.

Thanks.

-- 
tejun

Re: [PATCH] cgroup/cpuset: Don't filter offline CPUs in cpuset_cpus_allowed() for top cpuset tasks

[Will Deacon]

On Mon, Feb 06, 2023 at 10:17:05AM -1000, Tejun Heo wrote:
> On Mon, Feb 06, 2023 at 11:05:41AM +0000, Will Deacon wrote:
> > > 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.
> > 
> > This needs to land for 6.2 to fix the regression. The next merge window is
> > too late. That's why I cooked the reverts [1] as an alternative.
> 
> Yeah, I think Waiman meant before the coming merge window. We have at least
> one more week so let's try the two patches and see how that goes. We can
> always revert if it doesn't work out.

I see this has landed in Linus' tree now, so thanks for that! I also tested
his branch just to make sure and it all seems fine in my asymmetric
environment.

Cheers,

Will