[RFC] cgroup: deprecate clone_children

Commit Message

Tejun Heo Nov. 5, 2012, 6:02 p.m. UTC

clone_children makes cgroup invoke ->post_clone() callback if it
exists and sets CGRP_CLONE_CHILDREN.  ->post_clone(), while being
named generically, is only supposed to copy configuration from its
parent.

This is an entirely convenience feature which is only used by cpuset
to alter its configuration propagation.  As the mount option and
cgroupfs knobs are cgroup-wide and different controllers differ in
their configuration propagations, it's awkward to use for multiple
controllers especially if they're co-mounted.  At this point, we can't
use this flag for any other controller anyway as it would implicitly
change the behavior.

As this is unnecessary feature with very limited use and awkward in
co-mounted use cases, let's try to deprecate it.  Whine on the mount
option and accesses to cgroupfs knobs.

Signed-off-by: Tejun Heo <[e-mail]>
Cc: Glauber Costa <[e-mail]>
Cc: Peter Zijlstra <[e-mail]>
---
Glauber, I think this is more befitting change for .post_clone().  If
people aren't depending on this, I'd much prefer to just remove it.

Thanks.

 kernel/cgroup.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Serge E. Hallyn Nov. 5, 2012, 7:17 p.m. UTC | #1

Quoting Tejun Heo ([e-mail]):
> clone_children makes cgroup invoke ->post_clone() callback if it
> exists and sets CGRP_CLONE_CHILDREN.  ->post_clone(), while being
> named generically, is only supposed to copy configuration from its
> parent.
> 
> This is an entirely convenience feature which is only used by cpuset
> to alter its configuration propagation.  As the mount option and
> cgroupfs knobs are cgroup-wide and different controllers differ in
> their configuration propagations, it's awkward to use for multiple
> controllers especially if they're co-mounted.  At this point, we can't
> use this flag for any other controller anyway as it would implicitly
> change the behavior.
> 
> As this is unnecessary feature with very limited use and awkward in

clone_children is currently required by lxc.  Of course lxc could
manually set the .cpus and .mems in the newly created child, but if
those interfaces or others like it ever change (i.e. any new ones which
must be set before a task can join a cgroup) we'll get into yet more of
a rats nets of code to support different kernel versions.

(Just as an idea, if there was a way to generically tell from the list
of files in my cgroups dir which files need to be initialized before a
task can join, I think that would suffice.  Maybe a cgroups.needssetup
file which right now contains 'cpuset.mems\ncpuset.cpus'. Then if we
find a file we don't recognize in there we can throw an intelligent
error, or guess at duplicating the parent value.)

> co-mounted use cases, let's try to deprecate it.  Whine on the mount
> option and accesses to cgroupfs knobs.
> 
> Signed-off-by: Tejun Heo <[e-mail]>
> Cc: Glauber Costa <[e-mail]>
> Cc: Peter Zijlstra <[e-mail]>
> ---
> Glauber, I think this is more befitting change for .post_clone().  If

What do you mean by that?  That he is working on a patch-set which will
remove post_clone and this belongs there, or that there is a proposed
alternative?

> people aren't depending on this, I'd much prefer to just remove it.
> 
> Thanks.
> 
>  kernel/cgroup.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1162,6 +1162,8 @@ static int parse_cgroupfs_options(char *
>  			continue;
>  		}
>  		if (!strcmp(token, "clone_children")) {
> +			pr_warning("cgroup: mount option clone_children is deprecated (pid=%d comm=%s)\n",
> +				   task_tgid_nr(current), current->comm);
>  			opts->clone_children = true;
>  			continue;
>  		}
> @@ -3837,6 +3839,9 @@ fail:
>  static u64 cgroup_clone_children_read(struct cgroup *cgrp,
>  				    struct cftype *cft)
>  {
> +	printk_ratelimited(KERN_WARNING "cgroup: clone_children is deprecated (pid=%d comm=%s)\n",
> +			   task_tgid_nr(current), current->comm);
> +
>  	return clone_children(cgrp);
>  }
>  
> @@ -3844,6 +3849,9 @@ static int cgroup_clone_children_write(s
>  				     struct cftype *cft,
>  				     u64 val)
>  {
> +	printk_ratelimited(KERN_WARNING "cgroup: clone_children is deprecated (pid=%d comm=%s)\n",
> +			   task_tgid_nr(current), current->comm);
> +
>  	if (val)
>  		set_bit(CGRP_CLONE_CHILDREN, &cgrp->flags);
>  	else
> _______________________________________________
> Containers mailing list
> [e-mail]
> https://lists.linuxfoundation.org/mailman/listinfo/containers
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tejun Heo Nov. 5, 2012, 7:26 p.m. UTC | #2

Hello, Serge.

On Mon, Nov 05, 2012 at 01:17:14PM -0600, Serge Hallyn wrote:
> > As this is unnecessary feature with very limited use and awkward in
> 
> clone_children is currently required by lxc.  Of course lxc could
> manually set the .cpus and .mems in the newly created child, but if
> those interfaces or others like it ever change (i.e. any new ones which
> must be set before a task can join a cgroup) we'll get into yet more of
> a rats nets of code to support different kernel versions.

If lxc is using it, this will have to stay then.  I'll try to make it
a cpuset specific thing.

> (Just as an idea, if there was a way to generically tell from the list
> of files in my cgroups dir which files need to be initialized before a
> task can join, I think that would suffice.  Maybe a cgroups.needssetup
> file which right now contains 'cpuset.mems\ncpuset.cpus'. Then if we
> find a file we don't recognize in there we can throw an intelligent
> error, or guess at duplicating the parent value.)

Hmmm... I think the root problem is that different controllers don't
agree on the way they inherit configurations from parents.  cgroup
really is a trainwreck.  :(

I don't know whether "needssetup" is the right way to deal with it.
I'll think more about it.

> > co-mounted use cases, let's try to deprecate it.  Whine on the mount
> > option and accesses to cgroupfs knobs.
> > 
> > Signed-off-by: Tejun Heo <[e-mail]>
> > Cc: Glauber Costa <[e-mail]>
> > Cc: Peter Zijlstra <[e-mail]>
> > ---
> > Glauber, I think this is more befitting change for .post_clone().  If
> 
> What do you mean by that?  That he is working on a patch-set which will
> remove post_clone and this belongs there, or that there is a proposed
> alternative?

The former.  There was a patchset from Glauber updating ->post_clone()
(which didn't change the behavior).

Thanks.

Patch

--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1162,6 +1162,8 @@  static int parse_cgroupfs_options(char *
 			continue;
 		}
 		if (!strcmp(token, "clone_children")) {
+			pr_warning("cgroup: mount option clone_children is deprecated (pid=%d comm=%s)\n",
+				   task_tgid_nr(current), current->comm);
 			opts->clone_children = true;
 			continue;
 		}
@@ -3837,6 +3839,9 @@  fail:
 static u64 cgroup_clone_children_read(struct cgroup *cgrp,
 				    struct cftype *cft)
 {
+	printk_ratelimited(KERN_WARNING "cgroup: clone_children is deprecated (pid=%d comm=%s)\n",
+			   task_tgid_nr(current), current->comm);
+
 	return clone_children(cgrp);
 }
 
@@ -3844,6 +3849,9 @@  static int cgroup_clone_children_write(s
 				     struct cftype *cft,
 				     u64 val)
 {
+	printk_ratelimited(KERN_WARNING "cgroup: clone_children is deprecated (pid=%d comm=%s)\n",
+			   task_tgid_nr(current), current->comm);
+
 	if (val)
 		set_bit(CGRP_CLONE_CHILDREN, &cgrp->flags);
 	else