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

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

On 6/16/21 10:31 AM, Viresh Kumar wrote:
> On 16-06-21, 10:03, Lukasz Luba wrote:
>> On 6/16/21 8:35 AM, Viresh Kumar wrote:
>>> On 15-06-21, 18:15, Vincent Donnefort wrote:
>>>> But if we sum-up:
>>>>
>>>> 1. em_dev_register_perf_domain() find inefficiencies
>>>>
>>>> 2. dev_pm_opp_of_register_em() apply EM inefficiencies into the OPP structures
>>>
>>> I was looking to add a new API to the OPP core
>>> (dev_pm_opp_mark_inefficient()) to mark an OPP inefficient. And then
>>> get it called from em_create_perf_table().
>>>
>>> But I now see that EM core rather has callbacks to call into and with
>>
>> Exactly, that's what I was trying to stress.
>>
>>> that I think you should rather add another callback
>>> (.mark_inefficient()) in struct em_data_callback, to set inefficient
>>> frequencies.
>>
>> I disagree. That's why I prefer Vincent's approach in this patch set.
>> This proposal would cause more mess.
>>
>> Vincent proposed a small and clean modification. This modification
>> can be done easily in this cpufreq place because we have EM in
>> device cpu struct.
> 
> This may look clean to you, but not to me, sorry about that.
> 
> 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 ;)

> 
> For example this patch:
> 
> https://lore.kernel.org/linux-pm/1622804761-126737-6-git-send-email-vincent.donnefort@arm.com/
> 
> tries to add EM stuff in cpufreq core. Cpufreq core doesn't care about
> EM and it shouldn't. And this piece of code doesn't belong here.
> 
> Would you guys like to add a cpufreq specific call into the EM core? I
> won't, that's not a place for cpufreq stuff. It is the EM core. I was
> fine with not including OPP core into this, and I gave up that
> argument earlier, but then we realized that the cpufreq core isn't
> ready at the time we register with EM core.
> 
> Honestly, OPP core looks to be a better place holder for such stuff.
> This is exactly the purpose of the OPP core. Moreover, we can apply
> the same logic to devfreq or other devices later, with or without EM
> core. Again, OPP core fits better.
> 
> The cpufreq core already has the relevant APIs in place to the OPP
> core and this won't require a new API there.

I don't see an API function in the OPP framework or a field in the
OPP struct which gives information that this freq is inefficient.
Thus, it will require new API changes: cpufreq --> OPP.

> 
>> Let's don't over-engineering. The inefficient information is only valid
>> for schedutil, thus IMHO it can live like this patch set made - in the
>> cpufreq table.
> 
> For now, yes. There is no guarantee though that we won't have more in
> future.

And there won't be in near future. We don't build massive interfaces
because there *might* be potential *oneday*.
Even for this idea, it was a massive work to do the research and prove
it that this is worth to put mainline so all vendors will get it.

The GPUs are slightly different beasts and they have different
workloads (not util + SchedUtil driven AFAIK).

> 
>> Compare the v1 (I still don't understand why it was blocked),
> 
> IIRC, it required more work to be done in the hotpath, i.e. traversing
> the freq list twice.

In v1 there was LUT. IMHO we have too easily gave and said:
'Remove the Look-up table as the numbers weren't strong enough to 
justify the implementation.'
But it had other benefits, which are now pointed.

There was different issue, which we could fix now.
With this patch set [1] EAS could have the freq_max limit, which
SchedUtil has in the hotpath.

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.

> 
>> 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.


[1] https://lore.kernel.org/lkml/20210614185815.15136-1-lukasz.luba@arm.com/
[2] 
https://lore.kernel.org/lkml/1617901829-381963-2-git-send-email-vincent.donnefort@arm.com/