Re: [UPDATE][PATCH] cpufreq: intel_pstate: Fix EPP restore after offline/online

[Srinivas Pandruvada]

On Tue, 2021-11-16 at 18:47 +0100, Rafael J. Wysocki wrote:
> On Mon, Nov 15, 2021 at 2:40 PM Srinivas Pandruvada
> <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> > 
> > When using performance policy, EPP value is restored to non
> > "performance"
> > mode EPP after offline and online.
> > 
> > For example:
> > cat
> > /sys/devices/system/cpu/cpu1/cpufreq/energy_performance_preference
> > performance
> > echo 0 > /sys/devices/system/cpu/cpu1/online
> > echo 1 > /sys/devices/system/cpu/cpu1/online
> > cat
> > /sys/devices/system/cpu/cpu1/cpufreq/energy_performance_preference
> > balance_performance
> > 
> > The commit 4adcf2e5829f ("cpufreq: intel_pstate: Add ->offline and
> > ->online callbacks")
> > optimized save restore path of the HWP request MSR, when there is
> > no
> > change in the policy. Also added special processing for performance
> > mode
> > EPP. If EPP has been set to "performance" by the active mode
> > "performance"
> > scaling algorithm, replace that value with the cached EPP. This
> > ends up
> > replacing with cached EPP during offline, which is restored during
> > online
> > again.
> > 
> > So add a change which will set cpu_data->epp_policy to zero, when
> > in
> > performance policy and has non zero epp. In this way EPP is set to
> > zero
> > again.
> > 
> > Fixes: 4adcf2e5829f ("cpufreq: intel_pstate: Add ->offline and -
> > >online callbacks")
> > Signed-off-by: Srinivas Pandruvada <
> > srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx # v5.9+
> > ---
> > Update: Minor optimization to skip non performance policy code path
> > 
> >  drivers/cpufreq/intel_pstate.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/intel_pstate.c
> > b/drivers/cpufreq/intel_pstate.c
> > index 815df3daae9d..6d7d73a0c66b 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -936,11 +936,17 @@ static void intel_pstate_hwp_set(unsigned int
> > cpu)
> >         max = cpu_data->max_perf_ratio;
> >         min = cpu_data->min_perf_ratio;
> > 
> > -       if (cpu_data->policy == CPUFREQ_POLICY_PERFORMANCE)
> > -               min = max;
> > -
> >         rdmsrl_on_cpu(cpu, MSR_HWP_REQUEST, &value);
> > 
> > +       if (cpu_data->policy == CPUFREQ_POLICY_PERFORMANCE) {
> > +               min = max;
> > +               epp = 0;
> > +               if (boot_cpu_has(X86_FEATURE_HWP_EPP))
> > +                       epp = (value >> 24) & 0xff;
> > +               if (epp)
> > +                       cpu_data->epp_policy = 0;
> > +       }
> 
> I understand the bug, but it should not be necessary to check this
> every time intel_pstate_hwp_set() runs.
> 
> > +
> >         value &= ~HWP_MIN_PERF(~0L);
> >         value |= HWP_MIN_PERF(min);
> > 
> > --
> 
> Isn't the following sufficient (modulo the gmail-induced whitespace
> damage)?
> 
> ---
>  drivers/cpufreq/intel_pstate.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -1006,6 +1006,12 @@ static void intel_pstate_hwp_offline(str
>           */
>          value &= ~GENMASK_ULL(31, 24);
>          value |= HWP_ENERGY_PERF_PREFERENCE(cpu->epp_cached);
> +        /*
> +         * However, make sure that EPP will be set to "performance"
> when
> +         * the CPU is brought back online again and the
> "performance"
> +         * scaling algorithm is still in effect.
> +         */
> +        cpu->epp_policy = CPUFREQ_POLICY_UNKNOWN;
>      }
> 
>      /*
This works also.

Thanks,
Srinivas