Re: [PATCH-next v3 3/9] cgroup/cpuset: Consider all the exclusive CPUs when doing housekeeping check

[Waiman Long <longman@redhat.com>]

On 10/15/26 10:18 PM, Guopeng Zhang wrote:
>
> 在 2026/10/14 22:05, Waiman Long 写道:
>> When prstate_housekeeping_conflict() is called to perform housekeeping
>> check with cpumask changes, the cpumask changes are passed in via addmask
>> and delmask. With this arrangement, the exclusive CPUs owned by the child
>> partitions and not affected by the cpumask change, if any, are excluded
>> in the analysis. If the current cpuset is an isolated partition with
>> root partition children, it is possible the cpumask change will still
>> leave active housekeeping CPUs but then no housekeeping CPUs will be
>> left when the child partitions are disabled or invalidated.
>>
>> As a first step to protect against this possibility, all the exclusive
>> CPUs of the cpuset should be considered to be owned by the current cpuset
>> when doing housekeeping check. As a result, there is no need to pass in
>> separate add_mask and del_mask parameters. They are combined into a single
>> new_cpus parameter. The parent_prs parameter can be removed as well.
>>
>> With this change, the prstate_housekeeping_conflict() call in
>> update_parent_effective_cpumask() with partcmd_update and newmask
>> is now duplicative and can be removed. The housekeeping check in
>> remote_cpus_update(), however, will still be useful as this function can
>> be called directly from update_cpumasks_hier() without going through
>> partition_cpus_change(). The update_parent_effective_cpumask() call
>> from update_cpumasks_hier() is partcmd_update with no cpumask which
>> still have the housekeeping check and so is covered.
>>
>> Fixes: 103b08709e8a ("cgroup/cpuset: Fail if isolated and nohz_full don't leave any housekeeping")
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>   kernel/cgroup/cpuset.c | 77 ++++++++++++++++--------------------------
>>   1 file changed, 30 insertions(+), 47 deletions(-)
>>
>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>> index 2b9aefc0edb8..b2684c98d8c0 100644
>> --- a/kernel/cgroup/cpuset.c
>> +++ b/kernel/cgroup/cpuset.c
>> @@ -1355,26 +1355,20 @@ static void partition_xcpus_del(int old_prs, struct cpuset *parent,
>>   /*
>>    * 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
>> + * @new_cpus: cpu mask to be checked
>>    * Return: true if there is conflict, false otherwise
>>    *
>>    * 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.
>> + * 1) If new_prs is PRS_ROOT, none of the @new_cpus can be a boot-time isolated
>> + *    CPU. IOW, the whole @new_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.
>> + *    still leave housekeeping CPUs.
>>    */
>> -static bool prstate_housekeeping_conflict(int new_prs, int parent_prs,
>> -					  struct cpumask *add_cpus,
>> -					  struct cpumask *del_cpus)
>> +static bool prstate_housekeeping_conflict(int new_prs, struct cpumask *new_cpus)
>>   {
>>   	cpumask_var_t full_hk_cpus;
>>   	int res;
>>   
>> -	WARN_ON_ONCE(parent_prs <= 0);
>>   	if (!new_prs)
>>   		return false;
>>   
>> @@ -1393,30 +1387,18 @@ static bool prstate_housekeeping_conflict(int new_prs, int parent_prs,
>>   		new_prs = -new_prs;
>>   
>>   	if (housekeeping_enabled(HK_TYPE_DOMAIN_BOOT) && (new_prs == PRS_ROOT) &&
>> -	    !cpumask_subset(add_cpus, housekeeping_cpumask(HK_TYPE_DOMAIN_BOOT)))
>> +	    !cpumask_subset(new_cpus, housekeeping_cpumask(HK_TYPE_DOMAIN_BOOT)))
>>   		return true;
>>   
>>   	/*
>>   	 * Exit 2nd check early and return false if
>>   	 * 1) there is no boot time isolated nohz_full CPU
>> -	 * 2) new_prs is PRS_ROOT, parent_prs is PRS_ISOLATED and no CPU will
>> -	 *    be returned back to the parent.
>> +	 * 2) @new_prs is not PRS_ISOLATED
>> +	 * 3) @new_cpus is a subset of isolated_cpus
>>   	 */
>>   	if (!housekeeping_enabled(HK_TYPE_KERNEL_NOISE) ||
>> -	    (!del_cpus && (new_prs == PRS_ROOT) && (parent_prs == PRS_ISOLATED)))
>> -		return false;
>> -
>> -	/*
>> -	 * Now make sure that @add_cpus contains new CPUs to be isolated and
>> -	 * @del_cpus contains isolated CPUs to be un-isolated.
>> -	 * With the guards above, @add_cpus can't be NULL after the swap.
>> -	 */
>> -	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))
>> +	    (new_prs != PRS_ISOLATED) ||
>> +	    cpumask_subset(new_cpus, isolated_cpus))
>>   		return false;
>>   
>>   	if (!alloc_cpumask_var(&full_hk_cpus, GFP_KERNEL))
>> @@ -1426,7 +1408,7 @@ static bool prstate_housekeeping_conflict(int new_prs, int parent_prs,
>>   		    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);
>> -	res = !cpumask_weight_andnot(full_hk_cpus, add_cpus);
>> +	res = !cpumask_weight_andnot(full_hk_cpus, new_cpus);
>>   
> One of my test cases regressed in v3 compared to v2. CPUs 0-15 are online,
> with nohz_full=2-15 and no isolcpus:
>
>      # cd /sys/fs/cgroup
>      # echo +cpuset > cgroup.subtree_control
>      # mkdir A
>      # echo 0 > A/cpuset.cpus
>      # echo isolated > A/cpuset.cpus.partition
>      # cat cpuset.cpus.isolated
>      0
>      # echo 1 > A/cpuset.cpus
>      # cat A/cpuset.cpus.partition
>      isolated invalid (partition config conflicts with housekeeping setup)
>
> The last write should release CPU 0 for housekeeping, leaving A as a valid
> isolated partition on CPU 1. It looks like the check uses the state before
> the update, sees only CPU 1 available for housekeeping, and incorrectly
> invalidates A.
>
> thanks,
> Guopeng

Yes, it is a known issue and will be fixed in the next version.

Cheers,
Longman

>>   	free_cpumask_var(full_hk_cpus);
>>   	return res;
>> @@ -1613,7 +1595,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 (prstate_housekeeping_conflict(new_prs, PRS_ROOT, tmp->new_cpus, NULL))
>> +	if (prstate_housekeeping_conflict(new_prs, tmp->new_cpus))
>>   		return PERR_HKEEPING;
>>   
>>   	spin_lock_irq(&callback_lock);
>> @@ -1715,8 +1697,7 @@ 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 (prstate_housekeeping_conflict(prs, PRS_ROOT,
>> -						       tmp->addmask, tmp->delmask))
>> +		else if (prstate_housekeeping_conflict(prs, excpus))
>>   			WRITE_ONCE(cs->prs_err, PERR_HKEEPING);
>>   		if (cs->prs_err)
>>   			goto invalidate;
>> @@ -1796,7 +1777,6 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   	int old_prs, new_prs;
>>   	int part_error = PERR_NONE;	/* Partition error? */
>>   	struct cpumask *xcpus = user_xcpus(cs);
>> -	int parent_prs = parent->partition_root_state;
>>   	bool nocpu;
>>   
>>   	lockdep_assert_cpuset_lock_held();
>> @@ -1858,7 +1838,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, parent_prs, xcpus, NULL))
>> +		if (prstate_housekeeping_conflict(new_prs, xcpus))
>>   			return PERR_HKEEPING;
>>   
>>   		if (tasks_nocpu_error(parent, cs, xcpus))
>> @@ -1930,14 +1910,6 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   					       parent->effective_xcpus);
>>   		}
>>   
>> -		/*
>> -		 * Check for housekeeping conflicts
>> -		 */
>> -		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
>>   		 * must be present.
>> @@ -2430,6 +2402,21 @@ static void partition_cpus_change(struct cpuset *cs, struct cpuset *trialcs,
>>   		return;
>>   
>>   	prs_err = validate_partition(cs, trialcs);
>> +	if (!prs_err && !cpumask_empty(trialcs->effective_xcpus)) {
>> +		int parent_prs = is_remote_partition(cs)
>> +			       ? PRS_ROOT : parent_cs(cs)->partition_root_state;
>> +		/*
>> +		 * Check for housekeeping CPUs conflict assuming that all
>> +		 * the exclusive CPUs belong to the current cpuset. We rely
>> +		 * on the fact that effective_xcpus has been set up by
>> +		 * compute_trial_excpus() which is called previously in
>> +		 * update_cpumask() and update_exclusive_cpumask().
>> +		 */
>> +		if (parent_prs > 0 &&
>> +		    prstate_housekeeping_conflict(trialcs->partition_root_state,
>> +						  trialcs->effective_xcpus))
>> +			prs_err = PERR_HKEEPING;
>> +	}
>>   	if (prs_err) {
>>   		WRITE_ONCE(cs->prs_err, prs_err);
>>   		trialcs->prs_err = prs_err;
>> @@ -2922,15 +2909,11 @@ 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 (prstate_housekeeping_conflict(new_prs, parent_prs,
>> -						  cs->effective_xcpus, NULL))
>> +		if (prstate_housekeeping_conflict(new_prs, cs->effective_xcpus))
>>   			err = PERR_HKEEPING;
>>   		else
>>   			isolcpus_updated = true;