Message-ID: <CAJZ5v0jvvEvxs2dwsGd1kerTT3pJTJj6fVg0ndtdeUhiq+K_UQ@mail.gmail.com>
Date:   Mon, 27 Jul 2020 16:02:18 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Ionela Voinescu <ionela.voinescu@....com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Sudeep Holla <sudeep.holla@....com>,
        Will Deacon <will@...nel.org>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency
 Invariance (FI)

On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
<ionela.voinescu@....com> wrote:
>
> Now that the update of the FI scale factor is done in cpufreq core for
> selected functions - target(), target_index() and fast_switch(),
> we can provide feedback to the task scheduler and architecture code
> on whether cpufreq supports FI.
>
> For this purpose, provide error and debug messages, together with an
> external function to expose whether the cpufreq drivers support FI, by
> using a static key.
>
> The logic behind the enablement of cpufreq-based invariance is as
> follows:
>  - cpufreq-based invariance is disabled by default
>  - cpufreq-based invariance is enabled if any of the callbacks
>    above is implemented while the unsupported setpolicy() is not
>  - if enabled, cpufreq-based invariance will be disabled during the
>    call of the default arch_set_freq_scale() function which does
>    not set a scale factor.
>
> Signed-off-by: Ionela Voinescu <ionela.voinescu@....com>
> Cc: Rafael J. Wysocki <rjw@...ysocki.net>
> Cc: Viresh Kumar <viresh.kumar@...aro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 28 ++++++++++++++++++++++++++++
>  include/linux/cpufreq.h   |  5 +++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 3497c1cd6818..1d0b046fe8e9 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -61,6 +61,9 @@ static struct cpufreq_driver *cpufreq_driver;
>  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
>  static DEFINE_RWLOCK(cpufreq_driver_lock);
>
> +/* Mark support for the scheduler's frequency invariance engine */
> +static DEFINE_STATIC_KEY_FALSE(cpufreq_set_freq_scale);
> +
>  /* Flag to suspend/resume CPUFreq governors */
>  static bool cpufreq_suspended;
>
> @@ -69,6 +72,25 @@ static inline bool has_target(void)
>         return cpufreq_driver->target_index || cpufreq_driver->target;
>  }
>
> +static inline
> +void enable_cpufreq_freq_invariance(struct cpufreq_driver *driver)
> +{
> +       if ((driver->target || driver->target_index || driver->fast_switch) &&
> +           !driver->setpolicy) {
> +
> +               static_branch_enable_cpuslocked(&cpufreq_set_freq_scale);
> +               pr_debug("%s: Driver %s can provide frequency invariance.",
> +                        __func__, driver->name);
> +       } else
> +               pr_err("%s: Driver %s cannot provide frequency invariance.",
> +               __func__, driver->name);

This doesn't follow the kernel coding style (the braces around the
pr_err() statement are missing).

Besides, IMO on architectures where arch_set_freq_scale() is empty,
this should be empty as well.

> +}
> +
> +bool cpufreq_sets_freq_scale(void)
> +{
> +       return static_branch_likely(&cpufreq_set_freq_scale);
> +}
> +
>  /* internal prototypes */
>  static unsigned int __cpufreq_get(struct cpufreq_policy *policy);
>  static int cpufreq_init_governor(struct cpufreq_policy *policy);
> @@ -159,6 +181,9 @@ EXPORT_SYMBOL_GPL(get_cpu_idle_time);
>  __weak void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
>                 unsigned long max_freq)
>  {
> +       if (cpufreq_sets_freq_scale())
> +               static_branch_disable_cpuslocked(&cpufreq_set_freq_scale);
> +
>  }
>  EXPORT_SYMBOL_GPL(arch_set_freq_scale);
>
> @@ -2722,6 +2747,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>         cpufreq_driver = driver_data;
>         write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>
> +       enable_cpufreq_freq_invariance(cpufreq_driver);
> +
>         if (driver_data->setpolicy)
>                 driver_data->flags |= CPUFREQ_CONST_LOOPS;
>
> @@ -2791,6 +2818,7 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver)
>         cpus_read_lock();
>         subsys_interface_unregister(&cpufreq_interface);
>         remove_boost_sysfs_file();
> +       static_branch_disable_cpuslocked(&cpufreq_set_freq_scale);
>         cpuhp_remove_state_nocalls_cpuslocked(hp_online);
>
>         write_lock_irqsave(&cpufreq_driver_lock, flags);
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index e62b022cb07e..f81215ad76f1 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -217,6 +217,7 @@ void refresh_frequency_limits(struct cpufreq_policy *policy);
>  void cpufreq_update_policy(unsigned int cpu);
>  void cpufreq_update_limits(unsigned int cpu);
>  bool have_governor_per_policy(void);
> +bool cpufreq_sets_freq_scale(void);
>  struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy);
>  void cpufreq_enable_fast_switch(struct cpufreq_policy *policy);
>  void cpufreq_disable_fast_switch(struct cpufreq_policy *policy);
> @@ -237,6 +238,10 @@ static inline unsigned int cpufreq_get_hw_max_freq(unsigned int cpu)
>  {
>         return 0;
>  }
> +static inline bool cpufreq_sets_freq_scale(void)
> +{
> +       return false;
> +}
>  static inline void disable_cpufreq(void) { }
>  #endif
>
> --
> 2.17.1
>

Powered by blists - more mailing lists