linux-kernel - Re: [RFC PATCH v2 0/6] Introduce Active Stats framework with CPU performance statistics

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: <1de9d474-fdeb-8db6-0b01-53a90f7c20c8@arm.com>
Date:   Tue, 6 Jul 2021 16:56:02 +0100
From:   Lukasz Luba <lukasz.luba@....com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Amit Kucheria <amitk@...nel.org>,
        "Zhang, Rui" <rui.zhang@...el.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Chris Redpath <Chris.Redpath@....com>, Beata.Michalska@....com,
        Viresh Kumar <viresh.kumar@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Amit Kachhap <amit.kachhap@...il.com>
Subject: Re: [RFC PATCH v2 0/6] Introduce Active Stats framework with CPU
 performance statistics



On 7/6/21 4:28 PM, Rafael J. Wysocki wrote:
> On Tue, Jul 6, 2021 at 3:18 PM Lukasz Luba <lukasz.luba@....com> wrote:
>>
>> Hi all,
>>
>> This patch set introduces a new mechanism: Active Stats framework (ASF), which
>> gathers and maintains statistics of CPU performance - time residency at each
>> performance state.
>>
>> The ASF tracks all the frequency transitions as well as CPU
>> idle entry/exit events for all CPUs. Based on that it accounts the active
>> (non-idle) residency time for each CPU at each frequency. This information can
>> be used by some other subsystems (like thermal governor) to enhance their
>> estimations about CPU usage at a given period.
> 
> This seems to mean that what is needed is something like the cpufreq
> stats but only collected during the time when CPUs are not in idle
> states.

Yes

> 
>> Does it fix something in mainline?
>> Yes, there is thermal governor Intelligent Power Allocation (IPA), which
>> estimates the CPUs power used in the past. IPA is sampling the CPU utilization
>> and frequency and relies on the info available at the time of sampling
>> and this imposes the estimation errors.
>> The use of ASF solve the issue and enables IPA to make better estimates.
> 
> Obviously the IPA is not used on all platforms where cpufreq and
> cpuidle are used.  What platforms are going to benefit from this
> change?

Arm platforms which still use kernel thermal to control temperature,
such as Chromebooks or mid-, low-end phones.

> 
>> Why it couldn't be done using existing frameworks?
>> The CPUFreq and CPUIdle statistics are not combined, so it is not possible
>> to derive the information on how long exactly the CPU was running with a given
>> frequency.
> 
> But it doesn't mean that the statistics could not be combined.
> 
> For instance, the frequency of the CPU cannot change via cpufreq when
> active_stats_cpu_idle_enter() is running, so instead of using an
> entirely new framework for collecting statistics it might update the
> existing cpufreq stats to register that event.

True, but keep in mind that the cpufreq most likely works for a few
CPUs (policy::related_cpus), while cpuidle in a per-cpu fashion.
I would say that cpuidle should check during enter/exit what is
the currently set frequency for cluster and account its active
period.

> 
> And analogously for the wakeup.
> 
>> This new framework combines that information and provides
>> it in a handy way.
> 
> I'm not convinced about the last piece.

The handy structure is called Active Stats Monitor. It samples
the stats gathered after processing idle. That private
structure maintains statistics which are for a given period
(current snapshot - previous snapshot).

> 
>> IMHO it has to be implemented as a new framework, next to
>> CPUFreq and CPUIdle, due to a clean design and not just hooks from thermal
>> governor into the frequency change and idle code paths.
> 
> As far as the design is concerned, I'm not sure if I agree with it.
> 
>  From my perspective it's all a 1000-line patch that I have to read and
> understand to figure out what the design is.

I can help you with understanding it with some design docs if you want.

> 
>> Tha patch 4/6 introduces a new API for cooling devices, which allows to
>> stop tracking the freq and idle statistics.
>>
>> The patch set contains also a patches 5/6 6/6 which adds the new power model
>> based on ASF into the cpufreq cooling (used by thermal governor IPA).
>> It is added as ifdef option, since Active Stats might be not compiled in.
>> The ASF is a compile time option, but that might be changed and IPA could
>> select it, which would allow to remove some redundant code from
>> cpufreq_cooling.c.
>>
>> Comments and suggestions are very welcome.
> 
> I'm totally not convinced that it is necessary to put the extra 1000
> lines of code into the kernel to address the problem at hand.
> 

I understand your concerns. If you have another idea than this framework
I'm happy to hear it. Maybe better stats in cpuidle, which would be
are of the cpufreq?