It was found that any change to the current cpuset hierarchy may reset
the cpumask of the tasks in the affected cpusets to the default cpuset
value even if those tasks have cpus affinity explicitly set by the users
before. That is especially easy to trigger under a cgroup v2 environment
where writing "+cpuset" to the root cgroup's cgroup.subtree_control
file will reset the cpus affinity of all the processes in the system.

That is problematic in a nohz_full environment where the tasks running
in the nohz_full CPUs usually have their cpus affinity explicitly set
and will behave incorrectly if cpus affinity changes.

Fix this problem by looking at user_cpus_ptr which will be set if
cpus affinity have been explicitly set before and use it to restrcit
the given cpumask unless there is no overlap. In that case, it will
fallback to the given one.

With that change in place, it was verified that tasks that have its
cpus affinity explicitly set will not be affected by changes made to
the v2 cgroup.subtree_control files.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/cgroup/cpuset.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index 71a418858a5e..2e3af93bed03 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -704,6 +704,26 @@ static int validate_change(struct cpuset *cur, struct cpuset *trial)
 	return ret;
 }
 
+/*
+ * Preserve user provided cpumask if set unless there is no overlap.
+ */
+static int cpuset_set_cpus_allowed_ptr(struct task_struct *p,
+				       const struct cpumask *mask)
+{
+	if (p->user_cpus_ptr && cpumask_intersects(p->user_cpus_ptr, mask)) {
+		cpumask_var_t new_mask;
+		int ret;
+
+		alloc_cpumask_var(&new_mask, GFP_KERNEL);
+		cpumask_and(new_mask, p->user_cpus_ptr, mask);
+		ret = set_cpus_allowed_ptr(p, new_mask);
+		free_cpumask_var(new_mask);
+		return ret;
+	}
+
+	return set_cpus_allowed_ptr(p, mask);
+}
+
 #ifdef CONFIG_SMP
 /*
  * Helper routine for generate_sched_domains().
@@ -1130,7 +1150,7 @@ static void update_tasks_cpumask(struct cpuset *cs)
 
 	css_task_iter_start(&cs->css, 0, &it);
 	while ((task = css_task_iter_next(&it)))
-		set_cpus_allowed_ptr(task, cs->effective_cpus);
+		cpuset_set_cpus_allowed_ptr(task, cs->effective_cpus);
 	css_task_iter_end(&it);
 }
 
@@ -2303,7 +2323,7 @@ static void cpuset_attach(struct cgroup_taskset *tset)
 		 * can_attach beforehand should guarantee that this doesn't
 		 * fail.  TODO: have a better way to handle failure here
 		 */
-		WARN_ON_ONCE(set_cpus_allowed_ptr(task, cpus_attach));
+		WARN_ON_ONCE(cpuset_set_cpus_allowed_ptr(task, cpus_attach));
 
 		cpuset_change_task_nodemask(task, &cpuset_attach_nodemask_to);
 		cpuset_update_task_spread_flag(cs, task);
-- 
2.31.1

(cc'ing Linus)

Hello,

On Mon, Aug 01, 2022 at 11:41:24AM -0400, Waiman Long wrote:
> It was found that any change to the current cpuset hierarchy may reset
> the cpumask of the tasks in the affected cpusets to the default cpuset
> value even if those tasks have cpus affinity explicitly set by the users
> before. That is especially easy to trigger under a cgroup v2 environment
> where writing "+cpuset" to the root cgroup's cgroup.subtree_control
> file will reset the cpus affinity of all the processes in the system.
> 
> That is problematic in a nohz_full environment where the tasks running
> in the nohz_full CPUs usually have their cpus affinity explicitly set
> and will behave incorrectly if cpus affinity changes.
> 
> Fix this problem by looking at user_cpus_ptr which will be set if
> cpus affinity have been explicitly set before and use it to restrcit
> the given cpumask unless there is no overlap. In that case, it will
> fallback to the given one.
> 
> With that change in place, it was verified that tasks that have its
> cpus affinity explicitly set will not be affected by changes made to
> the v2 cgroup.subtree_control files.

The fact that the kernel clobbers user-specified cpus_allowed as cpu
availability changes always bothered me and it has been causing this sort of
problems w/ cpu hotplug and cpuset. We've been patching this up partially
here and there but I think it would be better if we just make the rules
really simple - ie. allow users to configure whatever cpus_allowed as long
as that's within cpu_possible_mask and override only the effective
cpus_allowed if the mask leaves no runnable CPUs, so that we can restore the
original configured behavior if and when some of the cpus become available
again.

One obvious problem with changing the behavior is that it may affect /
confuse users expecting the current behavior however inconsistent it may be,
but given that we have partially changed how cpus_allowed interacts with
hotplug in the past and the current behavior can be inconsistent and
surprising, I don't think this is a bridge we can't cross. What do others
think?

Thanks.

-- 
tejun

On 8/9/22 15:55, Tejun Heo wrote:
> (cc'ing Linus)
>
> Hello,
>
> On Mon, Aug 01, 2022 at 11:41:24AM -0400, Waiman Long wrote:
>> It was found that any change to the current cpuset hierarchy may reset
>> the cpumask of the tasks in the affected cpusets to the default cpuset
>> value even if those tasks have cpus affinity explicitly set by the users
>> before. That is especially easy to trigger under a cgroup v2 environment
>> where writing "+cpuset" to the root cgroup's cgroup.subtree_control
>> file will reset the cpus affinity of all the processes in the system.
>>
>> That is problematic in a nohz_full environment where the tasks running
>> in the nohz_full CPUs usually have their cpus affinity explicitly set
>> and will behave incorrectly if cpus affinity changes.
>>
>> Fix this problem by looking at user_cpus_ptr which will be set if
>> cpus affinity have been explicitly set before and use it to restrcit
>> the given cpumask unless there is no overlap. In that case, it will
>> fallback to the given one.
>>
>> With that change in place, it was verified that tasks that have its
>> cpus affinity explicitly set will not be affected by changes made to
>> the v2 cgroup.subtree_control files.
> The fact that the kernel clobbers user-specified cpus_allowed as cpu
> availability changes always bothered me and it has been causing this sort of
> problems w/ cpu hotplug and cpuset. We've been patching this up partially
> here and there but I think it would be better if we just make the rules
> really simple - ie. allow users to configure whatever cpus_allowed as long
> as that's within cpu_possible_mask and override only the effective
> cpus_allowed if the mask leaves no runnable CPUs, so that we can restore the
> original configured behavior if and when some of the cpus become available
> again.
>
> One obvious problem with changing the behavior is that it may affect /
> confuse users expecting the current behavior however inconsistent it may be,
> but given that we have partially changed how cpus_allowed interacts with
> hotplug in the past and the current behavior can be inconsistent and
> surprising, I don't think this is a bridge we can't cross. What do others
> think?

My patch will still subject the cpus_allowed list to the constraint 
imposed by the current cpuset. It will keep as much of what the user 
specified though. If we are worrying about backward compatibility, maybe 
we can restrict that change in behavior to cgroup v2 only or we can add 
a sysctl parameter to restore old behavior if the user choose to.

Users are now gradually migrating over to cgroup v2 and they do 
understand that there are some changes in behavior when using cgroup v2.

Cheers,
Longman