Messages in this thread | | | Date | Tue, 10 Oct 2017 16:15:23 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH v4 1/2] pid: Replace pid bitmap implementation with IDR API |
| |
On 10/10, Gargi Sharma wrote: > > On Tue, Oct 10, 2017 at 12:50 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > On 10/09, Andrew Morton wrote: > >> > >> Especially here. I don't think pidmap_lock is held. Is that IDR > >> iteration safe? > > > > Yes, this doesn't look right, we need rcu_read_lock() or pidmap_lock. > > > > And, we also need rcu_read_lock() for another reason, to protect "struct pid". > > Ah, I missed this. From what I understood idr_for_each_entry_continue > should be safe
Without rcu? Why?
> because calls idr_get_next which in turn calls > radix_tree_iter_find to find the next populated entry in the idr.
and then it does rcu_dereference_raw(*slot). Without rcu or pidmap this slot can got away if we race with free_pid().
> If > the pid that you are looking up the task for is deleted, task will get > a NULL from pid_task and no signal to kill will be sent.
pid->tasks is protected by tasklist_lock, but the pid itself can go away without rcu lock.
So I think you need to take rcu_read_lock() right after tasklist_lock.
> > Gargi, I suggested to use idr_for_each_entry_continue(), but now I am wondering > > if we should use idr_for_each() instead. IIUC this would be a bit faster? Not > > that I think this is really important... > > I can run benchmarks with idr_for_each to see how much speed up is > achieved and then we can go with whatever we think is better. How does > that sounds?
Up to you ;) I am fine either way.
Oleg.
| |