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

Groups > linux.kernel > #1690328 > 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 Waiman Long <longman@redhat.com> - 2017-07-18 16:40 +0200
    Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Tejun Heo <tj@kernel.org> - 2017-07-18 19:20 +0200
      Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Waiman Long <longman@redhat.com> - 2017-07-18 19:30 +0200
        Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Tejun Heo <tj@kernel.org> - 2017-07-19 18:30 +0200
          Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Waiman Long <longman@redhat.com> - 2017-07-19 19:10 +0200
            Re: [PATCH 5/6] cgroup: implement cgroup v2 thread support Tejun Heo <tj@kernel.org> - 2017-07-19 19:50 +0200

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

On 07/17/2017 04:56 PM, Waiman Long wrote:
> On 07/17/2017 10:14 AM, Peter Zijlstra wrote:
>> On Sun, Jul 16, 2017 at 10:07:20PM -0400, Tejun Heo wrote:
>>> v4: - Updated to marking each cgroup threaded as suggested by PeterZ.
>>>
>>> +On creation, a cgroup is always a domain cgroup and can be made
>>> +threaded by writing "threaded" to the "cgroup.type" file.  The
>>> +operation is single direction::
>>> +
>>> +  # echo threaded > cgroup.type
>>> +
>>> +Once threaded, the cgroup can't be made a domain again.  To enable the
>>> +thread mode, the following conditions must be met.
>>> +
>>> +- As the cgroup will join the parent's resource domain.  The parent
>>> +  must either be a valid (threaded) domain or a threaded cgroup.
>>> +
>>> +- The cgroup must be empty.  No enabled controllers, child cgroups or
>>> +  processes.
>>> +
>>> +Topology-wise, a cgroup can be in an invalid state.  Please consider
>>> +the following toplogy::
>>> +
>>> +  A (threaded domain) - B (threaded) - C (domain, just created)
>>> +

Thinking about it some more. There is a place for invalid domain. It is
not the child of a threaded cgroup. It is the siblings of a threaded
cgroup whose parent is not root.

       Root - A (domain) - B (domain)
                         \ C (domain)

With "echo threaded > B/cgroup.type":

       Root - A (threaded domain) - B (threaded)
                                  \ C (domain, invalid)

Any children of a threaded cgroup should be threaded.

Cheers,
Longman

[toc] | [next] | [standalone]

#1690499

Hello, Waiman.

On Tue, Jul 18, 2017 at 10:37:41AM -0400, Waiman Long wrote:
> Thinking about it some more. There is a place for invalid domain. It is
> not the child of a threaded cgroup. It is the siblings of a threaded
> cgroup whose parent is not root.
> 
>        Root - A (domain) - B (domain)
>                          \ C (domain)
> 
> With "echo threaded > B/cgroup.type":
> 
>        Root - A (threaded domain) - B (threaded)
>                                   \ C (domain, invalid)

Yes, I noted that when I was replying to Peter.

> Any children of a threaded cgroup should be threaded.

It's really difficult to discuss if you just declare that something
should be a certain way without giving rationale for thinking so.

If we could get rid of the invalid state completely that way, I'd
completely agree with you but that isn't the case here as you noted
yourself, so the choice between the two isn't something trivially
clear.  Both choices come with their pros and cons.  We can absoultely
discuss them comparing the pros and cons.

Thanks.

-- 
tejun

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

#1690513

On 07/18/2017 01:10 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Tue, Jul 18, 2017 at 10:37:41AM -0400, Waiman Long wrote:
>> Thinking about it some more. There is a place for invalid domain. It is
>> not the child of a threaded cgroup. It is the siblings of a threaded
>> cgroup whose parent is not root.
>>
>>        Root - A (domain) - B (domain)
>>                          \ C (domain)
>>
>> With "echo threaded > B/cgroup.type":
>>
>>        Root - A (threaded domain) - B (threaded)
>>                                   \ C (domain, invalid)
> Yes, I noted that when I was replying to Peter.
>
>> Any children of a threaded cgroup should be threaded.
> It's really difficult to discuss if you just declare that something
> should be a certain way without giving rationale for thinking so.
>
> If we could get rid of the invalid state completely that way, I'd
> completely agree with you but that isn't the case here as you noted
> yourself, so the choice between the two isn't something trivially
> clear.  Both choices come with their pros and cons.  We can absoultely
> discuss them comparing the pros and cons.
I am not advocating on removing the invalid state now as I note about
sibling cgroups. I am just saying that there is no point in not doing an
automatic conversion to threaded for newly created children of threaded
cgroups (not thread root). I don't see any cons in doing that.

Cheers,
Longman

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

#1692012

Hello,

On Tue, Jul 18, 2017 at 01:23:14PM -0400, Waiman Long wrote:
> > If we could get rid of the invalid state completely that way, I'd
> > completely agree with you but that isn't the case here as you noted
> > yourself, so the choice between the two isn't something trivially
> > clear.  Both choices come with their pros and cons.  We can absoultely
> > discuss them comparing the pros and cons.
>
> I am not advocating on removing the invalid state now as I note about

Yeah, removing invalid state would be great but we can't at least yet.

> sibling cgroups. I am just saying that there is no point in not doing an
> automatic conversion to threaded for newly created children of threaded
> cgroups (not thread root). I don't see any cons in doing that.

So, the cons I see is inconsistency, now and in the future.

This may seem less clear with system root because we can have both
domain and theraded children below it, which makes a newly created
cgroup being a domain seem natural.  More importantly, we can't do it
any other way because we'd break existing users otherwise - creating a
threaded cgroup would cause future first level cgroups to be threaded
which will be very unexpected.

Let's think about a non-root threaded domain.  At least for now, a
non-root threaded domain is terminal - they can't host valid domain
children.  As the alternative term "thread root" implies, the threaded
domain can be the root of a threaded subtree and nothing else, so it's
kinda weird to make a new child cgroup there start out as a domain
which can't be used, just like it'd be for the second level descendant
cgroup.

However, the alternative is even stranger.  Let's say we make the
first level child automatically threaded, but that is inconsistent
with when we first enable threaded mode.  We either would have to turn
all siblings at the same time or disallow enabling threaded mode if
there are domain siblings, which I fear would be unnecessarily
restrictive.

Another point is that what if we eventually make non-root threaded
roots able to host domain children?  Making children automatically
threaded wouldn't make any sense then, right?  I'll come back to this
later.

So, it looks like if we're gonna automatically turn on threaded mode
for new cgroups, the only thing we can do right now is what you're
suggesting; however, we didn't arrive there through some
straight-forward intuition or overall design.  It started as a simple
idea (I want it to be automatic) but the end result is a contorted
destination shaped by constraints and happenstance.

To me, behaving differently on the first-level threaded children than
on second+ level ones is too strange to be justified by the
convenience of not having to turn on threaded on new cgroups.

On top of that, what happens if we get to implement PeterZ's idea of
skipping over threaded internal cgroups to allow domains under
threaded subtrees?  That'd imply that we'd be able to host domains
under threaded domains too.  The end result would be completely
non-sensical.  We'd be defaulting to different modes for different
reasons where half of those reasons won't hold anymore.  This isn't
surprising given that there's nothing actually consistent about the
suggested default behavior.

So, that's why I think it'd be better to be simple here, even if that
adds a bit of hassle when creating threded children.  It is simple and
consistent and can stay that way even if we make the hierarchy more
flexible in the future.

Thanks.

-- 
tejun

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

#1692038

On 07/19/2017 12:29 PM, Tejun Heo wrote:
> Hello,
>
> On Tue, Jul 18, 2017 at 01:23:14PM -0400, Waiman Long wrote:
>>> If we could get rid of the invalid state completely that way, I'd
>>> completely agree with you but that isn't the case here as you noted
>>> yourself, so the choice between the two isn't something trivially
>>> clear.  Both choices come with their pros and cons.  We can absoultely
>>> discuss them comparing the pros and cons.
>> I am not advocating on removing the invalid state now as I note about
> Yeah, removing invalid state would be great but we can't at least yet.
>
>> sibling cgroups. I am just saying that there is no point in not doing an
>> automatic conversion to threaded for newly created children of threaded
>> cgroups (not thread root). I don't see any cons in doing that.
> So, the cons I see is inconsistency, now and in the future.
>
> This may seem less clear with system root because we can have both
> domain and theraded children below it, which makes a newly created
> cgroup being a domain seem natural.  More importantly, we can't do it
> any other way because we'd break existing users otherwise - creating a
> threaded cgroup would cause future first level cgroups to be threaded
> which will be very unexpected.
>
> Let's think about a non-root threaded domain.  At least for now, a
> non-root threaded domain is terminal - they can't host valid domain
> children.  As the alternative term "thread root" implies, the threaded
> domain can be the root of a threaded subtree and nothing else, so it's
> kinda weird to make a new child cgroup there start out as a domain
> which can't be used, just like it'd be for the second level descendant
> cgroup.
>
> However, the alternative is even stranger.  Let's say we make the
> first level child automatically threaded, but that is inconsistent
> with when we first enable threaded mode.  We either would have to turn
> all siblings at the same time or disallow enabling threaded mode if
> there are domain siblings, which I fear would be unnecessarily
> restrictive.
>
> Another point is that what if we eventually make non-root threaded
> roots able to host domain children?  Making children automatically
> threaded wouldn't make any sense then, right?  I'll come back to this
> later.
>
> So, it looks like if we're gonna automatically turn on threaded mode
> for new cgroups, the only thing we can do right now is what you're
> suggesting; however, we didn't arrive there through some
> straight-forward intuition or overall design.  It started as a simple
> idea (I want it to be automatic) but the end result is a contorted
> destination shaped by constraints and happenstance.
>
> To me, behaving differently on the first-level threaded children than
> on second+ level ones is too strange to be justified by the
> convenience of not having to turn on threaded on new cgroups.

OK, I get your point of being inconsistent. However, I don't think that
is a big deal.

> On top of that, what happens if we get to implement PeterZ's idea of
> skipping over threaded internal cgroups to allow domains under
> threaded subtrees?  That'd imply that we'd be able to host domains
> under threaded domains too.  The end result would be completely
> non-sensical.  We'd be defaulting to different modes for different
> reasons where half of those reasons won't hold anymore.  This isn't
> surprising given that there's nothing actually consistent about the
> suggested default behavior.

For me, that is the only good reason why we should keep the current
behavior. So I am fine with that.

+ cgrp->dom_cgrp = cgrp->dom_cgrp;

However, I am still puzzled by above line of code, should it be just

  cgrp->dom_cgrp = cgrp;

Cheers,
Longman

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

#1692063

Hello, Waiman.

On Wed, Jul 19, 2017 at 01:09:38PM -0400, Waiman Long wrote:
> For me, that is the only good reason why we should keep the current
> behavior. So I am fine with that.
> 
> + cgrp->dom_cgrp = cgrp->dom_cgrp;
> 
> However, I am still puzzled by above line of code, should it be just
> 
>   cgrp->dom_cgrp = cgrp;

Oh I see.  Yeah, that's just a silly (harmless) bug.  The field gets
properly initialized in init_cgroup_housekeeping().  I'll remove that
line.

Thanks!

-- 
tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web