diff mbox

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

Message ID CAPKp9ubg3V0979dGTGi+kjXGiBczCG2+QBGgfrZBKjMgSuhy-A@mail.gmail.com
State New
Headers show

Commit Message

Sudeep Holla Jan. 13, 2016, 5:27 p.m. UTC 
Hi Rik,

This change break idle on ARM64(may be on other ARM?) platform.
Sorry for reporting late, but missed to check cpuidle in -next.

On Tue, Nov 3, 2015 at 10:34 PM,  <riel@redhat.com> wrote:
> From: Rik van Riel <riel@redhat.com>

>

> The menu governor carefully figures out how much time we typically

> sleep for an estimated sleep interval, or whether there is a repeating

> pattern going on, and corrects that estimate for the CPU load.

>

> Then it proceeds to ignore that information when determining whether

> or not to consider polling. This is not a big deal on most x86 CPUs,

> which have very low C1 latencies, and the patch should not have any

> effect on those CPUs.

>

> However, certain CPUs (eg. Atom) have much higher C1 latencies, and

> it would be good to not waste performance and power on those CPUs if

> we are expecting a very low wakeup latency.

>

> Disable polling based on the estimated interactivity requirement, not

> on the time to the next timer interrupt.

>

> Signed-off-by: Rik van Riel <riel@redhat.com>

> ---

>  drivers/cpuidle/governors/menu.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c

> index ecc242a586c9..b1a55731f921 100644

> --- a/drivers/cpuidle/governors/menu.c

> +++ b/drivers/cpuidle/governors/menu.c

> @@ -330,7 +330,7 @@ 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 (data->next_timer_us > 20 &&

> +       if (interactivity_req > 20 &&


I found that data->predicted_us is gets overwritten in get_typical_interval
when avg computed = 0 which is the case initially on boot when the past
intervals are not yet accumulated.

I just tried a hack and that seem to work and proved what I anticipated
(i.e. interactivity_req = 0). Let me know if you have any clues on how to
solve it ? I can help you getting the change tested.

Regards,
Sudeep

--->8

                data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Sudeep Holla Jan. 14, 2016, 10:33 a.m. UTC | #1 
On 13/01/16 21:58, Rafael J. Wysocki wrote:
> Hi,

>

> On Wed, Jan 13, 2016 at 6:27 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

>> Hi Rik,

>>

>> This change break idle on ARM64(may be on other ARM?) platform.

>> Sorry for reporting late, but missed to check cpuidle in -next.

>

> OK, so first of all, how exactly is idle broken on those systems?

>


Sorry for the hasty bug report.

> Do they crash or does something different happen?  If something

> different happens, then what's that?

>


No they just don't enter any idle states(as if CPUIdle was disabled)

[...]

>>

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

>


True, I just wanted to avoid using interactivity_req when 0, it was just
a quick hack.

>> +               data->next_timer_us > 20) &&

>

> I guess that this simply restores the previous behavior on the

> platforms in question.

>


Yes.

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

>


That's correct.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla Jan. 14, 2016, 10:40 a.m. UTC | #2 
On 13/01/16 22:07, Rafael J. Wysocki wrote:
> On Wednesday, January 13, 2016 10:58:13 PM Rafael J. Wysocki wrote:


[..]

>

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

>


Yes it does fix. As you mentioned earlier, CPUIDLE_DRIVER_STATE_START is
0 on ARM platforms and hence data->last_state_idx ended up as -1.

You can add by Tested-by when you push this change. Thanks for the quick
fix.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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) ||
+               data->next_timer_us > 20) &&
            !drv->states[CPUIDLE_DRIVER_STATE_START].disabled &&
                dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0)