diff mbox series

[v5,3/5] cpufreq: report whether cpufreq supports Frequency Invariance (FI)

Message ID 20200901205549.30096-4-ionela.voinescu@arm.com (mailing list archive)
State Mainlined, archived
Headers show
Series cpufreq: improve frequency invariance support | expand

Commit Message

Ionela Voinescu Sept. 1, 2020, 8:55 p.m. UTC 
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 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

The cpufreq_supports_freq_invariance() function only returns whether
cpufreq is instrumented with the arch_set_freq_scale() calls that
result in support for frequency invariance. Due to the lack of knowledge
on whether the implementation of arch_set_freq_scale() actually results
in the setting of a scale factor based on cpufreq information, it is up
to the architecture code to ensure the setting and provision of the
scale factor to the scheduler.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 16 ++++++++++++++++
 include/linux/cpufreq.h   |  5 +++++
 2 files changed, 21 insertions(+)

Comments

Sudeep Holla Sept. 2, 2020, 1:28 p.m. UTC | #1 
On Tue, Sep 01, 2020 at 09:55:47PM +0100, Ionela Voinescu 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 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
> 
> The cpufreq_supports_freq_invariance() function only returns whether
> cpufreq is instrumented with the arch_set_freq_scale() calls that
> result in support for frequency invariance. Due to the lack of knowledge
> on whether the implementation of arch_set_freq_scale() actually results
> in the setting of a scale factor based on cpufreq information, it is up
> to the architecture code to ensure the setting and provision of the
> scale factor to the scheduler.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 16 ++++++++++++++++
>  include/linux/cpufreq.h   |  5 +++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 4d5fe777184a..570bf2ebe9d4 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -61,6 +61,12 @@ static struct cpufreq_driver *cpufreq_driver;
>  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
>  static DEFINE_RWLOCK(cpufreq_driver_lock);
>  
> +static DEFINE_STATIC_KEY_FALSE(cpufreq_freq_invariance);
> +bool cpufreq_supports_freq_invariance(void)
> +{
> +	return static_branch_likely(&cpufreq_freq_invariance);
> +}
> +
>  /* Flag to suspend/resume CPUFreq governors */
>  static bool cpufreq_suspended;
>  
> @@ -2720,6 +2726,15 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>  	cpufreq_driver = driver_data;
>  	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>  
> +	/*
> +	 * Mark support for the scheduler's frequency invariance engine for
> +	 * drivers that implement target(), target_index() or fast_switch().
> +	 */
> +	if (!cpufreq_driver->setpolicy) {
> +		static_branch_enable_cpuslocked(&cpufreq_freq_invariance);
> +		pr_debug("supports frequency invariance");
> +	}
> +
>  	if (driver_data->setpolicy)

[super nit] while I understand cpufreq_driver = driver_data, it looks odd
if 2 consecutive statements refer it with different variables. Or am I
confusing myself hugely.
Ionela Voinescu Sept. 3, 2020, 1:45 p.m. UTC | #2 
Hi Sudeep,

Thank you for your review here and for the other patches.

On Wednesday 02 Sep 2020 at 14:28:38 (+0100), Sudeep Holla wrote:
[..]
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 4d5fe777184a..570bf2ebe9d4 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -61,6 +61,12 @@ static struct cpufreq_driver *cpufreq_driver;
> >  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> >  static DEFINE_RWLOCK(cpufreq_driver_lock);
> >  
> > +static DEFINE_STATIC_KEY_FALSE(cpufreq_freq_invariance);
> > +bool cpufreq_supports_freq_invariance(void)
> > +{
> > +	return static_branch_likely(&cpufreq_freq_invariance);
> > +}
> > +
> >  /* Flag to suspend/resume CPUFreq governors */
> >  static bool cpufreq_suspended;
> >  
> > @@ -2720,6 +2726,15 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
> >  	cpufreq_driver = driver_data;
> >  	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> >  
> > +	/*
> > +	 * Mark support for the scheduler's frequency invariance engine for
> > +	 * drivers that implement target(), target_index() or fast_switch().
> > +	 */
> > +	if (!cpufreq_driver->setpolicy) {
> > +		static_branch_enable_cpuslocked(&cpufreq_freq_invariance);
> > +		pr_debug("supports frequency invariance");
> > +	}
> > +
> >  	if (driver_data->setpolicy)
> 
> [super nit] while I understand cpufreq_driver = driver_data, it looks odd
> if 2 consecutive statements refer it with different variables. Or am I
> confusing myself hugely.
> 

No, you are right. If you look at the rest of the register function,
after cpufreq_driver = driver_data, both driver_data and cpufreq_driver
are used. For me using cpufreq_driver seemed more natural as after being
assigned driver_data, it will continue to be used after registration.

If it's alright with you I won't make this change for now. It's possible
that a better solution is to change the other occurrences of either
cpufreq_driver or driver_data in a separate patch, to make things
consistent across the function.

Thank you,
Ionela.

> -- 
> Regards,
> Sudeep
Sudeep Holla Sept. 3, 2020, 2:19 p.m. UTC | #3 
On Thu, Sep 03, 2020 at 02:45:08PM +0100, Ionela Voinescu wrote:
> Hi Sudeep,
> 
> Thank you for your review here and for the other patches.
> 
> On Wednesday 02 Sep 2020 at 14:28:38 (+0100), Sudeep Holla wrote:
> [..]
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 4d5fe777184a..570bf2ebe9d4 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -61,6 +61,12 @@ static struct cpufreq_driver *cpufreq_driver;
> > >  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> > >  static DEFINE_RWLOCK(cpufreq_driver_lock);
> > >  
> > > +static DEFINE_STATIC_KEY_FALSE(cpufreq_freq_invariance);
> > > +bool cpufreq_supports_freq_invariance(void)
> > > +{
> > > +	return static_branch_likely(&cpufreq_freq_invariance);
> > > +}
> > > +
> > >  /* Flag to suspend/resume CPUFreq governors */
> > >  static bool cpufreq_suspended;
> > >  
> > > @@ -2720,6 +2726,15 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
> > >  	cpufreq_driver = driver_data;
> > >  	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> > >  
> > > +	/*
> > > +	 * Mark support for the scheduler's frequency invariance engine for
> > > +	 * drivers that implement target(), target_index() or fast_switch().
> > > +	 */
> > > +	if (!cpufreq_driver->setpolicy) {
> > > +		static_branch_enable_cpuslocked(&cpufreq_freq_invariance);
> > > +		pr_debug("supports frequency invariance");
> > > +	}
> > > +
> > >  	if (driver_data->setpolicy)
> > 
> > [super nit] while I understand cpufreq_driver = driver_data, it looks odd
> > if 2 consecutive statements refer it with different variables. Or am I
> > confusing myself hugely.
> > 
> 
> No, you are right. If you look at the rest of the register function,
> after cpufreq_driver = driver_data, both driver_data and cpufreq_driver
> are used. For me using cpufreq_driver seemed more natural as after being
> assigned driver_data, it will continue to be used after registration.
>

Ah OK, I haven't seen the whole file/function, just looked at the patch.

> If it's alright with you I won't make this change for now. It's possible
> that a better solution is to change the other occurrences of either
> cpufreq_driver or driver_data in a separate patch, to make things
> consistent across the function.
> 

I am fine to keep it as is, hence I mentioned it as super nit. If there
are other occurrences, then better to take it up separately.
diff mbox series

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4d5fe777184a..570bf2ebe9d4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -61,6 +61,12 @@  static struct cpufreq_driver *cpufreq_driver;
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
 static DEFINE_RWLOCK(cpufreq_driver_lock);
 
+static DEFINE_STATIC_KEY_FALSE(cpufreq_freq_invariance);
+bool cpufreq_supports_freq_invariance(void)
+{
+	return static_branch_likely(&cpufreq_freq_invariance);
+}
+
 /* Flag to suspend/resume CPUFreq governors */
 static bool cpufreq_suspended;
 
@@ -2720,6 +2726,15 @@  int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 	cpufreq_driver = driver_data;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+	/*
+	 * Mark support for the scheduler's frequency invariance engine for
+	 * drivers that implement target(), target_index() or fast_switch().
+	 */
+	if (!cpufreq_driver->setpolicy) {
+		static_branch_enable_cpuslocked(&cpufreq_freq_invariance);
+		pr_debug("supports frequency invariance");
+	}
+
 	if (driver_data->setpolicy)
 		driver_data->flags |= CPUFREQ_CONST_LOOPS;
 
@@ -2789,6 +2804,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_freq_invariance);
 	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 a911e5d06845..e54767e2a68a 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_supports_freq_invariance(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_supports_freq_invariance(void)
+{
+	return false;
+}
 static inline void disable_cpufreq(void) { }
 #endif