Re: [PATCH-next 2/6] cgroup/cpuset: Properly compute effective xcpus in update_parent_effective_cpumask()

[Guopeng Zhang <guopeng.zhang@linux.dev>]

Hi Longman,

While re-reading and studying this patch, I noticed a few comments
in update_parent_effective_cpumask() that still describe the pre-patch
behavior.

在 2026/10/22 11:16, Waiman Long 写道:
> Since the introduction of cpuset.cpus.exclusive in commit
> e2ffe502ba45 ("cgroup/cpuset: Add cpuset.cpus.exclusive for v2"), a
> compute_effective_exclusive_cpumask() helper which is later renamed to
> compute_excpus() is added to compute the effective xcpus of a partition
> root. However update_parent_effective_cpumask() is not updated in all
> proper places to use the new helper to check if a given cpuset should
> remain a valid partition root. This can cause a cpuset to incorrectly
> remain a valid partition root even if no effective xcpus is left.
> 
> Fix that problem by using compute_excpus() to compute the effective xcpus
> into delmask in all cases except partcmd_disable and partcmd_invalidate
> and use it for tasks_nocpu_error() check as well as checking its
> emptiness. This enables us to catch invalid partition root more
> accurately in update_parent_effective_cpumask().
> 
> Some test case are added to test_cpuset_prs.sh for invalidating a
> partition root due to its effective xcpus becoming empty as well as
> making sure that an invalid partition can only be revived when exclusive
> CPUs are available agin.
> 
> Fixes: e2ffe502ba45 ("cgroup/cpuset: Add cpuset.cpus.exclusive for v2")
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  kernel/cgroup/cpuset.c                        | 36 +++++++++----------
>  .../selftests/cgroup/test_cpuset_prs.sh       |  6 ++++
>  2 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 7ead8c3b7974..db6e4663f4be 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -1798,7 +1798,6 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,

The function doc header above says:

    Otherwise, effective_xcpus is assumed to remain the same.

With this patch applied, the no-newmask path recomputes the effective
xcpus via compute_excpus() and invalidates the partition when the
result is empty, so that sentence no longer matches the actual
behavior. Maybe:

    Otherwise, the requested cpu list is unchanged, but the effective
    xcpus is recomputed and an empty result invalidates the partition.

or some better wording.

>  	int deleting;	/* Deleting cpus from parent's effective_cpus	*/
>  	int old_prs, new_prs;
>  	int part_error = PERR_NONE;	/* Partition error? */
> -	struct cpumask *xcpus = user_xcpus(cs);
>  	int parent_prs = parent->partition_root_state;
>  	bool nocpu;
>  
> @@ -1841,30 +1840,29 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>  	if (!newmask && xcpus_empty(cs))
>  		return PERR_CPUSEMPTY;
>  
> -	nocpu = tasks_nocpu_error(parent, cs, xcpus);
> +	/*
> +	 * Except for partcmd_disable, use delmask to compute the current
> +	 * effective xcpus value and use it for tasks_nocpu_error check.
> +	 */
> +	if (cmd != partcmd_disable) {
> +		compute_excpus(cs, tmp->delmask);
> +		nocpu = tasks_nocpu_error(parent, cs, tmp->delmask);
> +	}
>  
>  	if ((cmd == partcmd_enable) || (cmd == partcmd_enablei)) {
> -		/*
> -		 * Need to call compute_excpus() in case
> -		 * exclusive_cpus not set. Sibling conflict should only happen
> -		 * if exclusive_cpus isn't set.
> -		 */
> -		xcpus = tmp->delmask;
> -		if (compute_excpus(cs, xcpus))
> -			WARN_ON_ONCE(!cpumask_empty(cs->exclusive_cpus));
>  		new_prs = (cmd == partcmd_enable) ? PRS_ROOT : PRS_ISOLATED;
>  
>  		/*
>  		 * Enabling partition root is not allowed if its
>  		 * effective_xcpus is empty.
>  		 */
> -		if (cpumask_empty(xcpus))
> +		if (cpumask_empty(tmp->delmask))
>  			return PERR_INVCPUS;
>  
> -		if (prstate_housekeeping_conflict(new_prs, parent_prs, xcpus, NULL))
> +		if (prstate_housekeeping_conflict(new_prs, parent_prs, tmp->delmask, NULL))
>  			return PERR_HKEEPING;
>  
> -		if (tasks_nocpu_error(parent, cs, xcpus))
> +		if (nocpu)
>  			return PERR_NOCPUS;
>  
>  		/*
> @@ -1876,9 +1874,8 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>  		 *
>  		 * Warn if it is not the case.
>  		 */
> -		cpumask_and(tmp->new_cpus, xcpus, cpu_active_mask);
> +		cpumask_and(tmp->new_cpus, tmp->delmask, cpu_active_mask);
>  		WARN_ON_ONCE(!cpumask_subset(tmp->new_cpus, parent->effective_cpus));
> -
>  		deleting = true;
>  	} else if (cmd == partcmd_disable) {

The comment in this partcmd_disable branch still refers to "xcpus",

    /* ... xcpus may contain CPUs that shouldn't be removed ... */

but the xcpus variable is removed by this patch.

>  		/*
> @@ -1973,7 +1970,7 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>  		 * A partition error happens when parent has tasks and all
>  		 * its effective CPUs will have to be distributed out.
>  		 */
> -		if (nocpu) {
> +		if (nocpu || cpumask_empty(tmp->delmask)) {
The header comment of this "partcmd_update w/o newmask" branch above
still says

  /*  delmask = effective_xcpus & parent->effective_cpus ... */

That matched the old code where the branch computed delmask from
user_xcpus() itself. Now delmask comes from the compute_excpus() call
at the top of the function, and the branch only intersects it with
parent->effective_cpus when reviving an invalid partition.

All minor comment updates, of course.

Thanks,
Guopeng