Re: [PATCH v3 08/19] x86/resctrl: Add cpumask_any_housekeeping() for limbo/overflow

[James Morse]

Hi Reinette,

On 28/04/2023 00:36, Reinette Chatre wrote:
> On 4/27/2023 7:10 AM, James Morse wrote:
>> On 01/04/2023 00:24, Reinette Chatre wrote:
>>> On 3/20/2023 10:26 AM, James Morse wrote:

>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> index 87545e4beb70..0b5fd5a0cda2 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -55,6 +56,28 @@
>>>>  /* Max event bits supported */
>>>>  #define MAX_EVT_CONFIG_BITS		GENMASK(6, 0)
>>>>  
>>>> +/**
>>>> + * cpumask_any_housekeeping() - Chose any cpu in @mask, preferring those that
>>>> + *			        aren't marked nohz_full
>>>> + * @mask:	The mask to pick a CPU from.
>>>> + *
>>>> + * Returns a CPU in @mask. If there are houskeeping CPUs that don't use
>>>> + * nohz_full, these are preferred.
>>>> + */
>>>> +static inline unsigned int cpumask_any_housekeeping(const struct cpumask *mask)
>>>> +{
>>>> +	int cpu, hk_cpu;
>>>> +
>>>> +	cpu = cpumask_any(mask);
>>>> +	if (tick_nohz_full_cpu(cpu)) {
>>>> +		hk_cpu = cpumask_nth_andnot(0, mask, tick_nohz_full_mask);
>>>> +		if (hk_cpu < nr_cpu_ids)
>>>> +			cpu = hk_cpu;
>>>> +	}
>>>> +
>>
>>> I think as a start this could perhaps be a #if defined(CONFIG_NO_HZ_FULL). There
>>> appears to be a precedent for this in kernel/rcu/tree_nocb.h.
>>
>> This harms readability, and prevents the compiler from testing that this is valid C code
>> for any compile of this code.
>>
>> With if-def's here you'd be reliant on come CI system to build with the required
>> combination of Kconfig symbols to expose any warnings.
>>
>> It's much better to use IS_ENABLED() in the helpers and rely on the compiler's
>> dead-code-elimination to remove paths that have been configured out.
>>
>> (See the section on Conditional Compilation in coding-style for a much better summary!)
> 
> My assumption was that you intended to implement what is described first in
> the document you point to. That is, providing no-stub versions for all
> and then calling everything unconditionally. Since I did not see universal stubs
> for the code you are using I was looking at how other areas in the kernel handled
> the same. 
> 
> Reading your response to Ilpo and what you write later I now see that you are using
> a combination of no-op stubs and conditional compilation. That is, you use a no-op stub,
> instead of "IS_ENABLED()" or "#if" to conditionally compile some code. I am not familiar
> with how compilers handle these scenarios.
> 

>>>> diff --git a/include/linux/tick.h b/include/linux/tick.h
>>>> index bfd571f18cfd..ae2e9019fc18 100644
>>>> --- a/include/linux/tick.h
>>>> +++ b/include/linux/tick.h
>>>> @@ -174,9 +174,10 @@ static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; }
>>>>  static inline void tick_nohz_idle_stop_tick_protected(void) { }
>>>>  #endif /* !CONFIG_NO_HZ_COMMON */
>>>>  
>>>> +extern cpumask_var_t tick_nohz_full_mask;
>>>> +
>>>>  #ifdef CONFIG_NO_HZ_FULL
>>>>  extern bool tick_nohz_full_running;
>>>> -extern cpumask_var_t tick_nohz_full_mask;
>>>>  
>>>>  static inline bool tick_nohz_full_enabled(void)
>>>>  {
>>>
>>> In addition to what Ilpo pointed out, be careful here.
>>> cpumask_var_t is a pointer (or array) and needs to be
>>> allocated before use. Moving its declaration but not the
>>> allocation code seems risky.
>>
>> Risky how? Any use of tick_nohz_full_mask that isn't guarded by something like
>> tick_nohz_full_cpu() will lead to a link error regardless of the type.
> 
> I assumed that the intention was to create an actual "no-op" stub for this
> mask, enabling it to be used unconditionally. That the intention is for it
> to be guarded and how the compiler deals with this was not obvious to me. I think
> it would be good to call out this usage when submitting this to the appropriate
> maintainers. A comment near the declaration may help users to know how it is
> intended to be used.

Right, I'll add a comment:
/*
 * Mask of CPUs that are nohz_full.
 *
 * Users should be guarded by CONFIG_NO_HZ_FULL or a tick_nohz_full_cpu()
 * check.
 */




Thanks,

James