From: "Rafael J. Wysocki" <firstname.lastname@example.org>
To: Lukasz Luba <email@example.com>
Cc: "Rafael J. Wysocki" <firstname.lastname@example.org>,
Linux Kernel Mailing List <email@example.com>,
Daniel Lezcano <firstname.lastname@example.org>,
Linux PM <email@example.com>,
Amit Kucheria <firstname.lastname@example.org>,
"Zhang, Rui" <email@example.com>,
Dietmar Eggemann <firstname.lastname@example.org>,
Chris Redpath <Chris.Redpath@arm.com>,
Beata.Michalska@arm.com, Viresh Kumar <email@example.com>,
"Rafael J. Wysocki" <firstname.lastname@example.org>,
Amit Kachhap <email@example.com>
Subject: Re: [RFC PATCH v2 0/6] Introduce Active Stats framework with CPU performance statistics
Date: Wed, 14 Jul 2021 20:30:33 +0200 [thread overview]
Message-ID: <CAJZ5v0i3UUKxQbcfTVWHOeje-LTOnuAMda3FX0D5sqHLXEs3Xg@mail.gmail.com> (raw)
Sorry for the delay.
On Tue, Jul 6, 2021 at 6:51 PM Lukasz Luba <firstname.lastname@example.org> wrote:
> On 7/6/21 5:34 PM, Rafael J. Wysocki wrote:
> > On Tue, Jul 6, 2021 at 5:56 PM Lukasz Luba <email@example.com> wrote:
> >> On 7/6/21 4:28 PM, Rafael J. Wysocki wrote:
> >>> On Tue, Jul 6, 2021 at 3:18 PM Lukasz Luba <firstname.lastname@example.org> 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.
> >>>> 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.
> > 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.
> >>>> 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.
> 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).
next prev parent reply other threads:[~2021-07-14 18:30 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-06 13:18 [RFC PATCH v2 0/6] Introduce Active Stats framework with CPU performance statistics Lukasz Luba
2021-07-06 13:18 ` [RFC PATCH v2 1/6] PM: Introduce Active Stats framework Lukasz Luba
2021-07-06 13:18 ` [RFC PATCH v2 2/6] cpuidle: Add Active Stats calls tracking idle entry/exit Lukasz Luba
2021-07-06 13:18 ` [RFC PATCH v2 3/6] cpufreq: Add Active Stats calls tracking frequency changes Lukasz Luba
2021-07-06 13:18 ` [RFC PATCH v2 4/6] thermal: Add interface to cooling devices to handle governor change Lukasz Luba
2021-07-06 13:18 ` [RFC PATCH v2 5/6] thermal/core/power allocator: Prepare power actors and calm down when not used Lukasz Luba
2021-07-06 13:18 ` [RFC PATCH v2 6/6] thermal: cpufreq_cooling: Improve power estimation based on Active Stats framework Lukasz Luba
2021-07-06 15:28 ` [RFC PATCH v2 0/6] Introduce Active Stats framework with CPU performance statistics Rafael J. Wysocki
2021-07-06 15:56 ` Lukasz Luba
2021-07-06 16:34 ` Rafael J. Wysocki
2021-07-06 16:51 ` Lukasz Luba
2021-07-14 18:30 ` Rafael J. Wysocki [this message]
2021-07-19 8:52 ` Lukasz Luba
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).