Re: [PATCH] cpuidle: Move cpuidle driver forward before acpi driver in Makefile

[Rafael J. Wysocki]

On Tue, Jul 26, 2022 at 3:46 AM Mi, Dapeng1 <dapeng1.mi@xxxxxxxxx> wrote:
>
> > From: Mi, Dapeng1
> > Sent: Thursday, July 21, 2022 11:09 AM
> > To: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> > Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>;
> > Bart Van Assche <bvanassche@xxxxxxx>; Linux Kernel Mailing List <linux-
> > kernel@xxxxxxxxxxxxxxx>; Linux PM <linux-pm@xxxxxxxxxxxxxxx>; Zhenyu
> > Wang <zhenyuw@xxxxxxxxxxxxxxx>
> > Subject: RE: [PATCH] cpuidle: Move cpuidle driver forward before acpi driver
> > in Makefile
> >
> > > From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> > > Sent: Wednesday, July 20, 2022 6:24 PM
> > > To: Mi, Dapeng1 <dapeng1.mi@xxxxxxxxx>
> > > Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>; Michael S. Tsirkin
> > > <mst@xxxxxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>; Bart Van Assche
> > > <bvanassche@xxxxxxx>; Linux Kernel Mailing List <linux-
> > > kernel@xxxxxxxxxxxxxxx>; Linux PM <linux-pm@xxxxxxxxxxxxxxx>; Zhenyu
> > > Wang <zhenyuw@xxxxxxxxxxxxxxx>
> > > Subject: Re: [PATCH] cpuidle: Move cpuidle driver forward before acpi
> > > driver in Makefile
> > >
> > > On Wed, Jul 20, 2022 at 5:00 AM Mi, Dapeng1 <dapeng1.mi@xxxxxxxxx>
> > > wrote:
> > > >
> > > > > > From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> > > > > > Sent: Thursday, July 14, 2022 1:53 AM
> > > > > > To: Mi, Dapeng1 <dapeng1.mi@xxxxxxxxx>
> > > > > > Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>; Michael S. Tsirkin
> > > > > > <mst@xxxxxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>; Bart Van
> > > Assche
> > > > > > <bvanassche@xxxxxxx>; Linux Kernel Mailing List <linux-
> > > > > > kernel@xxxxxxxxxxxxxxx>; Linux PM <linux-pm@xxxxxxxxxxxxxxx>
> > > > > > Subject: Re: [PATCH] cpuidle: Move cpuidle driver forward before
> > > > > > acpi driver in Makefile
> > > > > >
> > > > > > On Wed, Jul 13, 2022 at 10:21 AM Dapeng Mi
> > > > > > <dapeng1.mi@xxxxxxxxx>
> > > > > wrote:
> > > > > > >
> > > > > > > As long as Kconfig ACPI_PROCESSOR is enabled, ACPI_PROCESSOR
> > > > > > > would select ACPI_PROCESSOR_IDLE and acpi_idle driver is
> > > > > > > enabled. But in current driver loading order acpi_idle driver
> > > > > > > is always loaded before cpuidle_haltpoll driver. This leads to
> > > > > > > cpuidle_hatpoll driver has no chance to be loaded when it's enabled.
> > > > > > >
> > > > > > > Thus, move cpuidle driver forward before acpi driver and make
> > > > > > > cpuidle-hatpoll driver has a chance to be run when it's enabled.
> > > > > > >
> > > > > > > Signed-off-by: Dapeng Mi <dapeng1.mi@xxxxxxxxx>
> > > > > > > ---
> > > > > > >  drivers/Makefile | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/Makefile b/drivers/Makefile index
> > > > > > > 9a30842b22c5..921ed481b520 100644
> > > > > > > --- a/drivers/Makefile
> > > > > > > +++ b/drivers/Makefile
> > > > > > > @@ -26,6 +26,7 @@ obj-y                         += idle/
> > > > > > >  # IPMI must come before ACPI in order to provide IPMI
> > > > > > > opregion
> > > > > support
> > > > > > >  obj-y                          += char/ipmi/
> > > > > > >
> > > > > > > +obj-$(CONFIG_CPU_IDLE)         += cpuidle/
> > > > > > >  obj-$(CONFIG_ACPI)             += acpi/
> > > > > > >
> > > > > > >  # PnP must come after ACPI since it will eventually need to
> > > > > > > check if
> > > acpi
> > > > > > > @@ -126,7 +127,6 @@ obj-$(CONFIG_EDAC)          += edac/
> > > > > > >  obj-$(CONFIG_EISA)             += eisa/
> > > > > > >  obj-$(CONFIG_PM_OPP)           += opp/
> > > > > > >  obj-$(CONFIG_CPU_FREQ)         += cpufreq/
> > > > > > > -obj-$(CONFIG_CPU_IDLE)         += cpuidle/
> > > > > > >  obj-y                          += mmc/
> > > > > > >  obj-y                          += ufs/
> > > > > > >  obj-$(CONFIG_MEMSTICK)         += memstick/
> > > > > > > --
> > > > > >
> > > > > > Well, this change doesn't guarantee loading haltpoll before ACPI idle.
> > > > > >
> > > > > > Also what if haltpoll is enabled, but the user wants ACPI idle?
> > > > >
> > > > > Thanks Rafael for reviewing this patch.
> > > > >
> > > > > acpi_idle driver and cpuidle_haltpoll driver have same
> > > > > initialization level and both are initialized on the level
> > > > > device_initcall. So the building order would decide the loading
> > > > > sequence. Just like the intel_idle driver which also has same
> > > > > initialization level (device_initcall), but as it's built before
> > > > > acpi_idle driver, it would be loaded first before acpi_driver if
> > > > > intel_idle
> > > driver is enabled.
> > > > >
> > > > > There is another method to make cpuidle_haltpoll driver loaded
> > > > > first before acpi_driver, it's change the initialization level to
> > > > > postcore_initcall. I'm not sure which one is better, but it seems
> > > > > current
> > > patch is more reasonable.
> > > > >
> > > > > There is an parameter "force" to manage the haltpoll enabling. If
> > > > > user want to use ACPI idle, it can change this parameter to
> > > > > disable
> > > haltpolll driver.
> > >
> > > That would require things to be appended to the kernel command line in
> > > cases where that's not necessary today and that's not acceptable.
> > >
> > The haltpoll driver's "force" parameter is false by default, we don't need to
> > add extra command line options in most cases except we want to enable the
> > haltpolling driver.
> >
> > > What you really seem to be wanting to do is to use haltpoll instead of
> > > ACPI idle.  Is that correct?
> >
> > Yes, I'm trying to enable guest halt polling to improve the performance of
> > some Virtual Machine. But I found the halt poll driver can't be enabled as
> > long as acpi idle driver is enabled. I tried to disable the acpi idle first, but I
> > found there is no parameter to enable/disable acpi idle driver except
> > disabling the Kconfig "CONFIG_ACPI_PROCESSOR_IDLE", and but
> > unfortunately Kconfig "ACPI_PROCESSOR" would enable
> > "ACPI_PROCESSOR_IDLE" by default. If I want to disable acpi_idle driver, I
> > have to disable the "ACPI_PROCESSOR", but this would cause acpi throttling
> > and acpi thermal are also disabled. That's not what I want. That's why I do this
> > change to make halt poll driver has a  chance to run without disable the
> > entire acpi processor functionality .
> >
>
> Any feedback? Thanks.

I've already said that messing up with Makefiles is not the way to go here.

You have to enforce a specific ordering of initialization in the code.