Hi Matt,
Looks good. One small comment inline below.
Thanks,
Oren.
Matt Helsley wrote:
> 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.
>
> Signed-off-by: Matt Helsley <matthltc@xxxxxxxxxx>
> Cc: Paul Menage <menage@xxxxxxxxxx>
> Cc: Li Zefan <lizf@xxxxxxxxxxxxxx>
> Cc: Cedric Le Goater <legoater@xxxxxxx>
>
> Notes:
> This is an untested RFC patch for container checkpoint. It's meant to
> work with Oren's patch series.
> 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 ++++++
> checkpoint/checkpoint.c | 7 ++--
> include/linux/freezer.h | 8 +++++
> kernel/cgroup_freezer.c | 46 ++++++++++++++++++++++++++-
> 4 files changed, 67 insertions(+), 4 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/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
> index 7f5c18c..010743f 100644
> --- a/checkpoint/checkpoint.c
> +++ b/checkpoint/checkpoint.c
> @@ -489,7 +489,7 @@ static int get_container(struct ckpt_ctx *ctx, pid_t pid)
> return -EBUSY;
> }
>
> - return 0;
> + return cgroup_freezer_begin_checkpoint(ctx->root_task);
>
> out:
> if (task)
> @@ -531,7 +531,7 @@ int do_checkpoint(struct ckpt_ctx *ctx, pid_t pid)
>
> ret = ctx_checkpoint(ctx, pid);
> if (ret < 0)
> - goto out;
> + return ret;
> ret = build_tree(ctx);
> if (ret < 0)
> goto out;
> @@ -560,6 +560,7 @@ int do_checkpoint(struct ckpt_ctx *ctx, pid_t pid)
> /* on success, return (unique) checkpoint identifier */
> ctx->crid = atomic_inc_return(&ctx_count);
> ret = ctx->crid;
> - out:
> +out:
> + cgroup_freezer_end_checkpoint(ctx->root_task);
> return ret;
> }
Perhaps place the call to cgroup_freezer_begin_checkpoint() to
inside the do_checkpoint() function ? It will be easier to
read and understand the code.
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index 5a361f8..f1d770b 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -65,8 +65,16 @@ extern void cancel_freezing(struct task_struct *p);
>
> #ifdef CONFIG_CGROUP_FREEZER
> extern int cgroup_frozen(struct task_struct *task);
> +extern int cgroup_freezer_begin_checkpoint(struct task_struct *task);
> +extern void cgroup_freezer_end_checkpoint(struct task_struct *task);
> #else /* !CONFIG_CGROUP_FREEZER */
> static inline int cgroup_frozen(struct task_struct *task) { return 0; }
> +static inline int cgroup_freezer_begin_checkpoint(struct task_struct *task)
> +{
> + return -ENOTSUP;
> +}
> +static inline void cgroup_freezer_end_checkpoint(struct task_struct *task)
> +{}
> #endif /* !CONFIG_CGROUP_FREEZER */
>
> /*
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index fb249e2..0443226 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 {
> @@ -61,6 +62,45 @@ int cgroup_frozen(struct task_struct *task)
> }
>
> /*
> + * 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;
> + if ((state == CGROUP_FROZEN && next_state == CGROUP_CHECKPOINTING) ||
> + (state == CGROUP_CHECKPOINTING && next_state == CGROUP_FROZEN))
> + freezer->state = next_state;
> + spin_unlock_irq(&freezer->lock);
> + task_unlock(task);
> +
> + return state;
> +}
> +
> +int cgroup_freezer_begin_checkpoint(struct task_struct *task)
> +{
> + if (freezer_checkpointing(task, CGROUP_CHECKPOINTING) != CGROUP_FROZEN)
> + return -EBUSY;
> + return 0;
> +}
> +
> +void cgroup_freezer_end_checkpoint(struct task_struct *task)
> +{
> + /*
> + * If we weren't in CHECKPOINTING state then userspace could have
> + * unfrozen a task and given us an inconsistent checkpoint image
> + */
> + WARN_ON(freezer_checkpointing(task, CGROUP_FROZEN) != CGROUP_CHECKPOINTING);
> +}
> +
> +/*
> * cgroups_write_string() limits the size of freezer state strings to
> * CGROUP_LOCAL_BUFFER_SIZE
> */
> @@ -68,6 +108,7 @@ static const char *freezer_state_strs[] = {
> "THAWED",
> "FREEZING",
> "FROZEN",
> + "CHECKPOINTING",
> };
>
> /*
> @@ -309,7 +350,10 @@ static int freezer_change_state(struct cgroup *cgroup,
> freezer = cgroup_freezer(cgroup);
>
> spin_lock_irq(&freezer->lock);
> -
> + if (freezer->state == CGROUP_CHECKPOINTING) {
> + retval = -EBUSY;
> + goto out;
> + }
> update_freezer_state(cgroup, freezer);
> if (goal_state == freezer->state)
> goto out;
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
