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

[Viresh Kumar <viresh.kumar@linaro.org>]

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 ;)

:)

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

dev_pm_opp_init_cpufreq_table() is all we need here, we just need to
change it to update one more field in the cpufreq table's entries.

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

Yes, true.

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

Right, but even if there is a single user for this, I think getting
this through the right layers is a more cleaner solution.

> In v1 there was LUT.

Oops, yes, I started looking from V2 and not V1.

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

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

Anyway, that's my view, which can be wrong as well.

Rafael: You have any suggestions here ?

-- 
viresh