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

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

On 11/10/2026 4:18 PM, Guopeng Zhang wrote:
> 
> 
> 在 2026/11/10 11:14, Ridong Chen 写道:
>>
>>
>> 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/
>>
> Hi Ridong,
> 
> Since I've dropped my patch, I haven't added the reproducer to the selftests separately. If it's convenient, could you also include it in test_cpuset_prs.sh in v4?
> 
Hi Guopeng,

Thanks for the suggestion. I looked into it, but the reproducer doesn't
fit into the existing test matrix for now.

To trigger the leak, the root partition has to take all the CPUs of the
top cpuset except one, and that last CPU is then offlined. So the CPU
list depends on how many CPUs the machine has. The test matrix only
uses fixed CPU lists within CPUs 0-7, and on a machine with more CPUs
the top cpuset keeps the others, so offlining one CPU doesn't
invalidate the partition.

It would need a separate test function outside the matrix. The
selftest is already large and getting hard to maintain, so I'd rather
not add a special case for it. Instead, I put the reproducer in the
commit message of patch 1 in v4, so it can still be used to verify the
fix.

-- 
Best regards
Ridong