Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200124002811.228334-1-wvw@google.com> <20200124025238.jsf36n6w4rrn2ehc@e107158-lin>
 <20200124095125.GA121494@google.com> <849cc9f0-f4ae-f2b6-8449-f55697928cf5@arm.com>
 <20200124113050.i6ovkibcmutypm3q@e107158-lin> <CAGXk5yrTc2-k5oDjGyAwYn2KTroQy0JtEYQzSeOizjg_hyMGkg@mail.gmail.com>
 <20200125235934.wrs2nryuk3wmtkxr@e107158-lin>
In-Reply-To: <20200125235934.wrs2nryuk3wmtkxr@e107158-lin>
From:   Wei Wang <wei.vince.wang@gmail.com>
Date:   Sat, 25 Jan 2020 22:41:14 -0800
Message-ID: <CAMFybE7eQz60yXJKJfGCuzJXTAS-np0qmz3WWeOAEX6CuXhwDA@mail.gmail.com>
Subject: Re: [PATCH] [RFC] sched: restrict iowait boost for boosted task only
To:     Qais Yousef <qais.yousef@arm.com>
Cc:     Wei Wang <wvw@google.com>,
        Valentin Schneider <valentin.schneider@arm.com>,
        Quentin Perret <qperret@google.com>, dietmar.eggemann@arm.com,
        chris.redpath@arm.com, Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Jan 25, 2020 at 3:59 PM Qais Yousef <qais.yousef@arm.com> wrote:
>
> On 01/24/20 12:55, Wei Wang wrote:
> > > > So I'm pretty sure we *do* want tasks with the default clamps to get iowait
> > > > boost'd. What we don't want are background tasks driving up the frequency,
> > > > and that should be via uclamp.max (as Quentin is suggesting) rather than
> > > > uclamp.min (as is suggested in the patch).
> > > >
> > > > Now, whether that is overloading the usage of uclamp... I'm not sure.
> > > > One of the argument for uclamp was actually frequency selection, so if
> > > > we just make iowait boost respect that, IOW not boost further than
> > > > uclamp.max (which is a bit better than a simple on/off switch), that
> > > > wouldn't be too crazy I think.
> > >
> > > Capping iowait boost value in schedutil based on uclamp makes sense indeed.
> > >
> > > What didn't make sense to me is the use of uclamp as a switch to toggle iowait
> > > boost on/off.
> >
> > Sounds like we all agree on adding a new toggle, so will move forward
> > with that then.
>
> Looking more closely at iowait boost, it's not actually a generic cpufreq
> attribute. Only schedutil and intel_pstate have it. Other governors might
> implement something similar but under a different name.
>
> So I'm not sure how easy it'd be to implement a generic toggle for something
> that probably should be considered an implementation detail of a governor and
> userspace shouldn't care much about.
>
> Of course, the maintainers might have a different opinion. So don't let mine
> discourage you from pursuing this further! :-)
>
Indeed, that's why I was hesitate to add the toggle and wanted to
bring this up for Android common kernel first. :-)

> > For capping iowait boost, it should be a separate patch. I am not sure
> > if we want to apply what's the current max clamp on the rq but I do
> > see the per-task iowait boost makes sense.
>
> It is true the 2 patches are orthogonal, but if you already cap the max
> frequencies the background task can use, by ensuring the iowait_boost in
> schedutil respects the uclamp restrictions then this should solve your problem
> too, no?
>

We haven't tried on 5.4, and there is no uclamp policy placed for
background yet. Also I am not sure it is beneficial to do uclamp
restriction on those kernel threads (e.g. is f2fs's gc), but that is
an interesting experiment on power balance. Also for applying at rq
lever, what if there are foreground tasks (and it could be the case
sometimes).


> The patch below only compile tested.
>
>
>         background/cpu.uclamp.max = 200 # Cap background tasks max frequencies
>
> ---
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 9b8916fd00a2..a76c02eecdaf 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -421,7 +421,8 @@ static unsigned long sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time,
>          * into the same scale so we can compare.
>          */
>         boost = (sg_cpu->iowait_boost * max) >> SCHED_CAPACITY_SHIFT;
> -       return max(boost, util);
> +       boost = max(boost, util);
> +       return uclamp_util_with(cpu_rq(sg_cpu->cpu), boost, NULL);
>  }
>
>  #ifdef CONFIG_NO_HZ_COMMON
>
> --
> Qais Yousef