Re: [PATCH 5/5] cpufreq: schedutil: do not update rate limit ts when freq is unchanged

[Rafael J. Wysocki]

On Fri, May 20, 2016 at 1:39 PM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> On Fri, May 20, 2016 at 2:46 AM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>> On Fri, May 20, 2016 at 2:40 AM, Steve Muckle <steve.muckle@xxxxxxxxxx> wrote:
>>> On Fri, May 20, 2016 at 02:37:17AM +0200, Rafael J. Wysocki wrote:
>>>> Also I think that it would be good to avoid walking the frequency
>>>> table twice in case we end up wanting to update the frequency after
>>>> all.  With the [4/5] we'd do it once in get_next_freq() and then once
>>>> more in cpufreq_driver_fast_switch(), for example, and walking the
>>>> frequency table may be more expensive that doing the switch in the
>>>> first place.
>>>
>>> If a driver API is added to return the platform frequency associated
>>> with a target frequency, what do you think about requiring the
>>> fast_switch API to take a target-supported frequency?
>>
>> That doesn't help much, because it generally would need to find a
>> table entry corresponding to it anyway, to find the actual command
>> value to write to a register, for example.
>>
>> But the driver could be smart and cache the value returned from the
>> new callback along with the command value associated with it.  If
>> invoked with that particular frequency, it would use the cached
>> command.  Otherwise, it would walk the table.
>
> It also makes sense to save both the "raw" value computed by
> get_next_freq() and the corresponding "driver" value, because if the
> current "raw" value is equal to the previous "raw" value, it shouldn't
> be necessary to walk the frequency table at all (as the previous
> "driver" value would then be equal to the current "driver" value too).
>
> So maybe the "driver" value should only be checked after the "raw"
> value check in sugov_update_commit() or equivalent?

Moreover, you need to be careful about policy->min/max changes,
because both cpufreq_driver_fast_switch() and
__cpufreq_driver_target() clamp the target frequency between those and
if they change in the meantime, you may end up having to use a
different frequency at the driver level even if you get the same "raw"
value as last time.

It looks like we don't do the right thing here in the current code even ...