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

Gargi Sharma
2017-09-27T15:05:40Z

On Wed, Sep 27, 2017 at 6:39 PM, Rik van Riel <riel@surriel.com> wrote:
> 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.
Yes, will change this in the next version.
>
>> @@ -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.
Yes, one loop suffices. I will fix this in the next version.

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

Gargi
>
> --
> All Rights Reversed.