Message-ID: <20200122101944.GA12533@darkstar>
Date:   Wed, 22 Jan 2020 11:19:44 +0100
From:   Patrick Bellasi <patrick.bellasi@...bug.net>
To:     Qais Yousef <qais.yousef@....com>
Cc:     Valentin Schneider <valentin.schneider@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Iurii Zaikin <yzaikin@...gle.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        qperret@...gle.com, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] sched/rt: Add a new sysctl to control uclamp_util_min

On 14-Jan 21:34, Qais Yousef wrote:
> On 01/09/20 10:21, Patrick Bellasi wrote:
> > That's not entirely true. In that patch we introduce cgroup support
> > but, if you look at the code before that patch, for CFS tasks there is
> > only:
> >  - CFS task-specific values (min,max)=(0,1024) by default
> >  - CFS system-wide tunables (min,max)=(1024,1024) by default
> > and a change on the system-wide tunable allows for example to enforce
> > a uclamp_max=200 on all tasks.
> > 
> > A similar solution can be implemented for RT tasks, where we have:
> >  - RT task-specific values (min,max)=(1024,1024) by default
> >  - RT system-wide tunables (min,max)=(1024,1024) by default
> >  and a change on the system-wide tunable allows for example to enforce
> >  a uclamp_min=200 on all tasks.
> 
> I feel I'm already getting lost in the complexity of the interaction here. Do
> we really need to go that path?
> 
> So we will end up with a default system wide for all tasks + a CFS system wide
> default + an RT system wide default?
> 
> As I understand it, we have a single system wide default now.

Right now we have one system wide default and that's both for all
CFS/RT tasks, when cgroups are not in use, or for root group and
autogroup CFS/RT tasks, when cgroups are in use.

What I'm proposing is to limit the usage of the current system wide
default to CFS tasks only, while we add a similar new one specifically
for RT tasks.

At the end we will have two system wide defaults, not three.

> > > (Would we need CONFIG_RT_GROUP_SCHED for this? IIRC there's a few pain points
> > > when turning it on, but I think we don't have to if we just want things like
> > > uclamp value propagation?)
> > 
> > No, the current design for CFS tasks works also on !CONFIG_CFS_GROUP_SCHED.
> > That's because in this case:
> >   - uclamp_tg_restrict() returns just the task requested value
> >   - uclamp_eff_get() _always_ restricts the requested value considering
> >     the system defaults
> >  
> > > It's quite more work than the simple thing Qais is introducing (and on both
> > > user and kernel side).
> > 
> > But if in the future we will want to extend CGroups support to RT then
> > we will feel the pains because we do the effective computation in two
> > different places.
> 
> Hmm what you're suggesting here is that we want to have
> cpu.rt.uclamp.{min,max}? I'm not sure I can agree this is a good idea.

That's exactly what we already do for other controllers. For example,
if you look at the bandwidth controller, we have separate knobs for
CFS and RT tasks.

> It makes more sense to create a special group for all rt tasks rather than
> treat rt tasks in a cgroup differently.

Don't see why that should make more sense. This can work of course but
it would enforce a more strict configuration and usage of cgroups to
userspace.

I also have some doubths about this approach matching the delegation
model principles.

> > Do note that a proper CGroup support requires that the system default
> > values defines the values for the root group and are consistently
> > propagated down the hierarchy. Thus we need to add a dedicated pair of
> > cgroup attributes, e.g. cpu.util.rt.{min.max}.
> > 
> > To recap, we don't need CGROUP support right now but just to add a new
> > default tracking similar to what we do for CFS.
> >
> > We already proposed such a support in one of the initial versions of
> > the uclamp series:
> >    Message-ID: <20190115101513.2822-10-patrick.bellasi@....com>
> >    https://lore.kernel.org/lkml/20190115101513.2822-10-patrick.bellasi@arm.com/
> 
> IIUC what you're suggesting is:
> 
> 	1. Use the sysctl to specify the default_rt_uclamp_min
> 	2. Enforce this value in uclamp_eff_get() rather than my sync logic
> 	3. Remove the current hack to always set
> 	   rt_task->uclamp_min = uclamp_none(UCLAMP_MAX)

Right, that's the essence...

> If I got it correctly I'd be happy to experiment with it if this is what
> you're suggesting. Otherwise I'm afraid I'm failing to see the crust of the
> problem you're trying to highlight.

... from what your write above I think you got it right.

In my previous posting:

   Message-ID: <20200109092137.GA2811@...kstar>
   https://lore.kernel.org/lkml/20200109092137.GA2811@darkstar/

there is also the code snippet which should be good enough to extend
uclamp_eff_get(). Apart from that, what remains is:
- to add the two new sysfs knobs for sysctl_sched_uclamp_util_{min,max}_rt
- make a call about how rt tasks in cgroups are clamped, a simple
  proposal is in the second snippet of my message above.

Best,
Patrick

-- 
#include <best/regards.h>

Patrick Bellasi

Powered by blists - more mailing lists