Re: [PATCH v7 7/9] cgroup/cpuset: Move mpol_rebind_mm/cpuset_migrate_mm() calls inside cpuset_attach_task()

[Ridong Chen <ridong.chen@linux.dev>]

On 6/21/2026 11:28 AM, Waiman Long wrote:
> The cpuset_attach_task() was introduced in commit 42a11bf5c543
> ("cgroup/cpuset: Make cpuset_fork() handle CLONE_INTO_CGROUP properly")
> to enable the CLONE_INTO_CGROUP flag of clone(2) to behave more like
> moving a task from one cpuset into another one. That commits didn't
> move the mpol_rebind_mm() and cpuset_migrate_mm() calls for group leader
> into cpuset_attach_task().
> 
> When the CLONE_INTO_CGROUP flag is used without CLONE_THREAD, the new
> task is its own group leader. So it is still not equivalent to moving
> task between cpusets in this case. Make CLONE_INTO_CGROUP behaves
> more close to cpuset_attach() by moving the mpol_rebind_mm() and
> cpuset_migrate_mm() calls inside cpuset_attach_task(). As a result,
> the following static variables will have to be updated in cpuset_fork().
>   - cpuset_attach_old_cs
>   - attach_cpus_updated
>   - attach_mems_updated
>   - queue_task_work
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>   kernel/cgroup/cpuset.c | 105 ++++++++++++++++++++++++-----------------
>   1 file changed, 62 insertions(+), 43 deletions(-)
> 
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 0375dae26d0b..511afb077e2d 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -2981,8 +2981,13 @@ static int update_prstate(struct cpuset *cs, int new_prs)
>   /*
>    * cpuset_can_attach() and cpuset_attach() specific internal data
>    * Protected by cpuset_mutex
> + *
> + * The attach_cpus_updated/attach_mems_updated flags are set in either
> + * cpuset_attach() or cpuset_fork() and used in cpuset_attach_task().
>    */
>   static struct cpuset *cpuset_attach_old_cs;
> +static bool attach_cpus_updated;
> +static bool attach_mems_updated;
>   
>   /*
>    * Check to see if a cpuset can accept a new task
> @@ -3157,9 +3162,12 @@ static void cpuset_cancel_attach(struct cgroup_taskset *tset)
>    */
>   static cpumask_var_t cpus_attach;
>   static nodemask_t cpuset_attach_nodemask_to;
> +static bool queue_task_work;
>   

There are more and more of these standalone state variables now, and 
it's getting harder to maintain. Could we group them into a struct and 
manage them together rather than keep adding globals?

Just like:

```
struct cpuset_attach_ctx {
	struct cpuset     *old_cs;
	struct llist_head src_cs, dst_cs;
	bool              cpus_updated, mems_updated, queue_work;
	nodemask_t        nodemask_to;
};
```

>   static void cpuset_attach_task(struct cpuset *cs, struct task_struct *task)
>   {
> +	struct mm_struct *mm;
> +
>   	lockdep_assert_cpuset_lock_held();
>   
>   	if (cs != &top_cpuset)
> @@ -3173,28 +3181,60 @@ static void cpuset_attach_task(struct cpuset *cs, struct task_struct *task)
>   	 */
>   	WARN_ON_ONCE(set_cpus_allowed_ptr(task, cpus_attach));
>   
> +	if (cpuset_v2() && !attach_mems_updated)
> +		return;
> +
>   	cpuset_change_task_nodemask(task, &cpuset_attach_nodemask_to);
>   	cpuset1_update_task_spread_flags(cs, task);
> +
> +	if ((task != task->group_leader) ||
> +	    (!is_memory_migrate(cs) && !attach_mems_updated))
> +		return;
> +
> +	/*
> +	 * Change mm for threadgroup leader. This is expensive and may
> +	 * sleep and should be moved outside migration path proper.
> +	 */
> +	mm = get_task_mm(task);
> +	if (mm) {
> +		struct cpuset *oldcs = cpuset_attach_old_cs;
> +
> +		mpol_rebind_mm(mm, &cs->effective_mems);
> +
> +		/*
> +		 * old_mems_allowed is the same with mems_allowed
> +		 * here, except if this task is being moved
> +		 * automatically due to hotplug.  In that case
> +		 * @mems_allowed has been updated and is empty, so
> +		 * @old_mems_allowed is the right nodesets that we
> +		 * migrate mm from.
> +		 */
> +		if (is_memory_migrate(cs)) {
> +			cpuset_migrate_mm(mm, &oldcs->old_mems_allowed,
> +					  &cpuset_attach_nodemask_to);
> +			queue_task_work = true;
> +		} else {
> +			mmput(mm);
> +		}
> +	}
>   }
>   
>   static void cpuset_attach(struct cgroup_taskset *tset)
>   {
>   	struct task_struct *task;
> -	struct task_struct *leader;
>   	struct cgroup_subsys_state *css;
>   	struct cpuset *cs;
>   	struct cpuset *oldcs = cpuset_attach_old_cs;
> -	bool cpus_updated, mems_updated;
> -	bool queue_task_work = false;
>   
>   	cgroup_taskset_first(tset, &css);
>   	cs = css_cs(css);
>   
>   	lockdep_assert_cpus_held();	/* see cgroup_attach_lock() */
>   	mutex_lock(&cpuset_mutex);
> -	cpus_updated = !cpumask_equal(cs->effective_cpus,
> -				      oldcs->effective_cpus);
> -	mems_updated = !nodes_equal(cs->effective_mems, oldcs->effective_mems);
> +	queue_task_work = false;
> +
> +	attach_cpus_updated = !cpumask_equal(cs->effective_cpus, oldcs->effective_cpus);
> +	attach_mems_updated = !nodes_equal(cs->effective_mems, oldcs->effective_mems);
>   	guarantee_online_mems(cs, &cpuset_attach_nodemask_to);
>   
>   	/*
> @@ -3203,44 +3243,12 @@ static void cpuset_attach(struct cgroup_taskset *tset)
>   	 * and mems. In that case, we can optimize out by skipping the task
>   	 * iteration and update.
>   	 */
> -	if (cpuset_v2() && !cpus_updated && !mems_updated)
> +	if (cpuset_v2() && !attach_cpus_updated && !attach_mems_updated)
>   		goto out;
>   
>   	cgroup_taskset_for_each(task, css, tset)
>   		cpuset_attach_task(cs, task);
>   
> -	/*
> -	 * Change mm for all threadgroup leaders. This is expensive and may
> -	 * sleep and should be moved outside migration path proper. Skip it
> -	 * if there is no change in effective_mems and CS_MEMORY_MIGRATE is
> -	 * not set.
> -	 */
> -	if (!is_memory_migrate(cs) && !mems_updated)
> -		goto out;
> -
> -	cgroup_taskset_for_each_leader(leader, css, tset) {
> -		struct mm_struct *mm = get_task_mm(leader);
> -
> -		if (mm) {
> -			mpol_rebind_mm(mm, &cs->effective_mems);
> -
> -			/*
> -			 * old_mems_allowed is the same with mems_allowed
> -			 * here, except if this task is being moved
> -			 * automatically due to hotplug.  In that case
> -			 * @mems_allowed has been updated and is empty, so
> -			 * @old_mems_allowed is the right nodesets that we
> -			 * migrate mm from.
> -			 */
> -			if (is_memory_migrate(cs)) {
> -				cpuset_migrate_mm(mm, &oldcs->old_mems_allowed,
> -						  &cpuset_attach_nodemask_to);
> -				queue_task_work = true;
> -			} else
> -				mmput(mm);
> -		}
> -	}
> -
>   out:
>   	if (queue_task_work)
>   		schedule_flush_migrate_mm();
> @@ -3689,15 +3697,14 @@ static void cpuset_cancel_fork(struct task_struct *task, struct css_set *cset)
>    */
>   static void cpuset_fork(struct task_struct *task)
>   {
> -	struct cpuset *cs;
> -	bool same_cs;
> +	struct cpuset *cs, *oldcs;
>   
>   	rcu_read_lock();
>   	cs = task_cs(task);
> -	same_cs = (cs == task_cs(current));
> +	oldcs = task_cs(current);
>   	rcu_read_unlock();
>   
> -	if (same_cs) {
> +	if (cs == oldcs) {
>   		if (cs == &top_cpuset)
>   			return;
>   
> @@ -3709,7 +3716,19 @@ static void cpuset_fork(struct task_struct *task)
>   	/* CLONE_INTO_CGROUP */
>   	mutex_lock(&cpuset_mutex);
>   	guarantee_online_mems(cs, &cpuset_attach_nodemask_to);
> +	cs->old_mems_allowed = cpuset_attach_nodemask_to;
> +
> +	/*
> +	 * Assume CPUs and memory nodes are updated
> +	 * A CLONE_INTO_CGROUP operation should have taken the cgroup mutex
> +	 * and so there shouldn't be a competing cpuset_attach() operation.
> +	 */
> +	attach_cpus_updated = attach_mems_updated = true;
> +	queue_task_work = false;
> +	cpuset_attach_old_cs = oldcs;
>   	cpuset_attach_task(cs, task);
> +	if (queue_task_work)
> +		schedule_flush_migrate_mm();
>   
>   	dec_attach_in_progress_locked(cs);
>   	mutex_unlock(&cpuset_mutex);

-- 
Best regards
Ridong