Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <7ecf31b4-3782-41e7-9372-a1d4e7a46d39@arm.com>
Date:   Tue, 12 Dec 2023 19:50:33 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 14/23] PM: EM: Support late CPUs booting and capacity
 adjustment
Content-Language: en-US
To:     Lukasz Luba <lukasz.luba@arm.com>, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, rafael@kernel.org
Cc:     rui.zhang@intel.com, amit.kucheria@verdurent.com,
        amit.kachhap@gmail.com, daniel.lezcano@linaro.org,
        viresh.kumar@linaro.org, len.brown@intel.com, pavel@ucw.cz,
        mhiramat@kernel.org, qyousef@layalina.io, wvw@google.com
References: <20231129110853.94344-1-lukasz.luba@arm.com>
 <20231129110853.94344-15-lukasz.luba@arm.com>
From:   Dietmar Eggemann <dietmar.eggemann@arm.com>
In-Reply-To: <20231129110853.94344-15-lukasz.luba@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

[...]

> + * 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 ?

> +	 */
> +	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?


> +	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)

[...]