Re: [PATCH] xen/cpufreq: Reset policy after enabling/disabling turbo status

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.11.2021 10:19, Jane Malalane wrote:
> Before, user would change turbo status but this had no effect: boolean
> was set but policy wasn't reevaluated.  Policy must be reevaluated so
> that CPU frequency is chosen according to the turbo status and the
> current governor.

Aiui this only (or at least mainly) affects the ACPI driver. Powernow
updates CPB via its update hook. I think this wants clarifying.

> Therefore, add __cpufreq_governor() in cpufreq_update_turbo().
> 
> Reported-by: <edvin.torok@xxxxxxxxxx>
> Signed-off-by: <jane.malalane@xxxxxxxxxx>

Nit: These would look better with real names added. Without I'm not
even sure enclosing the email addresses in angle brackets yields
something that's valid.

> Release rationale:
> Not taking this patch means that turbo status is misleading.
> 
> Taking this patch is low-risk as it only uses a function that already
> exists and is already used for setting the chosen scaling governor.
> Essentially, this change is equivalent to running 'xenpm
> en/disable-turbo-mode' and, subsequently, running 'xenpm
> set-scaling-governor <current governor>'.

Otoh things have been this way virtually forever.

> --- a/xen/drivers/cpufreq/utility.c
> +++ b/xen/drivers/cpufreq/utility.c
> @@ -417,10 +417,14 @@ int cpufreq_update_turbo(int cpuid, int new_state)
>      {
>          ret = cpufreq_driver.update(cpuid, policy);
>          if (ret)
> +        {
>              policy->turbo = curr_state;
> +            return ret;
> +        }
>      }
>  
> -    return ret;
> +    /* Reevaluate current CPU policy. */
> +    return __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);

Is this really needed when an .update hook is present? IOW wouldn't
this want to be in an "else" to the preceding if()? Or if not,
would this perhaps be more logically done prior to invoking .update()
(such that the hook would observe the updated policy, in case that's
relevant to what it does)?

Jan