Re: [PATCH v7 21/24] x86/resctrl: Allow overflow/limbo handlers to be scheduled on any-but cpu

[James Morse <james.morse@arm.com>]

Hi Reinette,

On 12/14/23 18:53, Reinette Chatre wrote:
> On 12/14/2023 3:38 AM, James Morse wrote:
>> On 09/11/2023 17:48, Reinette Chatre wrote:
>>> On 10/25/2023 11:03 AM, James Morse wrote:

>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> index c4c1e1909058..f5fff2f0d866 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -61,19 +61,36 @@
>>>>    * cpumask_any_housekeeping() - Choose any CPU in @mask, preferring those that
>>>>    *			        aren't marked nohz_full
>>>>    * @mask:	The mask to pick a CPU from.
>>>> + * @exclude_cpu:The CPU to avoid picking.
>>>>    *
>>>> - * Returns a CPU in @mask. If there are housekeeping CPUs that don't use
>>>> - * nohz_full, these are preferred.
>>>> + * Returns a CPU from @mask, but not @exclude_cpu. If there are housekeeping
>>>> + * CPUs that don't use nohz_full, these are preferred. Pass
>>>> + * RESCTRL_PICK_ANY_CPU to avoid excluding any CPUs.
>>>> + *
>>>> + * When a CPU is excluded, returns >= nr_cpu_ids if no CPUs are available.
>>>>    */
>>>> -static inline unsigned int cpumask_any_housekeeping(const struct cpumask *mask)
>>>> +static inline unsigned int
>>>> +cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
>>>>   {
>>>>   	unsigned int cpu, hk_cpu;
>>>>   
>>>> -	cpu = cpumask_any(mask);
>>>> -	if (!tick_nohz_full_cpu(cpu))
>>>> +	if (exclude_cpu == RESCTRL_PICK_ANY_CPU)
>>>> +		cpu = cpumask_any(mask);
>>>> +	else
>>>> +		cpu = cpumask_any_but(mask, exclude_cpu);
>>>> +
>>>> +	if (!IS_ENABLED(CONFIG_NO_HZ_FULL))
>>>> +		return cpu;
>>>
>>> It is not clear to me how cpumask_any_but() failure is handled.
>>>
>>> cpumask_any_but() returns ">= nr_cpu_ids if no cpus set" ...
>>
>> It wasn't a satisfiable request, there are no CPUs for this domain other than the one that
>> was excluded. cpumask_any_but() also describes its errors as "returns >= nr_cpu_ids if no
>> CPUs are available".
>>
>> The places this can happen in resctrl are:
>> cqm_setup_limbo_handler(), where it causes the schedule_delayed_work_on() call to be skipped.
>> mbm_setup_overflow_handler(), which does similar.
>>
>> These two cases are triggered from resctrl_offline_cpu() when the last CPU in a domain is
>> going offline, and the domain is about to be free()d. This is how the limbo/overflow
>> 'threads' stop.

> Right ... yet this is a generic function, if there are any requirements on when/how it should
> be called then it needs to be specified in the function comments. I do not expect this to
> be the case for this function.

There are no special requirements, like all the other cpumask_foo() helpers, you can feed it
an empty bitmap and it will return '>= nr_cpu_ids' as an error.

[...]

>>> But that would have
>>> code continue ... so maybe it needs explicit error check of
>>> cpumask_any_but() failure with an earlier exit?
>>
>> I'm not sure what the problem you refer to here is.
>> If 'cpu' is valid, and not marked nohz_full, nothing more needs doing.
>> If 'cpu' is invalid or a CPU marked nohz_full, then a second attempt is made to find a
>> housekeeping CPU into 'hk_cpu'. If the second attempt is valid, it's used in preference.
> 
> Considering that the second attempt can only be on the same or smaller set of CPUs,
> how could the second attempt ever succeed if the first attempt failed? I do not see
> why it is worth continuing.

Its harmless, its not on a performance sensitive path and it would take extra logic in the
more common cases to detect this and return early.


Thanks,

James