[PATCH 4/4] cgroup freezer: --- replacement patch 4/4 (a)

Oren Laadan orenl at cs.columbia.edu
Thu Jun 4 06:54:38 PDT 2009


On Thu, 4 Jun 2009, Oren Laadan wrote:

> On Thu, 4 Jun 2009, Matt Helsley wrote:
> 
> > On Wed, Jun 03, 2009 at 08:10:24PM -0400, Oren Laadan wrote:
> > > 
> > > From 6cdb7d9504a19ad88e6da8ad85374267c7acb1b2 Mon Sep 17 00:00:00 2001
> > > From: Matt Helsley <matthltc at us.ibm.com>
> > > Date: Wed, 3 Jun 2009 02:31:21 -0700
> > > Subject: [PATCH] cgroup freezer: Add CHECKPOINTING state to safeguard container checkpoint
> > > 
> > > The CHECKPOINTING state prevents userspace from unfreezing tasks until
> > > sys_checkpoint() is finished. When doing container checkpoint userspace
> > > will do:
> > > 
> > > 	echo FROZEN > /cgroups/my_container/freezer.state
> > > 	...
> > > 	rc = sys_checkpoint( <pid of container root> );
> > > 
> > > To ensure a consistent checkpoint image userspace should not be allowed
> > > to thaw the cgroup (echo THAWED > /cgroups/my_container/freezer.state)
> > > during checkpoint.
> > > 
> > > "CHECKPOINTING" can only be set on a "FROZEN" cgroup using the checkpoint
> > > system call. Once in the "CHECKPOINTING" state, the cgroup may not leave until
> > > the checkpoint system call is finished and ready to return. Then the
> > > freezer state returns to "FROZEN". Writing any new state to freezer.state while
> > > checkpointing will return EBUSY. These semantics ensure that userspace cannot
> > > unfreeze the cgroup midway through the checkpoint system call.
> > > 
> > > The cgroup_freezer_begin_checkpoint() and cgroup_freezer_end_checkpoint()
> > > make relatively few assumptions about the task that is passed in. However the
> > > way they are called in do_checkpoint() assumes that the root of the container
> > > is in the same freezer cgroup as all the other tasks that will be
> > > checkpointed.
> > > 
> > > Matt Helsley's wrote the original patch.
> > > 
> > > Changlog:
> > >   [2009-Jun-03] change cgroup_freezer_{begin,end}_checkpoint() to take a
> > >   		struct cgroup_subsys_state pointer
> > > 		add struct cgroup_subsys_state *get_task_cgroup_freezer()
> > > 
> > > Signed-off-by: Oren Laadan <orenl at cs.columbia.edu>
> > > Cc: Matt Helsley <matthltc at us.ibm.com>
> > > Cc: Paul Menage <menage at google.com>
> > > Cc: Li Zefan <lizf at cn.fujitsu.com>
> > > Cc: Cedric Le Goater <legoater at free.fr>
> > > 
> > > Notes:
> > >         As a side-effect this prevents the multiple tasks from entering the
> > >                 CHECKPOINTING state simultaneously. All but one will get -EBUSY.
> > > ---
> > >  Documentation/cgroups/freezer-subsystem.txt |   10 ++
> > >  include/linux/freezer.h                     |   10 ++
> > >  kernel/cgroup_freezer.c                     |  149 +++++++++++++++++++--------
> > >  3 files changed, 127 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/Documentation/cgroups/freezer-subsystem.txt b/Documentation/cgroups/freezer-subsystem.txt
> > > index 41f37fe..92b68e6 100644
> > > --- a/Documentation/cgroups/freezer-subsystem.txt
> > > +++ b/Documentation/cgroups/freezer-subsystem.txt
> > > @@ -100,3 +100,13 @@ things happens:
> > >  		and returns EINVAL)
> > >  	3) The tasks that blocked the cgroup from entering the "FROZEN"
> > >  		state disappear from the cgroup's set of tasks.
> > > +
> > > +When the cgroup freezer is used to guard container checkpoint operations the
> > > +freezer.state may be "CHECKPOINTING". "CHECKPOINTING" can only be set on a
> > > +"FROZEN" cgroup using the checkpoint system call. Once in the "CHECKPOINTING"
> > > +state, the cgroup may not leave until the checkpoint system call returns the
> > > +freezer state to "FROZEN". Writing any new state to freezer.state while
> > > +checkpointing will return EBUSY. These semantics ensure that userspace cannot
> > > +unfreeze the cgroup midway through the checkpoint system call. Note that,
> > > +unlike "FROZEN" and "FREEZING", there is no corresponding "CHECKPOINTED"
> > > +state.
> > > diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> > > index da7e52b..1402911 100644
> > > --- a/include/linux/freezer.h
> > > +++ b/include/linux/freezer.h
> > > @@ -64,12 +64,22 @@ extern bool freeze_task(struct task_struct *p, bool sig_only);
> > >  extern void cancel_freezing(struct task_struct *p);
> > > 
> > >  #ifdef CONFIG_CGROUP_FREEZER
> > > +struct cgroup_subsys_state;
> > >  extern int cgroup_freezing_or_frozen(struct task_struct *task);
> > > +extern struct cgroup_subsys_state *get_task_cgroup_freezer(struct task_struct *task);
> > > +extern int cgroup_freezer_begin_checkpoint(struct cgroup_subsys_state *css);
> > > +extern void cgroup_freezer_end_checkpoint(struct cgroup_subsys_state *css);
> > >  #else /* !CONFIG_CGROUP_FREEZER */
> > >  static inline int cgroup_freezing_or_frozen(struct task_struct *task)
> > >  {
> > >  	return 0;
> > >  }
> > > +static inline int cgroup_freezer_begin_checkpoint(struct task_struct *task)
> > > +{
> > > +	return -ENOTSUPP;
> > > +}
> > > +static inline void cgroup_freezer_end_checkpoint(struct task_struct *task)
> > > +{}
> > 
> > With CONFIG_CHECKPOINT depending on CONFIG_FREEZER I don't see why these
> > empty definitions are needed. Maybe it's just too late in the evening...
> 
> zzzzz....
> 
> > 
> > >  #endif /* !CONFIG_CGROUP_FREEZER */
> > > 
> > >  /*
> > > diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> > > index 05795b7..4790fb9 100644
> > > --- a/kernel/cgroup_freezer.c
> > > +++ b/kernel/cgroup_freezer.c
> > > @@ -25,6 +25,7 @@ enum freezer_state {
> > >  	CGROUP_THAWED = 0,
> > >  	CGROUP_FREEZING,
> > >  	CGROUP_FROZEN,
> > > +	CGROUP_CHECKPOINTING,
> > >  };
> > > 
> > >  struct freezer {
> > > @@ -47,6 +48,18 @@ static inline struct freezer *task_freezer(struct task_struct *task)
> > >  			    struct freezer, css);
> > >  }
> > > 
> > > +struct cgroup_subsys_state *get_task_cgroup_freezer(struct task_struct *task)
> > > +{
> > > +	struct cgroup_subsys_state *css;
> > > +
> > > +	task_lock(task);
> > > +	css = task_subsys_state(task, freezer_subsys_id);
> > > +	css_get(css); /* make sure freezer doesn't go away */
> > > +	task_unlock(task);
> > > +
> > > +	return css;
> > > +}
> > 
> > Seems like there should be a better way to do this than grabbing
> > this reference. I'd prefer to just introduce:
> > 
> > int in_same_cgroup_freezer(struct task_struct *task1, 
> > 				struct task_struct *task2);
> > 
> > In the external checkpoint case the root task is frozen and hence cannot
> > change cgroups.
> > 
> > Regardless of that reference, I think the interaction between
> > sys_checkpoint() and the cgroup freezer as we have it right now is broken
> > in the self-checkpoint case. It doesn't work because the freezer allows a
> > task to freeze itself. So if it does simply:
> 
> There should be no interaction. 
> 
> While I do make sure to skip the freezer cgroup test for the self
> checkpoint test, I forgot to do it when calling begin/end checkpoint.
> 
> Since we don't require that a self checkpointing task freeze itself,
> this can be fixed by adding suitable 'if (t != current) before these
> two calls.

Ahhh ... scratch that. Call should remain, of course.

Instead, user can remove the checkpointer task (in self-checkpoint) 
from the freezer cgroup. The tests is may_checkpoint() already allow
current task to not be in the cgroup.

Oren.



More information about the Containers mailing list