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

[Reinette Chatre]

Hi James,

On 4/27/2023 7:10 AM, James Morse wrote:
> Hi Reinette,
> 
> On 01/04/2023 00:24, Reinette Chatre wrote:
>> On 3/20/2023 10:26 AM, James Morse wrote:
>>> The limbo and overflow code picks a CPU to use from the domain's list
>>> of online CPUs. Work is then scheduled on these CPUs to maintain
>>> the limbo list and any counters that may overflow.
>>>
>>> cpumask_any() may pick a CPU that is marked nohz_full, which will
>>> either penalise the work that CPU was dedicated to, or delay the
>>
>> penalise -> penalize
> 
> (s->z is the difference between British English and American English)

My apologies.

>>> processing of limbo list or counters that may overflow. Perhaps
>>> indefinitely. Delaying the overflow handling will skew the bandwidth
>>> values calculated by mba_sc, which expects to be called once a second.
>>>
>>> Add cpumask_any_housekeeping() as a replacement for cpumask_any()
>>> that prefers housekeeping CPUs. This helper will still return
>>> a nohz_full CPU if that is the only option. The CPU to use is
>>> re-evaluated each time the limbo/overflow work runs. This ensures
>>> the work will move off a nohz_full CPU once a houskeeping CPU is
>>> available.
> 
>>> 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.

>> Apart from the issue that Ilpo pointed out I would prefer that any changes outside
>> resctrl are submitted separately to that subsystem.
> 
> Sure, I'll pull those three lines out as a separate patch.
> 
> 
>>> @@ -801,6 +803,11 @@ void mbm_handle_overflow(struct work_struct *work)
>>>  			update_mba_bw(prgrp, d);
>>>  	}
>>>  
>>> +	/*
>>> +	 * Re-check for housekeeping CPUs. This allows the overflow handler to
>>> +	 * move off a nohz_full CPU quickly.
>>> +	 */
>>> +	cpu = cpumask_any_housekeeping(&d->cpu_mask);
>>>  	schedule_delayed_work_on(cpu, &d->mbm_over, delay);
>>>  
>>>  out_unlock:
>>
>> From what I can tell the nohz_full CPUs are set during boot and do not change.
> 
> But the house keeping CPUs can be taken offline, and brought back.
> 
> With this change the work moves off the nohz_full CPU and back to the housekeeping CPU the
> next time this runs. Without it, you're stuck on a nohz_full CPU until you take that CPU
> offline too.

Good point, thanks.

>>> 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.

Reinette