Re: [PATCH -next 1/3] cgroup/cpuset: Correct invalid remote parition prs

[Waiman Long]

On 8/16/24 04:27, Chen Ridong wrote:
> When enable a remote partition, I found that:
>
> cd /sys/fs/cgroup/
> mkdir test
> mkdir test/test1
> echo +cpuset > cgroup.subtree_control
> echo +cpuset >  test/cgroup.subtree_control
> echo 3 > test/test1/cpuset.cpus
> echo root > test/test1/cpuset.cpus.partition
> cat test/test1/cpuset.cpus.partition
> root invalid (Parent is not a partition root)
>
> The parent of a remote partition could not be a root. This is due to the
> emtpy effective_xcpus. It would be better to prompt the message "invalid
> cpu list in cpuset.cpus.exclusive".
>
> Signed-off-by: Chen Ridong <chenridong@xxxxxxxxxx>
> ---
>   kernel/cgroup/cpuset.c | 42 +++++++++++++++++++++++-------------------
>   1 file changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index e34fd6108b06..fdd5346616d3 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -80,6 +80,7 @@ enum prs_errcode {
>   	PERR_HOTPLUG,
>   	PERR_CPUSEMPTY,
>   	PERR_HKEEPING,
> +	PERR_PMT,
>   };
>   
>   static const char * const perr_strings[] = {
> @@ -91,6 +92,7 @@ static const char * const perr_strings[] = {
>   	[PERR_HOTPLUG]   = "No cpu available due to hotplug",
>   	[PERR_CPUSEMPTY] = "cpuset.cpus and cpuset.cpus.exclusive are empty",
>   	[PERR_HKEEPING]  = "partition config conflicts with housekeeping setup",
> +	[PERR_PMT]       = "Enable partition not permitted",
>   };
>   
>   struct cpuset {
> @@ -1669,7 +1671,7 @@ static int remote_partition_enable(struct cpuset *cs, int new_prs,
>   	 * The user must have sysadmin privilege.
>   	 */
>   	if (!capable(CAP_SYS_ADMIN))
> -		return 0;
> +		return PERR_PMT;
>   
>   	/*
>   	 * The requested exclusive_cpus must not be allocated to other
> @@ -1683,7 +1685,7 @@ static int remote_partition_enable(struct cpuset *cs, int new_prs,
>   	if (cpumask_empty(tmp->new_cpus) ||
>   	    cpumask_intersects(tmp->new_cpus, subpartitions_cpus) ||
>   	    cpumask_subset(top_cpuset.effective_cpus, tmp->new_cpus))
> -		return 0;
> +		return PERR_INVCPUS;
>   
>   	spin_lock_irq(&callback_lock);
>   	isolcpus_updated = partition_xcpus_add(new_prs, NULL, tmp->new_cpus);
> @@ -1698,7 +1700,7 @@ static int remote_partition_enable(struct cpuset *cs, int new_prs,
>   	 */
>   	update_tasks_cpumask(&top_cpuset, tmp->new_cpus);
>   	update_sibling_cpumasks(&top_cpuset, NULL, tmp);
> -	return 1;
> +	return 0;
>   }

Since you are changing the meaning of the function returned value, you 
should also update the return value comment as well.

>   
>   /*
> @@ -3151,24 +3153,26 @@ static int update_prstate(struct cpuset *cs, int new_prs)
>   		goto out;
>   
>   	if (!old_prs) {
> -		enum partition_cmd cmd = (new_prs == PRS_ROOT)
> -				       ? partcmd_enable : partcmd_enablei;
> -
>   		/*
> -		 * cpus_allowed and exclusive_cpus cannot be both empty.
> -		 */
> -		if (xcpus_empty(cs)) {
> -			err = PERR_CPUSEMPTY;
> -			goto out;
> -		}
> +		* If parent is valid partition, enable local partiion.
> +		* Otherwise, enable a remote partition.
> +		*/
> +		if (is_partition_valid(parent)) {
> +			enum partition_cmd cmd = (new_prs == PRS_ROOT)
> +					       ? partcmd_enable : partcmd_enablei;
>   
> -		err = update_parent_effective_cpumask(cs, cmd, NULL, &tmpmask);
> -		/*
> -		 * If an attempt to become local partition root fails,
> -		 * try to become a remote partition root instead.
> -		 */
> -		if (err && remote_partition_enable(cs, new_prs, &tmpmask))
> -			err = 0;
> +			/*
> +			 * cpus_allowed and exclusive_cpus cannot be both empty.
> +			 */
> +			if (xcpus_empty(cs)) {
> +				err = PERR_CPUSEMPTY;
> +				goto out;
> +			}

The xcpus_empty() check should be done for both local and remote partition.

Cheers,
Longman

> +
> +			err = update_parent_effective_cpumask(cs, cmd, NULL, &tmpmask);
> +		} else {
> +			err = remote_partition_enable(cs, new_prs, &tmpmask);
> +		}
>   	} else if (old_prs && new_prs) {
>   		/*
>   		 * A change in load balance state only, no change in cpumasks.