Re: [intel-pstate driver regression] processor frequency very high even if in idle

["Jörg Otte" <jrg.otte@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7325 bytes --]

2016-04-01 14:40 GMT+02:00 Rafael J. Wysocki <rjw@rjwysocki.net>:
> On Friday, April 01, 2016 11:20:42 AM Jörg Otte wrote:
>> 2016-03-31 17:43 GMT+02:00 Rafael J. Wysocki <rjw@rjwysocki.net>:
>> > On Thursday, March 31, 2016 05:25:18 PM Jörg Otte wrote:
>> >> 2016-03-31 13:42 GMT+02:00 Rafael J. Wysocki <rjw@rjwysocki.net>:
>> >> > On Thursday, March 31, 2016 11:05:56 AM Jörg Otte wrote:
>> >> >
>> >> > [cut]
>> >> >
>> >> >> >
>> >> >>
>> >> >> Yes, works for me.
>> >> >>
>> >> >> CPUID(7): No-SGX
>> >> >>      CPU Avg_MHz   Busy% Bzy_MHz TSC_MHz
>> >> >>        -      11    0.66 1682 2494
>> >> >>        0      11    0.60 1856 2494
>> >> >>        1       6    0.34    1898    2494
>> >> >>        2      13    0.82    1628    2494
>> >> >>        3      13    0.87    1528    2494
>> >> >>      CPU Avg_MHz   Busy% Bzy_MHz TSC_MHz
>> >> >>        -       6    0.58     963    2494
>> >> >>        0       8    0.83     957    2494
>> >> >>        1       1    0.08     984    2494
>> >> >>        2      10    1.04     975    2494
>> >> >>        3       3    0.35     934    2494
>> >> >>
>> >> >
>> >
>> > [cut]
>> >
>> >> >
>> >>
>> >> No, this patch doesn't help.
>> >
>> > Well, more work to do then.
>> >
>> > I've just noticed a bug in this patch, which is not relevant for the results,
>> > but below is a new version.
>> >
>> >> CPUID(7): No-SGX
>> >>       CPU Avg_MHz   Busy% Bzy_MHz TSC_MHz
>> >>        -       8    0.32    2507    2495
>> >>        0      13    0.53    2505    2495
>> >>        1       3    0.11    2523    2495
>> >>        2       1    0.06    2555    2495
>> >>        3      15    0.59    2500    2495
>> >>      CPU Avg_MHz   Busy% Bzy_MHz TSC_MHz
>> >>        -       8    0.33    2486    2495
>> >>        0      12    0.50    2482    2495
>> >>        1       5    0.22    2489    2495
>> >>        2       1    0.04    2492    2495
>> >>        3      15    0.59    2487    2495
>> >
>
> [cut]
>
>>
>> here they are.
>>
>
> Thanks!
>
> First of all, the sampling mechanics works as expected in the failing case,
> which is the most important thing I wanted to know.  However, there are anomalies
> in the failing case trace.  The core_busy column is clearly suspicious and it
> looks like CPUs 2 and 3 never really go idle.  I guess we'll need to find out
> why they don't go idle to get to the bottom of this, but it firmly falls into
> the weird stuff territory already.
>
> In the meantime, below is one more patch to test, on top of the previous one
> (that is, https://patchwork.kernel.org/patch/8714401/).
>
> Again, this is a change I'd like to make regardless, so it would be good to
> know if anything more has to be done before we go further.
>
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] intel_pstate: Avoid extra invocation of intel_pstate_sample()
>
> The initialization of intel_pstate for a given CPU involves populating
> the fields of its struct cpudata that represent the previous sample,
> but currently that is done in a problematic way.
>
> Namely, intel_pstate_init_cpu() makes an extra call to
> intel_pstate_sample() so it reads the current register values that
> will be used to populate the "previous sample" record during the
> next invocation of intel_pstate_sample().  However, after commit
> a4675fbc4a7a (cpufreq: intel_pstate: Replace timers with utilization
> update callbacks) that doesn't work for last_sample_time, because
> the time value is passed to intel_pstate_sample() as an argument now.
> Passing 0 to it from intel_pstate_init_cpu() is problematic, because
> that causes cpu->last_sample_time == 0 to be visible in
> get_target_pstate_use_performance() (and hence the extra
> cpu->last_sample_time > 0 check in there) and effectively allows
> the first invocation of intel_pstate_sample() from
> intel_pstate_update_util() to happen immediately after the
> initialization which may lead to a significant "turn on"
> effect in the governor algorithm.
>
> To mitigate that issue, rework the initialization to avoid the
> extra intel_pstate_sample() call from intel_pstate_init_cpu().
> Instead, make intel_pstate_sample() return false if it has been
> called with cpu->sample.time equal to zero, which will make
> intel_pstate_update_util() skip the sample in that case, and
> reset cpu->sample.time from intel_pstate_set_update_util_hook()
> to make the algorithm start properly every time the hook is set.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c |   21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -910,7 +910,14 @@ static inline bool intel_pstate_sample(s
>         cpu->prev_aperf = aperf;
>         cpu->prev_mperf = mperf;
>         cpu->prev_tsc = tsc;
> -       return true;
> +       /*
> +        * First time this function is invoked in a given cycle, all of the
> +        * previous sample data fields are equal to zero or stale and they must
> +        * be populated with meaningful numbers for things to work, so assume
> +        * that sample.time will always be reset before setting the utilization
> +        * update hook and make the caller skip the sample then.
> +        */
> +       return !!cpu->last_sample_time;
>  }
>
>  static inline int32_t get_avg_frequency(struct cpudata *cpu)
> @@ -984,8 +991,7 @@ static inline int32_t get_target_pstate_
>          * enough period of time to adjust our busyness.
>          */
>         duration_ns = cpu->sample.time - cpu->last_sample_time;
> -       if ((s64)duration_ns > pid_params.sample_rate_ns * 3
> -           && cpu->last_sample_time > 0) {
> +       if ((s64)duration_ns > pid_params.sample_rate_ns * 3) {
>                 sample_ratio = div_fp(int_tofp(pid_params.sample_rate_ns),
>                                       int_tofp(duration_ns));
>                 core_busy = mul_fp(core_busy, sample_ratio);
> @@ -1100,7 +1106,6 @@ static int intel_pstate_init_cpu(unsigne
>         intel_pstate_get_cpu_pstates(cpu);
>
>         intel_pstate_busy_pid_reset(cpu);
> -       intel_pstate_sample(cpu, 0);
>
>         cpu->update_util.func = intel_pstate_update_util;
>
> @@ -1121,9 +1126,13 @@ static unsigned int intel_pstate_get(uns
>         return get_avg_frequency(cpu);
>  }
>
> -static void intel_pstate_set_update_util_hook(unsigned int cpu)
> +static void intel_pstate_set_update_util_hook(unsigned int cpu_num)
>  {
> -       cpufreq_set_update_util_data(cpu, &all_cpu_data[cpu]->update_util);
> +       struct cpudata *cpu = all_cpu_data[cpu_num];
> +
> +       /* Prevent intel_pstate_update_util() from using stale data. */
> +       cpu->sample.time = 0;
> +       cpufreq_set_update_util_data(cpu_num, &cpu->update_util);
>  }
>
>  static void intel_pstate_clear_update_util_hook(unsigned int cpu)
>

Done. Attached the tracer.
For me it looks like the previous one of the failing case.

Thanks, Jörg

[-- Attachment #2: intel_pstate_tracer_3.xz --]
[-- Type: application/x-xz, Size: 8868 bytes --]