Message-ID: <CAJZ5v0i3UUKxQbcfTVWHOeje-LTOnuAMda3FX0D5sqHLXEs3Xg@mail.gmail.com>
Date:   Wed, 14 Jul 2021 20:30:33 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Lukasz Luba <lukasz.luba@....com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        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

Sorry for the delay.

On Tue, Jul 6, 2021 at 6:51 PM Lukasz Luba <lukasz.luba@....com> wrote:
>
>
>
> On 7/6/21 5:34 PM, Rafael J. Wysocki wrote:
> > On Tue, Jul 6, 2021 at 5:56 PM Lukasz Luba <lukasz.luba@....com> wrote:
> >>
> >> 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
> >
> > So this is a clear problem statement: the cpufreq statistics cover the
> > time when CPUs are in idle states, so they are not suitable for
> > certain purposes, like thermal control.
>
> Agree, it's better described problem statement.
>
> >
> > The most straightforward approach to address it seems to be to modify
> > the collection of cpufreq statistics so that they don't include the
> > time spent by CPUs in idle states, or to make it possible to
> > distinguish the time spent in idle states from the "active" time.
> >
> >>>> 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.
> >
> > Which means that this feature is not going to be universally useful.
> >
> > However, if the time spent by CPUs in idle states were accounted for
> > in the cpufreq statistics, that would be universally useful.
>
> True
>
> >
> >>>
> >>>> 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.
> >
> > Yes, that's not particularly difficult to achieve in principle: on
> > idle entry and exit, update the cpufreq statistics of the policy
> > including the current CPU.
>
> Sounds good.
>
> >
> >>>
> >>> 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).
> >
> > So collecting the statistics should be fast and simple and processing
> > them need not be.
> >
> > Ideally, they should be processed only when somebody asks the data.
>
> Correct.
>
> >
> > I'm not sure if that is the case in the current patchset.
>
> Which is the case in current implementation, where the Active Stats
> Monitor (ASM) is run in the context of thermal workqueue. It is
> scheduled every 100ms to run IPA throttling, which does the
> statistics gathering and calculation in the ASM. Time accounted for this
> major calculation is moved to the 'client' (like IPA) context.
>
> >
> >>>
> >>>> 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.
> >
> > That may help, but let's avoid doing extra work just yet.
>
> Understood
>
> >
> >>>
> >>>> 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?
> >
> > One idea that I have is outlined above and I'm not seeing a reason to
> > put cpufreq statistics into cpuidle.
> >
>
> I'm happy to prepare such RFC if you like.

Well, it should be quite clear that I would prefer this to the
original approach, if viable at all. :-)

> I would just need a bit more information.

OK

> It sounds your proposed solution might be smaller in code
> size, since the client's statistics accounting might be moved to the
> cpufreq_cooling.c (which now live in the ASM). Also, there won't be a
> new framework to maintain (which is a big plus).

Right.

Powered by blists - more mailing lists