On Fri, May 29, 2009 at 02:35:21PM -0400, Oren Laadan wrote:
>
> Matt Helsley wrote:
> > The CHECKPOINTING state prevents userspace from unfreezing tasks until
> > sys_checkpoint() is finished. When doing container checkpoint userspace
> > will do:
>
> [...]
>
> > /*
> > + * cgroup freezer state changes made without the aid of the cgroup filesystem
> > + * must go through this function to ensure proper locking is observed.
> > + */
> > +static int freezer_checkpointing(struct task_struct *task,
> > + enum freezer_state next_state)
> > +{
> > + struct freezer *freezer;
> > + enum freezer_state state;
> > +
> > + task_lock(task);
> > + freezer = task_freezer(task);
> > + spin_lock_irq(&freezer->lock);
> > + state = freezer->state;
>
> After "echo FROZEN > /freezer/0/freezer_state", it is likely that
> freezer->state remains CGROUP_FREEZING --
>
> Then, looking at freezer_read():
>
> ...
> if (state == CGROUP_FREEZING) {
> /* We change from FREEZING to FROZEN lazily if the cgroup was
> * only partially frozen when we exitted write. */
> update_freezer_state(cgroup, freezer);
> state = freezer->state;
> }
> ...
>
> IOW, usually, only if someone reads the freezer_state file will
> the freezer->status reliably reflect the state.
>
> So, I searched for where else freezer->state is consulted, and found:
>
> int cgroup_frozen(struct task_struct *task)
> {
> struct freezer *freezer;
> enum freezer_state state;
>
> task_lock(task);
> freezer = task_freezer(task);
> state = freezer->state;
> task_unlock(task);
>
> return state == CGROUP_FROZEN;
> }
>
> Which is called (only?) from kernel/power/process.c:thaw_tasks().
Currently that is the only place it's called, yes.
> Here, too, unless some read from the freezer_state file, this
> test will incorrectly fail.
True. For kernel/power/process.c:thaw_tasks() we're really trying to
prevent resume from thawing tasks that were frozen via the cgroup
freezer.
So really CGROUP_FREEZING should also be tested for in that code path.
However that doesn't match the name so a name change would be in order
too.
I audited the rest of the cgroup freezer code without the CHECKPOINTING
patch and the remaining tests for CGROUP_FROZEN look fine to me:
can_attach:
This looks OK. If we're not in the frozen state then it's OK to
add a new task to the cgroup. So the analogous scenario is we're
in CGROUP_FREEZING but all of the tasks are frozen. Adding a
new task -- frozen or otherwise -- would not pose a problem.
BUG_ON() in freezer_fork:
The forking task isn't frozen and hence its cgroup is not
can't be FROZEN. But there's a BUG_ON() here "just in case" --
it should never happen. So at worst it won't find bugs here until
userspace does a read() on freezer.state.
Lastly, this means we'd need a new function for other parts of the
kernel which definitely want to know if the cgroup is frozen. That
function would have to do a lazy update if the state is
CGROUP_FREEZING.
I'll make a patch which changes cgroup_frozen() and send it off for
mainline.
Then I'll re-add cgroup_frozen() for CHECKPOINTING and ensure that it
works with the lazy update.
Cheers,
-Matt
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
