Hi Juri,
On Mon, Jul 10, 2017 at 3:55 AM, Juri Lelli <juri.le...@arm.com> wrote:
> Hi Joel,
>
> On 09/07/17 10:08, Joel Fernandes wrote:
>> Currently the iowait_boost feature in schedutil makes the frequency go to
>> max.
>> This feature was added to handle a case that Peter described where the
>> throughput of operations involving continuous I/O requests [1] is reduced due
>> to running at a lower frequency, however the lower throughput itself causes
>> utilization to be low and hence causing frequency to be low hence its
>> "stuck".
>>
>> Instead of going to max, its also possible to achieve the same effect by
>> ramping up to max if there are repeated in_iowait wakeups happening. This
>> patch
>> is an attempt to do that. We start from a lower frequency (iowait_boost_min)
>> and double the boost for every consecutive iowait update until we reach the
>> maximum iowait boost frequency (iowait_boost_max).
>>
>> I ran a synthetic test on an x86 machine with intel_pstate in passive mode
>> using schedutil. The patch achieves the desired effect as the existing
>> behavior. Also tested on ARM64 platform and see that there the transient
>> iowait
>> requests aren't causing frequency spikes.
>>
>> [1] https://patchwork.kernel.org/patch/9735885/
>>
>> Cc: Srinivas Pandruvada <srinivas.pandruv...@linux.intel.com>
>> Cc: Len Brown <l...@kernel.org>
>> Cc: Rafael J. Wysocki <r...@rjwysocki.net>
>> Cc: Viresh Kumar <viresh.ku...@linaro.org>
>> Cc: Ingo Molnar <mi...@redhat.com>
>> Cc: Peter Zijlstra <pet...@infradead.org>
>> Suggested-by: Peter Zijlstra <pet...@infradead.org>
>> Signed-off-by: Joel Fernandes <joe...@google.com>
[..]
>>
>> 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;
>> + }
>> } 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;
>
> In this case we might just also want to reset prev_iowait_boost
> unconditionally and return.
Ok, will do.
>> +
>> + /*
>> + * Since we don't decay iowait_boost when its consumed during
>> + * the previous SCHED_CPUFREQ_IOWAIT update, decay it now.
>> + */
>
> Code below seems pretty self-explaning to me. :)
>
Ok :)
>> + if (sg_cpu->prev_iowait_boost) {
>> + sugov_decay_iowait_boost(sg_cpu);
>> + sg_cpu->prev_iowait_boost = false;
>> + }
>> }
>> }
>>
>> static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, unsigned long
>> *util,
>> - unsigned long *max)
>> + unsigned long *max, unsigned int flags)
>> {
>> unsigned long boost_util = sg_cpu->iowait_boost;
>> unsigned long boost_max = sg_cpu->iowait_boost_max;
>> @@ -195,7 +222,16 @@ static void sugov_iowait_boost(struct sugov_cpu
>> *sg_cpu, unsigned long *util,
>> *util = boost_util;
>> *max = boost_max;
>> }
>> - sg_cpu->iowait_boost >>= 1;
>> +
>> + /*
>> + * Incase iowait boost just happened on this CPU, don't reduce it right
>> + * away since then the iowait boost will never increase on subsequent
>> + * in_iowait wakeups.
>> + */
>> + if (flags & SCHED_CPUFREQ_IOWAIT && this_cpu_ptr(&sugov_cpu) == sg_cpu)
>> + return;
>> +
>> + sugov_decay_iowait_boost(sg_cpu);
>
> Mmm, do we need to decay even when we are computing frequency requests
> for a domain?
>
> Considering it a per-cpu thing, isn't enough that it gets bumped up or
> decayed only when a CPU does an update (by using the above from
> sugov_update_shared)?
>
> If we go this way I think we will only need to reset prev_iowait_boost
> if delta_ns > TICK_NSEC during the for_each_cpu() loop of sugov_next_
> freq_shared().
>
Actually the "decay" was already being done before (without this
patch), I am just preserving the same old behavior where we do decay.
Perhaps your proposal can be a separate match? Or did I miss something
else subtle here?
Thanks,
-Joel
