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

Groups > linux.kernel > #1641674 > unrolled thread

[RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics

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

  [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics Waiman Long <longman@redhat.com> - 2017-05-15 15:40 +0200
    Re: [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics Tejun Heo <tj@kernel.org> - 2017-05-17 23:50 +0200
      Re: [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics Waiman Long <longman@redhat.com> - 2017-05-18 19:30 +0200
    Re: [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics Tejun Heo <tj@kernel.org> - 2017-05-19 22:30 +0200
      Re: [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics Tejun Heo <tj@kernel.org> - 2017-05-19 23:00 +0200
      Re: [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics Waiman Long <longman@redhat.com> - 2017-05-22 19:20 +0200
        Re: [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics Waiman Long <longman@redhat.com> - 2017-05-22 19:40 +0200
        Re: [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics Tejun Heo <tj@kernel.org> - 2017-05-24 22:40 +0200
          Re: [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics Waiman Long <longman@redhat.com> - 2017-05-24 23:20 +0200
            Re: [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics Tejun Heo <tj@kernel.org> - 2017-05-24 23:30 +0200

#1641674 — [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics

The current thread mode semantics aren't sufficient to fully support
threaded controllers like cpu. The main problem is that when thread
mode is enabled at root (mainly for performance reason), all the
non-threaded controllers cannot be supported at all.

To alleviate this problem, the roles of thread root and threaded
cgroups are now further separated. Now thread mode can only be enabled
on a non-root leaf cgroup whose parent will then become the thread
root. All the descendants of a threaded cgroup will still need to be
threaded. All the non-threaded resource will be accounted for in the
thread root. Unlike the previous thread mode, however, a thread root
can have non-threaded children where system resources like memory
can be further split down the hierarchy.

Now we could have something like

	R -- A -- B
	 \
	  T1 -- T2

where R is the thread root, A and B are non-threaded cgroups, T1 and
T2 are threaded cgroups. The cgroups R, T1, T2 form a threaded subtree
where all the non-threaded resources are accounted for in R.  The no
internal process constraint does not apply in the threaded subtree.
Non-threaded controllers need to properly handle the competition
between internal processes and child cgroups at the thread root.

This model will be flexible enough to support the need of the threaded
controllers.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 Documentation/cgroup-v2.txt     |  51 +++++++----
 kernel/cgroup/cgroup-internal.h |  10 +++
 kernel/cgroup/cgroup.c          | 186 +++++++++++++++++++++++++++++++++++-----
 3 files changed, 209 insertions(+), 38 deletions(-)

diff --git a/Documentation/cgroup-v2.txt b/Documentation/cgroup-v2.txt
index 1c6f5a9..3ae7e9c 100644
--- a/Documentation/cgroup-v2.txt
+++ b/Documentation/cgroup-v2.txt
@@ -222,21 +222,32 @@ process can be put in different cgroups and are not subject to the no
 internal process constraint - threaded controllers can be enabled on
 non-leaf cgroups whether they have threads in them or not.
 
-To enable the thread mode, the following conditions must be met.
+To enable the thread mode on a cgroup, the following conditions must
+be met.
 
-- The thread root doesn't have any child cgroups.
+- The cgroup doesn't have any child cgroups.
 
-- The thread root doesn't have any controllers enabled.
+- The cgroup doesn't have any non-threaded controllers enabled.
+
+- The cgroup doesn't have any processes attached to it.
 
 Thread mode can be enabled by writing "enable" to "cgroup.threads"
 file.
 
   # echo enable > cgroup.threads
 
-Inside a threaded subtree, "cgroup.threads" can be read and contains
-the list of the thread IDs of all threads in the cgroup.  Except that
-the operations are per-thread instead of per-process, "cgroup.threads"
-has the same format and behaves the same way as "cgroup.procs".
+The parent of the threaded cgroup will become the thread root, if
+it hasn't been a thread root yet. In other word, thread mode cannot
+be enabled on the root cgroup as it doesn't have a parent cgroup. A
+thread root can have child cgroups and controllers enabled before
+becoming one.
+
+A threaded subtree includes the thread root and all the threaded child
+cgroups as well as their descendants which are all threaded cgroups.
+"cgroup.threads" can be read and contains the list of the thread
+IDs of all threads in the cgroup.  Except that the operations are
+per-thread instead of per-process, "cgroup.threads" has the same
+format and behaves the same way as "cgroup.procs".
 
 The thread root serves as the resource domain for the whole subtree,
 and, while the threads can be scattered across the subtree, all the
@@ -246,25 +257,30 @@ not readable in the subtree proper.  However, "cgroup.procs" can be
 written to from anywhere in the subtree to migrate all threads of the
 matching process to the cgroup.
 
-Only threaded controllers can be enabled in a threaded subtree.  When
-a threaded controller is enabled inside a threaded subtree, it only
-accounts for and controls resource consumptions associated with the
-threads in the cgroup and its descendants.  All consumptions which
-aren't tied to a specific thread belong to the thread root.
+Only threaded controllers can be enabled in a non-root threaded cgroup.
+When a threaded controller is enabled inside a threaded subtree,
+it only accounts for and controls resource consumptions associated
+with the threads in the cgroup and its descendants.  All consumptions
+which aren't tied to a specific thread belong to the thread root.
 
 Because a threaded subtree is exempt from no internal process
 constraint, a threaded controller must be able to handle competition
 between threads in a non-leaf cgroup and its child cgroups.  Each
 threaded controller defines how such competitions are handled.
 
+A new child cgroup created under a thread root will not be threaded.
+Thread mode has to be explicitly enabled on each of the thread root's
+children.  Descendants of a threaded cgroup, however, will always be
+threaded and that mode cannot be disabled.
+
 To disable the thread mode, the following conditions must be met.
 
-- The cgroup is a thread root.  Thread mode can't be disabled
-  partially in the subtree.
+- The cgroup is a child of a thread root.  Thread mode can't be
+  disabled partially further down the hierarchy.
 
-- The thread root doesn't have any child cgroups.
+- The cgroup doesn't have any child cgroups.
 
-- The thread root doesn't have any controllers enabled.
+- The cgroup doesn't have any threads attached to it.
 
 Thread mode can be disabled by writing "disable" to "cgroup.threads"
 file.
@@ -366,6 +382,9 @@ with any other cgroups and requires special treatment from most
 controllers.  How resource consumption in the root cgroup is governed
 is up to each controller.
 
+The threaded cgroups and the thread roots are also exempt from this
+restriction.
+
 Note that the restriction doesn't get in the way if there is no
 enabled controller in the cgroup's "cgroup.subtree_control".  This is
 important as otherwise it wouldn't be possible to create children of a
diff --git a/kernel/cgroup/cgroup-internal.h b/kernel/cgroup/cgroup-internal.h
index 2c8e3a9..15abaa0 100644
--- a/kernel/cgroup/cgroup-internal.h
+++ b/kernel/cgroup/cgroup-internal.h
@@ -124,6 +124,16 @@ static inline bool notify_on_release(const struct cgroup *cgrp)
 	return test_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags);
 }
 
+static inline bool cgroup_is_threaded(const struct cgroup *cgrp)
+{
+	return cgrp->proc_cgrp && (cgrp->proc_cgrp != cgrp);
+}
+
+static inline bool cgroup_is_thread_root(const struct cgroup *cgrp)
+{
+	return cgrp->proc_cgrp == cgrp;
+}
+
 void put_css_set_locked(struct css_set *cset);
 
 static inline void put_css_set(struct css_set *cset)
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 7e3ddfb..11cb091 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -334,8 +334,13 @@ static u16 cgroup_control(struct cgroup *cgrp)
 	struct cgroup *parent = cgroup_parent(cgrp);
 	u16 root_ss_mask = cgrp->root->subsys_mask;
 
-	if (parent)
-		return parent->subtree_control;
+	if (parent) {
+		u16 ss_mask = parent->subtree_control;
+
+		if (cgroup_is_threaded(cgrp))
+			ss_mask &= cgrp_dfl_threaded_ss_mask;
+		return ss_mask;
+	}
 
 	if (cgroup_on_dfl(cgrp))
 		root_ss_mask &= ~(cgrp_dfl_inhibit_ss_mask |
@@ -348,8 +353,13 @@ static u16 cgroup_ss_mask(struct cgroup *cgrp)
 {
 	struct cgroup *parent = cgroup_parent(cgrp);
 
-	if (parent)
-		return parent->subtree_ss_mask;
+	if (parent) {
+		u16 ss_mask = parent->subtree_ss_mask;
+
+		if (cgroup_is_threaded(cgrp))
+			ss_mask &= cgrp_dfl_threaded_ss_mask;
+		return ss_mask;
+	}
 
 	return cgrp->root->subsys_mask;
 }
@@ -598,6 +608,24 @@ static bool css_set_threaded(struct css_set *cset)
 }
 
 /**
+ * threaded_children_count - returns # of threaded children
+ * @cgrp: cgroup to be tested
+ *
+ * cgroup_mutex must be held by the caller.
+ */
+static int threaded_children_count(struct cgroup *cgrp)
+{
+	struct cgroup *child;
+	int count = 0;
+
+	lockdep_assert_held(&cgroup_mutex);
+	cgroup_for_each_live_child(child, cgrp)
+		if (cgroup_is_threaded(child))
+			count++;
+	return count;
+}
+
+/**
  * cgroup_update_populated - updated populated count of a cgroup
  * @cgrp: the target cgroup
  * @populated: inc or dec populated count
@@ -2926,15 +2954,15 @@ static ssize_t cgroup_subtree_control_write(struct kernfs_open_file *of,
 	}
 
 	/* can't enable !threaded controllers on a threaded cgroup */
-	if (cgrp->proc_cgrp && (enable & ~cgrp_dfl_threaded_ss_mask)) {
+	if (cgroup_is_threaded(cgrp) && (enable & ~cgrp_dfl_threaded_ss_mask)) {
 		ret = -EBUSY;
 		goto out_unlock;
 	}
 
 	/*
-	 * Except for root and threaded cgroups, subtree_control must be
-	 * zero for a cgroup with tasks so that child cgroups don't compete
-	 * against tasks.
+	 * Except for root, thread roots and threaded cgroups, subtree_control
+	 * must be zero for a cgroup with tasks so that child cgroups don't
+	 * compete against tasks.
 	 */
 	if (enable && cgroup_parent(cgrp) && !cgrp->proc_cgrp) {
 		struct cgrp_cset_link *link;
@@ -2982,22 +3010,48 @@ static int cgroup_enable_threaded(struct cgroup *cgrp)
 	LIST_HEAD(csets);
 	struct cgrp_cset_link *link;
 	struct css_set *cset, *cset_next;
+	struct cgroup *child;
 	int ret;
+	u16 ss_mask;
 
 	lockdep_assert_held(&cgroup_mutex);
 
 	/* noop if already threaded */
-	if (cgrp->proc_cgrp)
+	if (cgroup_is_threaded(cgrp))
 		return 0;
 
-	/* allow only if there are neither children or enabled controllers */
-	if (css_has_online_children(&cgrp->self) || cgrp->subtree_control)
+	/*
+	 * Allow only if it is not the root and there are:
+	 * 1) no children,
+	 * 2) no non-threaded controllers are enabled, and
+	 * 3) no attached tasks.
+	 *
+	 * With no attached tasks, it is assumed that no css_sets will be
+	 * linked to the current cgroup. This may not be true if some dead
+	 * css_sets linger around due to task_struct leakage, for example.
+	 */
+	if (css_has_online_children(&cgrp->self) ||
+	   (cgroup_control(cgrp) & ~cgrp_dfl_threaded_ss_mask) ||
+	   !cgroup_parent(cgrp) || cgroup_is_populated(cgrp))
 		return -EBUSY;
 
-	/* find all csets which need ->proc_cset updated */
+	/* make the parent cgroup a thread root */
+	child = cgrp;
+	cgrp = cgroup_parent(child);
+
+	/* noop for parent if parent has already been threaded */
+	if (cgrp->proc_cgrp)
+		goto setup_child;
+
+	/*
+	 * For the parent cgroup, we need to find all csets which need
+	 * ->proc_cset updated
+	 */
 	spin_lock_irq(&css_set_lock);
 	list_for_each_entry(link, &cgrp->cset_links, cset_link) {
 		cset = link->cset;
+		if (cset->dead)
+			continue;
 		if (css_set_populated(cset)) {
 			WARN_ON_ONCE(css_set_threaded(cset));
 			WARN_ON_ONCE(cset->pcset_preload);
@@ -3036,7 +3090,34 @@ static int cgroup_enable_threaded(struct cgroup *cgrp)
 	/* mark it threaded */
 	cgrp->proc_cgrp = cgrp;
 
-	return 0;
+setup_child:
+	ss_mask = cgroup_ss_mask(child);
+	/*
+	 * If some non-threaded controllers are enabled, they have to be
+	 * disabled.
+	 */
+	if (ss_mask & ~cgrp_dfl_threaded_ss_mask) {
+		cgroup_save_control(child);
+		child->proc_cgrp = cgrp;
+		ret = cgroup_apply_control(child);
+		cgroup_finalize_control(child, ret);
+		kernfs_activate(child->kn);
+
+		/*
+		 * If an error happen (it shouldn't), the thread mode
+		 * enablement fails, but the parent will remain as thread
+		 * root. That shouldn't be a problem as a thread root
+		 * without threaded children is not much different from
+		 * a non-threaded cgroup.
+		 */
+		WARN_ON_ONCE(ret);
+		if (ret)
+			child->proc_cgrp = NULL;
+	} else {
+		child->proc_cgrp = cgrp;
+		ret = 0;
+	}
+	return ret;
 
 err_put_csets:
 	spin_lock_irq(&css_set_lock);
@@ -3055,26 +3136,71 @@ static int cgroup_enable_threaded(struct cgroup *cgrp)
 static int cgroup_disable_threaded(struct cgroup *cgrp)
 {
 	struct cgrp_cset_link *link;
+	struct cgroup *parent = cgroup_parent(cgrp);
 
 	lockdep_assert_held(&cgroup_mutex);
 
-	/* noop if already !threaded */
-	if (!cgrp->proc_cgrp)
-		return 0;
-
 	/* partial disable isn't supported */
-	if (cgrp->proc_cgrp != cgrp)
+	if (cgrp->proc_cgrp != parent)
 		return -EBUSY;
 
-	/* allow only if there are neither children or enabled controllers */
-	if (css_has_online_children(&cgrp->self) || cgrp->subtree_control)
+	/* noop if not a threaded cgroup */
+	if (!cgroup_is_threaded(cgrp))
+		return 0;
+
+	/*
+	 * Allow only if there are
+	 * 1) no children, and
+	 * 2) no attached tasks.
+	 *
+	 * With no attached tasks, it is assumed that no css_sets will be
+	 * linked to the current cgroup. This may not be true if some dead
+	 * css_sets linger around due to task_struct leakage, for example.
+	 */
+	if (css_has_online_children(&cgrp->self) || cgroup_is_populated(cgrp))
 		return -EBUSY;
 
-	/* walk all csets and reset ->proc_cset */
+	/*
+	 * If the cgroup has some non-threaded controllers enabled at the
+	 * subtree_control level of the parent, we need to re-enabled those
+	 * controllers.
+	 */
+	cgrp->proc_cgrp = NULL;
+	if (cgroup_ss_mask(cgrp) & ~cgrp_dfl_threaded_ss_mask) {
+		int ret;
+
+		cgrp->proc_cgrp = parent;
+		cgroup_save_control(cgrp);
+		cgrp->proc_cgrp = NULL;
+		ret = cgroup_apply_control(cgrp);
+		cgroup_finalize_control(cgrp, ret);
+		kernfs_activate(cgrp->kn);
+
+		/*
+		 * If an error happen, we abandon update to the thread root
+		 * and return the erorr.
+		 */
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Check remaining threaded children count to see if the threaded
+	 * csets of the parent need to be removed and ->proc_cset reset.
+	 */
 	spin_lock_irq(&css_set_lock);
+
+	if (threaded_children_count(parent))
+		goto out_unlock;	/* still have threaded children left */
+
+	cgrp = parent;
 	list_for_each_entry(link, &cgrp->cset_links, cset_link) {
 		struct css_set *cset = link->cset;
 
+		/* skip dead css_set */
+		if (cset->dead)
+			continue;
+
 		if (css_set_threaded(cset)) {
 			struct css_set *pcset = proc_css_set(cset);
 
@@ -3090,6 +3216,7 @@ static int cgroup_disable_threaded(struct cgroup *cgrp)
 		}
 	}
 	cgrp->proc_cgrp = NULL;
+out_unlock:
 	spin_unlock_irq(&css_set_lock);
 
 	return 0;
@@ -4480,7 +4607,16 @@ static struct cgroup *cgroup_create(struct cgroup *parent)
 	cgrp->self.parent = &parent->self;
 	cgrp->root = root;
 	cgrp->level = level;
-	cgrp->proc_cgrp = parent->proc_cgrp;
+
+	/*
+	 * A child cgroup created directly under a thread root will not
+	 * be threaded. Thread mode has to be explicitly enabled for it.
+	 * The child cgroup will be threaded if its parent is threaded.
+	 */
+	if (cgroup_is_thread_root(parent))
+		cgrp->proc_cgrp = NULL;
+	else
+		cgrp->proc_cgrp = parent->proc_cgrp;
 
 	for (tcgrp = cgrp; tcgrp; tcgrp = cgroup_parent(tcgrp))
 		cgrp->ancestor_ids[tcgrp->level] = tcgrp->id;
@@ -4712,6 +4848,12 @@ static int cgroup_destroy_locked(struct cgroup *cgrp)
 		return -EBUSY;
 
 	/*
+	 * Do an implicit thread mode disable if on default hierarchy.
+	 */
+	if (cgroup_on_dfl(cgrp))
+		cgroup_disable_threaded(cgrp);
+
+	/*
 	 * Mark @cgrp and the associated csets dead.  The former prevents
 	 * further task migration and child creation by disabling
 	 * cgroup_lock_live_group().  The latter makes the csets ignored by
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1643654

Hello, Waiman.

On Mon, May 15, 2017 at 09:34:10AM -0400, Waiman Long wrote:
> The current thread mode semantics aren't sufficient to fully support
> threaded controllers like cpu. The main problem is that when thread
> mode is enabled at root (mainly for performance reason), all the
> non-threaded controllers cannot be supported at all.
> 
> To alleviate this problem, the roles of thread root and threaded
> cgroups are now further separated. Now thread mode can only be enabled
> on a non-root leaf cgroup whose parent will then become the thread
> root. All the descendants of a threaded cgroup will still need to be
> threaded. All the non-threaded resource will be accounted for in the
> thread root. Unlike the previous thread mode, however, a thread root
> can have non-threaded children where system resources like memory
> can be further split down the hierarchy.
> 
> Now we could have something like
> 
> 	R -- A -- B
> 	 \
> 	  T1 -- T2
> 
> where R is the thread root, A and B are non-threaded cgroups, T1 and
> T2 are threaded cgroups. The cgroups R, T1, T2 form a threaded subtree
> where all the non-threaded resources are accounted for in R.  The no
> internal process constraint does not apply in the threaded subtree.
> Non-threaded controllers need to properly handle the competition
> between internal processes and child cgroups at the thread root.
> 
> This model will be flexible enough to support the need of the threaded
> controllers.

I do like the approach and it does address the issue with requiring at
least one level of nesting for the thread mode to be used with other
controllers.  I need to think a bit more about it and mull over what
Peterz was suggesting in the old thread.  I'll get back to you soon
but I'd really prefer this and the earlier related patches to be in
its own patchset so that we aren't dealing with different things at
the same time.

Thanks.

-- 
tejun

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

#1644767

On 05/17/2017 05:47 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Mon, May 15, 2017 at 09:34:10AM -0400, Waiman Long wrote:
>> The current thread mode semantics aren't sufficient to fully support
>> threaded controllers like cpu. The main problem is that when thread
>> mode is enabled at root (mainly for performance reason), all the
>> non-threaded controllers cannot be supported at all.
>>
>> To alleviate this problem, the roles of thread root and threaded
>> cgroups are now further separated. Now thread mode can only be enabled
>> on a non-root leaf cgroup whose parent will then become the thread
>> root. All the descendants of a threaded cgroup will still need to be
>> threaded. All the non-threaded resource will be accounted for in the
>> thread root. Unlike the previous thread mode, however, a thread root
>> can have non-threaded children where system resources like memory
>> can be further split down the hierarchy.
>>
>> Now we could have something like
>>
>> 	R -- A -- B
>> 	 \
>> 	  T1 -- T2
>>
>> where R is the thread root, A and B are non-threaded cgroups, T1 and
>> T2 are threaded cgroups. The cgroups R, T1, T2 form a threaded subtree
>> where all the non-threaded resources are accounted for in R.  The no
>> internal process constraint does not apply in the threaded subtree.
>> Non-threaded controllers need to properly handle the competition
>> between internal processes and child cgroups at the thread root.
>>
>> This model will be flexible enough to support the need of the threaded
>> controllers.
> I do like the approach and it does address the issue with requiring at
> least one level of nesting for the thread mode to be used with other
> controllers.  I need to think a bit more about it and mull over what
> Peterz was suggesting in the old thread.  I'll get back to you soon
> but I'd really prefer this and the earlier related patches to be in
> its own patchset so that we aren't dealing with different things at
> the same time.
>
> Thanks.
>
I have studied the email exchanges with your original thread mode
patchset. This patchset is aimed to hopefully address all the concerns
that Peterz has. This enhanced thread mode should address a big part of
the concern. However, I am not sure if this patch, by itself, is enough
to address all his concerns. That is why I also include 2 other major
changes in the next 2 patches. My goal is to move forward to allow all
controllers to be enabled for v2 eventually. We are not there yet, but I
hope this patchset can move thing forward meaningfully.

Regards,
Longman

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

#1645898

Hello, Waiman.

On Mon, May 15, 2017 at 09:34:10AM -0400, Waiman Long wrote:
> Now we could have something like
> 
> 	R -- A -- B
> 	 \
> 	  T1 -- T2
> 
> where R is the thread root, A and B are non-threaded cgroups, T1 and
> T2 are threaded cgroups. The cgroups R, T1, T2 form a threaded subtree
> where all the non-threaded resources are accounted for in R.  The no
> internal process constraint does not apply in the threaded subtree.
> Non-threaded controllers need to properly handle the competition
> between internal processes and child cgroups at the thread root.
> 
> This model will be flexible enough to support the need of the threaded
> controllers.

Maybe I'm misunderstanding the design, but this seems to push the
processes which belong to the threaded subtree to the parent which is
part of the usual resource domain hierarchy thus breaking the no
internal competition constraint.  I'm not sure this is something we'd
want.  Given that the limitation of the original threaded mode was the
required nesting below root and that we treat root special anyway
(exactly in the way necessary), I wonder whether it'd be better to
simply allow root to be both domain and thread root.

Specific review points below but we'd probably want to discuss the
overall design first.

> +static inline bool cgroup_is_threaded(const struct cgroup *cgrp)
> +{
> +	return cgrp->proc_cgrp && (cgrp->proc_cgrp != cgrp);
> +}
> +
> +static inline bool cgroup_is_thread_root(const struct cgroup *cgrp)
> +{
> +	return cgrp->proc_cgrp == cgrp;
> +}

Maybe add a bit of comments explaining what's going on with
->proc_cgrp?

>  /**
> + * threaded_children_count - returns # of threaded children
> + * @cgrp: cgroup to be tested
> + *
> + * cgroup_mutex must be held by the caller.
> + */
> +static int threaded_children_count(struct cgroup *cgrp)
> +{
> +	struct cgroup *child;
> +	int count = 0;
> +
> +	lockdep_assert_held(&cgroup_mutex);
> +	cgroup_for_each_live_child(child, cgrp)
> +		if (cgroup_is_threaded(child))
> +			count++;
> +	return count;
> +}

It probably would be a good idea to keep track of the count so that we
don't have to count them each time.  There are cases where people end
up creating a very high number of cgroups and we've already been
bitten a couple times with silly complexity issues.

> @@ -2982,22 +3010,48 @@ static int cgroup_enable_threaded(struct cgroup *cgrp)
>  	LIST_HEAD(csets);
>  	struct cgrp_cset_link *link;
>  	struct css_set *cset, *cset_next;
> +	struct cgroup *child;
>  	int ret;
> +	u16 ss_mask;
>  
>  	lockdep_assert_held(&cgroup_mutex);
>  
>  	/* noop if already threaded */
> -	if (cgrp->proc_cgrp)
> +	if (cgroup_is_threaded(cgrp))
>  		return 0;
>  
> -	/* allow only if there are neither children or enabled controllers */
> -	if (css_has_online_children(&cgrp->self) || cgrp->subtree_control)
> +	/*
> +	 * Allow only if it is not the root and there are:
> +	 * 1) no children,
> +	 * 2) no non-threaded controllers are enabled, and
> +	 * 3) no attached tasks.
> +	 *
> +	 * With no attached tasks, it is assumed that no css_sets will be
> +	 * linked to the current cgroup. This may not be true if some dead
> +	 * css_sets linger around due to task_struct leakage, for example.
> +	 */

It doesn't look like the code is actually making this (incorrect)
assumption.  I suppose the comment is from before
cgroup_is_populated() was added?

>  	spin_lock_irq(&css_set_lock);
>  	list_for_each_entry(link, &cgrp->cset_links, cset_link) {
>  		cset = link->cset;
> +		if (cset->dead)
> +			continue;

Hmm... is this a bug fix which is necessary regardless of whether we
change the threadroot semantics or not?

Thanks.

-- 
tejun

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

#1645925

Hello,

On Fri, May 19, 2017 at 04:26:24PM -0400, Tejun Heo wrote:
> (exactly in the way necessary), I wonder whether it'd be better to
> simply allow root to be both domain and thread root.

I'll give this approach a shot early next week.

Thanks.

-- 
tejun

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

#1647205

On 05/19/2017 04:26 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Mon, May 15, 2017 at 09:34:10AM -0400, Waiman Long wrote:
>> Now we could have something like
>>
>> 	R -- A -- B
>> 	 \
>> 	  T1 -- T2
>>
>> where R is the thread root, A and B are non-threaded cgroups, T1 and
>> T2 are threaded cgroups. The cgroups R, T1, T2 form a threaded subtree
>> where all the non-threaded resources are accounted for in R.  The no
>> internal process constraint does not apply in the threaded subtree.
>> Non-threaded controllers need to properly handle the competition
>> between internal processes and child cgroups at the thread root.
>>
>> This model will be flexible enough to support the need of the threaded
>> controllers.
> Maybe I'm misunderstanding the design, but this seems to push the
> processes which belong to the threaded subtree to the parent which is
> part of the usual resource domain hierarchy thus breaking the no
> internal competition constraint.  I'm not sure this is something we'd
> want.  Given that the limitation of the original threaded mode was the
> required nesting below root and that we treat root special anyway
> (exactly in the way necessary), I wonder whether it'd be better to
> simply allow root to be both domain and thread root.

Yes, root can be both domain and thread root. I haven't placed any
restriction on that.

>
> Specific review points below but we'd probably want to discuss the
> overall design first.
>
>> +static inline bool cgroup_is_threaded(const struct cgroup *cgrp)
>> +{
>> +	return cgrp->proc_cgrp && (cgrp->proc_cgrp != cgrp);
>> +}
>> +
>> +static inline bool cgroup_is_thread_root(const struct cgroup *cgrp)
>> +{
>> +	return cgrp->proc_cgrp == cgrp;
>> +}
> Maybe add a bit of comments explaining what's going on with
> ->proc_cgrp?

Sure, will do that.

>>  /**
>> + * threaded_children_count - returns # of threaded children
>> + * @cgrp: cgroup to be tested
>> + *
>> + * cgroup_mutex must be held by the caller.
>> + */
>> +static int threaded_children_count(struct cgroup *cgrp)
>> +{
>> +	struct cgroup *child;
>> +	int count = 0;
>> +
>> +	lockdep_assert_held(&cgroup_mutex);
>> +	cgroup_for_each_live_child(child, cgrp)
>> +		if (cgroup_is_threaded(child))
>> +			count++;
>> +	return count;
>> +}
> It probably would be a good idea to keep track of the count so that we
> don't have to count them each time.  There are cases where people end
> up creating a very high number of cgroups and we've already been
> bitten a couple times with silly complexity issues.

Thanks for the suggestion, I can keep a count in the cgroup strcture to
avoid doing that repetitively.

>
>> @@ -2982,22 +3010,48 @@ static int cgroup_enable_threaded(struct cgroup *cgrp)
>>  	LIST_HEAD(csets);
>>  	struct cgrp_cset_link *link;
>>  	struct css_set *cset, *cset_next;
>> +	struct cgroup *child;
>>  	int ret;
>> +	u16 ss_mask;
>>  
>>  	lockdep_assert_held(&cgroup_mutex);
>>  
>>  	/* noop if already threaded */
>> -	if (cgrp->proc_cgrp)
>> +	if (cgroup_is_threaded(cgrp))
>>  		return 0;
>>  
>> -	/* allow only if there are neither children or enabled controllers */
>> -	if (css_has_online_children(&cgrp->self) || cgrp->subtree_control)
>> +	/*
>> +	 * Allow only if it is not the root and there are:
>> +	 * 1) no children,
>> +	 * 2) no non-threaded controllers are enabled, and
>> +	 * 3) no attached tasks.
>> +	 *
>> +	 * With no attached tasks, it is assumed that no css_sets will be
>> +	 * linked to the current cgroup. This may not be true if some dead
>> +	 * css_sets linger around due to task_struct leakage, for example.
>> +	 */
> It doesn't look like the code is actually making this (incorrect)
> assumption.  I suppose the comment is from before
> cgroup_is_populated() was added?

Yes, it is a bug. I should have checked the tasks_count instead of using
cgroup_is_populated. Thanks for catching that.

>
>>  	spin_lock_irq(&css_set_lock);
>>  	list_for_each_entry(link, &cgrp->cset_links, cset_link) {
>>  		cset = link->cset;
>> +		if (cset->dead)
>> +			continue;
> Hmm... is this a bug fix which is necessary regardless of whether we
> change the threadroot semantics or not?

That is true. I put it there because the the reference counting bug
fixed in patch 6 caused a lot of dead csets hanging around before the
fix. I can pull this out as a separate patch.

Cheers,
Longman

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

#1647211

On 05/22/2017 01:13 PM, Waiman Long wrote:
> On 05/19/2017 04:26 PM, Tejun Heo wrote:
>>> @@ -2982,22 +3010,48 @@ static int cgroup_enable_threaded(struct cgroup *cgrp)
>>>  	LIST_HEAD(csets);
>>>  	struct cgrp_cset_link *link;
>>>  	struct css_set *cset, *cset_next;
>>> +	struct cgroup *child;
>>>  	int ret;
>>> +	u16 ss_mask;
>>>  
>>>  	lockdep_assert_held(&cgroup_mutex);
>>>  
>>>  	/* noop if already threaded */
>>> -	if (cgrp->proc_cgrp)
>>> +	if (cgroup_is_threaded(cgrp))
>>>  		return 0;
>>>  
>>> -	/* allow only if there are neither children or enabled controllers */
>>> -	if (css_has_online_children(&cgrp->self) || cgrp->subtree_control)
>>> +	/*
>>> +	 * Allow only if it is not the root and there are:
>>> +	 * 1) no children,
>>> +	 * 2) no non-threaded controllers are enabled, and
>>> +	 * 3) no attached tasks.
>>> +	 *
>>> +	 * With no attached tasks, it is assumed that no css_sets will be
>>> +	 * linked to the current cgroup. This may not be true if some dead
>>> +	 * css_sets linger around due to task_struct leakage, for example.
>>> +	 */
>> It doesn't look like the code is actually making this (incorrect)
>> assumption.  I suppose the comment is from before
>> cgroup_is_populated() was added?
> Yes, it is a bug. I should have checked the tasks_count instead of using
> cgroup_is_populated. Thanks for catching that.

Sorry, I would like to take it back. I think cgroup_is_populated() will
be set if there is any task attached to the cgroup. So I think it is
doing the right thing with regard to (3).

Cheers,
Longman

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

#1649934

Hello, Waiman.

On Mon, May 22, 2017 at 01:13:16PM -0400, Waiman Long wrote:
> > Maybe I'm misunderstanding the design, but this seems to push the
> > processes which belong to the threaded subtree to the parent which is
> > part of the usual resource domain hierarchy thus breaking the no
> > internal competition constraint.  I'm not sure this is something we'd
> > want.  Given that the limitation of the original threaded mode was the
> > required nesting below root and that we treat root special anyway
> > (exactly in the way necessary), I wonder whether it'd be better to
> > simply allow root to be both domain and thread root.
> 
> Yes, root can be both domain and thread root. I haven't placed any
> restriction on that.

I've been playing with the proposed "make the parent resource domain".
Unfortunately, the parent - child relationship becomes weird.

The parent becomes the thread root, which means that its
cgroup.threads file becomes writable and threads can be put in there.
It's really weird to write to a child's interface and have the
parent's behavior changed.  This becomes weirder with delegation.  If
a cgroup is delegated, its cgroup.threads should be delegated too but
if the child enables threaded mode, that makes the undelegated parent
thread root, which means that either 1. the delegatee can't migrate
threads to the thread root or 2. if the parent's cgroup.threads is
writeable, the delegatee can mass with other descendants under it
which shouldn't be allowed.

I think the operation of making a cgroup a thread root should happen
on the cgroup where that's requested; otherwise, nesting becomes too
twisted.  This should be solvable.  Will think more about it.

Thanks.

-- 
tejun

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

#1649951

On 05/24/2017 04:36 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Mon, May 22, 2017 at 01:13:16PM -0400, Waiman Long wrote:
>>> Maybe I'm misunderstanding the design, but this seems to push the
>>> processes which belong to the threaded subtree to the parent which is
>>> part of the usual resource domain hierarchy thus breaking the no
>>> internal competition constraint.  I'm not sure this is something we'd
>>> want.  Given that the limitation of the original threaded mode was the
>>> required nesting below root and that we treat root special anyway
>>> (exactly in the way necessary), I wonder whether it'd be better to
>>> simply allow root to be both domain and thread root.
>> Yes, root can be both domain and thread root. I haven't placed any
>> restriction on that.
> I've been playing with the proposed "make the parent resource domain".
> Unfortunately, the parent - child relationship becomes weird.
>
> The parent becomes the thread root, which means that its
> cgroup.threads file becomes writable and threads can be put in there.
> It's really weird to write to a child's interface and have the
> parent's behavior changed.  This becomes weirder with delegation.  If
> a cgroup is delegated, its cgroup.threads should be delegated too but
> if the child enables threaded mode, that makes the undelegated parent
> thread root, which means that either 1. the delegatee can't migrate
> threads to the thread root or 2. if the parent's cgroup.threads is
> writeable, the delegatee can mass with other descendants under it
> which shouldn't be allowed.
>
> I think the operation of making a cgroup a thread root should happen
> on the cgroup where that's requested; otherwise, nesting becomes too
> twisted.  This should be solvable.  Will think more about it.
>
> Thanks.
>
An alternative is to have separate enabling for thread root. For example,

# echo root > cgroup.threads
# echo enable > child/cgroup.threads

The first statement make the current cgroup the thread root. However,
setting it to a thread root doesn't make its child to be threaded. This
have to be explicitly done on each of the children. Once a child cgroup
is made to be threaded, all its descendants will be threaded. That will
have the same effect as the current patch.

With delegation, do you mean the relationship between a container and
its host?

Cheers,
Longman

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

#1649954

Hello,

On Wed, May 24, 2017 at 05:17:13PM -0400, Waiman Long wrote:
> An alternative is to have separate enabling for thread root. For example,
> 
> # echo root > cgroup.threads
> # echo enable > child/cgroup.threads
> 
> The first statement make the current cgroup the thread root. However,
> setting it to a thread root doesn't make its child to be threaded. This
> have to be explicitly done on each of the children. Once a child cgroup
> is made to be threaded, all its descendants will be threaded. That will
> have the same effect as the current patch.

Yeah, I'm toying with different ideas.  I'll get back to you once
things get more concrete.

> With delegation, do you mean the relationship between a container and
> its host?

It can be but doesn't have to be.  For example, it can be delegations
to users without namespace / container being involved.

Thanks.

-- 
tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web