Re: [PATCH -next RFC -v2 05/11] cpuset: refactor CPU mask buffer parsing logic

[Waiman Long <llong@redhat.com>]

On 9/8/25 11:32 PM, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
>
> The current implementation contains redundant handling for empty mask
> inputs, as cpulist_parse() already properly handles these cases. This
> refactoring introduces a new helper function parse_cpuset_cpulist() to
> consolidate CPU list parsing logic and eliminate special-case checks for
> empty inputs.
>
> Additionally, the effective_xcpus computation for trial cpusets has been
> simplified. Rather than computing effective_xcpus only when exclusive_cpus
> is set or when the cpuset forms a valid partition, we now recalculate it
> on every cpuset.cpus update. This approach ensures consistency and allows
> removal of redundant effective_xcpus logic in subsequent patches.
>
> The trial cpuset's effective_xcpus calculation follows two distinct cases:
> 1. For member cpusets: effective_xcpus is determined by the intersection
>     of cpuset->exclusive_cpus and the parent's effective_xcpus.
> 2. For non-member cpusets: effective_xcpus is derived from the intersection
>     of user_xcpus and the parent's effective_xcpus.
>
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> ---
>   kernel/cgroup/cpuset.c | 59 +++++++++++++++++++++---------------------
>   1 file changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 6015322a10ac..55674a5ad2f9 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -169,6 +169,11 @@ static inline bool is_partition_invalid(const struct cpuset *cs)
>   	return cs->partition_root_state < 0;
>   }
>   
> +static inline bool cs_is_member(const struct cpuset *cs)
> +{
> +	return cs->partition_root_state == PRS_MEMBER;
> +}
> +
>   /*
>    * Callers should hold callback_lock to modify partition_root_state.
>    */
> @@ -1478,7 +1483,13 @@ static int compute_trialcs_excpus(struct cpuset *trialcs, struct cpuset *cs)
>   	struct cpuset *parent = parent_cs(trialcs);
>   	struct cpumask *excpus = trialcs->effective_xcpus;
>   
> -	cpumask_and(excpus, user_xcpus(trialcs), parent->effective_xcpus);
> +	/* trialcs is member, cpuset.cpus has no impact to excpus */
> +	if (cs_is_member(cs))
> +		cpumask_and(excpus, trialcs->exclusive_cpus,
> +				parent->effective_xcpus);
> +	else
> +		cpumask_and(excpus, user_xcpus(trialcs), parent->effective_xcpus);
> +
>   	return rm_siblings_excl_cpus(parent, cs, excpus);
>   }
>   
> @@ -2348,6 +2359,19 @@ static void update_sibling_cpumasks(struct cpuset *parent, struct cpuset *cs,
>   	rcu_read_unlock();
>   }
>   
> +static int parse_cpuset_cpulist(const char *buf, struct cpumask *out_mask)
> +{
> +	int retval;
> +
> +	retval = cpulist_parse(buf, out_mask);
> +	if (retval < 0)
> +		return retval;
> +	if (!cpumask_subset(out_mask, top_cpuset.cpus_allowed))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>   /**
>    * update_cpumask - update the cpus_allowed mask of a cpuset and all tasks in it
>    * @cs: the cpuset to consider
> @@ -2364,34 +2388,9 @@ static int update_cpumask(struct cpuset *cs, struct cpuset *trialcs,
>   	bool force = false;
>   	int old_prs = cs->partition_root_state;
>   
> -	/*
> -	 * An empty cpus_allowed is ok only if the cpuset has no tasks.
> -	 * Since cpulist_parse() fails on an empty mask, we special case
> -	 * that parsing.  The validate_change() call ensures that cpusets
> -	 * with tasks have cpus.
> -	 */
> -	if (!*buf) {
> -		cpumask_clear(trialcs->cpus_allowed);
> -		if (cpumask_empty(trialcs->exclusive_cpus))
> -			cpumask_clear(trialcs->effective_xcpus);
> -	} else {
> -		retval = cpulist_parse(buf, trialcs->cpus_allowed);
> -		if (retval < 0)
> -			return retval;
> -
> -		if (!cpumask_subset(trialcs->cpus_allowed,
> -				    top_cpuset.cpus_allowed))
> -			return -EINVAL;
> -
> -		/*
> -		 * When exclusive_cpus isn't explicitly set, it is constrained
> -		 * by cpus_allowed and parent's effective_xcpus. Otherwise,
> -		 * trialcs->effective_xcpus is used as a temporary cpumask
> -		 * for checking validity of the partition root.
> -		 */
> -		if (!cpumask_empty(trialcs->exclusive_cpus) || is_partition_valid(cs))
> -			compute_trialcs_excpus(trialcs, cs);
> -	}
> +	retval = parse_cpuset_cpulist(buf, trialcs->cpus_allowed);
> +	if (retval < 0)
> +		return retval;
>   
>   	/* Nothing to do if the cpus didn't change */
>   	if (cpumask_equal(cs->cpus_allowed, trialcs->cpus_allowed))
> @@ -2400,6 +2399,8 @@ static int update_cpumask(struct cpuset *cs, struct cpuset *trialcs,
>   	if (alloc_tmpmasks(&tmp))
>   		return -ENOMEM;
>   
> +	compute_trialcs_excpus(trialcs, cs);
> +
>   	if (old_prs) {
>   		if (is_partition_valid(cs) &&
>   		    cpumask_empty(trialcs->effective_xcpus)) {
Reviewed-by: Waiman Long <longman@redhat.com>