* [PATCH 0/3] Simple cleanups for cgroups
@ 2011-12-11 14:45 Glauber Costa
[not found] ` <1323614738-7405-1-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
0 siblings, 1 reply; 19+ messages in thread
From: Glauber Costa @ 2011-12-11 14:45 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA,
bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A,
kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, tj-DgEjT+Ai2ygdnm+yROfE0A
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
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 19+ messages in thread[parent not found: <1323614738-7405-1-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>]
* [PATCH] make clone_children a flag [not found] ` <1323614738-7405-1-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> @ 2011-12-11 14:45 ` Glauber Costa 2011-12-11 14:48 ` Glauber Costa 2011-12-11 14:45 ` [PATCH 1/3] nitpick: make simple functions inline Glauber Costa ` (2 subsequent siblings) 3 siblings, 1 reply; 19+ messages in thread From: Glauber Costa @ 2011-12-11 14:45 UTC (permalink / raw) To: linux-kernel-u79uwXL29TY76Z2rM5mHXA Cc: jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, tj-DgEjT+Ai2ygdnm+yROfE0A, 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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> --- 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 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] make clone_children a flag 2011-12-11 14:45 ` [PATCH] make clone_children a flag Glauber Costa @ 2011-12-11 14:48 ` Glauber Costa 0 siblings, 0 replies; 19+ messages in thread From: Glauber Costa @ 2011-12-11 14:48 UTC (permalink / raw) To: Glauber Costa Cc: linux-kernel, jbottomley, cgroups, bsingharora, devel, kamezawa.hiroyu, tj 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] nitpick: make simple functions inline [not found] ` <1323614738-7405-1-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 2011-12-11 14:45 ` [PATCH] make clone_children a flag Glauber Costa @ 2011-12-11 14:45 ` Glauber Costa [not found] ` <1323614738-7405-3-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 2011-12-11 14:45 ` [PATCH 2/3] make clone_children a flag Glauber Costa 2011-12-11 14:45 ` [PATCH 3/3] make 'none' field " Glauber Costa 3 siblings, 1 reply; 19+ messages in thread From: Glauber Costa @ 2011-12-11 14:45 UTC (permalink / raw) To: linux-kernel-u79uwXL29TY76Z2rM5mHXA Cc: jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, tj-DgEjT+Ai2ygdnm+yROfE0A, 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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> --- 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 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 19+ messages in thread
[parent not found: <1323614738-7405-3-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>]
* Re: [PATCH 1/3] nitpick: make simple functions inline [not found] ` <1323614738-7405-3-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> @ 2011-12-11 18:55 ` KOSAKI Motohiro [not found] ` <4EE4FCBB.9050600-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2011-12-14 1:09 ` Li Zefan 1 sibling, 1 reply; 19+ messages in thread From: KOSAKI Motohiro @ 2011-12-11 18:55 UTC (permalink / raw) To: Glauber Costa Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, tj-DgEjT+Ai2ygdnm+yROfE0A > -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. -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <4EE4FCBB.9050600-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 1/3] nitpick: make simple functions inline [not found] ` <4EE4FCBB.9050600-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2011-12-11 20:44 ` Glauber Costa [not found] ` <4EE51646.3030900-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 0 siblings, 1 reply; 19+ messages in thread From: Glauber Costa @ 2011-12-11 20:44 UTC (permalink / raw) To: KOSAKI Motohiro Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, tj-DgEjT+Ai2ygdnm+yROfE0A 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. -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <4EE51646.3030900-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>]
* Re: [PATCH 1/3] nitpick: make simple functions inline [not found] ` <4EE51646.3030900-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> @ 2011-12-12 17:27 ` Tejun Heo 0 siblings, 0 replies; 19+ messages in thread From: Tejun Heo @ 2011-12-12 17:27 UTC (permalink / raw) To: Glauber Costa Cc: KOSAKI Motohiro, linux-kernel-u79uwXL29TY76Z2rM5mHXA, jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A 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 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] nitpick: make simple functions inline [not found] ` <1323614738-7405-3-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 2011-12-11 18:55 ` KOSAKI Motohiro @ 2011-12-14 1:09 ` Li Zefan 1 sibling, 0 replies; 19+ messages in thread From: Li Zefan @ 2011-12-14 1:09 UTC (permalink / raw) To: Glauber Costa Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, tj-DgEjT+Ai2ygdnm+yROfE0A 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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> > --- > 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); > } -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/3] make clone_children a flag [not found] ` <1323614738-7405-1-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 2011-12-11 14:45 ` [PATCH] make clone_children a flag Glauber Costa 2011-12-11 14:45 ` [PATCH 1/3] nitpick: make simple functions inline Glauber Costa @ 2011-12-11 14:45 ` Glauber Costa [not found] ` <1323614738-7405-4-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 2011-12-11 14:45 ` [PATCH 3/3] make 'none' field " Glauber Costa 3 siblings, 1 reply; 19+ messages in thread From: Glauber Costa @ 2011-12-11 14:45 UTC (permalink / raw) To: linux-kernel-u79uwXL29TY76Z2rM5mHXA Cc: jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, tj-DgEjT+Ai2ygdnm+yROfE0A, 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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> --- 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 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 19+ messages in thread
[parent not found: <1323614738-7405-4-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>]
* Re: [PATCH 2/3] make clone_children a flag [not found] ` <1323614738-7405-4-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> @ 2011-12-11 18:58 ` KOSAKI Motohiro 2011-12-13 15:39 ` Tejun Heo 1 sibling, 0 replies; 19+ messages in thread From: KOSAKI Motohiro @ 2011-12-11 18:58 UTC (permalink / raw) To: Glauber Costa Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, tj-DgEjT+Ai2ygdnm+yROfE0A (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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> > --- > kernel/cgroup.c | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) Reviewed-by: KOSAKI Motohiro <kosaki.motohiro-+CUm20s59erQFUHtdCDX3A@public.gmane.org> -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] make clone_children a flag [not found] ` <1323614738-7405-4-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 2011-12-11 18:58 ` KOSAKI Motohiro @ 2011-12-13 15:39 ` Tejun Heo [not found] ` <20111213153921.GE25802-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 1 sibling, 1 reply; 19+ messages in thread From: Tejun Heo @ 2011-12-13 15:39 UTC (permalink / raw) To: Glauber Costa Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A 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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> Doesn't this change how remount conditions are checked? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20111213153921.GE25802-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 2/3] make clone_children a flag [not found] ` <20111213153921.GE25802-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> @ 2011-12-14 2:29 ` Li Zefan [not found] ` <4EE80A0D.7090808-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> 0 siblings, 1 reply; 19+ messages in thread From: Li Zefan @ 2011-12-14 2:29 UTC (permalink / raw) To: Tejun Heo Cc: Glauber Costa, linux-kernel-u79uwXL29TY76Z2rM5mHXA, jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A 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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> > > 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 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <4EE80A0D.7090808-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>]
* Re: [PATCH 2/3] make clone_children a flag [not found] ` <4EE80A0D.7090808-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> @ 2011-12-14 7:09 ` Glauber Costa [not found] ` <4EE84B9A.90901-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 0 siblings, 1 reply; 19+ messages in thread From: Glauber Costa @ 2011-12-14 7:09 UTC (permalink / raw) To: Li Zefan Cc: Tejun Heo, linux-kernel-u79uwXL29TY76Z2rM5mHXA, jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A 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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> >> >> 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 ? -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <4EE84B9A.90901-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>]
* Re: [PATCH 2/3] make clone_children a flag [not found] ` <4EE84B9A.90901-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> @ 2011-12-14 18:18 ` Tejun Heo 2011-12-15 7:03 ` Glauber Costa 0 siblings, 1 reply; 19+ messages in thread From: Tejun Heo @ 2011-12-14 18:18 UTC (permalink / raw) To: Glauber Costa Cc: Li Zefan, linux-kernel-u79uwXL29TY76Z2rM5mHXA, jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A 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 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] make clone_children a flag 2011-12-14 18:18 ` Tejun Heo @ 2011-12-15 7:03 ` Glauber Costa 0 siblings, 0 replies; 19+ messages in thread From: Glauber Costa @ 2011-12-15 7:03 UTC (permalink / raw) To: Tejun Heo Cc: Li Zefan, linux-kernel, jbottomley, cgroups, bsingharora, devel, kamezawa.hiroyu 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] make 'none' field a flag [not found] ` <1323614738-7405-1-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> ` (2 preceding siblings ...) 2011-12-11 14:45 ` [PATCH 2/3] make clone_children a flag Glauber Costa @ 2011-12-11 14:45 ` Glauber Costa 2011-12-11 18:59 ` KOSAKI Motohiro [not found] ` <1323614738-7405-5-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 3 siblings, 2 replies; 19+ messages in thread From: Glauber Costa @ 2011-12-11 14:45 UTC (permalink / raw) To: linux-kernel-u79uwXL29TY76Z2rM5mHXA Cc: jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, tj-DgEjT+Ai2ygdnm+yROfE0A, 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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> --- 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 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] make 'none' field a flag 2011-12-11 14:45 ` [PATCH 3/3] make 'none' field " Glauber Costa @ 2011-12-11 18:59 ` KOSAKI Motohiro [not found] ` <1323614738-7405-5-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 1 sibling, 0 replies; 19+ messages in thread From: KOSAKI Motohiro @ 2011-12-11 18:59 UTC (permalink / raw) To: Glauber Costa Cc: linux-kernel, jbottomley, cgroups, bsingharora, devel, kamezawa.hiroyu, tj (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> ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <1323614738-7405-5-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>]
* Re: [PATCH 3/3] make 'none' field a flag [not found] ` <1323614738-7405-5-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> @ 2011-12-13 15:41 ` Tejun Heo [not found] ` <20111213154108.GF25802-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 19+ messages in thread From: Tejun Heo @ 2011-12-13 15:41 UTC (permalink / raw) To: Glauber Costa Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A 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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> Same as the previous patch. Doesn't this change how remount conditions are checked? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <20111213154108.GF25802-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 3/3] make 'none' field a flag [not found] ` <20111213154108.GF25802-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> @ 2011-12-14 2:09 ` Li Zefan 0 siblings, 0 replies; 19+ messages in thread From: Li Zefan @ 2011-12-14 2:09 UTC (permalink / raw) To: Tejun Heo Cc: Glauber Costa, linux-kernel-u79uwXL29TY76Z2rM5mHXA, jbottomley-bzQdu9zFT3WakBO8gow8eQ, cgroups-u79uwXL29TY76Z2rM5mHXA, bsingharora-Re5JQEeQqe8AvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A 于 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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> > > 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) -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2011-12-15 7:03 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-11 14:45 [PATCH 0/3] Simple cleanups for cgroups Glauber Costa
[not found] ` <1323614738-7405-1-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
2011-12-11 14:45 ` [PATCH] make clone_children a flag Glauber Costa
2011-12-11 14:48 ` Glauber Costa
2011-12-11 14:45 ` [PATCH 1/3] nitpick: make simple functions inline Glauber Costa
[not found] ` <1323614738-7405-3-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
2011-12-11 18:55 ` KOSAKI Motohiro
[not found] ` <4EE4FCBB.9050600-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2011-12-11 20:44 ` Glauber Costa
[not found] ` <4EE51646.3030900-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
2011-12-12 17:27 ` Tejun Heo
2011-12-14 1:09 ` Li Zefan
2011-12-11 14:45 ` [PATCH 2/3] make clone_children a flag Glauber Costa
[not found] ` <1323614738-7405-4-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
2011-12-11 18:58 ` KOSAKI Motohiro
2011-12-13 15:39 ` Tejun Heo
[not found] ` <20111213153921.GE25802-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2011-12-14 2:29 ` Li Zefan
[not found] ` <4EE80A0D.7090808-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
2011-12-14 7:09 ` Glauber Costa
[not found] ` <4EE84B9A.90901-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
2011-12-14 18:18 ` Tejun Heo
2011-12-15 7:03 ` Glauber Costa
2011-12-11 14:45 ` [PATCH 3/3] make 'none' field " Glauber Costa
2011-12-11 18:59 ` KOSAKI Motohiro
[not found] ` <1323614738-7405-5-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
2011-12-13 15:41 ` Tejun Heo
[not found] ` <20111213154108.GF25802-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2011-12-14 2:09 ` Li Zefan
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).