diff mbox series

[v4,03/18] PM: EM: Find first CPU online while updating OPP efficiency

Message ID 20230925081139.1305766-4-lukasz.luba@arm.com
State New
Headers
Series Introduce runtime modifiable Energy Model |

Commit Message

Lukasz Luba Sept. 25, 2023, 8:11 a.m. UTC 
  The Energy Model might be updated at runtime and the energy efficiency
for each OPP may change. Thus, there is a need to update also the
cpufreq framework and make it aligned to the new values. In order to
do that, use a first online CPU from the Performance Domain.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 kernel/power/energy_model.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Rafael J. Wysocki Sept. 26, 2023, 6:32 p.m. UTC | #1 
On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> The Energy Model might be updated at runtime and the energy efficiency
> for each OPP may change. Thus, there is a need to update also the
> cpufreq framework and make it aligned to the new values. In order to
> do that, use a first online CPU from the Performance Domain.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  kernel/power/energy_model.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index 42486674b834..3dafdd7731c4 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -243,12 +243,19 @@ em_cpufreq_update_efficiencies(struct device *dev, struct em_perf_state *table)
>         struct em_perf_domain *pd = dev->em_pd;
>         struct cpufreq_policy *policy;
>         int found = 0;
> -       int i;
> +       int i, cpu;
>
>         if (!_is_cpu_device(dev) || !pd)
>                 return;
>
> -       policy = cpufreq_cpu_get(cpumask_first(em_span_cpus(pd)));
> +       /* Try to get a CPU which is online and in this PD */
> +       cpu = cpumask_first_and(em_span_cpus(pd), cpu_active_mask);

The comment talks about "online" and cpu_active_mask is used.  Isn't
it a bit inconsistent?

> +       if (cpu >= nr_cpu_ids) {
> +               dev_warn(dev, "EM: No online CPU for CPUFreq policy\n");
> +               return;
> +       }
> +
> +       policy = cpufreq_cpu_get(cpu);
>         if (!policy) {
>                 dev_warn(dev, "EM: Access to CPUFreq policy failed\n");
>                 return;
> --
> 2.25.1
>
Lukasz Luba Sept. 29, 2023, 8:32 a.m. UTC | #2 
Hi Rafael,

Thank you having reviewing those patches!

On 9/26/23 19:32, Rafael J. Wysocki wrote:
> On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> The Energy Model might be updated at runtime and the energy efficiency
>> for each OPP may change. Thus, there is a need to update also the
>> cpufreq framework and make it aligned to the new values. In order to
>> do that, use a first online CPU from the Performance Domain.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   kernel/power/energy_model.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
>> index 42486674b834..3dafdd7731c4 100644
>> --- a/kernel/power/energy_model.c
>> +++ b/kernel/power/energy_model.c
>> @@ -243,12 +243,19 @@ em_cpufreq_update_efficiencies(struct device *dev, struct em_perf_state *table)
>>          struct em_perf_domain *pd = dev->em_pd;
>>          struct cpufreq_policy *policy;
>>          int found = 0;
>> -       int i;
>> +       int i, cpu;
>>
>>          if (!_is_cpu_device(dev) || !pd)
>>                  return;
>>
>> -       policy = cpufreq_cpu_get(cpumask_first(em_span_cpus(pd)));
>> +       /* Try to get a CPU which is online and in this PD */
>> +       cpu = cpumask_first_and(em_span_cpus(pd), cpu_active_mask);
> 
> The comment talks about "online" and cpu_active_mask is used.  Isn't
> it a bit inconsistent?

good point, I'll change the word to 'active'

> 
>> +       if (cpu >= nr_cpu_ids) {
>> +               dev_warn(dev, "EM: No online CPU for CPUFreq policy\n");
>> +               return;
>> +       }
>> +
>> +       policy = cpufreq_cpu_get(cpu);
>>          if (!policy) {
>>                  dev_warn(dev, "EM: Access to CPUFreq policy failed\n");
>>                  return;
>> --
>> 2.25.1
>>
Daniel Lezcano Oct. 23, 2023, 5:06 p.m. UTC | #3 
Hi Lukasz,

On 25/09/2023 10:11, Lukasz Luba wrote:
> The Energy Model might be updated at runtime and the energy efficiency
> for each OPP may change. Thus, there is a need to update also the
> cpufreq framework and make it aligned to the new values. In order to
> do that, use a first online CPU from the Performance Domain.

I'm failing to do the connection with the description and the change.

Perhaps, the changelog shall explain why 'cpu' must be replaced with the 
first active cpu ?

> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>   kernel/power/energy_model.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index 42486674b834..3dafdd7731c4 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -243,12 +243,19 @@ em_cpufreq_update_efficiencies(struct device *dev, struct em_perf_state *table)
>   	struct em_perf_domain *pd = dev->em_pd;
>   	struct cpufreq_policy *policy;
>   	int found = 0;
> -	int i;
> +	int i, cpu;
>   
>   	if (!_is_cpu_device(dev) || !pd)
>   		return;
>   
> -	policy = cpufreq_cpu_get(cpumask_first(em_span_cpus(pd)));
> +	/* Try to get a CPU which is online and in this PD */
> +	cpu = cpumask_first_and(em_span_cpus(pd), cpu_active_mask);
> +	if (cpu >= nr_cpu_ids) {
> +		dev_warn(dev, "EM: No online CPU for CPUFreq policy\n");
> +		return;
> +	}
> +
> +	policy = cpufreq_cpu_get(cpu);
>   	if (!policy) {
>   		dev_warn(dev, "EM: Access to CPUFreq policy failed\n");
>   		return;
Lukasz Luba Oct. 24, 2023, 7:50 a.m. UTC | #4 
Hi Daniel,

Thanks for looking at the patches!

On 10/23/23 18:06, Daniel Lezcano wrote:
> 
> Hi Lukasz,
> 
> On 25/09/2023 10:11, Lukasz Luba wrote:
>> The Energy Model might be updated at runtime and the energy efficiency
>> for each OPP may change. Thus, there is a need to update also the
>> cpufreq framework and make it aligned to the new values. In order to
>> do that, use a first online CPU from the Performance Domain.
> 
> I'm failing to do the connection with the description and the change.
> 
> Perhaps, the changelog shall explain why 'cpu' must be replaced with the 
> first active cpu ?

It's not a big problem now for EM, since during the boot the first CPU
in the 'policy' is actually registering the EM. Although, this is an
assumption and for the new runtime update of EM, we cannot assume
that first is online. That's the motivation of the change. In a corner
case all CPUs might be put offline, but the EM is still there because
we never unregister EM for CPUs (to not race with task scheduler).

I will add that description to the patch header.

Thanks,
Lukasz
diff mbox series

Patch

diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
index 42486674b834..3dafdd7731c4 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -243,12 +243,19 @@  em_cpufreq_update_efficiencies(struct device *dev, struct em_perf_state *table)
 	struct em_perf_domain *pd = dev->em_pd;
 	struct cpufreq_policy *policy;
 	int found = 0;
-	int i;
+	int i, cpu;
 
 	if (!_is_cpu_device(dev) || !pd)
 		return;
 
-	policy = cpufreq_cpu_get(cpumask_first(em_span_cpus(pd)));
+	/* Try to get a CPU which is online and in this PD */
+	cpu = cpumask_first_and(em_span_cpus(pd), cpu_active_mask);
+	if (cpu >= nr_cpu_ids) {
+		dev_warn(dev, "EM: No online CPU for CPUFreq policy\n");
+		return;
+	}
+
+	policy = cpufreq_cpu_get(cpu);
 	if (!policy) {
 		dev_warn(dev, "EM: Access to CPUFreq policy failed\n");
 		return;