On Tue, 2017-10-10 at 19:22 -0400, Gargi Sharma wrote: > This patch replaces the current bitmap implemetation for > Process ID allocation. Functions that are no longer required, > for example, free_pidmap(), alloc_pidmap(), etc. are removed. > The rest of the functions are modified to use the IDR API. > The change was made to make the PID allocation less complex by > replacing custom code with calls to generic API. > > Signed-off-by: Gargi Sharma <[email protected]>
The patch looks mostly good, just a few style nitpicks remaining.
> @@ -308,8 +165,28 @@ struct pid *alloc_pid(struct pid_namespace *ns)
>
> tmp = ns;
> pid->level = ns->level;
> +
> for (i = ns->level; i >= 0; i--) {
> - nr = alloc_pidmap(tmp);
> + int pid_min = 1;
> +
> + idr_preload(GFP_KERNEL);
> + spin_lock_irq(&pidmap_lock);
> +
> + /*
> + * init really needs pid 1, but after reaching the
> maximum
> + * wrap back to RESERVED_PIDS
> + */
> + if (idr_get_cursor(&tmp->idr) > RESERVED_PIDS)
> + pid_min = RESERVED_PIDS;
> +
> + /* Store a null pointer so find_pid_ns does not find
> + * a partially initialized PID (see below).
> + */
This comment should start with an empty line, just like the one above:
/*
* Store a null pointer so find_pid_ns does not find a
* partially initialized PID (see below).
*/
> @@ -240,18 +230,15 @@ void zap_pid_ns_processes(struct pid_namespace
> *pid_ns)
> *
> */
> read_lock(&tasklist_lock);
> - nr = next_pidmap(pid_ns, 1);
> - while (nr > 0) {
> - rcu_read_lock();
> + nr = 2;
>
> - task = pid_task(find_vpid(nr), PIDTYPE_PID);
> + rcu_read_lock();
I would place the rcu_read_lock() on the line above the
read_lock(&tasklist_lock) and the rcu_read_unlock() below
the read_unlock(&tasklist_lock).
Why? No real reason, except the rcu lock is "a larger lock"
than the read-write lock, and it seems to be how things
are done commonly...
I may be wrong here, and I hope somebody else corrects me
if I am wrong :)
> + idr_for_each_entry_continue(&pid_ns->idr, pid, nr) {
> + task = pid_task(pid, PIDTYPE_PID);
> if (task && !__fatal_signal_pending(task))
> send_sig_info(SIGKILL, SEND_SIG_FORCED,
> task);
> -
> - rcu_read_unlock();
> -
> - nr = next_pidmap(pid_ns, nr);
> }
> + rcu_read_unlock();
> read_unlock(&tasklist_lock);
>
> /*
--
All Rights Reversed.
signature.asc
Description: This is a digitally signed message part
