Messages in this thread | | | Subject | Re: [RFC PATCH v2 0/6] Introduce Active Stats framework with CPU performance statistics | From | Lukasz Luba <> | Date | Tue, 6 Jul 2021 16:56:02 +0100 |
| |
On 7/6/21 4:28 PM, Rafael J. Wysocki wrote: > On Tue, Jul 6, 2021 at 3:18 PM Lukasz Luba <lukasz.luba@arm.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?
| |