[PATCH] cpuidle: don't wakeup processor when set a longer latency

[PATCH] cpuidle: don't wakeup processor when set a longer latency

[Lianwei Wang]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

When a PM-Qos is updated, the cpuidle driver will wakeup all the CPUs
no matter what a latency is set. But actually it only need to wakeup
the CPUs when a shorter latency is set. In this way we can reduce the
cpu wakeup count and save battery.

So we can pass the prev_value to the notifier callback and check the
latency curr_value and prev_value in the cpuidle latency notifier
callback. It modify a common interface(dummy --> prev_value) but shall
be safe since no one use the dummy parameter currently.

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index e1f6860..1e1758c 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -498,7 +498,11 @@ static void smp_callback(void *v)
 static int cpuidle_latency_notify(struct notifier_block *b,
                unsigned long l, void *v)
 {
-       smp_call_function(smp_callback, NULL, 1);
+       unsigned long prev_value = (unsigned long) v;
+
+       /* Dont't waktup processor when set a longer latency */
+       if (l < prev_value)
+               smp_call_function(smp_callback, NULL, 1);
        return NOTIFY_OK;
 }

diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 9322ff7..533b8bc 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -205,7 +205,7 @@ int pm_qos_update_target(struct pm_qos_constraints
*c, struct plist_node *node,
        if (prev_value != curr_value) {
                blocking_notifier_call_chain(c->notifiers,
                                             (unsigned long)curr_value,
-                                            NULL);
+                                            (void *)prev_value);
                return 1;
        } else {
                return 0;

[-- Attachment #2: 0001-cpuidle-don-t-wakeup-processor-when-set-a-longer-lat.patch --]
[-- Type: application/octet-stream, Size: 1708 bytes --]

From 00d5e2ddb1c56f36358be290c2104e9feec84465 Mon Sep 17 00:00:00 2001
From: Lianwei Wang <lianwei.wang@gmail.com>
Date: Fri, 26 Apr 2013 10:59:24 +0800
Subject: [PATCH] cpuidle: don't wakeup processor when set a longer latency

We don't need wakeup the other CPUs immediately if a longer latency
is set. Pass the prev_value to the notifier callback so the latency
can be checked in cpuidle driver and only wakeup all the other CPUs
when a shorter latency is set.

Change-Id: If564fd0d9c53cf100bd85247bfd509dfeaf54c13
Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
---
 drivers/cpuidle/cpuidle.c |    6 +++++-
 kernel/power/qos.c        |    2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index e1f6860..1e1758c 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -498,7 +498,11 @@ static void smp_callback(void *v)
 static int cpuidle_latency_notify(struct notifier_block *b,
 		unsigned long l, void *v)
 {
-	smp_call_function(smp_callback, NULL, 1);
+	unsigned long prev_value = (unsigned long) v;
+
+	/* Dont't waktup processor when set a longer latency */
+	if (l < prev_value)
+		smp_call_function(smp_callback, NULL, 1);
 	return NOTIFY_OK;
 }
 
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 9322ff7..533b8bc 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -205,7 +205,7 @@ int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
 	if (prev_value != curr_value) {
 		blocking_notifier_call_chain(c->notifiers,
 					     (unsigned long)curr_value,
-					     NULL);
+					     (void *)prev_value);
 		return 1;
 	} else {
 		return 0;
-- 
1.7.4.1

Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency

[Daniel Lezcano]

On 05/08/2013 04:44 AM, Lianwei Wang wrote:
> When a PM-Qos is updated, the cpuidle driver will wakeup all the CPUs
> no matter what a latency is set. But actually it only need to wakeup
> the CPUs when a shorter latency is set. In this way we can reduce the
> cpu wakeup count and save battery.

I am curious, how many times could the pm_qos be changed in a system
live cycle to measure an improvement with this patch ?

Do you have a scenario where you measured a noticeable power saving ?

> So we can pass the prev_value to the notifier callback and check the
> latency curr_value and prev_value in the cpuidle latency notifier
> callback. It modify a common interface(dummy --> prev_value) but shall
> be safe since no one use the dummy parameter currently.
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index e1f6860..1e1758c 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -498,7 +498,11 @@ static void smp_callback(void *v)
>  static int cpuidle_latency_notify(struct notifier_block *b,
>                 unsigned long l, void *v)
>  {
> -       smp_call_function(smp_callback, NULL, 1);
> +       unsigned long prev_value = (unsigned long) v;
> +
> +       /* Dont't waktup processor when set a longer latency */

                    ^^^^^^
                    wakeup

Instead of passing prev and curr, using the dummy variable, why don't
you pass the result of (curr - prev) ?

A negative value means, the latency is smaller and positive is bigger.

Also, may be the optimization could be more improved: if the latency is
bigger than the next wakeup event, it is not necessary to wakeup the cpus.

> +       if (l < prev_value)
> +               smp_call_function(smp_callback, NULL, 1);
>         return NOTIFY_OK;
>  }
> 
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 9322ff7..533b8bc 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -205,7 +205,7 @@ int pm_qos_update_target(struct pm_qos_constraints
> *c, struct plist_node *node,
>         if (prev_value != curr_value) {
>                 blocking_notifier_call_chain(c->notifiers,
>                                              (unsigned long)curr_value,
> -                                            NULL);
> +                                            (void *)prev_value);
>                 return 1;
>         } else {
>                 return 0;
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency

[Lianwei Wang]

[-- Attachment #1: Type: text/plain, Size: 4593 bytes --]

Thank you very much. I have a quick updated patch based on your comments.

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 2f0083a..cd1af4b 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -18,6 +18,7 @@
 #include <linux/ktime.h>
 #include <linux/hrtimer.h>
 #include <linux/module.h>
+#include <linux/tick.h>
 #include <trace/events/power.h>

 #include "cpuidle.h"
@@ -466,7 +467,20 @@ static void smp_callback(void *v)
 static int cpuidle_latency_notify(struct notifier_block *b,
                unsigned long l, void *v)
 {
-       smp_call_function(smp_callback, NULL, 1);
+       int cpu, rcpu = smp_processor_id();
+       s64 s;
+       struct tick_device *td;
+
+       for_each_online_cpu(cpu) {
+               if (cpu == rcpu)
+                       continue;
+               td = tick_get_device(cpu);
+               s = ktime_us_delta(td->evtdev->next_event, ktime_get());
+               if ((long)l < (long)s) {
+                       smp_call_function_single(cpu, smp_callback, NULL, 1);
+               }
+       }
+
        return NOTIFY_OK;
 }

Thanks,
Lianwei

2013/5/8 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 05/08/2013 04:44 AM, Lianwei Wang wrote:
>> When a PM-Qos is updated, the cpuidle driver will wakeup all the CPUs
>> no matter what a latency is set. But actually it only need to wakeup
>> the CPUs when a shorter latency is set. In this way we can reduce the
>> cpu wakeup count and save battery.
>
> I am curious, how many times could the pm_qos be changed in a system
> live cycle to measure an improvement with this patch ?
>
> Do you have a scenario where you measured a noticeable power saving ?
>
The PM-Qos is not updated most of time, especially for home idle case.
 But for some specific case, the PM-Qos may update too frequently.
(E.g. my measurement show that it is changed frequently between
2us/3us/200us/200s for bootup and usb case.) The battery current drain
is measured from PMIC or battery eliminator. Although this is just a
little saving, it is still reasonable to improve it.

>> So we can pass the prev_value to the notifier callback and check the
>> latency curr_value and prev_value in the cpuidle latency notifier
>> callback. It modify a common interface(dummy --> prev_value) but shall
>> be safe since no one use the dummy parameter currently.
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index e1f6860..1e1758c 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -498,7 +498,11 @@ static void smp_callback(void *v)
>>  static int cpuidle_latency_notify(struct notifier_block *b,
>>                 unsigned long l, void *v)
>>  {
>> -       smp_call_function(smp_callback, NULL, 1);
>> +       unsigned long prev_value = (unsigned long) v;
>> +
>> +       /* Dont't waktup processor when set a longer latency */
>
>                     ^^^^^^
>                     wakeup
>
> Instead of passing prev and curr, using the dummy variable, why don't
> you pass the result of (curr - prev) ?
>
> A negative value means, the latency is smaller and positive is bigger.
>
> Also, may be the optimization could be more improved: if the latency is
> bigger than the next wakeup event, it is not necessary to wakeup the cpus.
>
This is good idea. So it need to check the next_event on each CPU and
wakeup the cpu if the requested latency is smaller than it. A quick
patch is attached.

>> +       if (l < prev_value)
>> +               smp_call_function(smp_callback, NULL, 1);
>>         return NOTIFY_OK;
>>  }
>>
>> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
>> index 9322ff7..533b8bc 100644
>> --- a/kernel/power/qos.c
>> +++ b/kernel/power/qos.c
>> @@ -205,7 +205,7 @@ int pm_qos_update_target(struct pm_qos_constraints
>> *c, struct plist_node *node,
>>         if (prev_value != curr_value) {
>>                 blocking_notifier_call_chain(c->notifiers,
>>                                              (unsigned long)curr_value,
>> -                                            NULL);
>> +                                            (void *)prev_value);
>>                 return 1;
>>         } else {
>>                 return 0;
>>
>
>
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

[-- Attachment #2: 0001-cpuidle-wakeup-processor-on-a-smaller-latency.patch --]
[-- Type: application/octet-stream, Size: 1481 bytes --]

From 07a3f329340975a3f3e830d343f7a2c6e83a441f Mon Sep 17 00:00:00 2001
From: Lianwei Wang <lianwei.wang@gmail.com>
Date: Fri, 26 Apr 2013 10:59:24 +0800
Subject: [PATCH] cpuidle: wakeup processor on a smaller latency

Checking the PM-Qos latency and cpu idle sleep latency, and only
wakeup the cpu if the requested PM-Qos latency is smaller than its
idle sleep latency.

Change-Id: If564fd0d9c53cf100bd85247bfd509dfeaf54c13
Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
---
 drivers/cpuidle/cpuidle.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 2f0083a..cd1af4b 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -18,6 +18,7 @@
 #include <linux/ktime.h>
 #include <linux/hrtimer.h>
 #include <linux/module.h>
+#include <linux/tick.h>
 #include <trace/events/power.h>
 
 #include "cpuidle.h"
@@ -466,7 +467,20 @@ static void smp_callback(void *v)
 static int cpuidle_latency_notify(struct notifier_block *b,
 		unsigned long l, void *v)
 {
-	smp_call_function(smp_callback, NULL, 1);
+	int cpu, rcpu = smp_processor_id();
+	s64 s;
+	struct tick_device *td;
+
+	for_each_online_cpu(cpu) {
+		if (cpu == rcpu)
+			continue;
+		td = tick_get_device(cpu);
+		s = ktime_us_delta(td->evtdev->next_event, ktime_get());
+		if ((long)l < (long)s) {
+			smp_call_function_single(cpu, smp_callback, NULL, 1);
+		}
+	}
+
 	return NOTIFY_OK;
 }
 
-- 
1.7.4.1

Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency

[Daniel Lezcano]

On 05/09/2013 09:14 AM, Lianwei Wang wrote:
> Thank you very much. I have a quick updated patch based on your comments.
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 2f0083a..cd1af4b 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -18,6 +18,7 @@
>  #include <linux/ktime.h>
>  #include <linux/hrtimer.h>
>  #include <linux/module.h>
> +#include <linux/tick.h>
>  #include <trace/events/power.h>
> 
>  #include "cpuidle.h"
> @@ -466,7 +467,20 @@ static void smp_callback(void *v)
>  static int cpuidle_latency_notify(struct notifier_block *b,
>                 unsigned long l, void *v)
>  {
> -       smp_call_function(smp_callback, NULL, 1);
> +       int cpu, rcpu = smp_processor_id();
> +       s64 s;
> +       struct tick_device *td;
> +
> +       for_each_online_cpu(cpu) {
> +               if (cpu == rcpu)
> +                       continue;
> +               td = tick_get_device(cpu);
> +               s = ktime_us_delta(td->evtdev->next_event, ktime_get());
> +               if ((long)l < (long)s) {
> +                       smp_call_function_single(cpu, smp_callback, NULL, 1);
> +               }
> +       }
> +
>         return NOTIFY_OK;
>  }

The patch sounds reasonable. A comment and explicit names for the
variables would be nice.

eg.
l => latency
s => sleep

> Thanks,
> Lianwei
> 
> 2013/5/8 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 05/08/2013 04:44 AM, Lianwei Wang wrote:
>>> When a PM-Qos is updated, the cpuidle driver will wakeup all the CPUs
>>> no matter what a latency is set. But actually it only need to wakeup
>>> the CPUs when a shorter latency is set. In this way we can reduce the
>>> cpu wakeup count and save battery.
>>
>> I am curious, how many times could the pm_qos be changed in a system
>> live cycle to measure an improvement with this patch ?
>>
>> Do you have a scenario where you measured a noticeable power saving ?
>>
> The PM-Qos is not updated most of time, especially for home idle case.
>  But for some specific case, the PM-Qos may update too frequently.
> (E.g. my measurement show that it is changed frequently between
> 2us/3us/200us/200s for bootup and usb case.) The battery current drain
> is measured from PMIC or battery eliminator. Although this is just a
> little saving, it is still reasonable to improve it.

Thanks for the information. Can you add this information in the changelog ?

>>> So we can pass the prev_value to the notifier callback and check the
>>> latency curr_value and prev_value in the cpuidle latency notifier
>>> callback. It modify a common interface(dummy --> prev_value) but shall
>>> be safe since no one use the dummy parameter currently.
>>>
>>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>>> index e1f6860..1e1758c 100644
>>> --- a/drivers/cpuidle/cpuidle.c
>>> +++ b/drivers/cpuidle/cpuidle.c
>>> @@ -498,7 +498,11 @@ static void smp_callback(void *v)
>>>  static int cpuidle_latency_notify(struct notifier_block *b,
>>>                 unsigned long l, void *v)
>>>  {
>>> -       smp_call_function(smp_callback, NULL, 1);
>>> +       unsigned long prev_value = (unsigned long) v;
>>> +
>>> +       /* Dont't waktup processor when set a longer latency */
>>
>>                     ^^^^^^
>>                     wakeup
>>
>> Instead of passing prev and curr, using the dummy variable, why don't
>> you pass the result of (curr - prev) ?
>>
>> A negative value means, the latency is smaller and positive is bigger.
>>
>> Also, may be the optimization could be more improved: if the latency is
>> bigger than the next wakeup event, it is not necessary to wakeup the cpus.
>>
> This is good idea. So it need to check the next_event on each CPU and
> wakeup the cpu if the requested latency is smaller than it. A quick
> patch is attached.

Yes, it sounds good.

>>> +       if (l < prev_value)
>>> +               smp_call_function(smp_callback, NULL, 1);
>>>         return NOTIFY_OK;
>>>  }
>>>
>>> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
>>> index 9322ff7..533b8bc 100644
>>> --- a/kernel/power/qos.c
>>> +++ b/kernel/power/qos.c
>>> @@ -205,7 +205,7 @@ int pm_qos_update_target(struct pm_qos_constraints
>>> *c, struct plist_node *node,
>>>         if (prev_value != curr_value) {
>>>                 blocking_notifier_call_chain(c->notifiers,
>>>                                              (unsigned long)curr_value,
>>> -                                            NULL);
>>> +                                            (void *)prev_value);
>>>                 return 1;
>>>         } else {
>>>                 return 0;
>>>
>>
>>
>> --
>>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
>>


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency

[Lianwei Wang]

[-- Attachment #1: Type: text/plain, Size: 7929 bytes --]

Thank you. Patch is updated.

>From 2d0b4afb5461847dcdf08a87b02015d061b12e85 Mon Sep 17 00:00:00 2001
From: Lianwei Wang <lianwei.wang@gmail.com>
Date: Fri, 26 Apr 2013 10:59:24 +0800
Subject: [PATCH] cpuidle: wakeup processor on a smaller latency

Checking the PM-Qos latency and cpu idle sleep latency, and only
wakeup the cpu if the requested PM-Qos latency is smaller than its
idle sleep latency. This can reduce at least 50% cpu wakeup count
on PM-Qos updated.

The PM-Qos is not updated most of time, especially for home idle
case. But for some specific case, the PM-Qos may be updated too
frequently. (E.g. my measurement show that it is changed frequently
between 2us/3us/200us/200s for bootup and usb case.)

The battery current drain is measured from PMIC or battery eliminator.
Although this is just a little saving, it is still reasonable to
improve it.

Change-Id: If564fd0d9c53cf100bd85247bfd509dfeaf54c13
Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
---
 drivers/cpuidle/cpuidle.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 2f0083a..a0829ad 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -18,6 +18,7 @@
 #include <linux/ktime.h>
 #include <linux/hrtimer.h>
 #include <linux/module.h>
+#include <linux/tick.h>
 #include <trace/events/power.h>

 #include "cpuidle.h"
@@ -462,11 +463,25 @@ static void smp_callback(void *v)
  * requirement.  This means we need to get all processors out of their C-state,
  * and then recalculate a new suitable C-state. Just do a cross-cpu IPI; that
  * wakes them all right up.
+ * l - > latency in us
  */
 static int cpuidle_latency_notify(struct notifier_block *b,
                unsigned long l, void *v)
 {
-       smp_call_function(smp_callback, NULL, 1);
+       int cpu, rcpu = smp_processor_id();
+       s64 s; /* sleep_length in us */
+       struct tick_device *td;
+
+       for_each_online_cpu(cpu) {
+               if (cpu == rcpu)
+                       continue;
+               td = tick_get_device(cpu);
+               s = ktime_us_delta(td->evtdev->next_event, ktime_get());
+               if ((long)l < (long)s) {
+                       smp_call_function_single(cpu, smp_callback, NULL, 1);
+               }
+       }
+
        return NOTIFY_OK;
 }

--
1.7.4.1

2013/5/10 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 05/09/2013 09:14 AM, Lianwei Wang wrote:
>> Thank you very much. I have a quick updated patch based on your comments.
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index 2f0083a..cd1af4b 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/ktime.h>
>>  #include <linux/hrtimer.h>
>>  #include <linux/module.h>
>> +#include <linux/tick.h>
>>  #include <trace/events/power.h>
>>
>>  #include "cpuidle.h"
>> @@ -466,7 +467,20 @@ static void smp_callback(void *v)
>>  static int cpuidle_latency_notify(struct notifier_block *b,
>>                 unsigned long l, void *v)
>>  {
>> -       smp_call_function(smp_callback, NULL, 1);
>> +       int cpu, rcpu = smp_processor_id();
>> +       s64 s;
>> +       struct tick_device *td;
>> +
>> +       for_each_online_cpu(cpu) {
>> +               if (cpu == rcpu)
>> +                       continue;
>> +               td = tick_get_device(cpu);
>> +               s = ktime_us_delta(td->evtdev->next_event, ktime_get());
>> +               if ((long)l < (long)s) {
>> +                       smp_call_function_single(cpu, smp_callback, NULL, 1);
>> +               }
>> +       }
>> +
>>         return NOTIFY_OK;
>>  }
>
> The patch sounds reasonable. A comment and explicit names for the
> variables would be nice.
>
> eg.
> l => latency
> s => sleep
>
>> Thanks,
>> Lianwei
>>
>> 2013/5/8 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>> On 05/08/2013 04:44 AM, Lianwei Wang wrote:
>>>> When a PM-Qos is updated, the cpuidle driver will wakeup all the CPUs
>>>> no matter what a latency is set. But actually it only need to wakeup
>>>> the CPUs when a shorter latency is set. In this way we can reduce the
>>>> cpu wakeup count and save battery.
>>>
>>> I am curious, how many times could the pm_qos be changed in a system
>>> live cycle to measure an improvement with this patch ?
>>>
>>> Do you have a scenario where you measured a noticeable power saving ?
>>>
>> The PM-Qos is not updated most of time, especially for home idle case.
>>  But for some specific case, the PM-Qos may update too frequently.
>> (E.g. my measurement show that it is changed frequently between
>> 2us/3us/200us/200s for bootup and usb case.) The battery current drain
>> is measured from PMIC or battery eliminator. Although this is just a
>> little saving, it is still reasonable to improve it.
>
> Thanks for the information. Can you add this information in the changelog ?
>
>>>> So we can pass the prev_value to the notifier callback and check the
>>>> latency curr_value and prev_value in the cpuidle latency notifier
>>>> callback. It modify a common interface(dummy --> prev_value) but shall
>>>> be safe since no one use the dummy parameter currently.
>>>>
>>>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>>>> index e1f6860..1e1758c 100644
>>>> --- a/drivers/cpuidle/cpuidle.c
>>>> +++ b/drivers/cpuidle/cpuidle.c
>>>> @@ -498,7 +498,11 @@ static void smp_callback(void *v)
>>>>  static int cpuidle_latency_notify(struct notifier_block *b,
>>>>                 unsigned long l, void *v)
>>>>  {
>>>> -       smp_call_function(smp_callback, NULL, 1);
>>>> +       unsigned long prev_value = (unsigned long) v;
>>>> +
>>>> +       /* Dont't waktup processor when set a longer latency */
>>>
>>>                     ^^^^^^
>>>                     wakeup
>>>
>>> Instead of passing prev and curr, using the dummy variable, why don't
>>> you pass the result of (curr - prev) ?
>>>
>>> A negative value means, the latency is smaller and positive is bigger.
>>>
>>> Also, may be the optimization could be more improved: if the latency is
>>> bigger than the next wakeup event, it is not necessary to wakeup the cpus.
>>>
>> This is good idea. So it need to check the next_event on each CPU and
>> wakeup the cpu if the requested latency is smaller than it. A quick
>> patch is attached.
>
> Yes, it sounds good.
>
>>>> +       if (l < prev_value)
>>>> +               smp_call_function(smp_callback, NULL, 1);
>>>>         return NOTIFY_OK;
>>>>  }
>>>>
>>>> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
>>>> index 9322ff7..533b8bc 100644
>>>> --- a/kernel/power/qos.c
>>>> +++ b/kernel/power/qos.c
>>>> @@ -205,7 +205,7 @@ int pm_qos_update_target(struct pm_qos_constraints
>>>> *c, struct plist_node *node,
>>>>         if (prev_value != curr_value) {
>>>>                 blocking_notifier_call_chain(c->notifiers,
>>>>                                              (unsigned long)curr_value,
>>>> -                                            NULL);
>>>> +                                            (void *)prev_value);
>>>>                 return 1;
>>>>         } else {
>>>>                 return 0;
>>>>
>>>
>>>
>>> --
>>>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>>
>>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>>> <http://twitter.com/#!/linaroorg> Twitter |
>>> <http://www.linaro.org/linaro-blog/> Blog
>>>
>
>
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

[-- Attachment #2: 0001-cpuidle-wakeup-processor-on-a-smaller-latency.patch --]
[-- Type: application/octet-stream, Size: 2191 bytes --]

From 2d0b4afb5461847dcdf08a87b02015d061b12e85 Mon Sep 17 00:00:00 2001
From: Lianwei Wang <lianwei.wang@gmail.com>
Date: Fri, 26 Apr 2013 10:59:24 +0800
Subject: [PATCH] cpuidle: wakeup processor on a smaller latency

Checking the PM-Qos latency and cpu idle sleep latency, and only
wakeup the cpu if the requested PM-Qos latency is smaller than its
idle sleep latency. This can reduce at least 50% cpu wakeup count
on PM-Qos updated.

The PM-Qos is not updated most of time, especially for home idle
case. But for some specific case, the PM-Qos may be updated too
frequently. (E.g. my measurement show that it is changed frequently
between 2us/3us/200us/200s for bootup and usb case.)

The battery current drain is measured from PMIC or battery eliminator.
Although this is just a little saving, it is still reasonable to
improve it.

Change-Id: If564fd0d9c53cf100bd85247bfd509dfeaf54c13
Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
---
 drivers/cpuidle/cpuidle.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 2f0083a..a0829ad 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -18,6 +18,7 @@
 #include <linux/ktime.h>
 #include <linux/hrtimer.h>
 #include <linux/module.h>
+#include <linux/tick.h>
 #include <trace/events/power.h>
 
 #include "cpuidle.h"
@@ -462,11 +463,25 @@ static void smp_callback(void *v)
  * requirement.  This means we need to get all processors out of their C-state,
  * and then recalculate a new suitable C-state. Just do a cross-cpu IPI; that
  * wakes them all right up.
+ * l - > latency in us
  */
 static int cpuidle_latency_notify(struct notifier_block *b,
 		unsigned long l, void *v)
 {
-	smp_call_function(smp_callback, NULL, 1);
+	int cpu, rcpu = smp_processor_id();
+	s64 s; /* sleep_length in us */
+	struct tick_device *td;
+
+	for_each_online_cpu(cpu) {
+		if (cpu == rcpu)
+			continue;
+		td = tick_get_device(cpu);
+		s = ktime_us_delta(td->evtdev->next_event, ktime_get());
+		if ((long)l < (long)s) {
+			smp_call_function_single(cpu, smp_callback, NULL, 1);
+		}
+	}
+
 	return NOTIFY_OK;
 }
 
-- 
1.7.4.1

Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency

[Lianwei Wang]

[-- Attachment #1: Type: text/plain, Size: 10184 bytes --]

smp_call_function_single may not good for quad or more cores because
it just send one interrupt one time and wait there...
Update the patch to wakeup the cpus at the same time by calling
smp_call_function_many.

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 2f0083a..e1f618f 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -18,6 +18,7 @@
 #include <linux/ktime.h>
 #include <linux/hrtimer.h>
 #include <linux/module.h>
+#include <linux/tick.h>
 #include <trace/events/power.h>

 #include "cpuidle.h"
@@ -462,11 +463,34 @@ static void smp_callback(void *v)
  * requirement.  This means we need to get all processors out of their C-state,
  * and then recalculate a new suitable C-state. Just do a cross-cpu IPI; that
  * wakes them all right up.
+ * l - > latency in us
  */
 static int cpuidle_latency_notify(struct notifier_block *b,
                unsigned long l, void *v)
 {
-       smp_call_function(smp_callback, NULL, 1);
+       cpumask_var_t cpus;
+
+       if (!alloc_cpumask_var(&cpus, GFP_ATOMIC))
+               smp_call_function(smp_callback, NULL, 1);
+       else {
+               int cpu, rcpu = smp_processor_id();
+               s64 s; /* sleep_length in us */
+               struct tick_device *td;
+
+               for_each_online_cpu(cpu) {
+                       if (cpu == rcpu)
+                               continue;
+                       td = tick_get_device(cpu);
+                       s = ktime_us_delta(td->evtdev->next_event, ktime_get());
+                       if ((long)l < (long)s)
+                               cpumask_set_cpu(cpu, cpus);
+               }
+               preempt_disable();
+               smp_call_function_many(cpus, smp_callback, NULL, 1);
+               preempt_enable();
+               free_cpumask_var(cpus);
+       }
+
        return NOTIFY_OK;
 }

2013/5/13 Lianwei Wang <lianwei.wang@gmail.com>:
> Thank you. Patch is updated.
>
> From 2d0b4afb5461847dcdf08a87b02015d061b12e85 Mon Sep 17 00:00:00 2001
> From: Lianwei Wang <lianwei.wang@gmail.com>
> Date: Fri, 26 Apr 2013 10:59:24 +0800
> Subject: [PATCH] cpuidle: wakeup processor on a smaller latency
>
> Checking the PM-Qos latency and cpu idle sleep latency, and only
> wakeup the cpu if the requested PM-Qos latency is smaller than its
> idle sleep latency. This can reduce at least 50% cpu wakeup count
> on PM-Qos updated.
>
> The PM-Qos is not updated most of time, especially for home idle
> case. But for some specific case, the PM-Qos may be updated too
> frequently. (E.g. my measurement show that it is changed frequently
> between 2us/3us/200us/200s for bootup and usb case.)
>
> The battery current drain is measured from PMIC or battery eliminator.
> Although this is just a little saving, it is still reasonable to
> improve it.
>
> Change-Id: If564fd0d9c53cf100bd85247bfd509dfeaf54c13
> Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
> ---
>  drivers/cpuidle/cpuidle.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 2f0083a..a0829ad 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -18,6 +18,7 @@
>  #include <linux/ktime.h>
>  #include <linux/hrtimer.h>
>  #include <linux/module.h>
> +#include <linux/tick.h>
>  #include <trace/events/power.h>
>
>  #include "cpuidle.h"
> @@ -462,11 +463,25 @@ static void smp_callback(void *v)
>   * requirement.  This means we need to get all processors out of their C-state,
>   * and then recalculate a new suitable C-state. Just do a cross-cpu IPI; that
>   * wakes them all right up.
> + * l - > latency in us
>   */
>  static int cpuidle_latency_notify(struct notifier_block *b,
>                 unsigned long l, void *v)
>  {
> -       smp_call_function(smp_callback, NULL, 1);
> +       int cpu, rcpu = smp_processor_id();
> +       s64 s; /* sleep_length in us */
> +       struct tick_device *td;
> +
> +       for_each_online_cpu(cpu) {
> +               if (cpu == rcpu)
> +                       continue;
> +               td = tick_get_device(cpu);
> +               s = ktime_us_delta(td->evtdev->next_event, ktime_get());
> +               if ((long)l < (long)s) {
> +                       smp_call_function_single(cpu, smp_callback, NULL, 1);
> +               }
> +       }
> +
>         return NOTIFY_OK;
>  }
>
> --
> 1.7.4.1
>
> 2013/5/10 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 05/09/2013 09:14 AM, Lianwei Wang wrote:
>>> Thank you very much. I have a quick updated patch based on your comments.
>>>
>>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>>> index 2f0083a..cd1af4b 100644
>>> --- a/drivers/cpuidle/cpuidle.c
>>> +++ b/drivers/cpuidle/cpuidle.c
>>> @@ -18,6 +18,7 @@
>>>  #include <linux/ktime.h>
>>>  #include <linux/hrtimer.h>
>>>  #include <linux/module.h>
>>> +#include <linux/tick.h>
>>>  #include <trace/events/power.h>
>>>
>>>  #include "cpuidle.h"
>>> @@ -466,7 +467,20 @@ static void smp_callback(void *v)
>>>  static int cpuidle_latency_notify(struct notifier_block *b,
>>>                 unsigned long l, void *v)
>>>  {
>>> -       smp_call_function(smp_callback, NULL, 1);
>>> +       int cpu, rcpu = smp_processor_id();
>>> +       s64 s;
>>> +       struct tick_device *td;
>>> +
>>> +       for_each_online_cpu(cpu) {
>>> +               if (cpu == rcpu)
>>> +                       continue;
>>> +               td = tick_get_device(cpu);
>>> +               s = ktime_us_delta(td->evtdev->next_event, ktime_get());
>>> +               if ((long)l < (long)s) {
>>> +                       smp_call_function_single(cpu, smp_callback, NULL, 1);
>>> +               }
>>> +       }
>>> +
>>>         return NOTIFY_OK;
>>>  }
>>
>> The patch sounds reasonable. A comment and explicit names for the
>> variables would be nice.
>>
>> eg.
>> l => latency
>> s => sleep
>>
>>> Thanks,
>>> Lianwei
>>>
>>> 2013/5/8 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>>> On 05/08/2013 04:44 AM, Lianwei Wang wrote:
>>>>> When a PM-Qos is updated, the cpuidle driver will wakeup all the CPUs
>>>>> no matter what a latency is set. But actually it only need to wakeup
>>>>> the CPUs when a shorter latency is set. In this way we can reduce the
>>>>> cpu wakeup count and save battery.
>>>>
>>>> I am curious, how many times could the pm_qos be changed in a system
>>>> live cycle to measure an improvement with this patch ?
>>>>
>>>> Do you have a scenario where you measured a noticeable power saving ?
>>>>
>>> The PM-Qos is not updated most of time, especially for home idle case.
>>>  But for some specific case, the PM-Qos may update too frequently.
>>> (E.g. my measurement show that it is changed frequently between
>>> 2us/3us/200us/200s for bootup and usb case.) The battery current drain
>>> is measured from PMIC or battery eliminator. Although this is just a
>>> little saving, it is still reasonable to improve it.
>>
>> Thanks for the information. Can you add this information in the changelog ?
>>
>>>>> So we can pass the prev_value to the notifier callback and check the
>>>>> latency curr_value and prev_value in the cpuidle latency notifier
>>>>> callback. It modify a common interface(dummy --> prev_value) but shall
>>>>> be safe since no one use the dummy parameter currently.
>>>>>
>>>>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>>>>> index e1f6860..1e1758c 100644
>>>>> --- a/drivers/cpuidle/cpuidle.c
>>>>> +++ b/drivers/cpuidle/cpuidle.c
>>>>> @@ -498,7 +498,11 @@ static void smp_callback(void *v)
>>>>>  static int cpuidle_latency_notify(struct notifier_block *b,
>>>>>                 unsigned long l, void *v)
>>>>>  {
>>>>> -       smp_call_function(smp_callback, NULL, 1);
>>>>> +       unsigned long prev_value = (unsigned long) v;
>>>>> +
>>>>> +       /* Dont't waktup processor when set a longer latency */
>>>>
>>>>                     ^^^^^^
>>>>                     wakeup
>>>>
>>>> Instead of passing prev and curr, using the dummy variable, why don't
>>>> you pass the result of (curr - prev) ?
>>>>
>>>> A negative value means, the latency is smaller and positive is bigger.
>>>>
>>>> Also, may be the optimization could be more improved: if the latency is
>>>> bigger than the next wakeup event, it is not necessary to wakeup the cpus.
>>>>
>>> This is good idea. So it need to check the next_event on each CPU and
>>> wakeup the cpu if the requested latency is smaller than it. A quick
>>> patch is attached.
>>
>> Yes, it sounds good.
>>
>>>>> +       if (l < prev_value)
>>>>> +               smp_call_function(smp_callback, NULL, 1);
>>>>>         return NOTIFY_OK;
>>>>>  }
>>>>>
>>>>> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
>>>>> index 9322ff7..533b8bc 100644
>>>>> --- a/kernel/power/qos.c
>>>>> +++ b/kernel/power/qos.c
>>>>> @@ -205,7 +205,7 @@ int pm_qos_update_target(struct pm_qos_constraints
>>>>> *c, struct plist_node *node,
>>>>>         if (prev_value != curr_value) {
>>>>>                 blocking_notifier_call_chain(c->notifiers,
>>>>>                                              (unsigned long)curr_value,
>>>>> -                                            NULL);
>>>>> +                                            (void *)prev_value);
>>>>>                 return 1;
>>>>>         } else {
>>>>>                 return 0;
>>>>>
>>>>
>>>>
>>>> --
>>>>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>>>
>>>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>>>> <http://twitter.com/#!/linaroorg> Twitter |
>>>> <http://www.linaro.org/linaro-blog/> Blog
>>>>
>>
>>
>> --
>>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
>>

[-- Attachment #2: 0001-cpuidle-wakeup-processor-on-a-smaller-latency.patch --]
[-- Type: application/octet-stream, Size: 2431 bytes --]

From 30c2639346a3fab2df3eac7ded37404d818802e4 Mon Sep 17 00:00:00 2001
From: Lianwei Wang <lianwei.wang@gmail.com>
Date: Fri, 26 Apr 2013 10:59:24 +0800
Subject: [PATCH] cpuidle: wakeup processor on a smaller latency

Checking the PM-Qos latency and cpu idle sleep latency, and only
wakeup the cpu if the requested PM-Qos latency is smaller than its
idle sleep latency. This can reduce at least 50% cpu wakeup count
on PM-Qos updated.

The PM-Qos is not updated most of time, especially for home idle
case. But for some specific case, the PM-Qos may be updated too
frequently. (E.g. my measurement show that it is changed frequently
between 2us/3us/200us/200s for bootup and usb case.)

The battery current drain is measured from PMIC or battery eliminator.
Although this is just a little saving, it is still reasonable to
improve it.

Change-Id: If564fd0d9c53cf100bd85247bfd509dfeaf54c13
Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
---
 drivers/cpuidle/cpuidle.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 2f0083a..e1f618f 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -18,6 +18,7 @@
 #include <linux/ktime.h>
 #include <linux/hrtimer.h>
 #include <linux/module.h>
+#include <linux/tick.h>
 #include <trace/events/power.h>
 
 #include "cpuidle.h"
@@ -462,11 +463,34 @@ static void smp_callback(void *v)
  * requirement.  This means we need to get all processors out of their C-state,
  * and then recalculate a new suitable C-state. Just do a cross-cpu IPI; that
  * wakes them all right up.
+ * l - > latency in us
  */
 static int cpuidle_latency_notify(struct notifier_block *b,
 		unsigned long l, void *v)
 {
-	smp_call_function(smp_callback, NULL, 1);
+	cpumask_var_t cpus;
+
+	if (!alloc_cpumask_var(&cpus, GFP_ATOMIC))
+		smp_call_function(smp_callback, NULL, 1);
+	else {
+		int cpu, rcpu = smp_processor_id();
+		s64 s; /* sleep_length in us */
+		struct tick_device *td;
+
+		for_each_online_cpu(cpu) {
+			if (cpu == rcpu)
+				continue;
+			td = tick_get_device(cpu);
+			s = ktime_us_delta(td->evtdev->next_event, ktime_get());
+			if ((long)l < (long)s)
+				cpumask_set_cpu(cpu, cpus);
+		}
+		preempt_disable();
+		smp_call_function_many(cpus, smp_callback, NULL, 1);
+		preempt_enable();
+		free_cpumask_var(cpus);
+	}
+
 	return NOTIFY_OK;
 }
 
-- 
1.7.4.1

Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency

[Srivatsa S. Bhat]

On 05/13/2013 12:22 PM, Lianwei Wang wrote:
> Thank you. Patch is updated.
> 
> From 2d0b4afb5461847dcdf08a87b02015d061b12e85 Mon Sep 17 00:00:00 2001
> From: Lianwei Wang <lianwei.wang@gmail.com>
> Date: Fri, 26 Apr 2013 10:59:24 +0800
> Subject: [PATCH] cpuidle: wakeup processor on a smaller latency
> 
> Checking the PM-Qos latency and cpu idle sleep latency, and only
> wakeup the cpu if the requested PM-Qos latency is smaller than its
> idle sleep latency. This can reduce at least 50% cpu wakeup count
> on PM-Qos updated.
>
> The PM-Qos is not updated most of time, especially for home idle
> case. But for some specific case, the PM-Qos may be updated too
> frequently. (E.g. my measurement show that it is changed frequently
> between 2us/3us/200us/200s for bootup and usb case.)
> 
> The battery current drain is measured from PMIC or battery eliminator.
> Although this is just a little saving, it is still reasonable to
> improve it.
>

I don't understand how this patch is supposed to improve things.

IIUC, if a CPU was sleeping for a short duration, and you set the latency
requirement for a longer value, this patch will avoid waking that CPU.
But how does that help? Perhaps, during the short sleep, the CPU was
actually in a shallow (less power-saving) sleep state, and hence it might
actually be better off waking it up and then putting it into a much
deeper sleep state no?

And if we ignore the sleep length for a moment, in the case that you
set a very strict latency requirement and then later relax the constraint,
does it not make sense to wake up the CPUs and allow them to go to
deeper sleep states?

And IMHO there are other problems with this patch as well, see below.

> Change-Id: If564fd0d9c53cf100bd85247bfd509dfeaf54c13
> Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
> ---
>  drivers/cpuidle/cpuidle.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 2f0083a..a0829ad 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -18,6 +18,7 @@
>  #include <linux/ktime.h>
>  #include <linux/hrtimer.h>
>  #include <linux/module.h>
> +#include <linux/tick.h>
>  #include <trace/events/power.h>
> 
>  #include "cpuidle.h"
> @@ -462,11 +463,25 @@ static void smp_callback(void *v)
>   * requirement.  This means we need to get all processors out of their C-state,
>   * and then recalculate a new suitable C-state. Just do a cross-cpu IPI; that
>   * wakes them all right up.
> + * l - > latency in us
>   */
>  static int cpuidle_latency_notify(struct notifier_block *b,
>                 unsigned long l, void *v)
>  {
> -       smp_call_function(smp_callback, NULL, 1);
> +       int cpu, rcpu = smp_processor_id();

This is not atomic context. So your rcpu is not guaranteed to remain valid
(because you can get migrated to another cpu).

> +       s64 s; /* sleep_length in us */
> +       struct tick_device *td;
> +
> +       for_each_online_cpu(cpu) {

You need to protect against CPU hotplug, by using get/put_online_cpus().

> +               if (cpu == rcpu)
> +                       continue;
> +               td = tick_get_device(cpu);
> +               s = ktime_us_delta(td->evtdev->next_event, ktime_get());

What happens if that wakeup event got cancelled just after this? And hence
the CPU sleeps longer than expected... In that case, you'll be violating
the latency requirement set by the user, no?

Moreover, looking at the menu and ladder cpu idle governors, the value set
in cpu_dma_latency is used to compare with the *exit-latency* of the sleep
state in order to decide which sleep state to go to. IOW, it has got *nothing*
to do with the duration of the sleep!!

> +               if ((long)l < (long)s) {

... and hence, this check doesn't make sense at all!

> +                       smp_call_function_single(cpu, smp_callback, NULL, 1);
> +               }
> +       }
> +
>         return NOTIFY_OK;
>  }
> 

Regards,
Srivatsa S. Bhat

Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency

[Daniel Lezcano]

On 05/13/2013 11:04 AM, Srivatsa S. Bhat wrote:
> On 05/13/2013 12:22 PM, Lianwei Wang wrote:
>> Thank you. Patch is updated.
>>
>> From 2d0b4afb5461847dcdf08a87b02015d061b12e85 Mon Sep 17 00:00:00 2001
>> From: Lianwei Wang <lianwei.wang@gmail.com>
>> Date: Fri, 26 Apr 2013 10:59:24 +0800
>> Subject: [PATCH] cpuidle: wakeup processor on a smaller latency
>>
>> Checking the PM-Qos latency and cpu idle sleep latency, and only
>> wakeup the cpu if the requested PM-Qos latency is smaller than its
>> idle sleep latency. This can reduce at least 50% cpu wakeup count
>> on PM-Qos updated.
>>
>> The PM-Qos is not updated most of time, especially for home idle
>> case. But for some specific case, the PM-Qos may be updated too
>> frequently. (E.g. my measurement show that it is changed frequently
>> between 2us/3us/200us/200s for bootup and usb case.)
>>
>> The battery current drain is measured from PMIC or battery eliminator.
>> Although this is just a little saving, it is still reasonable to
>> improve it.
>>
> 
> I don't understand how this patch is supposed to improve things.
> 
> IIUC, if a CPU was sleeping for a short duration, and you set the latency
> requirement for a longer value, this patch will avoid waking that CPU.
> But how does that help? Perhaps, during the short sleep, the CPU was
> actually in a shallow (less power-saving) sleep state, and hence it might
> actually be better off waking it up and then putting it into a much
> deeper sleep state no?

Yes, that is a good point. But I am wondering if what you described
could happen with the commit 69a37beabf1f0a6705c08e879bdd5d82ff6486c4.

If a non-deepest idle state is chosen by the menu governor, a timer will
expire right after the target residency and wakeup the cpu. That will
re-evaluate the c-state.

> And if we ignore the sleep length for a moment, in the case that you
> set a very strict latency requirement and then later relax the constraint,
> does it not make sense to wake up the CPUs and allow them to go to
> deeper sleep states?
> 
> And IMHO there are other problems with this patch as well, see below.
> 
>> Change-Id: If564fd0d9c53cf100bd85247bfd509dfeaf54c13
>> Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
>> ---
>>  drivers/cpuidle/cpuidle.c |   17 ++++++++++++++++-
>>  1 files changed, 16 insertions(+), 1 deletions(-)
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index 2f0083a..a0829ad 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/ktime.h>
>>  #include <linux/hrtimer.h>
>>  #include <linux/module.h>
>> +#include <linux/tick.h>
>>  #include <trace/events/power.h>
>>
>>  #include "cpuidle.h"
>> @@ -462,11 +463,25 @@ static void smp_callback(void *v)
>>   * requirement.  This means we need to get all processors out of their C-state,
>>   * and then recalculate a new suitable C-state. Just do a cross-cpu IPI; that
>>   * wakes them all right up.
>> + * l - > latency in us
>>   */
>>  static int cpuidle_latency_notify(struct notifier_block *b,
>>                 unsigned long l, void *v)
>>  {
>> -       smp_call_function(smp_callback, NULL, 1);
>> +       int cpu, rcpu = smp_processor_id();
> 
> This is not atomic context. So your rcpu is not guaranteed to remain valid
> (because you can get migrated to another cpu).
> 
>> +       s64 s; /* sleep_length in us */
>> +       struct tick_device *td;
>> +
>> +       for_each_online_cpu(cpu) {
> 
> You need to protect against CPU hotplug, by using get/put_online_cpus().
> 
>> +               if (cpu == rcpu)
>> +                       continue;
>> +               td = tick_get_device(cpu);
>> +               s = ktime_us_delta(td->evtdev->next_event, ktime_get());
> 
> What happens if that wakeup event got cancelled just after this? And hence
> the CPU sleeps longer than expected... In that case, you'll be violating
> the latency requirement set by the user, no?
> 
> Moreover, looking at the menu and ladder cpu idle governors, the value set
> in cpu_dma_latency is used to compare with the *exit-latency* of the sleep
> state in order to decide which sleep state to go to. IOW, it has got *nothing*
> to do with the duration of the sleep!!
> 
>> +               if ((long)l < (long)s) {
> 
> ... and hence, this check doesn't make sense at all!
> 
>> +                       smp_call_function_single(cpu, smp_callback, NULL, 1);
>> +               }
>> +       }
>> +
>>         return NOTIFY_OK;
>>  }
>>
> 
> Regards,
> Srivatsa S. Bhat
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency

[Srivatsa S. Bhat]

On 05/13/2013 08:55 PM, Daniel Lezcano wrote:
> On 05/13/2013 11:04 AM, Srivatsa S. Bhat wrote:
>> On 05/13/2013 12:22 PM, Lianwei Wang wrote:
>>> Thank you. Patch is updated.
>>>
>>> From 2d0b4afb5461847dcdf08a87b02015d061b12e85 Mon Sep 17 00:00:00 2001
>>> From: Lianwei Wang <lianwei.wang@gmail.com>
>>> Date: Fri, 26 Apr 2013 10:59:24 +0800
>>> Subject: [PATCH] cpuidle: wakeup processor on a smaller latency
>>>
>>> Checking the PM-Qos latency and cpu idle sleep latency, and only
>>> wakeup the cpu if the requested PM-Qos latency is smaller than its
>>> idle sleep latency. This can reduce at least 50% cpu wakeup count
>>> on PM-Qos updated.
>>>
>>> The PM-Qos is not updated most of time, especially for home idle
>>> case. But for some specific case, the PM-Qos may be updated too
>>> frequently. (E.g. my measurement show that it is changed frequently
>>> between 2us/3us/200us/200s for bootup and usb case.)
>>>
>>> The battery current drain is measured from PMIC or battery eliminator.
>>> Although this is just a little saving, it is still reasonable to
>>> improve it.
>>>
>>
>> I don't understand how this patch is supposed to improve things.
>>
>> IIUC, if a CPU was sleeping for a short duration, and you set the latency
>> requirement for a longer value, this patch will avoid waking that CPU.
>> But how does that help? Perhaps, during the short sleep, the CPU was
>> actually in a shallow (less power-saving) sleep state, and hence it might
>> actually be better off waking it up and then putting it into a much
>> deeper sleep state no?
> 
> Yes, that is a good point. But I am wondering if what you described
> could happen with the commit 69a37beabf1f0a6705c08e879bdd5d82ff6486c4.
> 
> If a non-deepest idle state is chosen by the menu governor, a timer will
> expire right after the target residency and wakeup the cpu. That will
> re-evaluate the c-state.
> 

Yeah, that commit adds a good safety-check to ensure that we don't suffer
too much loss in power-savings due to mis-predictions in the menu governor.

But I don't think this particular patch helps. So, IMHO, we should leave
that cpu_dma_latency handler as it is.

Regards,
Srivatsa S. Bhat

>> And if we ignore the sleep length for a moment, in the case that you
>> set a very strict latency requirement and then later relax the constraint,
>> does it not make sense to wake up the CPUs and allow them to go to
>> deeper sleep states?
>>
>> And IMHO there are other problems with this patch as well, see below.
>>
>>> Change-Id: If564fd0d9c53cf100bd85247bfd509dfeaf54c13
>>> Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
>>> ---
>>>  drivers/cpuidle/cpuidle.c |   17 ++++++++++++++++-
>>>  1 files changed, 16 insertions(+), 1 deletions(-)
>>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>>> index 2f0083a..a0829ad 100644
>>> --- a/drivers/cpuidle/cpuidle.c
>>> +++ b/drivers/cpuidle/cpuidle.c
>>> @@ -18,6 +18,7 @@
>>>  #include <linux/ktime.h>
>>>  #include <linux/hrtimer.h>
>>>  #include <linux/module.h>
>>> +#include <linux/tick.h>
>>>  #include <trace/events/power.h>
>>>
>>>  #include "cpuidle.h"
>>> @@ -462,11 +463,25 @@ static void smp_callback(void *v)
>>>   * requirement.  This means we need to get all processors out of their C-state,
>>>   * and then recalculate a new suitable C-state. Just do a cross-cpu IPI; that
>>>   * wakes them all right up.
>>> + * l - > latency in us
>>>   */
>>>  static int cpuidle_latency_notify(struct notifier_block *b,
>>>                 unsigned long l, void *v)
>>>  {
>>> -       smp_call_function(smp_callback, NULL, 1);
>>> +       int cpu, rcpu = smp_processor_id();
>>
>> This is not atomic context. So your rcpu is not guaranteed to remain valid
>> (because you can get migrated to another cpu).
>>
>>> +       s64 s; /* sleep_length in us */
>>> +       struct tick_device *td;
>>> +
>>> +       for_each_online_cpu(cpu) {
>>
>> You need to protect against CPU hotplug, by using get/put_online_cpus().
>>
>>> +               if (cpu == rcpu)
>>> +                       continue;
>>> +               td = tick_get_device(cpu);
>>> +               s = ktime_us_delta(td->evtdev->next_event, ktime_get());
>>
>> What happens if that wakeup event got cancelled just after this? And hence
>> the CPU sleeps longer than expected... In that case, you'll be violating
>> the latency requirement set by the user, no?
>>
>> Moreover, looking at the menu and ladder cpu idle governors, the value set
>> in cpu_dma_latency is used to compare with the *exit-latency* of the sleep
>> state in order to decide which sleep state to go to. IOW, it has got *nothing*
>> to do with the duration of the sleep!!
>>
>>> +               if ((long)l < (long)s) {
>>
>> ... and hence, this check doesn't make sense at all!
>>
>>> +                       smp_call_function_single(cpu, smp_callback, NULL, 1);
>>> +               }
>>> +       }
>>> +
>>>         return NOTIFY_OK;
>>>  }
>>>
>>
>> Regards,
>> Srivatsa S. Bhat
>>
>