Re: [PATCH v5 14/23] PM: EM: Support late CPUs booting and capacity adjustment

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

On 12/12/23 18:50, Dietmar Eggemann wrote:
> On 29/11/2023 12:08, Lukasz Luba wrote:
>> The patch adds needed infrastructure to handle the late CPUs boot, which
>> might change the previous CPUs capacity values. With this changes the new
>> CPUs which try to register EM will trigger the needed re-calculations for
>> other CPUs EMs. Thanks to that the em_per_state::performance values will
>> be aligned with the CPU capacity information after all CPUs finish the
>> boot and EM registrations.
> 
> IMHO, it's worth mentioning here that this added functionality is the 1.
> use case of the modifiable EM.

Make sense. I will add that. It's quite important information, since
it also justifies the EM update feature.

> 
> [...]
> 
>> + * Adjustment of CPU performance values after boot, when all CPUs capacites
>> + * are correctly calculated.
>> + */
>> +static void em_adjust_new_capacity(struct device *dev,
>> +				   struct em_perf_domain *pd,
>> +				   u64 max_cap)
>> +{
> 
> [...]
> 
>> +	/*
>> +	 * This is one-time-update, so give up the ownership in this updater.
>> +	 * The EM fwk will keep the reference and free the memory when needed.
> 
> s/fwk/framework ?

OK

> 
>> +	 */
>> +	em_free_table(runtime_table);
>> +}
>> +
>> +static void em_check_capacity_update(void)
>> +{
>> +	cpumask_var_t cpu_done_mask;
>> +	struct em_perf_state *table;
>> +	struct em_perf_domain *pd;
>> +	unsigned long cpu_capacity;
>> +	int cpu;
>> +
>> +	if (!zalloc_cpumask_var(&cpu_done_mask, GFP_KERNEL)) {
>> +		pr_warn("no free memory\n");
>> +		return;
>> +	}
>> +
>> +	/* Check if CPUs capacity has changed than update EM */
> 
> s/than/then ?
> 
> Maybe this comment is not needed since there is (1) further down?

Yes, I'll remove that.

> 
> 
>> +	for_each_possible_cpu(cpu) {
>> +		struct cpufreq_policy *policy;
>> +		unsigned long em_max_perf;
>> +		struct device *dev;
>> +		int nr_states;
>> +
>> +		if (cpumask_test_cpu(cpu, cpu_done_mask))
>> +			continue;
>> +
>> +		policy = cpufreq_cpu_get(cpu);
>> +		if (!policy) {
>> +			pr_debug("Accessing cpu%d policy failed\n", cpu);
>> +			schedule_delayed_work(&em_update_work,
>> +					      msecs_to_jiffies(1000));
>> +			break;
>> +		}
>> +		cpufreq_cpu_put(policy);
>> +
>> +		pd = em_cpu_get(cpu);
>> +		if (!pd || em_is_artificial(pd))
>> +			continue;
>> +
>> +		cpumask_or(cpu_done_mask, cpu_done_mask,
>> +			   em_span_cpus(pd));
>> +
>> +		nr_states = pd->nr_perf_states;
>> +		cpu_capacity = arch_scale_cpu_capacity(cpu);
>> +
>> +		table = em_get_table(pd);
>> +		em_max_perf = table[pd->nr_perf_states - 1].performance;
>> +		em_put_table();
>> +
>> +		/*
>> +		 * Check if the CPU capacity has been adjusted during boot
>> +		 * and trigger the update for new performance values.
>> +		 */
> 
> (1)
> 
> [...]