LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Tejun Heo <tj@kernel.org>
To: Waiman Long <longman@redhat.com>
Cc: "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>,
	"Michal Koutný" <mkoutny@suse.com>
Subject: Re: [PATCH v6 5/6] cgroup/cpuset: Update description of cpuset.cpus.partition in cgroup-v2.rst
Date: Mon, 16 Aug 2021 07:08:31 -1000	[thread overview]
Message-ID: <YRqbj5+ZdS+7k0Fn@slm.duckdns.org> (raw)
In-Reply-To: <20210814205743.3039-6-longman@redhat.com>

On Sat, Aug 14, 2021 at 04:57:42PM -0400, Waiman Long wrote:
> +	A parent partition may distribute all its CPUs to its child
> +	partitions as long as it is not the root cgroup and there is no
> +	task directly associated with that parent partition.  Otherwise,

"there is not task directly associated with the parent partition" isn't
necessary, right? That's already enforced by the cgroup hierarchy itself.

> +	there must be at least one cpu left in the parent partition.
> +	A new task cannot be moved to a partition root with no effective
> +	cpu.
> +
> +	Once becoming a partition root, changes to "cpuset.cpus"
> +	is generally allowed as long as the first condition above
> +	(cpu exclusivity rule) is true.

All the above ultimately says is that "a new task cannot be moved to a
partition root with no effective cpu", but I don't understand why this would
be a separate rule. Shouldn't the partition just stop being a partition when
it doesn't have any exclusive cpu? What's the benefit of having multiple its
own failure mode?

> +	Sometimes, changes to "cpuset.cpus" or cpu hotplug may cause
> +	the state of the partition root to become invalid when the
> +	other constraints of partition root are violated.  Therefore,
> +	it is recommended that users should always set "cpuset.cpus"
> +	to the proper value first before enabling partition.  In case
> +	"cpuset.cpus" has to be modified after partition is enabled,
> +	users should check the state of "cpuset.cpus.partition" after
> +	making change to it to make sure that the partition is still
> +	valid.

So, idk why the this doesn't cover the one above it. Also, this really
should be worded a lot stronger. It's not just recommended - confirming and
monitoring the transitions is an integral and essential part of using
cpuset.

...
> +	An invalid partition is not a real partition even though the
> +	restriction of the cpu exclusivity rule will still apply.

Is there a reason we can't bring this in line with other failure behaviors?

> +	In the special case of a parent partition competing with a child
> +	partition for the only CPU left, the parent partition wins and
> +	the child partition becomes invalid.

Given that parent partitions are *always* empty, this rule doesn't seem to
make sense.

So, I think this definitely is a step in the right direction but still seems
to be neither here or there. Before, we pretended that we could police the
input when we couldn't. Now, we're changing the interface so that it
includes configuration failures as an integral part; however, we're still
policing some particular inputs while letting other inputs pass through and
trigger failures and why one is handled one way while the other differently
seems rather arbitrary.

Thanks.

-- 
tejun

  reply	other threads:[~2021-08-16 17:08 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-14 20:57 [PATCH-cgroup v6 0/6] cgroup/cpuset: Add new cpuset partition type & empty effecitve cpus Waiman Long
2021-08-14 20:57 ` [PATCH v6 1/6] cgroup/cpuset: Properly transition to invalid partition Waiman Long
2021-08-14 20:57 ` [PATCH v6 2/6] cgroup/cpuset: Show invalid partition reason string Waiman Long
2021-08-14 20:57 ` [PATCH v6 3/6] cgroup/cpuset: Add a new isolated cpus.partition type Waiman Long
2021-08-14 20:57 ` [PATCH v6 4/6] cgroup/cpuset: Allow non-top parent partition to distribute out all CPUs Waiman Long
2021-08-14 20:57 ` [PATCH v6 5/6] cgroup/cpuset: Update description of cpuset.cpus.partition in cgroup-v2.rst Waiman Long
2021-08-16 17:08   ` Tejun Heo [this message]
2021-08-24  5:35     ` Waiman Long
2021-08-24 19:04       ` Tejun Heo
2021-08-25 19:21         ` Waiman Long
2021-08-25 19:24           ` Tejun Heo
2021-08-14 20:57 ` [PATCH v6 6/6] kselftest/cgroup: Add cpuset v2 partition root state test Waiman Long

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YRqbj5+ZdS+7k0Fn@slm.duckdns.org \
    --to=tj@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=cgroups@vger.kernel.org \
    --cc=corbet@lwn.net \
    --cc=frederic@kernel.org \
    --cc=guro@fb.com \
    --cc=hannes@cmpxchg.org \
    --cc=juri.lelli@redhat.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=lizefan.x@bytedance.com \
    --cc=longman@redhat.com \
    --cc=mkoutny@suse.com \
    --cc=mtosatti@redhat.com \
    --cc=pauld@redhat.com \
    --cc=peterz@infradead.org \
    --cc=shuah@kernel.org \
    --subject='Re: [PATCH v6 5/6] cgroup/cpuset: Update description of cpuset.cpus.partition in cgroup-v2.rst' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).