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

[Gargi Sharma]

On Wed, Sep 27, 2017 at 6:39 PM, Rik van Riel <riel@xxxxxxxxxxx> 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.