On 10/10, Gargi Sharma wrote: > > On Tue, Oct 10, 2017 at 12:50 PM, Oleg Nesterov <o...@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.
