| Message ID | 1351931915-1701-7-git-send-email-tj@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
(2012/11/03 17:38), Tejun Heo wrote: > freezer->state was an enum value - one of THAWED, FREEZING and FROZEN. > As the scheduled full hierarchy support requires more than one > freezing condition, switch it to mask of flags. If FREEZING is not > set, it's thawed. FREEZING is set if freezing or frozen. If frozen, > both FREEZING and FROZEN are set. Now that tasks can be attached to > an already frozen cgroup, this also makes freezing condition checks > more natural. > > This patch doesn't introduce any behavior change. > > Signed-off-by: Tejun Heo <tj@kernel.org> > --- > kernel/cgroup_freezer.c | 60 ++++++++++++++++++++++--------------------------- > 1 file changed, 27 insertions(+), 33 deletions(-) > > diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c > index 2690830..e76aa9f 100644 > --- a/kernel/cgroup_freezer.c > +++ b/kernel/cgroup_freezer.c > @@ -22,15 +22,14 @@ > #include <linux/freezer.h> > #include <linux/seq_file.h> > > -enum freezer_state { > - CGROUP_THAWED = 0, > - CGROUP_FREEZING, > - CGROUP_FROZEN, > +enum freezer_state_flags { > + CGROUP_FREEZING = (1 << 1), /* this freezer is freezing */ > + CGROUP_FROZEN = (1 << 3), /* this and its descendants frozen */ > }; > > struct freezer { > struct cgroup_subsys_state css; > - enum freezer_state state; > + unsigned int state; > spinlock_t lock; > }; > > @@ -48,12 +47,10 @@ static inline struct freezer *task_freezer(struct task_struct *task) > > bool cgroup_freezing(struct task_struct *task) > { > - enum freezer_state state; > bool ret; > > rcu_read_lock(); > - state = task_freezer(task)->state; > - ret = state == CGROUP_FREEZING || state == CGROUP_FROZEN; > + ret = task_freezer(task)->state & CGROUP_FREEZING; > rcu_read_unlock(); > > return ret; > @@ -63,10 +60,13 @@ bool cgroup_freezing(struct task_struct *task) > * cgroups_write_string() limits the size of freezer state strings to > * CGROUP_LOCAL_BUFFER_SIZE > */ > -static const char *freezer_state_strs[] = { > - "THAWED", > - "FREEZING", > - "FROZEN", > +static const char *freezer_state_strs(unsigned int state) > +{ > + if (state & CGROUP_FROZEN) > + return "FROZEN"; > + if (state & CGROUP_FREEZING) > + return "FREEZING"; > + return "THAWED"; > }; > > /* > @@ -91,7 +91,6 @@ static struct cgroup_subsys_state *freezer_create(struct cgroup *cgroup) > return ERR_PTR(-ENOMEM); > > spin_lock_init(&freezer->lock); > - freezer->state = CGROUP_THAWED; > return &freezer->css; > } > > @@ -99,7 +98,7 @@ static void freezer_destroy(struct cgroup *cgroup) > { > struct freezer *freezer = cgroup_freezer(cgroup); > > - if (freezer->state != CGROUP_THAWED) > + if (freezer->state & CGROUP_FREEZING) > atomic_dec(&system_freezing_cnt); > kfree(freezer); > } > @@ -129,15 +128,13 @@ static void freezer_attach(struct cgroup *new_cgrp, struct cgroup_taskset *tset) > * Tasks in @tset are on @new_cgrp but may not conform to its > * current state before executing the following - !frozen tasks may > * be visible in a FROZEN cgroup and frozen tasks in a THAWED one. > - * This means that, to determine whether to freeze, one should test > - * whether the state equals THAWED. > */ > cgroup_taskset_for_each(task, new_cgrp, tset) { > - if (freezer->state == CGROUP_THAWED) { > + if (!(freezer->state & CGROUP_FREEZING)) { > __thaw_task(task); > } else { > freeze_task(task); > - freezer->state = CGROUP_FREEZING; > + freezer->state &= ~CGROUP_FROZEN; > } > } > > @@ -159,11 +156,7 @@ static void freezer_fork(struct task_struct *task) > goto out; > > spin_lock_irq(&freezer->lock); > - /* > - * @task might have been just migrated into a FROZEN cgroup. Test > - * equality with THAWED. Read the comment in freezer_attach(). > - */ > - if (freezer->state != CGROUP_THAWED) > + if (freezer->state & CGROUP_FREEZING) > freeze_task(task); > spin_unlock_irq(&freezer->lock); > out: > @@ -184,7 +177,8 @@ static void update_if_frozen(struct freezer *freezer) > struct cgroup_iter it; > struct task_struct *task; > > - if (freezer->state != CGROUP_FREEZING) > + if (!(freezer->state & CGROUP_FREEZING) || > + (freezer->state & CGROUP_FROZEN)) > return; Hmm.... How about enum { __CGROUP_FREEZING, __CGROUP_FROZEN, }; #define CGROUP_FREEZER_STATE_MASK 0x3 #define CGROUP_FREEZER_STATE(state) ((state) & CGROUP_FREEZER_STATE_MASK) #define CGROUP_THAW(state) (CGROUP_FREEZER_STATE(state) == 0) #define CGROUP_FREEZING(state) (CGROUP_FREEZER_STATE(state) == __CGROUP_FREEZING) #define CGROUP_FROZEN(state)\ (CGROUP_FREEZER_STATE(state) == (__CGROUP_FREEZING | __CGROUP_FROZEN)) > > cgroup_iter_start(cgroup, &it); > @@ -202,7 +196,7 @@ static void update_if_frozen(struct freezer *freezer) > } > } > > - freezer->state = CGROUP_FROZEN; > + freezer->state |= CGROUP_FROZEN; > notyet: > cgroup_iter_end(cgroup, &it); > } > @@ -211,14 +205,14 @@ static int freezer_read(struct cgroup *cgroup, struct cftype *cft, > struct seq_file *m) > { > struct freezer *freezer = cgroup_freezer(cgroup); > - enum freezer_state state; > + unsigned int state; > > spin_lock_irq(&freezer->lock); > update_if_frozen(freezer); > state = freezer->state; > spin_unlock_irq(&freezer->lock); > > - seq_puts(m, freezer_state_strs[state]); > + seq_puts(m, freezer_state_strs(state)); > seq_putc(m, '\n'); > return 0; > } > @@ -258,14 +252,14 @@ static void freezer_apply_state(struct freezer *freezer, bool freeze) > lockdep_assert_held(&freezer->lock); > > if (freeze) { > - if (freezer->state == CGROUP_THAWED) > + if (!(freezer->state & CGROUP_FREEZING)) > atomic_inc(&system_freezing_cnt); > - freezer->state = CGROUP_FREEZING; > + freezer->state |= CGROUP_FREEZING; > freeze_cgroup(freezer); > } else { > - if (freezer->state != CGROUP_THAWED) > + if (freezer->state & CGROUP_FREEZING) > atomic_dec(&system_freezing_cnt); > - freezer->state = CGROUP_THAWED; > + freezer->state &= ~(CGROUP_FREEZING | CGROUP_FROZEN); > unfreeze_cgroup(freezer); > } > } > @@ -290,9 +284,9 @@ static int freezer_write(struct cgroup *cgroup, struct cftype *cft, > { > bool freeze; > > - if (strcmp(buffer, freezer_state_strs[CGROUP_THAWED]) == 0) > + if (strcmp(buffer, freezer_state_strs(0)) == 0) Can't we have a name for "0" ? > freeze = false; > - else if (strcmp(buffer, freezer_state_strs[CGROUP_FROZEN]) == 0) > + else if (strcmp(buffer, freezer_state_strs(CGROUP_FROZEN)) == 0) > freeze = true; > else > return -EINVAL; > Thanks, -Kame -- 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
Hello, Kame. On Thu, Nov 08, 2012 at 01:37:50PM +0900, Kamezawa Hiroyuki wrote: > How about > enum { > __CGROUP_FREEZING, > __CGROUP_FROZEN, > }; > > #define CGROUP_FREEZER_STATE_MASK 0x3 > #define CGROUP_FREEZER_STATE(state) ((state) & CGROUP_FREEZER_STATE_MASK) > #define CGROUP_THAW(state) (CGROUP_FREEZER_STATE(state) == 0) > #define CGROUP_FREEZING(state) (CGROUP_FREEZER_STATE(state) == __CGROUP_FREEZING) > #define CGROUP_FROZEN(state)\ > (CGROUP_FREEZER_STATE(state) == (__CGROUP_FREEZING | __CGROUP_FROZEN)) I think it's a bit overdone and we have cases where we test for FREEZING regardless of FROZEN and cases where test for FREEZING && !FROZEN. We can have, say, CGROUP_FREZING() and then CGROUP_FREEZING_BUT_NOT_FROZEN(), but it feels more like obfuscation than anything else. > >@@ -290,9 +284,9 @@ static int freezer_write(struct cgroup *cgroup, struct cftype *cft, > > { > > bool freeze; > > > >- if (strcmp(buffer, freezer_state_strs[CGROUP_THAWED]) == 0) > >+ if (strcmp(buffer, freezer_state_strs(0)) == 0) > > Can't we have a name for "0" ? We can define CGROUP_THAWED to be zero. I'd rather keep it explicitly zero tho. freezer state is mask of freezing and frozen flags and no flag set means thawed. Thanks.
(2012/11/08 13:42), Tejun Heo wrote: > Hello, Kame. > > On Thu, Nov 08, 2012 at 01:37:50PM +0900, Kamezawa Hiroyuki wrote: >> How about >> enum { >> __CGROUP_FREEZING, >> __CGROUP_FROZEN, >> }; >> >> #define CGROUP_FREEZER_STATE_MASK 0x3 >> #define CGROUP_FREEZER_STATE(state) ((state) & CGROUP_FREEZER_STATE_MASK) >> #define CGROUP_THAW(state) (CGROUP_FREEZER_STATE(state) == 0) >> #define CGROUP_FREEZING(state) (CGROUP_FREEZER_STATE(state) == __CGROUP_FREEZING) >> #define CGROUP_FROZEN(state)\ >> (CGROUP_FREEZER_STATE(state) == (__CGROUP_FREEZING | __CGROUP_FROZEN)) > > I think it's a bit overdone and we have cases where we test for > FREEZING regardless of FROZEN and cases where test for FREEZING && > !FROZEN. We can have, say, CGROUP_FREZING() and then > CGROUP_FREEZING_BUT_NOT_FROZEN(), but it feels more like obfuscation > than anything else. > Hm, then, I'm glad if I can see what combinations of flags are valid and meanings of them in source code comments. Anyway, 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:32, Tejun Heo wrote: > freezer->state was an enum value - one of THAWED, FREEZING and FROZEN. > As the scheduled full hierarchy support requires more than one > freezing condition, switch it to mask of flags. If FREEZING is not > set, it's thawed. FREEZING is set if freezing or frozen. If frozen, > both FREEZING and FROZEN are set. Now that tasks can be attached to > an already frozen cgroup, this also makes freezing condition checks > more natural. > > This patch doesn't introduce any behavior change. > > Signed-off-by: Tejun Heo <tj@kernel.org> I think it would be nicer to use freezer_state_flags enum rather than unsigned int for the state. I would even expect gcc to complain about that but it looks like -fstrict-enums is c++ specific (so long enum safety). Anyway Reviewed-by: Michal Hocko <mhocko@suse.cz> > --- > kernel/cgroup_freezer.c | 60 ++++++++++++++++++++++--------------------------- > 1 file changed, 27 insertions(+), 33 deletions(-) > > diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c > index 2690830..e76aa9f 100644 > --- a/kernel/cgroup_freezer.c > +++ b/kernel/cgroup_freezer.c > @@ -22,15 +22,14 @@ > #include <linux/freezer.h> > #include <linux/seq_file.h> > > -enum freezer_state { > - CGROUP_THAWED = 0, > - CGROUP_FREEZING, > - CGROUP_FROZEN, > +enum freezer_state_flags { > + CGROUP_FREEZING = (1 << 1), /* this freezer is freezing */ > + CGROUP_FROZEN = (1 << 3), /* this and its descendants frozen */ > }; > > struct freezer { > struct cgroup_subsys_state css; > - enum freezer_state state; > + unsigned int state; > spinlock_t lock; > }; > > @@ -48,12 +47,10 @@ static inline struct freezer *task_freezer(struct task_struct *task) > > bool cgroup_freezing(struct task_struct *task) > { > - enum freezer_state state; > bool ret; > > rcu_read_lock(); > - state = task_freezer(task)->state; > - ret = state == CGROUP_FREEZING || state == CGROUP_FROZEN; > + ret = task_freezer(task)->state & CGROUP_FREEZING; > rcu_read_unlock(); > > return ret; > @@ -63,10 +60,13 @@ bool cgroup_freezing(struct task_struct *task) > * cgroups_write_string() limits the size of freezer state strings to > * CGROUP_LOCAL_BUFFER_SIZE > */ > -static const char *freezer_state_strs[] = { > - "THAWED", > - "FREEZING", > - "FROZEN", > +static const char *freezer_state_strs(unsigned int state) > +{ > + if (state & CGROUP_FROZEN) > + return "FROZEN"; > + if (state & CGROUP_FREEZING) > + return "FREEZING"; > + return "THAWED"; > }; > > /* > @@ -91,7 +91,6 @@ static struct cgroup_subsys_state *freezer_create(struct cgroup *cgroup) > return ERR_PTR(-ENOMEM); > > spin_lock_init(&freezer->lock); > - freezer->state = CGROUP_THAWED; > return &freezer->css; > } > > @@ -99,7 +98,7 @@ static void freezer_destroy(struct cgroup *cgroup) > { > struct freezer *freezer = cgroup_freezer(cgroup); > > - if (freezer->state != CGROUP_THAWED) > + if (freezer->state & CGROUP_FREEZING) > atomic_dec(&system_freezing_cnt); > kfree(freezer); > } > @@ -129,15 +128,13 @@ static void freezer_attach(struct cgroup *new_cgrp, struct cgroup_taskset *tset) > * Tasks in @tset are on @new_cgrp but may not conform to its > * current state before executing the following - !frozen tasks may > * be visible in a FROZEN cgroup and frozen tasks in a THAWED one. > - * This means that, to determine whether to freeze, one should test > - * whether the state equals THAWED. > */ > cgroup_taskset_for_each(task, new_cgrp, tset) { > - if (freezer->state == CGROUP_THAWED) { > + if (!(freezer->state & CGROUP_FREEZING)) { > __thaw_task(task); > } else { > freeze_task(task); > - freezer->state = CGROUP_FREEZING; > + freezer->state &= ~CGROUP_FROZEN; > } > } > > @@ -159,11 +156,7 @@ static void freezer_fork(struct task_struct *task) > goto out; > > spin_lock_irq(&freezer->lock); > - /* > - * @task might have been just migrated into a FROZEN cgroup. Test > - * equality with THAWED. Read the comment in freezer_attach(). > - */ > - if (freezer->state != CGROUP_THAWED) > + if (freezer->state & CGROUP_FREEZING) > freeze_task(task); > spin_unlock_irq(&freezer->lock); > out: > @@ -184,7 +177,8 @@ static void update_if_frozen(struct freezer *freezer) > struct cgroup_iter it; > struct task_struct *task; > > - if (freezer->state != CGROUP_FREEZING) > + if (!(freezer->state & CGROUP_FREEZING) || > + (freezer->state & CGROUP_FROZEN)) > return; > > cgroup_iter_start(cgroup, &it); > @@ -202,7 +196,7 @@ static void update_if_frozen(struct freezer *freezer) > } > } > > - freezer->state = CGROUP_FROZEN; > + freezer->state |= CGROUP_FROZEN; > notyet: > cgroup_iter_end(cgroup, &it); > } > @@ -211,14 +205,14 @@ static int freezer_read(struct cgroup *cgroup, struct cftype *cft, > struct seq_file *m) > { > struct freezer *freezer = cgroup_freezer(cgroup); > - enum freezer_state state; > + unsigned int state; > > spin_lock_irq(&freezer->lock); > update_if_frozen(freezer); > state = freezer->state; > spin_unlock_irq(&freezer->lock); > > - seq_puts(m, freezer_state_strs[state]); > + seq_puts(m, freezer_state_strs(state)); > seq_putc(m, '\n'); > return 0; > } > @@ -258,14 +252,14 @@ static void freezer_apply_state(struct freezer *freezer, bool freeze) > lockdep_assert_held(&freezer->lock); > > if (freeze) { > - if (freezer->state == CGROUP_THAWED) > + if (!(freezer->state & CGROUP_FREEZING)) > atomic_inc(&system_freezing_cnt); > - freezer->state = CGROUP_FREEZING; > + freezer->state |= CGROUP_FREEZING; > freeze_cgroup(freezer); > } else { > - if (freezer->state != CGROUP_THAWED) > + if (freezer->state & CGROUP_FREEZING) > atomic_dec(&system_freezing_cnt); > - freezer->state = CGROUP_THAWED; > + freezer->state &= ~(CGROUP_FREEZING | CGROUP_FROZEN); > unfreeze_cgroup(freezer); > } > } > @@ -290,9 +284,9 @@ static int freezer_write(struct cgroup *cgroup, struct cftype *cft, > { > bool freeze; > > - if (strcmp(buffer, freezer_state_strs[CGROUP_THAWED]) == 0) > + if (strcmp(buffer, freezer_state_strs(0)) == 0) > freeze = false; > - else if (strcmp(buffer, freezer_state_strs[CGROUP_FROZEN]) == 0) > + else if (strcmp(buffer, freezer_state_strs(CGROUP_FROZEN)) == 0) > freeze = true; > else > return -EINVAL; > -- > 1.7.11.7 >
Hello, Kamezawa. On Thu, Nov 08, 2012 at 02:00:34PM +0900, Kamezawa Hiroyuki wrote: > (2012/11/08 13:42), Tejun Heo wrote: > >Hello, Kame. > > > >On Thu, Nov 08, 2012 at 01:37:50PM +0900, Kamezawa Hiroyuki wrote: > >>How about > >>enum { > >> __CGROUP_FREEZING, > >> __CGROUP_FROZEN, > >>}; > >> > >>#define CGROUP_FREEZER_STATE_MASK 0x3 > >>#define CGROUP_FREEZER_STATE(state) ((state) & CGROUP_FREEZER_STATE_MASK) > >>#define CGROUP_THAW(state) (CGROUP_FREEZER_STATE(state) == 0) > >>#define CGROUP_FREEZING(state) (CGROUP_FREEZER_STATE(state) == __CGROUP_FREEZING) > >>#define CGROUP_FROZEN(state)\ > >> (CGROUP_FREEZER_STATE(state) == (__CGROUP_FREEZING | __CGROUP_FROZEN)) > > > >I think it's a bit overdone and we have cases where we test for > >FREEZING regardless of FROZEN and cases where test for FREEZING && > >!FROZEN. We can have, say, CGROUP_FREZING() and then > >CGROUP_FREEZING_BUT_NOT_FROZEN(), but it feels more like obfuscation > >than anything else. > > > > Hm, then, I'm glad if I can see what combinations of flags are valid and > meanings of them in source code comments. Ooh, the following comment is added by a later patch when the whole thing is complete. /* * A cgroup is freezing if any FREEZING flags are set. FREEZING_SELF is * set if "FROZEN" is written to freezer.state cgroupfs file, and cleared * for "THAWED". FREEZING_PARENT is set if the parent freezer is FREEZING * for whatever reason. IOW, a cgroup has FREEZING_PARENT set if one of * its ancestors has FREEZING_SELF set. */ Thanks.
Hello, Michal. On Thu, Nov 08, 2012 at 11:39:28AM +0100, Michal Hocko wrote: > On Sat 03-11-12 01:38:32, Tejun Heo wrote: > > freezer->state was an enum value - one of THAWED, FREEZING and FROZEN. > > As the scheduled full hierarchy support requires more than one > > freezing condition, switch it to mask of flags. If FREEZING is not > > set, it's thawed. FREEZING is set if freezing or frozen. If frozen, > > both FREEZING and FROZEN are set. Now that tasks can be attached to > > an already frozen cgroup, this also makes freezing condition checks > > more natural. > > > > This patch doesn't introduce any behavior change. > > > > Signed-off-by: Tejun Heo <tj@kernel.org> > > I think it would be nicer to use freezer_state_flags enum rather than > unsigned int for the state. I would even expect gcc to complain about > that but it looks like -fstrict-enums is c++ specific (so long enum > safety). But if you use it as flag bits, the resulting value no longer is inside the defined enum values. Isn't that weird? Thanks.
On Thu 08-11-12 06:39:52, Tejun Heo wrote: > Hello, Michal. > > On Thu, Nov 08, 2012 at 11:39:28AM +0100, Michal Hocko wrote: > > On Sat 03-11-12 01:38:32, Tejun Heo wrote: > > > freezer->state was an enum value - one of THAWED, FREEZING and FROZEN. > > > As the scheduled full hierarchy support requires more than one > > > freezing condition, switch it to mask of flags. If FREEZING is not > > > set, it's thawed. FREEZING is set if freezing or frozen. If frozen, > > > both FREEZING and FROZEN are set. Now that tasks can be attached to > > > an already frozen cgroup, this also makes freezing condition checks > > > more natural. > > > > > > This patch doesn't introduce any behavior change. > > > > > > Signed-off-by: Tejun Heo <tj@kernel.org> > > > > I think it would be nicer to use freezer_state_flags enum rather than > > unsigned int for the state. I would even expect gcc to complain about > > that but it looks like -fstrict-enums is c++ specific (so long enum > > safety). > > But if you use it as flag bits, the resulting value no longer is > inside the defined enum values. Isn't that weird? Ahh, there is always CGROUP_FREEZER_ONLINE which would overcomplicate the possible and valid masks. Please ignore this...
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c index 2690830..e76aa9f 100644 --- a/kernel/cgroup_freezer.c +++ b/kernel/cgroup_freezer.c @@ -22,15 +22,14 @@ #include <linux/freezer.h> #include <linux/seq_file.h> -enum freezer_state { - CGROUP_THAWED = 0, - CGROUP_FREEZING, - CGROUP_FROZEN, +enum freezer_state_flags { + CGROUP_FREEZING = (1 << 1), /* this freezer is freezing */ + CGROUP_FROZEN = (1 << 3), /* this and its descendants frozen */ }; struct freezer { struct cgroup_subsys_state css; - enum freezer_state state; + unsigned int state; spinlock_t lock; }; @@ -48,12 +47,10 @@ static inline struct freezer *task_freezer(struct task_struct *task) bool cgroup_freezing(struct task_struct *task) { - enum freezer_state state; bool ret; rcu_read_lock(); - state = task_freezer(task)->state; - ret = state == CGROUP_FREEZING || state == CGROUP_FROZEN; + ret = task_freezer(task)->state & CGROUP_FREEZING; rcu_read_unlock(); return ret; @@ -63,10 +60,13 @@ bool cgroup_freezing(struct task_struct *task) * cgroups_write_string() limits the size of freezer state strings to * CGROUP_LOCAL_BUFFER_SIZE */ -static const char *freezer_state_strs[] = { - "THAWED", - "FREEZING", - "FROZEN", +static const char *freezer_state_strs(unsigned int state) +{ + if (state & CGROUP_FROZEN) + return "FROZEN"; + if (state & CGROUP_FREEZING) + return "FREEZING"; + return "THAWED"; }; /* @@ -91,7 +91,6 @@ static struct cgroup_subsys_state *freezer_create(struct cgroup *cgroup) return ERR_PTR(-ENOMEM); spin_lock_init(&freezer->lock); - freezer->state = CGROUP_THAWED; return &freezer->css; } @@ -99,7 +98,7 @@ static void freezer_destroy(struct cgroup *cgroup) { struct freezer *freezer = cgroup_freezer(cgroup); - if (freezer->state != CGROUP_THAWED) + if (freezer->state & CGROUP_FREEZING) atomic_dec(&system_freezing_cnt); kfree(freezer); } @@ -129,15 +128,13 @@ static void freezer_attach(struct cgroup *new_cgrp, struct cgroup_taskset *tset) * Tasks in @tset are on @new_cgrp but may not conform to its * current state before executing the following - !frozen tasks may * be visible in a FROZEN cgroup and frozen tasks in a THAWED one. - * This means that, to determine whether to freeze, one should test - * whether the state equals THAWED. */ cgroup_taskset_for_each(task, new_cgrp, tset) { - if (freezer->state == CGROUP_THAWED) { + if (!(freezer->state & CGROUP_FREEZING)) { __thaw_task(task); } else { freeze_task(task); - freezer->state = CGROUP_FREEZING; + freezer->state &= ~CGROUP_FROZEN; } } @@ -159,11 +156,7 @@ static void freezer_fork(struct task_struct *task) goto out; spin_lock_irq(&freezer->lock); - /* - * @task might have been just migrated into a FROZEN cgroup. Test - * equality with THAWED. Read the comment in freezer_attach(). - */ - if (freezer->state != CGROUP_THAWED) + if (freezer->state & CGROUP_FREEZING) freeze_task(task); spin_unlock_irq(&freezer->lock); out: @@ -184,7 +177,8 @@ static void update_if_frozen(struct freezer *freezer) struct cgroup_iter it; struct task_struct *task; - if (freezer->state != CGROUP_FREEZING) + if (!(freezer->state & CGROUP_FREEZING) || + (freezer->state & CGROUP_FROZEN)) return; cgroup_iter_start(cgroup, &it); @@ -202,7 +196,7 @@ static void update_if_frozen(struct freezer *freezer) } } - freezer->state = CGROUP_FROZEN; + freezer->state |= CGROUP_FROZEN; notyet: cgroup_iter_end(cgroup, &it); } @@ -211,14 +205,14 @@ static int freezer_read(struct cgroup *cgroup, struct cftype *cft, struct seq_file *m) { struct freezer *freezer = cgroup_freezer(cgroup); - enum freezer_state state; + unsigned int state; spin_lock_irq(&freezer->lock); update_if_frozen(freezer); state = freezer->state; spin_unlock_irq(&freezer->lock); - seq_puts(m, freezer_state_strs[state]); + seq_puts(m, freezer_state_strs(state)); seq_putc(m, '\n'); return 0; } @@ -258,14 +252,14 @@ static void freezer_apply_state(struct freezer *freezer, bool freeze) lockdep_assert_held(&freezer->lock); if (freeze) { - if (freezer->state == CGROUP_THAWED) + if (!(freezer->state & CGROUP_FREEZING)) atomic_inc(&system_freezing_cnt); - freezer->state = CGROUP_FREEZING; + freezer->state |= CGROUP_FREEZING; freeze_cgroup(freezer); } else { - if (freezer->state != CGROUP_THAWED) + if (freezer->state & CGROUP_FREEZING) atomic_dec(&system_freezing_cnt); - freezer->state = CGROUP_THAWED; + freezer->state &= ~(CGROUP_FREEZING | CGROUP_FROZEN); unfreeze_cgroup(freezer); } } @@ -290,9 +284,9 @@ static int freezer_write(struct cgroup *cgroup, struct cftype *cft, { bool freeze; - if (strcmp(buffer, freezer_state_strs[CGROUP_THAWED]) == 0) + if (strcmp(buffer, freezer_state_strs(0)) == 0) freeze = false; - else if (strcmp(buffer, freezer_state_strs[CGROUP_FROZEN]) == 0) + else if (strcmp(buffer, freezer_state_strs(CGROUP_FROZEN)) == 0) freeze = true; else return -EINVAL;
freezer->state was an enum value - one of THAWED, FREEZING and FROZEN. As the scheduled full hierarchy support requires more than one freezing condition, switch it to mask of flags. If FREEZING is not set, it's thawed. FREEZING is set if freezing or frozen. If frozen, both FREEZING and FROZEN are set. Now that tasks can be attached to an already frozen cgroup, this also makes freezing condition checks more natural. This patch doesn't introduce any behavior change. Signed-off-by: Tejun Heo <tj@kernel.org> --- kernel/cgroup_freezer.c | 60 ++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 33 deletions(-)