Sender: Tejun Heo <htejun@gmail.com>
Date:   Wed, 1 Dec 2021 06:46:50 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Waiman Long <longman@redhat.com>
Cc:     Michal =?iso-8859-1?Q?Koutn=FD?= <mkoutny@suse.com>,
        Zefan Li <lizefan.x@bytedance.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Shuah Khan <shuah@kernel.org>, cgroups@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kselftest@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Roman Gushchin <guro@fb.com>, Phil Auld <pauld@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Frederic Weisbecker <frederic@kernel.org>,
        Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH v8 5/6] cgroup/cpuset: Update description of
 cpuset.cpus.partition in cgroup-v2.rst
Message-ID: <Yaem+r/YZ9BNXv9R@slm.duckdns.org>
References: <20211018143619.205065-1-longman@redhat.com>
 <20211018143619.205065-6-longman@redhat.com>
 <20211115193122.GA16798@blackbody.suse.cz>
 <8f68692b-bd8f-33fd-44ae-f6f83bf2dc00@redhat.com>
 <20211116175411.GA50019@blackbody.suse.cz>
 <293d7abf-aff6-fcd8-c999-b1dbda1cffb8@redhat.com>
 <YaZbXArNIMNvwJD/@slm.duckdns.org>
 <2347fe66-dc68-6d58-e63b-7ed2b8077b48@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2347fe66-dc68-6d58-e63b-7ed2b8077b48@redhat.com>
Precedence: bulk

Hello, Waiman.

On Tue, Nov 30, 2021 at 10:56:34PM -0500, Waiman Long wrote:
> > What happens if an isolated domain becomes invalid and then valid again due
> > to cpu hotplug? Does it go "root invalid" and then back to "isolated"?
>
> Yes, the current code allow recovering from an invalid state. In this
> particular case, the transition will be "isolated" --> "root invalid" -->
> "isolated".

Wouldn't it be clearer if it became "isolated invalid"?

> > While it isn't necessarily tied to this series, it's a big no-no to restrict
> > what a parent can do depending on what its descendants are doing. A cgroup
> > higher up in the hierarchy should be able to change configuration however it
> > sees fit as deligation breaks down otherwise.
> > 
> > Maybe you can argue that cpuset is special and shouldn't be subject to such
> > convention but I can't see strong enough justifications especially given
> > that most of these restrictions can be broken by hotplug operations anyway
> > and thus need code to handle those situations.
> 
> These are all pre-existing restrictions before the introduction of
> partition. These are checks done in validate_change(). I am just saying out
> loud the existing behavior. If you think that needs to be changed, I am fine
> with that. However, it will be a separate patch as it is not a behavior that
> is introduced by this series.

I see. It looks more problematic now with the addtion of the state
transition error reporting, more possible state transitions and, well,
actual documentation.

> Once an invalid partition is changed to "member", there is no way for a
> child invalid partition root to recover and become valid again. There is why
> I force them to become "member" also. I am OK if you believe it is better to
> keep them in the invalid state forever until we explicitly changed them to
> "member" eventually.

That's because we don't allow turning a cgroup with descendants into a
partition, right?

So, when we were first adding the partition support, the thinking was that
as it's pretty niche anyway, we can take some aberrations and restrictions,
but I don't think it's a good direction to be building up on top of those
like this and would much prefer to clean up the rules and restrictions. I
know that this has been going on for quite a while and am sorry that am
coming back to the same issue repeatedly which isn't necessarily caused by
the proposed change. What do you think?

Thanks.

-- 
tejun