diff mbox series

[RFC,3/4] cpufreq: Add Active Stats calls tracking frequency changes

Message ID 20210622075925.16189-4-lukasz.luba@arm.com
State Superseded
Headers show
Series Introduce Active Stats framework with CPU performance statistics | expand

Commit Message

Lukasz Luba June 22, 2021, 7:59 a.m. UTC
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(+)

Comments

Rafael J. Wysocki June 22, 2021, 1:51 p.m. UTC | #1
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?

> >
> > 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.

> >
> >>          if (trace_cpu_frequency_enabled()) {
> >>                  for_each_cpu(cpu, policy->cpus)
> >>                          trace_cpu_frequency(freq, cpu);
> >> --
>
>
> [1]
> https://elixir.bootlin.com/linux/latest/source/kernel/sched/cpufreq_schedutil.c#L447
Lukasz Luba June 22, 2021, 2:09 p.m. UTC | #2
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).
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.
Rafael J. Wysocki June 22, 2021, 2:59 p.m. UTC | #3
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.
Lukasz Luba June 22, 2021, 3:02 p.m. UTC | #4
On 6/22/21 3:51 PM, Rafael J. Wysocki 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?

True, it will get enough data after a first freq switch made by
cpufreq governor. I don't want to race with schedutil and check
the current freq, but I will check that option too.

> 
>> 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.

I see your point. Fair enough.

> 
> 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.
> 

Sure thing. I'll improve this.
Lukasz Luba June 22, 2021, 3:10 p.m. UTC | #5
On 6/22/21 3:59 PM, Rafael J. Wysocki wrote:
> 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?

In Arm world (especially 32-bit world) there is 'a lot' custom idle
and cpufreq drivers. It's probably even not feasible to do.
We also has this CPU_IDLE_MULTIPLE_DRIVERS mechanism. It's a pain,
especially when not having all possible platfroms.

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

I'll try to make it as low-overhead as possible and turn off if there is
no client subsystem (like IPA) currently using it. That might be
feasible.

Thank you Rafael for valuable comments.
Viresh Kumar June 23, 2021, 3:15 a.m. UTC | #6
On 22-06-21, 12:07, Lukasz Luba wrote:
> That cpufreq_stats_record_transition() is present only if

> CONFIG_CPU_FREQ_STAT is set. I didn't wanted to be dependent on

> this config.


Right, but you can also update the dummy
cpufreq_stats_record_transition() function as well and you won't
depend on the CONFIG then.

> > 

> > Plus don't you need to record this for all policy->cpus instead of

> > just policy->cpu ?

> > 

> 

> It will be accounted for all cpus in that freq domain. The

> active_stats_cpu_freq_fast_change() implementation uses

> a shared structure (single for whole domain) 'shared_ast':

> _active_stats_cpu_freq_change(ast->shared_ast, freq, ts)

> (from patch 1/4)


Ahh, I missed that then.

-- 
viresh
Lukasz Luba June 23, 2021, 9:14 a.m. UTC | #7
On 6/23/21 4:15 AM, Viresh Kumar wrote:
> On 22-06-21, 12:07, Lukasz Luba wrote:

>> That cpufreq_stats_record_transition() is present only if

>> CONFIG_CPU_FREQ_STAT is set. I didn't wanted to be dependent on

>> this config.

> 

> Right, but you can also update the dummy

> cpufreq_stats_record_transition() function as well and you won't

> depend on the CONFIG then.


It would look a bit 'hidden', but I can do that.
It will require the header active_stats.h in that cpufreq.h
to add the line in the empty cpufreq_stats_record_transition().

If Rafael would also agree, then I can put it there.
diff mbox series

Patch

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);
+
 	if (trace_cpu_frequency_enabled()) {
 		for_each_cpu(cpu, policy->cpus)
 			trace_cpu_frequency(freq, cpu);