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

Groups > linux.kernel > #1308654 > unrolled thread

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

Started bySudeep Holla <sudeep.holla@arm.com>
First post2016-01-13 18:30 +0100
Last post2016-01-14 11:40 +0100
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling Sudeep Holla <sudeep.holla@arm.com> - 2016-01-13 18:30 +0100
    Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling "Rafael J. Wysocki" <rafael@kernel.org> - 2016-01-13 23:00 +0100
      Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-13 23:10 +0100
        Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable  polling Sudeep Holla <sudeep.holla@arm.com> - 2016-01-14 11:50 +0100
          Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-15 01:50 +0100
      Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable  polling Sudeep Holla <sudeep.holla@arm.com> - 2016-01-14 11:40 +0100

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

FromSudeep Holla <sudeep.holla@arm.com>
Date2016-01-13 18:30 +0100
SubjectRe: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling
Message-ID<qQxp8-2W5-15@gated-at.bofh.it>
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

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)
                data->last_state_idx = CPUIDLE_DRIVER_STATE_START;

[toc] | [next] | [standalone]

#1308850

From"Rafael J. Wysocki" <rafael@kernel.org>
Date2016-01-13 23:00 +0100
Message-ID<qQBCq-5Gx-11@gated-at.bofh.it>
In reply to#1308654
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?

Do they crash or does something different happen?  If something
different happens, then what's that?

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

Thanks,
Rafael

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

#1308862

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2016-01-13 23:10 +0100
Message-ID<qQBM8-5Z4-61@gated-at.bofh.it>
In reply to#1308850
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@arm.com> 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

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

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

FromSudeep Holla <sudeep.holla@arm.com>
Date2016-01-14 11:50 +0100
SubjectRe: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling
Message-ID<qQNDA-5Lf-17@gated-at.bofh.it>
In reply to#1308862

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

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

#1309784

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2016-01-15 01:50 +0100
Message-ID<qR0Ku-6GB-3@gated-at.bofh.it>
In reply to#1309174
On Thursday, January 14, 2016 10:40:44 AM Sudeep Holla wrote:
> 
> 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.

No problem, thanks for testing!

Below it goes again with a changelog and all.

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] cpuidle: menu: Fix menu_select() for CPUIDLE_DRIVER_STATE_START == 0

Commit a9ceb78bc75c (cpuidle,menu: use interactivity_req to disable
polling) exposed a bug in menu_select() causing it to return -1
on systems with CPUIDLE_DRIVER_STATE_START equal to zero, although
it should have returned 0.  As a result, idle states are not entered
by CPUs on those systems.

Namely, on the systems in question data->last_state_idx is initially
equal to -1 and the above commit modified the condition that would
have caused it to be changed to 0 to be less likely to trigger which
exposed the problem.  However, setting data->last_state_idx initially
to -1 doesn't make sense at all and on the affected systems it should
always be set to CPUIDLE_DRIVER_STATE_START (ie. 0) unconditionally,
so make that happen.

Fixes: a9ceb78bc75c (cpuidle,menu: use interactivity_req to disable polling)
Reported-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 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

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

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

FromSudeep Holla <sudeep.holla@arm.com>
Date2016-01-14 11:40 +0100
SubjectRe: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling
Message-ID<qQNtU-5HN-21@gated-at.bofh.it>
In reply to#1308850

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web