Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1363926 > unrolled thread

[PATCH] cpuidle: arm: make enter idle operation a bit more efficient

Back to article view | Back to linux.kernel

Contents

  [PATCH] cpuidle: arm: make enter idle operation a bit more efficient Jisheng Zhang <jszhang@marvell.com> - 2016-03-24 06:20 +0100
    Re: [PATCH] cpuidle: arm: make enter idle operation a bit more efficient "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-24 14:10 +0100
    Re: [PATCH] cpuidle: arm: make enter idle operation a bit more  efficient Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-03-24 17:10 +0100
      Re: [PATCH] cpuidle: arm: make enter idle operation a bit more  efficient Jisheng Zhang <jszhang@marvell.com> - 2016-03-25 07:40 +0100
        Re: [PATCH] cpuidle: arm: make enter idle operation a bit more  efficient Jisheng Zhang <jszhang@marvell.com> - 2016-03-25 14:30 +0100

#1363926 — [PATCH] cpuidle: arm: make enter idle operation a bit more efficient

Currently, entering idle need to check the idx every time to choose the
real entering idle routine. But this check could be avoided by pointing
the idle enter function pointer of each idle states to the routines
suitable for each states directly.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/cpuidle/cpuidle-arm.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 545069d..48a620f 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -23,6 +23,13 @@
 
 #include "dt_idle_states.h"
 
+static int arm_enter_wfi_state(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv, int idx)
+{
+	cpu_do_idle();
+	return 0;
+}
+
 /*
  * arm_enter_idle_state - Programs CPU to enter the specified state
  *
@@ -38,11 +45,6 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
 {
 	int ret;
 
-	if (!idx) {
-		cpu_do_idle();
-		return idx;
-	}
-
 	ret = cpu_pm_enter();
 	if (!ret) {
 		/*
@@ -69,7 +71,7 @@ static struct cpuidle_driver arm_idle_driver = {
 	 * handler for idle state index 0.
 	 */
 	.states[0] = {
-		.enter                  = arm_enter_idle_state,
+		.enter                  = arm_enter_wfi_state,
 		.exit_latency           = 1,
 		.target_residency       = 1,
 		.power_usage		= UINT_MAX,
-- 
2.8.0.rc3

[toc] | [next] | [standalone]

#1364149

On Thu, Mar 24, 2016 at 6:07 AM, Jisheng Zhang <jszhang@marvell.com> wrote:
> Currently, entering idle need to check the idx every time to choose the
> real entering idle routine. But this check could be avoided by pointing
> the idle enter function pointer of each idle states to the routines
> suitable for each states directly.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>

This looks reasonable to me, but I need an ACK from the maintainer of this code.

> ---
>  drivers/cpuidle/cpuidle-arm.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 545069d..48a620f 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -23,6 +23,13 @@
>
>  #include "dt_idle_states.h"
>
> +static int arm_enter_wfi_state(struct cpuidle_device *dev,
> +                              struct cpuidle_driver *drv, int idx)
> +{
> +       cpu_do_idle();
> +       return 0;
> +}
> +
>  /*
>   * arm_enter_idle_state - Programs CPU to enter the specified state
>   *
> @@ -38,11 +45,6 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
>  {
>         int ret;
>
> -       if (!idx) {
> -               cpu_do_idle();
> -               return idx;
> -       }
> -
>         ret = cpu_pm_enter();
>         if (!ret) {
>                 /*
> @@ -69,7 +71,7 @@ static struct cpuidle_driver arm_idle_driver = {
>          * handler for idle state index 0.
>          */
>         .states[0] = {
> -               .enter                  = arm_enter_idle_state,
> +               .enter                  = arm_enter_wfi_state,
>                 .exit_latency           = 1,
>                 .target_residency       = 1,
>                 .power_usage            = UINT_MAX,
> --

[toc] | [prev] | [next] | [standalone]

#1364300 — Re: [PATCH] cpuidle: arm: make enter idle operation a bit more efficient

On Thu, Mar 24, 2016 at 01:07:18PM +0800, Jisheng Zhang wrote:
> Currently, entering idle need to check the idx every time to choose the
> real entering idle routine. But this check could be avoided by pointing
> the idle enter function pointer of each idle states to the routines
> suitable for each states directly.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/cpuidle/cpuidle-arm.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 545069d..48a620f 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -23,6 +23,13 @@
>  
>  #include "dt_idle_states.h"
>  
> +static int arm_enter_wfi_state(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int idx)
> +{
> +	cpu_do_idle();
> +	return 0;
> +}
> +
>  /*
>   * arm_enter_idle_state - Programs CPU to enter the specified state
>   *
> @@ -38,11 +45,6 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
>  {
>  	int ret;
>  
> -	if (!idx) {
> -		cpu_do_idle();
> -		return idx;
> -	}

Mmm...if I wanted to paint your bikeshed I would say idx is in a
register and you are removing a simple comparison to exchange it
with a function that adds to code footprint and may even make
performance worse instead of improving anything.

I am not sure this patch makes anything more efficient, happy to be
proven wrong, with significant data.

Thanks,
Lorenzo

> -
>  	ret = cpu_pm_enter();
>  	if (!ret) {
>  		/*
> @@ -69,7 +71,7 @@ static struct cpuidle_driver arm_idle_driver = {
>  	 * handler for idle state index 0.
>  	 */
>  	.states[0] = {
> -		.enter                  = arm_enter_idle_state,
> +		.enter                  = arm_enter_wfi_state,
>  		.exit_latency           = 1,
>  		.target_residency       = 1,
>  		.power_usage		= UINT_MAX,
> -- 
> 2.8.0.rc3
> 

[toc] | [prev] | [next] | [standalone]

#1364570 — Re: [PATCH] cpuidle: arm: make enter idle operation a bit more efficient

Hi Lorenzo,

On Thu, 24 Mar 2016 16:06:00 +0000 Lorenzo Pieralisi wrote:

> On Thu, Mar 24, 2016 at 01:07:18PM +0800, Jisheng Zhang wrote:
> > Currently, entering idle need to check the idx every time to choose the
> > real entering idle routine. But this check could be avoided by pointing
> > the idle enter function pointer of each idle states to the routines
> > suitable for each states directly.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  drivers/cpuidle/cpuidle-arm.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> > index 545069d..48a620f 100644
> > --- a/drivers/cpuidle/cpuidle-arm.c
> > +++ b/drivers/cpuidle/cpuidle-arm.c
> > @@ -23,6 +23,13 @@
> >  
> >  #include "dt_idle_states.h"
> >  
> > +static int arm_enter_wfi_state(struct cpuidle_device *dev,
> > +			       struct cpuidle_driver *drv, int idx)
> > +{
> > +	cpu_do_idle();
> > +	return 0;
> > +}
> > +
> >  /*
> >   * arm_enter_idle_state - Programs CPU to enter the specified state
> >   *
> > @@ -38,11 +45,6 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
> >  {
> >  	int ret;
> >  
> > -	if (!idx) {
> > -		cpu_do_idle();
> > -		return idx;
> > -	}  
> 
> Mmm...if I wanted to paint your bikeshed I would say idx is in a
> register and you are removing a simple comparison to exchange it
> with a function that adds to code footprint and may even make
> performance worse instead of improving anything.
> 
> I am not sure this patch makes anything more efficient, happy to be
> proven wrong, with significant data.

Thanks for pointing this out. I'll do some measurement and get back to you

Thanks,
Jisheng

[toc] | [prev] | [next] | [standalone]

#1364663 — Re: [PATCH] cpuidle: arm: make enter idle operation a bit more efficient

Hi Lorenzo,

On Fri, 25 Mar 2016 14:25:13 +0800 Jisheng Zhang wrote:

> Hi Lorenzo,
> 
> On Thu, 24 Mar 2016 16:06:00 +0000 Lorenzo Pieralisi wrote:
> 
> > On Thu, Mar 24, 2016 at 01:07:18PM +0800, Jisheng Zhang wrote:  
> > > Currently, entering idle need to check the idx every time to choose the
> > > real entering idle routine. But this check could be avoided by pointing
> > > the idle enter function pointer of each idle states to the routines
> > > suitable for each states directly.
> > > 
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > ---
> > >  drivers/cpuidle/cpuidle-arm.c | 14 ++++++++------
> > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> > > index 545069d..48a620f 100644
> > > --- a/drivers/cpuidle/cpuidle-arm.c
> > > +++ b/drivers/cpuidle/cpuidle-arm.c
> > > @@ -23,6 +23,13 @@
> > >  
> > >  #include "dt_idle_states.h"
> > >  
> > > +static int arm_enter_wfi_state(struct cpuidle_device *dev,
> > > +			       struct cpuidle_driver *drv, int idx)
> > > +{
> > > +	cpu_do_idle();
> > > +	return 0;
> > > +}
> > > +
> > >  /*
> > >   * arm_enter_idle_state - Programs CPU to enter the specified state
> > >   *
> > > @@ -38,11 +45,6 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
> > >  {
> > >  	int ret;
> > >  
> > > -	if (!idx) {
> > > -		cpu_do_idle();
> > > -		return idx;
> > > -	}    
> > 
> > Mmm...if I wanted to paint your bikeshed I would say idx is in a
> > register and you are removing a simple comparison to exchange it
> > with a function that adds to code footprint and may even make
> > performance worse instead of improving anything.
> > 
> > I am not sure this patch makes anything more efficient, happy to be
> > proven wrong, with significant data.  
> 
> Thanks for pointing this out. I'll do some measurement and get back to you
> 

I have done the measurement, the fact shows you are correct!

If there's nothing running in the system, the change improve performance by
about 2.8%

while if there's something running, I saw performance regression.

so let's drop this patch.

Thanks for your reviewing,
Jisheng

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web