[RFC][PATCH] Check may_checkpoint() early

[RFC][PATCH] Check may_checkpoint() early

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Date: Mon, 16 Feb 2009 12:05:50 -0800
Subject: [PATCH] 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.

Signed-off-by: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 checkpoint/checkpoint.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
index 64155de..2bbb409 100644
--- a/checkpoint/checkpoint.c
+++ b/checkpoint/checkpoint.c
@@ -347,11 +347,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 so then try again later */
 			if (nr == tasks_nr) {
@@ -409,6 +416,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);
-- 
1.5.2.5

Re: [RFC][PATCH] Check may_checkpoint() early

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> 
> From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Date: Mon, 16 Feb 2009 12:05:50 -0800
> Subject: [PATCH] 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

Yes we'll at least need to add checks that there is no running task
sharing an mm or files_struct with a checkpointed task, else some of
our checks will definately be racy (and are even now).

> can remove that later.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  checkpoint/checkpoint.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
> index 64155de..2bbb409 100644
> --- a/checkpoint/checkpoint.c
> +++ b/checkpoint/checkpoint.c
> @@ -347,11 +347,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 so then try again later */
>  			if (nr == tasks_nr) {
> @@ -409,6 +416,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);
> -- 
> 1.5.2.5