Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data

[Rafael J. Wysocki]

On Wed, Mar 30, 2016 at 7:05 PM, Steve Muckle <steve.muckle@xxxxxxxxxx> wrote:
> On 03/30/2016 04:31 AM, Rafael J. Wysocki wrote:
>>>> >> +static int sugov_limits(struct cpufreq_policy *policy)
>>>> >> +{
>>>> >> +     struct sugov_policy *sg_policy = policy->governor_data;
>>>> >> +
>>>> >> +     if (!policy->fast_switch_enabled) {
>>>> >> +             mutex_lock(&sg_policy->work_lock);
>>>> >> +
>>>> >> +             if (policy->max < policy->cur)
>>>> >> +                     __cpufreq_driver_target(policy, policy->max,
>>>> >> +                                             CPUFREQ_RELATION_H);
>>>> >> +             else if (policy->min > policy->cur)
>>>> >> +                     __cpufreq_driver_target(policy, policy->min,
>>>> >> +                                             CPUFREQ_RELATION_L);
>>>> >> +
>>>> >> +             mutex_unlock(&sg_policy->work_lock);
>>>> >> +     }
>>>> >> +
>>>> >> +     sg_policy->need_freq_update = true;
>>> >
>>> > I am wondering why we need to do this for !fast_switch_enabled case?
>>
>> That will cause the rate limit to be ignored in the utilization update
>> handler which may be necessary if it is set to a relatively large
>> value (like 1 s).
>
> But why is that necessary for !fast_switch_enabled? In that case the
> frequency has been adjusted to satisfy the new limits here, so ignoring
> the rate limit shouldn't be necessary. In other words why not
>
> } else {
>         sg_policy->need_freq_update = true;
> }

My thinking here was that the governor might decide to use something
different from the limit enforced here, so it would be good to make it
do so as soon as possible.  In particular in the
non-frequency-invariant utilization case in which new frequency
depends on the current one.

That said i'm not particularly opposed to making that change if that's
preferred.

Thanks,
Rafael