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,
};
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;
}
/*
@@ -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;
+ }
+
+ 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.
--
2.34.1
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,
> };
>
> 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.
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
>> };
>> 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.
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.
>
>
© 2016 - 2026 Red Hat, Inc.