On Thu, Jul 21, 2022 at 09:58:00PM +0100, Lee Jones wrote: > On Thu, 21 Jul 2022, Yonghong Song wrote: > > > > > > > On 7/21/22 5:14 AM, Jiri Olsa wrote: > > > On Thu, Jul 21, 2022 at 12:59:09PM +0100, Lee Jones wrote: > > > > On Thu, 21 Jul 2022, Jiri Olsa wrote: > > > > > > > > > On Thu, Jul 21, 2022 at 12:14:30PM +0100, Lee Jones wrote: > > > > > > The documentation for find_pid() clearly states: > > > > > > typo find_vpid > > > > > > > > > > > > > > > "Must be called with the tasklist_lock or rcu_read_lock() held." > > > > > > > > > > > > Presently we do neither. > > > > > > just curious, did you see crash related to this or you just spot that > > > > > > > > > > > > > > > In an ideal world we would wrap the in-lined call to find_vpid() along > > > > > > with get_pid_task() in the suggested rcu_read_lock() and have done. > > > > > > However, looking at get_pid_task()'s internals, it already does that > > > > > > independently, so this would lead to deadlock. > > > > > > > > > > hm, we can have nested rcu_read_lock calls, right? > > > > > > > > I assumed not, but that might be an oversight on my part. > > > > From kernel documentation, nested rcu_read_lock is allowed. > > https://www.kernel.org/doc/Documentation/RCU/Design/Requirements/Requirements.html > > > > RCU's grace-period guarantee allows updaters to wait for the completion of > > all pre-existing RCU read-side critical sections. An RCU read-side critical > > section begins with the marker rcu_read_lock() and ends with the marker > > rcu_read_unlock(). These markers may be nested, and RCU treats a nested set > > as one big RCU read-side critical section. Production-quality > > implementations of rcu_read_lock() and rcu_read_unlock() are extremely > > lightweight, and in fact have exactly zero overhead in Linux kernels built > > for production use with CONFIG_PREEMPT=n. > > > > > > > > > > Would that be your preference? > > > > > > seems simpler than calling get/put for ppid > > > > The current implementation seems okay since we can hide > > rcu_read_lock() inside find_get_pid(). We can also avoid > > nested rcu_read_lock(), which is although allowed but > > not pretty. > > Right, this was my thinking. > > Happy to go with whatever you guys decide though. > > Make the call and I'll rework, or not. ok, I can live with the current version ;-) could you please resend with fixed changelog? thanks, jirka
