[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <96e694e3-2a65-4f39-ad35-3d1e1459102f@arm.com>
Date: Wed, 30 Oct 2024 11:50:57 +0000
From: Lukasz Luba <lukasz.luba@....com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
dietmar.eggemann@....com
Subject: Re: [PATCH v2 1/1] PM: EM: Add min/max available performance state
limits
On 10/30/24 11:40, Rafael J. Wysocki wrote:
> On Tue, Oct 29, 2024 at 10:43 AM Lukasz Luba <lukasz.luba@....com> wrote:
>>
>> On some devices there are HW dependencies for shared frequency and voltage
>> between devices. It will impact Energy Aware Scheduler (EAS) decision,
>> where CPUs share the voltage & frequency domain with other CPUs or devices
>> e.g.
>> - Mid CPUs + Big CPU
>> - Little CPU + L3 cache in DSU
>> - some other device + Little CPUs
>>
>> Detailed explanation of one example:
>> When the L3 cache frequency is increased, the affected Little CPUs might
>> run at higher voltage and frequency. That higher voltage causes higher CPU
>> power and thus more energy is used for running the tasks. This is
>> important for background running tasks, which try to run on energy
>> efficient CPUs.
>>
>> Therefore, add performance state limits which are applied for the device
>> (in this case CPU). This is important on SoCs with HW dependencies
>> mentioned above so that the Energy Aware Scheduler (EAS) does not use
>> performance states outside the valid min-max range for energy calculation.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@....com>
>> ---
>> include/linux/energy_model.h | 24 ++++++++++++++---
>> kernel/power/energy_model.c | 52 ++++++++++++++++++++++++++++++++++++
>> 2 files changed, 72 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
>> index 1ff52020cf757..e83bf230e18d1 100644
>> --- a/include/linux/energy_model.h
>> +++ b/include/linux/energy_model.h
>> @@ -55,6 +55,8 @@ struct em_perf_table {
>> * struct em_perf_domain - Performance domain
>> * @em_table: Pointer to the runtime modifiable em_perf_table
>> * @nr_perf_states: Number of performance states
>> + * @min_ps: Minimum allowed Performance State index
>> + * @max_ps: Maximum allowed Performance State index
>
> Any problem with renaming these to min_perf_state and max_perf_state
> respectively?
OK, I will change those names.
>
> That would improve the code clarity quite a bit IMV.
>
[snip]
>> static inline int
>> em_pd_get_efficient_state(struct em_perf_state *table, int nr_perf_states,
>> - unsigned long max_util, unsigned long pd_flags)
>> + unsigned long max_util, unsigned long pd_flags,
>> + int min_ps, int max_ps)
>> {
>> struct em_perf_state *ps;
>> int i;
>>
>> - for (i = 0; i < nr_perf_states; i++) {
>> + for (i = min_ps; i <= max_ps; i++) {
>> ps = &table[i];
>> if (ps->performance >= max_util) {
>> if (pd_flags & EM_PERF_DOMAIN_SKIP_INEFFICIENCIES &&
>> @@ -204,7 +213,7 @@ em_pd_get_efficient_state(struct em_perf_state *table, int nr_perf_states,
>> }
>> }
>>
>> - return nr_perf_states - 1;
>> + return max_ps;
>> }
>>
>> /**
>> @@ -254,7 +263,8 @@ static inline unsigned long em_cpu_energy(struct em_perf_domain *pd,
>> */
>> em_table = rcu_dereference(pd->em_table);
>> i = em_pd_get_efficient_state(em_table->state, pd->nr_perf_states,
>> - max_util, pd->flags);
>> + max_util, pd->flags, pd->min_ps,
>> + pd->max_ps);
>
> Couldn't em_pd_get_efficient_state() just take pd as an argument and
> dereference it by itself?
>
> The code would be much easier to follow then.
That's possible. I will keep the em_table rcu_dereference as is, so
only the rest of arguments with 'pd->' will be taken inside
em_pd_get_efficient_state().
So the call would look like:
em_pd_get_efficient_state(em_table->state, pd, max_util);
Thanks for the review. I will send a v3.
Powered by blists - more mailing lists