| Message ID | 1351931915-1701-9-git-send-email-tj@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
(2012/11/03 17:38), Tejun Heo wrote: > A cgroup is online and visible to iteration between ->post_create() > and ->pre_destroy(). This patch introduces CGROUP_FREEZER_ONLINE and > toggles it from the newly added freezer_post_create() and > freezer_pre_destroy() while holding freezer->lock such that a > cgroup_freezer can be reilably distinguished to be online. This will > be used by full hierarchy support. > > ONLINE test is added to freezer_apply_state() but it currently doesn't > make any difference as freezer_write() can only be called for an > online cgroup. > > Adjusting system_freezing_cnt on destruction is moved from > freezer_destroy() to the new freezer_pre_destroy() for consistency. > > This patch doesn't introduce any noticeable behavior change. > > Signed-off-by: Tejun Heo <tj@kernel.org> > --- > kernel/cgroup_freezer.c | 42 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 40 insertions(+), 2 deletions(-) > > diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c > index b8ad93c..4f12d31 100644 > --- a/kernel/cgroup_freezer.c > +++ b/kernel/cgroup_freezer.c > @@ -23,6 +23,7 @@ > #include <linux/seq_file.h> > > enum freezer_state_flags { > + CGROUP_FREEZER_ONLINE = (1 << 0), /* freezer is fully online */ Could you explain what 'online' means here again, rather than changelog ? BTW, 'online' is a shared concept, between post_create() and pre_destroy(), among developpers ? Is it new ? Anyway, the patch itself is simple. Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat 03-11-12 01:38:34, Tejun Heo wrote: > A cgroup is online and visible to iteration between ->post_create() > and ->pre_destroy(). This patch introduces CGROUP_FREEZER_ONLINE and > toggles it from the newly added freezer_post_create() and > freezer_pre_destroy() while holding freezer->lock such that a > cgroup_freezer can be reilably distinguished to be online. This will > be used by full hierarchy support. I am thinking whether freezer_pre_destroy is really needed. Once we reach pre_destroy then there are no tasks nor any children in the group so there is nobody to wake up if the group was frozen and the destroy callback is called after synchronize_rcu so the traversing should be safe. > ONLINE test is added to freezer_apply_state() but it currently doesn't > make any difference as freezer_write() can only be called for an > online cgroup. > > Adjusting system_freezing_cnt on destruction is moved from > freezer_destroy() to the new freezer_pre_destroy() for consistency. > > This patch doesn't introduce any noticeable behavior change. > > Signed-off-by: Tejun Heo <tj@kernel.org> Reviewed-by: Michal Hocko <mhocko@suse.cz> > --- > kernel/cgroup_freezer.c | 42 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 40 insertions(+), 2 deletions(-) > > diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c > index b8ad93c..4f12d31 100644 > --- a/kernel/cgroup_freezer.c > +++ b/kernel/cgroup_freezer.c > @@ -23,6 +23,7 @@ > #include <linux/seq_file.h> > > enum freezer_state_flags { > + CGROUP_FREEZER_ONLINE = (1 << 0), /* freezer is fully online */ > CGROUP_FREEZING_SELF = (1 << 1), /* this freezer is freezing */ > CGROUP_FREEZING_PARENT = (1 << 2), /* the parent freezer is freezing */ > CGROUP_FROZEN = (1 << 3), /* this and its descendants frozen */ > @@ -98,13 +99,45 @@ static struct cgroup_subsys_state *freezer_create(struct cgroup *cgroup) > return &freezer->css; > } > > -static void freezer_destroy(struct cgroup *cgroup) > +/** > + * freezer_post_create - commit creation of a freezer cgroup > + * @cgroup: cgroup being created > + * > + * We're committing to creation of @cgroup. Mark it online. > + */ > +static void freezer_post_create(struct cgroup *cgroup) > { > struct freezer *freezer = cgroup_freezer(cgroup); > > + spin_lock_irq(&freezer->lock); > + freezer->state |= CGROUP_FREEZER_ONLINE; > + spin_unlock_irq(&freezer->lock); > +} > + > +/** > + * freezer_pre_destroy - initiate destruction of @cgroup > + * @cgroup: cgroup being destroyed > + * > + * @cgroup is going away. Mark it dead and decrement system_freezing_count > + * if it was holding one. > + */ > +static void freezer_pre_destroy(struct cgroup *cgroup) > +{ > + struct freezer *freezer = cgroup_freezer(cgroup); > + > + spin_lock_irq(&freezer->lock); > + > if (freezer->state & CGROUP_FREEZING) > atomic_dec(&system_freezing_cnt); > - kfree(freezer); > + > + freezer->state = 0; > + > + spin_unlock_irq(&freezer->lock); > +} > + > +static void freezer_destroy(struct cgroup *cgroup) > +{ > + kfree(cgroup_freezer(cgroup)); > } > > /* > @@ -260,6 +293,9 @@ static void freezer_apply_state(struct freezer *freezer, bool freeze, > /* also synchronizes against task migration, see freezer_attach() */ > lockdep_assert_held(&freezer->lock); > > + if (!(freezer->state & CGROUP_FREEZER_ONLINE)) > + return; > + > if (freeze) { > if (!(freezer->state & CGROUP_FREEZING)) > atomic_inc(&system_freezing_cnt); > @@ -347,6 +383,8 @@ static struct cftype files[] = { > struct cgroup_subsys freezer_subsys = { > .name = "freezer", > .create = freezer_create, > + .post_create = freezer_post_create, > + .pre_destroy = freezer_pre_destroy, > .destroy = freezer_destroy, > .subsys_id = freezer_subsys_id, > .attach = freezer_attach, > -- > 1.7.11.7 >
Hello, Kamezawa. On Thu, Nov 08, 2012 at 01:48:25PM +0900, Kamezawa Hiroyuki wrote: > Could you explain what 'online' means here again, rather than changelog ? > BTW, 'online' is a shared concept, between post_create() and pre_destroy(), among > developpers ? Is it new ? I'm prepping a patch to rename create, post_create, pre_destroy, destroy operations to allocate, online, offline, free, so yeah the terms and concepts will be used for all of cgroup. I'll update the docs in that patch. Thanks!
Hello, On Thu, Nov 08, 2012 at 02:23:06PM +0100, Michal Hocko wrote: > On Sat 03-11-12 01:38:34, Tejun Heo wrote: > > A cgroup is online and visible to iteration between ->post_create() > > and ->pre_destroy(). This patch introduces CGROUP_FREEZER_ONLINE and > > toggles it from the newly added freezer_post_create() and > > freezer_pre_destroy() while holding freezer->lock such that a > > cgroup_freezer can be reilably distinguished to be online. This will > > be used by full hierarchy support. > > I am thinking whether freezer_pre_destroy is really needed. Once we > reach pre_destroy then there are no tasks nor any children in the group > so there is nobody to wake up if the group was frozen and the destroy > callback is called after synchronize_rcu so the traversing should be > safe. Yeah, it might be true, but I'd still like to keep the offlining in ->pre_destroy() so that it's symmetrical w/ ->post_create(). I'll rename and document the ops so that the roles of each are clearer. Thanks.
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c index b8ad93c..4f12d31 100644 --- a/kernel/cgroup_freezer.c +++ b/kernel/cgroup_freezer.c @@ -23,6 +23,7 @@ #include <linux/seq_file.h> enum freezer_state_flags { + CGROUP_FREEZER_ONLINE = (1 << 0), /* freezer is fully online */ CGROUP_FREEZING_SELF = (1 << 1), /* this freezer is freezing */ CGROUP_FREEZING_PARENT = (1 << 2), /* the parent freezer is freezing */ CGROUP_FROZEN = (1 << 3), /* this and its descendants frozen */ @@ -98,13 +99,45 @@ static struct cgroup_subsys_state *freezer_create(struct cgroup *cgroup) return &freezer->css; } -static void freezer_destroy(struct cgroup *cgroup) +/** + * freezer_post_create - commit creation of a freezer cgroup + * @cgroup: cgroup being created + * + * We're committing to creation of @cgroup. Mark it online. + */ +static void freezer_post_create(struct cgroup *cgroup) { struct freezer *freezer = cgroup_freezer(cgroup); + spin_lock_irq(&freezer->lock); + freezer->state |= CGROUP_FREEZER_ONLINE; + spin_unlock_irq(&freezer->lock); +} + +/** + * freezer_pre_destroy - initiate destruction of @cgroup + * @cgroup: cgroup being destroyed + * + * @cgroup is going away. Mark it dead and decrement system_freezing_count + * if it was holding one. + */ +static void freezer_pre_destroy(struct cgroup *cgroup) +{ + struct freezer *freezer = cgroup_freezer(cgroup); + + spin_lock_irq(&freezer->lock); + if (freezer->state & CGROUP_FREEZING) atomic_dec(&system_freezing_cnt); - kfree(freezer); + + freezer->state = 0; + + spin_unlock_irq(&freezer->lock); +} + +static void freezer_destroy(struct cgroup *cgroup) +{ + kfree(cgroup_freezer(cgroup)); } /* @@ -260,6 +293,9 @@ static void freezer_apply_state(struct freezer *freezer, bool freeze, /* also synchronizes against task migration, see freezer_attach() */ lockdep_assert_held(&freezer->lock); + if (!(freezer->state & CGROUP_FREEZER_ONLINE)) + return; + if (freeze) { if (!(freezer->state & CGROUP_FREEZING)) atomic_inc(&system_freezing_cnt); @@ -347,6 +383,8 @@ static struct cftype files[] = { struct cgroup_subsys freezer_subsys = { .name = "freezer", .create = freezer_create, + .post_create = freezer_post_create, + .pre_destroy = freezer_pre_destroy, .destroy = freezer_destroy, .subsys_id = freezer_subsys_id, .attach = freezer_attach,
A cgroup is online and visible to iteration between ->post_create() and ->pre_destroy(). This patch introduces CGROUP_FREEZER_ONLINE and toggles it from the newly added freezer_post_create() and freezer_pre_destroy() while holding freezer->lock such that a cgroup_freezer can be reilably distinguished to be online. This will be used by full hierarchy support. ONLINE test is added to freezer_apply_state() but it currently doesn't make any difference as freezer_write() can only be called for an online cgroup. Adjusting system_freezing_cnt on destruction is moved from freezer_destroy() to the new freezer_pre_destroy() for consistency. This patch doesn't introduce any noticeable behavior change. Signed-off-by: Tejun Heo <tj@kernel.org> --- kernel/cgroup_freezer.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)