Re: [RFC][v4][PATCH 0/7] clone_with_pids() system call

[Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>]

Eric W. Biederman [ebiederm@xmission.com] wrote:

| > But last_pid is from the pid_ns. Do you mean to have alloc_pidmap()
| > take a pid_min and pid_max and when choosing a specific pid, have
| > pid_min == pid_max == target_pid ?
| 
| Yes. It already takes a pid_min and a pid_max from the environment.
| I guess the pid_min is RESERVED_PIDS by default.

Well, defining alloc_pidmap() as:

	int alloc_pidmap(pid_ns, int min, int max)

seems to unnecessarily complicate alloc_pidmap() - what if 'min' is 0
but 'max' is not or vice-versa. Generalizing alloc_pidmap() to handle
all combinations seems like an overkill and/or expose RESERVED_PIDS and
pid_max caller.

Maybe we can drop the set_pidmap() call by sticking to 

	int alloc_pidmap(pid_ns, target_pid)

and setting 'max_scan' to 1 when target_pid is set (see quick patch below).

| 
| > | No changes to copy_process are needed it already takes a struct pid
| > | argument.
| >
| >
| > I see your point about passing in both 'struct pid*' and target_pids[].
| > But in the common case the struct pid passed into copy_process() is
| > NULL - allocating pid in do_fork() would significantly alter the
| > existing control flow - no ? alloc_pid() assumes any new pid namespace
| > has been created - in copy_namespaces(). Moving the alloc_pid() to
| > do_fork() would require parsing clone_flags in do_fork() and pulling
| > pid namespace code out of copy_namespaces().
| 
| Why change do_fork?

Sorry, maybe I am missing something. If we don't pass target_pids as a
parameter to copy_process(), how do we specify the target pids ?
Fill in a dummy struct pid with the target-pids and pass it into
copy_process() ?

| 
| > | I haven't been following closely what is gained by having a clone_with_pids
| > | syscall?  
| >
| > When restarting an application from a checkpoint, the application must get
| > the same pid it had at the time of checkpoint. clone_with_pids() would be
| > used during restart so the child can be created with a specific set of pids.
| 
| That part I understand.  What I don't understand is why have that one part be
| special and have user space do the work?

By 'work' do you mean the rest of the process-restart logic ?

The user-level restart program creates the necessary process using
clone_with_pids() and each child process calls another system call,
sys_restart() which restores the process state.

Sukadev

---

Index: linux-2.6/kernel/pid.c
===================================================================
--- linux-2.6.orig/kernel/pid.c	2009-08-17 18:43:15.000000000 -0700
+++ linux-2.6/kernel/pid.c	2009-08-17 19:41:57.000000000 -0700
@@ -122,18 +122,29 @@
 	atomic_inc(&map->nr_free);
 }
 
-static int alloc_pidmap(struct pid_namespace *pid_ns)
+static int alloc_pidmap(struct pid_namespace *pid_ns, int target_pid)
 {
 	int i, offset, max_scan, pid, last = pid_ns->last_pid;
 	struct pidmap *map;
 	int rc;
 
-	pid = last + 1;
-	if (pid >= pid_max)
-		pid = RESERVED_PIDS;
+	if (target_pid) {
+		if (target_pid < 0 || target_pid >= pid_max)
+			return -EINVAL;
+		pid = target_pid;
+		max_scan = 1;
+	} else {
+		pid = last + 1;
+		if (pid >= pid_max)
+			pid = RESERVED_PIDS;
+	}
+
 	offset = pid & BITS_PER_PAGE_MASK;
 	map = &pid_ns->pidmap[pid/BITS_PER_PAGE];
+
 	max_scan = (pid_max + BITS_PER_PAGE - 1)/BITS_PER_PAGE - !offset;
+	if (target_pid)
+		max_scan = 1;
 
 	rc = -EAGAIN;
 	for (i = 0; i <= max_scan; ++i) {
@@ -258,7 +269,7 @@
 
 	tmp = ns;
 	for (i = ns->level; i >= 0; i--) {
-		nr = alloc_pidmap(tmp);
+		nr = alloc_pidmap(tmp, 0);
 		if (nr < 0)
 			goto out_free;