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]

Message-ID: <20170719154046.GN3365493@devbig577.frc2.facebook.com>
Date:   Wed, 19 Jul 2017 11:40:46 -0400
From:   Tejun Heo <tj@...nel.org>
To:     Waiman Long <longman@...hat.com>
Cc:     lizefan@...wei.com, hannes@...xchg.org, peterz@...radead.org,
        mingo@...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,
        guro@...com
Subject: Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support

Hello, Waiman.

On Mon, Jul 17, 2017 at 05:12:31PM -0400, Waiman Long wrote:
> > +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.

Yeah, I was a bit hesitant to introduce an extra term for it, but both
terms make sense and thread root is less cumbersome.  I'll incorporate
it into the doc.

> >    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?

Yeah, will update.

> > @@ -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.

I'll respond to this on the other sub-thread.

Thanks.

-- 
tejun