[PATCH v2 0/1] Update Energy Model with performance limits

[PATCH v2 0/1] Update Energy Model with performance limits

[Lukasz Luba]

Hi all,

This patch set allows to specify in the EM the range of performance levels that
the device is allowed to operate. It will impact EAS decision, especially for
SoCs 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

The minimum allowed frequency will be taken into account while doing EAS task
placement simulation. When the min frequency is higher for the whole domain
and not driven by the CPUs in that PD utilization, than the energy for
computation in that PD will be higher. This patch helps to reflect that higher
cost.

More explanation can be found in my presentation at OSPM2023 [1].
I have shown experiments with Big CPU running high frequency and increasing
the L3 cache frequency (to reduce the latency), but that impacted Little
CPU which are in the same DVFS domain with L3 cache. It had bad impact for
total energy consumed by small tasks placed on Little CPU. The EAS was not
aware about the min frequency&voltage of the Little CPUs and energy estimation
was wrong.

Changelog:
v2:
- added mutex to guard simultaneous updates and make them atomic (Hongyan)
- added Doxygen two new arguments description (Dietmar)
- changed patch header description (Dietmar) and added an example
- dropped the SCMI patch for now, since the notifications are not merged there yet
v1 [2]:
- basic implementation

Regards,
Lukasz Luba

[1] https://www.youtube.com/watch?v=2C-5uikSbtM&list=PL0fKordpLTjKsBOUcZqnzlHShri4YBL1H
[2] https://lore.kernel.org/lkml/20240403162315.1458337-1-lukasz.luba@arm.com/


Lukasz Luba (1):
  PM: EM: Add min/max available performance state limits

 include/linux/energy_model.h | 24 ++++++++++++++---
 kernel/power/energy_model.c  | 52 ++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 4 deletions(-)

-- 
2.46.0

[PATCH v2 1/1] PM: EM: Add min/max available performance state limits

[Lukasz Luba]

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@arm.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
  * @flags:		See "em_perf_domain flags"
  * @cpus:		Cpumask covering the CPUs of the domain. It's here
  *			for performance reasons to avoid potential cache
@@ -70,6 +72,8 @@ struct em_perf_table {
 struct em_perf_domain {
 	struct em_perf_table __rcu *em_table;
 	int nr_perf_states;
+	int min_ps;
+	int max_ps;
 	unsigned long flags;
 	unsigned long cpus[];
 };
@@ -173,6 +177,8 @@ void em_table_free(struct em_perf_table __rcu *table);
 int em_dev_compute_costs(struct device *dev, struct em_perf_state *table,
 			 int nr_states);
 int em_dev_update_chip_binning(struct device *dev);
+int em_update_performance_limits(struct em_perf_domain *pd,
+		unsigned long freq_min_khz, unsigned long freq_max_khz);
 
 /**
  * em_pd_get_efficient_state() - Get an efficient performance state from the EM
@@ -180,6 +186,8 @@ int em_dev_update_chip_binning(struct device *dev);
  * @nr_perf_states:	Number of performance states
  * @max_util:		Max utilization to map with the EM
  * @pd_flags:		Performance Domain flags
+ * @min_ps:		Minimum allowed Performance State index
+ * @max_ps:		Maximum allowed Performance State index
  *
  * It is called from the scheduler code quite frequently and as a consequence
  * doesn't implement any check.
@@ -189,12 +197,13 @@ int em_dev_update_chip_binning(struct device *dev);
  */
 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);
 	ps = &em_table->state[i];
 
 	/*
@@ -391,6 +401,12 @@ static inline int em_dev_update_chip_binning(struct device *dev)
 {
 	return -EINVAL;
 }
+static inline
+int em_update_performance_limits(struct em_perf_domain *pd,
+		unsigned long freq_min_khz, unsigned long freq_max_khz)
+{
+	return -EINVAL;
+}
 #endif
 
 #endif
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
index 927cc55ba0b3d..436c2b8fdf9eb 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -628,6 +628,8 @@ int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
 		goto unlock;
 
 	dev->em_pd->flags |= flags;
+	dev->em_pd->min_ps = 0;
+	dev->em_pd->max_ps = nr_states - 1;
 
 	em_cpufreq_update_efficiencies(dev, dev->em_pd->em_table->state);
 
@@ -856,3 +858,53 @@ int em_dev_update_chip_binning(struct device *dev)
 	return em_recalc_and_update(dev, pd, em_table);
 }
 EXPORT_SYMBOL_GPL(em_dev_update_chip_binning);
+
+
+/**
+ * em_update_performance_limits() - Update Energy Model with performance
+ *				limits information.
+ * @pd			: Performance Domain with EM that has to be updated.
+ * @freq_min_khz	: New minimum allowed frequency for this device.
+ * @freq_max_khz	: New maximum allowed frequency for this device.
+ *
+ * This function allows to update the EM with information about available
+ * performance levels. It takes the minimum and maximum frequency in kHz
+ * and does internal translation to performance levels.
+ * Returns 0 on success or -EINVAL when failed.
+ */
+int em_update_performance_limits(struct em_perf_domain *pd,
+		unsigned long freq_min_khz, unsigned long freq_max_khz)
+{
+	struct em_perf_state *table;
+	int min_ps = -1;
+	int max_ps = -1;
+	int i;
+
+	if (!pd)
+		return -EINVAL;
+
+	rcu_read_lock();
+	table = em_perf_state_from_pd(pd);
+
+	for (i = 0; i < pd->nr_perf_states; i++) {
+		if (freq_min_khz == table[i].frequency)
+			min_ps = i;
+		if (freq_max_khz == table[i].frequency)
+			max_ps = i;
+	}
+	rcu_read_unlock();
+
+	/* Only update when both are found and sane */
+	if (min_ps < 0 || max_ps < 0 || max_ps < min_ps)
+		return -EINVAL;
+
+
+	/* Guard simultaneous updates and make them atomic */
+	mutex_lock(&em_pd_mutex);
+	pd->min_ps = min_ps;
+	pd->max_ps = max_ps;
+	mutex_unlock(&em_pd_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(em_update_performance_limits);
-- 
2.46.0

Re: [PATCH v2 1/1] PM: EM: Add min/max available performance state limits

[Rafael J. Wysocki]

On Tue, Oct 29, 2024 at 10:43 AM Lukasz Luba <lukasz.luba@arm.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@arm.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
>   * @flags:             See "em_perf_domain flags"
>   * @cpus:              Cpumask covering the CPUs of the domain. It's here
>   *                     for performance reasons to avoid potential cache
> @@ -70,6 +72,8 @@ struct em_perf_table {
>  struct em_perf_domain {
>         struct em_perf_table __rcu *em_table;
>         int nr_perf_states;
> +       int min_ps;
> +       int max_ps;
>         unsigned long flags;
>         unsigned long cpus[];
>  };
> @@ -173,6 +177,8 @@ void em_table_free(struct em_perf_table __rcu *table);
>  int em_dev_compute_costs(struct device *dev, struct em_perf_state *table,
>                          int nr_states);
>  int em_dev_update_chip_binning(struct device *dev);
> +int em_update_performance_limits(struct em_perf_domain *pd,
> +               unsigned long freq_min_khz, unsigned long freq_max_khz);
>
>  /**
>   * em_pd_get_efficient_state() - Get an efficient performance state from the EM
> @@ -180,6 +186,8 @@ int em_dev_update_chip_binning(struct device *dev);
>   * @nr_perf_states:    Number of performance states
>   * @max_util:          Max utilization to map with the EM
>   * @pd_flags:          Performance Domain flags
> + * @min_ps:            Minimum allowed Performance State index
> + * @max_ps:            Maximum allowed Performance State index
>   *
>   * It is called from the scheduler code quite frequently and as a consequence
>   * doesn't implement any check.
> @@ -189,12 +197,13 @@ int em_dev_update_chip_binning(struct device *dev);
>   */
>  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);
>         ps = &em_table->state[i];
>
>         /*
> @@ -391,6 +401,12 @@ static inline int em_dev_update_chip_binning(struct device *dev)
>  {
>         return -EINVAL;
>  }
> +static inline
> +int em_update_performance_limits(struct em_perf_domain *pd,
> +               unsigned long freq_min_khz, unsigned long freq_max_khz)
> +{
> +       return -EINVAL;
> +}
>  #endif
>
>  #endif
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index 927cc55ba0b3d..436c2b8fdf9eb 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -628,6 +628,8 @@ int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
>                 goto unlock;
>
>         dev->em_pd->flags |= flags;
> +       dev->em_pd->min_ps = 0;
> +       dev->em_pd->max_ps = nr_states - 1;
>
>         em_cpufreq_update_efficiencies(dev, dev->em_pd->em_table->state);
>
> @@ -856,3 +858,53 @@ int em_dev_update_chip_binning(struct device *dev)
>         return em_recalc_and_update(dev, pd, em_table);
>  }
>  EXPORT_SYMBOL_GPL(em_dev_update_chip_binning);
> +
> +
> +/**
> + * em_update_performance_limits() - Update Energy Model with performance
> + *                             limits information.
> + * @pd                 : Performance Domain with EM that has to be updated.
> + * @freq_min_khz       : New minimum allowed frequency for this device.
> + * @freq_max_khz       : New maximum allowed frequency for this device.
> + *
> + * This function allows to update the EM with information about available
> + * performance levels. It takes the minimum and maximum frequency in kHz
> + * and does internal translation to performance levels.
> + * Returns 0 on success or -EINVAL when failed.
> + */
> +int em_update_performance_limits(struct em_perf_domain *pd,
> +               unsigned long freq_min_khz, unsigned long freq_max_khz)
> +{
> +       struct em_perf_state *table;
> +       int min_ps = -1;
> +       int max_ps = -1;
> +       int i;
> +
> +       if (!pd)
> +               return -EINVAL;
> +
> +       rcu_read_lock();
> +       table = em_perf_state_from_pd(pd);
> +
> +       for (i = 0; i < pd->nr_perf_states; i++) {
> +               if (freq_min_khz == table[i].frequency)
> +                       min_ps = i;
> +               if (freq_max_khz == table[i].frequency)
> +                       max_ps = i;
> +       }
> +       rcu_read_unlock();
> +
> +       /* Only update when both are found and sane */
> +       if (min_ps < 0 || max_ps < 0 || max_ps < min_ps)
> +               return -EINVAL;
> +
> +
> +       /* Guard simultaneous updates and make them atomic */
> +       mutex_lock(&em_pd_mutex);
> +       pd->min_ps = min_ps;
> +       pd->max_ps = max_ps;
> +       mutex_unlock(&em_pd_mutex);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(em_update_performance_limits);

It would be good to have at least one caller of this function in the tree.

Re: [PATCH v2 1/1] PM: EM: Add min/max available performance state limits

[Lukasz Luba]

Hi Rafael,

On 10/29/24 18:29, Rafael J. Wysocki wrote:
> On Tue, Oct 29, 2024 at 10:43 AM Lukasz Luba <lukasz.luba@arm.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@arm.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
>>    * @flags:             See "em_perf_domain flags"
>>    * @cpus:              Cpumask covering the CPUs of the domain. It's here
>>    *                     for performance reasons to avoid potential cache
>> @@ -70,6 +72,8 @@ struct em_perf_table {
>>   struct em_perf_domain {
>>          struct em_perf_table __rcu *em_table;
>>          int nr_perf_states;
>> +       int min_ps;
>> +       int max_ps;
>>          unsigned long flags;
>>          unsigned long cpus[];
>>   };
>> @@ -173,6 +177,8 @@ void em_table_free(struct em_perf_table __rcu *table);
>>   int em_dev_compute_costs(struct device *dev, struct em_perf_state *table,
>>                           int nr_states);
>>   int em_dev_update_chip_binning(struct device *dev);
>> +int em_update_performance_limits(struct em_perf_domain *pd,
>> +               unsigned long freq_min_khz, unsigned long freq_max_khz);
>>
>>   /**
>>    * em_pd_get_efficient_state() - Get an efficient performance state from the EM
>> @@ -180,6 +186,8 @@ int em_dev_update_chip_binning(struct device *dev);
>>    * @nr_perf_states:    Number of performance states
>>    * @max_util:          Max utilization to map with the EM
>>    * @pd_flags:          Performance Domain flags
>> + * @min_ps:            Minimum allowed Performance State index
>> + * @max_ps:            Maximum allowed Performance State index
>>    *
>>    * It is called from the scheduler code quite frequently and as a consequence
>>    * doesn't implement any check.
>> @@ -189,12 +197,13 @@ int em_dev_update_chip_binning(struct device *dev);
>>    */
>>   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);
>>          ps = &em_table->state[i];
>>
>>          /*
>> @@ -391,6 +401,12 @@ static inline int em_dev_update_chip_binning(struct device *dev)
>>   {
>>          return -EINVAL;
>>   }
>> +static inline
>> +int em_update_performance_limits(struct em_perf_domain *pd,
>> +               unsigned long freq_min_khz, unsigned long freq_max_khz)
>> +{
>> +       return -EINVAL;
>> +}
>>   #endif
>>
>>   #endif
>> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
>> index 927cc55ba0b3d..436c2b8fdf9eb 100644
>> --- a/kernel/power/energy_model.c
>> +++ b/kernel/power/energy_model.c
>> @@ -628,6 +628,8 @@ int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
>>                  goto unlock;
>>
>>          dev->em_pd->flags |= flags;
>> +       dev->em_pd->min_ps = 0;
>> +       dev->em_pd->max_ps = nr_states - 1;
>>
>>          em_cpufreq_update_efficiencies(dev, dev->em_pd->em_table->state);
>>
>> @@ -856,3 +858,53 @@ int em_dev_update_chip_binning(struct device *dev)
>>          return em_recalc_and_update(dev, pd, em_table);
>>   }
>>   EXPORT_SYMBOL_GPL(em_dev_update_chip_binning);
>> +
>> +
>> +/**
>> + * em_update_performance_limits() - Update Energy Model with performance
>> + *                             limits information.
>> + * @pd                 : Performance Domain with EM that has to be updated.
>> + * @freq_min_khz       : New minimum allowed frequency for this device.
>> + * @freq_max_khz       : New maximum allowed frequency for this device.
>> + *
>> + * This function allows to update the EM with information about available
>> + * performance levels. It takes the minimum and maximum frequency in kHz
>> + * and does internal translation to performance levels.
>> + * Returns 0 on success or -EINVAL when failed.
>> + */
>> +int em_update_performance_limits(struct em_perf_domain *pd,
>> +               unsigned long freq_min_khz, unsigned long freq_max_khz)
>> +{
>> +       struct em_perf_state *table;
>> +       int min_ps = -1;
>> +       int max_ps = -1;
>> +       int i;
>> +
>> +       if (!pd)
>> +               return -EINVAL;
>> +
>> +       rcu_read_lock();
>> +       table = em_perf_state_from_pd(pd);
>> +
>> +       for (i = 0; i < pd->nr_perf_states; i++) {
>> +               if (freq_min_khz == table[i].frequency)
>> +                       min_ps = i;
>> +               if (freq_max_khz == table[i].frequency)
>> +                       max_ps = i;
>> +       }
>> +       rcu_read_unlock();
>> +
>> +       /* Only update when both are found and sane */
>> +       if (min_ps < 0 || max_ps < 0 || max_ps < min_ps)
>> +               return -EINVAL;
>> +
>> +
>> +       /* Guard simultaneous updates and make them atomic */
>> +       mutex_lock(&em_pd_mutex);
>> +       pd->min_ps = min_ps;
>> +       pd->max_ps = max_ps;
>> +       mutex_unlock(&em_pd_mutex);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(em_update_performance_limits);
> 
> It would be good to have at least one caller of this function in the tree.

Yes, I know, but we had delays with the SCMI cpufreq to get the
notifications support, which are sent from FW...

The patch using this API was part of v1 but with assumption that
those SCMI notifications are merged.

The patch v1 for the SCMI cpufreq driver [1].

In that v1 cover letter I mentioned that the 2nd patch depends
on notifications [2].

I will have to work with Cristian on that notification in SCMI
then this API will be used. I can see that it stuck for a while
in v5. Let me sort that out (probably not in this merge window
though).

Can we do it this way?

Regards,
Lukasz

[1] 
https://lore.kernel.org/lkml/20240403162315.1458337-3-lukasz.luba@arm.com/
[2] 
https://lore.kernel.org/lkml/20240403162315.1458337-1-lukasz.luba@arm.com/
[3] 
https://lore.kernel.org/lkml/20240603192654.2167620-1-quic_sibis@quicinc.com/

Re: [PATCH v2 1/1] PM: EM: Add min/max available performance state limits

[Lukasz Luba]

On 10/30/24 08:48, Lukasz Luba wrote:
> Hi Rafael,
> 
> On 10/29/24 18:29, Rafael J. Wysocki wrote:
>> On Tue, Oct 29, 2024 at 10:43 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>

[snip]

>>> +EXPORT_SYMBOL_GPL(em_update_performance_limits);
>>
>> It would be good to have at least one caller of this function in the 
>> tree.
> 
> Yes, I know, but we had delays with the SCMI cpufreq to get the
> notifications support, which are sent from FW...
> 
> The patch using this API was part of v1 but with assumption that
> those SCMI notifications are merged.
> 
> The patch v1 for the SCMI cpufreq driver [1].
> 
> In that v1 cover letter I mentioned that the 2nd patch depends
> on notifications [2].
> 
> I will have to work with Cristian on that notification in SCMI
> then this API will be used. I can see that it stuck for a while
> in v5. Let me sort that out (probably not in this merge window
> though).

Just to link the effort which has been started into that direction:

https://lore.kernel.org/lkml/ab36709d-a181-4621-a8e5-0ef38b80186b@arm.com/


> 
> Can we do it this way?
> 
> Regards,
> Lukasz
> 
> [1] 
> https://lore.kernel.org/lkml/20240403162315.1458337-3-lukasz.luba@arm.com/
> [2] 
> https://lore.kernel.org/lkml/20240403162315.1458337-1-lukasz.luba@arm.com/
> [3] 
> https://lore.kernel.org/lkml/20240603192654.2167620-1-quic_sibis@quicinc.com/
> 

Re: [PATCH v2 1/1] PM: EM: Add min/max available performance state limits

[Rafael J. Wysocki]

On Wed, Oct 30, 2024 at 11:00 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> On 10/30/24 08:48, Lukasz Luba wrote:
> > Hi Rafael,
> >
> > On 10/29/24 18:29, Rafael J. Wysocki wrote:
> >> On Tue, Oct 29, 2024 at 10:43 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>
>
> [snip]
>
> >>> +EXPORT_SYMBOL_GPL(em_update_performance_limits);
> >>
> >> It would be good to have at least one caller of this function in the
> >> tree.
> >
> > Yes, I know, but we had delays with the SCMI cpufreq to get the
> > notifications support, which are sent from FW...
> >
> > The patch using this API was part of v1 but with assumption that
> > those SCMI notifications are merged.
> >
> > The patch v1 for the SCMI cpufreq driver [1].
> >
> > In that v1 cover letter I mentioned that the 2nd patch depends
> > on notifications [2].
> >
> > I will have to work with Cristian on that notification in SCMI
> > then this API will be used. I can see that it stuck for a while
> > in v5. Let me sort that out (probably not in this merge window
> > though).
>
> Just to link the effort which has been started into that direction:
>
> https://lore.kernel.org/lkml/ab36709d-a181-4621-a8e5-0ef38b80186b@arm.com/

OK, then this can be queued up as a prerequisite for the upcoming changes.

I would, however, mention that in the patch changelog and add a Link:
tag pointing to the above.

> > [1]
> > https://lore.kernel.org/lkml/20240403162315.1458337-3-lukasz.luba@arm.com/
> > [2]
> > https://lore.kernel.org/lkml/20240403162315.1458337-1-lukasz.luba@arm.com/
> > [3]
> > https://lore.kernel.org/lkml/20240603192654.2167620-1-quic_sibis@quicinc.com/
> >

Re: [PATCH v2 1/1] PM: EM: Add min/max available performance state limits

[Lukasz Luba]

On 10/30/24 11:23, Rafael J. Wysocki wrote:
> On Wed, Oct 30, 2024 at 11:00 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> On 10/30/24 08:48, Lukasz Luba wrote:
>>> Hi Rafael,
>>>
>>> On 10/29/24 18:29, Rafael J. Wysocki wrote:
>>>> On Tue, Oct 29, 2024 at 10:43 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>>
>>
>> [snip]
>>
>>>>> +EXPORT_SYMBOL_GPL(em_update_performance_limits);
>>>>
>>>> It would be good to have at least one caller of this function in the
>>>> tree.
>>>
>>> Yes, I know, but we had delays with the SCMI cpufreq to get the
>>> notifications support, which are sent from FW...
>>>
>>> The patch using this API was part of v1 but with assumption that
>>> those SCMI notifications are merged.
>>>
>>> The patch v1 for the SCMI cpufreq driver [1].
>>>
>>> In that v1 cover letter I mentioned that the 2nd patch depends
>>> on notifications [2].
>>>
>>> I will have to work with Cristian on that notification in SCMI
>>> then this API will be used. I can see that it stuck for a while
>>> in v5. Let me sort that out (probably not in this merge window
>>> though).
>>
>> Just to link the effort which has been started into that direction:
>>
>> https://lore.kernel.org/lkml/ab36709d-a181-4621-a8e5-0ef38b80186b@arm.com/
> 
> OK, then this can be queued up as a prerequisite for the upcoming changes.
> 
> I would, however, mention that in the patch changelog and add a Link:
> tag pointing to the above.
> 

Thank you Rafael!

Re: [PATCH v2 1/1] PM: EM: Add min/max available performance state limits

[Rafael J. Wysocki]

On Tue, Oct 29, 2024 at 10:43 AM Lukasz Luba <lukasz.luba@arm.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@arm.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?

That would improve the code clarity quite a bit IMV.

>   * @flags:             See "em_perf_domain flags"
>   * @cpus:              Cpumask covering the CPUs of the domain. It's here
>   *                     for performance reasons to avoid potential cache
> @@ -70,6 +72,8 @@ struct em_perf_table {
>  struct em_perf_domain {
>         struct em_perf_table __rcu *em_table;
>         int nr_perf_states;
> +       int min_ps;
> +       int max_ps;
>         unsigned long flags;
>         unsigned long cpus[];
>  };
> @@ -173,6 +177,8 @@ void em_table_free(struct em_perf_table __rcu *table);
>  int em_dev_compute_costs(struct device *dev, struct em_perf_state *table,
>                          int nr_states);
>  int em_dev_update_chip_binning(struct device *dev);
> +int em_update_performance_limits(struct em_perf_domain *pd,
> +               unsigned long freq_min_khz, unsigned long freq_max_khz);
>
>  /**
>   * em_pd_get_efficient_state() - Get an efficient performance state from the EM
> @@ -180,6 +186,8 @@ int em_dev_update_chip_binning(struct device *dev);
>   * @nr_perf_states:    Number of performance states
>   * @max_util:          Max utilization to map with the EM
>   * @pd_flags:          Performance Domain flags
> + * @min_ps:            Minimum allowed Performance State index
> + * @max_ps:            Maximum allowed Performance State index
>   *
>   * It is called from the scheduler code quite frequently and as a consequence
>   * doesn't implement any check.
> @@ -189,12 +197,13 @@ int em_dev_update_chip_binning(struct device *dev);
>   */
>  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.

>         ps = &em_table->state[i];
>
>         /*
> @@ -391,6 +401,12 @@ static inline int em_dev_update_chip_binning(struct device *dev)
>  {
>         return -EINVAL;
>  }
> +static inline
> +int em_update_performance_limits(struct em_perf_domain *pd,
> +               unsigned long freq_min_khz, unsigned long freq_max_khz)
> +{
> +       return -EINVAL;
> +}
>  #endif
>
>  #endif
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index 927cc55ba0b3d..436c2b8fdf9eb 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -628,6 +628,8 @@ int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
>                 goto unlock;
>
>         dev->em_pd->flags |= flags;
> +       dev->em_pd->min_ps = 0;
> +       dev->em_pd->max_ps = nr_states - 1;
>
>         em_cpufreq_update_efficiencies(dev, dev->em_pd->em_table->state);
>
> @@ -856,3 +858,53 @@ int em_dev_update_chip_binning(struct device *dev)
>         return em_recalc_and_update(dev, pd, em_table);
>  }
>  EXPORT_SYMBOL_GPL(em_dev_update_chip_binning);
> +
> +
> +/**
> + * em_update_performance_limits() - Update Energy Model with performance
> + *                             limits information.
> + * @pd                 : Performance Domain with EM that has to be updated.
> + * @freq_min_khz       : New minimum allowed frequency for this device.
> + * @freq_max_khz       : New maximum allowed frequency for this device.
> + *
> + * This function allows to update the EM with information about available
> + * performance levels. It takes the minimum and maximum frequency in kHz
> + * and does internal translation to performance levels.
> + * Returns 0 on success or -EINVAL when failed.
> + */
> +int em_update_performance_limits(struct em_perf_domain *pd,
> +               unsigned long freq_min_khz, unsigned long freq_max_khz)
> +{
> +       struct em_perf_state *table;
> +       int min_ps = -1;
> +       int max_ps = -1;
> +       int i;
> +
> +       if (!pd)
> +               return -EINVAL;
> +
> +       rcu_read_lock();
> +       table = em_perf_state_from_pd(pd);
> +
> +       for (i = 0; i < pd->nr_perf_states; i++) {
> +               if (freq_min_khz == table[i].frequency)
> +                       min_ps = i;
> +               if (freq_max_khz == table[i].frequency)
> +                       max_ps = i;
> +       }
> +       rcu_read_unlock();
> +
> +       /* Only update when both are found and sane */
> +       if (min_ps < 0 || max_ps < 0 || max_ps < min_ps)
> +               return -EINVAL;
> +
> +
> +       /* Guard simultaneous updates and make them atomic */
> +       mutex_lock(&em_pd_mutex);
> +       pd->min_ps = min_ps;
> +       pd->max_ps = max_ps;
> +       mutex_unlock(&em_pd_mutex);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(em_update_performance_limits);
> --

Re: [PATCH v2 1/1] PM: EM: Add min/max available performance state limits

[Rafael J. Wysocki]

On Wed, Oct 30, 2024 at 12:33 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 10/30/24 11:23, Rafael J. Wysocki wrote:
> > On Wed, Oct 30, 2024 at 11:00 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >> On 10/30/24 08:48, Lukasz Luba wrote:
> >>> Hi Rafael,
> >>>
> >>> On 10/29/24 18:29, Rafael J. Wysocki wrote:
> >>>> On Tue, Oct 29, 2024 at 10:43 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>>>
> >>
> >> [snip]
> >>
> >>>>> +EXPORT_SYMBOL_GPL(em_update_performance_limits);
> >>>>
> >>>> It would be good to have at least one caller of this function in the
> >>>> tree.
> >>>
> >>> Yes, I know, but we had delays with the SCMI cpufreq to get the
> >>> notifications support, which are sent from FW...
> >>>
> >>> The patch using this API was part of v1 but with assumption that
> >>> those SCMI notifications are merged.
> >>>
> >>> The patch v1 for the SCMI cpufreq driver [1].
> >>>
> >>> In that v1 cover letter I mentioned that the 2nd patch depends
> >>> on notifications [2].
> >>>
> >>> I will have to work with Cristian on that notification in SCMI
> >>> then this API will be used. I can see that it stuck for a while
> >>> in v5. Let me sort that out (probably not in this merge window
> >>> though).
> >>
> >> Just to link the effort which has been started into that direction:
> >>
> >> https://lore.kernel.org/lkml/ab36709d-a181-4621-a8e5-0ef38b80186b@arm.com/
> >
> > OK, then this can be queued up as a prerequisite for the upcoming changes.
> >
> > I would, however, mention that in the patch changelog and add a Link:
> > tag pointing to the above.
> >
>
> Thank you Rafael!

Well, there are a few comments to address first.

Please see the reply to the patch I've just sent.

Re: [PATCH v2 1/1] PM: EM: Add min/max available performance state limits

[Lukasz Luba]

On 10/30/24 11:40, Rafael J. Wysocki wrote:
> On Tue, Oct 29, 2024 at 10:43 AM Lukasz Luba <lukasz.luba@arm.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@arm.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.

Re: [PATCH v2 1/1] PM: EM: Add min/max available performance state limits

[Rafael J. Wysocki]

On Wed, Oct 30, 2024 at 12:49 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 10/30/24 11:40, Rafael J. Wysocki wrote:
> > On Tue, Oct 29, 2024 at 10:43 AM Lukasz Luba <lukasz.luba@arm.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@arm.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);

Yes, that's what I had in mind.

> Thanks for the review. I will send a v3.

Thank you!