linux-kernel - Re: [RFC][PATCH v0.1 3/6] PM: EM: Add special case to em_dev_register_perf

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: <173507d9-ec20-431d-a444-0531fd346c03@arm.com>
Date: Mon, 18 Nov 2024 15:24:58 +0000
From: Hongyan Xia <hongyan.xia2@....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>,
 Len Brown <len.brown@...el.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>
Subject: Re: [RFC][PATCH v0.1 3/6] PM: EM: Add special case to
 em_dev_register_perf_domain()

On 08/11/2024 16:38, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> Allow em_dev_register_perf_domain() to register a cost-only stub
> perf domain with one-element states table if the .active_power()
> callback is not provided.
> 
> Subsequently, this will be used by the intel_pstate driver to register
> stub perf domains for CPUs on hybrid systems.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
>   kernel/power/energy_model.c |   26 +++++++++++++++++++++++---
>   1 file changed, 23 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/kernel/power/energy_model.c
> ===================================================================
> --- linux-pm.orig/kernel/power/energy_model.c
> +++ linux-pm/kernel/power/energy_model.c
> @@ -426,9 +426,11 @@ static int em_create_pd(struct device *d
>   	if (!em_table)
>   		goto free_pd;
>   
> -	ret = em_create_perf_table(dev, pd, em_table->state, cb, flags);
> -	if (ret)
> -		goto free_pd_table;
> +	if (cb->active_power) {
> +		ret = em_create_perf_table(dev, pd, em_table->state, cb, flags);
> +		if (ret)
> +			goto free_pd_table;
> +	}
>   
>   	ret = em_compute_costs(dev, em_table->state, cb, nr_states, flags);
>   	if (ret)
> @@ -561,11 +563,20 @@ int em_dev_register_perf_domain(struct d
>   {
>   	unsigned long cap, prev_cap = 0;
>   	unsigned long flags = 0;
> +	bool stub_pd = false;
>   	int cpu, ret;
>   
>   	if (!dev || !nr_states || !cb)
>   		return -EINVAL;
>   
> +	if (!cb->active_power) {
> +		if (!cb->get_cost || nr_states > 1 || microwatts)
> +			return -EINVAL;
> +
> +		/* Special case: a stub perf domain. */
> +		stub_pd = true;
> +	}
> +

I wonder if the only purpose of stub_pd is to just skip the capacity 
check below, which doesn't look very nice.

I may be echoing Dietmar's comments here. What's the problem of just 
having 3 domains?

Or, could you just specify the same capacities so that the same-capacity 
check won't fail, but just to use hardware load or CPU pressure to model 
the slight difference in real capacities? This way you'd re-use a lot of 
existing infrastructure.

>   	/*
>   	 * Use a mutex to serialize the registration of performance domains and
>   	 * let the driver-defined callback functions sleep.
> @@ -590,6 +601,15 @@ int em_dev_register_perf_domain(struct d
>   				ret = -EEXIST;
>   				goto unlock;
>   			}
> +
> +			/*
> +			 * The capacity need not be the same for all CPUs in a
> +			 * stub perf domain, so long as the average cost of
> +			 * running on each of them is approximately the same.
> +			 */
> +			if (stub_pd)
> +				continue;
> +
>   			/*
>   			 * All CPUs of a domain must have the same
>   			 * micro-architecture since they all share the same
> 
> 
>