2016-03-24 05:11:37

by Jisheng Zhang

Subject: [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 <[email protected]>
---
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

2016-03-24 13:01:18

by Rafael J. Wysocki

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

On Thu, Mar 24, 2016 at 6:07 AM, Jisheng Zhang <[email protected]> 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 <[email protected]>

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,
> --

2016-03-24 16:03:42

by Lorenzo Pieralisi

Subject: 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 <[email protected]>
> ---
> 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
>

2016-03-25 06:32:03

by Jisheng Zhang

Subject: 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 <[email protected]>
> > ---
> > 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

2016-03-25 13:24:51

by Jisheng Zhang

Subject: 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 <[email protected]>
> > > ---
> > > 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