Re: [PATCH] sched: cgroup SCHED_IDLE support

From: Josh Don <joshdon@google.com>
To: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: 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>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Paul Turner <pjt@google.com>,
	David Rientjes <rientjes@google.com>,
	Oleg Rombakh <olegrom@google.com>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Steve Sistare <steven.sistare@oracle.com>,
	Tejun Heo <tj@kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] sched: cgroup SCHED_IDLE support
Date: Fri, 11 Jun 2021 16:34:27 -0700	[thread overview]
Message-ID: <CABk29NtVRG8cotfbK=R0kKXuKCnkEG514H=6ncri=CM8Qr9uiQ@mail.gmail.com> (raw)
In-Reply-To: <7222c20a-5cbb-d443-a2fd-19067652a38e@arm.com>

On Fri, Jun 11, 2021 at 9:43 AM Dietmar Eggemann
<dietmar.eggemann@arm.com> wrote:
>
> On 10/06/2021 21:14, Josh Don wrote:
> > Hey Dietmar,
> >
> > On Thu, Jun 10, 2021 at 5:53 AM Dietmar Eggemann
> > <dietmar.eggemann@arm.com> wrote:
> >>
> >> Any reason why this should only work on cgroup-v2?
> >
> > My (perhaps incorrect) assumption that new development should not
> > extend v1. I'd actually prefer making this work on v1 as well; I'll
> > add that support.
> >
> >> struct cftype cpu_legacy_files[] vs. cpu_files[]
> >>
> >> [...]
> >>
> >>> @@ -11340,10 +11408,14 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
> >>>
> >>>  static DEFINE_MUTEX(shares_mutex);
> >>>
> >>> -int sched_group_set_shares(struct task_group *tg, unsigned long shares)
> >>> +#define IDLE_WEIGHT sched_prio_to_weight[ARRAY_SIZE(sched_prio_to_weight) - 1]
> >>
> >> Why not 3 ? Like for tasks (WEIGHT_IDLEPRIO)?
> >>
> >> [...]
> >
> > Went back and forth on this; on second look, I do think it makes sense
> > to use the IDLEPRIO weight of 3 here. This gets converted to a 0,
> > rather than a 1 for display of cpu.weight, which is also actually a
> > nice property.
>
> I'm struggling to see the benefit here.
>
> For a taskgroup A: Why setting A/cpu.idle=1 to force a minimum A->shares
> when you can set it directly via A/cpu.weight (to 1 (minimum))?
>
> WEIGHT     cpu.weight   tg->shares
>
> 3          0            3072
>
> 15         1            15360
>
>            1            10240
>
> `A/cpu.weight` follows cgroup-v2's `weights` `resource distribution
> model`* but I can only see `A/cpu.idle` as a layer on top of it forcing
> `A/cpu.weight` to get its minimum value?
>
> *Documentation/admin-guide/cgroup-v2.rst

Setting cpu.idle carries additional properties in addition to just the
weight. Currently, it primarily includes (a) special wakeup preemption
handling, and (b) contribution to idle_h_nr_running for the purpose of
marking a cpu as a sched_idle_cpu(). Essentially, the current
SCHED_IDLE mechanics. I've also discussed with Peter a potential
extension to SCHED_IDLE to manipulate vruntime.

We set the cgroup weight here, since by definition SCHED_IDLE entities
have the least scheduling weight. From the perspective of your
question, the analogous statement for tasks would be that we set task
weight to the min when doing setsched(SCHED_IDLE), even though we
already have a renice mechanism.