Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling

[Rafael J. Wysocki]

On Wednesday, January 13, 2016 10:58:13 PM Rafael J. Wysocki wrote:
> Hi,
> 
> On Wed, Jan 13, 2016 at 6:27 PM, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> > Hi Rik,

[cut]

> >
> > diff --git i/drivers/cpuidle/governors/menu.c w/drivers/cpuidle/governors/menu.c
> > index 7b0971d97cc3..7c7f4059705a 100644
> > --- i/drivers/cpuidle/governors/menu.c
> > +++ w/drivers/cpuidle/governors/menu.c
> > @@ -330,7 +330,8 @@ static int menu_select(struct cpuidle_driver *drv,
> > struct cpuidle_device *dev)
> >          * We want to default to C1 (hlt), not to busy polling
> >          * unless the timer is happening really really soon.
> >          */
> > -       if (interactivity_req > 20 &&
> > +       if (((interactivity_req && interactivity_req > 20) ||
> 
> Well, if interactivity_req > 20, then it also is different from 0, so
> the first check should not be necessary here.
> 
> > +               data->next_timer_us > 20) &&
> 
> I guess that this simply restores the previous behavior on the
> platforms in question.
> 
> Now, the reason why it may matter is because
> CPUIDLE_DRIVER_STATE_START is 0 and so data->last_state_idx may end up
> as -1 on them.  So I think this piece of code only ever makes sense if
> CPUIDLE_DRIVER_STATE_START is 1.
> 
> >             !drv->states[CPUIDLE_DRIVER_STATE_START].disabled &&
> >                 dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0)
> >                 data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
> > --

So I'm wondering if the appended patch helps by any chance?

Rafael

---
 drivers/cpuidle/governors/menu.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -294,8 +294,6 @@ static int menu_select(struct cpuidle_dr
 		data->needs_update = 0;
 	}
 
-	data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1;
-
 	/* Special case when user has set very strict latency requirement */
 	if (unlikely(latency_req == 0))
 		return 0;
@@ -326,14 +324,19 @@ static int menu_select(struct cpuidle_dr
 	if (latency_req > interactivity_req)
 		latency_req = interactivity_req;
 
-	/*
-	 * We want to default to C1 (hlt), not to busy polling
-	 * unless the timer is happening really really soon.
-	 */
-	if (interactivity_req > 20 &&
-	    !drv->states[CPUIDLE_DRIVER_STATE_START].disabled &&
-		dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0)
+	if (CPUIDLE_DRIVER_STATE_START > 0) {
+		data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1;
+		/*
+		 * We want to default to C1 (hlt), not to busy polling
+		 * unless the timer is happening really really soon.
+		 */
+		if (interactivity_req > 20 &&
+		    !drv->states[CPUIDLE_DRIVER_STATE_START].disabled &&
+			dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0)
+			data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
+	} else {
 		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
+	}
 
 	/*
 	 * Find the idle state with the lowest power while satisfying