lkml.org 
[lkml]   [2009]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC v12][PATCH 14/14] Restart multiple processes
    Quoting Oren Laadan (orenl@cs.columbia.edu):
    > Restarting of multiple processes expects all restarting tasks to call
    > sys_restart(). Once inside the system call, each task will restart
    > itself at the same order that they were saved. The internals of the
    > syscall will take care of in-kernel synchronization bewteen tasks.
    >
    > This patch does _not_ create the task tree in the kernel. Instead it
    > assumes that all tasks are created in some way and then invoke the
    > restart syscall. You can use the userspace mktree.c program to do
    > that.
    >
    > The init task (*) has a special role: it allocates the restart context
    > (ctx), and coordinates the operation. In particular, it first waits
    > until all participating tasks enter the kernel, and provides them the
    > common restart context. Once everyone in ready, it begins to restart
    > itself.
    >
    > In contrast, the other tasks enter the kernel, locate the init task (*)
    > and grab its restart context, and then wait for their turn to restore.
    >
    > When a task (init or not) completes its restart, it hands the control
    > over to the next in line, by waking that task.
    >
    > An array of pids (the one saved during the checkpoint) is used to
    > synchronize the operation. The first task in the array is the init
    > task (*). The restart context (ctx) maintain a "current position" in
    > the array, which indicates which task is currently active. Once the
    > currently active task completes its own restart, it increments that
    > position and wakes up the next task.
    >
    > Restart assumes that userspace provides meaningful data, otherwise
    > it's garbage-in-garbage-out. In this case, the syscall may block
    > indefinitely, but in TASK_INTERRUPTIBLE, so the user can ctrl-c or
    > otherwise kill the stray restarting tasks.
    >
    > In terms of security, restart runs as the user the invokes it, so it
    > will not allow a user to do more than is otherwise permitted by the
    > usual system semantics and policy.
    >
    > Currently we ignore threads and zombies, as well as session ids.
    > Add support for multiple processes
    >
    > (*) For containers, restart should be called inside a fresh container
    > by the init task of that container. However, it is also possible to
    > restart applications not necessarily inside a container, and without
    > restoring the original pids of the processes (that is, provided that
    > the application can tolerate such behavior). This is useful to allow
    > multi-process restart of tasks not isolated inside a container, and
    > also for debugging.
    >
    > Changelog[v12]:
    > - Replace obsolete cr_debug() with pr_debug()
    >
    > Signed-off-by: Oren Laadan <orenl@cs.columbia.edu>

    Thanks, Oren.

    Acked-by: Serge Hallyn <serue@us.ibm.com>

    with a few comments below.

    > ---
    > checkpoint/restart.c | 214 +++++++++++++++++++++++++++++++++++++++++++-
    > checkpoint/sys.c | 34 ++++++--
    > include/linux/checkpoint.h | 23 ++++-
    > include/linux/sched.h | 1 +
    > 4 files changed, 258 insertions(+), 14 deletions(-)
    >
    > diff --git a/checkpoint/restart.c b/checkpoint/restart.c
    > index 0c46abf..6b4cd75 100644
    > --- a/checkpoint/restart.c
    > +++ b/checkpoint/restart.c
    > @@ -10,6 +10,7 @@
    >
    > #include <linux/version.h>
    > #include <linux/sched.h>
    > +#include <linux/wait.h>
    > #include <linux/file.h>
    > #include <linux/magic.h>
    > #include <linux/checkpoint.h>
    > @@ -276,30 +277,235 @@ static int cr_read_task(struct cr_ctx *ctx)
    > return ret;
    > }
    >
    > +/* cr_read_tree - read the tasks tree into the checkpoint context */
    > +static int cr_read_tree(struct cr_ctx *ctx)
    > +{
    > + struct cr_hdr_tree *hh = cr_hbuf_get(ctx, sizeof(*hh));
    > + int parent, size, ret = -EINVAL;
    > +
    > + parent = cr_read_obj_type(ctx, hh, sizeof(*hh), CR_HDR_TREE);
    > + if (parent < 0) {
    > + ret = parent;
    > + goto out;
    > + } else if (parent != 0)
    > + goto out;
    > +
    > + if (hh->tasks_nr < 0)
    > + goto out;
    > +
    > + ctx->pids_nr = hh->tasks_nr;
    > + size = sizeof(*ctx->pids_arr) * ctx->pids_nr;
    > + if (size < 0) /* overflow ? */
    > + goto out;
    > +
    > + ctx->pids_arr = kmalloc(size, GFP_KERNEL);
    > + if (!ctx->pids_arr) {
    > + ret = -ENOMEM;
    > + goto out;
    > + }
    > + ret = cr_kread(ctx, ctx->pids_arr, size);
    > + out:
    > + cr_hbuf_put(ctx, sizeof(*hh));
    > + return ret;
    > +}
    > +
    > +static int cr_wait_task(struct cr_ctx *ctx)
    > +{
    > + pid_t pid = task_pid_vnr(current);
    > +
    > + pr_debug("pid %d waiting\n", pid);
    > + return wait_event_interruptible(ctx->waitq, ctx->pids_active == pid);
    > +}
    > +
    > +static int cr_next_task(struct cr_ctx *ctx)
    > +{
    > + struct task_struct *tsk;
    > +
    > + ctx->pids_pos++;
    > +
    > + pr_debug("pids_pos %d %d\n", ctx->pids_pos, ctx->pids_nr);
    > + if (ctx->pids_pos == ctx->pids_nr) {
    > + complete(&ctx->complete);
    > + return 0;
    > + }
    > +
    > + ctx->pids_active = ctx->pids_arr[ctx->pids_pos].vpid;
    > +
    > + pr_debug("pids_next %d\n", ctx->pids_active);
    > +
    > + rcu_read_lock();
    > + tsk = find_task_by_pid_ns(ctx->pids_active, ctx->root_nsproxy->pid_ns);
    > + if (tsk)
    > + wake_up_process(tsk);
    > + rcu_read_unlock();
    > +
    > + if (!tsk) {
    > + ctx->pids_err = -ESRCH;
    > + complete(&ctx->complete);
    > + return -ESRCH;
    > + }
    > +
    > + return 0;
    > +}
    > +
    > +/* FIXME: this should be per container */
    > +DECLARE_WAIT_QUEUE_HEAD(cr_restart_waitq);
    > +
    > +static int do_restart_task(struct cr_ctx *ctx, pid_t pid)

    Passing ctx in here, when it is always NULL, is just a bit
    confusing, and, since you goto out and cr_ctx_put(ctx) without
    initializing ctx, you make verifying that that's ok a step
    harder.

    Do you intend to ever pass in non-NULL in later patches?

    > +{
    > + struct task_struct *root_task;
    > + int ret;
    > +
    > + rcu_read_lock();
    > + root_task = find_task_by_pid_ns(pid, current->nsproxy->pid_ns);
    > + if (root_task)
    > + get_task_struct(root_task);
    > + rcu_read_unlock();
    > +
    > + if (!root_task)
    > + return -EINVAL;
    > +
    > + /*
    > + * wait for container init to initialize the restart context, then
    > + * grab a reference to that context, and if we're the last task to
    > + * do it, notify the container init.
    > + */
    > + ret = wait_event_interruptible(cr_restart_waitq,
    > + root_task->checkpoint_ctx);

    Would seem more sensible to do the above using the ctx which
    you grabbed under task_lock(root_task) next. Your whole
    locking of root_task->checkpoint_ctx seems haphazard (see below).

    > + if (ret < 0)
    > + goto out;
    > +
    > + task_lock(root_task);
    > + ctx = root_task->checkpoint_ctx;
    > + if (ctx)
    > + cr_ctx_get(ctx);
    > + task_unlock(root_task);
    > +
    > + if (!ctx) {
    > + ret = -EAGAIN;
    > + goto out;
    > + }
    > +
    > + if (atomic_dec_and_test(&ctx->tasks_count))
    > + complete(&ctx->complete);
    > +
    > + /* wait for our turn, do the restore, and tell next task in line */
    > + ret = cr_wait_task(ctx);
    > + if (ret < 0)
    > + goto out;
    > + ret = cr_read_task(ctx);
    > + if (ret < 0)
    > + goto out;
    > + ret = cr_next_task(ctx);
    > +
    > + out:
    > + cr_ctx_put(ctx);
    > + put_task_struct(root_task);
    > + return ret;
    > +}
    > +
    > +static int cr_wait_all_tasks_start(struct cr_ctx *ctx)
    > +{
    > + int ret;
    > +
    > + if (ctx->pids_nr == 1)
    > + return 0;
    > +
    > + init_completion(&ctx->complete);
    > + current->checkpoint_ctx = ctx;
    > +
    > + wake_up_all(&cr_restart_waitq);
    > +
    > + ret = wait_for_completion_interruptible(&ctx->complete);
    > + if (ret < 0)
    > + return ret;
    > +
    > + task_lock(current);
    > + current->checkpoint_ctx = NULL;
    > + task_unlock(current);

    Who can you be racing with here? All other tasks should have
    already dereferenced current->checkpoint_ctx.

    If you want to always lock root_task around setting and
    reading of (root_task|current)->checkpoint_ctx, that's
    ok, but I think you can sufficiently argue that your
    completion is completely protecint readers from the sole
    writer.

    > +
    > + return 0;
    > +}
    > +
    > +static int cr_wait_all_tasks_finish(struct cr_ctx *ctx)
    > +{
    > + int ret;
    > +
    > + if (ctx->pids_nr == 1)
    > + return 0;
    > +
    > + init_completion(&ctx->complete);
    > +
    > + ret = cr_next_task(ctx);
    > + if (ret < 0)
    > + return ret;
    > +
    > + ret = wait_for_completion_interruptible(&ctx->complete);
    > + if (ret < 0)
    > + return ret;
    > +
    > + return 0;
    > +}
    > +
    > /* setup restart-specific parts of ctx */
    > static int cr_ctx_restart(struct cr_ctx *ctx, pid_t pid)
    > {
    > + ctx->root_pid = pid;
    > + ctx->root_task = current;
    > + ctx->root_nsproxy = current->nsproxy;
    > +
    > + get_task_struct(ctx->root_task);
    > + get_nsproxy(ctx->root_nsproxy);
    > +
    > + atomic_set(&ctx->tasks_count, ctx->pids_nr - 1);
    > +
    > return 0;
    > }
    >
    > -int do_restart(struct cr_ctx *ctx, pid_t pid)
    > +static int do_restart_root(struct cr_ctx *ctx, pid_t pid)
    > {
    > int ret;
    >
    > + ret = cr_read_head(ctx);
    > + if (ret < 0)
    > + goto out;
    > + ret = cr_read_tree(ctx);
    > + if (ret < 0)
    > + goto out;
    > +
    > ret = cr_ctx_restart(ctx, pid);
    > if (ret < 0)
    > goto out;
    > - ret = cr_read_head(ctx);
    > +
    > + /* wait for all other tasks to enter do_restart_task() */
    > + ret = cr_wait_all_tasks_start(ctx);
    > if (ret < 0)
    > goto out;
    > +
    > ret = cr_read_task(ctx);
    > if (ret < 0)
    > goto out;
    > - ret = cr_read_tail(ctx);
    > +
    > + /* wait for all other tasks to complete do_restart_task() */
    > + ret = cr_wait_all_tasks_finish(ctx);
    > if (ret < 0)
    > goto out;
    >
    > - /* on success, adjust the return value if needed [TODO] */
    > + ret = cr_read_tail(ctx);
    > +
    > out:
    > return ret;
    > }
    > +
    > +int do_restart(struct cr_ctx *ctx, pid_t pid)
    > +{
    > + int ret;
    > +
    > + if (ctx)
    > + ret = do_restart_root(ctx, pid);
    > + else
    > + ret = do_restart_task(ctx, pid);
    > +
    > + /* on success, adjust the return value if needed [TODO] */
    > + return ret;
    > +}
    > diff --git a/checkpoint/sys.c b/checkpoint/sys.c
    > index 0436ef3..f26b0c6 100644
    > --- a/checkpoint/sys.c
    > +++ b/checkpoint/sys.c
    > @@ -167,6 +167,8 @@ static void cr_task_arr_free(struct cr_ctx *ctx)
    >
    > static void cr_ctx_free(struct cr_ctx *ctx)
    > {
    > + BUG_ON(atomic_read(&ctx->refcount));
    > +
    > if (ctx->file)
    > fput(ctx->file);
    >
    > @@ -185,6 +187,8 @@ static void cr_ctx_free(struct cr_ctx *ctx)
    > if (ctx->root_task)
    > put_task_struct(ctx->root_task);
    >
    > + kfree(ctx->pids_arr);
    > +
    > kfree(ctx);
    > }
    >
    > @@ -199,8 +203,10 @@ static struct cr_ctx *cr_ctx_alloc(int fd, unsigned long flags)
    >
    > ctx->flags = flags;
    >
    > + atomic_set(&ctx->refcount, 0);
    > INIT_LIST_HEAD(&ctx->pgarr_list);
    > INIT_LIST_HEAD(&ctx->pgarr_pool);
    > + init_waitqueue_head(&ctx->waitq);
    >
    > err = -EBADF;
    > ctx->file = fget(fd);
    > @@ -215,6 +221,7 @@ static struct cr_ctx *cr_ctx_alloc(int fd, unsigned long flags)
    > if (cr_objhash_alloc(ctx) < 0)
    > goto err;
    >
    > + atomic_inc(&ctx->refcount);
    > return ctx;
    >
    > err:
    > @@ -222,6 +229,17 @@ static struct cr_ctx *cr_ctx_alloc(int fd, unsigned long flags)
    > return ERR_PTR(err);
    > }
    >
    > +void cr_ctx_get(struct cr_ctx *ctx)
    > +{
    > + atomic_inc(&ctx->refcount);
    > +}
    > +
    > +void cr_ctx_put(struct cr_ctx *ctx)
    > +{
    > + if (ctx && atomic_dec_and_test(&ctx->refcount))
    > + cr_ctx_free(ctx);
    > +}
    > +
    > /**
    > * sys_checkpoint - checkpoint a container
    > * @pid: pid of the container init(1) process
    > @@ -249,7 +267,7 @@ asmlinkage long sys_checkpoint(pid_t pid, int fd, unsigned long flags)
    > if (!ret)
    > ret = ctx->crid;
    >
    > - cr_ctx_free(ctx);
    > + cr_ctx_put(ctx);
    > return ret;
    > }
    >
    > @@ -264,7 +282,7 @@ asmlinkage long sys_checkpoint(pid_t pid, int fd, unsigned long flags)
    > */
    > asmlinkage long sys_restart(int crid, int fd, unsigned long flags)
    > {
    > - struct cr_ctx *ctx;
    > + struct cr_ctx *ctx = NULL;
    > pid_t pid;
    > int ret;
    >
    > @@ -272,15 +290,17 @@ asmlinkage long sys_restart(int crid, int fd, unsigned long flags)
    > if (flags)
    > return -EINVAL;
    >
    > - ctx = cr_ctx_alloc(fd, flags | CR_CTX_RSTR);
    > - if (IS_ERR(ctx))
    > - return PTR_ERR(ctx);
    > -
    > /* FIXME: for now, we use 'crid' as a pid */
    > pid = (pid_t) crid;
    >
    > + if (pid == task_pid_vnr(current))
    > + ctx = cr_ctx_alloc(fd, flags | CR_CTX_RSTR);
    > +
    > + if (IS_ERR(ctx))
    > + return PTR_ERR(ctx);
    > +
    > ret = do_restart(ctx, pid);
    >
    > - cr_ctx_free(ctx);
    > + cr_ctx_put(ctx);
    > return ret;
    > }
    > diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
    > index 86fcec9..041bd4d 100644
    > --- a/include/linux/checkpoint.h
    > +++ b/include/linux/checkpoint.h
    > @@ -13,10 +13,11 @@
    > #include <linux/fs.h>
    > #include <linux/path.h>
    > #include <linux/sched.h>
    > +#include <asm/atomic.h>
    >
    > #ifdef CONFIG_CHECKPOINT_RESTART
    >
    > -#define CR_VERSION 2
    > +#define CR_VERSION 3
    >
    > struct cr_ctx {
    > int crid; /* unique checkpoint id */
    > @@ -34,8 +35,7 @@ struct cr_ctx {
    > void *hbuf; /* temporary buffer for headers */
    > int hpos; /* position in headers buffer */
    >
    > - struct task_struct **tasks_arr; /* array of all tasks in container */
    > - int tasks_nr; /* size of tasks array */
    > + atomic_t refcount;
    >
    > struct cr_objhash *objhash; /* hash for shared objects */
    >
    > @@ -43,6 +43,20 @@ struct cr_ctx {
    > struct list_head pgarr_pool; /* pool of empty page arrays chain */
    >
    > struct path fs_mnt; /* container root (FIXME) */
    > +
    > + /* [multi-process checkpoint] */
    > + struct task_struct **tasks_arr; /* array of all tasks [checkpoint] */
    > + int tasks_nr; /* size of tasks array */
    > +
    > + /* [multi-process restart] */
    > + struct cr_hdr_pids *pids_arr; /* array of all pids [restart] */
    > + int pids_nr; /* size of pids array */
    > + int pids_pos; /* position pids array */
    > + int pids_err; /* error occured ? */
    > + pid_t pids_active; /* pid of (next) active task */
    > + atomic_t tasks_count; /* sync of restarting tasks */

    'sync of restarting tasks' for 3 vars suggests one or two or three
    might benefit from more detailed comment :)

    > + struct completion complete; /* sync of restarting tasks */
    > + wait_queue_head_t waitq; /* sync of restarting tasks */
    > };
    >
    > /* cr_ctx: flags */
    > @@ -55,6 +69,9 @@ extern int cr_kread(struct cr_ctx *ctx, void *buf, int count);
    > extern void *cr_hbuf_get(struct cr_ctx *ctx, int n);
    > extern void cr_hbuf_put(struct cr_ctx *ctx, int n);
    >
    > +extern void cr_ctx_get(struct cr_ctx *ctx);
    > +extern void cr_ctx_put(struct cr_ctx *ctx);
    > +
    > /* shared objects handling */
    >
    > enum {
    > diff --git a/include/linux/sched.h b/include/linux/sched.h
    > index faa2ec6..0150e90 100644
    > --- a/include/linux/sched.h
    > +++ b/include/linux/sched.h
    > @@ -1359,6 +1359,7 @@ struct task_struct {
    >
    > #ifdef CONFIG_CHECKPOINT_RESTART
    > atomic_t may_checkpoint;
    > + struct cr_ctx *checkpoint_ctx;
    > #endif
    > };
    >
    > --
    > 1.5.4.3


    \
     
     \ /
      Last update: 2009-01-04 21:23    [W:0.054 / U:0.944 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site