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

[Huang Rui]

On Fri, Apr 29, 2022 at 10:36:06AM +0800, Fontenot, Nathan wrote:
> 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?
> 

Hmm, maybe we can enable an easy way to help the users to rework the module
in their existing platform like acpi-cpufreq "off" and set "shared_mem" on
amd-pstate firstly. Actually, many users mails me that how to enable the
module. We can dig out an easy way for them.

Thanks,
Ray