Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670315 > unrolled thread

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant  load-tracking support Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-20 08:20 +0200
    Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking  support Saravana Kannan <skannan@codeaurora.org> - 2017-06-21 02:40 +0200
      Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant  load-tracking support Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-21 07:40 +0200
        Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant  load-tracking support Morten Rasmussen <morten.rasmussen@arm.com> - 2017-06-21 19:00 +0200
          Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant  load-tracking support Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-22 06:10 +0200
            Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant  load-tracking support Morten Rasmussen <morten.rasmussen@arm.com> - 2017-06-22 12:00 +0200
      Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant  load-tracking support Dietmar Eggemann <dietmar.eggemann@arm.com> - 2017-06-21 19:10 +0200
    Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant  load-tracking support Dietmar Eggemann <dietmar.eggemann@arm.com> - 2017-06-21 18:40 +0200
      Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant  load-tracking support Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-22 06:00 +0200

#1670315 — Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
<dietmar.eggemann@arm.com> 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

[toc] | [next] | [standalone]

#1671260 — 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
> <dietmar.eggemann@arm.com> 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

[toc] | [prev] | [next] | [standalone]

#1671367

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
> ><dietmar.eggemann@arm.com> 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

[toc] | [prev] | [next] | [standalone]

#1671812

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
> > ><dietmar.eggemann@arm.com> 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

[toc] | [prev] | [next] | [standalone]

#1672295

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

[toc] | [prev] | [next] | [standalone]

#1672480

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

[toc] | [prev] | [next] | [standalone]

#1671817

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
>> <dietmar.eggemann@arm.com> 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.

[...]

[toc] | [prev] | [next] | [standalone]

#1671790

On 20/06/17 07:17, Viresh Kumar wrote:
> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
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 <dietmar.eggemann@arm.com>
---
 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);
 }

 #else
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1672289

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 <dietmar.eggemann@arm.com>
> 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 <dietmar.eggemann@arm.com>
> ---
>  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);
>  }

This may work, but lets see if we can find a way of doing this for
everyone at once.

(I will continue to reply on Morten's email now)..

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web