On 2026/1/5 5:25, Waiman Long wrote:
> On 1/3/26 9:48 PM, Chen Ridong wrote:
>>
>> On 2026/1/2 3:15, Waiman Long wrote:
>>> Since commit f62a5d39368e ("cgroup/cpuset: Remove remote_partition_check()
>>> & make update_cpumasks_hier() handle remote partition"), the
>>> compute_effective_exclusive_cpumask() helper was extended to
>>> strip exclusive CPUs from siblings when computing effective_xcpus
>>> (cpuset.cpus.exclusive.effective). This helper was later renamed to
>>> compute_excpus() in commit 86bbbd1f33ab ("cpuset: Refactor exclusive
>>> CPU mask computation logic").
>>>
>>> This helper is supposed to be used consistently to compute
>>> effective_xcpus. However, there is an exception within the callback
>>> critical section in update_cpumasks_hier() when exclusive_cpus of a
>>> valid partition root is empty. This can cause effective_xcpus value to
>>> differ depending on where exactly it is last computed. Fix this by using
>>> compute_excpus() in this case to give a consistent result.
>>>
>>> Signed-off-by: Waiman Long <[email protected]>
>>> ---
>>> kernel/cgroup/cpuset.c | 14 +++++---------
>>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>>> index da2b3b51630e..37d118a9ad4d 100644
>>> --- a/kernel/cgroup/cpuset.c
>>> +++ b/kernel/cgroup/cpuset.c
>>> @@ -2168,17 +2168,13 @@ static void update_cpumasks_hier(struct cpuset *cs,
>>> struct tmpmasks *tmp,
>>> spin_lock_irq(&callback_lock);
>>> cpumask_copy(cp->effective_cpus, tmp->new_cpus);
>>> cp->partition_root_state = new_prs;
>>> - if (!cpumask_empty(cp->exclusive_cpus) && (cp != cs))
>>> - compute_excpus(cp, cp->effective_xcpus);
>>> -
>>> /*
>>> - * Make sure effective_xcpus is properly set for a valid
>>> - * partition root.
>>> + * Need to compute effective_xcpus if either exclusive_cpus
>>> + * is non-empty or it is a valid partition root.
>>> */
>>> - if ((new_prs > 0) && cpumask_empty(cp->exclusive_cpus))
>>> - cpumask_and(cp->effective_xcpus,
>>> - cp->cpus_allowed, parent->effective_xcpus);
>>> - else if (new_prs < 0)
>>> + if ((new_prs > 0) || !cpumask_empty(cp->exclusive_cpus))
>>> + compute_excpus(cp, cp->effective_xcpus);
>>> + if (new_prs < 0)
>>> reset_partition_data(cp);
>>> spin_unlock_irq(&callback_lock);
>>>
>> The code resets partition data only for new_prs < 0. My understanding is
>> that a partition is invalid
>> when new_prs <= 0. Shouldn't reset_partition_data() also be called when
>> new_prs = 0? Is there a
>> specific reason to skip the reset in that case?
>
> update_cpumasks_hier() is called when changes in a cpuset or hotplug affects
> other cpusets in the
> hierarchy. With respect to changes in partition state, it is either from
> valid to invalid or vice
> versa. It will not change from a valid partition to member. The only way
> new_prs = 0 is when old_prs
> = 0. Even if the affected cpuset is processed again in update_cpumask_hier(),
> any state change from
> valid partition to member (update_prstate()), reset_partition_data() should
> have been called there.
> That is why we only care about when new_prs != 0.
>
Thank you for your patience.
> The code isn't wrong here. However I can change the condition to (new_prs <=
> 0) if it makes it
> easier to understand.
>
I agree there's nothing wrong with the current logic. However, for clarity, I
suggest changing the
condition to (new_prs <= 0). This allows the function's logic to be fully
self-consistent and
focused on a single responsibility. This approach would allow us to simplify
the code to:
if (new_prs > 0)
compute_excpus(cp, cp->effective_xcpus);
else
reset_partition_data(cp);
Since reset_partition_data() already handles cases whether cp->exclusive_cpus
is empty or not, this
implementation would be more concise while correctly covering all scenarios.
--
Best regards,
Ridong
