Re: [linux-pm] [PATCH] cpuidle: don't wakeup processor when set alonger 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@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?

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


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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/