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

[Rik van Riel <riel@surriel.com>]

[-- Attachment #1: Type: text/plain, Size: 2274 bytes --]

On Wed, 2017-09-27 at 01:06 -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.

I like where this is going. Just a few last comments from me :)

> --- a/arch/powerpc/platforms/cell/spufs/sched.c
> +++ b/arch/powerpc/platforms/cell/spufs/sched.c
> @@ -1093,7 +1093,7 @@ static int show_spu_loadavg(struct seq_file *s,
> void *private)
>  		LOAD_INT(c), LOAD_FRAC(c),
>  		count_active_contexts(),
>  		atomic_read(&nr_spu_contexts),
> -		task_active_pid_ns(current)->last_pid);
> +		task_active_pid_ns(current)->idr.idr_next-1);
>  	return 0;
>  }

Everywhere you use idr.idr_next or idr->idr_next directly,
you could use idr_get_cursor(idr) instead, exposing less
of the IDR internals to the rest of the code.

> @@ -240,17 +230,18 @@ 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();
> +	pid = idr_get_next(&pid_ns->idr, &nr);
> +	while (pid) {
>  
> -		task = pid_task(find_vpid(nr), PIDTYPE_PID);
> -		if (task && !__fatal_signal_pending(task))
> -			send_sig_info(SIGKILL, SEND_SIG_FORCED,
> task);
> +		rcu_read_lock();
>  
> +		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);
>  	}
>  	read_unlock(&tasklist_lock);
>  

I believe we should be fine with just the idr_for_each_entry_continue()
surrounding the loop, and not need the while (pid) around that.

That should still iterate over all the pids in the namespace, and
simplify the code even more.

You have done a great job understanding some complicated code, and
simplifying it during your Outreachy internship.

-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]