Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Fri, 6 Oct 2023 22:07:41 +0200 | Subject | Re: [PATCH v2] cpufreq: Rebuild sched-domains when removing cpufreq driver |
| |
On Fri, Oct 6, 2023 at 9:07 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 05-10-23, 15:41, Pierre Gondois wrote: > > The Energy Aware Scheduler (EAS) relies on the schedutil governor. > > When moving to/from the schedutil governor, sched domains must be > > rebuilt to allow re-evaluating the enablement conditions of EAS. > > This is done through sched_cpufreq_governor_change(). > > > > Having a cpufreq governor assumes a cpufreq driver is running. > > Inserting/removing a cpufreq driver should trigger a re-evaluation > > of EAS enablement conditions, avoiding to see EAS enabled when > > removing a running cpufreq driver. > > > > Rebuild the sched domains in schedutil's sugov_init()/sugov_exit(), > > allowing to check EAS's enablement condition whenever schedutil > > governor is initialized/exited from. > > Move relevant code up in schedutil.c to avoid a split and conditional > > function declaration. > > Rename sched_cpufreq_governor_change() to sugov_eas_rebuild_sd(). > > > > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > > --- > > drivers/cpufreq/cpufreq.c | 3 +- > > include/linux/cpufreq.h | 8 ----- > > kernel/sched/cpufreq_schedutil.c | 55 +++++++++++++++++--------------- > > 3 files changed, 30 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 60ed89000e82..4bc15634d49c 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -1544,7 +1544,7 @@ static int cpufreq_online(unsigned int cpu) > > > > /* > > * Register with the energy model before > > - * sched_cpufreq_governor_change() is called, which will result > > + * sugov_eas_rebuild_sd() is called, which will result > > * in rebuilding of the sched domains, which should only be done > > * once the energy model is properly initialized for the policy > > * first. > > @@ -2652,7 +2652,6 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, > > ret = cpufreq_start_governor(policy); > > if (!ret) { > > pr_debug("governor change\n"); > > - sched_cpufreq_governor_change(policy, old_gov); > > return 0; > > } > > cpufreq_exit_governor(policy); > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > > index 71d186d6933a..1c5ca92a0555 100644 > > --- a/include/linux/cpufreq.h > > +++ b/include/linux/cpufreq.h > > @@ -1193,14 +1193,6 @@ static inline int of_perf_domain_get_sharing_cpumask(int pcpu, const char *list_ > > } > > #endif > > > > -#if defined(CONFIG_ENERGY_MODEL) && defined(CONFIG_CPU_FREQ_GOV_SCHEDUTIL) > > -void sched_cpufreq_governor_change(struct cpufreq_policy *policy, > > - struct cpufreq_governor *old_gov); > > -#else > > -static inline void sched_cpufreq_governor_change(struct cpufreq_policy *policy, > > - struct cpufreq_governor *old_gov) { } > > -#endif > > - > > extern unsigned int arch_freq_get_on_cpu(int cpu); > > > > #ifndef arch_set_freq_scale > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > > index 4492608b7d7f..901cada51ba7 100644 > > --- a/kernel/sched/cpufreq_schedutil.c > > +++ b/kernel/sched/cpufreq_schedutil.c > > @@ -555,6 +555,31 @@ static const struct kobj_type sugov_tunables_ktype = { > > > > /********************** cpufreq governor interface *********************/ > > > > +#ifdef CONFIG_ENERGY_MODEL > > +static void rebuild_sd_workfn(struct work_struct *work) > > +{ > > + rebuild_sched_domains_energy(); > > +} > > + > > +static DECLARE_WORK(rebuild_sd_work, rebuild_sd_workfn); > > + > > +/* > > + * EAS shouldn't be attempted without sugov, so rebuild the sched_domains > > + * on governor changes to make sure the scheduler knows about it. > > + */ > > +static void sugov_eas_rebuild_sd(void) > > +{ > > + /* > > + * When called from the cpufreq_register_driver() path, the > > + * cpu_hotplug_lock is already held, so use a work item to > > + * avoid nested locking in rebuild_sched_domains(). > > + */ > > + schedule_work(&rebuild_sd_work); > > +} > > +#else > > +static inline void sugov_eas_rebuild_sd(void) { }; > > +#endif > > + > > struct cpufreq_governor schedutil_gov; > > > > static struct sugov_policy *sugov_policy_alloc(struct cpufreq_policy *policy) > > @@ -709,6 +734,8 @@ static int sugov_init(struct cpufreq_policy *policy) > > if (ret) > > goto fail; > > > > + sugov_eas_rebuild_sd(); > > + > > out: > > mutex_unlock(&global_tunables_lock); > > return 0; > > @@ -750,6 +777,8 @@ static void sugov_exit(struct cpufreq_policy *policy) > > sugov_kthread_stop(sg_policy); > > sugov_policy_free(sg_policy); > > cpufreq_disable_fast_switch(policy); > > + > > + sugov_eas_rebuild_sd(); > > } > > > > static int sugov_start(struct cpufreq_policy *policy) > > @@ -838,29 +867,3 @@ struct cpufreq_governor *cpufreq_default_governor(void) > > #endif > > > > cpufreq_governor_init(schedutil_gov); > > - > > -#ifdef CONFIG_ENERGY_MODEL > > -static void rebuild_sd_workfn(struct work_struct *work) > > -{ > > - rebuild_sched_domains_energy(); > > -} > > -static DECLARE_WORK(rebuild_sd_work, rebuild_sd_workfn); > > - > > -/* > > - * EAS shouldn't be attempted without sugov, so rebuild the sched_domains > > - * on governor changes to make sure the scheduler knows about it. > > - */ > > -void sched_cpufreq_governor_change(struct cpufreq_policy *policy, > > - struct cpufreq_governor *old_gov) > > -{ > > - if (old_gov == &schedutil_gov || policy->governor == &schedutil_gov) { > > - /* > > - * When called from the cpufreq_register_driver() path, the > > - * cpu_hotplug_lock is already held, so use a work item to > > - * avoid nested locking in rebuild_sched_domains(). > > - */ > > - schedule_work(&rebuild_sd_work); > > - } > > - > > -} > > -#endif > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Applied as 6.7 material, thanks!
| |