Re: [PATCH v5 1/2] pid: Replace pid bitmap implementation with IDR API

[Rik van Riel]

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 <gs051095@xxxxxxxxx>

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.

Attachment: signature.asc
Description: This is a digitally signed message part