Hi Viresh,
On Tue, Jul 11, 2017 at 3:14 AM, Viresh Kumar <viresh.ku...@linaro.org> wrote:
> On 09-07-17, 10:08, Joel Fernandes wrote:
>> diff --git a/kernel/sched/cpufreq_schedutil.c
>> b/kernel/sched/cpufreq_schedutil.c
>> index 622eed1b7658..4d9e8b96bed1 100644
>> --- a/kernel/sched/cpufreq_schedutil.c
>> +++ b/kernel/sched/cpufreq_schedutil.c
>> @@ -53,7 +53,9 @@ struct sugov_cpu {
>> struct update_util_data update_util;
>> struct sugov_policy *sg_policy;
>>
>> + bool prev_iowait_boost;
>> unsigned long iowait_boost;
>> + unsigned long iowait_boost_min;
>> unsigned long iowait_boost_max;
>> u64 last_update;
>>
>> @@ -168,22 +170,47 @@ static void sugov_get_util(unsigned long *util,
>> unsigned long *max)
>> *max = cfs_max;
>> }
>>
>> +static void sugov_decay_iowait_boost(struct sugov_cpu *sg_cpu)
>> +{
>> + sg_cpu->iowait_boost >>= 1;
>> +
>> + if (sg_cpu->iowait_boost < sg_cpu->iowait_boost_min)
>> + sg_cpu->iowait_boost = 0;
>> +}
>> +
>> static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
>> unsigned int flags)
>> {
>> if (flags & SCHED_CPUFREQ_IOWAIT) {
>> - sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
>> + /* Remember for next time that we did an iowait boost */
>> + sg_cpu->prev_iowait_boost = true;
>> + if (sg_cpu->iowait_boost) {
>> + sg_cpu->iowait_boost <<= 1;
>> + sg_cpu->iowait_boost = min(sg_cpu->iowait_boost,
>> + sg_cpu->iowait_boost_max);
>> + } else {
>> + sg_cpu->iowait_boost = sg_cpu->iowait_boost_min;
>
> I am not sure if boost should start from the min frequency, as the current
> frequency will at least be equal to that. Which means that with no boost
> initially, we will never increase the frequency for the first IOWAIT flag
> event.
I think the whole point of IOWAIT boost was to solve the issue with a
long sequence of repeated I/O requests as described in the commit
message. So IIUC there isn't a usecase for that (increase freq. on
first request). Also its just for the first couple of requests in my
testing and doesn't hurt the performance at all for the intended
usecase while still not causing transient spikes.
Another approach than setting min in sugov_set_iowait_boost, is, since
we have already retrieved the current util, we can check if flags ==
SCHED_CPUFREQ_IOWAIT, then set initial the iowait_boost such that
(iowait_boost / iowait_boost_max) is aleast equal to (util / max) or
iowait_boost_min, which ever is lower. This still will not increase
frequency on the first request, but will ensure the next one will
benefit. Then again I fear running into slightly longer-term
transients where 2 iowait requests are enough to boost the frequency
to high values. I can try to measure how often this can hurt power for
common usecases if you agree its worth exploring.
>
>> + }
>> } else if (sg_cpu->iowait_boost) {
>> s64 delta_ns = time - sg_cpu->last_update;
>>
>> /* Clear iowait_boost if the CPU apprears to have been idle. */
>> if (delta_ns > TICK_NSEC)
>> sg_cpu->iowait_boost = 0;
>> +
>> + /*
>> + * Since we don't decay iowait_boost when its consumed during
>> + * the previous SCHED_CPUFREQ_IOWAIT update, decay it now.
>> + */
>> + if (sg_cpu->prev_iowait_boost) {
>
> SCHED_CPUFREQ_IOWAIT flag is set only by CFS from the enqueue_task() and in
> many
> cases we call the util hook twice from the same enqueue_task() instance before
> returning (2nd one after updating util). And in such cases we will set
> iowait_boost as 0 on the second call.
>
> Have you ever seen two consecutive calls to sugov_set_iowait_boost() with
> IOWAIT
> flag set ? Can we get the ratio of that against the other case where we have
> IOWAIT flag set in first call, followed by one or more non-IOWAIT calls and
> then
> IOWAIT again ?
>
> I am asking because if the calls with IOWAIT flag aren't consecutive then we
> will make iowait_boost as 0 in the next non-IOWAIT call.
Yes, I've seen that happen in my testing (consecutive iowait). I
haven't seen the other case where you have IOWAIT followed by
non-IOWAIT for a repeated set of IOWAIT requests. Would you more
comfortable if we moved sugov_set_iowait_boost() after the
sugov_should_update_freq() ? That way if there are consecutive
requests in the same path, then it most likely rate-limiting will
prevent such updates. I will also try to collect some stats as you
suggested to see if how often if at all this can happen.
thanks,
-Joel
