Re: [PATCH v3 3/6] cpufreq: Add an interface to mark inefficient frequencies

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

Hi Rafael,

This is a gentle ping.
You have probably not seen this discussion thread.

On 6/16/21 1:45 PM, Lukasz Luba wrote:
> 
> 
> On 6/16/21 11:53 AM, Viresh Kumar wrote:
>> On 16-06-21, 11:33, Lukasz Luba wrote:
>>> On 6/16/21 10:31 AM, Viresh Kumar wrote:
>>>> On 16-06-21, 10:03, Lukasz Luba wrote:
>>>> Clean is not lesser number of lines for me, but rather having the
>>>> right ownership of such things.
>>>
>>> Some developers do like patches which removes more lines then adds ;)
>>
>> :)
>>
> 
> [snip]
> 
>>>
>>> What could be the modified v1 [2]:
>>> - LUT which holds two IDs: efficient, inefficient, take one
>>>    according to the clamp f_max
>>> - add new argument 'policy->max' to em_pd_get_efficient_freq()
>>>
>>> freq = em_pd_get_efficient_freq(em_cpu_get(policy->cpu), freq, 
>>> policy->max);
>>>
>>> The problem was that EAS couldn't know the clamp freq_max,
>>> which shouldn't be the blocker now.
>>
>> If you can do that without adding any EM specific stuff in the cpufreq
>> core, I will mostly be fine.
>>
>> But honestly speaking, creating more data structures to keep related
>> information doesn't scale well.
>>
>> We already have so many tables for keeping freq/voltage pairs, OPP,
>> cpufreq, EM. You tried to add one more in EM I think V1, not sure.
>>
>> It is always better to consolidate and we almost reached to a point
>> where that could have been done very easily. I understand that you
>> didn't want to touch so many different parts, but anyway..
> 
> Yes, I don't want to touch some many generic parts because they
> are intended to be generic. This heuristic is only for EM platforms,
> which are arm, arm64 battery powered (not servers or other).
> Thus, I wanted to keep it locally. The cost of EM extra structures
> (the LUT) will only be for platforms for which EM discovers that
> they have inefficient performance levels.
> The code would even not be compiled in for x86, ppc, etc, in hot path.
> 
>>>>> this v3 and your proposal.
>>>>
>>>> IMHO, adding such callbacks to the EM core, like .mark_efficient(),
>>>> will only make this easier to handle for all different frameworks, and
>>>> not otherwise. The code will look much cleaner everywhere..
>>>>
>>>
>>> What about coming back to the slightly modified v1 idea?
>>> That was really self-contained modification for this
>>> inefficient opps heuristic.
>>
>> I am not sure if I really understand what that would be, but again
>> adding another table is going to create more problems then it should.
> 
> IMHO your proposals are more invasive for the generic code, while
> not always being even used. Plenty of architectures don't even set EM,
> even arm64 for servers doesn't use it. You and Rafael would have to
> maintain these modifications in generic code. It might be hard to remove
> it. While I recommend to keep this heuristic feature inside the EM and
> we will maintain it. If we decide after a few years that new arm64
> platforms use some smarter FW for performance level, we might just
> disable this heuristic.
> 
>>
>> Anyway, that's my view, which can be wrong as well.
>>
>> Rafael: You have any suggestions here ?
>>
> 
> I would also like to hear Rafael's opinion :)

It would be great if you could have a look.
I will be grateful for your time spend on it and opinion.

Regards,
Lukasz