linux-kernel - Re: [PATCH V6 2/3] cpuidle: play_idle: Specify play

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9dfe3bd3-1903-4dc4-0605-27cc867d87ad@linaro.org>
Date:   Fri, 8 Nov 2019 11:47:07 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:     mathieu.poirier@...aro.org, mingo@...hat.com, peterz@...radead.org,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        ulf.hansson@...aro.org
Subject: Re: [PATCH V6 2/3] cpuidle: play_idle: Specify play_idle with an idle
 state

On 08/11/2019 02:20, Rafael J. Wysocki wrote:
> On Wednesday, October 30, 2019 8:51:40 AM CET Daniel Lezcano wrote:
>> Currently, the play_idle function does not allow to tell which idle
>> state we want to go. Improve this by passing the idle state as
>> parameter to the function.
>>
>> Export cpuidle_find_deepest_state() symbol as it is used from the
>> intel_powerclamp driver as a module.
>>
>> There is no functional changes, the cpuidle state is the deepest one.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
>> Acked-by: Mathieu Poirier <mathieu.poirier@...aro.org>
>> Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>
>> ---
>>   V6:
>>    - Change variable name 'state' -> 'index':
>>      https://lkml.org/lkml/2019/10/28/874
>>   V4:
>>    - Add EXPORT_SYMBOL_GPL(cpuidle_find_deepest_state) for the
>>      intel_powerclamp driver when this one is compiled as a module
>>   V3:
>>    - Add missing cpuidle.h header
>> ---
>>  drivers/cpuidle/cpuidle.c                | 1 +
>>  drivers/powercap/idle_inject.c           | 4 +++-
>>  drivers/thermal/intel/intel_powerclamp.c | 4 +++-
>>  include/linux/cpu.h                      | 2 +-
>>  kernel/sched/idle.c                      | 4 ++--
>>  5 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index 18523ea6b11b..b871fc2e8e67 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -126,6 +126,7 @@ int cpuidle_find_deepest_state(void)
>>  
>>  	return find_deepest_state(drv, dev, UINT_MAX, 0, false);
>>  }
>> +EXPORT_SYMBOL_GPL(cpuidle_find_deepest_state);
> 
> That doesn't appear to be really necessary to me.
> 
>>  
>>  #ifdef CONFIG_SUSPEND
>>  static void enter_s2idle_proper(struct cpuidle_driver *drv,
>> diff --git a/drivers/powercap/idle_inject.c b/drivers/powercap/idle_inject.c
>> index cd1270614cc6..233c878cbf46 100644
>> --- a/drivers/powercap/idle_inject.c
>> +++ b/drivers/powercap/idle_inject.c
>> @@ -38,6 +38,7 @@
>>  #define pr_fmt(fmt) "ii_dev: " fmt
>>  
>>  #include <linux/cpu.h>
>> +#include <linux/cpuidle.h>
>>  #include <linux/hrtimer.h>
>>  #include <linux/kthread.h>
>>  #include <linux/sched.h>
>> @@ -138,7 +139,8 @@ static void idle_inject_fn(unsigned int cpu)
>>  	 */
>>  	iit->should_run = 0;
>>  
>> -	play_idle(READ_ONCE(ii_dev->idle_duration_us));
>> +	play_idle(READ_ONCE(ii_dev->idle_duration_us),
>> +		  cpuidle_find_deepest_state());
> 
> The next patch changes this again and I'm not sure why this intermediate
> change is useful.
> 
>>  }
>>  
>>  /**
>> diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c
>> index 53216dcbe173..b55786c169ae 100644
>> --- a/drivers/thermal/intel/intel_powerclamp.c
>> +++ b/drivers/thermal/intel/intel_powerclamp.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/kthread.h>
>>  #include <linux/cpu.h>
>> +#include <linux/cpuidle.h>
>>  #include <linux/thermal.h>
>>  #include <linux/slab.h>
>>  #include <linux/tick.h>
>> @@ -430,7 +431,8 @@ static void clamp_idle_injection_func(struct kthread_work *work)
>>  	if (should_skip)
>>  		goto balance;
>>  
>> -	play_idle(jiffies_to_usecs(w_data->duration_jiffies));
>> +	play_idle(jiffies_to_usecs(w_data->duration_jiffies),
>> +		  cpuidle_find_deepest_state());
> 
> I don't see a reason for changing the code here like this.
> 
> What you really need is to have a way to set a limit on the idle
> state exit latency for idle injection on ARM.

Mmh, yes you are right. The idle state number is part of the internals
of the cpuidle framework while the exit latency is an input (from user
or kernel).

> For that you can pass the exit latency limit to play_idle(), but then
> you need to change powerclamp to pass UNIT_MAX or similar which is
> ugly, or you can redefine cpuidle_use_deepest_state() to take the
> exit latency limit as the arg (with 0 meaning use_deepest_state == false).

Should it make sense to just get the resume latency in
cpuidle_use_deepest_state() and pass the value to find_deepest_state()?

It is the only code path where the constraint is not taken into account
AFAICT.

With this simple change, we can manage everything from the pm_qos API
then and this series is no longer needed.

> In the latter case, it would be quite straightforward to add an
> exit_latency argument to cpuidle_find_deepest_state() and note that
> find_deepest_state() takes a max_latency arg already, so that would be
> a trivial change (hint!).


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