Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1690509 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Peter Zijlstra <peterz@infradead.org> - 2017-07-18 19:30 +0200
    Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Waiman Long <longman@redhat.com> - 2017-07-18 19:40 +0200
    Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Tejun Heo <tj@kernel.org> - 2017-07-18 20:00 +0200
      Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Peter Zijlstra <peterz@infradead.org> - 2017-07-18 20:50 +0200
        Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Tejun Heo <tj@kernel.org> - 2017-07-18 20:50 +0200
          Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Peter Zijlstra <peterz@infradead.org> - 2017-07-19 16:10 +0200
            Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Tejun Heo <tj@kernel.org> - 2017-07-19 18:40 +0200

#1690509 — Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support

On Mon, Jul 17, 2017 at 10:26:09AM -0400, Tejun Heo wrote:
> Hello, Peter.
> 
> On Mon, Jul 17, 2017 at 04:14:09PM +0200, Peter Zijlstra wrote:
> > AFAICT this is not in fact what I suggested... :/
> 
> Heh, sorry about misattributing that.  I was mostly referring to the
> overall idea of marking each cgroup domain or threaded rather than
> subtree.
> 
> > My proposal did not have that invalid state. It would simply refuse to
> > change the type from thread to domain in the case where the parent is
> > not a domain.
> > 
> > Also, my proposal maintained the normal property inheritance rules. A
> > child cgroup's creation 'type' would be that of its parent and not
> > always be 'domain'.
> 
> But aren't both of the above get weird when the parent can host both
> domain and threaded children?
> 
> 	  R
>        /
>       A(D)
> 
> If you create another child B under R, it's naturally gonna be a
> domain.  Let's say you turn that to threaded.
> 
> 	   R
>        /   \
>      A(D) B(T)
> 
> And now try to create another child C, should that be a domain or
> threaded?

Domain of course, as R must be a domain, and hence all its children
start out as such.

> If we only inherit from the second level on, which is in itself
> already confusing, that still leads to invalid configs for non-root
> thread roots.

I don't see how. I don't get the example Waiman gave, what is wrong
with:

	R (D)
	|
	A (D)
       / \
     C(D) B(T) 

? Afaict that's a perfectly valid configuration.

[toc] | [next] | [standalone]

#1690518

On 07/18/2017 01:28 PM, Peter Zijlstra wrote:
> On Mon, Jul 17, 2017 at 10:26:09AM -0400, Tejun Heo wrote:
>> Hello, Peter.
>>
>> On Mon, Jul 17, 2017 at 04:14:09PM +0200, Peter Zijlstra wrote:
>>> AFAICT this is not in fact what I suggested... :/
>> Heh, sorry about misattributing that.  I was mostly referring to the
>> overall idea of marking each cgroup domain or threaded rather than
>> subtree.
>>
>>> My proposal did not have that invalid state. It would simply refuse to
>>> change the type from thread to domain in the case where the parent is
>>> not a domain.
>>>
>>> Also, my proposal maintained the normal property inheritance rules. A
>>> child cgroup's creation 'type' would be that of its parent and not
>>> always be 'domain'.
>> But aren't both of the above get weird when the parent can host both
>> domain and threaded children?
>>
>> 	  R
>>        /
>>       A(D)
>>
>> If you create another child B under R, it's naturally gonna be a
>> domain.  Let's say you turn that to threaded.
>>
>> 	   R
>>        /   \
>>      A(D) B(T)
>>
>> And now try to create another child C, should that be a domain or
>> threaded?
> Domain of course, as R must be a domain, and hence all its children
> start out as such.
>
>> If we only inherit from the second level on, which is in itself
>> already confusing, that still leads to invalid configs for non-root
>> thread roots.
> I don't see how. I don't get the example Waiman gave, what is wrong
> with:
>
> 	R (D)
> 	|
> 	A (D)
>        / \
>      C(D) B(T) 
>
> ? Afaict that's a perfectly valid configuration.

From what I understand, C is considered to be in an invalid state
because of the no internal process rule. A in this case is the thread
root, so it can have internal process. If C is a domain and a child of
A, we will have the case that internal processes in A is competing
against cgroup C.

I have been advocating (in one of my RFC patches) that we should relax
the rules to allow internal processes when only threaded controllers are
enabled as they are supposed to be able to handle internal processes.

Cheers,
Longman

[toc] | [prev] | [next] | [standalone]

#1690537

Hello, Peter.

On Tue, Jul 18, 2017 at 07:28:01PM +0200, Peter Zijlstra wrote:
> > And now try to create another child C, should that be a domain or
> > threaded?
> 
> Domain of course, as R must be a domain, and hence all its children
> start out as such.

I don't think it's a matter of course as R also is the root of a
threaded subtree, but this is more or less bikeshedding.

> > If we only inherit from the second level on, which is in itself
> > already confusing, that still leads to invalid configs for non-root
> > thread roots.
> 
> I don't see how. I don't get the example Waiman gave, what is wrong
> with:
> 
> 	R (D)
> 	|
> 	A (D)
>        / \
>      C(D) B(T) 
> 
> ? Afaict that's a perfectly valid configuration.

Okay, we're kinda off the rails now.  Just to verify that we're on the
same page, are you also saying that the following should be a valid
configuration?

	R (D)
        |
	A (D and has processes in it and controllers enabled)
        |
        C (D and has processes in it) 

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1690565

On Tue, Jul 18, 2017 at 01:54:56PM -0400, Tejun Heo wrote:

> Okay, we're kinda off the rails now.  Just to verify that we're on the
> same page, are you also saying that the following should be a valid
> configuration?
> 
> 	R (D)
>         |
> 	A (D and has processes in it and controllers enabled)
>         |
>         C (D and has processes in it) 
> 

Argh, the no internal process thing again -- I completely forgot about
that :-(

[toc] | [prev] | [next] | [standalone]

#1690567

On Tue, Jul 18, 2017 at 08:41:35PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 18, 2017 at 01:54:56PM -0400, Tejun Heo wrote:
> 
> > Okay, we're kinda off the rails now.  Just to verify that we're on the
> > same page, are you also saying that the following should be a valid
> > configuration?
> > 
> > 	R (D)
> >         |
> > 	A (D and has processes in it and controllers enabled)
> >         |
> >         C (D and has processes in it) 
> > 
> 
> Argh, the no internal process thing again -- I completely forgot about
> that :-(

Heh, yeah, we wouldn't be talking about all these otherwise.  The
restriction is pain in the ass but at the same time useful for
full(er)-scope resource control.  Were there other things that caught
your eyes?

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1691703

On Tue, Jul 18, 2017 at 02:47:14PM -0400, Tejun Heo wrote:
> Were there other things that caught your eyes?

I didn't immediately see the point of "domain (threaded" output, and I
think it might be useful to have a "threaded" column in /proc/cgroups.
But no, I've been over this a few times now and I think we're good.

Yes, I think we can work with this. Thanks!

[toc] | [prev] | [next] | [standalone]

#1692019

Hello, Peter.

On Wed, Jul 19, 2017 at 04:07:28PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 18, 2017 at 02:47:14PM -0400, Tejun Heo wrote:
> > Were there other things that caught your eyes?
> 
> I didn't immediately see the point of "domain (threaded" output, and I

I'll probably drop the parens but I think it's meaningful to show the
state to userland as it affects what users can and can't do.

> think it might be useful to have a "threaded" column in /proc/cgroups.

Will add that.  We *might* want cgroup.controllers_threaded too but we
can think about that later.

> But no, I've been over this a few times now and I think we're good.
> 
> Yes, I think we can work with this. Thanks!

Awesome.  I'll update the patchset.

Thanks a lot for the inputs and patience!

-- 
tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web