Re: [PATCH v3 3/4] cpuset: settle isolation state when CPUs are given back to parent
From: Waiman Long
Date: Mon Nov 09 2026 - 22:35:37 EST
On 11/9/26 10:14 PM, Ridong Chen wrote:
Yes, that looks good to me.
On 11/9/2026 10:51 PM, Waiman Long wrote:
On 11/7/26 7:00 AM, Ridong Chen wrote:Hi Longman,
From: Ridong Chen <chenridong@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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.
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@xxxxxxxxxx>
Closes: https://lore.kernel.org/all/20261105130344.664336-4-guopeng.zhang@xxxxxxxxx/
Does that look OK to you?
[1] https://lore.kernel.org/all/20261105130344.664336-4-guopeng.zhang@xxxxxxxxx/
Thanks,
Longman