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@xxxxxxxxx>
> 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@xxxxxxxxx>
> ---
> 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
