linux-kernel - Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Jul 2017 17:12:31 -0400
From:   Waiman Long <longman@...hat.com>
To:     Tejun Heo <tj@...nel.org>, lizefan@...wei.com, hannes@...xchg.org,
        peterz@...radead.org, mingo@...hat.com
Cc:     cgroups@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-team@...com, pjt@...gle.com, luto@...capital.net,
        efault@....de, torvalds@...ux-foundation.org, guro@...com
Subject: Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support

On 07/16/2017 10:07 PM, Tejun Heo wrote:
>  
> +Threads
> +~~~~~~~
> +
> +cgroup v2 supports thread granularity for a subset of controllers to
> +support use cases requiring hierarchical resource distribution across
> +the threads of a group of processes.  By default, all threads of a
> +process belong to the same cgroup, which also serves as the resource
> +domain to host resource consumptions which are not specific to a
> +process or thread.  The thread mode allows threads to be spread across
> +a subtree while still maintaining the common resource domain for them.
> +
> +Controllers which support thread mode are called threaded controllers.
> +The ones which don't are called domain controllers.
> +
> +Marking a cgroup threaded makes it join the resource domain of its
> +parent as a threaded cgroup.  The parent may be another threaded
> +cgroup whose resource domain is further up in the hierarchy.  The root
> +of a threaded subtree, that is, the nearest ancestor which is not
> +threaded, is called threaded domain and serves as the resource domain
> +for the entire subtree.

The cgroup code uses the term "thread root" in quite a number of places.
So a developer may be confused when comparing the code and the
documentation. I would recommend either introducing "thread root" as an
alias for threaded domain here in the documentation or documenting that
"threaded domain = thread root" in the code.

> +  cgroup.type
> +
> +	A read-write single value file which exists on non-root
> +	cgroups.
> +
> +	When read, it indicates the current type of the cgroup, which
> +	can be one of the following values.
> +
> +	- "domain" : A normal valid domain cgroup.
> +
> +	- "domain (threaded)" : A threaded domain cgroup which is
> +          serving as the root of a threaded subtree.
> +
> +	- "domain (invalid)" : A cgroup which is in an invalid state.
> +	  It can't be populated or have controllers enabled.  It may
> +	  be allowed to become a threaded cgroup.
> +
> +	- "threaded" : A threaded cgroup which is a member of a
> +          threaded subtree.
> +
> +	A cgroup can be turned into a threaded cgroup by writing
> +	"threaded" to this file.
> +
>    cgroup.procs
>  	A read-write new-line separated values file which exists on
>  	all cgroups.

Do we need to document that cgroup.procs isn't writable in a threaded
cgroup?

> @@ -4301,6 +4606,7 @@ static struct cgroup *cgroup_create(struct cgroup *parent)
>  	cgrp->self.parent = &parent->self;
>  	cgrp->root = root;
>  	cgrp->level = level;
> +	cgrp->dom_cgrp = cgrp->dom_cgrp;

It is a no-op. I think it is better to modify it to

+    cgrp->dom_cgrp = cgroup_is_threaded(parent) ? parent->dom_cgrp : cgrp;

Then we won't have an invalid domain state.

Cheers,
Longman