Sukadev Bhattiprolu wrote:
> Couple of nits and couple of not-so minor comments
>
> Oren Laadan [or...@cs.columbia.edu] wrote:
> | From 7162fef93ee3d9fd30a457dd7b0c7ad0200d5bcb Mon Sep 17 00:00:00 2001
> | From: Oren Laadan <or...@cs.columbia.edu>
> | Date: Mon, 30 Mar 2009 15:06:13 -0400
> | Subject: [PATCH 15/29] 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.
> |
[...]
> |
> | struct cr_ctx {
> | int crid; /* unique checkpoint id */
> | @@ -31,8 +34,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 */
> |
> | @@ -40,6 +42,19 @@ 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 */
>
> Nit: Since we already have a pid_nr() that refers to something different,
> can we call this 'nr_pids' (and nr_tasks above) like mm_context->nr_threads ?
> Of course, there is no convention, so its easy to argue the other way.
Ok.
>
> Secondly, isn't pids_nr same as tasks_nr ? If so do we need both ?
As the comment says: one is used exclusively for checkpoint and the
other exclusively for restart.
So we don't strictly need both. I thought that for readability of it's
useful to have @pids_nr (ok, @nr_pids ...) when dealing with a @pids_arr,
and a @tasks_nr (ok .. @nr_tasks ...) when dealing with @tasks_arr.
>
> Or is this intended to address the issue of multiple pid_nr values that a
> task in a nested container can have ? If so, pids_nr is > tasks_nr and that
> brings up two comments :-)
Ugh. This topic is TBD.
>
> First, mktree.c and cr_next_task() are using 'ctx->pids_nr' to determine how
> many tasks to start. If we are talking about nested containers, pids_nr
> will be greater than tasks_nr so, mktree and cr_next_task() should be
> use 'ctx->tasks_nr' to determine how many tasks to create. Also if
> checkpointing a nested container we should view the multiple nested pid
> values a process as an attribute of the task and maybe save them in
> cr_write_task() rather than in cr_write_tree().
Lol .. who's talking about nested containers ? ;)
(seriously: I'm not considering that now; my gut feeling is that it may
be useful to do pid_ns in userspace, like task creation - and in that
case it makes sense to keep it in cr_write_tree(). then again, I have
not looked at it in depth).
>
> My second comment is more an orthogonal question. Suppose init_pid_ns = level
> 0 and we have a container that is nested at level 3. If we checkpoint just
> this container, we would want to be able to restore this container at any
> level
> 0 right ?
True. Do you see any limitation in the current code that prevents this ?
>
> | + int pids_pos; /* position pids array */
> | + pid_t pids_active; /* pid of (next) active task */
>
> Do we need both pids_pos and pids_active in the ctx ? Can pids_active
> just be a local variable in cr_next_task() and cr_wait_task() ?
> IOW, isn't this always true
>
> pids_arr[pids_pos] == pids_active
Ok.
Oren.
>
> | + atomic_t tasks_count; /* sync of tasks: used to coordinate */
>
> Name is a bit confusing with 'tasks_nr', but the comment helps and I can't
> think of a better name.
>
> | + struct completion complete; /* container root and other tasks on */
> | + wait_queue_head_t waitq; /* start, end, and restart ordering */
> | };
>
> Sukadev
>
_______________________________________________
Containers mailing list
contain...@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
_______________________________________________
Devel mailing list
Devel@openvz.org
https://openvz.org/mailman/listinfo/devel
