Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Paul E. McKenney]

On Mon, Nov 08, 2010 at 07:04:43AM +0900, Tetsuo Handa wrote:
> Hello.
> 
> Paul E. McKenney wrote:
> > > Users missing rcu_read_lock() when calling find_task_by_vpid():
> > > 
> > >   check_clock() in kernel/posix-cpu-timers.c
> > 
> > 	This one has read_lock(&tasklist_lock).
> > 
> Excuse me. Holding tasklist_lock lock does not help.
> We must call rcu_read_lock() explicitly.
> That's why 9728e5d6 "kernel/pid.c: update comment on find_task_by_pid_ns" was made.

OK, good point, there are a few more kernels of unpopped corn here.

> I think there are users who needlessly call read_lock(&tasklist_lock)
> when they can use rcu_read_lock() instead.
> But I don't know when to use read_lock(&tasklist_lock).
> 
> If read_lock(&tasklist_lock) is needed only when we want to access
> the "struct task_struct" after rcu_read_unlock(), maybe it is cleaner to
> use a helper like
> 
> struct task_struct *find_task_and_get(pid_t pid)
> {
> 	struct task_struct *task;
> 	read_lock(&tasklist_lock);
> 	rcu_read_lock();
> 	task = find_task_by_vpid(pid);
> 	rcu_read_unlock();
> 	if (task)
> 		get_task_struct(task);
> 	read_unlock(&tasklist_lock);
> 	return task;
> }
> 
> and hide tasklist_lock.

This makes a lot of sense to me!  That said, most of the current
open-coded variants of your find_task_and_get() seem to have the
rcu_read_unlock() after the get_task_struct() rather than before.  But I
don't claim to understand the locking design of this part of the kernel
well enough to say which is the best approach.

So, either way, will you be submitting the patches for this?

							Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/