Re: [PATCH v3 3/4] cpuset: settle isolation state when CPUs are given back to parent

[Ridong Chen <ridong.chen@linux.dev>]

On 11/9/2026 10:51 PM, Waiman Long wrote:
> On 11/7/26 7:00 AM, Ridong Chen wrote:
>> From: Ridong Chen <chenridong@xiaomi.com>
>>
>> partition_xcpus_del() only updates isolated_cpus when the partition that
>> gives back the CPUs has a different state than its parent. CPUs that were
>> further distributed to descendant isolated partitions are not taken into
>> account, so isolated_cpus_release() has to be called afterwards in
>> several places to walk up the ancestors and fix isolated_cpus.
>>
>> Make partition_xcpus_del() always set the isolation state of the given
>> back CPUs to the state of the parent that receives them. As the
>> effective_xcpus of a partition covers the CPUs distributed to its
>> descendants, the isolation state of those CPUs is settled at the same
>> time. Boot-time isolated CPUs are still preserved by isolated_cpu_update().
>>
>> compute_partition_effective_cpumask() invalidates a child partition
>> without going through partition_xcpus_del(), so do the same there: the
>> CPUs of the invalidated child take the isolation state of its parent.
>>
>> As the parent may be shrunk and its effective_xcpus not updated yet,
>> only do so for the CPUs the parent still owns. This is to keep the
>> isolation state correct for now, the in-place invalidation here will be
>> removed later in the partition rework.
>>
>> isolated_cpus_update() assumes the old and new states are different,
>> which no longer holds now that partition_xcpus_del() proceeds regardless
>> of the states. Drop its @old_prs argument and the WARN_ON_ONCE() so that
>> it simply sets the isolation state of the given CPUs, and use it
>> everywhere. The state check in partition_xcpus_add() is dropped as well,
>> as setting the state is a no-op when the states are the same.
>>
>> isolated_cpus_release() is no longer needed and is removed.
>>
>> Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
>> ---
>>   kernel/cgroup/cpuset.c | 98 ++++++++----------------------------------
>>   1 file changed, 19 insertions(+), 79 deletions(-)
>>
>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>> index 85e5b529d144..fc9de6e16ff5 100644
>> --- a/kernel/cgroup/cpuset.c
>> +++ b/kernel/cgroup/cpuset.c
>> @@ -1244,17 +1244,17 @@ static bool isolated_cpu_update(int new_prs, int cpu)
>>   }
>>   /*
>> - * isolated_cpus_update - Update the isolated_cpus mask
>> - * @old_prs: old partition_root_state
>> - * @new_prs: new partition_root_state
>> - * @xcpus: exclusive CPUs with state change
>> + * isolated_cpus_update - Set the isolation state of @xcpus
>> + * @new_prs: partition root state that @xcpus will belong to
>> + * @xcpus: exclusive CPUs whose isolation state may change
>> + *
>> + * CPUs isolated at boot time are kept isolated.
>>    */
>> -static void isolated_cpus_update(int old_prs, int new_prs, struct cpumask 
>> *xcpus)
>> +static void isolated_cpus_update(int new_prs, struct cpumask *xcpus)
>>   {
>>       bool updated = false;
>>       int cpu;
>> -    WARN_ON_ONCE(old_prs == new_prs);
>>       lockdep_assert_held(&callback_lock);
>>       lockdep_assert_held(&cpuset_mutex);
>>       for_each_cpu(cpu, xcpus)
>> @@ -1263,45 +1263,6 @@ static void isolated_cpus_update(int old_prs, int 
>> new_prs, struct cpumask *xcpus
>>           update_housekeeping = true;
>>   }
>> -/*
>> - * isolated_cpus_release - Remove the relevant CPUs in @xcpus from isolated_cpus
>> - * @cs: the current cpuset
>> - * @xcpus: the set of effective xcpus to be considered for removal
>> - *
>> - * This function is called when an isolated partition is invalidated or when
>> - * some of its effective xcpus are to be removed from isolated_cpus due to
>> - * changes in parent cpuset. The given @xcpus must be modifiable as the function
>> - * will update its value to determine the right set of CPUs to be removed.
>> - */
>> -static void isolated_cpus_release(struct cpuset *cs, struct cpumask *xcpus)
>> -{
>> -    struct cpuset *ancestor = parent_cs(cs);
>> -    struct cpuset *iancestor = NULL;
>> -
>> -    /*
>> -     * Search the furthest isolated partition root ancestor available
>> -     * until top_cpuset or a remote partition root is hit.
>> -     */
>> -    for (; ancestor; ancestor = parent_cs(ancestor)) {
>> -        if (ancestor->partition_root_state == PRS_ISOLATED)
>> -            iancestor = ancestor;
>> -        if (is_remote_partition(ancestor))
>> -            break;
>> -    }
>> -
>> -    /*
>> -     * If a furthest isolated partition acnestor is found, use it to
>> -     * determine which of the CPUs in @xcpus should be removed from
>> -     * isolated_cpus. Otherwise, all of them will be removed.
>> -     */
>> -    if (iancestor && !cpumask_andnot(xcpus, xcpus, iancestor->effective_xcpus))
>> -        return; /* No CPU should be removed from isolated_cpus */
>> -
>> -    spin_lock_irq(&callback_lock);
>> -    isolated_cpus_update(PRS_ISOLATED, PRS_ROOT, xcpus);
>> -    spin_unlock_irq(&callback_lock);
>> -}
>> -
>>   /*
>>    * partition_xcpus_add - Add new exclusive CPUs to partition
>>    * @new_prs: new partition_root_state
>> @@ -1325,9 +1286,7 @@ static void partition_xcpus_add(int new_prs, struct 
>> cpuset *parent,
>>       if (parent == &top_cpuset)
>>           cpumask_or(subpartitions_cpus, subpartitions_cpus, xcpus);
>> -    if (new_prs != parent->partition_root_state)
>> -        isolated_cpus_update(parent->partition_root_state, new_prs,
>> -                     xcpus);
>> +    isolated_cpus_update(new_prs, xcpus);
>>       cpumask_andnot(parent->effective_cpus, parent->effective_cpus, xcpus);
>>   }
>> @@ -1355,9 +1314,8 @@ static void partition_xcpus_del(int old_prs, struct 
>> cpuset *parent,
>>       if (parent == &top_cpuset)
>>           cpumask_andnot(subpartitions_cpus, subpartitions_cpus, xcpus);
>> -    if (old_prs != parent->partition_root_state)
>> -        isolated_cpus_update(old_prs, parent->partition_root_state,
>> -                     xcpus);
>> +    /* CPUs given back, including those of descendants, take parent's state */
>> +    isolated_cpus_update(parent->partition_root_state, xcpus);
>>       cpumask_or(parent->effective_cpus, parent->effective_cpus, xcpus);
>>       cpumask_and(parent->effective_cpus, parent->effective_cpus, 
>> cpu_active_mask);
>> @@ -1612,6 +1570,7 @@ static void compute_partition_effective_cpumask(struct 
>> cpuset *cs,
>>       struct cgroup_subsys_state *css;
>>       struct cpuset *child;
>>       bool populated = partition_is_populated(cs, NULL);
>> +    static cpumask_t xcpus;        /* Protected by cpuset_mutex */
>>       /*
>>        * Check child partition roots to see if they should be invalidated
>> @@ -1640,14 +1599,15 @@ static void compute_partition_effective_cpumask(struct 
>> cpuset *cs,
>>               int parent_prs = cs->partition_root_state;
>>               /*
>> -             * Invalidate child partition and update isolated_cpus
>> -             * if necessary.
>> +             * Invalidate child partition. Its CPUs still owned by
>> +             * cs take the isolation state of cs. effective_xcpus
>> +             * of cs may not be updated yet, so recompute it.
>>                */
>>               spin_lock_irq(&callback_lock);
>>               WRITE_ONCE(child->prs_err, PERR_NOCPUS);
>> -            if ((old_prs == PRS_ISOLATED) && (parent_prs == PRS_ROOT))
>> -                isolated_cpus_update(PRS_ISOLATED, PRS_ROOT,
>> -                             child->effective_xcpus);
>> +            compute_excpus(cs, &xcpus);
>> +            cpumask_and(&xcpus, &xcpus, child->effective_xcpus);
>> +            isolated_cpus_update(parent_prs, &xcpus);
>>               child->partition_root_state = -old_prs;
>>               spin_unlock_irq(&callback_lock);
>>               notify_partition_change(child, old_prs);
>> @@ -2080,22 +2040,8 @@ static int update_parent_effective_cpumask(struct 
>> cpuset *cs, int cmd,
>>            *
>>            * A partition error happens when parent has tasks and all
>>            * its effective CPUs will have to be distributed out.
>> -         *
>> -         * For an isolated partition with a shrunk root partition parent
>> -         * we may need to return some or all of its isolated CPUs.
>>            */
>>           nocpu |= cpumask_empty(tmp->delmask);
>> -        if ((parent_prs == PRS_ROOT) && (old_prs == PRS_ISOLATED)) {
>> -            cpumask_clear(tmp->addmask);
>> -            if (nocpu)
>> -                cpumask_copy(tmp->addmask, cs->effective_xcpus);
>> -            else if (!cpumask_subset(cs->effective_xcpus, parent- 
>> >effective_xcpus))
>> -                cpumask_andnot(tmp->addmask, cs->effective_xcpus,
>> -                                 parent->effective_xcpus);
>> -            if (!cpumask_empty(tmp->addmask))
>> -                isolated_cpus_release(cs, tmp->addmask);
>> -            /* Expansion of parent's effective_xcpus is handled below */
>> -        }
>>           if (nocpu) {
>>               part_error = PERR_NOCPUS;
>>               if (is_partition_valid(cs))
>> @@ -2341,18 +2287,12 @@ static void update_cpumasks_hier(struct cpuset *cs, 
>> struct tmpmasks *tmp,
>>                   /*
>>                    * When parent is not a valid partition root,
>>                    * child partition roots become invalid too.
>> -                 *
>> -                 * For a to be invalidated isolated partition,
>> -                 * call return_isolated_cpus() to remove the
>> -                 * relevant CPUs from isolated_cpus.
>> +                 * The isolation state of their CPUs has been
>> +                 * settled when the parent gave them back.
>>                    */
>>                   if (is_partition_valid(cp))
>>                       new_prs = -old_prs;
>> -                if (old_prs == PRS_ISOLATED) {
>> -                    cpumask_copy(tmp->delmask, cp->effective_xcpus);
>> -                    isolated_cpus_release(cp, tmp->delmask);
>> -                }
>>                   WRITE_ONCE(cp->prs_err,
>>                          is_partition_invalid(parent)
>>                          ? PERR_INVPARENT : PERR_NOTPART);
>> @@ -3108,7 +3048,7 @@ static int update_prstate(struct cpuset *cs, int new_prs)
>>       if (!is_partition_valid(cs))
>>           reset_partition_data(cs);
>>       else if (isolcpus_updated)
>> -        isolated_cpus_update(old_prs, new_prs, cs->effective_xcpus);
>> +        isolated_cpus_update(new_prs, cs->effective_xcpus);
>>       spin_unlock_irq(&callback_lock);
>>       /* Force update if partition is disabled & update effective_xcpus */
> 
> The code change looks good to me. However this is not just a code restructuring, 
> it also includes a bug fix. I wonder if we should clarify that and maybe add a 
> Fixes tag.
> 
Hi Longman,

Thanks for the review. You are right, it also fixes the isolated CPU
leak reported by Guopeng [1]. When a root partition with an isolated
child partition is invalidated, partition_xcpus_del() gives the CPUs
back to the parent without updating isolated_cpus as both are root
partitions, so the CPUs of the isolated child are left in
isolated_cpus.

I'd like to describe it in the commit message and add the following
tags in the next version:

Fixes: 11e5f407b64a ("cgroup/cpuset: Keep track of CPUs in isolated partitions")
Reported-by: Guopeng Zhang <zhangguopeng@kylinos.cn>
Closes: https://lore.kernel.org/all/20261105130344.664336-4-guopeng.zhang@linux.dev/

Does that look OK to you?

[1] https://lore.kernel.org/all/20261105130344.664336-4-guopeng.zhang@linux.dev/

-- 
Best regards
Ridong