Re: [PATCH V2 0/7] CPU cooling device new strategies

[Eduardo Valentin]

Hello Daniel,

On Wed, Feb 21, 2018 at 04:29:21PM +0100, Daniel Lezcano wrote:
> Changelog:
>   V2:
>      - Dropped the cpu combo cooling device
>      - Added the acked-by tags
>      - Replaced task array by a percpu structure
>      - Fixed the copyright dates
>      - Fixed the extra lines
>      - Fixed the compilation macros to be reused
>      - Fixed the list node removal
>      - Massaged a couple of function names
> 
> 
> The following series provides a new way to cool down a SoC by reducing
> the dissipated power on the CPUs. Based on the initial work from Kevin
> Wangtao, the series implements a CPU cooling device based on idle
> injection, relying on the cpuidle framework.

Nice! Glad to see that Linaro took this work again. I have a few
questions, as follows.

> 
> The patchset is designed to have the current DT binding for the
> cpufreq cooling device to be compatible with the new cooling devices.
> 
> Different cpu cooling devices can not co-exist on the system, the cpu
> cooling device is enabled or not, and one cooling strategy is selected
> (cpufreq or cpuidle). It is not possible to have all of them available
> at the same time. However, the goal is to enable them all and be able
> to switch from one to another at runtime but that needs a rework of the
> thermal framework which is orthogonal to the feature we are providing.
> 

Can you please elaborate on the limitations you found? Please be more
specific.

> This series is divided into two parts.
> 
> The first part just provides trivial changes for the copyright and
> removes an unused field in the cpu freq cooling device structure.
> 

Ok..

> The second part provides the idle injection cooling device, allowing a SoC
> without a cpufreq driver to use this cooling device as an alternative.
> 

which is awesome!


> The preliminary benchmarks show the following changes:
> 
> On the hikey6220, dhrystone shows a throughtput increase of 40% for an
> increase of the latency of 16% while sysbench shows a latency increase
> of 5%.

I don't follow these numbers. Throughput increase while injecting idle?
compared to what? percentages of what? Please be more specific to better
describer your work..

> 
> Initially, the first version provided also the cpuidle + cpufreq combo
> cooling device but regarding the latest comments, there is a misfit with
> how the cpufreq cooling device and suspend/resume/cpu hotplug/module
> loading|unloading behave together while the combo cooling device was
> designed assuming the cpufreq cooling device was always there. This
> dynamic is investigated and the combo cooling device will be posted
> separetely after this series gets merged.

Yeah, this is one of the confusing parts. Could you please
remind us of the limitations here? Why can't we enable CPUfreq
on higher trip points and CPUidle on lower trip points, for example?
Specially from a system design point of view, the system engineer
typically would benefit of idle injections to achieve overall
average CPU frequencies in a more granular fashion, for example,
achieving performance vs. cooling between available real
frequencies, avoiding real switches.

Also, there is a major design question here. After Linaro's attempt
to send a cpufreq+cpuidle cooling device(s), there was an attempt
to generalize and extend intel powerclamp driver. Do you mind
explaining why refactoring intel powerclamp is not possible? Basic
idea is the same, no?



> 
> Daniel Lezcano (7):
>   thermal/drivers/cpu_cooling: Fixup the header and copyright
>   thermal/drivers/cpu_cooling: Add Software Package Data Exchange (SPDX)
>   thermal/drivers/cpu_cooling: Remove pointless field
>   thermal/drivers/Kconfig: Convert the CPU cooling device to a choice
>   thermal/drivers/cpu_cooling: Add idle cooling device documentation
>   thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver
>   cpuidle/drivers/cpuidle-arm: Register the cooling device
> 
>  Documentation/thermal/cpu-idle-cooling.txt | 165 ++++++++++
>  drivers/cpuidle/cpuidle-arm.c              |   5 +
>  drivers/thermal/Kconfig                    |  30 +-
>  drivers/thermal/cpu_cooling.c              | 480 +++++++++++++++++++++++++++--
>  include/linux/cpu_cooling.h                |  15 +-
>  5 files changed, 668 insertions(+), 27 deletions(-)
>  create mode 100644 Documentation/thermal/cpu-idle-cooling.txt
> 
> -- 
> 2.7.4
>