* [PATCH 2/2] cgroup, cpuset: remove cgroup_subsys->post_clone()
[not found] ` <20121113202714.GA21185-9pTldWuhBndy/B6EtB590w@public.gmane.org>
@ 2012-11-13 20:27 ` Tejun Heo
[not found] ` <20121113202745.GB21185-9pTldWuhBndy/B6EtB590w@public.gmane.org>
2012-11-14 15:50 ` [PATCH 1/2] cgroup: s/CGRP_CLONE_CHILDREN/CGRP_CPUSET_CLONE_CHILDREN/ Serge Hallyn
2012-11-19 5:39 ` Li Zefan
2 siblings, 1 reply; 6+ messages in thread
From: Tejun Heo @ 2012-11-13 20:27 UTC (permalink / raw)
To: Li Zefan
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From bbf2566f9f4fc79ff3320e83cafb69533efc9ea0 Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Tue, 13 Nov 2012 12:21:50 -0800
Currently CGRP_CPUSET_CLONE_CHILDREN triggers ->post_clone(). Now
that clone_children is cpuset specific, there's no reason to have this
rather odd option activation mechanism in cgroup core. cpuset can
check the flag from its ->css_allocate() and take the necessary
action.
Move cpuset_post_clone() logic to the end of cpuset_css_alloc() and
remove cgroup_subsys->post_clone().
Loosely based on Glauber's "generalize post_clone into post_create"
patch.
Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Original-patch-by: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Original-patch: <1351686554-22592-2-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
---
Documentation/cgroups/cgroups.txt | 8 ----
include/linux/cgroup.h | 1 -
kernel/cgroup.c | 4 --
kernel/cpuset.c | 80 ++++++++++++++++++---------------------
4 files changed, 36 insertions(+), 57 deletions(-)
diff --git a/Documentation/cgroups/cgroups.txt b/Documentation/cgroups/cgroups.txt
index 24cdf76..bcf1a00 100644
--- a/Documentation/cgroups/cgroups.txt
+++ b/Documentation/cgroups/cgroups.txt
@@ -642,14 +642,6 @@ void exit(struct task_struct *task)
Called during task exit.
-void post_clone(struct cgroup *cgrp)
-(cgroup_mutex held by caller)
-
-Called during cgroup_create() to do any parameter
-initialization which might be required before a task could attach. For
-example, in cpusets, no task may attach before 'cpus' and 'mems' are set
-up.
-
void bind(struct cgroup *root)
(cgroup_mutex held by caller)
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 926d8d1..d5fc8a7 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -449,7 +449,6 @@ struct cgroup_subsys {
void (*fork)(struct task_struct *task);
void (*exit)(struct cgroup *cgrp, struct cgroup *old_cgrp,
struct task_struct *task);
- void (*post_clone)(struct cgroup *cgrp);
void (*bind)(struct cgroup *root);
int subsys_id;
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index a49572e..35aa18b 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4142,10 +4142,6 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
if (err)
goto err_free_all;
}
- /* At error, ->css_free() callback has to free assigned ID. */
- if (test_bit(CGRP_CPUSET_CLONE_CHILDREN, &parent->flags) &&
- ss->post_clone)
- ss->post_clone(cgrp);
if (ss->broken_hierarchy && !ss->warned_broken_hierarchy &&
parent->parent) {
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 0693133..b017887 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1784,56 +1784,20 @@ static struct cftype files[] = {
};
/*
- * post_clone() is called during cgroup_create() when the
- * clone_children mount argument was specified. The cgroup
- * can not yet have any tasks.
- *
- * Currently we refuse to set up the cgroup - thereby
- * refusing the task to be entered, and as a result refusing
- * the sys_unshare() or clone() which initiated it - if any
- * sibling cpusets have exclusive cpus or mem.
- *
- * If this becomes a problem for some users who wish to
- * allow that scenario, then cpuset_post_clone() could be
- * changed to grant parent->cpus_allowed-sibling_cpus_exclusive
- * (and likewise for mems) to the new cgroup. Called with cgroup_mutex
- * held.
- */
-static void cpuset_post_clone(struct cgroup *cgroup)
-{
- struct cgroup *parent, *child;
- struct cpuset *cs, *parent_cs;
-
- parent = cgroup->parent;
- list_for_each_entry(child, &parent->children, sibling) {
- cs = cgroup_cs(child);
- if (is_mem_exclusive(cs) || is_cpu_exclusive(cs))
- return;
- }
- cs = cgroup_cs(cgroup);
- parent_cs = cgroup_cs(parent);
-
- mutex_lock(&callback_mutex);
- cs->mems_allowed = parent_cs->mems_allowed;
- cpumask_copy(cs->cpus_allowed, parent_cs->cpus_allowed);
- mutex_unlock(&callback_mutex);
- return;
-}
-
-/*
* cpuset_css_alloc - allocate a cpuset css
* cont: control group that the new cpuset will be part of
*/
static struct cgroup_subsys_state *cpuset_css_alloc(struct cgroup *cont)
{
- struct cpuset *cs;
- struct cpuset *parent;
+ struct cgroup *parent_cg = cont->parent;
+ struct cgroup *tmp_cg;
+ struct cpuset *parent, *cs;
- if (!cont->parent) {
+ if (!parent_cg)
return &top_cpuset.css;
- }
- parent = cgroup_cs(cont->parent);
+ parent = cgroup_cs(parent_cg);
+
cs = kmalloc(sizeof(*cs), GFP_KERNEL);
if (!cs)
return ERR_PTR(-ENOMEM);
@@ -1855,7 +1819,36 @@ static struct cgroup_subsys_state *cpuset_css_alloc(struct cgroup *cont)
cs->parent = parent;
number_of_cpusets++;
- return &cs->css ;
+
+ if (!test_bit(CGRP_CPUSET_CLONE_CHILDREN, &cont->flags))
+ goto skip_clone;
+
+ /*
+ * Clone @parent's configuration if CGRP_CPUSET_CLONE_CHILDREN is
+ * set. This flag handling is implemented in cgroup core for
+ * histrical reasons - the flag may be specified during mount.
+ *
+ * Currently, if any sibling cpusets have exclusive cpus or mem, we
+ * refuse to clone the configuration - thereby refusing the task to
+ * be entered, and as a result refusing the sys_unshare() or
+ * clone() which initiated it. If this becomes a problem for some
+ * users who wish to allow that scenario, then this could be
+ * changed to grant parent->cpus_allowed-sibling_cpus_exclusive
+ * (and likewise for mems) to the new cgroup.
+ */
+ list_for_each_entry(tmp_cg, &parent_cg->children, sibling) {
+ struct cpuset *tmp_cs = cgroup_cs(tmp_cg);
+
+ if (is_mem_exclusive(tmp_cs) || is_cpu_exclusive(tmp_cs))
+ goto skip_clone;
+ }
+
+ mutex_lock(&callback_mutex);
+ cs->mems_allowed = parent->mems_allowed;
+ cpumask_copy(cs->cpus_allowed, parent->cpus_allowed);
+ mutex_unlock(&callback_mutex);
+skip_clone:
+ return &cs->css;
}
/*
@@ -1882,7 +1875,6 @@ struct cgroup_subsys cpuset_subsys = {
.css_free = cpuset_css_free,
.can_attach = cpuset_can_attach,
.attach = cpuset_attach,
- .post_clone = cpuset_post_clone,
.subsys_id = cpuset_subsys_id,
.base_cftypes = files,
.early_init = 1,
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] cgroup: s/CGRP_CLONE_CHILDREN/CGRP_CPUSET_CLONE_CHILDREN/
[not found] ` <20121113202714.GA21185-9pTldWuhBndy/B6EtB590w@public.gmane.org>
2012-11-13 20:27 ` [PATCH 2/2] cgroup, cpuset: remove cgroup_subsys->post_clone() Tejun Heo
@ 2012-11-14 15:50 ` Serge Hallyn
2012-11-19 5:39 ` Li Zefan
2 siblings, 0 replies; 6+ messages in thread
From: Serge Hallyn @ 2012-11-14 15:50 UTC (permalink / raw)
To: Tejun Heo
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Quoting Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org):
> From 6e405c1ae4018d813e8ed9e0bd463d6976aebfa8 Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Date: Tue, 13 Nov 2012 12:21:50 -0800
>
> clone_children is only meaningful for cpuset and will stay that way.
> Rename the flag to reflect that and update documentation. Also, drop
> clone_children() wrapper in cgroup.c. The thin wrapper is used only a
> few times and one of them will go away soon.
>
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Thanks.
(both)
Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
> Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> ---
> These two patches are based on top of "cgroup: allow->post_create() to
> fail" patchset.
>
> http://thread.gmane.org/gmane.linux.kernel.cgroups/5047
> git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-cgroup-online-may-fail
>
> and availalbe in the following branch.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-cgroup-remove-post_clone
>
> Thanks.
>
> Documentation/cgroups/cgroups.txt | 8 +++-----
> include/linux/cgroup.h | 6 ++++--
> kernel/cgroup.c | 28 ++++++++++++----------------
> 3 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/Documentation/cgroups/cgroups.txt b/Documentation/cgroups/cgroups.txt
> index b06eea2..24cdf76 100644
> --- a/Documentation/cgroups/cgroups.txt
> +++ b/Documentation/cgroups/cgroups.txt
> @@ -299,11 +299,9 @@ a cgroup hierarchy's release_agent path is empty.
> 1.5 What does clone_children do ?
> ---------------------------------
>
> -If the clone_children flag is enabled (1) in a cgroup, then all
> -cgroups created beneath will call the post_clone callbacks for each
> -subsystem of the newly created cgroup. Usually when this callback is
> -implemented for a subsystem, it copies the values of the parent
> -subsystem, this is the case for the cpuset.
> +This flag only affects the cpuset controller. If the clone_children
> +flag is enabled (1) in a cgroup, a new cpuset cgroup will copy its
> +configuration from the parent during initialization.
>
> 1.6 How do I use cgroups ?
> --------------------------
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 89c631d..926d8d1 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -143,9 +143,11 @@ enum {
> /* Control Group requires release notifications to userspace */
> CGRP_NOTIFY_ON_RELEASE,
> /*
> - * Clone cgroup values when creating a new child cgroup
> + * Clone the parent's configuration when creating a new child
> + * cpuset cgroup. For historical reasons, this option can be
> + * specified at mount time and thus is implemented here.
> */
> - CGRP_CLONE_CHILDREN,
> + CGRP_CPUSET_CLONE_CHILDREN,
> };
>
> struct cgroup {
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 46c5119..a49572e 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -296,11 +296,6 @@ static int notify_on_release(const struct cgroup *cgrp)
> return test_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags);
> }
>
> -static int clone_children(const struct cgroup *cgrp)
> -{
> - return test_bit(CGRP_CLONE_CHILDREN, &cgrp->flags);
> -}
> -
> /*
> * for_each_subsys() allows you to iterate on each subsystem attached to
> * an active hierarchy
> @@ -1101,7 +1096,7 @@ static int cgroup_show_options(struct seq_file *seq, struct dentry *dentry)
> seq_puts(seq, ",xattr");
> if (strlen(root->release_agent_path))
> seq_printf(seq, ",release_agent=%s", root->release_agent_path);
> - if (clone_children(&root->top_cgroup))
> + if (test_bit(CGRP_CPUSET_CLONE_CHILDREN, &root->top_cgroup.flags))
> seq_puts(seq, ",clone_children");
> if (strlen(root->name))
> seq_printf(seq, ",name=%s", root->name);
> @@ -1113,7 +1108,7 @@ struct cgroup_sb_opts {
> unsigned long subsys_mask;
> unsigned long flags;
> char *release_agent;
> - bool clone_children;
> + bool cpuset_clone_children;
> char *name;
> /* User explicitly requested empty subsystem */
> bool none;
> @@ -1164,7 +1159,7 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
> continue;
> }
> if (!strcmp(token, "clone_children")) {
> - opts->clone_children = true;
> + opts->cpuset_clone_children = true;
> continue;
> }
> if (!strcmp(token, "xattr")) {
> @@ -1474,8 +1469,8 @@ static struct cgroupfs_root *cgroup_root_from_opts(struct cgroup_sb_opts *opts)
> strcpy(root->release_agent_path, opts->release_agent);
> if (opts->name)
> strcpy(root->name, opts->name);
> - if (opts->clone_children)
> - set_bit(CGRP_CLONE_CHILDREN, &root->top_cgroup.flags);
> + if (opts->cpuset_clone_children)
> + set_bit(CGRP_CPUSET_CLONE_CHILDREN, &root->top_cgroup.flags);
> return root;
> }
>
> @@ -3905,7 +3900,7 @@ fail:
> static u64 cgroup_clone_children_read(struct cgroup *cgrp,
> struct cftype *cft)
> {
> - return clone_children(cgrp);
> + return test_bit(CGRP_CPUSET_CLONE_CHILDREN, &cgrp->flags);
> }
>
> static int cgroup_clone_children_write(struct cgroup *cgrp,
> @@ -3913,9 +3908,9 @@ static int cgroup_clone_children_write(struct cgroup *cgrp,
> u64 val)
> {
> if (val)
> - set_bit(CGRP_CLONE_CHILDREN, &cgrp->flags);
> + set_bit(CGRP_CPUSET_CLONE_CHILDREN, &cgrp->flags);
> else
> - clear_bit(CGRP_CLONE_CHILDREN, &cgrp->flags);
> + clear_bit(CGRP_CPUSET_CLONE_CHILDREN, &cgrp->flags);
> return 0;
> }
>
> @@ -4130,8 +4125,8 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
> if (notify_on_release(parent))
> set_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags);
>
> - if (clone_children(parent))
> - set_bit(CGRP_CLONE_CHILDREN, &cgrp->flags);
> + if (test_bit(CGRP_CPUSET_CLONE_CHILDREN, &parent->flags))
> + set_bit(CGRP_CPUSET_CLONE_CHILDREN, &cgrp->flags);
>
> for_each_subsys(root, ss) {
> struct cgroup_subsys_state *css;
> @@ -4148,7 +4143,8 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
> goto err_free_all;
> }
> /* At error, ->css_free() callback has to free assigned ID. */
> - if (clone_children(parent) && ss->post_clone)
> + if (test_bit(CGRP_CPUSET_CLONE_CHILDREN, &parent->flags) &&
> + ss->post_clone)
> ss->post_clone(cgrp);
>
> if (ss->broken_hierarchy && !ss->warned_broken_hierarchy &&
> --
> 1.7.11.7
>
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply [flat|nested] 6+ messages in thread