diff mbox series

[v2] cpufreq: Rebuild sched-domains when removing cpufreq driver

Message ID 20231005134120.1217829-1-pierre.gondois@arm.com
State New
Headers show
Series [v2] cpufreq: Rebuild sched-domains when removing cpufreq driver | expand

Commit Message

Pierre Gondois Oct. 5, 2023, 1:41 p.m. UTC
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(-)

Comments

Viresh Kumar Oct. 6, 2023, 7:07 a.m. UTC | #1
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>
Rafael J. Wysocki Oct. 6, 2023, 8:07 p.m. UTC | #2
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!
diff mbox series

Patch

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