| Message ID | 1314988070-12244-2-git-send-email-tj@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 09/03, Tejun Heo wrote: > > @@ -308,24 +308,24 @@ static int freezer_change_state(struct cgroup *cgroup, > spin_lock_irq(&freezer->lock); > > update_if_frozen(cgroup, freezer); > - if (goal_state == freezer->state) > - goto out; > - > - freezer->state = goal_state; > > switch (goal_state) { > case CGROUP_THAWED: > - atomic_dec(&system_freezing_cnt); > + if (freezer->state != CGROUP_THAWED) > + atomic_dec(&system_freezing_cnt); > + freezer->state = CGROUP_THAWED; > unfreeze_cgroup(cgroup, freezer); > break; > case CGROUP_FROZEN: > - atomic_inc(&system_freezing_cnt); > + if (freezer->state == CGROUP_THAWED) > + atomic_inc(&system_freezing_cnt); > + freezer->state = CGROUP_FREEZING; > retval = try_to_freeze_cgroup(cgroup, freezer); Cough. Now it doesn't look right to me ;) If the user write "FROZEN" into the control file, we should not turn the CGROUP_FROZEN state into CGROUP_FREEZING. Probably this is harmless, but this looks wrong and this doesn't match the current behaviour, user-visible change. Oleg.
On Sun, Sep 04, 2011 at 08:02:06PM +0200, Oleg Nesterov wrote: > Cough. Now it doesn't look right to me ;) > > If the user write "FROZEN" into the control file, we should not > turn the CGROUP_FROZEN state into CGROUP_FREEZING. Probably this > is harmless, but this looks wrong and this doesn't match the > current behaviour, user-visible change. It's not user-visible as the state is always updated before shown to userland. If we're gonna be re-priming freezing on each FROZEN write, resetting the state to freezing to force state re-calculation is logical thing to do. Thanks.
On 09/05, Tejun Heo wrote: > > On Sun, Sep 04, 2011 at 08:02:06PM +0200, Oleg Nesterov wrote: > > Cough. Now it doesn't look right to me ;) > > > > If the user write "FROZEN" into the control file, we should not > > turn the CGROUP_FROZEN state into CGROUP_FREEZING. Probably this > > is harmless, but this looks wrong and this doesn't match the > > current behaviour, user-visible change. > > It's not user-visible as the state is always updated before shown to > userland. Ah, indeed, thanks. OK. So, the only proble I can see is that update_if_frozen() can hit BUG_ON(nfrozen > 0), but this is off-topic. I think this patch is correct. Oleg.
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c index 4e82525..56a457a 100644 --- a/kernel/cgroup_freezer.c +++ b/kernel/cgroup_freezer.c @@ -308,24 +308,24 @@ static int freezer_change_state(struct cgroup *cgroup, spin_lock_irq(&freezer->lock); update_if_frozen(cgroup, freezer); - if (goal_state == freezer->state) - goto out; - - freezer->state = goal_state; switch (goal_state) { case CGROUP_THAWED: - atomic_dec(&system_freezing_cnt); + if (freezer->state != CGROUP_THAWED) + atomic_dec(&system_freezing_cnt); + freezer->state = CGROUP_THAWED; unfreeze_cgroup(cgroup, freezer); break; case CGROUP_FROZEN: - atomic_inc(&system_freezing_cnt); + if (freezer->state == CGROUP_THAWED) + atomic_inc(&system_freezing_cnt); + freezer->state = CGROUP_FREEZING; retval = try_to_freeze_cgroup(cgroup, freezer); break; default: BUG(); } -out: + spin_unlock_irq(&freezer->lock); return retval;
d02f52811d0e "cgroup_freezer: prepare for removal of TIF_FREEZE" moved setting of freezer->state into freezer_change_state(); unfortunately, while doing so, when it's beginning to freeze tasks, it sets the state to CGROUP_FROZEN instead of CGROUP_FREEZING ending up skipping the whole freezing state. Fix it. -v2: Oleg pointed out that re-freezing FROZEN cgroup could increment system_freezing_cnt. Fixed. -v3: Matt pointed out that setting CGROUP_FROZEN always invoked try_to_freeze_cgroup() regardless of the current state. Patch updated such that the actual freeze/thaw operations are always performed on state changes. This shouldn't make any difference unless something is broken. Signed-off-by: Tejun Heo <tj@kernel.org> Reported-by: Oleg Nesterov <oleg@redhat.com> Cc: Paul Menage <paul@paulmenage.org> Cc: "Rafael J. Wysocki" <rjw@sisk.pl> Cc: Matt Helsley <matthltc@us.ibm.com> --- kernel/cgroup_freezer.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)