Message-ID: <20190122150137.fp4g4kdng2qpy6qx@e110439-lin>
Date:   Tue, 22 Jan 2019 15:01:37 +0000
From:   Patrick Bellasi <patrick.bellasi@....com>
To:     Quentin Perret <quentin.perret@....com>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-api@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Tejun Heo <tj@...nel.org>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Paul Turner <pjt@...gle.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Morten Rasmussen <morten.rasmussen@....com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Todd Kjos <tkjos@...gle.com>,
        Joel Fernandes <joelaf@...gle.com>,
        Steve Muckle <smuckle@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>
Subject: Re: [PATCH v6 11/16] sched/fair: Add uclamp support to
 energy_compute()

On 22-Jan 14:39, Quentin Perret wrote:
> On Tuesday 22 Jan 2019 at 14:26:06 (+0000), Patrick Bellasi wrote:
> > On 22-Jan 13:29, Quentin Perret wrote:
> > > On Tuesday 22 Jan 2019 at 12:45:46 (+0000), Patrick Bellasi wrote:
> > > > On 22-Jan 12:13, Quentin Perret wrote:
> > > > > On Tuesday 15 Jan 2019 at 10:15:08 (+0000), Patrick Bellasi wrote:
> > > > > > The Energy Aware Scheduler (AES) estimates the energy impact of waking

[...]

> > > Ah, sorry, I guess my message was misleading. I'm saying this is
> > > changing the way _EAS_ deals with RT tasks. Right now we don't actually
> > > consider the RT-go-to-max thing at all in the EAS prediction. Your
> > > patch is changing that AFAICT. It actually changes the way EAS sees RT
> > > tasks even without uclamp ...
> > 
> > Lemme see if I get it right.
> > 
> > Currently, whenever we look at CPU utilization for ENERGY_UTIL, we
> > always use cpu_util_rt() for RT tasks:
> > 
> > ---8<---
> >         util = util_cfs;
> >         util += cpu_util_rt(rq);
> >         util += dl_util;
> > ---8<---
> > 
> > Thus, even when RT tasks are RUNNABLE, we don't always assume the CPU
> > running at the max capacity but just whatever is the aggregated
> > utilization across all the classes.
> > 
> > With uclamp, we have:
> > 
> > ---8<---
> >         util = cpu_util_rt(rq) + util_cfs;
> >         if (type == FREQUENCY_UTIL)
> >             util = uclamp_util_with(rq, util, p);
> >         dl_util = cpu_util_dl(rq);
> >         if (type == ENERGY_UTIL)
> >             util += dl_util;
> > ---8<---
> > 
> > So, I would say that, in terms of ENERGY_UTIL we do the same both
> > w/ and w/o uclamp. Isn't it?
> 
> Yes but now you use FREQUENCY_UTIL for computing 'max_util' in the EAS
> prediction.

Right, I overlook that "little" detail... :/

> Let's take an example. You have a perf domain with two CPUs. One CPU is
> busy running a RT task, the other CPU runs a CFS task. Right now in
> compute_energy() we only use ENERGY_UTIL, so 'max_util' ends up being
> the max between the utilization of the two tasks. So we don't predict
> we're going to max freq.

+1

> With your patch, we use FREQUENCY_UTIL to compute 'max_util', so we
> _will_ predict that we're going to max freq.

Right, with the default conf yes.

> And we will do that even if CONFIG_UCLAMP_TASK=n.

While this should not happen, as I wrote in the RT integration patch,
that's happening because I'm missing some compilation guard or
similar. In this configurations we should always go to max... will
look into that.

> The default EAS calculation will be different with your patch when there
> are runnable RT tasks in the system. This needs to be documented, I
> think.

Sure...

> > > But I'm not hostile to the idea since it's possible to enable uclamp and
> > > set the min cap to 0 for RT if you want. So there is a story there.
> > > However, I think this needs be documented somewhere, at the very least.
> > 
> > The only difference I see is that the actual frequency could be
> > different (lower then max) when a clamped RT task is RUNNABLE.
> > 
> > Are you worried that running RT on a lower freq could have side
> > effects on the estimated busy time the CPU ?
> > 
> > I also still don't completely get why you say it could be useful to
> >    "set the min cap to 0 for RT if you want"
> 
> I'm not saying it's useful, I'm saying userspace can decide to do that
> if it thinks it is a good idea. The default should be min_cap = 1024 for
> RT, no questions. But you _can_ change it at runtime if you want to.
> That's my point. And doing that basically provides the same behaviour as
> what we have right now in terms of EAS calculation (but it changes the
> freq selection obviously) which is why I'm not fundamentally opposed to
> your patch.

Well, I think it's tricky to say whether the current or new approach
is better... it probably depends on the use-case.

> So in short, I'm fine with the behavioural change, but please at least
> mention it somewhere :-)

Anyway... agree, it's just that to add some documentation I need to
get what you are pointing out ;)

Will come up with some additional text to be added to the changelog

Maybe we can add a more detailed explanation of the different
behaviors you can get in the EAS documentation which is coming to
mainline ?

> Thanks,
> Quentin

-- 
#include <best/regards.h>

Patrick Bellasi

Powered by blists - more mailing lists