Skip to contentSkip to conversation

/proc/pid/checkpointable

PATCH In review Sukadev Bhattiprolu 7 participants 26 messages Mar 17, 2009 – Mar 26, 2009 15 messages not in the archive

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover /proc/pid/checkpointable

Started out implementing '/proc/pid/checkpointable' which reports 0 or 1 depending on whether a process is checkpointable atm. But realized hat some patches that were discussed earlier are missing from Dave Hansen's tree.

This patchset includes those missing patches plus two new patches that create /proc/pid/checkpointable.

[PATCH 1/6] Checkpoint multiple processes [PATCH 2/6] Restart multiple processes
[PATCH 3/6] Check 'may_checkpoint()' early [PATCH 4/6] Deny external checkpoint unless task is frozen [PATCH 5/6] Define and use proc_pid_checkpointable() [PATCH 6/6] Explain reason for task being uncheckpointable

A message that is not in the archive (sent off-list or not yet received): 20090317062754.GA2377-r/[email protected]

author Acked-by Serge Hallyn
#2 lore raw

From: Oren Laadan <[email protected]>
Date: Thu, 12 Mar 2009 14:16:05 -0700
Subject: [PATCH 1/6] Checkpoint multiple processes

Checkpointing of multiple processes works by recording the tasks tree structure below a given task (usually this task is the container init).

For a given task, do a DFS scan of the tasks tree and collect them into an array (keeping a reference to each task). Using DFS simplifies the recreation of tasks either in user space or kernel space. For each task collected, test if it can be checkpointed, and save its pid, tgid, and ppid.

The actual work is divided into two passes: a first scan counts the tasks, then memory is allocated and a second scan fills the array.

The logic is suitable for creation of processes during restart either in userspace or by the kernel.

Currently we ignore threads and zombies, as well as session ids.

Changelog[v12]:
- Replace obsolete cr_debug() with pr_debug()

---
 checkpoint/checkpoint.c        |  217 ++++++++++++++++++++++++++++++++++++++--
 checkpoint/sys.c               |   16 +++
 include/linux/checkpoint.h     |    3 +
 include/linux/checkpoint_hdr.h |   13 ++-
 4 files changed, 238 insertions(+), 11 deletions(-)
checkpoint/checkpoint.c +207 -10
index e0af8a2..ed4fd3d 100644
@@ -226,13 +226,6 @@ static int cr_write_task(struct cr_ctx *ctx, struct task_struct *t)
 {
 	int ret;
 
-	/* TODO: verity that the task is frozen (unless self) */
-
-	if (t->state == TASK_DEAD) {
-		pr_warning("c/r: task may not be in state TASK_DEAD\n");
-		return -EAGAIN;
-	}
-
 	ret = cr_write_task_struct(ctx, t);
 	pr_debug("task_struct: ret %d\n", ret);
 	if (ret < 0)
@@ -255,6 +248,200 @@ static int cr_write_task(struct cr_ctx *ctx, struct task_struct *t)
 	return ret;
 }
 
+/* dump all tasks in ctx->tasks_arr[] */
+static int cr_write_all_tasks(struct cr_ctx *ctx)
+{
+	int n, ret = 0;
+
+	for (n = 0; n < ctx->tasks_nr; n++) {
+		pr_debug("dumping task #%d\n", n);
+		ret = cr_write_task(ctx, ctx->tasks_arr[n]);
+		if (ret < 0)
+			break;
+	}
+
+	return ret;
+}
+
+static int cr_may_checkpoint_task(struct task_struct *t, struct cr_ctx *ctx)
+{
+	pr_debug("check %d\n", task_pid_nr_ns(t, ctx->root_nsproxy->pid_ns));
+
+	if (t->state == TASK_DEAD) {
+		pr_warning("c/r: task %d is TASK_DEAD\n", task_pid_vnr(t));
+		return -EAGAIN;
+	}
+
+	if (!ptrace_may_access(t, PTRACE_MODE_READ))
+		return -EPERM;
+
+	/* FIXME: verify that the task is frozen (unless self) */
+
+	/* FIXME: change this for nested containers */
+	if (task_nsproxy(t) != ctx->root_nsproxy)
+		return -EPERM;
+
+	return 0;
+}
+
+#define CR_HDR_PIDS_CHUNK	256
+
+static int cr_write_pids(struct cr_ctx *ctx)
+{
+	struct cr_hdr_pids *hh;
+	struct pid_namespace *ns;
+	struct task_struct *task;
+	struct task_struct **tasks_arr;
+	int tasks_nr, n, ret = 0, pos = 0;
+
+	ns = ctx->root_nsproxy->pid_ns;
+	tasks_arr = ctx->tasks_arr;
+	tasks_nr = ctx->tasks_nr;
+
+	hh = cr_hbuf_get(ctx, sizeof(*hh) * CR_HDR_PIDS_CHUNK);
+
+	while (tasks_nr > 0) {
+		rcu_read_lock();
+		for (n = min(tasks_nr, CR_HDR_PIDS_CHUNK); n; n--) {
+			task = tasks_arr[pos];
+
+			/* is this task cool ? */
+			ret = cr_may_checkpoint_task(task, ctx);
+			if (ret < 0) {
+				rcu_read_unlock();
+				goto out;
+			}
+			hh[pos].vpid = task_pid_nr_ns(task, ns);
+			hh[pos].vtgid = task_tgid_nr_ns(task, ns);
+			hh[pos].vppid = task_tgid_nr_ns(task->real_parent, ns);
+			pr_debug("task[%d]: vpid %d vtgid %d parent %d\n", pos,
+				 hh[pos].vpid, hh[pos].vtgid, hh[pos].vppid);
+			pos++;
+		}
+		rcu_read_unlock();
+
+		n = min(tasks_nr, CR_HDR_PIDS_CHUNK);
+		ret = cr_kwrite(ctx, hh, n * sizeof(*hh));
+		if (ret < 0)
+			break;
+
+		tasks_nr -= n;
+	}
+ out:
+	cr_hbuf_put(ctx, sizeof(*hh));
+	return ret;
+}
+
+/* count number of tasks in tree (and optionally fill pid's in array) */
+static int cr_tree_count_tasks(struct cr_ctx *ctx)
+{
+	struct task_struct *root = ctx->root_task;
+	struct task_struct *task = root;
+	struct task_struct *parent = NULL;
+	struct task_struct **tasks_arr = ctx->tasks_arr;
+	int tasks_nr = ctx->tasks_nr;
+	int nr = 0;
+
+	read_lock(&tasklist_lock);
+
+	/* count tasks via DFS scan of the tree */
+	while (1) {
+		if (tasks_arr) {
+			/* unlikely, but ... */
+			if (nr == tasks_nr)
+				return -EBUSY;	/* cleanup in cr_ctx_free() */
+			tasks_arr[nr] = task;
+			get_task_struct(task);
+		}
+
+		nr++;
+
+		/* if has children - proceed with child */
+		if (!list_empty(&task->children)) {
+			parent = task;
+			task = list_entry(task->children.next,
+					  struct task_struct, sibling);
+			continue;
+		}
+
+		while (task != root) {
+			/* if has sibling - proceed with sibling */
+			if (!list_is_last(&task->sibling, &parent->children)) {
+				task = list_entry(task->sibling.next,
+						  struct task_struct, sibling);
+				break;
+			}
+
+			/* else, trace back to parent and proceed */
+			task = parent;
+			parent = parent->real_parent;
+		}
+
+		if (task == root)
+			break;
+	}
+
+	read_unlock(&tasklist_lock);
+	return nr;
+}
+
+/*
+ * cr_build_tree - scan the tasks tree in DFS order and fill in array
+ * @ctx: checkpoint context
+ *
+ * Using DFS order simplifies the restart logic to re-create the tasks.
+ *
+ * On success, ctx->tasks_arr will be allocated and populated with all
+ * tasks (reference taken), and ctx->tasks_nr will hold the total count.
+ * The array is cleaned up by cr_ctx_free().
+ */
+static int cr_build_tree(struct cr_ctx *ctx)
+{
+	int n, m;
+
+	/* count tasks (no side effects) */
+	n = cr_tree_count_tasks(ctx);
+
+	ctx->tasks_nr = n;
+	ctx->tasks_arr = kzalloc(n * sizeof(*ctx->tasks_arr), GFP_KERNEL);
+	if (!ctx->tasks_arr)
+		return -ENOMEM;
+
+	/* count again (now will fill array) */
+	m = cr_tree_count_tasks(ctx);
+
+	/* unlikely, but ... (cleanup in cr_ctx_free) */
+	if (m < 0)
+		return m;
+	else if (m != n)
+		return -EBUSY;
+
+	return 0;
+}
+
+/* dump the array that describes the tasks tree */
+static int cr_write_tree(struct cr_ctx *ctx)
+{
+	struct cr_hdr h;
+	struct cr_hdr_tree *hh;
+	int ret;
+
+	h.type = CR_HDR_TREE;
+	h.len = sizeof(*hh);
+	h.parent = 0;
+
+	hh = cr_hbuf_get(ctx, sizeof(*hh));
+	hh->tasks_nr = ctx->tasks_nr;
+
+	ret = cr_write_obj(ctx, &h, hh);
+	cr_hbuf_put(ctx, sizeof(*hh));
+	if (ret < 0)
+		return ret;
+
+	ret = cr_write_pids(ctx);
+	return ret;
+}
+
 static int cr_get_container(struct cr_ctx *ctx, pid_t pid)
 {
 	struct task_struct *task = NULL;
@@ -272,7 +459,7 @@ static int cr_get_container(struct cr_ctx *ctx, pid_t pid)
 	if (!task)
 		goto out;
 
-#if 0	/* enable to use containers */
+#if 0	/* enable with containers */
 	if (!is_container_init(task)) {
 		err = -EINVAL;
 		goto out;
@@ -294,7 +481,7 @@ static int cr_get_container(struct cr_ctx *ctx, pid_t pid)
 	if (!nsproxy)
 		goto out;
 
-	/* TODO: verify that the container is frozen */
+	/* FIXME: verify that the container is frozen */
 
 	ctx->root_task = task;
 	ctx->root_nsproxy = nsproxy;
@@ -342,12 +529,22 @@ int do_checkpoint(struct cr_ctx *ctx, pid_t pid)
 	ret = cr_ctx_checkpoint(ctx, pid);
 	if (ret < 0)
 		goto out;
+
+	ret = cr_build_tree(ctx);
+	if (ret < 0)
+		goto out;
+
 	ret = cr_write_head(ctx);
 	if (ret < 0)
 		goto out;
-	ret = cr_write_task(ctx, ctx->root_task);
+	ret = cr_write_tree(ctx);
 	if (ret < 0)
 		goto out;
+
+	ret = cr_write_all_tasks(ctx);
+	if (ret < 0)
+		goto out;
+
 	ret = cr_write_tail(ctx);
 	if (ret < 0)
 		goto out;
checkpoint/sys.c +16 -0
index 4a51ed3..0436ef3 100644
@@ -152,6 +152,19 @@ void cr_hbuf_put(struct cr_ctx *ctx, int n)
  * restart operation, and persists until the operation is completed.
  */
 
+static void cr_task_arr_free(struct cr_ctx *ctx)
+{
+	int n;
+
+	for (n = 0; n < ctx->tasks_nr; n++) {
+		if (ctx->tasks_arr[n]) {
+			put_task_struct(ctx->tasks_arr[n]);
+			ctx->tasks_arr[n] = NULL;
+		}
+	}
+	kfree(ctx->tasks_arr);
+}
+
 static void cr_ctx_free(struct cr_ctx *ctx)
 {
 	if (ctx->file)
@@ -164,6 +177,9 @@ static void cr_ctx_free(struct cr_ctx *ctx)
 	cr_pgarr_free(ctx);
 	cr_objhash_free(ctx);
 
+	if (ctx->tasks_arr)
+		cr_task_arr_free(ctx);
+
 	if (ctx->root_nsproxy)
 		put_nsproxy(ctx->root_nsproxy);
 	if (ctx->root_task)
include/linux/checkpoint.h +3 -0
index e77393f..6307511 100644
@@ -34,6 +34,9 @@ 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 */
+
 	struct cr_objhash *objhash;	/* hash for shared objects */
 
 	struct list_head pgarr_list;	/* page array to dump VMA contents */
include/linux/checkpoint_hdr.h +12 -1
index 66313bd..0acf7c0 100644
@@ -45,7 +45,8 @@ enum {
 	CR_HDR_STRING,
 	CR_HDR_FNAME,
 
-	CR_HDR_TASK = 101,
+	CR_HDR_TREE = 101,
+	CR_HDR_TASK,
 	CR_HDR_THREAD,
 	CR_HDR_CPU,
 
@@ -81,6 +82,16 @@ struct cr_hdr_tail {
 	__u64 magic;
 } __attribute__((aligned(8)));
 
+struct cr_hdr_tree {
+	__u32 tasks_nr;
+} __attribute__((aligned(8)));
+
+struct cr_hdr_pids {
+	__s32 vpid;
+	__s32 vtgid;
+	__s32 vppid;
+} __attribute__((aligned(8)));
+
 struct cr_hdr_task {
 	__u32 state;
 	__u32 exit_state;
Signature · 1 line
1.5.2.5
#3 lore raw
In reply to a message not in the archive

From: Oren Laadan <[email protected]>
Date: Thu, 12 Mar 2009 14:19:41 -0700
Subject: [PATCH 2/6] Restart multiple processes

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()

---
 checkpoint/restart.c       |  214 +++++++++++++++++++++++++++++++++++++++++++-
 checkpoint/sys.c           |   34 ++++++--
 include/linux/checkpoint.h |   25 +++++-
 include/linux/sched.h      |    3 +
 4 files changed, 262 insertions(+), 14 deletions(-)
checkpoint/restart.c +210 -4
index 0c46abf..6b4cd75 100644
@@ -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)
+{
+	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);
+	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);
+
+	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;
+}
checkpoint/sys.c +27 -7
index 0436ef3..f26b0c6 100644
@@ -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;
 }
include/linux/checkpoint.h +22 -3
index 6307511..484be56 100644
@@ -13,10 +13,13 @@
 #include <linux/path.h>
 #include <linux/fs.h>
 #include <linux/fdtable.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 +37,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 +45,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 */
+	struct completion complete;	/* sync of restarting tasks */
+	wait_queue_head_t waitq;	/* sync of restarting tasks */
 };
 
 /* cr_ctx: flags */
@@ -55,6 +71,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 {
include/linux/sched.h +3 -0
index 8c216e0..c5d5987 100644
@@ -1416,6 +1416,9 @@ struct task_struct {
 #ifdef CONFIG_TRACING
 	/* state flags for use by tracers */
 	unsigned long trace;
+#endif 
+#ifdef CONFIG_CHECKPOINT_RESTART
+	struct cr_ctx *checkpoint_ctx;
 #endif
 };
 
Signature · 1 line
1.5.2.5
#4 lore raw
In reply to a message not in the archive

From: Sukadev Bhattiprolu <[email protected]>
Date: Thu, 12 Mar 2009 14:19:54 -0700
Subject: [PATCH 3/6] Check 'may_checkpoint()' early

We currently check if a task is checkpointable when writing the task information to checkpoint file. The small downside of doing this check late is that we may have processed several processes in the tree before hitting one that cannot be checkpointed.

We anyway walk the process tree we are checkpointing earlier, while counting the tasks. We could check if all processes in the tree are checkpointable at that time and fail early if any of them are not. Since the process tree should be frozen, checking earlier should not matter ?

For now, the patch leaves the existing check in cr_write_pids(). We can remove that later.

---
 checkpoint/checkpoint.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
checkpoint/checkpoint.c +9 -0
index ed4fd3d..dae9b97 100644
@@ -341,11 +341,18 @@ static int cr_tree_count_tasks(struct cr_ctx *ctx)
 	struct task_struct **tasks_arr = ctx->tasks_arr;
 	int tasks_nr = ctx->tasks_nr;
 	int nr = 0;
+	int ret;
 
 	read_lock(&tasklist_lock);
 
 	/* count tasks via DFS scan of the tree */
 	while (1) {
+		ret = cr_may_checkpoint_task(task, ctx);
+		if (ret < 0) {
+			nr = ret;
+			break;
+		}
+
 		if (tasks_arr) {
 			/* unlikely, but ... */
 			if (nr == tasks_nr)
@@ -401,6 +408,8 @@ static int cr_build_tree(struct cr_ctx *ctx)
 
 	/* count tasks (no side effects) */
 	n = cr_tree_count_tasks(ctx);
+	if (n < 0)
+		return n;
 
 	ctx->tasks_nr = n;
 	ctx->tasks_arr = kzalloc(n * sizeof(*ctx->tasks_arr), GFP_KERNEL);
Signature · 1 line
1.5.2.5
#5 lore raw
In reply to a message not in the archive

From: Sukadev Bhattiprolu <[email protected]>
Date: Thu, 12 Mar 2009 14:20:02 -0700
Subject: [PATCH 4/6] Deny external checkpoint unless task is frozen

Remove a 'FIXME' and ensure that the tasks we are checkpointing are frozen unless its a self-checkpoint.

---
 checkpoint/checkpoint.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
checkpoint/checkpoint.c +4 -1
index dae9b97..c16f30c 100644
@@ -19,6 +19,7 @@
 #include <linux/mount.h>
 #include <linux/utsname.h>
 #include <linux/magic.h>
+#include <linux/freezer.h>
 #include <linux/checkpoint.h>
 #include <linux/checkpoint_hdr.h>
 
@@ -275,7 +276,9 @@ static int cr_may_checkpoint_task(struct task_struct *t, struct cr_ctx *ctx)
 	if (!ptrace_may_access(t, PTRACE_MODE_READ))
 		return -EPERM;
 
-	/* FIXME: verify that the task is frozen (unless self) */
+	/* verify that the task is frozen (unless self) */
+	if (t != current && !frozen(t))
+		return -EBUSY;
 
 	/* FIXME: change this for nested containers */
 	if (task_nsproxy(t) != ctx->root_nsproxy)
Signature · 1 line
1.5.2.5
#6 lore raw
In reply to a message not in the archive

From: Sukadev Bhattiprolu <[email protected]>
Date: Fri, 13 Mar 2009 17:25:42 -0700
Subject: [PATCH 5/6] Define and use proc_pid_checkpointable()

Create a proc file, /proc/pid/checkpointable, which shows '1' if task is checkpointable and '0' if it is not.

To determine whether a task is checkpointable, the handler for this new proc file, shares the same code with sys_checkpoint().

---
 checkpoint/checkpoint.c    |   23 ++++++++++++++++-------
 fs/proc/base.c             |   13 +++++++++++++
 include/linux/checkpoint.h |    7 +++++++
 3 files changed, 36 insertions(+), 7 deletions(-)
checkpoint/checkpoint.c +16 -7
index c16f30c..ad0956c 100644
@@ -264,22 +264,31 @@ static int cr_write_all_tasks(struct cr_ctx *ctx)
 	return ret;
 }
 
+int task_checkpointable(struct task_struct *t)
+{
+	if (t->state == TASK_DEAD) {
+		pr_warning("c/r: task %d is TASK_DEAD\n", task_pid_vnr(t));
+		return 0;
+	}
+
+	/* Verify that task is frozen, unless it is self-checkpoint */
+	if (t != current && !frozen(t))
+		return 0;
+
+	return 1;
+}
+
 static int cr_may_checkpoint_task(struct task_struct *t, struct cr_ctx *ctx)
 {
 	pr_debug("check %d\n", task_pid_nr_ns(t, ctx->root_nsproxy->pid_ns));
 
-	if (t->state == TASK_DEAD) {
-		pr_warning("c/r: task %d is TASK_DEAD\n", task_pid_vnr(t));
+	/* verify that the task is checkpointable */
+	if (!task_checkpointable(t))
 		return -EAGAIN;
-	}
 
 	if (!ptrace_may_access(t, PTRACE_MODE_READ))
 		return -EPERM;
 
-	/* verify that the task is frozen (unless self) */
-	if (t != current && !frozen(t))
-		return -EBUSY;
-
 	/* FIXME: change this for nested containers */
 	if (task_nsproxy(t) != ctx->root_nsproxy)
 		return -EPERM;
fs/proc/base.c +13 -0
index bde92c3..989ca93 100644
@@ -2501,6 +2501,13 @@ static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns,
 	return 0;
 }
 
+#ifdef CONFIG_CHECKPOINT_RESTART
+static int proc_pid_checkpointable(struct task_struct *task, char *buffer)
+{
+	return sprintf(buffer, "%d\n", task_checkpointable(task));
+}
+#endif
+
 /*
  * Thread groups
  */
@@ -2580,6 +2587,9 @@ static const struct pid_entry tgid_base_stuff[] = {
 #ifdef CONFIG_TASK_IO_ACCOUNTING
 	INF("io",	S_IRUGO, proc_tgid_io_accounting),
 #endif
+#ifdef CONFIG_CHECKPOINT_RESTART
+	INF("checkpointable", S_IRUGO, proc_pid_checkpointable),
+#endif
 };
 
 static int proc_tgid_base_readdir(struct file * filp,
@@ -2915,6 +2925,9 @@ static const struct pid_entry tid_base_stuff[] = {
 #ifdef CONFIG_TASK_IO_ACCOUNTING
 	INF("io",	S_IRUGO, proc_tid_io_accounting),
 #endif
+#ifdef CONFIG_CHECKPOINT_RESTART
+	INF("checkpointable", S_IRUGO, proc_pid_checkpointable),
+#endif
 };
 
 static int proc_tid_base_readdir(struct file * filp,
include/linux/checkpoint.h +7 -0
index 484be56..d1f53a6 100644
@@ -142,6 +142,8 @@ static inline int cr_enabled(void)
 	return 1;
 }
 
+int task_checkpointable(struct task_struct *t);
+
 #else /* !CONFIG_CHECKPOINT_RESTART */
 
 static inline void files_deny_checkpointing(struct files_struct *files) {}
@@ -162,5 +164,10 @@ static inline int cr_enabled(void)
 	return 0;
 }
 
+static inline int task_checkpointable(struct task_struct *t)
+{
+	return 0;
+}
+
 #endif /* CONFIG_CHECKPOINT_RESTART */
 #endif /* _CHECKPOINT_CKPT_H_ */
Signature · 1 line
1.5.2.5

A message that is not in the archive (sent off-list or not yet received): 20090317063940.GF2377-r/[email protected]

#8 lore raw
Quoted 9 lines · Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
Date: Fri, 13 Mar 2009 17:25:42 -0700
Subject: [PATCH 5/6] Define and use proc_pid_checkpointable()

Create a proc file, /proc/pid/checkpointable, which shows '1' if task is checkpointable and '0' if it is not.

To determine whether a task is checkpointable, the handler for this new proc file, shares the same code with sys_checkpoint().

I still don't understand why we would like to do it this way.

First, it makes little sense to do it per-task, because we are supposed to checkpoint an entire container.

Second, what's wrong with doing a "dry" checkpoint on the container (or if you prefer, the task, for what it's worth), that will not buffer nor write out any data - just say "yes" or "no" ?

(we could use a flag "CR_CTX_DRYRUN" when calling sys_checkpoint() for this, and test for this flag in, say, kwrite/kread).

After all, we don't expect applications or users to continuously and repeatedly test if they can checkpoint, so it isn't performance critical. So we simply reuse the existing code.

This would also catch cases where we can't checkpoint because the kernel is low on memory - which wouldn't show up otherwise.

And in any case, this is orthogonal to what Dave is pushing, following Ingo's comment, to know when a task _becomes_ not-checkpointable. (And in any case, I think our time is better spent on adding functionality instead).

Oren.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#10 lore raw
Quoted 7 lines · Oren Laadan
Quoted 4 lines · Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
Date: Fri, 13 Mar 2009 17:25:42 -0700
Subject: [PATCH 5/6] Define and use proc_pid_checkpointable()

Quoted 5 lines
Quoted 5 lines

Create a proc file, /proc/pid/checkpointable, which shows '1' if task is checkpointable and '0' if it is not.

To determine whether a task is checkpointable, the handler for this new proc file, shares the same code with sys_checkpoint().

Hey Oren,

3 counter-points:

Quoted 4 lines

I still don't understand why we would like to do it this way.

First, it makes little sense to do it per-task, because we are supposed to checkpoint an entire container.

Yes we need per-container info too. Actually, per-checkpoint-job-init, so if we send pids in for that, it should return false if we send in the pid of a task which isn't a proper checkpoint-job-init.

But we also want the info per-task, for debugging info.

I don't know how to represent those two cases, though.

Also debugfs may be a more appropriate medium.

Quoted 3 lines

Second, what's wrong with doing a "dry" checkpoint on the container (or if you prefer, the task, for what it's worth), that will not buffer nor write out any data - just say "yes" or "no" ?

You can't get a text explanation like 'fd 4 (/sys/class/net/eth0) is not checkpointable'. That's what's wrong with it.

Quoted 6 lines

(we could use a flag "CR_CTX_DRYRUN" when calling sys_checkpoint() for this, and test for this flag in, say, kwrite/kread).

After all, we don't expect applications or users to continuously and repeatedly test if they can checkpoint, so it isn't performance critical. So we simply reuse the existing code.

No, but we do expect someone trying to checkpoint their job and failing to be curious as to why.

Quoted 7 lines

This would also catch cases where we can't checkpoint because the kernel is low on memory - which wouldn't show up otherwise.

And in any case, this is orthogonal to what Dave is pushing, following Ingo's comment, to know when a task _becomes_ not-checkpointable. (And in any case, I think our time is better spent on adding functionality instead).

I think Suka's patch is small enough that there's not a lot of pursuing going on. Dave's may_checkpoint is a lot more ambitious.

Just to be clear - are you saying you think both this patch and Dave's may_checkpoint patchsets ought to be delayed, or just Suka's, or just Dave's? :)

-serge

A message that is not in the archive (sent off-list or not yet received): 20090318135953.GE22636-r/[email protected]

#12 lore raw
Quoted 19 lines · Serge E. Hallyn
Quoted 11 lines · Oren Laadan
Quoted 9 lines · Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
Date: Fri, 13 Mar 2009 17:25:42 -0700
Subject: [PATCH 5/6] Define and use proc_pid_checkpointable()

Create a proc file, /proc/pid/checkpointable, which shows '1' if task is checkpointable and '0' if it is not.

To determine whether a task is checkpointable, the handler for this new proc file, shares the same code with sys_checkpoint().

Hey Oren,

3 counter-points:

Quoted 2 lines

I still don't understand why we would like to do it this way.

Quoted 9 lines
Quoted 2 lines

First, it makes little sense to do it per-task, because we are supposed to checkpoint an entire container.

Yes we need per-container info too. Actually, per-checkpoint-job-init, so if we send pids in for that, it should return false if we send in the pid of a task which isn't a proper checkpoint-job-init.

But we also want the info per-task, for debugging info.

My suggestions works for this two: we add a flag CR_CTX_DRYRUN; a task can ask to checkpoint itself, or another task, with CR_CTX_DRYRUN and the checkpoint code runs without actual effect. (If we don't want to expose the actual flag to userspace, then we simply use it in an implementation of a /proc/PID/checkpointable operation).

Quoted 3 lines

I don't know how to represent those two cases, though.

Also debugfs may be a more appropriate medium.

True.

Quoted 7 lines
Quoted 4 lines

Second, what's wrong with doing a "dry" checkpoint on the container (or if you prefer, the task, for what it's worth), that will not buffer nor write out any data - just say "yes" or "no" ?

You can't get a text explanation like 'fd 4 (/sys/class/net/eth0) is not checkpointable'. That's what's wrong with it.

I'm not opposing the idea of a descriptive text message - on the contrary. I suggest an alternative implementation to the approach in this patch set (or, more precisely, a generalization). And I argue that we don't need anything beyond that (a la Ingo's may_checkpoint), because it does not add enough value to justify the extra code, efforts and maintenance.

Quoted 10 lines
Quoted 7 lines

(we could use a flag "CR_CTX_DRYRUN" when calling sys_checkpoint() for this, and test for this flag in, say, kwrite/kread).

After all, we don't expect applications or users to continuously and repeatedly test if they can checkpoint, so it isn't performance critical. So we simply reuse the existing code.

No, but we do expect someone trying to checkpoint their job and failing to be curious as to why.

Which is why I think a descriptive text message is a great idea.

Quoted 8 lines
Quoted 7 lines

This would also catch cases where we can't checkpoint because the kernel is low on memory - which wouldn't show up otherwise.

And in any case, this is orthogonal to what Dave is pushing, following Ingo's comment, to know when a task _becomes_ not-checkpointable. (And in any case, I think our time is better spent on adding functionality instead).

Quoted 7 lines

I think Suka's patch is small enough that there's not a lot of pursuing going on. Dave's may_checkpoint is a lot more ambitious.

Just to be clear - are you saying you think both this patch and Dave's may_checkpoint patchsets ought to be delayed, or just Suka's, or just Dave's? :)

I think Suka's patch is the right way to go - and that we can generalize the approach to test for a "dryrun" attribute (e.g. on ctx->flags) throughout the checkpointing code.

Dave's patch has two parts now: the concept of "may_checkpoint" which, I think, is diverting our efforts from other, more important, issues. This is because knowing exactly when an application becomes 'uncheckpointable' is not that more useful then knowing why it fails to checkpoint at the time of the checkpoint. Also, there are all these issues of how to transition back into 'checkpointable' state.

The other part is the fops->checkpoint() method, which saves the (or some) state of the file, and can also be used to report "no-go", and I think that is a good idea. Moreover, it can be made to understand a "dryrun" flag and do nothing but check that a checkpoint would work...

Oren.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#14 lore raw
Quoted 5 lines · Oren Laadan

My suggestions works for this two: we add a flag CR_CTX_DRYRUN; a task can ask to checkpoint itself, or another task, with CR_CTX_DRYRUN and the checkpoint code runs without actual effect. (If we don't want to expose the actual flag to userspace, then we simply use it in an implementation of a /proc/PID/checkpointable operation).

This mostly falls short answering the question "Where/when did I go wrong?" Personally, I think that is critical for getting good bug reports out of normal Joes that might not really be interested in c/r development itself. It is like lockdep. The guys/gals posting those reports really don't know about kernel locking, but they are able to improve it anyway.

-- Dave

#15 lore raw
Quoted 7 lines · Dave Hansen
Quoted 5 lines · Oren Laadan

My suggestions works for this two: we add a flag CR_CTX_DRYRUN; a task can ask to checkpoint itself, or another task, with CR_CTX_DRYRUN and the checkpoint code runs without actual effect. (If we don't want to expose the actual flag to userspace, then we simply use it in an implementation of a /proc/PID/checkpointable operation).

Quoted 7 lines

This mostly falls short answering the question "Where/when did I go wrong?" Personally, I think that is critical for getting good bug reports out of normal Joes that might not really be interested in c/r development itself. It is like lockdep. The guys/gals posting those reports really don't know about kernel locking, but they are able to improve it anyway.

As long as we have the descriptive text accompanied, it will answer the "where/why" question.

I can't think of an example where for me, as the developer, the "when" question was important in finding and fixing an unsupported corner.

Can you think of examples where this would be that much more informative then simply answering the "where/what" question ?

Oren.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#17 lore raw
Quoted 16 lines · Oren Laadan
Quoted 7 lines · Dave Hansen
Quoted 5 lines · Oren Laadan

My suggestions works for this two: we add a flag CR_CTX_DRYRUN; a task can ask to checkpoint itself, or another task, with CR_CTX_DRYRUN and the checkpoint code runs without actual effect. (If we don't want to expose the actual flag to userspace, then we simply use it in an implementation of a /proc/PID/checkpointable operation).

Quoted 7 lines

This mostly falls short answering the question "Where/when did I go wrong?" Personally, I think that is critical for getting good bug reports out of normal Joes that might not really be interested in c/r development itself. It is like lockdep. The guys/gals posting those reports really don't know about kernel locking, but they are able to improve it anyway.

Quoted 8 lines

As long as we have the descriptive text accompanied, it will answer the "where/why" question.

I can't think of an example where for me, as the developer, the "when" question was important in finding and fixing an unsupported corner.

Can you think of examples where this would be that much more informative then simply answering the "where/what" question ?

I think it is crucially important if a ->may_checkpoint-style flag is persistent and sticky.

If we're *just* considering moment-by-moment state it doesn't matter at all.

How about I rephrase: "Where/when did I first go wrong?"

I consider that an important question to answer. What was the first thing that I did that made this thing uncheckpointable? If I want my container to always be checkpointable, how else do I find race conditions that may may it uncheckpointable for a moment?

Yep, it requires extra code. But it also completely changes the way in which we can go after features. Oh, well. You don't like it. It is different from how Zap did it. Let's drop it.

-- Dave

#18 lore raw
In reply to a message not in the archive
Quoted 19 lines · Serge E. Hallyn
Quoted 11 lines · Oren Laadan
Quoted 9 lines · Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
Date: Fri, 13 Mar 2009 17:25:42 -0700
Subject: [PATCH 5/6] Define and use proc_pid_checkpointable()

Create a proc file, /proc/pid/checkpointable, which shows '1' if task is checkpointable and '0' if it is not.

To determine whether a task is checkpointable, the handler for this new proc file, shares the same code with sys_checkpoint().

Hey Oren,

3 counter-points:

Quoted 2 lines

I still don't understand why we would like to do it this way.

Quoted 9 lines
Quoted 2 lines

First, it makes little sense to do it per-task, because we are supposed to checkpoint an entire container.

Yes we need per-container info too. Actually, per-checkpoint-job-init, so if we send pids in for that, it should return false if we send in the pid of a task which isn't a proper checkpoint-job-init.

But we also want the info per-task, for debugging info.

My suggestions works for this two: we add a flag CR_CTX_DRYRUN; a task can ask to checkpoint itself, or another task, with CR_CTX_DRYRUN and the checkpoint code runs without actual effect. (If we don't want to expose the actual flag to userspace, then we simply use it in an implementation of a /proc/PID/checkpointable operation).

Quoted 3 lines

I don't know how to represent those two cases, though.

Also debugfs may be a more appropriate medium.

Yes, I think that's better the /proc (which is then carved in stone ...)

Quoted 7 lines
Quoted 4 lines

Second, what's wrong with doing a "dry" checkpoint on the container (or if you prefer, the task, for what it's worth), that will not buffer nor write out any data - just say "yes" or "no" ?

You can't get a text explanation like 'fd 4 (/sys/class/net/eth0) is not checkpointable'. That's what's wrong with it.

I'm all for it.

Quoted 10 lines
Quoted 7 lines

(we could use a flag "CR_CTX_DRYRUN" when calling sys_checkpoint() for this, and test for this flag in, say, kwrite/kread).

After all, we don't expect applications or users to continuously and repeatedly test if they can checkpoint, so it isn't performance critical. So we simply reuse the existing code.

No, but we do expect someone trying to checkpoint their job and failing to be curious as to why.

I'm not opposing the idea of a descriptive text message - on the contrary, and I have supported this in earlier emails.

However, I suggest an alternative implementation to the approach in this patch set (or, more precisely, a generalization).

And I argue that we don't need anything beyond that (a la Ingo's may_checkpoint), because it does not add enough value to justify the extra code, efforts and maintenance.

Quoted 9 lines
Quoted 8 lines

This would also catch cases where we can't checkpoint because the kernel is low on memory - which wouldn't show up otherwise.

And in any case, this is orthogonal to what Dave is pushing, following Ingo's comment, to know when a task _becomes_ not-checkpointable. (And in any case, I think our time is better spent on adding functionality instead).

Quoted 7 lines

I think Suka's patch is small enough that there's not a lot of pursuing going on. Dave's may_checkpoint is a lot more ambitious.

Just to be clear - are you saying you think both this patch and Dave's may_checkpoint patchsets ought to be delayed, or just Suka's, or just Dave's? :)

I think Suka's patch is the right way to go - and that we can generalize the approach to test for a "dryrun" attribute (e.g. on ctx->flags) throughout the checkpointing code.

Dave's patch has two parts now: the concept of "may_checkpoint" which, I think, is diverting our efforts from other, more important, issues. This is because knowing exactly when an application becomes 'uncheckpointable' is not that more useful then knowing why it fails to checkpoint at the time of the checkpoint. Also, there are all these issues of how to transition back into 'checkpointable' state.

The other part is the fops->checkpoint() method, which saves the (or some) state of the file, and can also be used to report "no-go", and I think that is a good idea. Moreover, it can be made to understand a "dryrun" flag and do nothing but check that a checkpoint would work...

Oren.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#20 lore raw
Quoted 29 lines · Oren Laadan
Quoted 26 lines · Serge E. Hallyn
Quoted 11 lines · Oren Laadan
Quoted 9 lines · Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
Date: Fri, 13 Mar 2009 17:25:42 -0700
Subject: [PATCH 5/6] Define and use proc_pid_checkpointable()

Create a proc file, /proc/pid/checkpointable, which shows '1' if task is checkpointable and '0' if it is not.

To determine whether a task is checkpointable, the handler for this new proc file, shares the same code with sys_checkpoint().

Hey Oren,

3 counter-points:

Quoted 4 lines

I still don't understand why we would like to do it this way.

First, it makes little sense to do it per-task, because we are supposed to checkpoint an entire container.

Yes we need per-container info too. Actually, per-checkpoint-job-init, so if we send pids in for that, it should return false if we send in the pid of a task which isn't a proper checkpoint-job-init.

Quoted 8 lines
Quoted 2 lines

But we also want the info per-task, for debugging info.

My suggestions works for this two: we add a flag CR_CTX_DRYRUN; a task can ask to checkpoint itself, or another task, with CR_CTX_DRYRUN and the checkpoint code runs without actual effect. (If we don't want to expose the actual flag to userspace, then we simply use it in an implementation of a /proc/PID/checkpointable operation).

Hmm, so if we pass in CR_CTX_DRYRUN, then the fd can point to a file wherein to store a text represenation of the reason?

Dave will probably hate it, but it could be worse...

-serge

A message that is not in the archive (sent off-list or not yet received): 20090318171840.GA29523-r/[email protected]

#22 lore raw
Quoted 34 lines · Serge E. Hallyn
Quoted 25 lines · Oren Laadan
Quoted 23 lines · Serge E. Hallyn
Quoted 10 lines · Oren Laadan
Quoted 9 lines · Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
Date: Fri, 13 Mar 2009 17:25:42 -0700
Subject: [PATCH 5/6] Define and use proc_pid_checkpointable()

Create a proc file, /proc/pid/checkpointable, which shows '1' if task is checkpointable and '0' if it is not.

To determine whether a task is checkpointable, the handler for this new proc file, shares the same code with sys_checkpoint().

Hey Oren,

3 counter-points:

Quoted 4 lines

I still don't understand why we would like to do it this way.

First, it makes little sense to do it per-task, because we are supposed to checkpoint an entire container.

Yes we need per-container info too. Actually, per-checkpoint-job-init, so if we send pids in for that, it should return false if we send in the pid of a task which isn't a proper checkpoint-job-init.

Quoted 7 lines
Quoted 2 lines

But we also want the info per-task, for debugging info.

My suggestions works for this two: we add a flag CR_CTX_DRYRUN; a task can ask to checkpoint itself, or another task, with CR_CTX_DRYRUN and the checkpoint code runs without actual effect. (If we don't want to expose the actual flag to userspace, then we simply use it in an implementation of a /proc/PID/checkpointable operation).

Quoted 4 lines

Hmm, so if we pass in CR_CTX_DRYRUN, then the fd can point to a file wherein to store a text represenation of the reason?

Dave will probably hate it, but it could be worse...

Either that.

Or continue using a debugfs interface, except that the implementation will go through an internal interface as I suggest.

Or spit the reason on the kernel console so the user can check dmesg for it (like when 'modprobe' fails).

Oren.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#24 lore raw
Quoted 39 lines · Oren Laadan
Quoted 32 lines · Serge E. Hallyn
Quoted 24 lines · Oren Laadan
Quoted 23 lines · Serge E. Hallyn
Quoted 10 lines · Oren Laadan
Quoted 9 lines · Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
Date: Fri, 13 Mar 2009 17:25:42 -0700
Subject: [PATCH 5/6] Define and use proc_pid_checkpointable()

Create a proc file, /proc/pid/checkpointable, which shows '1' if task is checkpointable and '0' if it is not.

To determine whether a task is checkpointable, the handler for this new proc file, shares the same code with sys_checkpoint().

Hey Oren,

3 counter-points:

Quoted 4 lines

I still don't understand why we would like to do it this way.

First, it makes little sense to do it per-task, because we are supposed to checkpoint an entire container.

Yes we need per-container info too. Actually, per-checkpoint-job-init, so if we send pids in for that, it should return false if we send in the pid of a task which isn't a proper checkpoint-job-init.

Quoted 7 lines
Quoted 2 lines

But we also want the info per-task, for debugging info.

My suggestions works for this two: we add a flag CR_CTX_DRYRUN; a task can ask to checkpoint itself, or another task, with CR_CTX_DRYRUN and the checkpoint code runs without actual effect. (If we don't want to expose the actual flag to userspace, then we simply use it in an implementation of a /proc/PID/checkpointable operation).

Quoted 4 lines

Hmm, so if we pass in CR_CTX_DRYRUN, then the fd can point to a file wherein to store a text represenation of the reason?

Dave will probably hate it, but it could be worse...

Quoted 7 lines

Either that.

Or continue using a debugfs interface, except that the implementation will go through an internal interface as I suggest.

Or spit the reason on the kernel console so the user can check dmesg for it (like when 'modprobe' fails).

It'd be nice if the error message could be gotten directly from the call. Would something like a new packet in the output stream (cr_hdr->type == CR_HDR_FAILURE) with something descriptive in the body of the packet make sense? That could then be scanned for when sys_checkpoint fails..

Polluting the dmesg buffer with messages from common failures (consider a multi-user cluster where checkpoints may or may not succeed) isn't very useful.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#26 lore raw
Quoted 3 lines · Mike Waychison

Polluting the dmesg buffer with messages from common failures (consider a multi-user cluster where checkpoints may or may not succeed) isn't very useful.

Yeah, I've already gotten an earful from Serge and Dan S. about this. :)

Serge suggested that, perhaps, the audit framework could be used. We might also use an ftrace buffer if we want to keep a whole ton of messages around, too.

dmesg is definitely not workable long-term at all.

-- Dave

#27 lore raw
Quoted 5 lines · Dave Hansen
Quoted 3 lines · Mike Waychison

Polluting the dmesg buffer with messages from common failures (consider a multi-user cluster where checkpoints may or may not succeed) isn't very useful.

Quoted 7 lines

Yeah, I've already gotten an earful from Serge and Dan S. about this. :)

Serge suggested that, perhaps, the audit framework could be used. We might also use an ftrace buffer if we want to keep a whole ton of messages around, too.

dmesg is definitely not workable long-term at all.

How about having place holder objects in the generated checkpoint. Then instead of having a failure you have a non-restoreable checkpoint. But you know which fd, or which mmaped region, or which other thing is causing the problem and if you want more information you can look at that resource.

That gives user space the freedom and scrub out the non-checkpointable bits and replace them with something like /dev/null so that we can continue on and restore the checkpoint anyway, if we think our app can cope with some things going away.

Eric

A message that is not in the archive (sent off-list or not yet received): m17i2dx00b.fsf-+imSwln9KH6u2/kzUuoCbdi2O/[email protected]

#29 lore raw
Quoted 21 lines · Eric W. Biederman
Quoted 5 lines · Dave Hansen
Quoted 3 lines · Mike Waychison

Polluting the dmesg buffer with messages from common failures (consider a multi-user cluster where checkpoints may or may not succeed) isn't very useful.

Quoted 7 lines

Yeah, I've already gotten an earful from Serge and Dan S. about this. :)

Serge suggested that, perhaps, the audit framework could be used. We might also use an ftrace buffer if we want to keep a whole ton of messages around, too.

dmesg is definitely not workable long-term at all.

How about having place holder objects in the generated checkpoint. Then instead of having a failure you have a non-restoreable checkpoint. But you know which fd, or which mmaped region, or which other thing is causing the problem and if you want more information you can look at that resource.

Quoted 6 lines

That gives user space the freedom and scrub out the non-checkpointable bits and replace them with something like /dev/null so that we can continue on and restore the checkpoint anyway, if we think our app can cope with some things going away.

Eric

I like this idea.

Subystems which are temporarily entirely unsupported (like sysvipc) would need at least a dummy section in the format wherein we can at least say 'unsupported', otherwise we'll still just get a meaningless -EINVAL.

I actually got bitten yesterday by trying to checkpoint a task that wasn't frozen. I forgot v14 had that check, and my failures (a segfault actually) weren't helpful.

-serge

A message that is not in the archive (sent off-list or not yet received): 20090325172938.GA18957-r/[email protected]

#31 lore raw
Quoted 20 lines · Serge E. Hallyn
Quoted 19 lines · Eric W. Biederman
Quoted 11 lines · Dave Hansen
Quoted 3 lines · Mike Waychison

Polluting the dmesg buffer with messages from common failures (consider a multi-user cluster where checkpoints may or may not succeed) isn't very useful.

Yeah, I've already gotten an earful from Serge and Dan S. about this. :)

Serge suggested that, perhaps, the audit framework could be used. We might also use an ftrace buffer if we want to keep a whole ton of messages around, too.

dmesg is definitely not workable long-term at all.

How about having place holder objects in the generated checkpoint. Then instead of having a failure you have a non-restoreable checkpoint. But you know which fd, or which mmaped region, or which other thing is causing the problem and if you want more information you can look at that resource.

Quoted 8 lines
Quoted 6 lines

That gives user space the freedom and scrub out the non-checkpointable bits and replace them with something like /dev/null so that we can continue on and restore the checkpoint anyway, if we think our app can cope with some things going away.

Eric

I like this idea.

yes. This is something required to replace stdios for example, when you execute an application under ssh, checkpoint and then restart on an other host. This a topical scenario for a batch manager in an HPC environment.

identified resources of the container are tracked to be ignored by checkpoint and to be replaced by similar ones at restart.

C.

Quoted 15 lines

Subystems which are temporarily entirely unsupported (like sysvipc) would need at least a dummy section in the format wherein we can at least say 'unsupported', otherwise we'll still just get a meaningless -EINVAL.

I actually got bitten yesterday by trying to checkpoint a task that wasn't frozen. I forgot v14 had that check, and my failures (a segfault actually) weren't helpful.

-serge
_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

A message that is not in the archive (sent off-list or not yet received): [email protected]

#33 lore raw
Quoted 30 lines · Cedric Le Goater
Quoted 20 lines · Serge E. Hallyn
Quoted 19 lines · Eric W. Biederman
Quoted 11 lines · Dave Hansen
Quoted 3 lines · Mike Waychison

Polluting the dmesg buffer with messages from common failures (consider a multi-user cluster where checkpoints may or may not succeed) isn't very useful.

Yeah, I've already gotten an earful from Serge and Dan S. about this. :)

Serge suggested that, perhaps, the audit framework could be used. We might also use an ftrace buffer if we want to keep a whole ton of messages around, too.

dmesg is definitely not workable long-term at all.

How about having place holder objects in the generated checkpoint. Then instead of having a failure you have a non-restoreable checkpoint. But you know which fd, or which mmaped region, or which other thing is causing the problem and if you want more information you can look at that resource.

Quoted 8 lines
Quoted 6 lines

That gives user space the freedom and scrub out the non-checkpointable bits and replace them with something like /dev/null so that we can continue on and restore the checkpoint anyway, if we think our app can cope with some things going away.

Eric

I like this idea.

Quoted 7 lines

yes. This is something required to replace stdios for example, when you execute an application under ssh, checkpoint and then restart on an other host. This a topical scenario for a batch manager in an HPC environment.

identified resources of the container are tracked to be ignored by checkpoint and to be replaced by similar ones at restart.

So in that case how are the resources identified? Does the user specify them at checkpoint? Do you look for specific strings (/dev/pts/*) at restart?

-serge

#34 lore raw
In reply to a message not in the archive
Quoted 7 lines · On Wed, 2009-03-18 at 04:55 -0400, Oren Laadan wrote: Bhattiprolu wrote:
Quoted 7 lines

From: Sukadev Bhattiprolu <[email protected]>
Date: Fri, 13 Mar 2009 17:25:42 -0700
Subject: [PATCH 5/6] Define and use proc_pid_checkpointable()

Create a proc file, /proc/pid/checkpointable, which shows '1' if task is checkpointable and '0' if it is not.

Quoted 7 lines
Quoted 2 lines

To determine whether a task is checkpointable, the handler for this new proc file, shares the same code with sys_checkpoint().

I still don't understand why we would like to do it this way.

First, it makes little sense to do it per-task, because we are supposed to checkpoint an entire container.

I take it you didn't like the 'files' or 'uts' versions of this, either. Do we have a container object in the kernel that we could tag, anyway?

Quoted 10 lines

Second, what's wrong with doing a "dry" checkpoint on the container (or if you prefer, the task, for what it's worth), that will not buffer nor write out any data - just say "yes" or "no" ?

(we could use a flag "CR_CTX_DRYRUN" when calling sys_checkpoint() for this, and test for this flag in, say, kwrite/kread).

After all, we don't expect applications or users to continuously and repeatedly test if they can checkpoint, so it isn't performance critical. So we simply reuse the existing code.

Heh. Did you read Ingo's suggestions? He suggested *exactly* that.

http://lkml.org/lkml/2008/10/9/196

Quoted 9 lines

This would also catch cases where we can't checkpoint because the kernel is low on memory - which wouldn't show up otherwise.

And in any case, this is orthogonal to what Dave is pushing, following Ingo's comment, to know when a task _becomes_ not-checkpointable. (And in any case, I think our time is better spent on adding functionality instead).

Oren.

-- Dave

#35 lore raw
In reply to a message not in the archive

From: Sukadev Bhattiprolu <[email protected]>
Date: Sat, 14 Mar 2009 10:21:07 -0700
Subject: [PATCH 6/6] Explain reason for task being uncheckpointable

Try to give an useful message on why a task is uncheckpointable.

---
 checkpoint/checkpoint.c    |   13 +++++++++----
 fs/proc/base.c             |   19 ++++++++++++++++++-
 include/linux/checkpoint.h |    7 +++++++
 3 files changed, 34 insertions(+), 5 deletions(-)
checkpoint/checkpoint.c +9 -4
index ad0956c..c8c377d 100644
@@ -264,18 +264,23 @@ static int cr_write_all_tasks(struct cr_ctx *ctx)
 	return ret;
 }
 
-int task_checkpointable(struct task_struct *t)
+int __task_checkpointable(struct task_struct *t)
 {
 	if (t->state == TASK_DEAD) {
 		pr_warning("c/r: task %d is TASK_DEAD\n", task_pid_vnr(t));
-		return 0;
+		return CR_DEAD;
 	}
 
 	/* Verify that task is frozen, unless it is self-checkpoint */
 	if (t != current && !frozen(t))
-		return 0;
+		return CR_NOT_FROZEN;
+
+	return CR_CHECKPOINTABLE;
+}
 
-	return 1;
+int task_checkpointable(struct task_struct *t)
+{
+	return __task_checkpointable(t) == CR_CHECKPOINTABLE;
 }
 
 static int cr_may_checkpoint_task(struct task_struct *t, struct cr_ctx *ctx)
fs/proc/base.c +18 -1
index 989ca93..6e4c07c 100644
@@ -2502,9 +2502,26 @@ static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns,
 }
 
 #ifdef CONFIG_CHECKPOINT_RESTART
+static char *explain_checkpointable(int reason)
+{
+	char *message;
+
+	switch(reason) {
+	case CR_CHECKPOINTABLE: message = "1"; break;
+	case CR_DEAD: 		message = "0 (dead)"; break;
+	case CR_NOT_FROZEN: 	message = "0 (not frozen)"; break;
+	default:		message = "0 (unknown)"; break;
+	}
+	return message;
+}
+
 static int proc_pid_checkpointable(struct task_struct *task, char *buffer)
 {
-	return sprintf(buffer, "%d\n", task_checkpointable(task));
+	int rc;
+
+	rc = __task_checkpointable(task);
+
+	return scnprintf(buffer, PAGE_SIZE, "%s\n", explain_checkpointable(rc));
 }
 #endif
 
include/linux/checkpoint.h +7 -0
index d1f53a6..a1eba73 100644
@@ -142,6 +142,13 @@ static inline int cr_enabled(void)
 	return 1;
 }
 
+enum cr_uncheckpointable_reason {
+	CR_CHECKPOINTABLE = 0,
+	CR_DEAD,
+	CR_NOT_FROZEN
+};
+
+int __task_checkpointable(struct task_struct *t);
 int task_checkpointable(struct task_struct *t);
 
 #else /* !CONFIG_CHECKPOINT_RESTART */
Signature · 1 line
1.5.2.5

A message that is not in the archive (sent off-list or not yet received): 20090317063958.GG2377-r/[email protected]

#37 lore raw
Quoted 13 lines · Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
Date: Sat, 14 Mar 2009 10:21:07 -0700
Subject: [PATCH 6/6] Explain reason for task being uncheckpointable

Try to give an useful message on why a task is uncheckpointable.

---
 checkpoint/checkpoint.c    |   13 +++++++++----
 fs/proc/base.c             |   19 ++++++++++++++++++-
 include/linux/checkpoint.h |    7 +++++++
 3 files changed, 34 insertions(+), 5 deletions(-)

[...]

fs/proc/base.c static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns, · line 2512

Quoted 9 lines

#ifdef CONFIG_CHECKPOINT_RESTART

fs/proc/base.c +8 -0
+static char *explain_checkpointable(int reason)
+{
+	char *message;
+
+	switch(reason) {
+	case CR_CHECKPOINTABLE: message = "1"; break;
+	case CR_DEAD: 		message = "0 (dead)"; break;
+	case CR_NOT_FROZEN: 	message = "0 (not frozen)"; break;

Hmmm... so a task cannot test if it itself is checkpointable, because it will never be frozen and will fail right there before proceeding to addition (future) tests ?

[...]

Oren.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#39 lore raw
Quoted 19 lines · Oren Laadan
Quoted 13 lines · Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
Date: Sat, 14 Mar 2009 10:21:07 -0700
Subject: [PATCH 6/6] Explain reason for task being uncheckpointable

Try to give an useful message on why a task is uncheckpointable.

---
 checkpoint/checkpoint.c    |   13 +++++++++----
 fs/proc/base.c             |   19 ++++++++++++++++++-
 include/linux/checkpoint.h |    7 +++++++
 3 files changed, 34 insertions(+), 5 deletions(-)

[...]

Quoted 13 lines

fs/proc/base.c static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns, · line 2512

Quoted 9 lines

#ifdef CONFIG_CHECKPOINT_RESTART

fs/proc/base.c +8 -0
+static char *explain_checkpointable(int reason)
+{
+	char *message;
+
+	switch(reason) {
+	case CR_CHECKPOINTABLE: message = "1"; break;
+	case CR_DEAD: 		message = "0 (dead)"; break;
+	case CR_NOT_FROZEN: 	message = "0 (not frozen)"; break;

Hmmm... so a task cannot test if it itself is checkpointable, because it will never be frozen and will fail right there before proceeding to addition (future) tests ?

No, no... :

/* Verify that task is frozen, unless it is self-checkpoint */ if (t != current && !frozen(t))
- return 0;
+ return CR_NOT_FROZEN;

-serge