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

[Lukasz Luba <lukasz.luba@arm.com>]

On 11/10/26 09:49, Guo Chen wrote:
> 
> On 11/9/2026 5:40 PM, Lukasz Luba wrote:
>>
>>
>> On 11/9/26 06:47, Guo Chen wrote:
>>>
>>> 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?
>> I also thought about that possible racy corner case, where
>> CPU can go offline (or already be offline). Although, in current
>> usage this should be OK, since we do this in boot phase and
>> platforms are sane.
>>
>> We can can add that step with 'cpu_active_mask' to play safe,
>> but no hard promises it would be atomic.
>>
>> The warning also would help, for bring-up phase, so engineers would
>> notice it and address the racy hot-plug booting system when it's there.
> 
> Thanks Lukasz,
> 
> I’ll add the 'cpu_active_mask' check and the warning in v3. As the
> overall approach is unchanged, I’ll keep your reviewed-by.
> Please let me know if you’d rather have it removed.

That's fine, please continue.