Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On 08/06/17 08:55, Dietmar Eggemann wrote:
> Implements an arch-specific frequency-scaling function
> topology_get_freq_scale() which provides the following frequency
> scaling factor:
>
> current_freq(cpu) << SCHED_CAPACITY_SHIFT / max_supported_freq(cpu)
[...]
Frequency and cpu-invariant load tracking are part of the task
schedulers hot path:
e.g.:
__update_load_avg_se()-> ___update_load_avg() -> accumulate_sum()
That's why function calls should be avoided here.
I would like to fold the following changes into patch 2/6 in v2:
commit 1397770fe47ce5d34511e7062bd3a8bc96a74590
Author: Dietmar Eggemann
Date: Sat Jun 24 16:46:45 2017 +0100
drivers base/arch_topology: eliminate function call for cpu and
frequency-invariant accounting
topology_get_cpu_scale() and topology_get_freq_scale() are the arm/arm64
architecture specific implementations to provide cpu-invariant and
frequency-invariant accounting support up to the task scheduler.
Define them as static inline functions to allow cpu-invariant and
frequency-invariant accounting to happen without an extra function call
involved.
Test results on JUNO (arm64):
root@juno:~# grep
"__update_load_avg_\|update_group_capacity\|topology_get"
available_filter_functions > set_ftrace_filter
root@juno:~# echo function_graph > current_tracer
root@juno:~# cat trace | tail -50
w/ this patch:
...
3) 0.700 us| __update_load_avg_se.isra.5();
...
3) 0.750 us| __update_load_avg_cfs_rq();
...
3) 0.780 us| update_group_capacity();
...
w/o this patch:
4) | __update_load_avg_cfs_rq() {
4) 0.380 us|topology_get_freq_scale();
4) 0.340 us|topology_get_cpu_scale();
4) 6.420 us| }
...
4) | __update_load_avg_se.isra.4() {
4) 0.300 us|topology_get_freq_scale();
4) 0.260 us|topology_get_cpu_scale();
4) 5.800 us| }
...
4) | update_group_capacity() {
4) 0.260 us|topology_get_cpu_scale();
4) 3.540 us| }
...
So these extra function calls cost ~2.5us each (on Cortex A53,
cpu0,3,4,5). Since this happens in the task scheduler hot-path,
they have to be avoided.
Signed-off-by: Dietmar Eggemann
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index d7e130c268fb..8dfa4c3dbfc2 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -23,18 +23,8 @@
#include
static DEFINE_MUTEX(cpu_scale_mutex);
-static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
-static DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
-
-unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu)
-{
- return per_cpu(cpu_scale, cpu);
-}
-
-unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu)
-{
- return per_cpu(freq_scale, cpu);
-}
+DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
+DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
{
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 3fb4d8ccb179..cf22631e6765 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -9,10 +9,21 @@ void topology_normalize_cpu_scale(void);
struct device_node;
int topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu);
+DECLARE_PER_CPU(unsigned long, cpu_scale);
+DECLARE_PER_CPU(unsigned long, freq_scale);
+
struct sched_domain;
-unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu);
+static inline
+unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu)
+{
+ return per_cpu(cpu_scale, cpu);
+}
-unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu);
+static inline
+unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu)
+{
+ return per_cpu(freq_scale, cpu);
+}
void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity);
[...]
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On Thu, Jun 22, 2017 at 09:36:43AM +0530, Viresh Kumar wrote:
> On 21-06-17, 17:57, Morten Rasmussen wrote:
> > It is true that this patch relies on the notifiers, but I don't see how
> > that prevents us from adding a non-notifier based solution for
> > fast-switch enabled platforms later?
>
> No it doesn't, but I thought it would be better to have a single
> solution (if possible) for all the cases here.
Right. As I mentioned further down in my reply. There is no single
solution that fits all. Smart platforms with HW counters, like x86,
would want to use those. IIUC, cpufreq has no idea what the true
delivered performance is anyway on those platforms.
>
> > > I think this patch doesn't really need to go down the notifiers way.
> > >
> > > We can do something like this in the implementation of
> > > topology_get_freq_scale():
> > >
> > > return (policy->cur << SCHED_CAPACITY_SHIFT) / max;
> > >
> > > Though, we would be required to take care of policy structure in this
> > > case somehow.
> >
> > This is exactly what this patch implements. Unfortunately we can't be
> > sure that there is a valid policy data structure where we can read the
> > information from.
>
> Actually there is a way around that.
>
> - Revert one of my patches:
> commit f9f41e3ef99a ("cpufreq: Remove policy create/remove notifiers")
>
> - Use those notifiers in init_cpu_capacity_callback() instead of
> CPUFREQ_NOTIFY and set/reset a local policy pointer.
>
> - And this pointer we can use safely/reliably in
> topology_get_freq_scale(). We may need to use RCU read side
> protection in topology_get_freq_scale() though, to make sure the
> local policy pointer isn't getting updated simultaneously.
>
> - If the policy pointer isn't set, then we can use
> SCHED_CAPACITY_SCALE value instead.
IIUC, you are proposing to maintain an RCU protected pointer in the
topology driver to the policy data structure inside cpufreq and keep it
up to date through cpufreq notifiers. So instead of getting notified
when the frequency changes so we can recompute the scaling ratio, we
have to poll the value and recompute the ratio on each access.
If we are modifying cpufreq, why not just make cpufreq responsible for
providing the scaling factor? It seems easier, cleaner, and a lot
less fragile.
>
>
> > Isn't the policy protected by a lock as well?
>
> There are locks, but you don't need any to read policy->cur.
Okay, but you need to rely on notifiers to know when it is valid.
>
> > Another thing is that I don't think a transition notifier based solution
> > or any other solution based on the cur/max ratio is really the right way
> > to go for fast-switching platforms. If we can do very frequent frequency
> > switching it makes less sense to use the current ratio whenever we
> > update the PELT averages as the frequency might have changed multiple
> > times since the last update. So it would make more sense to have an
> > average ratio instead.
>
> > If the platform has HW counters (e.g. APERF/MPERF) that can provide the
> > ratio then we should of course use those, if not, one solution could be
> > to let cpufreq track the average frequency for each cpu over a suitable
> > time window (around one sched period I think). It should be fairly low
> > overhead to maintain. In the topology driver, we would then choose
> > whether the scaling factor is provided by the cpufreq average frequency
> > ratio or the current transition notifier based approach based on the
> > capabilities of the platform.
>
> Hmm, maybe.
You said you wanted a solution that works for fast-switch enabled
platforms ;-)
The cur/max ratio isn't sufficient for those. PeterZ has already
proposed to use APERF/MPERF for x86 to use the average frequency for
PELT updates. I think other fast-switch platforms would want something
similar, as it makes much more sense.
Morten
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On 21-06-17, 17:57, Morten Rasmussen wrote:
> It is true that this patch relies on the notifiers, but I don't see how
> that prevents us from adding a non-notifier based solution for
> fast-switch enabled platforms later?
No it doesn't, but I thought it would be better to have a single
solution (if possible) for all the cases here.
> > I think this patch doesn't really need to go down the notifiers way.
> >
> > We can do something like this in the implementation of
> > topology_get_freq_scale():
> >
> > return (policy->cur << SCHED_CAPACITY_SHIFT) / max;
> >
> > Though, we would be required to take care of policy structure in this
> > case somehow.
>
> This is exactly what this patch implements. Unfortunately we can't be
> sure that there is a valid policy data structure where we can read the
> information from.
Actually there is a way around that.
- Revert one of my patches:
commit f9f41e3ef99a ("cpufreq: Remove policy create/remove notifiers")
- Use those notifiers in init_cpu_capacity_callback() instead of
CPUFREQ_NOTIFY and set/reset a local policy pointer.
- And this pointer we can use safely/reliably in
topology_get_freq_scale(). We may need to use RCU read side
protection in topology_get_freq_scale() though, to make sure the
local policy pointer isn't getting updated simultaneously.
- If the policy pointer isn't set, then we can use
SCHED_CAPACITY_SCALE value instead.
> Isn't the policy protected by a lock as well?
There are locks, but you don't need any to read policy->cur.
> Another thing is that I don't think a transition notifier based solution
> or any other solution based on the cur/max ratio is really the right way
> to go for fast-switching platforms. If we can do very frequent frequency
> switching it makes less sense to use the current ratio whenever we
> update the PELT averages as the frequency might have changed multiple
> times since the last update. So it would make more sense to have an
> average ratio instead.
> If the platform has HW counters (e.g. APERF/MPERF) that can provide the
> ratio then we should of course use those, if not, one solution could be
> to let cpufreq track the average frequency for each cpu over a suitable
> time window (around one sched period I think). It should be fairly low
> overhead to maintain. In the topology driver, we would then choose
> whether the scaling factor is provided by the cpufreq average frequency
> ratio or the current transition notifier based approach based on the
> capabilities of the platform.
Hmm, maybe.
--
viresh
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On 21-06-17, 17:38, Dietmar Eggemann wrote:
> On 20/06/17 07:17, Viresh Kumar wrote:
> > Any specific reason on why are we doing this from PRECHANGE and
> > not POSTCHANGE ? i.e. we are doing this before the frequency is
> > really updated.
>
> Not really. In case I get a CPUFREQ_POSTCHANGE all the time the
> frequency actually changed I can switch to CPUFREQ_POSTCHANGE.
Yes, you should always get that. And its not right to do any such
change in PRECHANGE notifier as we may fail to change the frequency as
well..
> > Wanted to make sure that we all understand the constraints this is going to
> > add
> > for the ARM64 platforms.
> >
> > With the introduction of this transition notifier, we would not be able to
> > use
> > the fast-switch path in the schedutil governor. I am not sure if there are
> > any
> > ARM platforms that can actually use the fast-switch path in future or not
> > though. The requirement of fast-switch path is that the freq can be changed
> > without sleeping in the hot-path.
>
> That's a good point. The cpufreq transition notifier based Frequency
> Invariance Engine (FIE) can only work if none of the cpufreq policies
> support fast frequency switching.
At least with the current design, yes.
> What about we still enable cpufreq transition notifier based FIE for
> systems where this is true. This will cover 100% of all arm/arm64
> systems today.
I would suggest having a single solution for everyone if we can.
> In case one day we have a cpufreq driver which allows fast frequency
> switching we would need a FIE based on something else than cpufreq
> transition notifier. Maybe based on performance counters (something
> similar to x86 APERF/MPERF) or cpufreq core could provide a function
> which provides the avg frequency value.
>
> I could make the current implementation more future-proof by only
> using the notifier based FIE in case all policies use slow frequency
> switching:
>
> >From afe64b5c0606cad4304b77fc5cff819d3083a88d Mon Sep 17 00:00:00 2001
> From: Dietmar Eggemann
> Date: Wed, 21 Jun 2017 14:53:26 +0100
> Subject: [PATCH] drivers base/arch_topology: enable cpufreq transistion
> notifier based FIE only for slow frequency switching
>
> Fast frequency switching is incompatible with cpufreq transition
> notifiers.
>
> Enable the cpufreq transition notifier based Frequency Invariance Engine
> (FIE) only in case there are no cpufreq policies able to use fast
> frequency switching.
>
> Currently there are no cpufreq drivers for arm/arm64 which support fast
> frequency switching. In case such a driver will appear the FEI
> topology_get_freq_scale() has to be extended to provide frequency
> invariance based on something else than cpufreq transition notifiers,
> e.g. performance counters.
>
> Signed-off-by: Dietmar Eggemann
> ---
> drivers/base/arch_topology.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index c2539dc584d5..bd14c5e81f63 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -171,6 +171,7 @@ static bool cap_parsing_done;
> static void parsing_done_workfn(struct work_struct *work);
> static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
> static DEFINE_PER_CPU(unsigned long, max_freq);
> +static bool enable_freq_inv = true;
>
> static int
> init_cpu_capacity_callback(struct notifier_block *nb,
> @@ -199,6 +200,8 @@ init_cpu_capacity_callback(struct notifier_block *nb,
> policy->cpuinfo.max_freq / 1000UL;
> capacity_scale = max(raw_capacity[cpu],
> capacity_scale);
> }
> + if (policy->fast_switch_possible)
> + enable_freq_inv = false;
> if (cpumask_empty(cpus_to_visit)) {
> if (!cap_parsing_failed) {
> topology_normalize_cpu_scale();
> @@ -268,21 +271,23 @@ static int __init register_cpufreq_notifier(void)
> ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> CPUFREQ_POLICY_NOTIFIER);
>
> - if (ret) {
> + if (ret)
> free_cpumask_var(cpus_to_visit);
> - return ret;
> - }
>
> - return cpufreq_register_notifier(&set_freq_scale_notifier,
> -CPUFREQ_TRANSITION_NOTIFIER);
> + return ret;
> }
> core_initcall(register_cpufreq_notifier);
>
> static void parsing_done_workfn(struct work_struct *work)
> {
> +
> free_cpumask_var(cpus_to_visit);
> cpufreq_unregister_notifier(&init_cpu_capacity_notifier,
> CPUFREQ_POLICY_NOTIFIER);
> +
> + if (enable_freq_inv)
> + cpufreq_register_notifier(&set_freq_scale_notifier,
> + CPUFREQ_TRANSITION_NOTIFIER);
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On 21/06/17 01:31, Saravana Kannan wrote:
> On 06/19/2017 11:17 PM, Viresh Kumar wrote:
>> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
>> wrote:
>>
>>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>>
>>> static int __init register_cpufreq_notifier(void)
>>> {
>>> + int ret;
>>> +
>>> /*
>>> * on ACPI-based systems we need to use the default cpu
>>> capacity
>>> * until we have the necessary code to parse the cpu
>>> capacity, so
>>> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
>>>
>>> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>>>
>>> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
>>> -CPUFREQ_POLICY_NOTIFIER);
>>> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
>>> + CPUFREQ_POLICY_NOTIFIER);
>>
>> Wanted to make sure that we all understand the constraints this is
>> going to add
>> for the ARM64 platforms.
>>
>> With the introduction of this transition notifier, we would not be
>> able to use
>> the fast-switch path in the schedutil governor. I am not sure if there
>> are any
>> ARM platforms that can actually use the fast-switch path in future or not
>> though. The requirement of fast-switch path is that the freq can be
>> changed
>> without sleeping in the hot-path.
>>
>> So, will we ever want fast-switching for ARM platforms ?
>>
>
> I don't think we should go down a path that'll prevent ARM platform from
> switching over to fast-switching in the future.
Understood. But IMHO implementing a cpufreq transition notifier based
Frequency Invariance Engine (FIE) which provides frequency-invariant
accounting for 100% of today's arm/arm64 system is legitimate.
Like I said in the other email in this thread today, I can make sure
that I only register the cpufreq transition notifier if none of the
policies support fast frequency switching. In this case people can use
mainline to experiment with cpufreq drivers supporting fast frequency
switching (without FIE support).
> Having said that, I'm not sure I fully agree with the decision to
> completely disable notifiers in the fast-switching case. How many of the
> current users of notifiers truly need support for sleeping in the
> notifier? Why not make all the transition notifiers atomic? Or at least
> add atomic transition notifiers that can be registered for separately if
> the client doesn't need the ability to sleep?
IMHO, that's a different construction side inside the cpufreq framework.
Patches which introduced the fast frequency switching support in cpufreq
clearly state that "... fast frequency switching is inherently
incompatible with cpufreq transition notifiers ...".
If we can get rid of this restriction, the cpufreq transition notifier
based FIE implementation could stay. Otherwise we need a FIE for systems
with fast frequency switching based on something else, e.g. performance
counters.
> Most of the clients don't seem like ones that'll need to sleep.
>
> There are a bunch of generic off-tree drivers (can't upstream them yet
> because it depends on the bus scaling framework) that also depend on
> CPUfreq transition notifiers that are going to stop working if fast
> switching becomes available in the future. So, this decision to disallow
> transition notifiers is painful for other reasons too.
Falls into the same bucket for me ... you have a requirement against the
cpufreq framework to let cpufreq transition notifier work for fast
frequency switching drivers.
[...]
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On Wed, Jun 21, 2017 at 11:07:35AM +0530, Viresh Kumar wrote:
> On 20-06-17, 17:31, Saravana Kannan wrote:
> > On 06/19/2017 11:17 PM, Viresh Kumar wrote:
> > >On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> > > wrote:
> > >
> > >>diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > >
> > >> static int __init register_cpufreq_notifier(void)
> > >> {
> > >>+ int ret;
> > >>+
> > >> /*
> > >> * on ACPI-based systems we need to use the default cpu capacity
> > >> * until we have the necessary code to parse the cpu capacity, so
> > >>@@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
> > >>
> > >> cpumask_copy(cpus_to_visit, cpu_possible_mask);
> > >>
> > >>- return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> > >>-CPUFREQ_POLICY_NOTIFIER);
> > >>+ ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> > >>+ CPUFREQ_POLICY_NOTIFIER);
> > >
> > >Wanted to make sure that we all understand the constraints this is going
> > >to add
> > >for the ARM64 platforms.
> > >
> > >With the introduction of this transition notifier, we would not be able to
> > >use
> > >the fast-switch path in the schedutil governor. I am not sure if there are
> > >any
> > >ARM platforms that can actually use the fast-switch path in future or not
> > >though. The requirement of fast-switch path is that the freq can be changed
> > >without sleeping in the hot-path.
> > >
> > >So, will we ever want fast-switching for ARM platforms ?
I hope that one day we will have such platforms.
> > >
> >
> > I don't think we should go down a path that'll prevent ARM platform from
> > switching over to fast-switching in the future.
>
> Yeah, that's why brought attention to this stuff.
It is true that this patch relies on the notifiers, but I don't see how
that prevents us from adding a non-notifier based solution for
fast-switch enabled platforms later?
>
> I think this patch doesn't really need to go down the notifiers way.
>
> We can do something like this in the implementation of
> topology_get_freq_scale():
>
> return (policy->cur << SCHED_CAPACITY_SHIFT) / max;
>
> Though, we would be required to take care of policy structure in this
> case somehow.
This is exactly what this patch implements. Unfortunately we can't be
sure that there is a valid policy data structure where we can read the
information from. Isn't the policy protected by a lock as well?
I don't quite see how you would solve that problem without having some
cached version of the scaling factor that is safe to read without
locking and is always available, even before cpufreq has come up.
Another thing is that I don't think a transition notifier based solution
or any other solution based on the cur/max ratio is really the right way
to go for fast-switching platforms. If we can do very frequent frequency
switching it makes less sense to use the current ratio whenever we
update the PELT averages as the frequency might have changed multiple
times since the last update. So it would make more sense to have an
average ratio instead.
If the platform has HW counters (e.g. APERF/MPERF) that can provide the
ratio then we should of course use those, if not, one solution could be
to let cpufreq track the average frequency for each cpu over a suitable
time window (around one sched period I think). It should be fairly low
overhead to maintain. In the topology driver, we would then choose
whether the scaling factor is provided by the cpufreq average frequency
ratio or the current transition notifier based approach based on the
capabilities of the platform.
Morten
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On 14/06/17 14:08, Vincent Guittot wrote: > On 14 June 2017 at 09:55, Dietmar Eggemann wrote: >> >> On 06/12/2017 04:27 PM, Vincent Guittot wrote: >>> On 8 June 2017 at 09:55, Dietmar Eggemann wrote: [...] >> >> Yes, we should free cpus_to_visit if the policy notifier registration >> fails. But IMHO also, once the parsing of the capacity-dmips-mhz property >> is done. free cpus_to_visit is only used in the notifier call >> init_cpu_capacity_callback() after being allocated and initialized in >> register_cpufreq_notifier(). >> >> We could add something like this as the first patch of this set. Only >> mildly tested on Juno. Juri, what do you think? >> >> Author: Dietmar Eggemann >> Date: Tue Jun 13 23:21:59 2017 +0100 >> >> drivers base/arch_topology: free cpumask cpus_to_visit >> >> Free cpumask cpus_to_visit in case registering >> init_cpu_capacity_notifier has failed or the parsing of the cpu >> capacity-dmips-mhz property is done. The cpumask cpus_to_visit is >> only used inside the notifier call init_cpu_capacity_callback. >> >> Reported-by: Vincent Guittot >> Signed-off-by: Dietmar Eggemann > > your proposal for freeing cpus_to_visit looks good for me > > Acked-by: Vincent Guittot Thanks. [...] >> IMHO, that's not necessary. >> >> The transition notifier works completely independent from the policy >> notifier. In case the latter gets registered correctly and the registration >> of the former fails, the notifier call of the policy notifier still parses >> the capacity-dmips-mhz property information and sets per_cpu(max_freq, cpu). >> >> The notifier call set_freq_scale_callback() of the transition notifier will >> not be called so that frequency invariance always returns >> SCHED_CAPACITY_SCALE. >> >> After the policy notifier has finished its work, it schedules >> parsing_done_work() in which it gets unregistered. > > Ok so IIUC, the transition notifier is somehow optional and we still > have the cpu invariance. > In this case, you should not return the error code of > cpufreq_register_notifier(&set_freq_scale_notifier, > CPUFREQ_TRANSITION_NOTIFIER) as the error code of the > register_cpufreq_notifier function. > you should better print a warning like " failed to init frequency > invariance" and return 0 for register_cpufreq_notifier() Makes sense. Will change this. [...]
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On 20/06/17 07:17, Viresh Kumar wrote:
> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> wrote:
>
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>
>> static int
>> init_cpu_capacity_callback(struct notifier_block *nb,
>> @@ -185,6 +192,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
>>cpus_to_visit,
>>policy->related_cpus);
>> for_each_cpu(cpu, policy->related_cpus) {
>> + per_cpu(max_freq, cpu) = policy->cpuinfo.max_freq;
>
> I am not sure about this but why shouldn't we use policy->max here ?
> As that is the
> max, we can set the frequency to right now.
>
No, frequency invariance is defined by:
current_freq(cpu) << SCHED_CAPACITY_SHIFT / max_supported_freq(cpu)
We don't want to scale against a value which might be restricted e.g.
by thermal capping.
>> if (cap_parsing_failed)
>> continue;
>> raw_capacity[cpu] = topology_get_cpu_scale(NULL,
>> cpu) *
[...]
>> +static int set_freq_scale_callback(struct notifier_block *nb,
>> + unsigned long val,
>> + void *data)
>> +{
>> + struct cpufreq_freqs *freq = data;
>> +
>> + switch (val) {
>> + case CPUFREQ_PRECHANGE:
>> + set_freq_scale(freq->cpu, freq->new);
>
> Any specific reason on why are we doing this from PRECHANGE and
> not POSTCHANGE ? i.e. we are doing this before the frequency is
> really updated.
Not really. In case I get a CPUFREQ_POSTCHANGE all the time the
frequency actually changed I can switch to CPUFREQ_POSTCHANGE.
[...]
>> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
>>
>> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>>
>> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
>> -CPUFREQ_POLICY_NOTIFIER);
>> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
>> + CPUFREQ_POLICY_NOTIFIER);
>
> Wanted to make sure that we all understand the constraints this is going to
> add
> for the ARM64 platforms.
>
> With the introduction of this transition notifier, we would not be able to use
> the fast-switch path in the schedutil governor. I am not sure if there are any
> ARM platforms that can actually use the fast-switch path in future or not
> though. The requirement of fast-switch path is that the freq can be changed
> without sleeping in the hot-path.
That's a good point. The cpufreq transition notifier based Frequency
Invariance Engine (FIE) can only work if none of the cpufreq policies
support fast frequency switching.
What about we still enable cpufreq transition notifier based FIE for
systems where this is true. This will cover 100% of all arm/arm64
systems today.
In case one day we have a cpufreq driver which allows fast frequency
switching we would need a FIE based on something else than cpufreq
transition notifier. Maybe based on performance counters (something
similar to x86 APERF/MPERF) or cpufreq core could provide a function
which provides the avg frequency value.
I could make the current implementation more future-proof by only
using the notifier based FIE in case all policies use slow frequency
switching:
>From afe64b5c0606cad4304b77fc5cff819d3083a88d Mon Sep 17 00:00:00 2001
From: Dietmar Eggemann
Date: Wed, 21 Jun 2017 14:53:26 +0100
Subject: [PATCH] drivers base/arch_topology: enable cpufreq transistion
notifier based FIE only for slow frequency switching
Fast frequency switching is incompatible with cpufreq transition
notifiers.
Enable the cpufreq transition notifier based Frequency Invariance Engine
(FIE) only in case there are no cpufreq policies able to use fast
frequency switching.
Currently there are no cpufreq drivers for arm/arm64 which support fast
frequency switching. In case such a driver will appear the FEI
topology_get_freq_scale() has to be extended to provide frequency
invariance based on something else than cpufreq transition notifiers,
e.g. performance counters.
Signed-off-by: Dietmar Eggemann
---
drivers/base/arch_topology.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index c2539dc584d5..bd14c5e81f63 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -171,6 +171,7 @@ static bool cap_parsing_done;
static void parsing_done_workfn(struct work_struct *work);
static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
static DEFINE_PER_CPU(unsigned long, max_freq);
+static bool enable_freq_inv = true;
static int
init_cpu_capacity_callback(struct notifier_block *nb,
@@ -199,6 +200,8 @@ init_cpu_capacity_callback(struct notifier_block *nb,
policy
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On 20-06-17, 17:31, Saravana Kannan wrote:
> On 06/19/2017 11:17 PM, Viresh Kumar wrote:
> >On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> > wrote:
> >
> >>diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> >
> >> static int __init register_cpufreq_notifier(void)
> >> {
> >>+ int ret;
> >>+
> >> /*
> >> * on ACPI-based systems we need to use the default cpu capacity
> >> * until we have the necessary code to parse the cpu capacity, so
> >>@@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
> >>
> >> cpumask_copy(cpus_to_visit, cpu_possible_mask);
> >>
> >>- return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> >>-CPUFREQ_POLICY_NOTIFIER);
> >>+ ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> >>+ CPUFREQ_POLICY_NOTIFIER);
> >
> >Wanted to make sure that we all understand the constraints this is going to
> >add
> >for the ARM64 platforms.
> >
> >With the introduction of this transition notifier, we would not be able to
> >use
> >the fast-switch path in the schedutil governor. I am not sure if there are
> >any
> >ARM platforms that can actually use the fast-switch path in future or not
> >though. The requirement of fast-switch path is that the freq can be changed
> >without sleeping in the hot-path.
> >
> >So, will we ever want fast-switching for ARM platforms ?
> >
>
> I don't think we should go down a path that'll prevent ARM platform from
> switching over to fast-switching in the future.
Yeah, that's why brought attention to this stuff.
I think this patch doesn't really need to go down the notifiers way.
We can do something like this in the implementation of
topology_get_freq_scale():
return (policy->cur << SCHED_CAPACITY_SHIFT) / max;
Though, we would be required to take care of policy structure in this
case somehow.
> Having said that, I'm not sure I fully agree with the decision to completely
> disable notifiers in the fast-switching case. How many of the current users
> of notifiers truly need support for sleeping in the notifier?
Its not just about sleeping here. We do not wish to call too much
stuff from scheduler hot path. Even if it doesn't sleep.
> Why not make
> all the transition notifiers atomic? Or at least add atomic transition
> notifiers that can be registered for separately if the client doesn't need
> the ability to sleep?
>
> Most of the clients don't seem like ones that'll need to sleep.
Only if the scheduler maintainers agree to getting these notifiers
called from hot path, which I don't think is going to happen.
> There are a bunch of generic off-tree drivers (can't upstream them yet
> because it depends on the bus scaling framework) that also depend on CPUfreq
> transition notifiers that are going to stop working if fast switching
> becomes available in the future. So, this decision to disallow transition
> notifiers is painful for other reasons too.
I think its kind of fine to work without fast switch in those cases,
as we are anyway ready to call notifiers which may end up taking any
amount of time.
This case was special as it is affecting entire arch here and so I
pointed it out.
--
viresh
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On 06/19/2017 11:17 PM, Viresh Kumar wrote:
On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
wrote:
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
static int __init register_cpufreq_notifier(void)
{
+ int ret;
+
/*
* on ACPI-based systems we need to use the default cpu capacity
* until we have the necessary code to parse the cpu capacity, so
@@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
cpumask_copy(cpus_to_visit, cpu_possible_mask);
- return cpufreq_register_notifier(&init_cpu_capacity_notifier,
-CPUFREQ_POLICY_NOTIFIER);
+ ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
+ CPUFREQ_POLICY_NOTIFIER);
Wanted to make sure that we all understand the constraints this is going to add
for the ARM64 platforms.
With the introduction of this transition notifier, we would not be able to use
the fast-switch path in the schedutil governor. I am not sure if there are any
ARM platforms that can actually use the fast-switch path in future or not
though. The requirement of fast-switch path is that the freq can be changed
without sleeping in the hot-path.
So, will we ever want fast-switching for ARM platforms ?
I don't think we should go down a path that'll prevent ARM platform from
switching over to fast-switching in the future.
Having said that, I'm not sure I fully agree with the decision to
completely disable notifiers in the fast-switching case. How many of the
current users of notifiers truly need support for sleeping in the
notifier? Why not make all the transition notifiers atomic? Or at least
add atomic transition notifiers that can be registered for separately if
the client doesn't need the ability to sleep?
Most of the clients don't seem like ones that'll need to sleep.
There are a bunch of generic off-tree drivers (can't upstream them yet
because it depends on the bus scaling framework) that also depend on
CPUfreq transition notifiers that are going to stop working if fast
switching becomes available in the future. So, this decision to disallow
transition notifiers is painful for other reasons too.
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
wrote:
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> static int
> init_cpu_capacity_callback(struct notifier_block *nb,
> @@ -185,6 +192,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
>cpus_to_visit,
>policy->related_cpus);
> for_each_cpu(cpu, policy->related_cpus) {
> + per_cpu(max_freq, cpu) = policy->cpuinfo.max_freq;
I am not sure about this but why shouldn't we use policy->max here ?
As that is the
max, we can set the frequency to right now.
> if (cap_parsing_failed)
> continue;
> raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu)
> *
> @@ -195,8 +203,10 @@ init_cpu_capacity_callback(struct notifier_block *nb,
> if (!cap_parsing_failed) {
> topology_normalize_cpu_scale();
> kfree(raw_capacity);
> + pr_debug("cpu_capacity: parsing done\n");
> + } else {
> + pr_debug("cpu_capacity: max frequency parsing
> done\n");
> }
> - pr_debug("cpu_capacity: parsing done\n");
> cap_parsing_done = true;
> schedule_work(&parsing_done_work);
> }
> @@ -208,8 +218,38 @@ static struct notifier_block init_cpu_capacity_notifier
> = {
> .notifier_call = init_cpu_capacity_callback,
> };
>
> +static void set_freq_scale(unsigned int cpu, unsigned long freq)
> +{
> + unsigned long max = per_cpu(max_freq, cpu);
> +
> + if (!max)
> + return;
> +
> + per_cpu(freq_scale, cpu) = (freq << SCHED_CAPACITY_SHIFT) / max;
> +}
> +
> +static int set_freq_scale_callback(struct notifier_block *nb,
> + unsigned long val,
> + void *data)
> +{
> + struct cpufreq_freqs *freq = data;
> +
> + switch (val) {
> + case CPUFREQ_PRECHANGE:
> + set_freq_scale(freq->cpu, freq->new);
Any specific reason on why are we doing this from PRECHANGE and
not POSTCHANGE ? i.e. we are doing this before the frequency is
really updated.
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block set_freq_scale_notifier = {
> + .notifier_call = set_freq_scale_callback,
> +};
> +
> static int __init register_cpufreq_notifier(void)
> {
> + int ret;
> +
> /*
> * on ACPI-based systems we need to use the default cpu capacity
> * until we have the necessary code to parse the cpu capacity, so
> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
>
> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>
> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> -CPUFREQ_POLICY_NOTIFIER);
> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> + CPUFREQ_POLICY_NOTIFIER);
Wanted to make sure that we all understand the constraints this is going to add
for the ARM64 platforms.
With the introduction of this transition notifier, we would not be able to use
the fast-switch path in the schedutil governor. I am not sure if there are any
ARM platforms that can actually use the fast-switch path in future or not
though. The requirement of fast-switch path is that the freq can be changed
without sleeping in the hot-path.
So, will we ever want fast-switching for ARM platforms ?
--
viresh
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
Hi, On 14/06/17 15:08, Vincent Guittot wrote: > On 14 June 2017 at 09:55, Dietmar Eggemann wrote: > > > > On 06/12/2017 04:27 PM, Vincent Guittot wrote: > > > On 8 June 2017 at 09:55, Dietmar Eggemann > > > wrote: > > > > Hi Vincent, > > > > Thanks for the review! > > > > [...] > > > > >> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void) > > >> > > >> cpumask_copy(cpus_to_visit, cpu_possible_mask); > > >> > > >> - return cpufreq_register_notifier(&init_cpu_capacity_notifier, > > >> -CPUFREQ_POLICY_NOTIFIER); > > >> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier, > > >> + CPUFREQ_POLICY_NOTIFIER); > > >> + > > >> + if (ret) > > > > > > Don't you have to free memory allocated for cpus_to_visit in case of > > > errot ? it was not done before your patch as well > > > > Yes, we should free cpus_to_visit if the policy notifier registration > > fails. But IMHO also, once the parsing of the capacity-dmips-mhz property > > is done. free cpus_to_visit is only used in the notifier call > > init_cpu_capacity_callback() after being allocated and initialized in > > register_cpufreq_notifier(). > > > > We could add something like this as the first patch of this set. Only > > mildly tested on Juno. Juri, what do you think? > > > > Author: Dietmar Eggemann > > Date: Tue Jun 13 23:21:59 2017 +0100 > > > > drivers base/arch_topology: free cpumask cpus_to_visit > > > > Free cpumask cpus_to_visit in case registering > > init_cpu_capacity_notifier has failed or the parsing of the cpu > > capacity-dmips-mhz property is done. The cpumask cpus_to_visit is > > only used inside the notifier call init_cpu_capacity_callback. > > > > Reported-by: Vincent Guittot > > Signed-off-by: Dietmar Eggemann > > your proposal for freeing cpus_to_visit looks good for me > > Acked-by: Vincent Guittot > Yep, looks good to me too. Thanks for fixing! Best, - Juri
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On 14 June 2017 at 09:55, Dietmar Eggemann wrote:
>
> On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> > On 8 June 2017 at 09:55, Dietmar Eggemann wrote:
>
> Hi Vincent,
>
> Thanks for the review!
>
> [...]
>
> >> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
> >>
> >> cpumask_copy(cpus_to_visit, cpu_possible_mask);
> >>
> >> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> >> -CPUFREQ_POLICY_NOTIFIER);
> >> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> >> + CPUFREQ_POLICY_NOTIFIER);
> >> +
> >> + if (ret)
> >
> > Don't you have to free memory allocated for cpus_to_visit in case of
> > errot ? it was not done before your patch as well
>
> Yes, we should free cpus_to_visit if the policy notifier registration
> fails. But IMHO also, once the parsing of the capacity-dmips-mhz property
> is done. free cpus_to_visit is only used in the notifier call
> init_cpu_capacity_callback() after being allocated and initialized in
> register_cpufreq_notifier().
>
> We could add something like this as the first patch of this set. Only
> mildly tested on Juno. Juri, what do you think?
>
> Author: Dietmar Eggemann
> Date: Tue Jun 13 23:21:59 2017 +0100
>
> drivers base/arch_topology: free cpumask cpus_to_visit
>
> Free cpumask cpus_to_visit in case registering
> init_cpu_capacity_notifier has failed or the parsing of the cpu
> capacity-dmips-mhz property is done. The cpumask cpus_to_visit is
> only used inside the notifier call init_cpu_capacity_callback.
>
> Reported-by: Vincent Guittot
> Signed-off-by: Dietmar Eggemann
your proposal for freeing cpus_to_visit looks good for me
Acked-by: Vincent Guittot
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index d1c33a85059e..f4832c662762 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -206,6 +206,8 @@ static struct notifier_block init_cpu_capacity_notifier =
> {
>
> static int __init register_cpufreq_notifier(void)
> {
> + int ret;
> +
> /*
> * on ACPI-based systems we need to use the default cpu capacity
> * until we have the necessary code to parse the cpu capacity, so
> @@ -221,13 +223,19 @@ static int __init register_cpufreq_notifier(void)
>
> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>
> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> -CPUFREQ_POLICY_NOTIFIER);
> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> + CPUFREQ_POLICY_NOTIFIER);
> +
> + if (ret)
> + free_cpumask_var(cpus_to_visit);
> +
> + return ret;
> }
> core_initcall(register_cpufreq_notifier);
>
> static void parsing_done_workfn(struct work_struct *work)
> {
> + free_cpumask_var(cpus_to_visit);
> cpufreq_unregister_notifier(&init_cpu_capacity_notifier,
> CPUFREQ_POLICY_NOTIFIER);
> }
>
> >> + return ret;
> >> +
> >> + return cpufreq_register_notifier(&set_freq_scale_notifier,
> >> +CPUFREQ_TRANSITION_NOTIFIER);
> >
> > Don't you have to unregister the other cpufreq notifier if an error is
> > returned and free the mem allocated for cpus_to_visit ?
>
> IMHO, that's not necessary.
>
> The transition notifier works completely independent from the policy
> notifier. In case the latter gets registered correctly and the registration
> of the former fails, the notifier call of the policy notifier still parses
> the capacity-dmips-mhz property information and sets per_cpu(max_freq, cpu).
>
> The notifier call set_freq_scale_callback() of the transition notifier will
> not be called so that frequency invariance always returns
> SCHED_CAPACITY_SCALE.
>
> After the policy notifier has finished its work, it schedules
> parsing_done_work() in which it gets unregistered.
Ok so IIUC, the transition notifier is somehow optional and we still
have the cpu invariance.
In this case, you should not return the error code of
cpufreq_register_notifier(&set_freq_scale_notifier,
CPUFREQ_TRANSITION_NOTIFIER) as the error code of the
register_cpufreq_notifier function.
you should better print a warning like " failed to init frequency
invariance" and return 0 for register_cpufreq_notifier()
>
> [...]
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> On 8 June 2017 at 09:55, Dietmar Eggemann wrote:
Hi Vincent,
Thanks for the review!
[...]
>> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
>>
>> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>>
>> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
>> -CPUFREQ_POLICY_NOTIFIER);
>> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
>> + CPUFREQ_POLICY_NOTIFIER);
>> +
>> + if (ret)
>
> Don't you have to free memory allocated for cpus_to_visit in case of
> errot ? it was not done before your patch as well
Yes, we should free cpus_to_visit if the policy notifier registration
fails. But IMHO also, once the parsing of the capacity-dmips-mhz property
is done. free cpus_to_visit is only used in the notifier call
init_cpu_capacity_callback() after being allocated and initialized in
register_cpufreq_notifier().
We could add something like this as the first patch of this set. Only
mildly tested on Juno. Juri, what do you think?
Author: Dietmar Eggemann
Date: Tue Jun 13 23:21:59 2017 +0100
drivers base/arch_topology: free cpumask cpus_to_visit
Free cpumask cpus_to_visit in case registering
init_cpu_capacity_notifier has failed or the parsing of the cpu
capacity-dmips-mhz property is done. The cpumask cpus_to_visit is
only used inside the notifier call init_cpu_capacity_callback.
Reported-by: Vincent Guittot
Signed-off-by: Dietmar Eggemann
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index d1c33a85059e..f4832c662762 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -206,6 +206,8 @@ static struct notifier_block init_cpu_capacity_notifier = {
static int __init register_cpufreq_notifier(void)
{
+ int ret;
+
/*
* on ACPI-based systems we need to use the default cpu capacity
* until we have the necessary code to parse the cpu capacity, so
@@ -221,13 +223,19 @@ static int __init register_cpufreq_notifier(void)
cpumask_copy(cpus_to_visit, cpu_possible_mask);
- return cpufreq_register_notifier(&init_cpu_capacity_notifier,
-CPUFREQ_POLICY_NOTIFIER);
+ ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
+ CPUFREQ_POLICY_NOTIFIER);
+
+ if (ret)
+ free_cpumask_var(cpus_to_visit);
+
+ return ret;
}
core_initcall(register_cpufreq_notifier);
static void parsing_done_workfn(struct work_struct *work)
{
+ free_cpumask_var(cpus_to_visit);
cpufreq_unregister_notifier(&init_cpu_capacity_notifier,
CPUFREQ_POLICY_NOTIFIER);
}
>> + return ret;
>> +
>> + return cpufreq_register_notifier(&set_freq_scale_notifier,
>> +CPUFREQ_TRANSITION_NOTIFIER);
>
> Don't you have to unregister the other cpufreq notifier if an error is
> returned and free the mem allocated for cpus_to_visit ?
IMHO, that's not necessary.
The transition notifier works completely independent from the policy
notifier. In case the latter gets registered correctly and the registration
of the former fails, the notifier call of the policy notifier still parses
the capacity-dmips-mhz property information and sets per_cpu(max_freq, cpu).
The notifier call set_freq_scale_callback() of the transition notifier will
not be called so that frequency invariance always returns
SCHED_CAPACITY_SCALE.
After the policy notifier has finished its work, it schedules
parsing_done_work() in which it gets unregistered.
[...]
Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
On 8 June 2017 at 09:55, Dietmar Eggemann wrote:
> Implements an arch-specific frequency-scaling function
> topology_get_freq_scale() which provides the following frequency
> scaling factor:
>
> current_freq(cpu) << SCHED_CAPACITY_SHIFT / max_supported_freq(cpu)
>
> The debug output in init_cpu_capacity_callback() has been changed to be
> able to distinguish whether cpu capacity and max frequency or only max
> frequency has been set. The latter case happens on systems where there
> is no or broken cpu capacity binding (cpu node property
> capacity-dmips-mhz) information.
>
> One possible consumer of this is the Per-Entity Load Tracking (PELT)
> mechanism of the task scheduler.
>
> Cc: Catalin Marinas
> Cc: Will Deacon
> Cc: Russell King
> Cc: Greg Kroah-Hartman
> Cc: Juri Lelli
> Signed-off-by: Dietmar Eggemann
> ---
> drivers/base/arch_topology.c | 52
> ---
> include/linux/arch_topology.h | 2 ++
> 2 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 272831c89feb..f6f14670bdab 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -24,12 +24,18 @@
>
> static DEFINE_MUTEX(cpu_scale_mutex);
> static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
> +static DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
>
> unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu)
> {
> return per_cpu(cpu_scale, cpu);
> }
>
> +unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu)
> +{
> + return per_cpu(freq_scale, cpu);
> +}
> +
> void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
> {
> per_cpu(cpu_scale, cpu) = capacity;
> @@ -164,6 +170,7 @@ static cpumask_var_t cpus_to_visit;
> static bool cap_parsing_done;
> static void parsing_done_workfn(struct work_struct *work);
> static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
> +static DEFINE_PER_CPU(unsigned long, max_freq);
>
> static int
> init_cpu_capacity_callback(struct notifier_block *nb,
> @@ -185,6 +192,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
>cpus_to_visit,
>policy->related_cpus);
> for_each_cpu(cpu, policy->related_cpus) {
> + per_cpu(max_freq, cpu) = policy->cpuinfo.max_freq;
> if (cap_parsing_failed)
> continue;
> raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu)
> *
> @@ -195,8 +203,10 @@ init_cpu_capacity_callback(struct notifier_block *nb,
> if (!cap_parsing_failed) {
> topology_normalize_cpu_scale();
> kfree(raw_capacity);
> + pr_debug("cpu_capacity: parsing done\n");
> + } else {
> + pr_debug("cpu_capacity: max frequency parsing
> done\n");
> }
> - pr_debug("cpu_capacity: parsing done\n");
> cap_parsing_done = true;
> schedule_work(&parsing_done_work);
> }
> @@ -208,8 +218,38 @@ static struct notifier_block init_cpu_capacity_notifier
> = {
> .notifier_call = init_cpu_capacity_callback,
> };
>
> +static void set_freq_scale(unsigned int cpu, unsigned long freq)
> +{
> + unsigned long max = per_cpu(max_freq, cpu);
> +
> + if (!max)
> + return;
> +
> + per_cpu(freq_scale, cpu) = (freq << SCHED_CAPACITY_SHIFT) / max;
> +}
> +
> +static int set_freq_scale_callback(struct notifier_block *nb,
> + unsigned long val,
> + void *data)
> +{
> + struct cpufreq_freqs *freq = data;
> +
> + switch (val) {
> + case CPUFREQ_PRECHANGE:
> + set_freq_scale(freq->cpu, freq->new);
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block set_freq_scale_notifier = {
> + .notifier_call = set_freq_scale_callback,
> +};
> +
> static int __init register_cpufreq_notifier(void)
> {
> + int ret;
> +
> /*
> * on ACPI-based systems we need to use the default cpu capacity
> * until we have the necessary code to parse the cpu capacity, so
> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
>
> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>
> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> -CPUFREQ_POLICY_NOTIFIER);
> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> + CPUFREQ_POLICY_NOTIFIER);
> +
> + if (ret)
Don't you have to free memory allocated for cpus_to_visit in ca
[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support
Implements an arch-specific frequency-scaling function
topology_get_freq_scale() which provides the following frequency
scaling factor:
current_freq(cpu) << SCHED_CAPACITY_SHIFT / max_supported_freq(cpu)
The debug output in init_cpu_capacity_callback() has been changed to be
able to distinguish whether cpu capacity and max frequency or only max
frequency has been set. The latter case happens on systems where there
is no or broken cpu capacity binding (cpu node property
capacity-dmips-mhz) information.
One possible consumer of this is the Per-Entity Load Tracking (PELT)
mechanism of the task scheduler.
Cc: Catalin Marinas
Cc: Will Deacon
Cc: Russell King
Cc: Greg Kroah-Hartman
Cc: Juri Lelli
Signed-off-by: Dietmar Eggemann
---
drivers/base/arch_topology.c | 52 ---
include/linux/arch_topology.h | 2 ++
2 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 272831c89feb..f6f14670bdab 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -24,12 +24,18 @@
static DEFINE_MUTEX(cpu_scale_mutex);
static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
+static DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu)
{
return per_cpu(cpu_scale, cpu);
}
+unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu)
+{
+ return per_cpu(freq_scale, cpu);
+}
+
void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
{
per_cpu(cpu_scale, cpu) = capacity;
@@ -164,6 +170,7 @@ static cpumask_var_t cpus_to_visit;
static bool cap_parsing_done;
static void parsing_done_workfn(struct work_struct *work);
static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
+static DEFINE_PER_CPU(unsigned long, max_freq);
static int
init_cpu_capacity_callback(struct notifier_block *nb,
@@ -185,6 +192,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
cpus_to_visit,
policy->related_cpus);
for_each_cpu(cpu, policy->related_cpus) {
+ per_cpu(max_freq, cpu) = policy->cpuinfo.max_freq;
if (cap_parsing_failed)
continue;
raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
@@ -195,8 +203,10 @@ init_cpu_capacity_callback(struct notifier_block *nb,
if (!cap_parsing_failed) {
topology_normalize_cpu_scale();
kfree(raw_capacity);
+ pr_debug("cpu_capacity: parsing done\n");
+ } else {
+ pr_debug("cpu_capacity: max frequency parsing
done\n");
}
- pr_debug("cpu_capacity: parsing done\n");
cap_parsing_done = true;
schedule_work(&parsing_done_work);
}
@@ -208,8 +218,38 @@ static struct notifier_block init_cpu_capacity_notifier = {
.notifier_call = init_cpu_capacity_callback,
};
+static void set_freq_scale(unsigned int cpu, unsigned long freq)
+{
+ unsigned long max = per_cpu(max_freq, cpu);
+
+ if (!max)
+ return;
+
+ per_cpu(freq_scale, cpu) = (freq << SCHED_CAPACITY_SHIFT) / max;
+}
+
+static int set_freq_scale_callback(struct notifier_block *nb,
+ unsigned long val,
+ void *data)
+{
+ struct cpufreq_freqs *freq = data;
+
+ switch (val) {
+ case CPUFREQ_PRECHANGE:
+ set_freq_scale(freq->cpu, freq->new);
+ }
+
+ return 0;
+}
+
+static struct notifier_block set_freq_scale_notifier = {
+ .notifier_call = set_freq_scale_callback,
+};
+
static int __init register_cpufreq_notifier(void)
{
+ int ret;
+
/*
* on ACPI-based systems we need to use the default cpu capacity
* until we have the necessary code to parse the cpu capacity, so
@@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
cpumask_copy(cpus_to_visit, cpu_possible_mask);
- return cpufreq_register_notifier(&init_cpu_capacity_notifier,
-CPUFREQ_POLICY_NOTIFIER);
+ ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
+ CPUFREQ_POLICY_NOTIFIER);
+
+ if (ret)
+ return ret;
+
+ return cpufreq_register_notifier(&set_freq_scale_notifier,
+CPUFREQ_TRANSITION_NOTIFIER);
}
core_initcall(register_cpufreq_notifier);
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 9af3c174c03a..3fb4d8ccb179 100644
--- a/include/linux/arch_to
