On 08/21, Kui-Feng Lee wrote: > > > On 8/21/23 08:09, Oleg Nesterov wrote: > >1. find_pid_ns() + get_pid_task() under rcu_read_lock() guarantees that we > > can safely iterate the task->thread_group list. Even if this task exits > > right after get_pid_task() (or goto retry) and pid_alive() returns 0 > > > Kill the unnecessary pid_alive() check. > > This function will return next_task holding a refcount, and release the > refcount until the next time calling the same function. Meanwhile, > the returned task A may be killed, and its next task B may be > killed after A as well, before calling this function again. > However, even task B is destroyed (free), A's next is still pointing to > task B. When this function is called again for the same iterator, > it doesn't promise that B is still there. Not sure I understand... OK, if we have a task pointer with incremented refcount and do not hold rcu lock, then yes, you can't remove the pid_alive() check in this code: rcu_read_lock(); if (pid_alive(task)) do_something(next_thread(task)); rcu_read_unlock(); because task and then task->next can exit and do call_rcu(delayed_put_task_struct) before we take rcu_read_lock(). But if you do something like rcu_read_lock(); task = find_task_in_some_rcu_protected_list(); do_something(next_thread(task)); rcu_read_unlock(); then next_thread(task) should be safe without pid_alive(). And iiuc task_group_seq_get_next() always does rcu_read_lock(); // the caller does lock/unlock task = get_pid_task(pid, PIDTYPE_PID); if (!task) return; next_task = next_thread(task); rcu_read_unlock(); Yes, both task and task->next can exit right after get_pid_task(), but since can only happen after we took rcu_read_lock(), delayed_put_task_struct() can't be called until we drop rcu lock. What have I missed? Oleg.
