lkml.org 
[lkml]   [2022]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86/aperfmperf: Fix arch_scale_freq_tick() on tickless systems
On Thu, Aug 04, 2022 at 04:17:28PM +0300, Yair Podemsky wrote:
> In order for the scheduler to be frequency invariant we measure the
> ratio between the maximum cpu frequency and the actual cpu frequency.
> During long tickless periods of time the calculations that keep track
> of that might overflow, in the function scale_freq_tick():
>
> if (check_shl_overflow(acnt, 2*SCHED_CAPACITY_SHIFT, &acnt))
> » goto error;
>
> eventually forcing the kernel to disable the feature with the
> message "Scheduler frequency invariance went wobbly, disabling!".
> Let's avoid that by detecting long tickless periods and bypassing
> the calculation for that tick.
>
> This calculation updates the value of arch_freq_scale, used by the
> capacity-aware scheduler to correct cpu duty cycles:
> task_util_freq_inv(p) = duty_cycle(p) * (curr_frequency(cpu) /
> max_frequency(cpu))
>
> However Consider a long tickless period, It takes should take 60 minutes
> for a tickless CPU running at 5GHz to trigger the acnt overflow,
> pick 10 minutes as a staleness threshold to be on the safe side,
> In our testing it took over 30 minutes for the overflow to happen,
> but since it's frequency/platform dependent we choose a smaller value
> to be on the safe side.
>
> Fixes: e2b0d619b400 ("x86, sched: check for counters overflow in frequency invariant accounting")
> Signed-off-by: Yair Podemsky <ypodemsk@redhat.com>
> ---
> arch/x86/kernel/cpu/aperfmperf.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/aperfmperf.c b/arch/x86/kernel/cpu/aperfmperf.c
> index 1f60a2b27936..dfe356034a60 100644
> --- a/arch/x86/kernel/cpu/aperfmperf.c
> +++ b/arch/x86/kernel/cpu/aperfmperf.c
> @@ -23,6 +23,13 @@
>
> #include "cpu.h"
>
> +/*
> + * Samples older then 10 minutes should not be proccessed,
> + * This time is long enough to prevent unneeded drops of data
> + * But short enough to prevent overflows
> + */
> +#define MAX_SAMPLE_AGE_NOHZ ((unsigned long)HZ * 600)
> +
> struct aperfmperf {
> seqcount_t seq;
> unsigned long last_update;
> @@ -373,6 +380,7 @@ static inline void scale_freq_tick(u64 acnt, u64 mcnt) { }
> void arch_scale_freq_tick(void)
> {
> struct aperfmperf *s = this_cpu_ptr(&cpu_samples);
> + unsigned long last = s->last_update;
> u64 acnt, mcnt, aperf, mperf;
>
> if (!cpu_feature_enabled(X86_FEATURE_APERFMPERF))
> @@ -392,7 +400,12 @@ void arch_scale_freq_tick(void)
> s->mcnt = mcnt;
> raw_write_seqcount_end(&s->seq);
>
> - scale_freq_tick(acnt, mcnt);
> + /*
> + * Avoid calling scale_freq_tick() when the last update was too long ago,
> + * as it might overflow during calulation.
> + */
> + if ((jiffies - last) <= MAX_SAMPLE_AGE_NOHZ)
> + scale_freq_tick(acnt, mcnt);
> }

All this patch does is avoid the warning; but afaict it doesn't make it
behave in a sane way.

I'm thinking that on nohz_full cpus you don't have load balancing, I'm
also thinking that on nohz_full cpus you don't have DVFS.

So *why* the heck are we setting this stuff to random values ? Should
you not simply kill th entire thing for nohz_full cpus?

\
 
 \ /
  Last update: 2022-09-06 17:46    [W:0.147 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site