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

[chenridong <chenridong@huawei.com>]

On 2024/8/19 10:18, Waiman Long wrote:
> On 8/18/24 22:14, Waiman Long wrote:
>>
>> 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@huawei.com>
>>> ---
>>>   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,
> 
> One more thing, the "PMT" acronym for the error code is hard to decode. 
> I will suggest you either use the "PERMISSION" or "ACCESS" like the 
> EACCESS errno.
> 
> Cheers,
> Longman
> 
Thanks, will do.
>>>   };
>>>     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.
>>
Will do.
>>>     /*
>>> @@ -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
>>
Thanks, I will do it at V2.

Thanks,
Ridong
>>> +
>>> +            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.
> 
>