[PATCH 1/2][v2] cpuidle-haltpoll: Replace default_idle with arch_cpu_idle

[PATCH 1/2][v2] cpuidle-haltpoll: Replace default_idle with arch_cpu_idle

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

When KVM guest has MWAIT and mwait_idle is used as default idle function,
but once cpuidle-haltpoll is loaded, default_idle in default_enter_idle is
used, which is using HLT, and cause a performance regression. As the commit
aebef63cf7ff ("x86: Remove vendor checks from prefer_mwait_c1_over_halt")
explains that mwait is preferred

so replace default_idle with arch_cpu_idle which can using MWAIT
optimization.

latency of sockperf ping-pong localhost test is reduced by more 20%
unixbench has 5% performance improvements for single core

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 arch/x86/kernel/process.c          | 1 +
 drivers/cpuidle/cpuidle-haltpoll.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index c21b734..303afad 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -721,6 +721,7 @@ void arch_cpu_idle(void)
 {
 	x86_idle();
 }
+EXPORT_SYMBOL(arch_cpu_idle);
 
 /*
  * We use this if we don't have any better idle routine..
diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c
index 3a39a7f..e66df22 100644
--- a/drivers/cpuidle/cpuidle-haltpoll.c
+++ b/drivers/cpuidle/cpuidle-haltpoll.c
@@ -32,7 +32,7 @@ static int default_enter_idle(struct cpuidle_device *dev,
 		local_irq_enable();
 		return index;
 	}
-	default_idle();
+	arch_cpu_idle();
 	return index;
 }
 
-- 
2.9.4

[PATCH 2/2][v2] cpuidle-haltpoll: Build as module by default

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

Allow user to unload it in running

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/cpuidle/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index ff71dd6..43ddb84 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -74,7 +74,7 @@ endmenu
 config HALTPOLL_CPUIDLE
 	tristate "Halt poll cpuidle driver"
 	depends on X86 && KVM_GUEST
-	default y
+	default m
 	help
 	 This option enables halt poll cpuidle driver, which allows to poll
 	 before halting in the guest (more efficient than polling in the
-- 
2.9.4

Re: [PATCH 2/2][v2] cpuidle-haltpoll: Build as module by default

[Rafael J. Wysocki]

On Wed, Dec 7, 2022 at 1:41 PM <lirongqing@baidu.com> wrote:
>
> From: Li RongQing <lirongqing@baidu.com>
>
> Allow user to unload it in running

Just like that?  And corrupt things left and right while at it?

No way.

And why do you need this?

> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  drivers/cpuidle/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> index ff71dd6..43ddb84 100644
> --- a/drivers/cpuidle/Kconfig
> +++ b/drivers/cpuidle/Kconfig
> @@ -74,7 +74,7 @@ endmenu
>  config HALTPOLL_CPUIDLE
>         tristate "Halt poll cpuidle driver"
>         depends on X86 && KVM_GUEST
> -       default y
> +       default m
>         help
>          This option enables halt poll cpuidle driver, which allows to poll
>          before halting in the guest (more efficient than polling in the
> --

RE: [PATCH 2/2][v2] cpuidle-haltpoll: Build as module by default

[Li,Rongqing]

> > Allow user to unload it in running
> 
> Just like that?  And corrupt things left and right while at it?
> 
> No way.
> 
> And why do you need this?

Cpuidle-haltpoll can not improve performance for all cases, like when guest has mwait, unixbench shows a small performance drop; 
So change it as module, user can insmod this drivers and rmmod this driver at run time

And some downstream os, centos and ubuntu build it module

Of cause, it will cause performance drop since it is not installed by default, but user insmod this module, this performance can restore, so I think this is acceptable
If this reasom is acceptable, I will add v3; or drop this patch.

Thanks

-Li

Re: [PATCH 2/2][v2] cpuidle-haltpoll: Build as module by default

[Rafael J. Wysocki]

On Thu, Dec 8, 2022 at 3:32 AM Li,Rongqing <lirongqing@baidu.com> wrote:
>
>
> > > Allow user to unload it in running
> >
> > Just like that?  And corrupt things left and right while at it?
> >
> > No way.
> >
> > And why do you need this?
>
> Cpuidle-haltpoll can not improve performance for all cases, like when guest has mwait, unixbench shows a small performance drop;
> So change it as module, user can insmod this drivers and rmmod this driver at run time

That is problematic, because in the mainline Linux kernel (which is
what we are talking about here) there is no support for modular
cpuidle governors.

Also, there is an interface for switching cpuidle governors at run
time already, so why can 't it be used to address this case?

> And some downstream os, centos and ubuntu build it module

Well, it's their problem.

RE: [PATCH 2/2][v2] cpuidle-haltpoll: Build as module by default

[Li,Rongqing]

> Also, there is an interface for switching cpuidle governors at run time already, so
> why can 't it be used to address this case?


I will study this interface, thanks

-Li

Re: [PATCH 2/2][v2] cpuidle-haltpoll: Build as module by default

[Rafael J. Wysocki]

On Thu, Dec 8, 2022 at 1:45 PM Li,Rongqing <lirongqing@baidu.com> wrote:
>
> > Also, there is an interface for switching cpuidle governors at run time already, so
> > why can 't it be used to address this case?
>
>
> I will study this interface, thanks

Sorry, this patch series is about the haltpoll driver, not the
haltpoll governor (which is there too), so you are right, it can be
modular, but it is not modular by default.

I guess it would be fine to make it modular by default, unless there
are expectations regarding it being present on system startup in the
field and that part is unclear.  I think it would be better to defer
this change until it can be clarified.

Re: [PATCH 2/2][v2] cpuidle-haltpoll: Build as module by default

[Peter Zijlstra]

On Thu, Dec 08, 2022 at 02:32:15AM +0000, Li,Rongqing wrote:
> 
> > > Allow user to unload it in running
> > 
> > Just like that?  And corrupt things left and right while at it?
> > 
> > No way.
> > 
> > And why do you need this?
> 
> Cpuidle-haltpoll can not improve performance for all cases, like when
> guest has mwait, unixbench shows a small performance drop; So change
> it as module, user can insmod this drivers and rmmod this driver at
> run time

I'm thinking all this can be achieved by a small change to
haltpoll_want().

Re: [PATCH 1/2][v2] cpuidle-haltpoll: Replace default_idle with arch_cpu_idle

[Rafael J. Wysocki]

On Wed, Dec 7, 2022 at 1:42 PM <lirongqing@baidu.com> wrote:
>
> From: Li RongQing <lirongqing@baidu.com>
>
> When KVM guest has MWAIT and mwait_idle is used as default idle function,
> but once cpuidle-haltpoll is loaded, default_idle in default_enter_idle is
> used, which is using HLT, and cause a performance regression. As the commit
> aebef63cf7ff ("x86: Remove vendor checks from prefer_mwait_c1_over_halt")
> explains that mwait is preferred
>
> so replace default_idle with arch_cpu_idle which can using MWAIT
> optimization.
>
> latency of sockperf ping-pong localhost test is reduced by more 20%
> unixbench has 5% performance improvements for single core
>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  arch/x86/kernel/process.c          | 1 +
>  drivers/cpuidle/cpuidle-haltpoll.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index c21b734..303afad 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -721,6 +721,7 @@ void arch_cpu_idle(void)
>  {
>         x86_idle();
>  }
> +EXPORT_SYMBOL(arch_cpu_idle);

Why do you need this export at all?

>
>  /*
>   * We use this if we don't have any better idle routine..
> diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c
> index 3a39a7f..e66df22 100644
> --- a/drivers/cpuidle/cpuidle-haltpoll.c
> +++ b/drivers/cpuidle/cpuidle-haltpoll.c
> @@ -32,7 +32,7 @@ static int default_enter_idle(struct cpuidle_device *dev,
>                 local_irq_enable();
>                 return index;
>         }
> -       default_idle();
> +       arch_cpu_idle();
>         return index;
>  }
>
> --
> 2.9.4
>

RE: [PATCH 1/2][v2] cpuidle-haltpoll: Replace default_idle with arch_cpu_idle

[Li,Rongqing]

> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Wednesday, December 7, 2022 10:38 PM
> To: Li,Rongqing <lirongqing@baidu.com>
> Cc: tglx@linutronix.de; mingo@redhat.com; bp@alien8.de;
> dave.hansen@linux.intel.com; x86@kernel.org; rafael@kernel.org;
> daniel.lezcano@linaro.org; peterz@infradead.org; akpm@linux-foundation.org;
> tony.luck@intel.com; jpoimboe@kernel.org; linux-kernel@vger.kernel.org;
> linux-pm@vger.kernel.org
> Subject: Re: [PATCH 1/2][v2] cpuidle-haltpoll: Replace default_idle with
> arch_cpu_idle
> 
> On Wed, Dec 7, 2022 at 1:42 PM <lirongqing@baidu.com> wrote:
> >
> > From: Li RongQing <lirongqing@baidu.com>
> >
> > When KVM guest has MWAIT and mwait_idle is used as default idle
> > function, but once cpuidle-haltpoll is loaded, default_idle in
> > default_enter_idle is used, which is using HLT, and cause a
> > performance regression. As the commit aebef63cf7ff ("x86: Remove
> > vendor checks from prefer_mwait_c1_over_halt") explains that mwait is
> > preferred
> >
> > so replace default_idle with arch_cpu_idle which can using MWAIT
> > optimization.
> >
> > latency of sockperf ping-pong localhost test is reduced by more 20%
> > unixbench has 5% performance improvements for single core
> >
> > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> >  arch/x86/kernel/process.c          | 1 +
> >  drivers/cpuidle/cpuidle-haltpoll.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index c21b734..303afad 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -721,6 +721,7 @@ void arch_cpu_idle(void)  {
> >         x86_idle();
> >  }
> > +EXPORT_SYMBOL(arch_cpu_idle);
> 
> Why do you need this export at all?
> 

When cpuidle-haltpoll is built as module, if arch_cpu_idle is not export, cpuidle-haltpoll will complain "arch_cpu_idle" undefined

Like

ERROR: modpost: "arch_cpu_idle" [drivers/cpuidle/cpuidle-haltpoll.ko] undefined!
make[1]: *** [scripts/Makefile.modpost:126: Module.symvers] Error 1
make: *** [Makefile:1944: modpost] Error 2
Error: Make failed!

I will add the reason to v3 
Thanks

-Li

Re: [PATCH 1/2][v2] cpuidle-haltpoll: Replace default_idle with arch_cpu_idle

[Rafael J. Wysocki]

On Thu, Dec 8, 2022 at 2:48 AM Li,Rongqing <lirongqing@baidu.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Rafael J. Wysocki <rafael@kernel.org>
> > Sent: Wednesday, December 7, 2022 10:38 PM
> > To: Li,Rongqing <lirongqing@baidu.com>
> > Cc: tglx@linutronix.de; mingo@redhat.com; bp@alien8.de;
> > dave.hansen@linux.intel.com; x86@kernel.org; rafael@kernel.org;
> > daniel.lezcano@linaro.org; peterz@infradead.org; akpm@linux-foundation.org;
> > tony.luck@intel.com; jpoimboe@kernel.org; linux-kernel@vger.kernel.org;
> > linux-pm@vger.kernel.org
> > Subject: Re: [PATCH 1/2][v2] cpuidle-haltpoll: Replace default_idle with
> > arch_cpu_idle
> >
> > On Wed, Dec 7, 2022 at 1:42 PM <lirongqing@baidu.com> wrote:
> > >
> > > From: Li RongQing <lirongqing@baidu.com>
> > >
> > > When KVM guest has MWAIT and mwait_idle is used as default idle
> > > function, but once cpuidle-haltpoll is loaded, default_idle in
> > > default_enter_idle is used, which is using HLT, and cause a
> > > performance regression. As the commit aebef63cf7ff ("x86: Remove
> > > vendor checks from prefer_mwait_c1_over_halt") explains that mwait is
> > > preferred
> > >
> > > so replace default_idle with arch_cpu_idle which can using MWAIT
> > > optimization.
> > >
> > > latency of sockperf ping-pong localhost test is reduced by more 20%
> > > unixbench has 5% performance improvements for single core
> > >
> > > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > > ---
> > >  arch/x86/kernel/process.c          | 1 +
> > >  drivers/cpuidle/cpuidle-haltpoll.c | 2 +-
> > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > > index c21b734..303afad 100644
> > > --- a/arch/x86/kernel/process.c
> > > +++ b/arch/x86/kernel/process.c
> > > @@ -721,6 +721,7 @@ void arch_cpu_idle(void)  {
> > >         x86_idle();
> > >  }
> > > +EXPORT_SYMBOL(arch_cpu_idle);
> >
> > Why do you need this export at all?
> >
>
> When cpuidle-haltpoll is built as module,

But it isn't now.

> if arch_cpu_idle is not export, cpuidle-haltpoll will complain "arch_cpu_idle" undefined

So no, this won't happen.

RE: [PATCH 1/2][v2] cpuidle-haltpoll: Replace default_idle with arch_cpu_idle

[Li,Rongqing]

> >
> > When cpuidle-haltpoll is built as module,
> 
> But it isn't now.

Centos is compiling it as module, it will fail;
Other user wants to compile it as module, they will fail, 
Syzbot random configuration building will fail

Unless prohibit to build it as module as below:

config HALTPOLL_CPUIDLE
-        tristate "Halt poll cpuidle driver"
+        bool "Halt poll cpuidle driver"
        depends on X86 && KVM_GUEST
        default y
        help


thanks

-Li

Re: [PATCH 1/2][v2] cpuidle-haltpoll: Replace default_idle with arch_cpu_idle

[Rafael J. Wysocki]

On Thu, Dec 8, 2022 at 1:43 PM Li,Rongqing <lirongqing@baidu.com> wrote:
>
> > >
> > > When cpuidle-haltpoll is built as module,
> >
> > But it isn't now.
>
> Centos is compiling it as module, it will fail;
> Other user wants to compile it as module, they will fail,
> Syzbot random configuration building will fail
>
> Unless prohibit to build it as module as below:
>
> config HALTPOLL_CPUIDLE
> -        tristate "Halt poll cpuidle driver"
> +        bool "Halt poll cpuidle driver"
>         depends on X86 && KVM_GUEST
>         default y
>         help

Ah, OK.  Sorry about the confusion.

Yes, the driver (not the governor, though), can be modular, so yes you
need the export, but please change it to EXPORT_SYMBOL_GPL().

RE: [PATCH 1/2][v2] cpuidle-haltpoll: Replace default_idle with arch_cpu_idle

[Li,Rongqing]

> > latency of sockperf ping-pong localhost test is reduced by more 20%
> > unixbench has 5% performance improvements for single core
> >

Sorry, The upper data are wrong since wrong governor is used

when guest has mwait, and haltpoll driver is loaded and haltpoll governor is used.

on Intel cpu, unixbench score are nearly same, but sockperf has 20% low latency 
on AMD milan cpu, the sockperf latency are similar , but unixbench single core score has 10% loss, because AMD cpu maybe has weak smt capability 

Replace default idle with arch cpu idle has little effect


Thanks

-Li


I