Message-ID: <20170612123150.scopfxela7v26dct@hirez.programming.kicks-ass.net>
Date:   Mon, 12 Jun 2017 14:31:50 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Tejun Heo <tj@...nel.org>
Cc:     Li Zefan <lizefan@...wei.com>, hannes@...xchg.org,
        mingo@...hat.com, longman@...hat.com, cgroups@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-team@...com, pjt@...gle.com,
        luto@...capital.net, efault@....de, torvalds@...ux-foundation.org
Subject: Re: [PATCHSET for-4.13] cgroup: implement cgroup2 thread mode, v2


Please don't rush this; also, I might not be around much the coming
weeks due to taking some leave 'soon' (kid #3 is imminent).

And I really need more time to look at this (and re-read the old
discussions, because I've forgot most everything again).

On Sat, Jun 10, 2017 at 10:03:41AM -0400, Tejun Heo wrote:

> * Thread mode is explicitly enabled on a cgroup by writing "enable"
>   into "cgroup.threads" file.  The cgroup shouldn't have any child
>   cgroups or enabled controllers.
> 
> * Once enabled, arbitrary sub-hierarchy can be created and threads can
>   be put anywhere in the subtree by writing TIDs into "cgroup.threads"
>   file.  Process granularity and no-internal-process constraint don't
>   apply in a threaded subtree.
> 
> * To be used in a threaded subtree, controllers should explicitly
>   declare thread mode support and should be able to handle internal
>   competition in some way.
> 
> * The root of a threaded subtree serves as the resource domain for the
>   whole subtree.  This is where all the controllers are guaranteed to
>   have a common ground and resource consumptions in the threaded
>   subtree which aren't tied to a specific thread are charged.
>   Non-threaded controllers never see beyond thread root and can assume
>   that all controllers will follow the same rules upto that point.
> 
> * Root cgroup can enable thread mode anytime and a first level child
>   can opt-in to that thread subtree anchored at root by writing "join"
>   to "cgroup.threads" files, start its own thread subtree or just be a
>   normal cgroup.

Yuck... this again is a consequence of tagging the 'wrong' thing. Again,
the primary construct is the resource domain.

If you use that as a tag, you don't need this weird join crap. Because
as soon as you clear the 'resource domain' flag on a group, it instantly
becomes a thread group and 'obviously' connects to the first parent that
is a resource domain.

And, as per the last time, this threaded marker isn't uniquely
identifying things, so it hard prohibits from ever extending the model
to allow resource domains nested in a thread subtree. Now I understand
why you don't implement that now -- you were struggling with the views
API, but that is no excuse to create an API that permanently disables
that feature.

I cannot at this time remember if there was a strong use-case for that
scenario -- like said, I really need to re-read the email threads, but I
might not have enough time to do so now.

Again, please don't rush this.

> This allows threaded controllers to implement thread granular resource
> control without getting in the way of system level resource
> partitioning.
> 
> This patchset contains the following ten patches.  For more details on
> the interface and behavior, please refer to 0007.
> 
>  0001-cgroup-separate-out-cgroup_has_tasks.patch
>  0002-cgroup-reorganize-cgroup.procs-task-write-path.patch
>  0003-cgroup-Fix-reference-counting-bug-in-cgroup_procs_wr.patch
>  0004-cgroup-add-flags-to-css_task_iter_start-and-implemen.patch
>  0005-cgroup-introduce-cgroup-proc_cgrp-and-threaded-css_s.patch
>  0006-cgroup-implement-CSS_TASK_ITER_THREADED.patch
>  0007-cgroup-implement-cgroup-v2-thread-support.patch
>  0008-sched-Misc-preps-for-cgroup-unified-hierarchy-interf.patch
>  0009-sched-Implement-interface-for-cgroup-unified-hierarc.patch
>  0010-sched-Make-cpu-cpuacct-threaded-controllers.patch
> 
> 0001-0007 implement cgroup2 thread mode.  0008-0010 enable CPU
> controller on cgroup2 and mark them as supporting thread mode.
> 0008-0010 are included for reference.

So I really regret the 'shares' interface; we really should have done a
nice thing.

  https://lkml.kernel.org/r/20170410073622.2y6tnpcd2ssuoztz@hirez.programming.kicks-ass.net

So I would like to change to that instead of the weird 100 thing.

As for the RT thing, the runtime/period thing is not a MAX but a MIN
limit (conceptually -- in practise its both).

Also, we need cpuset to be a thread controller.

Powered by blists - more mailing lists