Review: Can we move init_restart_ctx() up ?

PATCH Sukadev Bhattiprolu 2 participants 3 messages Oct 2, 2009 – Oct 3, 2009 2 messages not in the archive

Share & tools

Sukadev Bhattiprolu

author

2009-10-02 05:14 UTC #0 lore raw

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

checkpoint/restart.c +3 -7

Index: linux-cr/checkpoint/restart.c
===================================================================
@@ -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

checkpoint/sys.c +9 -4

Index: linux-cr/checkpoint/sys.c
===================================================================
@@ -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;

include/linux/checkpoint.h +1 -1

Index: linux-cr/include/linux/checkpoint.h
===================================================================
@@ -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);

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

Oren Laadan

2009-10-02 18:26 UTC #2 lore raw

Quoted 7 lines · 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.

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

Quoted 2 lines

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.

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

Sukadev Bhattiprolu

author

2009-10-03 01:48 UTC #4 lore raw

Oren Laadan [[email protected]] 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