| Message ID | 20200722093732.14297-5-ionela.voinescu@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | cpufreq: improve frequency invariance support | expand |
On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu <ionela.voinescu@arm.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@arm.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.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 >
Hi, On Monday 27 Jul 2020 at 16:02:18 (+0200), Rafael J. Wysocki wrote: > On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu > <ionela.voinescu@arm.com> wrote: [..] > > +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). > I'll fix this. Also, depending on the result of the discussion below, it might be best for this to be a warning, not an error. > Besides, IMO on architectures where arch_set_freq_scale() is empty, > this should be empty as well. > Yes, you are right, there are two aspects here: - (1) Whether a driver *can* provide frequency invariance. IOW, whether it implements the callbacks that result in the call to arch_set_freq_scale(). - (2) Whether cpufreq/driver *does* provide frequency invariance. IOW, whether the call to arch_set_freq_scale() actually results in the setting of the scale factor. Even when creating this v2 I was going back and forth between the options for this: (a) cpufreq should report whether it *can* provide frequency invariance (as described at (1)). If we go for this, for clarity I should change s/cpufreq_set_freq_scale/cpufreq_can_set_freq_scale_key s/cpufreq_sets_freq_scale()/cpufreq_can_set_freq_scale() Through this, cpufreq only reports that it calls arch_set_freq_scale(), independent on whether that call results in a scale factor being set. Then it would be up to the caller to ensure this information is used with a proper definition of arch_set_freq_scale(). (b) cpufreq should report whether it *does* provide frequency invariance A way of doing this is to use a arch_set_freq_scale define (as done for other arch functions, for example arch_scale_freq_tick()) and guard this enable_cpufreq_freq_invariance() function based on that definition. Therefore, cpufreq_sets_freq_scale() would report whether enable_cpufreq_freq_invariance() was successful and there is an external definition of arch_set_freq_scale() that sets the scale factor. The current version is somewhat a combination of (a) and (b): cpufreq_set_freq_scale would initially be enabled if the proper callbacks are implemented (a), but later if the weak version of arch_set_freq_scale() is called, we disabled it (b) (as can be seen below). [..] > > __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); I suppose a clear (a) or (b) solution might be better here. IMO, given that (b) cannot actually guarantee that a scale factor is set through arch_set_freq_scale() given cpufreq information about current and maximum frequencies, for me (a) is preferred as it conveys the only information that cpufreq can convey - the fact that it *can* set the scale factor, not that it *does*. Can you please confirm whether you still prefer (b), given the details above? Thank you, Ionela.
On 22-07-20, 10:37, Ionela Voinescu wrote: > 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) { Just checking for !driver->setpolicy should be enough here. > + static_branch_enable_cpuslocked(&cpufreq_set_freq_scale); > + pr_debug("%s: Driver %s can provide frequency invariance.", > + __func__, driver->name); I think a simpler print will work well too. pr_debug("Freq invariance enabled"); __func__ isn't really required as this is the only print with that kind of info in cpufreq.c. > + } else > + pr_err("%s: Driver %s cannot provide frequency invariance.", > + __func__, driver->name); Why not supporting freq-invariance an error ? I will just drop this message completely.
Hi Viresh, On Thursday 30 Jul 2020 at 10:13:46 (+0530), Viresh Kumar wrote: > On 22-07-20, 10:37, Ionela Voinescu wrote: > > 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) { > > Just checking for !driver->setpolicy should be enough here. > Right, cpufreq_register_driver() should check that at least one of them is present (although currently cpufreq_register_driver() will return -EINVAL if .fast_switch() alone is present - something to be fixed). Will do, on both accounts. > > + static_branch_enable_cpuslocked(&cpufreq_set_freq_scale); > > + pr_debug("%s: Driver %s can provide frequency invariance.", > > + __func__, driver->name); > > I think a simpler print will work well too. > > pr_debug("Freq invariance enabled"); > I think the right way of reporting this support is important here. By enabling the static key here, we're not actually enabling frequency invariance. So printing "Freq invariance enabled" would be very misleading. Frequency invariance (scheduler concept) being truly enabled depends on three things: - Having a source of information for current frequency and maximum frequency (cpufreq, counters) - Having arch support for using that information to set the frequency scale factor: arch_set_freq_scale(), arch_scale_freq_tick() - Having arch support for passing the set frequency scale factor to the scheduler and reporting support for frequency invariance: arch_scale_freq_capacity(), arch_scale_freq_invariant(). Therefore, cpufreq can only report that the current driver can be a source of information for frequency invariance "Driver %s can provide frequency invariance", but it can't guarantee that the other conditions are accomplished. So I would recommend to keep this original debug message. > __func__ isn't really required as this is the only print with that > kind of info in cpufreq.c. > Makes sense! > > + } else > > + pr_err("%s: Driver %s cannot provide frequency invariance.", > > + __func__, driver->name); > > Why not supporting freq-invariance an error ? I will just drop this > message completely. Yes, an error does not make sense here. I was thinking to demote it to a warning instead in my previous reply to Rafael, but removing it altogether might be better. Many thanks for the thorough review, Ionela. > > -- > viresh
On 03-08-20, 16:24, Ionela Voinescu wrote: > Right, cpufreq_register_driver() should check that at least one of them > is present > (although currently cpufreq_register_driver() will return > -EINVAL if .fast_switch() alone is present - something to be fixed). I think it is fine as there is no guarantee from cpufreq core if .fast_switch() will get called and so target/target_index must be present. We can't do fast-switch today without schedutil (as only that enables it) and if a notifier gets registered before the driver, then we are gone again. > Will do, on both accounts. > > > > > + static_branch_enable_cpuslocked(&cpufreq_set_freq_scale); > > > + pr_debug("%s: Driver %s can provide frequency invariance.", > > > + __func__, driver->name); > > > > I think a simpler print will work well too. > > > > pr_debug("Freq invariance enabled"); > > > > I think the right way of reporting this support is important here. Yeah, we can't say it is enabled as you explained, though I meant something else here then, i.e. getting rid of driver name and unimportant stuff. What about this now: pr_debug("supports frequency invariance"); This shall get printed as this finally: cpufreq: supports frequency invariance
On Tuesday 04 Aug 2020 at 12:16:56 (+0530), Viresh Kumar wrote: > On 03-08-20, 16:24, Ionela Voinescu wrote: > > Right, cpufreq_register_driver() should check that at least one of them > > is present > > > (although currently cpufreq_register_driver() will return > > -EINVAL if .fast_switch() alone is present - something to be fixed). > > I think it is fine as there is no guarantee from cpufreq core if > .fast_switch() will get called and so target/target_index must be > present. We can't do fast-switch today without schedutil (as only that > enables it) and if a notifier gets registered before the driver, then > we are gone again. > > > Will do, on both accounts. > > > > > > > > + static_branch_enable_cpuslocked(&cpufreq_set_freq_scale); > > > > + pr_debug("%s: Driver %s can provide frequency invariance.", > > > > + __func__, driver->name); > > > > > > I think a simpler print will work well too. > > > > > > pr_debug("Freq invariance enabled"); > > > > > > > I think the right way of reporting this support is important here. > > Yeah, we can't say it is enabled as you explained, though I meant > something else here then, i.e. getting rid of driver name and > unimportant stuff. What about this now: > > pr_debug("supports frequency invariance"); > > This shall get printed as this finally: > > cpufreq: supports frequency invariance > Will do! Thanks, Ionela. > -- > viresh
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); +} + +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
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@arm.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 28 ++++++++++++++++++++++++++++ include/linux/cpufreq.h | 5 +++++ 2 files changed, 33 insertions(+)