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:54 PM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> 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 ...

Scratch that, sorry.  We'll get the "limits" notification and the
need_freq_update thing will cause next_freq to become zero then.