linux-kernel - Re: [RFC][PATCH v021 3/9] PM: EM: Move perf rebuilding function from schedutil to EM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <76c73761-a8ce-4f30-8eed-2a4a4061b8ca@arm.com>
Date: Wed, 11 Dec 2024 10:32:10 +0000
From: Christian Loehle <christian.loehle@....com>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>,
 Linux PM <linux-pm@...r.kernel.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Lukasz Luba <lukasz.luba@....com>,
 Peter Zijlstra <peterz@...radead.org>,
 Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
 Dietmar Eggemann <dietmar.eggemann@....com>,
 Morten Rasmussen <morten.rasmussen@....com>,
 Vincent Guittot <vincent.guittot@...aro.org>,
 Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>,
 Pierre Gondois <pierre.gondois@....com>
Subject: Re: [RFC][PATCH v021 3/9] PM: EM: Move perf rebuilding function from
 schedutil to EM

On 11/29/24 15:59, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> The sugov_eas_rebuild_sd() function defined in the schedutil cpufreq
> governor implements generic functionality that may be useful in other
> places.  In particular, going forward it will be used in the intel_pstate
> driver.
> 
> For this reason, move it from schedutil to the energy model code and
> rename it to em_rebuild_perf_domains().
> 
> This also helps to get rid of some #ifdeffery in schedutil which is a
> plus.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
> 
> v0.1 -> v0.2:
>      * Update the comment regarding :register_em() in cpufreq.
>      * Changelog edits.
> 
> ---
>  drivers/cpufreq/cpufreq.c        |    2 +-
>  include/linux/energy_model.h     |    2 ++
>  kernel/power/energy_model.c      |   17 +++++++++++++++++
>  kernel/sched/cpufreq_schedutil.c |   33 ++++++---------------------------
>  4 files changed, 26 insertions(+), 28 deletions(-)
> 
> Index: linux-pm/kernel/power/energy_model.c
> ===================================================================
> --- linux-pm.orig/kernel/power/energy_model.c
> +++ linux-pm/kernel/power/energy_model.c
> @@ -908,3 +908,20 @@ int em_update_performance_limits(struct
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(em_update_performance_limits);
> +
> +static void rebuild_sd_workfn(struct work_struct *work)
> +{
> +	rebuild_sched_domains_energy();
> +}
> +
> +static DECLARE_WORK(rebuild_sd_work, rebuild_sd_workfn);
> +
> +void em_rebuild_perf_domains(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);
> +}
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -604,31 +604,6 @@ static const struct kobj_type sugov_tuna
>  
>  /********************** 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)
> @@ -784,7 +759,11 @@ static int sugov_init(struct cpufreq_pol
>  		goto fail;
>  
>  out:
> -	sugov_eas_rebuild_sd();
> +	/*
> +	 * EAS shouldn't be attempted without sugov, so rebuild the sched_domains
> +	 * on governor changes to make sure the scheduler knows about them.
> +	 */
> +	em_rebuild_perf_domains();

The sugov mention might be considered stale after the next patch?
Apart from that LGTM.