[RFC PATCH 0/2] cpuset: Skip possible unwanted CPU-mask updates.

[RFC PATCH 0/2] cpuset: Skip possible unwanted CPU-mask updates.

[Sebastian Andrzej Siewior]

Hi,

while playing with cgroups and tasks which alter their CPU-mask I
noticed a behaviour which is unwanted:
- Upon enabling the cpuset controller, all currently set CPU-mask are
  changed. Here one could argue how bad it is assuming that the
  controller gets only enabled once during boot.

- While having a task limited to only one CPU (or more but a subset of
  the cpuset's mask) then adding an additional CPU (or removing an
  unrelated CPU) from that cpuset results in changing the CPU-mask of
  that task and adding additional CPUs.

The test used to verify the behaviour:

# Limit to CPU 0-1
$ taskset -pc 0-1 $$
# Enable the cpuset controller
$  echo "+cpu" >> /sys/fs/cgroup/cgroup.subtree_control ; echo "+cpuset" >> /sys/fs/cgroup/cgroup.subtree_control

# Query the CPU-mask. Expect to see 0-1 since it is a subset of
# all-online-CPUs
$ taskset -pc $$

# Update the mask to CPUs 1-2
$ taskset -pc 1-2 $$

# Change the CPU-mask of the cgroup to CPUs 1-3.
$ echo 1-3 > /sys/fs/cgroup/user.slice/cpuset.cpus
# Expect to see 1-2 because it is a subset of 1-3
$ taskset -pc $$

# Change the CPU-mask of the cgroup to CPUs 2-3.
$ echo 2-3 > /sys/fs/cgroup/user.slice/cpuset.cpus
# Expect to see 2-3 because CPU 1 is not part of 2-3
$ taskset -pc $$

# Change the CPU-mask of the cgroup to CPUs 2-4.
$ echo 2-4 > /sys/fs/cgroup/user.slice/cpuset.cpus
# Expect to see 2-4 because task's old CPU-mask matches the old mask of
# the cpuset.
$ taskset -pc $$

Posting this as RFC in case I'm missing something obvious or breaking an
unknown (to me) requirement that this series would break.

Sebastian

[RFC PATCH 1/2] cpuset: Don't change the cpumask on attach if it is a subset.

[Sebastian Andrzej Siewior]

If a task it attached to a cgroup then its CPU-mask will be set to the
mask of the cgroup. Upon enabling the cpuset controller it will be set
to all online CPUs. All running user tasks, that changed their CPU-mask,
will have their mask altered without knowing it. Tasks with unchanged
CPU-mask are set to all online CPUs and this change is a nop.

If the task is already using a subset of the allowed (new) CPUs, skip
changing the mask. This will preserve the CPU-mask as long as it
contains only allowed CPUs from the new CPU-mask.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 kernel/cgroup/cpuset.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index b474289c15b82..8d5126684f9e6 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -2527,7 +2527,8 @@ 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));
+		if (!cpumask_subset(&task->cpus_mask, cpus_attach))
+			WARN_ON_ONCE(set_cpus_allowed_ptr(task, cpus_attach));
 
 		cpuset_change_task_nodemask(task, &cpuset_attach_nodemask_to);
 		cpuset_update_task_spread_flag(cs, task);
-- 
2.37.2

[RFC PATCH 2/2] cpuset: Don't change the cpumask if the task changed it.

[Sebastian Andrzej Siewior]

Upon changing the allowed CPUs of cgroup, all task within this cgroup
will get their CPU-mask updated to the new mask. Tasks that changed
their CPU-mask will get their mask changed without knowing. If task
restricted itself to subset of CPUs, there is no reason to change its
mask after a new CPU has been added or a CPU has been removed which was
not used by the task.

Skip CPU-mask updates if task's CPU-mask differs from the previous
CPU-mask of the cgroup (it was changed) and if this CPU-mask is a subset
of the requested new CPU mask.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 kernel/cgroup/cpuset.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index 8d5126684f9e6..6d0d07148cfaa 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -1206,7 +1206,7 @@ void rebuild_sched_domains(void)
  * effective cpuset's.  As this function is called with cpuset_rwsem held,
  * cpuset membership stays stable.
  */
-static void update_tasks_cpumask(struct cpuset *cs)
+static void update_tasks_cpumask(struct cpuset *cs, cpumask_var_t prev_mask)
 {
 	struct css_task_iter it;
 	struct task_struct *task;
@@ -1220,7 +1220,13 @@ static void update_tasks_cpumask(struct cpuset *cs)
 		if (top_cs && (task->flags & PF_KTHREAD) &&
 		    kthread_is_per_cpu(task))
 			continue;
-		set_cpus_allowed_ptr(task, cs->effective_cpus);
+		/*
+		 * Update if task's CPU-mask equals previous CPUset or if task's
+		 * CPU-mask has CPUs which are not part of the new CPUset
+		 */
+		if (!prev_mask || (cpumask_equal(&task->cpus_mask, prev_mask) ||
+				   !cpumask_subset(&task->cpus_mask, cs->effective_cpus)))
+			set_cpus_allowed_ptr(task, cs->effective_cpus);
 	}
 	css_task_iter_end(&it);
 }
@@ -1505,7 +1511,7 @@ static int update_parent_subparts_cpumask(struct cpuset *cs, int cmd,
 	spin_unlock_irq(&callback_lock);
 
 	if (adding || deleting)
-		update_tasks_cpumask(parent);
+		update_tasks_cpumask(parent, NULL);
 
 	/*
 	 * Set or clear CS_SCHED_LOAD_BALANCE when partcmd_update, if necessary.
@@ -1639,6 +1645,7 @@ static void update_cpumasks_hier(struct cpuset *cs, struct tmpmasks *tmp,
 			cpumask_clear(cp->subparts_cpus);
 		}
 
+		cpumask_copy(tmp->addmask, cp->effective_cpus);
 		cpumask_copy(cp->effective_cpus, tmp->new_cpus);
 		if (cp->nr_subparts_cpus) {
 			/*
@@ -1657,7 +1664,7 @@ static void update_cpumasks_hier(struct cpuset *cs, struct tmpmasks *tmp,
 		WARN_ON(!is_in_v2_mode() &&
 			!cpumask_equal(cp->cpus_allowed, cp->effective_cpus));
 
-		update_tasks_cpumask(cp);
+		update_tasks_cpumask(cp, tmp->addmask);
 
 		/*
 		 * On legacy hierarchy, if the effective cpumask of any non-
@@ -2305,7 +2312,7 @@ static int update_prstate(struct cpuset *cs, int new_prs)
 		}
 	}
 
-	update_tasks_cpumask(parent);
+	update_tasks_cpumask(parent, NULL);
 
 	if (parent->child_ecpus_count)
 		update_sibling_cpumasks(parent, cs, &tmpmask);
@@ -3318,7 +3325,7 @@ hotplug_update_tasks_legacy(struct cpuset *cs,
 	 * as the tasks will be migrated to an ancestor.
 	 */
 	if (cpus_updated && !cpumask_empty(cs->cpus_allowed))
-		update_tasks_cpumask(cs);
+		update_tasks_cpumask(cs, NULL);
 	if (mems_updated && !nodes_empty(cs->mems_allowed))
 		update_tasks_nodemask(cs);
 
@@ -3355,7 +3362,7 @@ hotplug_update_tasks(struct cpuset *cs,
 	spin_unlock_irq(&callback_lock);
 
 	if (cpus_updated)
-		update_tasks_cpumask(cs);
+		update_tasks_cpumask(cs, NULL);
 	if (mems_updated)
 		update_tasks_nodemask(cs);
 }
-- 
2.37.2

Re: [RFC PATCH 0/2] cpuset: Skip possible unwanted CPU-mask updates.

[Waiman Long]

On 11/2/22 06:55, Sebastian Andrzej Siewior wrote:
> Hi,
>
> while playing with cgroups and tasks which alter their CPU-mask I
> noticed a behaviour which is unwanted:
> - Upon enabling the cpuset controller, all currently set CPU-mask are
>    changed. Here one could argue how bad it is assuming that the
>    controller gets only enabled once during boot.
>
> - While having a task limited to only one CPU (or more but a subset of
>    the cpuset's mask) then adding an additional CPU (or removing an
>    unrelated CPU) from that cpuset results in changing the CPU-mask of
>    that task and adding additional CPUs.
>
> The test used to verify the behaviour:
>
> # Limit to CPU 0-1
> $ taskset -pc 0-1 $$
> # Enable the cpuset controller
> $  echo "+cpu" >> /sys/fs/cgroup/cgroup.subtree_control ; echo "+cpuset" >> /sys/fs/cgroup/cgroup.subtree_control
>
> # Query the CPU-mask. Expect to see 0-1 since it is a subset of
> # all-online-CPUs
> $ taskset -pc $$
>
> # Update the mask to CPUs 1-2
> $ taskset -pc 1-2 $$
>
> # Change the CPU-mask of the cgroup to CPUs 1-3.
> $ echo 1-3 > /sys/fs/cgroup/user.slice/cpuset.cpus
> # Expect to see 1-2 because it is a subset of 1-3
> $ taskset -pc $$
>
> # Change the CPU-mask of the cgroup to CPUs 2-3.
> $ echo 2-3 > /sys/fs/cgroup/user.slice/cpuset.cpus
> # Expect to see 2-3 because CPU 1 is not part of 2-3
> $ taskset -pc $$
>
> # Change the CPU-mask of the cgroup to CPUs 2-4.
> $ echo 2-4 > /sys/fs/cgroup/user.slice/cpuset.cpus
> # Expect to see 2-4 because task's old CPU-mask matches the old mask of
> # the cpuset.
> $ taskset -pc $$
>
> Posting this as RFC in case I'm missing something obvious or breaking an
> unknown (to me) requirement that this series would break.

Yes, that is a known issue which is especially problematic when cgroup 
v2 is used. That is why I have recently posted a set of patches to 
enable persistent user requested affinity and they have been merged into 
tip:

851a723e45d1 sched: Always clear user_cpus_ptr in do_set_cpus_allowed()
da019032819a sched: Enforce user requested affinity
8f9ea86fdf99 sched: Always preserve the user requested cpumask
713a2e21a513 sched: Introduce affinity_context
5584e8ac2c68 sched: Add __releases annotations to affine_move_task()

They should be able to address the problem that you list here. Please 
let me know if there are still problem remaining.

Thanks,
Longman

Re: [RFC PATCH 0/2] cpuset: Skip possible unwanted CPU-mask updates.

[Sebastian Andrzej Siewior]

On 2022-11-02 09:20:48 [-0400], Waiman Long wrote:
Hi,

> Yes, that is a known issue which is especially problematic when cgroup v2 is
> used. That is why I have recently posted a set of patches to enable
> persistent user requested affinity and they have been merged into tip:
> 
> 851a723e45d1 sched: Always clear user_cpus_ptr in do_set_cpus_allowed()
> da019032819a sched: Enforce user requested affinity
> 8f9ea86fdf99 sched: Always preserve the user requested cpumask
> 713a2e21a513 sched: Introduce affinity_context
> 5584e8ac2c68 sched: Add __releases annotations to affine_move_task()

> They should be able to address the problem that you list here. Please let me
> know if there are still problem remaining.

Thank you. This solves the most pressing issue. The CPU-mask is still
reset upon activation of the cpuset controller.
This is due to the set_cpus_allowed_ptr() in cpuset_attach().

Is is possible to push these patches stable?

> Thanks,
> Longman

Sebastian

Re: [RFC PATCH 0/2] cpuset: Skip possible unwanted CPU-mask updates.

[Waiman Long]

On 11/11/22 10:34, Sebastian Andrzej Siewior wrote:
> On 2022-11-02 09:20:48 [-0400], Waiman Long wrote:
> Hi,
>
>> Yes, that is a known issue which is especially problematic when cgroup v2 is
>> used. That is why I have recently posted a set of patches to enable
>> persistent user requested affinity and they have been merged into tip:
>>
>> 851a723e45d1 sched: Always clear user_cpus_ptr in do_set_cpus_allowed()
>> da019032819a sched: Enforce user requested affinity
>> 8f9ea86fdf99 sched: Always preserve the user requested cpumask
>> 713a2e21a513 sched: Introduce affinity_context
>> 5584e8ac2c68 sched: Add __releases annotations to affine_move_task()
>> They should be able to address the problem that you list here. Please let me
>> know if there are still problem remaining.
> Thank you. This solves the most pressing issue. The CPU-mask is still
> reset upon activation of the cpuset controller.
> This is due to the set_cpus_allowed_ptr() in cpuset_attach().
>
> Is is possible to push these patches stable?

Actually, I prefer to not call set_cpus_allowed_ptr() if the cpumask of 
the old and new cpusets are the same. That will save some cpu cycles. 
Similarly for node_mask. If there is any changes in the cpumask, we have 
to call set_cpus_allowed_ptr(). I will work out a patch to that effect 
when I have spare cycle.

Thanks,
Longman

Re: [RFC PATCH 0/2] cpuset: Skip possible unwanted CPU-mask updates.

[Sebastian Andrzej Siewior]

On 2022-11-11 11:37:03 [-0500], Waiman Long wrote:
> 
> On 11/11/22 10:34, Sebastian Andrzej Siewior wrote:
> > On 2022-11-02 09:20:48 [-0400], Waiman Long wrote:
> > Hi,
> > 
> > > Yes, that is a known issue which is especially problematic when cgroup v2 is
> > > used. That is why I have recently posted a set of patches to enable
> > > persistent user requested affinity and they have been merged into tip:
> > > 
> > > 851a723e45d1 sched: Always clear user_cpus_ptr in do_set_cpus_allowed()
> > > da019032819a sched: Enforce user requested affinity
> > > 8f9ea86fdf99 sched: Always preserve the user requested cpumask
> > > 713a2e21a513 sched: Introduce affinity_context
> > > 5584e8ac2c68 sched: Add __releases annotations to affine_move_task()
> > > They should be able to address the problem that you list here. Please let me
> > > know if there are still problem remaining.
> > Thank you. This solves the most pressing issue. The CPU-mask is still
> > reset upon activation of the cpuset controller.
> > This is due to the set_cpus_allowed_ptr() in cpuset_attach().
> > 
> > Is is possible to push these patches stable?
> 
> Actually, I prefer to not call set_cpus_allowed_ptr() if the cpumask of the
> old and new cpusets are the same. That will save some cpu cycles. Similarly
> for node_mask. If there is any changes in the cpumask, we have to call
> set_cpus_allowed_ptr(). I will work out a patch to that effect when I have
> spare cycle.

But couldn't we skip that set_cpus_allowed_ptr() if the current mask is
a subset of the new mask and keep the behavior that the mask isn't
changed if the cgroup's mask changes but is still a subset?
That cpuset_attach() callback is only invoked while the task is
initially added to the cgroup which happens during enabling of the
controller. Or do I miss another common usecase?

> Thanks,
> Longman

Sebastian

Re: [RFC PATCH 0/2] cpuset: Skip possible unwanted CPU-mask updates.

[Waiman Long]

On 11/11/22 11:49, Sebastian Andrzej Siewior wrote:
> On 2022-11-11 11:37:03 [-0500], Waiman Long wrote:
>> On 11/11/22 10:34, Sebastian Andrzej Siewior wrote:
>>> On 2022-11-02 09:20:48 [-0400], Waiman Long wrote:
>>> Hi,
>>>
>>>> Yes, that is a known issue which is especially problematic when cgroup v2 is
>>>> used. That is why I have recently posted a set of patches to enable
>>>> persistent user requested affinity and they have been merged into tip:
>>>>
>>>> 851a723e45d1 sched: Always clear user_cpus_ptr in do_set_cpus_allowed()
>>>> da019032819a sched: Enforce user requested affinity
>>>> 8f9ea86fdf99 sched: Always preserve the user requested cpumask
>>>> 713a2e21a513 sched: Introduce affinity_context
>>>> 5584e8ac2c68 sched: Add __releases annotations to affine_move_task()
>>>> They should be able to address the problem that you list here. Please let me
>>>> know if there are still problem remaining.
>>> Thank you. This solves the most pressing issue. The CPU-mask is still
>>> reset upon activation of the cpuset controller.
>>> This is due to the set_cpus_allowed_ptr() in cpuset_attach().
>>>
>>> Is is possible to push these patches stable?
>> Actually, I prefer to not call set_cpus_allowed_ptr() if the cpumask of the
>> old and new cpusets are the same. That will save some cpu cycles. Similarly
>> for node_mask. If there is any changes in the cpumask, we have to call
>> set_cpus_allowed_ptr(). I will work out a patch to that effect when I have
>> spare cycle.
> But couldn't we skip that set_cpus_allowed_ptr() if the current mask is
> a subset of the new mask and keep the behavior that the mask isn't
> changed if the cgroup's mask changes but is still a subset?
> That cpuset_attach() callback is only invoked while the task is
> initially added to the cgroup which happens during enabling of the
> controller. Or do I miss another common usecase?

The only way a userspace task can have a cpumask different from that of 
its cpuset is by calling sched_setaffinity() which will be covered by 
the persistent user requested mask change. So your patch 2 isn't really 
needed.

Your patch 1 is relatively simple and I don't have a big objection to 
that. I will let others chime in on what their thoughts are.

Cheers,
Longman

Re: [RFC PATCH 0/2] cpuset: Skip possible unwanted CPU-mask updates.

[Waiman Long]

On 11/11/22 12:34, Waiman Long wrote:
>
> On 11/11/22 11:49, Sebastian Andrzej Siewior wrote:
>> On 2022-11-11 11:37:03 [-0500], Waiman Long wrote:
>>> On 11/11/22 10:34, Sebastian Andrzej Siewior wrote:
>>>> On 2022-11-02 09:20:48 [-0400], Waiman Long wrote:
>>>> Hi,
>>>>
>>>>> Yes, that is a known issue which is especially problematic when 
>>>>> cgroup v2 is
>>>>> used. That is why I have recently posted a set of patches to enable
>>>>> persistent user requested affinity and they have been merged into 
>>>>> tip:
>>>>>
>>>>> 851a723e45d1 sched: Always clear user_cpus_ptr in 
>>>>> do_set_cpus_allowed()
>>>>> da019032819a sched: Enforce user requested affinity
>>>>> 8f9ea86fdf99 sched: Always preserve the user requested cpumask
>>>>> 713a2e21a513 sched: Introduce affinity_context
>>>>> 5584e8ac2c68 sched: Add __releases annotations to affine_move_task()
>>>>> They should be able to address the problem that you list here. 
>>>>> Please let me
>>>>> know if there are still problem remaining.
>>>> Thank you. This solves the most pressing issue. The CPU-mask is still
>>>> reset upon activation of the cpuset controller.
>>>> This is due to the set_cpus_allowed_ptr() in cpuset_attach().
>>>>
>>>> Is is possible to push these patches stable?
>>> Actually, I prefer to not call set_cpus_allowed_ptr() if the cpumask 
>>> of the
>>> old and new cpusets are the same. That will save some cpu cycles. 
>>> Similarly
>>> for node_mask. If there is any changes in the cpumask, we have to call
>>> set_cpus_allowed_ptr(). I will work out a patch to that effect when 
>>> I have
>>> spare cycle.
>> But couldn't we skip that set_cpus_allowed_ptr() if the current mask is
>> a subset of the new mask and keep the behavior that the mask isn't
>> changed if the cgroup's mask changes but is still a subset?
>> That cpuset_attach() callback is only invoked while the task is
>> initially added to the cgroup which happens during enabling of the
>> controller. Or do I miss another common usecase?
>
> The only way a userspace task can have a cpumask different from that 
> of its cpuset is by calling sched_setaffinity() which will be covered 
> by the persistent user requested mask change. So your patch 2 isn't 
> really needed.
>
> Your patch 1 is relatively simple and I don't have a big objection to 
> that. I will let others chime in on what their thoughts are.

I am sorry, patch 1 is still not OK.

Suppose you move a task from a cpuset of CPUs 1,2 to another one with 
CPUs 1,2,3, for example. With your patch 1, the cpumask will not change 
at all and that is not right.

Cheers,
Longman