[PATCH v2 0/3] cpufreq: avoid redundant driver calls in schedutil

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 0/3] cpufreq: avoid redundant driver calls in schedutil

Steve Muckle
In the series [0] I included a patch which attempted to avoid redundant driver
calls in the schedutil governor by mapping the raw required CPU frequencies to
driver frequencies. This vastly increases the likelihood of detecting a
redundant cpufreq driver call, i.e. one which will end up attempting to set the
CPU frequency to the frequency it is already running at. The redundant call can
be avoided. This is especially valuable in the case of drivers which do not
support fast path updates or if remote CPU cpufreq callbacks are implemented.

Unfortunately the implementation of this in [0] walked the frequency table
directly in schedutil. Rafael pointed out that not all drivers may have a
frequency table and that a driver callback might be implemented to return the
driver frequency associated with a particular target frequency. The driver
could then also cache this lookup and possibly use it on an ensuing
fast_switch. This series implements that approach.

Given that this change is beneficial on its own I've split it out into its own
series from the remote callback support.

[0] https://lkml.org/lkml/2016/5/9/853

Steve Muckle (3):
  cpufreq: add resolve_freq driver callback
  cpufreq: acpi-cpufreq: add resolve_freq callback
  cpufreq: schedutil: map raw required frequency to driver frequency

 drivers/cpufreq/acpi-cpufreq.c   | 56 ++++++++++++++++++++++++++++++----------
 drivers/cpufreq/cpufreq.c        | 25 ++++++++++++++++++
 include/linux/cpufreq.h          | 11 ++++++++
 kernel/sched/cpufreq_schedutil.c | 30 +++++++++++++++------
 4 files changed, 101 insertions(+), 21 deletions(-)

--
2.4.10

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to driver frequency

Steve Muckle
The slow-path frequency transition path is relatively expensive as it
requires waking up a thread to do work. Should support be added for
remote CPU cpufreq updates that is also expensive since it requires an
IPI. These activities should be avoided if they are not necessary.

To that end, calculate the actual driver-supported frequency required by
the new utilization value in schedutil by using the recently added
cpufreq_driver_resolve_freq callback. If it is the same as the
previously requested driver frequency then there is no need to continue
with the update assuming the cpu frequency limits have not changed. This
will have additional benefits should the semantics of the rate limit be
changed to apply solely to frequency transitions rather than to
frequency calculations in schedutil.

The last raw required frequency is cached. This allows the driver
frequency lookup to be skipped in the event that the new raw required
frequency matches the last one, assuming a frequency update has not been
forced due to limits changing (indicated by a next_freq value of
UINT_MAX, see sugov_should_update_freq).

Signed-off-by: Steve Muckle <[hidden email]>
---
 kernel/sched/cpufreq_schedutil.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 154ae3a51e86..ef73ca4d8d78 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -45,6 +45,8 @@ struct sugov_cpu {
  struct update_util_data update_util;
  struct sugov_policy *sg_policy;
 
+ unsigned int cached_raw_freq;
+
  /* The fields below are only needed when sharing a policy. */
  unsigned long util;
  unsigned long max;
@@ -104,7 +106,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
 
 /**
  * get_next_freq - Compute a new frequency for a given cpufreq policy.
- * @policy: cpufreq policy object to compute the new frequency for.
+ * @sg_cpu: schedutil cpu object to compute the new frequency for.
  * @util: Current CPU utilization.
  * @max: CPU capacity.
  *
@@ -119,14 +121,24 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
  * next_freq = C * curr_freq * util_raw / max
  *
  * Take C = 1.25 for the frequency tipping point at (util / max) = 0.8.
+ *
+ * The lowest driver-supported frequency which is equal or greater than the raw
+ * next_freq (as calculated above) is returned.
  */
-static unsigned int get_next_freq(struct cpufreq_policy *policy,
-  unsigned long util, unsigned long max)
+static unsigned int get_next_freq(struct sugov_cpu *sg_cpu, unsigned long util,
+  unsigned long max)
 {
+ struct sugov_policy *sg_policy = sg_cpu->sg_policy;
+ struct cpufreq_policy *policy = sg_policy->policy;
  unsigned int freq = arch_scale_freq_invariant() ?
  policy->cpuinfo.max_freq : policy->cur;
 
- return (freq + (freq >> 2)) * util / max;
+ freq = (freq + (freq >> 2)) * util / max;
+
+ if (freq == sg_cpu->cached_raw_freq && sg_policy->next_freq != UINT_MAX)
+ return sg_policy->next_freq;
+ sg_cpu->cached_raw_freq = freq;
+ return cpufreq_driver_resolve_freq(policy, freq);
 }
 
 static void sugov_update_single(struct update_util_data *hook, u64 time,
@@ -141,13 +153,14 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
  return;
 
  next_f = util == ULONG_MAX ? policy->cpuinfo.max_freq :
- get_next_freq(policy, util, max);
+ get_next_freq(sg_cpu, util, max);
  sugov_update_commit(sg_policy, time, next_f);
 }
 
-static unsigned int sugov_next_freq_shared(struct sugov_policy *sg_policy,
+static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu,
    unsigned long util, unsigned long max)
 {
+ struct sugov_policy *sg_policy = sg_cpu->sg_policy;
  struct cpufreq_policy *policy = sg_policy->policy;
  unsigned int max_f = policy->cpuinfo.max_freq;
  u64 last_freq_update_time = sg_policy->last_freq_update_time;
@@ -187,7 +200,7 @@ static unsigned int sugov_next_freq_shared(struct sugov_policy *sg_policy,
  }
  }
 
- return get_next_freq(policy, util, max);
+ return get_next_freq(sg_cpu, util, max);
 }
 
 static void sugov_update_shared(struct update_util_data *hook, u64 time,
@@ -204,7 +217,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
  sg_cpu->last_update = time;
 
  if (sugov_should_update_freq(sg_policy, time)) {
- next_f = sugov_next_freq_shared(sg_policy, util, max);
+ next_f = sugov_next_freq_shared(sg_cpu, util, max);
  sugov_update_commit(sg_policy, time, next_f);
  }
 
@@ -432,6 +445,7 @@ static int sugov_start(struct cpufreq_policy *policy)
  sg_cpu->util = ULONG_MAX;
  sg_cpu->max = 0;
  sg_cpu->last_update = 0;
+ sg_cpu->cached_raw_freq = 0;
  cpufreq_add_update_util_hook(cpu, &sg_cpu->update_util,
      sugov_update_shared);
  } else {
--
2.4.10

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 2/3] cpufreq: acpi-cpufreq: add resolve_freq callback

Steve Muckle
In reply to this post by Steve Muckle
Support the new resolve_freq cpufreq callback which resolves a target
frequency to a driver-supported frequency without actually setting it.

The target frequency and resolved frequency table entry are cached so
that a subsequent fast_switch operation may avoid the frequency table
walk assuming the requested target frequency is the same.

Suggested-by: Rafael J. Wysocki <[hidden email]>
Signed-off-by: Steve Muckle <[hidden email]>
---
 drivers/cpufreq/acpi-cpufreq.c | 56 ++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 13 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 7f38fb55f223..d87962eda1ed 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -66,6 +66,8 @@ enum {
 
 struct acpi_cpufreq_data {
  struct cpufreq_frequency_table *freq_table;
+ unsigned int cached_lookup_freq;
+ struct cpufreq_frequency_table *cached_lookup_entry;
  unsigned int resume;
  unsigned int cpu_feature;
  unsigned int acpi_perf_cpu;
@@ -458,26 +460,53 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy,
  return result;
 }
 
-unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
-      unsigned int target_freq)
+/*
+ * Find the closest frequency above target_freq.
+ *
+ * The table is sorted in the reverse order with respect to the
+ * frequency and all of the entries are valid (see the initialization).
+ */
+static inline struct cpufreq_frequency_table
+*lookup_freq(struct cpufreq_frequency_table *table, unsigned int target_freq)
 {
- struct acpi_cpufreq_data *data = policy->driver_data;
- struct acpi_processor_performance *perf;
- struct cpufreq_frequency_table *entry;
- unsigned int next_perf_state, next_freq, freq;
+ struct cpufreq_frequency_table *entry = table;
+ unsigned int freq;
 
- /*
- * Find the closest frequency above target_freq.
- *
- * The table is sorted in the reverse order with respect to the
- * frequency and all of the entries are valid (see the initialization).
- */
- entry = data->freq_table;
  do {
  entry++;
  freq = entry->frequency;
  } while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
  entry--;
+
+ return entry;
+}
+
+unsigned int acpi_cpufreq_resolve_freq(struct cpufreq_policy *policy,
+       unsigned int target_freq)
+{
+ struct acpi_cpufreq_data *data = policy->driver_data;
+ struct cpufreq_frequency_table *entry;
+
+ data->cached_lookup_freq = target_freq;
+ entry = lookup_freq(data->freq_table, target_freq);
+ data->cached_lookup_entry = entry;
+
+ return entry->frequency;
+}
+
+unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
+      unsigned int target_freq)
+{
+ struct acpi_cpufreq_data *data = policy->driver_data;
+ struct acpi_processor_performance *perf;
+ struct cpufreq_frequency_table *entry;
+ unsigned int next_perf_state, next_freq;
+
+ if (data->cached_lookup_entry &&
+    data->cached_lookup_freq == target_freq)
+ entry = data->cached_lookup_entry;
+ else
+ entry = lookup_freq(data->freq_table, target_freq);
  next_freq = entry->frequency;
  next_perf_state = entry->driver_data;
 
@@ -918,6 +947,7 @@ static struct cpufreq_driver acpi_cpufreq_driver = {
  .verify = cpufreq_generic_frequency_table_verify,
  .target_index = acpi_cpufreq_target,
  .fast_switch = acpi_cpufreq_fast_switch,
+ .resolve_freq = acpi_cpufreq_resolve_freq,
  .bios_limit = acpi_processor_get_bios_limit,
  .init = acpi_cpufreq_cpu_init,
  .exit = acpi_cpufreq_cpu_exit,
--
2.4.10

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 1/3] cpufreq: add resolve_freq driver callback

Steve Muckle
In reply to this post by Steve Muckle
Cpufreq governors may need to know what a particular target frequency
maps to in the driver without necessarily wanting to set the frequency.
Support this operation via a new cpufreq API,
cpufreq_driver_resolve_freq().

The above API will call a new cpufreq driver callback, resolve_freq(),
if it has been registered by the driver. If that callback has not been
registered and a frequency table is available then the frequency table
is walked using cpufreq_frequency_table_target().

UINT_MAX is returned if no driver callback or frequency table is
available.

Suggested-by: Rafael J. Wysocki <[hidden email]>
Signed-off-by: Steve Muckle <[hidden email]>
---
 drivers/cpufreq/cpufreq.c | 25 +++++++++++++++++++++++++
 include/linux/cpufreq.h   | 11 +++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 77d77a4e3b74..3b44f4bdc071 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1849,6 +1849,31 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
 
+unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
+ unsigned int target_freq)
+{
+ struct cpufreq_frequency_table *freq_table;
+ int index, retval;
+
+ clamp_val(target_freq, policy->min, policy->max);
+
+ if (cpufreq_driver->resolve_freq)
+ return cpufreq_driver->resolve_freq(policy, target_freq);
+
+ freq_table = cpufreq_frequency_get_table(policy->cpu);
+ if (!freq_table)
+ return UINT_MAX;
+
+ retval = cpufreq_frequency_table_target(policy, freq_table,
+ target_freq, CPUFREQ_RELATION_L,
+ &index);
+ if (retval)
+ return UINT_MAX;
+
+ return freq_table[index].frequency;
+}
+EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq);
+
 /* Must set freqs->new to intermediate frequency */
 static int __target_intermediate(struct cpufreq_policy *policy,
  struct cpufreq_freqs *freqs, int index)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 4e81e08db752..675f17f98e75 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -271,6 +271,13 @@ struct cpufreq_driver {
  int (*target_intermediate)(struct cpufreq_policy *policy,
        unsigned int index);
 
+ /*
+ * Return the driver-supported frequency that a particular target
+ * frequency maps to (does not set the new frequency).
+ */
+ unsigned int (*resolve_freq)(struct cpufreq_policy *policy,
+ unsigned int target_freq);
+
  /* should be defined, if possible */
  unsigned int (*get)(unsigned int cpu);
 
@@ -487,6 +494,10 @@ int cpufreq_driver_target(struct cpufreq_policy *policy,
 int __cpufreq_driver_target(struct cpufreq_policy *policy,
    unsigned int target_freq,
    unsigned int relation);
+
+unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
+ unsigned int target_freq);
+
 int cpufreq_register_governor(struct cpufreq_governor *governor);
 void cpufreq_unregister_governor(struct cpufreq_governor *governor);
 
--
2.4.10

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 1/3] cpufreq: add resolve_freq driver callback

viresh kumar
On 25-05-16, 19:52, Steve Muckle wrote:

> Cpufreq governors may need to know what a particular target frequency
> maps to in the driver without necessarily wanting to set the frequency.
> Support this operation via a new cpufreq API,
> cpufreq_driver_resolve_freq().
>
> The above API will call a new cpufreq driver callback, resolve_freq(),
> if it has been registered by the driver. If that callback has not been
> registered and a frequency table is available then the frequency table
> is walked using cpufreq_frequency_table_target().
>
> UINT_MAX is returned if no driver callback or frequency table is
> available.

Why should we return UINT_MAX here? We should return target_freq, no ?

> Suggested-by: Rafael J. Wysocki <[hidden email]>
> Signed-off-by: Steve Muckle <[hidden email]>
> ---
>  drivers/cpufreq/cpufreq.c | 25 +++++++++++++++++++++++++
>  include/linux/cpufreq.h   | 11 +++++++++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 77d77a4e3b74..3b44f4bdc071 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1849,6 +1849,31 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
>  
> +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,
> + unsigned int target_freq)
> +{
> + struct cpufreq_frequency_table *freq_table;
> + int index, retval;
> +
> + clamp_val(target_freq, policy->min, policy->max);
> +
> + if (cpufreq_driver->resolve_freq)
> + return cpufreq_driver->resolve_freq(policy, target_freq);
> +
> + freq_table = cpufreq_frequency_get_table(policy->cpu);

I have sent a separate patch to provide a light weight alternative to
this. If that gets accepted, we can switch over to using it.

> + if (!freq_table)
> + return UINT_MAX;
> +
> + retval = cpufreq_frequency_table_target(policy, freq_table,
> + target_freq, CPUFREQ_RELATION_L,
> + &index);
> + if (retval)
> + return UINT_MAX;
> +
> + return freq_table[index].frequency;
> +}
> +EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq);
> +
>  /* Must set freqs->new to intermediate frequency */
>  static int __target_intermediate(struct cpufreq_policy *policy,
>   struct cpufreq_freqs *freqs, int index)
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 4e81e08db752..675f17f98e75 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -271,6 +271,13 @@ struct cpufreq_driver {
>   int (*target_intermediate)(struct cpufreq_policy *policy,
>         unsigned int index);
>  
> + /*
> + * Return the driver-supported frequency that a particular target
> + * frequency maps to (does not set the new frequency).
> + */
> + unsigned int (*resolve_freq)(struct cpufreq_policy *policy,
> + unsigned int target_freq);

We have 3 categories of cpufreq-drivers today:
1. setpolicy drivers: They don't use the cpufreq governors we are
   working on.
2. non-setpolicy drivers:
  A. with ->target_index() callback, these will always provide a
     freq-table.
  B. with ->target() callback, ONLY these should be allowed to provide
     the ->resolve_freq() callback and no one else.

And so I would suggest adding an additional check in
cpufreq_register_driver() to catch incorrect usage of this callback.

--
viresh
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 2/3] cpufreq: acpi-cpufreq: add resolve_freq callback

viresh kumar
In reply to this post by Steve Muckle
On 25-05-16, 19:53, Steve Muckle wrote:
> Support the new resolve_freq cpufreq callback which resolves a target
> frequency to a driver-supported frequency without actually setting it.

And here is the first abuser of this API as I was talking about in the
earlier patch :)

But, I know why you are doing it and I think we can do it differently.

So, lets assume that the ->resolve_freq() callback will ONLY be
provided by the drivers which also provide a ->target() callback.

i.e. not by acpi-cpufreq at least.

> The target frequency and resolved frequency table entry are cached so
> that a subsequent fast_switch operation may avoid the frequency table
> walk assuming the requested target frequency is the same.
>
> Suggested-by: Rafael J. Wysocki <[hidden email]>
> Signed-off-by: Steve Muckle <[hidden email]>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 56 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 7f38fb55f223..d87962eda1ed 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -66,6 +66,8 @@ enum {
>  
>  struct acpi_cpufreq_data {
>   struct cpufreq_frequency_table *freq_table;
> + unsigned int cached_lookup_freq;
> + struct cpufreq_frequency_table *cached_lookup_entry;

This could have been an integer value 'Index', which could have been
used together with the freq-table to get the freq we wanted.

And, then we can move these two fields into the cpufreq_policy
structure and make them part of the first patch itself.

>   unsigned int resume;
>   unsigned int cpu_feature;
>   unsigned int acpi_perf_cpu;
> @@ -458,26 +460,53 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy,
>   return result;
>  }
>  
> -unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
> -      unsigned int target_freq)
> +/*
> + * Find the closest frequency above target_freq.
> + *
> + * The table is sorted in the reverse order with respect to the
> + * frequency and all of the entries are valid (see the initialization).
> + */
> +static inline struct cpufreq_frequency_table
> +*lookup_freq(struct cpufreq_frequency_table *table, unsigned int target_freq)
>  {
> - struct acpi_cpufreq_data *data = policy->driver_data;
> - struct acpi_processor_performance *perf;
> - struct cpufreq_frequency_table *entry;
> - unsigned int next_perf_state, next_freq, freq;
> + struct cpufreq_frequency_table *entry = table;
> + unsigned int freq;
>  
> - /*
> - * Find the closest frequency above target_freq.
> - *
> - * The table is sorted in the reverse order with respect to the
> - * frequency and all of the entries are valid (see the initialization).
> - */
> - entry = data->freq_table;
>   do {
>   entry++;
>   freq = entry->frequency;
>   } while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
>   entry--;
> +
> + return entry;
> +}
> +
> +unsigned int acpi_cpufreq_resolve_freq(struct cpufreq_policy *policy,
> +       unsigned int target_freq)
> +{
> + struct acpi_cpufreq_data *data = policy->driver_data;
> + struct cpufreq_frequency_table *entry;
> +
> + data->cached_lookup_freq = target_freq;
> + entry = lookup_freq(data->freq_table, target_freq);
> + data->cached_lookup_entry = entry;
> +
> + return entry->frequency;
> +}
> +

And then we could remove this callback from acpi-cpufreq.

> +unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy,
> +      unsigned int target_freq)
> +{
> + struct acpi_cpufreq_data *data = policy->driver_data;
> + struct acpi_processor_performance *perf;
> + struct cpufreq_frequency_table *entry;
> + unsigned int next_perf_state, next_freq;
> +
> + if (data->cached_lookup_entry &&
> +    data->cached_lookup_freq == target_freq)
> + entry = data->cached_lookup_entry;
> + else
> + entry = lookup_freq(data->freq_table, target_freq);

And a static inline callback in cpufreq.h to get this entry.

>   next_freq = entry->frequency;
>   next_perf_state = entry->driver_data;
>  
> @@ -918,6 +947,7 @@ static struct cpufreq_driver acpi_cpufreq_driver = {
>   .verify = cpufreq_generic_frequency_table_verify,
>   .target_index = acpi_cpufreq_target,
>   .fast_switch = acpi_cpufreq_fast_switch,
> + .resolve_freq = acpi_cpufreq_resolve_freq,
>   .bios_limit = acpi_processor_get_bios_limit,
>   .init = acpi_cpufreq_cpu_init,
>   .exit = acpi_cpufreq_cpu_exit,

Sounds reasonable ?

--
viresh
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to driver frequency

viresh kumar
In reply to this post by Steve Muckle
On 25-05-16, 19:53, Steve Muckle wrote:

> The slow-path frequency transition path is relatively expensive as it
> requires waking up a thread to do work. Should support be added for
> remote CPU cpufreq updates that is also expensive since it requires an
> IPI. These activities should be avoided if they are not necessary.
>
> To that end, calculate the actual driver-supported frequency required by
> the new utilization value in schedutil by using the recently added
> cpufreq_driver_resolve_freq callback. If it is the same as the
> previously requested driver frequency then there is no need to continue
> with the update assuming the cpu frequency limits have not changed. This
> will have additional benefits should the semantics of the rate limit be
> changed to apply solely to frequency transitions rather than to
> frequency calculations in schedutil.

Maybe mention here that this patch isn't avoiding those IPIs yet, I
got an impression earlier that they are avoided with it.

> The last raw required frequency is cached. This allows the driver
> frequency lookup to be skipped in the event that the new raw required
> frequency matches the last one, assuming a frequency update has not been
> forced due to limits changing (indicated by a next_freq value of
> UINT_MAX, see sugov_should_update_freq).

I am not sure this is required, see below..

> Signed-off-by: Steve Muckle <[hidden email]>
> ---
>  kernel/sched/cpufreq_schedutil.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 154ae3a51e86..ef73ca4d8d78 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -45,6 +45,8 @@ struct sugov_cpu {
>   struct update_util_data update_util;
>   struct sugov_policy *sg_policy;
>  
> + unsigned int cached_raw_freq;
> +
>   /* The fields below are only needed when sharing a policy. */
>   unsigned long util;
>   unsigned long max;
> @@ -104,7 +106,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
>  
>  /**
>   * get_next_freq - Compute a new frequency for a given cpufreq policy.
> - * @policy: cpufreq policy object to compute the new frequency for.
> + * @sg_cpu: schedutil cpu object to compute the new frequency for.
>   * @util: Current CPU utilization.
>   * @max: CPU capacity.
>   *
> @@ -119,14 +121,24 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
>   * next_freq = C * curr_freq * util_raw / max
>   *
>   * Take C = 1.25 for the frequency tipping point at (util / max) = 0.8.
> + *
> + * The lowest driver-supported frequency which is equal or greater than the raw
> + * next_freq (as calculated above) is returned.
>   */
> -static unsigned int get_next_freq(struct cpufreq_policy *policy,
> -  unsigned long util, unsigned long max)
> +static unsigned int get_next_freq(struct sugov_cpu *sg_cpu, unsigned long util,
> +  unsigned long max)
>  {
> + struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> + struct cpufreq_policy *policy = sg_policy->policy;
>   unsigned int freq = arch_scale_freq_invariant() ?
>   policy->cpuinfo.max_freq : policy->cur;
>  
> - return (freq + (freq >> 2)) * util / max;
> + freq = (freq + (freq >> 2)) * util / max;
> +
> + if (freq == sg_cpu->cached_raw_freq && sg_policy->next_freq != UINT_MAX)
> + return sg_policy->next_freq;

I am not sure what the benefit of the second comparison to UINT_MAX
is. The output of below code will be same if the freq was ==
cached_raw_freq, no matter what.

I also have a doubt (I am quite sure Rafael will have a reason for
that, which I am failing to understand now), on why we are doing
next_freq == UINT_MAX in sugov_should_update_freq().

I understand that because the limits might have changed,
need_freq_update would have been set to true. We should evaluate
next-freq again without worrying about the load or the time since last
evaluation.

But what will happen by forcefully calling the cpufreq routines to
change the frequency, if next_freq hasn't changed even after limits
updates? Wouldn't that call always return early because the new freq
and the current freq are going to be same ?

@Rafael: Sorry for asking this so late :(

--
viresh
Loading...