Message-ID: <e8018984-96ed-49fa-8571-79a0d39cb218@redhat.com>
Date: Mon, 20 Oct 2025 15:42:22 -0400
From: Waiman Long <llong@...hat.com>
To: Chen Ridong <chenridong@...weicloud.com>, Waiman Long <llong@...hat.com>,
 tj@...nel.org, hannes@...xchg.org, mkoutny@...e.com
Cc: cgups@...r.kernel.org, linux-kernel@...r.kernel.org,
 lujialin4@...wei.com, chenridong@...wei.com
Subject: Re: [PATCH -next RFC 03/16] cpuset: factor out partition_enable()
 function

On 10/20/25 3:48 AM, Chen Ridong wrote:
>
> On 2025/10/20 10:39, Waiman Long wrote:
>> On 9/28/25 3:12 AM, Chen Ridong wrote:
>>> From: Chen Ridong <chenridong@...wei.com>
>>>
>>> Extract the core partition enablement logic into a dedicated
>>> partition_enable() function. This refactoring centralizes updates to key
>>> cpuset data structures including remote_sibling, effective_xcpus,
>>> partition_root_state, and prs_err.
>>>
>>> The function handles the complete partition enablement workflow:
>>> - Adding exclusive CPUs via partition_xcpus_add()
>>> - Managing remote sibling relationships
>>> - Synchronizing effective exclusive CPUs mask
>>> - Updating partition state and error status
>>> - Triggering required scheduler domain rebuilds
>>>
>>> This creates a coherent interface for partition operations and establishes
>>> a foundation for future local partition support while maintaining existing
>>> remote partition behavior.
>>>
>>> Signed-off-by: Chen Ridong <chenridong@...wei.com>
>>> ---
>>>    kernel/cgroup/cpuset.c | 55 +++++++++++++++++++++++++++++++++---------
>>>    1 file changed, 44 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>>> index 0787904321a9..43ce62f4959c 100644
>>> --- a/kernel/cgroup/cpuset.c
>>> +++ b/kernel/cgroup/cpuset.c
>>> @@ -1515,6 +1515,49 @@ static inline bool is_local_partition(struct cpuset *cs)
>>>        return is_partition_valid(cs) && !is_remote_partition(cs);
>>>    }
>>>    +static void partition_state_update(struct cpuset *cs, int new_prs,
>>> +                      enum prs_errcode prs_err)
>>> +{
>>> +    lockdep_assert_held(&callback_lock);
>>> +
>>> +    cs->partition_root_state = new_prs;
>>> +    WRITE_ONCE(cs->prs_err, prs_err);
>>> +    if (!is_partition_valid(cs))
>>> +        reset_partition_data(cs);
>>> +}
>>> +
>>> +/**
>>> + * partition_enable - Transitions a cpuset to a partition root
>>> + * @cs: The cpuset to enable partition for
>>> + * @parent: Parent cpuset of @cs, NULL for remote parent
>>> + * @new_prs: New partition root state to set
>>> + * @new_excpus: New exclusive CPUs mask for the partition
>>> + *
>>> + * Transitions a cpuset to a partition root, only for v2.
>>> + */
>>> +static void partition_enable(struct cpuset *cs, struct cpuset *parent,
>>> +                 int new_prs, struct cpumask *new_excpus)
>>> +{
>>> +    bool isolcpus_updated;
>>> +
>>> +    lockdep_assert_held(&cpuset_mutex);
>>> +    WARN_ON_ONCE(new_prs <= 0);
>>> +    WARN_ON_ONCE(!cpuset_v2());
>>> +
>>> +    if (cs->partition_root_state == new_prs)
>>> +        return;
>>> +
>>> +    spin_lock_irq(&callback_lock);
>>> +    /* enable partition should only add exclusive cpus */
>>> +    isolcpus_updated = partition_xcpus_add(new_prs, parent, new_excpus);
>>> +    list_add(&cs->remote_sibling, &remote_children);
>>> +    cpumask_copy(cs->effective_xcpus, new_excpus);
>>> +    partition_state_update(cs, new_prs, PERR_NONE);
>>> +    spin_unlock_irq(&callback_lock);
>>> +    update_unbound_workqueue_cpumask(isolcpus_updated);
>>> +    cpuset_force_rebuild();
>>> +}
>>> +
>> partition_enable() is supposed to be a common helper used for the creation of both local and remote
>> partitions. The one in this patch does work for remote partition but not for local partition. I
>> would prefer to make it good for both cases when you introduce it instead adding code in patch 6 to
>> make it work for local partition later in the series. It will make it easier to review instead of
>> jumping back and forth to make sure that it will do the right thing.
>>
>> Cheers,
>> Longman
>>
> Thank you, Longman.
>
> My original intention was to keep the changes easier to review. Patches 3–5 are meant to be pure
> refactoring moves of code from the remote partition logic, without altering any behavior.
>
> Would it be clearer to proceed in the following stages:
>
> 1. Introduce partition_enable(), partition_disable(), and partition_update() with their complete
> logic first.
> 2. Replace the corresponding logic in remote partitions with these new helpers.
> 3. Then, replace the logic in local partitions with the same helpers.
>
Yes, that should make it easier to review.

Thanks,
Longman

Powered by blists - more mailing lists