Re: [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded

[Nathan Fontenot]

On 4/28/22 02:15, Huang Rui wrote:
> On Fri, Apr 22, 2022 at 02:38:32AM +0800, Rafael J. Wysocki wrote:
>> On Thu, Apr 14, 2022 at 7:58 PM Limonciello, Mario
>> <Mario.Limonciello@xxxxxxx> wrote:
>>>
>>> [Public]
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Fontenot, Nathan <Nathan.Fontenot@xxxxxxx>
>>>> Sent: Thursday, April 14, 2022 12:33
>>>> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Huang, Ray
>>>> <Ray.Huang@xxxxxxx>; Rafael J . Wysocki <rafael@xxxxxxxxxx>; Viresh
>>>> Kumar <viresh.kumar@xxxxxxxxxx>
>>>> Cc: open list:AMD PSTATE DRIVER <linux-pm@xxxxxxxxxxxxxxx>; Yuan, Perry
>>>> <Perry.Yuan@xxxxxxx>; open list <linux-kernel@xxxxxxxxxxxxxxx>
>>>> Subject: Re: [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-
>>>> cpufreq when loaded
>>>>
>>>> On 4/14/22 11:47, Mario Limonciello wrote:
>>>>> `amd-pstate` can be compiled as a module.  This however can be a
>>>>> deficiency because `acpi-cpufreq` will be loaded earlier when compiled
>>>>> into the kernel meaning `amd-pstate` doesn't get a chance.
>>>>> `acpi-cpufreq` is also unable to be unloaded in this circumstance.
>>>>>
>>>>> To better improve the usability of `amd-pstate` when compiled as a
>>>> module,
>>>>> add an optional module parameter that will force it to replace other
>>>>> cpufreq drivers at startup.
>>>>>
>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
>>>>> ---
>>>>> v2->v3:
>>>>>  * Rebase on earlier patches
>>>>>  * Use IS_REACHABLE
>>>>>  * Only add replace parameter if acpu-cpufreq is enabled
>>>>>  * Only show info message once
>>>>> v1->v2:
>>>>>  * Update to changes from v1.
>>>>>  * Verify the driver being matched is acpi-cpufreq.
>>>>>  * Show a message letting users know they can use amd-pstate.
>>>>>
>>>>>  drivers/cpufreq/amd-pstate.c | 22 ++++++++++++++++++++--
>>>>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>>>>> index d323f3e3888c..8ae65a2072d6 100644
>>>>> --- a/drivers/cpufreq/amd-pstate.c
>>>>> +++ b/drivers/cpufreq/amd-pstate.c
>>>>> @@ -63,6 +63,13 @@ module_param(shared_mem, bool, 0444);
>>>>>  MODULE_PARM_DESC(shared_mem,
>>>>>              "enable amd-pstate on processors with shared memory
>>>> solution (false = disabled (default), true = enabled)");
>>>>>
>>>>> +#if defined(CONFIG_X86_ACPI_CPUFREQ) ||
>>>> defined(CONFIG_X86_ACPI_CPUFREQ_MODULE)
>>>>> +static bool replace = false;
>>>>> +module_param(replace, bool, 0444);
>>>>> +MODULE_PARM_DESC(replace,
>>>>> +             "replace acpi-cpufreq driver upon init if necessary");
>>>>> +#endif
>>>>> +
>>>>>  static struct cpufreq_driver amd_pstate_driver;
>>>>>
>>>>>  /**
>>>>> @@ -643,6 +650,7 @@ static struct cpufreq_driver amd_pstate_driver = {
>>>>>
>>>>>  static int __init amd_pstate_init(void)
>>>>>  {
>>>>> +   const char *current_driver;
>>>>>     int ret;
>>>>>
>>>>>     if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
>>>>> @@ -666,9 +674,19 @@ static int __init amd_pstate_init(void)
>>>>>             return -ENODEV;
>>>>>     }
>>>>>
>>>>> -   /* don't keep reloading if cpufreq_driver exists */
>>>>> -   if (cpufreq_get_current_driver())
>>>>> +   current_driver = cpufreq_get_current_driver();
>>>>> +   if (current_driver) {
>>>>> +#if IS_REACHABLE(CONFIG_X86_ACPI_CPUFREQ)
>>>>> +           if (replace && strcmp(current_driver, "acpi-cpufreq") == 0) {
>>>>> +                   acpi_cpufreq_exit();
>>>>> +           } else {
>>>>> +                   pr_info_once("A processor on this system supports
>>>> amd-pstate, you can enable it with amd_pstate.replace=1\n");
>>>>> +                   return -EEXIST;
>>>>> +           }
>>>>> +#else
>>>>>             return -EEXIST;
>>>>> +#endif
>>>>> +   }
>>>>
>>>> A couple of thoughts. First, should this also provide a path to restore the
>>>> acpi_cpufreq driver
>>>> if the amd-pstate driver fails during init some time after calling
>>>> acpi_cpufreq_exit()?
>>>
>>> I think that's a reasonable idea; it would involve exporting acpi_cpufreq_init
>>> as well.
>>>
>>>>
>>>> Which leads me to wonder, should there be a more generic
>>>> cpufreq_replace_driver() routine that
>>>> could handle this?
>>>
>>> If changing the API for this, my proposal would be that there is a flag used
>>> in cpufreq_driver->flags to indicate that this driver should replace existing
>>> drivers when calling cpufreq_register_driver rather than a new routine.
>>> Then if it fails to register for any reason then the old driver can be restored.
>>>
>>> Rafael, what are your thoughts on this?
>>
>> IMV there need to be two things to make this really work.
>>
>> First, the currently running driver needs to provide a way to tell it
>> to go away.  For example, intel_pstate has the "off" mode (in which it
>> doesn't do anything) for that and similar interfaces can be added to
>> other drivers as needed.
>>
>> The reason why is because, for example, intel_pstate cannot go into
>> the "off" mode when HWP is enabled, because it cannot be disabled and
>> running acpi_cpufreq in that configuration wouldn't work.  So in
>> general you need to know that it is OK to unregister the current
>> driver.
>>
>> Second, there needs to be a mechanism for registering a driver
>> "weakly" for future use, so if it cannot be used right away, it will
>> be added to a list and wait until there's room for it to run.
> 
> The amd-pstate is a new module, we need to add "amd-pstate" on
> /etc/modules-load.d/modules.conf for most of the Linux distro to tell the
> module to load at boot time. However, there are some issues that we
> unregister acpi-cpufreq at runtime while the acpi-cpufreq is in-build.
> 
> As inspired by your suggestion, I am thinking whether we can add "off" mode
> in acpi-cpufreq driver, if user would like to use the amd-pstate driver on
> shared memory processors, they can set acpi-cpufreq "off" and set
> "shared_mem" on amd-pstate to enable the amd-pstate driver. I can add the
> RST documentation to describe the steps.
> 
> Or I can introduce a processor list (family id/model id) that can let user
> know clear which type of processors they are running. Then they can choose
> which driver that they want to use manually as well.
> 

This sounds like a move towards an infrastructure similar to governors where
all supported drivers register and users can choose from a list of available
drivers.

Ray, this seems like a lot of work to be able to dynamically switch to the
amd-pstate driver after boot. Given that this behavior does not currently work
how crucial is it to have this ability?

-Nathan