lkml.org 
[lkml]   [2009]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC][v4][PATCH 0/7] clone_with_pids() system call
    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;


    \
     
     \ /
      Last update: 2009-08-18 05:33    [W:0.024 / U:119.680 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site