• [PATCH v2 2/2] pid: Remove pidhash

    From Rik van Riel@21:1/5 to Oleg Nesterov on Mon Oct 2 22:50:08 2017
    On Mon, 2017-10-02 at 17:21 +0200, Oleg Nesterov wrote:
    Hi Rik,

    On 10/02, Rik van Riel wrote:

    Gargi and I are looking at that code, and trying to figure out
    exactly what needs to be done to make all of this correct.

    see another email I sent to Gargi a minute ago,

    2) With pid_ns_prepare_proc out of the way, we can put all the code    from below where the call to pid_ns_prepare_proc is now (except    error handing) into the main loop of pid allocation, so we can    do all that stuff under the pidmap_lock:

       for (i = ns->level; i >= 0; i--) {
           ...
           idr_alloc_cyclic(...)
           get_pid_ns(ns);
           atomic_set(&pid->count, 1);
           for (...)
                INIT_HLIST_HEAD(...)        ns->nr_allocated++;
           ...
      }

    I do not see how this can fix the problem with not-fully-initialized
    pid returned by find_pid_ns().

    As for PIDNS_ADDING/PIDNS_HASH_ADDING, _perhaps_ we can cleanup this
    logic
    a bit and do the check earlier, but imo this needs another/separate
    change.

    I'd suggest to keep the current logic and the order of initialization
    and
    just do

    for (i = ns->level; i >= 0; i--) {
    ...

    // do not expose the new pid to find_pid_ns() until it
    // is fully initialized
    nr = idr_alloc_cyclic(&tmp->idr, /*pid*/ NULL, ...);
    ...
    }

    ...

    spin_lock_irq(&pidmap_lock);
    if (!(ns->nr_hashed & PIDNS_HASH_ADDING))
    goto out_unlock;
    for ( ; upid >= pid->numbers; --upid) {
    - hlist_add_head_rcu(&upid->pid_chain,
    - &pid_hash[pid_hashfn(upid->nr, upid-
    ns)]);
    + // finally make it visible to find_pid_ns()
    + idr_replace(upid->ns-idr, pid, upid->nr);
    upid->ns->nr_hashed++;
    }
    spin_unlock_irq(&pidmap_lock);

    Or I missed something?

    You are right, that would both fix the problem, and keep the error
    paths relatively simple.

    Gargi, what do you think?

    --
    All Rights Reversed.
    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v2

    iQEcBAABCAAGBQJZ0lnEAAoJEM553pKExN6Da9sH/jj7BKzia+i1ManiiynqN1JR h5hP2mIWDPhbGsC1r4z9MFH+dsEdojZnNgwK8dhg37UYUx9umSjOpKkwrJQ58feL uHMLLIz9l8/+btgdkKvWyiUfOhNMpkVXs7iFiwRf5x8z9xaBpBlYYqz99l+Ta+RS RXSdTikfRiXthwdmK1972W0rPfNEEkWkYWbU3qrb9c409aNULaRgel2HwPJNESvY LA9c1/8P09KNki1wcjRBvodAZWxlWuEvDR8uDrQkVKzBVhjKK+6eIiep550gi10R 2qHvMOcZdFRsjHxNHfF0n3StpEONHx6fTho1HGIFUOE3+OyK1OXyKEBYSE88OWs=
    =2L+B
    -----END PGP SIGNATURE-----

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Gargi Sharma@21:1/5 to Rik van Riel on Mon Oct 2 22:50:15 2017
    On Mon, Oct 2, 2017 at 8:52 PM, Rik van Riel <riel@surriel.com> wrote:
    On Mon, 2017-10-02 at 17:21 +0200, Oleg Nesterov wrote:
    Hi Rik,

    On 10/02, Rik van Riel wrote:

    Gargi and I are looking at that code, and trying to figure out
    exactly what needs to be done to make all of this correct.

    see another email I sent to Gargi a minute ago,

    2) With pid_ns_prepare_proc out of the way, we can put all the code
    from below where the call to pid_ns_prepare_proc is now (except
    error handing) into the main loop of pid allocation, so we can
    do all that stuff under the pidmap_lock:

    for (i = ns->level; i >= 0; i--) {
    ...
    idr_alloc_cyclic(...)
    get_pid_ns(ns);
    atomic_set(&pid->count, 1);
    for (...)
    INIT_HLIST_HEAD(...)
    ns->nr_allocated++;
    ...
    }

    I do not see how this can fix the problem with not-fully-initialized
    pid returned by find_pid_ns().

    As for PIDNS_ADDING/PIDNS_HASH_ADDING, _perhaps_ we can cleanup this
    logic
    a bit and do the check earlier, but imo this needs another/separate
    change.

    I'd suggest to keep the current logic and the order of initialization
    and
    just do

    for (i = ns->level; i >= 0; i--) {
    ...

    // do not expose the new pid to find_pid_ns() until it
    // is fully initialized
    nr = idr_alloc_cyclic(&tmp->idr, /*pid*/ NULL, ...);
    ...
    }

    ...

    spin_lock_irq(&pidmap_lock);
    if (!(ns->nr_hashed & PIDNS_HASH_ADDING))
    goto out_unlock;
    for ( ; upid >= pid->numbers; --upid) {
    - hlist_add_head_rcu(&upid->pid_chain,
    - &pid_hash[pid_hashfn(upid->nr, upid-
    ns)]);
    + // finally make it visible to find_pid_ns()
    + idr_replace(upid->ns-idr, pid, upid->nr);
    upid->ns->nr_hashed++;
    }
    spin_unlock_irq(&pidmap_lock);

    Or I missed something?

    Thanks for detailed explanation Oleg!


    You are right, that would both fix the problem, and keep the error
    paths relatively simple.

    Gargi, what do you think?
    I understand better now. Thanks!

    Best,
    Gargi

    --
    All Rights Reversed.

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)