Review: Can we move init_restart_ctx() up ?

Review: Can we move init_restart_ctx() up ?

[Sukadev Bhattiprolu]

Would it be easier/cleaner if we initialized the restart ctx soon after
allocating it ? Looks like we already have the information we need at that
point.

That way, do_restart() won't need the 'pid' parameter and we anyway use
the 'if(ctx)' check to test for the coordinator.

BTW, can we move the '->active_pid = -1' in init_restart_ctx() to
ckpt_ctx_alloc() itself ?

(This is mostly as a code-review comment, have not tested the patch yet).

---
 checkpoint/restart.c       |   10 +++-------
 checkpoint/sys.c           |   13 +++++++++----
 include/linux/checkpoint.h |    2 +-
 3 files changed, 13 insertions(+), 12 deletions(-)

Index: linux-cr/checkpoint/restart.c
===================================================================
--- linux-cr.orig/checkpoint/restart.c	2009-10-01 21:42:21.000000000 -0700
+++ linux-cr/checkpoint/restart.c	2009-10-01 21:44:24.000000000 -0700
@@ -897,7 +897,7 @@ static int init_restart_ctx(struct ckpt_
 	return 0;
 }
 
-static int do_restore_coord(struct ckpt_ctx *ctx, pid_t pid)
+static int do_restore_coord(struct ckpt_ctx *ctx)
 {
 	struct ckpt_ctx *old_ctx;
 	int ret;
@@ -912,10 +912,6 @@ static int do_restore_coord(struct ckpt_
 	if ((ctx->uflags & RESTART_TASKSELF) && ctx->nr_pids != 1)
 		return -EINVAL;
 
-	ret = init_restart_ctx(ctx, pid);
-	if (ret < 0)
-		return ret;
-
 	/*
 	 * Populate own ->checkpoint_ctx: if an ancestor attempts to
 	 * prepare_descendants() on us, it will fail. Furthermore,
@@ -1033,12 +1029,12 @@ static long restore_retval(void)
 	return ret;
 }
 
-long do_restart(struct ckpt_ctx *ctx, pid_t pid, unsigned long flags)
+long do_restart(struct ckpt_ctx *ctx, unsigned long flags)
 {
 	long ret;
 
 	if (ctx)
-		ret = do_restore_coord(ctx, pid);
+		ret = do_restore_coord(ctx);
 	else if (flags & RESTART_GHOST)
 		ret = do_ghost_task();
 	else
Index: linux-cr/checkpoint/sys.c
===================================================================
--- linux-cr.orig/checkpoint/sys.c	2009-10-01 21:39:43.000000000 -0700
+++ linux-cr/checkpoint/sys.c	2009-10-01 21:42:07.000000000 -0700
@@ -330,12 +330,17 @@ SYSCALL_DEFINE3(restart, pid_t, pid, int
 	if (!ckpt_unpriv_allowed && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (pid)
+	if (pid) {
 		ctx = ckpt_ctx_alloc(fd, flags, CKPT_CTX_RESTART);
-	if (IS_ERR(ctx))
-		return PTR_ERR(ctx);
+		if (IS_ERR(ctx))
+			return PTR_ERR(ctx);
 
-	ret = do_restart(ctx, pid, flags);
+		ret = init_restart_ctx(ctx, pid);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = do_restart(ctx, flags);
 
 	ckpt_ctx_put(ctx);
 	return ret;
Index: linux-cr/include/linux/checkpoint.h
===================================================================
--- linux-cr.orig/include/linux/checkpoint.h	2009-10-01 21:44:54.000000000 -0700
+++ linux-cr/include/linux/checkpoint.h	2009-10-01 21:45:01.000000000 -0700
@@ -138,7 +138,7 @@ extern void ckpt_ctx_get(struct ckpt_ctx
 extern void ckpt_ctx_put(struct ckpt_ctx *ctx);
 
 extern long do_checkpoint(struct ckpt_ctx *ctx, pid_t pid);
-extern long do_restart(struct ckpt_ctx *ctx, pid_t pid, unsigned long flags);
+extern long do_restart(struct ckpt_ctx *ctx, unsigned long flags);
 
 /* task */
 extern int ckpt_activate_next(struct ckpt_ctx *ctx);

Re: Review: Can we move init_restart_ctx() up ?

[Oren Laadan]

Sukadev Bhattiprolu wrote:
> 
> Would it be easier/cleaner if we initialized the restart ctx soon after
> allocating it ? Looks like we already have the information we need at that
> point.
> 
> That way, do_restart() won't need the 'pid' parameter and we anyway use
> the 'if(ctx)' check to test for the coordinator.

I put the {checkpoint,restart}-related work in {checkpoint,restart}.c,
and leave sys.c "neutral".

I also like the symmetry between do_checkpoint() and do_restart() :p

> BTW, can we move the '->active_pid = -1' in init_restart_ctx() to
> ckpt_ctx_alloc() itself ?

@active_pid is specific to restart, that's why I initialize it there.

Oren.

Re: Review: Can we move init_restart_ctx() up ?

[Sukadev Bhattiprolu]

Oren Laadan [orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org] wrote:
| 
| 
| Sukadev Bhattiprolu wrote:
| > 
| > Would it be easier/cleaner if we initialized the restart ctx soon after
| > allocating it ? Looks like we already have the information we need at that
| > point.
| > 
| > That way, do_restart() won't need the 'pid' parameter and we anyway use
| > the 'if(ctx)' check to test for the coordinator.
| 
| I put the {checkpoint,restart}-related work in {checkpoint,restart}.c,
| and leave sys.c "neutral".
| 
| I also like the symmetry between do_checkpoint() and do_restart() :p
| 
| > BTW, can we move the '->active_pid = -1' in init_restart_ctx() to
| > ckpt_ctx_alloc() itself ?
| 
| @active_pid is specific to restart, that's why I initialize it there.

Ok. Spreading out the initialization code over different functions is a bit
confusing.  Maybe we could call init_checkpoint_ctx() or init_restart_ctx()
from ckpt_ctx_alloc() (depending on the kflags parameter) - and keep the
symmetry/nuetrality, But not a big deal.

Thanks,

Sukadev