Re: [PATCH v2] PM: EM: Exclude boost frequencies from inefficiency ranking

[Guo Chen <guo.chen@oss.qualcomm.com>]

On 11/6/2026 11:15 PM, Rafael J. Wysocki (Intel) wrote:
> On Tue, Nov 3, 2026 at 12:21 PM Guo Chen <guo.chen@oss.qualcomm.com> wrote:
>> em_compute_costs() ranks all the frequencies of the table, boost ones
>> included. A non-boost frequency can be marked inefficient only because
>> a faster boost frequency has a lower or equal cost, e.g.
>>
>>    3.9 GHz  4.0 GHz (cost C)  4.1 GHz (boost, cost C)
>>
>> With boost disabled 4.1 GHz cannot be selected, but 4.0 GHz is still
>> skipped by governors rounding down to efficient frequencies (e.g.
>> ondemand with CPUFREQ_RELATION_HE), which land on 3.9 GHz instead of
>> the best selectable frequency. Several frequencies can be affected.
>>
>> Exclude the boost frequencies from the ranking in em_compute_costs() and
>> keep them all efficient, using the highest non-boost frequency of the
>> CPUFreq table, returned by the new helper
>> cpufreq_table_max_nonboost_freq(), as the boundary.
>>
>> Fixes: c8ed99533dbc ("PM: EM: Mark inefficient states")
>> Signed-off-by: Guo Chen <guo.chen@oss.qualcomm.com>
> Please see Sashiko's feedback regarding this patch
>
> https://sashiko.dev/#/patchset/20261103-em-rank-boost-separately-v2-1-5a13d1b38d37%40oss.qualcomm.com
>
> and tell me what you think about it.
>
> Thanks!

Thanks for the review.

I did consider this case. If no policy is found, max_nonboost stays 0 and
the new logic is bypassed, i.e. the ranking falls back to the old
behaviour, which looked like the least intrusive option. I also thought
about warning and returning an error, but most of the in-tree callers of
em_recalc_and_update() ignore the return value (e.g.
em_adjust_new_capacity() and exynos_asv_update_opps()), so the update would
just be dropped without anyone noticing.

>> ---
>> Changes in v2:
>> - Drop prev_boost and keep all boost states efficient by resetting
>>    prev_cost for them, as suggested by Lukasz. Boost states are no
>>    longer ranked against each other.
>> - Retitle the patch to match the new behaviour (was "Rank boost and
>>    non-boost frequencies separately") and reword the commit message.
>>
>> Link to v1: https://lore.kernel.org/all/20261030-em-rank-boost-separately-v1-1-238a6b2bb888@oss.qualcomm.com/
>> ---
>>   include/linux/cpufreq.h     | 24 ++++++++++++++++++++++++
>>   kernel/power/energy_model.c | 16 ++++++++++++++++
>>   2 files changed, 40 insertions(+)
>>
>> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
>> index d3d0d9d02aa4..c93316141a11 100644
>> --- a/include/linux/cpufreq.h
>> +++ b/include/linux/cpufreq.h
>> @@ -1155,6 +1155,24 @@ cpufreq_table_set_inefficient(struct cpufreq_policy *policy,
>>          return -EINVAL;
>>   }
>>
>> +static inline unsigned int
>> +cpufreq_table_max_nonboost_freq(struct cpufreq_policy *policy)
>> +{
>> +       struct cpufreq_frequency_table *pos;
>> +       unsigned int max = 0;
>> +
>> +       if (!policy->freq_table)
>> +               return 0;
>> +
>> +       /* The table may be unsorted, scan all of it for the highest one. */
>> +       cpufreq_for_each_valid_entry(pos, policy->freq_table) {
>> +               if (!(pos->flags & CPUFREQ_BOOST_FREQ) && pos->frequency > max)
>> +                       max = pos->frequency;
>> +       }
>> +
>> +       return max;
>> +}
>> +
>>   static inline int parse_perf_domain(int cpu, const char *list_name,
>>                                      const char *cell_name,
>>                                      struct of_phandle_args *args)
>> @@ -1219,6 +1237,12 @@ cpufreq_table_set_inefficient(struct cpufreq_policy *policy,
>>          return -EINVAL;
>>   }
>>
>> +static inline unsigned int
>> +cpufreq_table_max_nonboost_freq(struct cpufreq_policy *policy)
>> +{
>> +       return 0;
>> +}
>> +
>>   static inline int of_perf_domain_get_sharing_cpumask(int pcpu, const char *list_name,
>>                                                       const char *cell_name, struct cpumask *cpumask,
>>                                                       struct of_phandle_args *pargs)
>> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
>> index d19a65461ae8..5b1d095fd376 100644
>> --- a/kernel/power/energy_model.c
>> +++ b/kernel/power/energy_model.c
>> @@ -255,15 +255,24 @@ static int em_compute_costs(struct device *dev, struct em_perf_state *table,
>>                              unsigned long flags)
>>   {
>>          unsigned long prev_cost = ULONG_MAX;
>> +       struct cpufreq_policy *policy;
>> +       unsigned int max_nonboost = 0;
>>          int i, ret;
>>
>>          /* This is needed only for CPUs and EAS skip other devices */
>>          if (!_is_cpu_device(dev))
>>                  return 0;
>>
>> +       policy = cpufreq_cpu_get(dev->id);
>> +       if (policy) {
>> +               max_nonboost = cpufreq_table_max_nonboost_freq(policy);
>> +               cpufreq_cpu_put(policy);
>> +       }
>> +

The lookup can be made more robust though: on the update paths dev->id may
be offline while other CPUs of the performance domain are online. In v3, if
dev->id has no policy and the performance domain already exists, I will use
an active CPU of the performance domain instead, as
em_cpufreq_update_efficiencies() does, and only warn and fall back to the
old behaviour if that fails as well.

I did not use the performance domain for every lookup because at
registration time dev->em_pd is not assigned yet (it is set after
em_create_perf_table() has returned):

policy = cpufreq_cpu_get(dev->id);
if (!policy && dev->em_pd) {
     cpu = cpumask_first_and(em_span_cpus(dev->em_pd), cpu_active_mask);
     if (cpu < nr_cpu_ids)
         policy = cpufreq_cpu_get(cpu);
}

if (policy) {
     max_nonboost = cpufreq_table_max_nonboost_freq(policy);
     cpufreq_cpu_put(policy);
  } else {
     dev_warn(dev, "EM: no CPUFreq policy, boost states are "
                         "not excluded from the ranking\n");
}

This makes it much less likely to miss the policy, but cannot rule it out:
the CPU can still go offline between picking it and calling
cpufreq_cpu_get(). em_cpufreq_update_efficiencies() has the same
limitation.

Would that work for you?

Thanks,
Guo

>>          /* Compute the cost of each performance state. */
>>          for (i = nr_states - 1; i >= 0; i--) {
>>                  unsigned long power_res, cost;
>> +               bool boost = max_nonboost && table[i].frequency > max_nonboost;
>>
>>                  if ((flags & EM_PERF_DOMAIN_ARTIFICIAL) && cb->get_cost) {
>>                          ret = cb->get_cost(dev, table[i].frequency, &cost);
>> @@ -284,6 +293,13 @@ static int em_compute_costs(struct device *dev, struct em_perf_state *table,
>>                          table[i].flags = EM_PERF_STATE_INEFFICIENT;
>>                          dev_dbg(dev, "EM: OPP:%lu is inefficient\n",
>>                                  table[i].frequency);
>> +               } else if (boost) {
>> +                       /*
>> +                        * Keep all boost states efficient. Resetting prev_cost
>> +                        * also keeps the highest non-boost state efficient,
>> +                        * then rank the rest on their own.
>> +                        */
>> +                       prev_cost = ULONG_MAX;
>>                  } else {
>>                          prev_cost = table[i].cost;
>>                  }
>>
>> ---
>> base-commit: f1ac44c32cba88ad65dcecd4a279661d14e93651
>> base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
>> change-id: 20261029-em-rank-boost-separately-a7e7c7d78058
>>
>> Best regards,
>> --
>> Guo Chen <guo.chen@oss.qualcomm.com>