[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <68fa943a-68e8-44d6-ad08-97519a151a79@huawei.com>
Date: Mon, 19 Aug 2024 14:25:27 +0800
From: chenridong <chenridong@...wei.com>
To: Waiman Long <longman@...hat.com>, <tj@...nel.org>,
<lizefan.x@...edance.com>, <hannes@...xchg.org>, <adityakali@...gle.com>,
<sergeh@...nel.org>, <mkoutny@...e.com>
CC: <cgroups@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -next 1/3] cgroup/cpuset: Correct invalid remote parition
prs
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@...wei.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.
>
>
Powered by blists - more mailing lists