[PATCH] sched/core: Bypass arch_scale_freq_tick() on nohz_full CPUs

[PATCH] sched/core: Bypass arch_scale_freq_tick() on nohz_full CPUs

[Nicolas Saenz Julienne]

arch_scale_freq_tick() calculations generally track the increment of
frequency performance counters in between scheduler ticks to provide
estimations on how DVFS swayed frequency on that CPU. This information
eventually allows for more precise scheduling decisions. It's all fine
and good, but nohz_full CPUs are capable of disabling the tick
indefinitely and potentially trigger overflows in
arch_scale_freq_tick()'s calculations once it's eventually re-enabled.

This will happen for both users of this interface: x86 and arm64. And
it's also relevant that the heuristic on what to do in case of
operations overflowing varies depending on the implementation. It goes
from fully disabling frequency invariance scaling on all CPUS, to
ignoring this is a possibility.

It's arguable that nohz_full CPUs are unlikely to benefit much from this
feature, since their aim is to allow for uninterrupted execution of a
single task, effectively getting the scheduler out of the way. Also,
DVFS itself is also unlikely be used on most nohz_full systems, given
its effects on latency.

So get around this by not calling arch_scale_freq_tick() on nohz_full
CPUs.

Note that tick_nohz_full_cpu() relies on a static branch, which avoids
degrading performance on the rest of systems.

Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
---
 kernel/sched/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2fd623b2270d..8c04ec0e073a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5016,7 +5016,13 @@ void scheduler_tick(void)
 	unsigned long thermal_pressure;
 	u64 resched_latency;
 
-	arch_scale_freq_tick();
+	/*
+	 * nohz_full CPUs are capable of disabling the scheduler tick
+	 * indefinitely, potentially overflowing arch_scale_freq_tick()
+	 * calculations once it's re-enabled.
+	 */
+	if (!tick_nohz_full_cpu(smp_processor_id()))
+		arch_scale_freq_tick();
 	sched_clock_tick();
 
 	rq_lock(rq, &rf);
-- 
2.31.1

Re: [PATCH] sched/core: Bypass arch_scale_freq_tick() on nohz_full CPUs

[Peter Zijlstra]

On Fri, Aug 27, 2021 at 08:49:10PM +0200, Nicolas Saenz Julienne wrote:
> arch_scale_freq_tick() calculations generally track the increment of
> frequency performance counters in between scheduler ticks to provide
> estimations on how DVFS swayed frequency on that CPU. This information
> eventually allows for more precise scheduling decisions. It's all fine
> and good, but nohz_full CPUs are capable of disabling the tick
> indefinitely and potentially trigger overflows in
> arch_scale_freq_tick()'s calculations once it's eventually re-enabled.
> 
> This will happen for both users of this interface: x86 and arm64. And
> it's also relevant that the heuristic on what to do in case of
> operations overflowing varies depending on the implementation. It goes
> from fully disabling frequency invariance scaling on all CPUS, to
> ignoring this is a possibility.
> 
> It's arguable that nohz_full CPUs are unlikely to benefit much from this
> feature, since their aim is to allow for uninterrupted execution of a
> single task, effectively getting the scheduler out of the way. Also,
> DVFS itself is also unlikely be used on most nohz_full systems, given
> its effects on latency.
> 
> So get around this by not calling arch_scale_freq_tick() on nohz_full
> CPUs.
> 
> Note that tick_nohz_full_cpu() relies on a static branch, which avoids
> degrading performance on the rest of systems.
> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
> ---
>  kernel/sched/core.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 2fd623b2270d..8c04ec0e073a 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5016,7 +5016,13 @@ void scheduler_tick(void)
>  	unsigned long thermal_pressure;
>  	u64 resched_latency;
>  
> -	arch_scale_freq_tick();
> +	/*
> +	 * nohz_full CPUs are capable of disabling the scheduler tick
> +	 * indefinitely, potentially overflowing arch_scale_freq_tick()
> +	 * calculations once it's re-enabled.
> +	 */
> +	if (!tick_nohz_full_cpu(smp_processor_id()))
> +		arch_scale_freq_tick();
>  	sched_clock_tick();
>  
>  	rq_lock(rq, &rf);

Hurmph,.. I'm not too happy with this.. Fundamentally the whole
NOHZ_FULL state should be dynamic, it currently isn't but that's
arguably a bug of the current implementation.

As such the above doesn't really dtrt, since a CPU disabling NOHZ_FULL
will then still suffer all them overflows you mentioned.

Frederic, how should this be done right? Is there a place where upon
entering/exiting NOHZ_FULL we can do fixups?