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

["Rafael J. Wysocki" <rafael@kernel.org>]

On Fri, Jul 2, 2021 at 6:14 PM Vincent Donnefort
<vincent.donnefort@arm.com> wrote:
>
> [...]
>
> Hi Rafael,
>
> > >
> > > It would be great if you could have a look.
> > > I will be grateful for your time spend on it and opinion.
> >
> > First of all, IMO checking whether or not a given frequency is
> > "efficient" doesn't belong to cpufreq governors.  The governor knows
> > what the min and max supported frequencies are and selects one from
> > that range and note that it doesn't even check whether or not the
> > selected frequency is present in the frequency table.  That part
> > belongs to the driver or the general frequency table handling in the
> > cpufreq core.
> >
> > So the governor doesn't care and it shouldn't care IMO.
>
> A governor such as userspace should probably be able select any frequency
> with no regard to efficiency.

You seem to be arguing that the decision whether or not to use
inefficient frequencies needs to be made at the governor level,
because the driver must use whatever frequency is selected by the
userspace governor, since the user process driving it may be assuming
that to be the case.

That's why you want to hack schedutil to filter out the inefficient
frequencies, so you don't need to worry about them below that level.

That would be putting driver knowledge into the governor which cpufreq
was specifically designed to avoid.

There is a way to do that, though, which is to put a governor into the
driver and use ->setoplicy(), but the good news is that you don't even
need to do that.

So there are those CPUFREQ_RELATION_* symbols used by governors to
tell drivers what to do next (I wonder what they would be good for
after your changes for that matter) and you can add more of them.  For
example, you can define a CPUFREQ_RELATION_EFFICIENT bit to tell the
driver to avoid inefficient frequencies and pass that from schedutil.

Besides, I'm not sure why you still want to use inefficient
frequencies if they are selected by the ondemand or conservative
governors.

> >
> > Besides, in the cpufreq_driver_fast_switch() case the driver's
> > ->fast_switch() callback is entirely responsible for applying the
> > selected frequency, so I'm not sure how this "efficient" thing is
> > going to work then?
>
> This shouldn't be a problem if a governor that leverages inefficient OPPs
> resolves an inefficient one to an efficient one. The target_freq would simply
> point to a frequency known as efficient.

No, that doesn't help.  Moreover, it is harmful, because it changes
the interface between this particular governor and drivers in a subtle
and potentially confusing way.

Namely, drivers now can expect that the frequency passed to
->fast_switch() is the one corresponding to the current utilization
demand as seen by the governor, subject to the policy limits, not
the-closest-efficient-one-present-in-the-frequency-table.

> > Anyway, since we are talking about frequency tables, that would be the
> > __cpufreq_driver_target() case only and specifically in the
> > __target_index() case only (note how far away this is from the
> > governor).
> >
> > Now, you may think about modifying cpufreq_frequency_table_target() to
> > skip "inefficient" frequencies, but then the question is why those
> > frequencies need to be there in the frequency table in the first
> > place?
>
> Cpufreq_cooling needs those frequencies, even inefficients,

I've already figured that out.

> also there's probably no reason to hide them from the userspace.