[PATCH 0/2] cpuidle optimizations (on top of linux-next)

[PATCH 0/2] cpuidle optimizations (on top of linux-next)

[Rafael J. Wysocki]

Hi,

When I was looking at the cpuidle code after the Sudeeps's problem report,
it occured to me that we had some pointless overhead there, so two
changes to reduce it follow.

[1/2] Make the fallback to to default_idle_call() in call_cpuidle()
      unnecessary and drop it.
[2/2] Make menu_select() avoid checking states that don't need to
      (or even shouldn't) be checked when making the selection.

Thanks,
Rafael

[PATCH 1/2] sched / idle: Drop default_idle_call() fallback from call_cpuidle()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit 9c4b2867ed7c (cpuidle: menu: Fix menu_select() for
CPUIDLE_DRIVER_STATE_START == 0) it is clear that menu_select()
cannot return negative values.  Moreover, ladder_select_state()
will never return a negative value too, so make find_deepest_state()
return non-negative values too and drop the default_idle_call()
fallback from call_cpuidle().

This eliminates one branch from the idle loop and makes the governors
and find_deepest_state() handle the case when all states have been
disabled from sysfs consistently.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Note: Commit 9c4b2867ed7c (cpuidle: menu: Fix menu_select() for
CPUIDLE_DRIVER_STATE_START == 0) is in linux-next only ATM.

---
 drivers/cpuidle/cpuidle.c |    6 +++---
 kernel/sched/idle.c       |    6 ------
 2 files changed, 3 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -79,9 +79,9 @@ static int find_deepest_state(struct cpu
 			      bool freeze)
 {
 	unsigned int latency_req = 0;
-	int i, ret = -ENXIO;
+	int i, ret = 0;
 
-	for (i = 0; i < drv->state_count; i++) {
+	for (i = 1; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
 		struct cpuidle_state_usage *su = &dev->states_usage[i];
 
@@ -243,7 +243,7 @@ int cpuidle_enter_state(struct cpuidle_d
  * @drv: the cpuidle driver
  * @dev: the cpuidle device
  *
- * Returns the index of the idle state.
+ * Returns the index of the idle state.  The return value must not be negative.
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -97,12 +97,6 @@ void default_idle_call(void)
 static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 		      int next_state)
 {
-	/* Fall back to the default arch idle method on errors. */
-	if (next_state < 0) {
-		default_idle_call();
-		return next_state;
-	}
-
 	/*
 	 * The idle task must be scheduled, it is pointless to go to idle, just
 	 * update no idle residency and return.

[PATCH 2/2] cpuidle: menu: Avoid pointless checks in menu_select()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] 

If menu_select() cannot find a suitable state to return, it will
return the state index stored in data->last_state_idx.  This
means that it is pointless to look at the states whose indices
are less than or equal to data->last_state_idx in the main loop,
so don't do that.

Given that those checks are done on every idle state selection, this
change can save quite a bit of completely unnecessary overhead.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/governors/menu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -342,7 +342,7 @@ static int menu_select(struct cpuidle_dr
 	 * Find the idle state with the lowest power while satisfying
 	 * our constraints.
 	 */
-	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
+	for (i = data->last_state_idx + 1; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
 		struct cpuidle_state_usage *su = &dev->states_usage[i];
 

Re: [PATCH 0/2] cpuidle optimizations (on top of linux-next)

[Sudeep Holla]

On 15/01/16 23:53, Rafael J. Wysocki wrote:
> Hi,
>
> When I was looking at the cpuidle code after the Sudeeps's problem report,
> it occured to me that we had some pointless overhead there, so two
> changes to reduce it follow.
>
> [1/2] Make the fallback to to default_idle_call() in call_cpuidle()
>        unnecessary and drop it.
> [2/2] Make menu_select() avoid checking states that don't need to
>        (or even shouldn't) be checked when making the selection.
>

Tested-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

Re: [PATCH 1/2] sched / idle: Drop default_idle_call() fallback from call_cpuidle()

[Peter Zijlstra]

On Sat, Jan 16, 2016 at 12:54:53AM +0100, Rafael J. Wysocki wrote:
> This eliminates one branch from the idle loop and makes the governors
> and find_deepest_state() handle the case when all states have been
> disabled from sysfs consistently.

> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -97,12 +97,6 @@ void default_idle_call(void)
>  static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>  		      int next_state)
>  {
> -	/* Fall back to the default arch idle method on errors. */
> -	if (next_state < 0) {
> -		default_idle_call();
> -		return next_state;
> -	}
> -
>  	/*
>  	 * The idle task must be scheduled, it is pointless to go to idle, just
>  	 * update no idle residency and return.


Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

[Resend][PATCH 2/2] cpuidle: menu: Avoid pointless checks in menu_select()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If menu_select() cannot find a suitable state to return, it will
return the state index stored in data->last_state_idx.  This
means that it is pointless to look at the states whose indices
are less than or equal to data->last_state_idx in the main loop,
so don't do that.

Given that those checks are done on every idle state selection, this
change can save quite a bit of completely unnecessary overhead.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Sudeep Holla <sudeep.holla@arm.com>
---

I left a pointless Subject: tag in the patch body when I sent it last time,
so resend.

Note that this also is based on commit 9c4b2867ed7c (cpuidle: menu: Fix
menu_select() for CPUIDLE_DRIVER_STATE_START == 0) only present in linux-next ATM.

---
 drivers/cpuidle/governors/menu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -342,7 +342,7 @@ static int menu_select(struct cpuidle_dr
 	 * Find the idle state with the lowest power while satisfying
 	 * our constraints.
 	 */
-	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
+	for (i = data->last_state_idx + 1; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
 		struct cpuidle_state_usage *su = &dev->states_usage[i];
 

Re: [PATCH 0/2] cpuidle optimizations (on top of linux-next)

[Ingo Molnar]

* Sudeep Holla <sudeep.holla@arm.com> wrote:

> On 15/01/16 23:53, Rafael J. Wysocki wrote:
> >Hi,
> >
> >When I was looking at the cpuidle code after the Sudeeps's problem report,
> >it occured to me that we had some pointless overhead there, so two
> >changes to reduce it follow.
> >
> >[1/2] Make the fallback to to default_idle_call() in call_cpuidle()
> >       unnecessary and drop it.
> >[2/2] Make menu_select() avoid checking states that don't need to
> >       (or even shouldn't) be checked when making the selection.
> >
> 
> Tested-by: Sudeep Holla <sudeep.holla@arm.com>

Rafael, can I pick these up into the scheduler tree?

Thanks,

	Ingo

Re: [PATCH 0/2] cpuidle optimizations (on top of linux-next)

[Rafael J. Wysocki]

On Tue, Jan 19, 2016 at 8:28 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>> On 15/01/16 23:53, Rafael J. Wysocki wrote:
>> >Hi,
>> >
>> >When I was looking at the cpuidle code after the Sudeeps's problem report,
>> >it occured to me that we had some pointless overhead there, so two
>> >changes to reduce it follow.
>> >
>> >[1/2] Make the fallback to to default_idle_call() in call_cpuidle()
>> >       unnecessary and drop it.
>> >[2/2] Make menu_select() avoid checking states that don't need to
>> >       (or even shouldn't) be checked when making the selection.
>> >
>>
>> Tested-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Rafael, can I pick these up into the scheduler tree?

They won't apply at this point as one commit they depend on is in my
linux-next branch waiting for the next push.

Would it be a problem if they went in through the PM tree instead?

Thanks,
Rafael

Re: [PATCH 0/2] cpuidle optimizations (on top of linux-next)

[Ingo Molnar]

* Rafael J. Wysocki <rafael@kernel.org> wrote:

> On Tue, Jan 19, 2016 at 8:28 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> >> On 15/01/16 23:53, Rafael J. Wysocki wrote:
> >> >Hi,
> >> >
> >> >When I was looking at the cpuidle code after the Sudeeps's problem report,
> >> >it occured to me that we had some pointless overhead there, so two
> >> >changes to reduce it follow.
> >> >
> >> >[1/2] Make the fallback to to default_idle_call() in call_cpuidle()
> >> >       unnecessary and drop it.
> >> >[2/2] Make menu_select() avoid checking states that don't need to
> >> >       (or even shouldn't) be checked when making the selection.
> >> >
> >>
> >> Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> >
> > Rafael, can I pick these up into the scheduler tree?
> 
> They won't apply at this point as one commit they depend on is in my
> linux-next branch waiting for the next push.
> 
> Would it be a problem if they went in through the PM tree instead?

Absolutely no problem:

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 0/2] cpuidle optimizations (on top of linux-next)

[Rafael J. Wysocki]

On Tuesday, January 19, 2016 02:28:58 PM Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rafael@kernel.org> wrote:
> 
> > On Tue, Jan 19, 2016 at 8:28 AM, Ingo Molnar <mingo@kernel.org> wrote:
> > >
> > > * Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > >> On 15/01/16 23:53, Rafael J. Wysocki wrote:
> > >> >Hi,
> > >> >
> > >> >When I was looking at the cpuidle code after the Sudeeps's problem report,
> > >> >it occured to me that we had some pointless overhead there, so two
> > >> >changes to reduce it follow.
> > >> >
> > >> >[1/2] Make the fallback to to default_idle_call() in call_cpuidle()
> > >> >       unnecessary and drop it.
> > >> >[2/2] Make menu_select() avoid checking states that don't need to
> > >> >       (or even shouldn't) be checked when making the selection.
> > >> >
> > >>
> > >> Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> > >
> > > Rafael, can I pick these up into the scheduler tree?
> > 
> > They won't apply at this point as one commit they depend on is in my
> > linux-next branch waiting for the next push.
> > 
> > Would it be a problem if they went in through the PM tree instead?
> 
> Absolutely no problem:
> 
> Acked-by: Ingo Molnar <mingo@kernel.org>

OK, thanks!

Rafael