Re: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation

[Waiman Long <longman@redhat.com>]

On 9/2/26 6:26 AM, Guopeng Zhang wrote:
> From: Guopeng Zhang <zhangguopeng@kylinos.cn>
>
> compute_partition_effective_cpumask() checks whether each valid child
> partition remains covered by the parent exclusive CPU mask and whether it
> would consume all remaining CPUs of a populated parent.
>
> Factor these two checks into child_partition_error() so the same rules can
> be reused when evaluating a proposed parent configuration. This is a
> preparatory refactoring with no intended functional change.
>
> Signed-off-by: Guopeng Zhang <zhangguopeng@kylinos.cn>
> ---
>   kernel/cgroup/cpuset.c | 38 +++++++++++++++++++++++++++++---------
>   1 file changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 8f24171b6055..6994dc75d940 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -2085,6 +2085,26 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>   	return 0;
>   }
>   
> +/*
> + * Return the error that will invalidate a child partition under a proposed
> + * parent partition configuration.
> + */
> +static enum prs_errcode
> +child_partition_error(struct cpuset *child,
> +		      const struct cpumask *partition_cpus,
> +		      const struct cpumask *remaining_cpus,
> +		      bool parent_populated)
I think you should add some functional comments on what "partition_cpus" 
and "remaining_cpus" are supposed to be so that caller knows what to 
pass into this helper.
> +{
> +	if (!cpumask_subset(child->effective_xcpus, partition_cpus))
> +		return PERR_INVCPUS;
> +
> +	if (parent_populated &&
> +	    cpumask_subset(remaining_cpus, child->effective_xcpus))
> +		return PERR_NOCPUS;
> +
> +	return PERR_NONE;
> +}
> +
>   /**
>    * compute_partition_effective_cpumask - compute effective_cpus for partition
>    * @cs: partition root cpuset
> @@ -2121,6 +2141,8 @@ static void compute_partition_effective_cpumask(struct cpuset *cs,
>   
>   	rcu_read_lock();
>   	cpuset_for_each_child(child, css, cs) {
> +		enum prs_errcode child_err;
> +
>   		if (!is_partition_valid(child))
>   			continue;
>   
> @@ -2129,15 +2151,13 @@ static void compute_partition_effective_cpumask(struct cpuset *cs,
>   		 * partition root.
>   		 */
>   		WARN_ON_ONCE(is_remote_partition(child));
> -		WRITE_ONCE(child->prs_err, 0);
> -		if (!cpumask_subset(child->effective_xcpus,
> -				    cs->effective_xcpus))
> -			WRITE_ONCE(child->prs_err, PERR_INVCPUS);
> -		else if (populated &&
> -			 cpumask_subset(new_ecpus, child->effective_xcpus))
> -			WRITE_ONCE(child->prs_err, PERR_NOCPUS);
> -
> -		if (child->prs_err) {
> +		WRITE_ONCE(child->prs_err, PERR_NONE);
> +		child_err = child_partition_error(child, cs->effective_xcpus,
> +						  new_ecpus, populated);
> +		if (child_err)
> +			WRITE_ONCE(child->prs_err, child_err);
> +

You can ignore the inital PERR_NONE write and always write the child_err 
value into child->prs_err.

Not big issue, just some nits.

Cheers,
Longman

> +		if (child_err) {
>   			int old_prs = child->partition_root_state;
>   
>   			/*