Re: [RFC][PATCH v0.3 5/6] PM: EM: Introduce em_adjust_cpu_capacity()

[Lukasz Luba <lukasz.luba@arm.com>]

On 3/7/25 19:39, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Add a function for updating the Energy Model for a CPU after its
> capacity has changed, which subsequently will be used by the
> intel_pstate driver.
> 
> An EM_PERF_DOMAIN_ARTIFICIAL check is added to em_adjust_new_capacity()
> to prevent it from calling em_compute_costs() for an "artificial" perf
> domain with a NULL cb parameter which would cause it to crash.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Note that this function is needed because the performance level values
> in the EM "state" table need to be adjusted on CPU capacity changes.  In
> the intel_pstate case the cost values associated with them don't change
> because they are artificial anyway, so replacing the entire table just
> in order to update the performance level values is a bit wasteful, but
> it seems to be an exception (in the other cases when the CPU capacity
> changes, the cost values change too AFAICS).
> 
> ---
>   include/linux/energy_model.h |    2 ++
>   kernel/power/energy_model.c  |   28 ++++++++++++++++++++++++----
>   2 files changed, 26 insertions(+), 4 deletions(-)
> 
> --- a/include/linux/energy_model.h
> +++ b/include/linux/energy_model.h
> @@ -179,6 +179,7 @@
>   int em_dev_update_chip_binning(struct device *dev);
>   int em_update_performance_limits(struct em_perf_domain *pd,
>   		unsigned long freq_min_khz, unsigned long freq_max_khz);
> +void em_adjust_cpu_capacity(unsigned int cpu);
>   void em_rebuild_sched_domains(void);
>   
>   /**
> @@ -405,6 +406,7 @@
>   {
>   	return -EINVAL;
>   }
> +void em_adjust_cpu_capacity(unsigned int cpu) {}
>   static inline void em_rebuild_sched_domains(void) {}
>   #endif
>   
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -698,10 +698,12 @@
>   {
>   	int ret;
>   
> -	ret = em_compute_costs(dev, em_table->state, NULL, pd->nr_perf_states,
> -			       pd->flags);
> -	if (ret)
> -		goto free_em_table;
> +	if (!(pd->flags & EM_PERF_DOMAIN_ARTIFICIAL)) {
> +		ret = em_compute_costs(dev, em_table->state, NULL,
> +				       pd->nr_perf_states, pd->flags);
> +		if (ret)
> +			goto free_em_table;
> +	}
>   
>   	ret = em_dev_update_perf_domain(dev, em_table);
>   	if (ret)
> @@ -751,6 +753,24 @@
>   	em_recalc_and_update(dev, pd, em_table);
>   }
>   
> +/**
> + * em_adjust_cpu_capacity() - Adjust the EM for a CPU after a capacity update.
> + * @cpu: Target CPU.
> + *
> + * Adjust the existing EM for @cpu after a capacity update under the assumption
> + * that the capacity has been updated in the same way for all of the CPUs in
> + * the same perf domain.
> + */
> +void em_adjust_cpu_capacity(unsigned int cpu)
> +{
> +	struct device *dev = get_cpu_device(cpu);
> +	struct em_perf_domain *pd;
> +
> +	pd = em_pd_get(dev);
> +	if (pd)
> +		em_adjust_new_capacity(cpu, dev, pd);
> +}
> +
>   static void em_check_capacity_update(void)
>   {
>   	cpumask_var_t cpu_done_mask;
> 
> 
> 

LGTM,

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>