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

[Reinette Chatre <reinette.chatre@intel.com>]

Hi James,

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.

>>> +
>>> +	/* If the CPU picked isn't marked nohz_full, we're done */
>>
>> Please don't impersonate code.
>>
>>> +	if (cpu <= nr_cpu_ids && !tick_nohz_full_cpu(cpu))
>>>  		return cpu;
>>
>> Is this intended to be "cpu < nr_cpu_ids"?
> 
> Yes, fixed - thanks!
> 
> 
>> 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.

> An error is returned if the request couldn't be satisfied, i.e. an empty mask was passed,
> or the only CPU set in the mask was excluded.
> There is a second attempt in this case for a housekeeping CPU - but that will fail too.
> As above, this only happens when CPUs are going offline, and this error is handled by the
> caller.

Reinette