On 2026/1/5 11:50, Waiman Long wrote:
> On 1/4/26 8:15 PM, Chen Ridong wrote:
>>
>> 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.
>
> effective_xcpus should be set when exclusive_cpus is not empty or when the
> cpuset is a valid
> partition root. So just checking new_prs for compute_excpus() is not enough.
>
If we change the condition to (new_prs <= 0), it will reset the partition data
even when we call
compute_excpus (for !cpumask_empty(cp->exclusive_cpus)), so we should still get
the same result, right?
--
Best regards,
Ridong
