[PATCH 0/3] Simple cleanups for cgroups

[PATCH 0/3] Simple cleanups for cgroups

[Glauber Costa]

Hi,

While hacking on other stuff I found these spots that could
receive some simple cleanup in cgroup.c. Nothing revolutionary.
Patch 1 is rather trivial, the other 2 are more of a matter of
taste I'd say, but I believe we'd be better of this way.

Glauber Costa (3):
  nitpick: make simple functions inline
  make clone_children a flag
  make 'none' field a flag

 kernel/cgroup.c |   30 +++++++++++++++++-------------
 1 files changed, 17 insertions(+), 13 deletions(-)

-- 
1.7.6.4

[PATCH] make clone_children a flag

[Glauber Costa]

There is no reason to have a flags field, and then a separate
bool field just to indicate if the clone_children flag is set.
Make it a flag

Signed-off-by: Glauber Costa <glommer@parallels.com>
---
 kernel/cgroup.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index e4b9d3c..fa405ee 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -231,6 +231,7 @@ inline int cgroup_is_removed(const struct cgroup *cgrp)
 /* bits in struct cgroupfs_root flags field */
 enum {
 	ROOT_NOPREFIX, /* mounted subsystems have no named prefix */
+	ROOT_CLONE_CHILDREN, /* mounted subsystems starts with clone_children */
 };
 
 static int cgroup_is_releasable(const struct cgroup *cgrp)
@@ -1062,7 +1063,6 @@ struct cgroup_sb_opts {
 	unsigned long subsys_bits;
 	unsigned long flags;
 	char *release_agent;
-	bool clone_children;
 	char *name;
 	/* User explicitly requested empty subsystem */
 	bool none;
@@ -1113,7 +1113,7 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 			continue;
 		}
 		if (!strcmp(token, "clone_children")) {
-			opts->clone_children = true;
+			set_bit(ROOT_CLONE_CHILDREN, &opts->flags);
 			continue;
 		}
 		if (!strncmp(token, "release_agent=", 14)) {
@@ -1400,7 +1400,7 @@ 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)
+	if (test_bit(ROOT_CLONE_CHILDREN, &opts->flags))
 		set_bit(CGRP_CLONE_CHILDREN, &root->top_cgroup.flags);
 	return root;
 }
-- 
1.7.6.4

Re: [PATCH] make clone_children a flag

[Glauber Costa]

On 12/11/2011 03:45 PM, Glauber Costa wrote:
> There is no reason to have a flags field, and then a separate
> bool field just to indicate if the clone_children flag is set.
> Make it a flag
>
> Signed-off-by: Glauber Costa<glommer@parallels.com>

This one is repeated, I don't know how it went through. Sorry.

[PATCH 1/3] nitpick: make simple functions inline

[Glauber Costa]

Those are quite simple bit-testing functions that are
only used within this file. Not reason for them not to
be inline.

Signed-off-by: Glauber Costa <glommer@parallels.com>
---
 kernel/cgroup.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index d9d5648..e4b9d3c 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -241,12 +241,12 @@ static int cgroup_is_releasable(const struct cgroup *cgrp)
 	return (cgrp->flags & bits) == bits;
 }
 
-static int notify_on_release(const struct cgroup *cgrp)
+static inline 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)
+static inline int clone_children(const struct cgroup *cgrp)
 {
 	return test_bit(CGRP_CLONE_CHILDREN, &cgrp->flags);
 }
-- 
1.7.6.4

Re: [PATCH 1/3] nitpick: make simple functions inline

[KOSAKI Motohiro]

> -static int notify_on_release(const struct cgroup *cgrp)
> +static inline 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)
> +static inline int clone_children(const struct cgroup *cgrp)
>   {
>   	return test_bit(CGRP_CLONE_CHILDREN,&cgrp->flags);
>   }

Can you please tell us which compiler failed automatic inlining?
I suspect gcc is enough sane and we don't need this patch.

Re: [PATCH 1/3] nitpick: make simple functions inline

[Glauber Costa]

On 12/11/2011 07:55 PM, KOSAKI Motohiro wrote:
>> -static int notify_on_release(const struct cgroup *cgrp)
>> +static inline 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)
>> +static inline int clone_children(const struct cgroup *cgrp)
>>    {
>>    	return test_bit(CGRP_CLONE_CHILDREN,&cgrp->flags);
>>    }
> 
> Can you please tell us which compiler failed automatic inlining?
> I suspect gcc is enough sane and we don't need this patch.

Of course we don't need, that's the very definition of a "nitpick".
This patch is directed towards the reader, not the compiler. Maintainers
are free to take it or not, although I believe being explicit is better.

Re: [PATCH 1/3] nitpick: make simple functions inline

[Tejun Heo]

Hello,

On Sun, Dec 11, 2011 at 09:44:54PM +0100, Glauber Costa wrote:
> On 12/11/2011 07:55 PM, KOSAKI Motohiro wrote:
> > Can you please tell us which compiler failed automatic inlining?
> > I suspect gcc is enough sane and we don't need this patch.
> 
> Of course we don't need, that's the very definition of a "nitpick".
> This patch is directed towards the reader, not the compiler. Maintainers
> are free to take it or not, although I believe being explicit is better.

These days, I don't think adding inline buys us much (other than
explicit cases where always_inline or noinline is necessary).  gcc
already does good enough job for inlining and 'inline' hint seems more
to hinder rather than help and I don't really see what it buys for
code readers either, so I won't be taking this one.

Thanks.

-- 
tejun

Re: [PATCH 1/3] nitpick: make simple functions inline

[Li Zefan]

22:45, Glauber Costa wrote:
> Those are quite simple bit-testing functions that are
> only used within this file. Not reason for them not to
> be inline.
> 

It's better to leave the optimization decision to gcc.

And I've confirmed they are inlined by gcc in my box.

(btw, please add "cgroup" prefix to the patch subject line)

> Signed-off-by: Glauber Costa <glommer@parallels.com>
> ---
>  kernel/cgroup.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index d9d5648..e4b9d3c 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -241,12 +241,12 @@ static int cgroup_is_releasable(const struct cgroup *cgrp)
>  	return (cgrp->flags & bits) == bits;
>  }
>  
> -static int notify_on_release(const struct cgroup *cgrp)
> +static inline 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)
> +static inline int clone_children(const struct cgroup *cgrp)
>  {
>  	return test_bit(CGRP_CLONE_CHILDREN, &cgrp->flags);
>  }

[PATCH 2/3] make clone_children a flag

[Glauber Costa]

There is no reason to have a flags field, and then a separate
bool field just to indicate if the clone_children flag is set.
Make it a flag

Signed-off-by: Glauber Costa <glommer@parallels.com>
---
 kernel/cgroup.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index e4b9d3c..fa405ee 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -231,6 +231,7 @@ inline int cgroup_is_removed(const struct cgroup *cgrp)
 /* bits in struct cgroupfs_root flags field */
 enum {
 	ROOT_NOPREFIX, /* mounted subsystems have no named prefix */
+	ROOT_CLONE_CHILDREN, /* mounted subsystems starts with clone_children */
 };
 
 static int cgroup_is_releasable(const struct cgroup *cgrp)
@@ -1062,7 +1063,6 @@ struct cgroup_sb_opts {
 	unsigned long subsys_bits;
 	unsigned long flags;
 	char *release_agent;
-	bool clone_children;
 	char *name;
 	/* User explicitly requested empty subsystem */
 	bool none;
@@ -1113,7 +1113,7 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 			continue;
 		}
 		if (!strcmp(token, "clone_children")) {
-			opts->clone_children = true;
+			set_bit(ROOT_CLONE_CHILDREN, &opts->flags);
 			continue;
 		}
 		if (!strncmp(token, "release_agent=", 14)) {
@@ -1400,7 +1400,7 @@ 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)
+	if (test_bit(ROOT_CLONE_CHILDREN, &opts->flags))
 		set_bit(CGRP_CLONE_CHILDREN, &root->top_cgroup.flags);
 	return root;
 }
-- 
1.7.6.4

Re: [PATCH 2/3] make clone_children a flag

[KOSAKI Motohiro]

(12/11/11 9:45 AM), Glauber Costa wrote:
> There is no reason to have a flags field, and then a separate
> bool field just to indicate if the clone_children flag is set.
> Make it a flag
> 
> Signed-off-by: Glauber Costa<glommer@parallels.com>
> ---
>   kernel/cgroup.c |    6 +++---
>   1 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH 2/3] make clone_children a flag

[Tejun Heo]

On Sun, Dec 11, 2011 at 03:45:37PM +0100, Glauber Costa wrote:
> There is no reason to have a flags field, and then a separate
> bool field just to indicate if the clone_children flag is set.
> Make it a flag
> 
> Signed-off-by: Glauber Costa <glommer@parallels.com>

Doesn't this change how remount conditions are checked?

Thanks.

-- 
tejun

Re: [PATCH 2/3] make clone_children a flag

[Li Zefan]

Tejun Heo wrote:
> On Sun, Dec 11, 2011 at 03:45:37PM +0100, Glauber Costa wrote:
>> There is no reason to have a flags field, and then a separate
>> bool field just to indicate if the clone_children flag is set.
>> Make it a flag
>>
>> Signed-off-by: Glauber Costa <glommer@parallels.com>
> 
> Doesn't this change how remount conditions are checked?
> 

Right. Currently we can do this:

	# mount -t cgroup xxx /mnt
	# mount -o remount,clone_children /mnt

with this patch, the above remount will fail.

But..the current bevaiour of remount is a bit confusing in that remount
with/without "clone_children" has no effect on anything:

	# mount -t cgroup -o clone_children xxx /mnt
	# cat /mnt/cgroup.clone_children
	1
	# mount -o remount xxx /mnt
	# mount | grep cgroup
	xxx on /mnt type cgroup (rw,clone_children)
	# cat /mnt/cgroup.clone_children
	1

Re: [PATCH 2/3] make clone_children a flag

[Glauber Costa]

On 12/14/2011 06:29 AM, Li Zefan wrote:
> Tejun Heo wrote:
>> On Sun, Dec 11, 2011 at 03:45:37PM +0100, Glauber Costa wrote:
>>> There is no reason to have a flags field, and then a separate
>>> bool field just to indicate if the clone_children flag is set.
>>> Make it a flag
>>>
>>> Signed-off-by: Glauber Costa<glommer@parallels.com>
>>
>> Doesn't this change how remount conditions are checked?
>>

Well, I was thinking it wouldn't, because I patched all callers. But I 
forget life is not always that simple: After you mentioned, I checked 
and we do test for changes in the flag field explicitly on remount. So I 
missed that, indeed.

> Right. Currently we can do this:
>
> 	# mount -t cgroup xxx /mnt
> 	# mount -o remount,clone_children /mnt
>
> with this patch, the above remount will fail.
>
> But..the current bevaiour of remount is a bit confusing in that remount
> with/without "clone_children" has no effect on anything:
>
> 	# mount -t cgroup -o clone_children xxx /mnt
> 	# cat /mnt/cgroup.clone_children
> 	1
> 	# mount -o remount xxx /mnt
> 	# mount | grep cgroup
> 	xxx on /mnt type cgroup (rw,clone_children)
> 	# cat /mnt/cgroup.clone_children
> 	1

That's indeed confusing, and it comes from the fact that we always 
inherit clone_children from the parent - which is sane, IMHO. So this 
flag only has any value in establishing the initial behaviour of the top 
root cgroup. I wonder then if it wouldn't better to just be explicit and 
fail in this case ?

Re: [PATCH 2/3] make clone_children a flag

[Tejun Heo]

Hello,

On Wed, Dec 14, 2011 at 11:09:14AM +0400, Glauber Costa wrote:
> That's indeed confusing, and it comes from the fact that we always
> inherit clone_children from the parent - which is sane, IMHO. So
> this flag only has any value in establishing the initial behaviour
> of the top root cgroup. I wonder then if it wouldn't better to just
> be explicit and fail in this case ?

I don't think all current behaviors are sane and if not let's change
them, but those things have to be explicit with proper description and
rationale.

Thanks.

-- 
tejun

Re: [PATCH 2/3] make clone_children a flag

[Glauber Costa]

On 12/14/2011 10:18 PM, Tejun Heo wrote:
> Hello,
>
> On Wed, Dec 14, 2011 at 11:09:14AM +0400, Glauber Costa wrote:
>> That's indeed confusing, and it comes from the fact that we always
>> inherit clone_children from the parent - which is sane, IMHO. So
>> this flag only has any value in establishing the initial behaviour
>> of the top root cgroup. I wonder then if it wouldn't better to just
>> be explicit and fail in this case ?
>
> I don't think all current behaviors are sane and if not let's change
> them, but those things have to be explicit with proper description and
> rationale.
>

140 % agree to that. As I said, I wrongly believed it to be functionally 
equivalent when I sent it, but missed the flags remount check.

If you believe the behavior we now get is saner, I can rewrite the 
Changelog and resend it.

[PATCH 3/3] make 'none' field a flag

[Glauber Costa]

There is no reason to have a flags field, and then a separate
bool field just to indicate if 'none' subsystem were explicitly
requested.

Make it a flag

Signed-off-by: Glauber Costa <glommer@parallels.com>
---
 kernel/cgroup.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index fa405ee..e700abe 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -232,6 +232,7 @@ inline int cgroup_is_removed(const struct cgroup *cgrp)
 enum {
 	ROOT_NOPREFIX, /* mounted subsystems have no named prefix */
 	ROOT_CLONE_CHILDREN, /* mounted subsystems starts with clone_children */
+	ROOT_NOSUBSYS, /* explicitly asked for 'none' subsystems */
 };
 
 static int cgroup_is_releasable(const struct cgroup *cgrp)
@@ -1064,13 +1065,16 @@ struct cgroup_sb_opts {
 	unsigned long flags;
 	char *release_agent;
 	char *name;
-	/* User explicitly requested empty subsystem */
-	bool none;
 
 	struct cgroupfs_root *new_root;
 
 };
 
+static inline int opts_no_subsys(const struct cgroup_sb_opts *opts)
+{
+	return test_bit(ROOT_NOSUBSYS, &opts->flags);
+}
+
 /*
  * Convert a hierarchy specifier into a bitmask of subsystems and flags. Call
  * with cgroup_mutex held to protect the subsys[] array. This function takes
@@ -1098,7 +1102,7 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 			return -EINVAL;
 		if (!strcmp(token, "none")) {
 			/* Explicitly have no subsystems */
-			opts->none = true;
+			set_bit(ROOT_NOSUBSYS, &opts->flags);
 			continue;
 		}
 		if (!strcmp(token, "all")) {
@@ -1178,7 +1182,7 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 	 * otherwise 'all, 'none' and a subsystem name options were not
 	 * specified, let's default to 'all'
 	 */
-	if (all_ss || (!all_ss && !one_ss && !opts->none)) {
+	if (all_ss || (!all_ss && !one_ss && !opts_no_subsys(opts))) {
 		for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 			struct cgroup_subsys *ss = subsys[i];
 			if (ss == NULL)
@@ -1202,7 +1206,7 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
 
 
 	/* Can't specify "none" and some subsystems */
-	if (opts->subsys_bits && opts->none)
+	if (opts->subsys_bits && opts_no_subsys(opts))
 		return -EINVAL;
 
 	/*
@@ -1370,7 +1374,7 @@ static int cgroup_test_super(struct super_block *sb, void *data)
 	 * If we asked for subsystems (or explicitly for no
 	 * subsystems) then they must match
 	 */
-	if ((opts->subsys_bits || opts->none)
+	if ((opts->subsys_bits || opts_no_subsys(opts))
 	    && (opts->subsys_bits != root->subsys_bits))
 		return 0;
 
@@ -1381,7 +1385,7 @@ static struct cgroupfs_root *cgroup_root_from_opts(struct cgroup_sb_opts *opts)
 {
 	struct cgroupfs_root *root;
 
-	if (!opts->subsys_bits && !opts->none)
+	if (!opts->subsys_bits && !opts_no_subsys(opts))
 		return NULL;
 
 	root = kzalloc(sizeof(*root), GFP_KERNEL);
@@ -1426,7 +1430,7 @@ static int cgroup_set_super(struct super_block *sb, void *data)
 	if (!opts->new_root)
 		return -EINVAL;
 
-	BUG_ON(!opts->subsys_bits && !opts->none);
+	BUG_ON(!opts->subsys_bits && !opts_no_subsys(opts));
 
 	ret = set_anon_super(sb, NULL);
 	if (ret)
-- 
1.7.6.4

Re: [PATCH 3/3] make 'none' field a flag

[KOSAKI Motohiro]

(12/11/11 9:45 AM), Glauber Costa wrote:
> There is no reason to have a flags field, and then a separate
> bool field just to indicate if 'none' subsystem were explicitly
> requested.
> 
> Make it a flag
> 
> Signed-off-by: Glauber Costa<glommer@parallels.com>

Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH 3/3] make 'none' field a flag

[Tejun Heo]

On Sun, Dec 11, 2011 at 03:45:38PM +0100, Glauber Costa wrote:
> There is no reason to have a flags field, and then a separate
> bool field just to indicate if 'none' subsystem were explicitly
> requested.
> 
> Make it a flag
> 
> Signed-off-by: Glauber Costa <glommer@parallels.com>

Same as the previous patch.  Doesn't this change how remount
conditions are checked?

Thanks.

-- 
tejun

Re: [PATCH 3/3] make 'none' field a flag

[Li Zefan]

于 2011年12月13日 23:41, Tejun Heo 写道:
> On Sun, Dec 11, 2011 at 03:45:38PM +0100, Glauber Costa wrote:
>> There is no reason to have a flags field, and then a separate
>> bool field just to indicate if 'none' subsystem were explicitly
>> requested.
>>
>> Make it a flag
>>
>> Signed-off-by: Glauber Costa <glommer@parallels.com>
> 
> Same as the previous patch.  Doesn't this change how remount
> conditions are checked?
> 

Right. The patch prevents us from doing:

	# mount -t cgroup -o none,name=tmp xxx /mnt
	# mount -o remount,cpuset xxx /mnt
	(failed)