Hi，Longman,
在 2026/10/11 06:19, Waiman Long 写道:
> The isolated_cpus_can_update() and prstate_housekeeping_conflict() are
> checking different aspects of upcoming cpumask changes that may conflict
> with the current setting of the housekeeping cpumasks. There are places
> where both are called together. There are also places where only one
> of them is called. That inconsistency can contribute to missing check
> where invalid cpumask changes may be allowed to move forward.
>
> Fix that by consolidating isolated_cpus_can_update() into
> prstate_housekeeping_conflict() and call prstate_housekeeping_conflict()
> in all the places where either one of them or both are called. The
> exception is the validate_partition() function where the
> prstate_housekeeping_conflict() call is removed. It is because
> validate_partition() is called only from partition_cpus_change()
> where prstate_housekeeping_conflict() will be called from either
> remote_cpus_update() or update_parent_effective_cpumask() with
> partcmd_update if not for partition invalidatation or disablement.
>
> Now prstate_housekeeping_conflict() will be called in the following
> locations:
> - when a partition is enabled in remote_partition_enable() or in
> update_parent_effective_cpumask() with partcmd_enable*.
> - when a cpumask is updated in remote_cpus_update() or in
> update_parent_effective_cpumask() with partcmd_update.
> - when a partition state changes from root to isolated or vice versa
> in update_prstate().
>
> Fixes: 4a74e418881f ("cgroup/cpuset: Check partition conflict with
> housekeeping setup")
> Fixes: 103b08709e8a ("cgroup/cpuset: Fail if isolated and nohz_full don't
> leave any housekeeping")
> Signed-off-by: Waiman Long <[email protected]>
> ---
> kernel/cgroup/cpuset.c | 123 +++++++++++++++++------------------------
> 1 file changed, 52 insertions(+), 71 deletions(-)
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 0ebba2d646c3..f3cebb277a68 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -1353,65 +1353,62 @@ static void partition_xcpus_del(int old_prs, struct
> cpuset *parent,
> }
>
> /*
> - * isolated_cpus_can_update - check for isolated & nohz_full conflicts
> - * @add_cpus: cpu mask for cpus that are going to be isolated
> - * @del_cpus: cpu mask for cpus that are no longer isolated, can be NULL
> - * Return: false if there is conflict, true otherwise
> - *
> - * If nohz_full is enabled and we have isolated CPUs, their combination must
> - * still leave housekeeping CPUs.
> + * prstate_housekeeping_conflict - check for partition & housekeeping
> conflicts
> + * @new_prs: new partition root state to be checked
> + * @parent_prs: Parent partition root state
> + * @add_cpus: additional CPUs to be added to current cpuset
> + * @del_cpus: CPUs to be removed from current cpuset, can be NULL
> + * Return: true if there is conflict, false otherwise
> *
> - * TBD: Should consider merging this function into
> - * prstate_housekeeping_conflict().
> + * There are two different housekeeping conflicts to be checked:
> + * 1) If new_prs is PRS_ROOT, none of the @add_cpus can be a boot-time
> isolated
> + * CPU. IOW, the whole @add_cpus must be a subset of HK_TYPE_DOMAIN_BOOT.
> + * 2) If nohz_full is enabled and we have isolated CPUs, their combination
> must
> + * still leave housekeeping CPUs. This check is only needed if new_prs
> + * differs from parent_prs and one of them is PRS_ISOLATED.
> */
> -static bool isolated_cpus_can_update(struct cpumask *add_cpus,
> - struct cpumask *del_cpus)
> +static bool prstate_housekeeping_conflict(int new_prs, int parent_prs,
> + struct cpumask *add_cpus,
> + struct cpumask *del_cpus)
> {
> cpumask_var_t full_hk_cpus;
> - int res = true;
> + int res;
>
> - if (!housekeeping_enabled(HK_TYPE_KERNEL_NOISE))
> + if (housekeeping_enabled(HK_TYPE_DOMAIN_BOOT) && (new_prs == PRS_ROOT)
> &&
> + !cpumask_subset(add_cpus,
> housekeeping_cpumask(HK_TYPE_DOMAIN_BOOT)))
> return true;
>
> - if (del_cpus && cpumask_weight_and(del_cpus,
> - housekeeping_cpumask(HK_TYPE_KERNEL_NOISE)))
> - return true;
> + if (!housekeeping_enabled(HK_TYPE_KERNEL_NOISE) ||
> + (new_prs == parent_prs) ||
> + (!del_cpus && (new_prs != PRS_ISOLATED)) ||
> + ((new_prs != PRS_ISOLATED) && (parent_prs != PRS_ISOLATED)))
> + return false;
>
When a child partition switches from root to isolated under an isolated
parent, this returns false even though the child's CPUs were not isolated
before the switch.
On a fresh boot with CPUs 0-15 online and nohz_full=2-15:
# cd /sys/fs/cgroup
# echo +cpuset > cgroup.subtree_control
# mkdir A B
# echo 1-2 > A/cpuset.cpus
# echo isolated > A/cpuset.cpus.partition
# echo +cpuset > A/cgroup.subtree_control
# mkdir A/C
# echo 1 > A/C/cpuset.cpus
# echo root > A/C/cpuset.cpus.partition
# echo 0 > B/cpuset.cpus
# echo isolated > B/cpuset.cpus.partition
# cat cpuset.cpus.isolated
0,2
# echo isolated > A/C/cpuset.cpus.partition
# cat A/C/cpuset.cpus.partition
isolated
# cat cpuset.cpus.isolated
0-2
CPU 1 was the last remaining housekeeping CPU: it was in neither the
nohz_full mask nor the cpuset.cpus.isolated mask. The final write isolates
CPU 1 as well, leaving no housekeeping CPU.
Should this check also take the child's previous partition state into
account, rather than just comparing its new state with the parent's state?
Thanks,
Guopeng
> - if (!alloc_cpumask_var(&full_hk_cpus, GFP_KERNEL))
> + /*
> + * Make sure that @add_cpus contains new CPUs to be isolated and
> + * @del_cpus contains isolated CPUs to be un-isolated.
> + */
> + if (parent_prs == PRS_ISOLATED)
> + swap(add_cpus, del_cpus);
> +
> + if (del_cpus &&
> + (cpumask_first_and_and(del_cpus,
> housekeeping_cpumask(HK_TYPE_KERNEL_NOISE),
> + cpu_active_mask) < nr_cpu_ids))
> return false;
>
> + if (!alloc_cpumask_var(&full_hk_cpus, GFP_KERNEL))
> + return true;
> +
> cpumask_and(full_hk_cpus, housekeeping_cpumask(HK_TYPE_KERNEL_NOISE),
> housekeeping_cpumask(HK_TYPE_DOMAIN));
> cpumask_andnot(full_hk_cpus, full_hk_cpus, isolated_cpus);
> cpumask_and(full_hk_cpus, full_hk_cpus, cpu_active_mask);
> - if (!cpumask_weight_andnot(full_hk_cpus, add_cpus))
> - res = false;
> + res = !cpumask_weight_andnot(full_hk_cpus, add_cpus);
>
> free_cpumask_var(full_hk_cpus);
> return res;
> }
>
> -/*
> - * prstate_housekeeping_conflict - check for partition & housekeeping
> conflicts
> - * @prstate: partition root state to be checked
> - * @new_cpus: cpu mask
> - * Return: true if there is conflict, false otherwise
> - *
> - * CPUs outside of HK_TYPE_DOMAIN_BOOT, if defined, can only be used in an
> - * isolated partition.
> - */
> -static bool prstate_housekeeping_conflict(int prstate, struct cpumask
> *new_cpus)
> -{
> - if (!housekeeping_enabled(HK_TYPE_DOMAIN_BOOT))
> - return false;
> -
> - if ((prstate != PRS_ISOLATED) &&
> - !cpumask_subset(new_cpus,
> housekeeping_cpumask(HK_TYPE_DOMAIN_BOOT)))
> - return true;
> -
> - return false;
> -}
> -
> /*
> * cpuset_update_sd_hk_unlock - Rebuild sched domains, update HK & unlock
> *
> @@ -1593,9 +1590,7 @@ static int remote_partition_enable(struct cpuset *cs,
> int new_prs,
> return PERR_INVCPUS;
> if (cpumask_intersects(tmp->new_cpus, subpartitions_cpus))
> return PERR_NOCPUS;
> - if (((new_prs == PRS_ISOLATED) &&
> - !isolated_cpus_can_update(tmp->new_cpus, NULL)) ||
> - prstate_housekeeping_conflict(new_prs, tmp->new_cpus))
> + if (prstate_housekeeping_conflict(new_prs, PRS_ROOT, tmp->new_cpus,
> NULL))
> return PERR_HKEEPING;
>
> spin_lock_irq(&callback_lock);
> @@ -1697,8 +1692,8 @@ static void remote_cpus_update(struct cpuset *cs,
> struct cpumask *xcpus,
> else if (cpumask_intersects(tmp->addmask, subpartitions_cpus) ||
> cpumask_subset(top_cpuset.effective_cpus,
> tmp->addmask))
> WRITE_ONCE(cs->prs_err, PERR_NOCPUS);
> - else if ((prs == PRS_ISOLATED) &&
> - !isolated_cpus_can_update(tmp->addmask, tmp->delmask))
> + else if (prstate_housekeeping_conflict(prs, PRS_ROOT,
> + tmp->addmask,
> tmp->delmask))
> WRITE_ONCE(cs->prs_err, PERR_HKEEPING);
> if (cs->prs_err)
> goto invalidate;
> @@ -1840,11 +1835,7 @@ static int update_parent_effective_cpumask(struct
> cpuset *cs, int cmd,
> if (cpumask_empty(xcpus))
> return PERR_INVCPUS;
>
> - if (prstate_housekeeping_conflict(new_prs, xcpus))
> - return PERR_HKEEPING;
> -
> - if ((new_prs == PRS_ISOLATED) && (new_prs != parent_prs) &&
> - !isolated_cpus_can_update(xcpus, NULL))
> + if (prstate_housekeeping_conflict(new_prs, parent_prs, xcpus,
> NULL))
> return PERR_HKEEPING;
>
> if (tasks_nocpu_error(parent, cs, xcpus))
> @@ -1917,20 +1908,12 @@ static int update_parent_effective_cpumask(struct
> cpuset *cs, int cmd,
> }
>
> /*
> - * TBD: Invalidate a currently valid child root partition may
> - * still break isolated_cpus_can_update() rule if parent is an
> - * isolated partition.
> + * Check for housekeeping conflicts
> */
> - if (is_partition_valid(cs) && (old_prs != parent_prs)) {
> - if ((parent_prs == PRS_ROOT) &&
> - /* Adding to parent means removing isolated CPUs */
> - !isolated_cpus_can_update(tmp->delmask,
> tmp->addmask))
> - part_error = PERR_HKEEPING;
> - if ((parent_prs == PRS_ISOLATED) &&
> - /* Adding to parent means adding isolated CPUs */
> - !isolated_cpus_can_update(tmp->addmask,
> tmp->delmask))
> - part_error = PERR_HKEEPING;
> - }
> + if (is_partition_valid(cs) &&
> + prstate_housekeeping_conflict(old_prs, parent_prs,
> + tmp->delmask, tmp->addmask))
> + part_error = PERR_HKEEPING;
>
> /*
> * The new CPUs to be removed from parent's effective CPUs
> @@ -2391,10 +2374,6 @@ static enum prs_errcode validate_partition(struct
> cpuset *cs, struct cpuset *tri
> if (cpumask_empty(trialcs->effective_xcpus))
> return PERR_INVCPUS;
>
> - if (prstate_housekeeping_conflict(trialcs->partition_root_state,
> - trialcs->effective_xcpus))
> - return PERR_HKEEPING;
> -
> if (tasks_nocpu_error(parent, cs, trialcs->effective_xcpus))
> return PERR_NOCPUS;
>
> @@ -2411,7 +2390,7 @@ static enum prs_errcode validate_partition(struct
> cpuset *cs, struct cpuset *tri
> * CPU modifications may cause a partition to be disabled or require state
> updates.
> */
> static void partition_cpus_change(struct cpuset *cs, struct cpuset *trialcs,
> - struct tmpmasks *tmp)
> + struct tmpmasks *tmp)
> {
> enum prs_errcode prs_err;
>
> @@ -2911,13 +2890,15 @@ static int update_prstate(struct cpuset *cs, int
> new_prs)
> err = remote_partition_enable(cs, new_prs, &tmpmask);
> }
> } else if (old_prs && new_prs) {
> + int parent_prs = is_remote_partition(cs)
> + ? PRS_ROOT : parent->partition_root_state;
> +
> /*
> * A change in load balance state only, no change in cpumasks.
> * Need to update isolated_cpus.
> */
> - if (((new_prs == PRS_ISOLATED) &&
> - !isolated_cpus_can_update(cs->effective_xcpus, NULL)) ||
> - prstate_housekeeping_conflict(new_prs, cs->effective_xcpus))
> + if (prstate_housekeeping_conflict(new_prs, parent_prs,
> + cs->effective_xcpus, NULL))
> err = PERR_HKEEPING;
> else
> isolcpus_updated = true;
