* [PATCH] cpuidle: don't wakeup processor when set a longer latency @ 2013-05-08 2:44 Lianwei Wang 2013-05-08 11:08 ` [linux-pm] " Daniel Lezcano 0 siblings, 1 reply; 9+ messages in thread From: Lianwei Wang @ 2013-05-08 2:44 UTC (permalink / raw) To: linux-kernel, linux-pm [-- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency 2013-05-08 2:44 [PATCH] cpuidle: don't wakeup processor when set a longer latency Lianwei Wang @ 2013-05-08 11:08 ` Daniel Lezcano 2013-05-09 7:14 ` Lianwei Wang 0 siblings, 1 reply; 9+ messages in thread From: Daniel Lezcano @ 2013-05-08 11:08 UTC (permalink / raw) To: Lianwei Wang; +Cc: linux-kernel, linux-pm 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency 2013-05-08 11:08 ` [linux-pm] " Daniel Lezcano @ 2013-05-09 7:14 ` Lianwei Wang 2013-05-09 23:45 ` Daniel Lezcano 0 siblings, 1 reply; 9+ messages in thread From: Lianwei Wang @ 2013-05-09 7:14 UTC (permalink / raw) To: Daniel Lezcano; +Cc: linux-kernel, linux-pm [-- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency 2013-05-09 7:14 ` Lianwei Wang @ 2013-05-09 23:45 ` Daniel Lezcano 2013-05-13 6:52 ` Lianwei Wang 0 siblings, 1 reply; 9+ messages in thread From: Daniel Lezcano @ 2013-05-09 23:45 UTC (permalink / raw) To: Lianwei Wang; +Cc: linux-kernel, linux-pm 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency 2013-05-09 23:45 ` Daniel Lezcano @ 2013-05-13 6:52 ` Lianwei Wang 2013-05-13 7:46 ` Lianwei Wang 2013-05-13 9:04 ` Srivatsa S. Bhat 0 siblings, 2 replies; 9+ messages in thread From: Lianwei Wang @ 2013-05-13 6:52 UTC (permalink / raw) To: Daniel Lezcano; +Cc: linux-kernel, linux-pm [-- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency 2013-05-13 6:52 ` Lianwei Wang @ 2013-05-13 7:46 ` Lianwei Wang 2013-05-13 9:04 ` Srivatsa S. Bhat 1 sibling, 0 replies; 9+ messages in thread From: Lianwei Wang @ 2013-05-13 7:46 UTC (permalink / raw) To: Daniel Lezcano; +Cc: linux-kernel, linux-pm [-- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency 2013-05-13 6:52 ` Lianwei Wang 2013-05-13 7:46 ` Lianwei Wang @ 2013-05-13 9:04 ` Srivatsa S. Bhat 2013-05-13 15:25 ` Daniel Lezcano 1 sibling, 1 reply; 9+ messages in thread From: Srivatsa S. Bhat @ 2013-05-13 9:04 UTC (permalink / raw) To: Lianwei Wang; +Cc: Daniel Lezcano, linux-pm, linux-kernel, Rafael J. Wysocki 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency 2013-05-13 9:04 ` Srivatsa S. Bhat @ 2013-05-13 15:25 ` Daniel Lezcano 2013-05-13 20:27 ` Srivatsa S. Bhat 0 siblings, 1 reply; 9+ messages in thread From: Daniel Lezcano @ 2013-05-13 15:25 UTC (permalink / raw) To: Srivatsa S. Bhat; +Cc: Lianwei Wang, linux-pm, linux-kernel, Rafael J. Wysocki 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set a longer latency 2013-05-13 15:25 ` Daniel Lezcano @ 2013-05-13 20:27 ` Srivatsa S. Bhat 0 siblings, 0 replies; 9+ messages in thread From: Srivatsa S. Bhat @ 2013-05-13 20:27 UTC (permalink / raw) To: Daniel Lezcano; +Cc: Lianwei Wang, linux-pm, linux-kernel, Rafael J. Wysocki 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 >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-05-13 20:30 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-05-08 2:44 [PATCH] cpuidle: don't wakeup processor when set a longer latency Lianwei Wang 2013-05-08 11:08 ` [linux-pm] " Daniel Lezcano 2013-05-09 7:14 ` Lianwei Wang 2013-05-09 23:45 ` Daniel Lezcano 2013-05-13 6:52 ` Lianwei Wang 2013-05-13 7:46 ` Lianwei Wang 2013-05-13 9:04 ` Srivatsa S. Bhat 2013-05-13 15:25 ` Daniel Lezcano 2013-05-13 20:27 ` Srivatsa S. Bhat
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox