lkml.org 
[lkml]   [2021]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFC PATCH 3/4] cpufreq: Add Active Stats calls tracking frequency changes
On Tue, Jun 22, 2021 at 4:51 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Jun 22, 2021 at 4:09 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >
> >
> >
> > On 6/22/21 2:51 PM, Rafael J. Wysocki wrote:
> > > On Tue, Jun 22, 2021 at 3:42 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > >>
> > >>
> > >>
> > >> On 6/22/21 1:28 PM, Rafael J. Wysocki wrote:
> > >>> On Tue, Jun 22, 2021 at 9:59 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > >>>>
> > >>>> The Active Stats framework tracks and accounts the activity of the CPU
> > >>>> for each performance level. It accounts the real residency, when the CPU
> > >>>> was not idle, at a given performance level. This patch adds needed calls
> > >>>> which provide the CPU frequency transition events to the Active Stats
> > >>>> framework.
> > >>>>
> > >>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> > >>>> ---
> > >>>> drivers/cpufreq/cpufreq.c | 5 +++++
> > >>>> 1 file changed, 5 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > >>>> index 802abc925b2a..d79cb9310572 100644
> > >>>> --- a/drivers/cpufreq/cpufreq.c
> > >>>> +++ b/drivers/cpufreq/cpufreq.c
> > >>>> @@ -14,6 +14,7 @@
> > >>>>
> > >>>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > >>>>
> > >>>> +#include <linux/active_stats.h>
> > >>>> #include <linux/cpu.h>
> > >>>> #include <linux/cpufreq.h>
> > >>>> #include <linux/cpu_cooling.h>
> > >>>> @@ -387,6 +388,8 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy,
> > >>>>
> > >>>> cpufreq_stats_record_transition(policy, freqs->new);
> > >>>> policy->cur = freqs->new;
> > >>>> +
> > >>>> + active_stats_cpu_freq_change(policy->cpu, freqs->new);
> > >>>> }
> > >>>> }
> > >>>>
> > >>>> @@ -2085,6 +2088,8 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
> > >>>> policy->cpuinfo.max_freq);
> > >>>> cpufreq_stats_record_transition(policy, freq);
> > >>>>
> > >>>> + active_stats_cpu_freq_fast_change(policy->cpu, freq);
> > >>>> +
> > >>>
> > >>> This is quite a bit of overhead and so why is it needed in addition to
> > >>> the code below?
> > >>
> > >> The code below is tracing, which is good for post-processing. We use in
> > >> our tool LISA, when we analyze the EAS decision, based on captured
> > >> trace data.
> > >>
> > >> This new code is present at run time, so subsystems like our thermal
> > >> governor IPA can use it and get better estimation about CPU used power
> > >> for any arbitrary period, e.g. 50ms, 100ms, 300ms, ...
> > >
> > > So can it be made not run when the IPA is not using it?
> >
> > I can make a Kconfig for IPA to select this ACTIVE_STATS.
> > Also, I can add description that this framework is mostly needed
> > for IPA, so don't enable it if you don't use IPA (default is 'n'
> > so it shouldn't harm others).
> >
> > This Active Stats shouldn't be stopped when thermal zone is switching
> > between governors at run time, e.g. IPA -> step_wise -> IPA
> > because when IPA is set next time, it might not have correct CPU
> > stats (what is the current frequency and for how long it has been
> > actively used).
>
> But after a while it will collect enough useful data I suppose?
>
> > Beside, switching governors at run time is not a good idea
> > (apart from stress testing them ;) ).
> >
> > >
> > >>>
> > >>> And pretty much the same goes for the idle loop change. There is
> > >>> quite a bit of instrumentation in that code already and it avoids
> > >>> adding new locking for a reason. Why is it a good idea to add more
> > >>> locking to that code?
> > >>
> > >> This active_stats_cpu_freq_fast_change() doesn't use the locking, it
> > >> relies on schedutil lock in [1].
> > >
> > > Ah, OK.
> > >
> > > But it still adds overhead AFAICS.
> >
> > Agree, it's an extra code. For platforms which use IPA it's a
> > justifiable cost, weighted by better estimation thanks to this calls.
> > For other platforms, this framework will be set to default 'n' option.
>
> A general problem with build-time configuration is for distros that
> want to ship one kernel binary to run on multiple hardware platforms.
> They need to enable those options anyway and then get the full cost on
> the platforms that don't need it, but want to use the common binary
> kernel.
>
> Again, please consider making this new code run only when it is needed
> even if configured in and if it runs, make it as low-overhead as
> possible.

Also, why don't you add these hooks to the drivers that are generally
worked with by the IPA?

That you won't need to worry about the possible impact on everybody else.

\
 
 \ /
  Last update: 2021-06-22 16:59    [W:0.057 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site