What protects rcu_dereference() in __sk_free()?

What protects rcu_dereference() in __sk_free()?

[Paul E. McKenney]

Hello, Dmitry,

Could you please tell me what protects the rcu_dereference() in
__sk_free()?  I am adding lockdep-based checking to RCU, and
"git blame" said I should ask you about this one.

The current code, rcu_dereference(), assumes that this is protected only
by RCU-bh.  My problem might be any of the following:

o	Some other flavor of RCU protects this, e.g., RCU-sched, which
	would require rcu_dereference_sched() in place of my current
	rcu_dereference_bh() for RCU-bh.

o	This is called from updates as well as from readers, and
	some lock protects the updates.

o	This is called during initialization, when this pointer is
	inaccessible to readers.
	
Please note that I can add a check to cover multiple possibilities.
For a real example in include/linux/fdtable.h:

	file = rcu_dereference_check(fdt->fd[fd],
				     rcu_read_lock_held() ||
				     lockdep_is_held(&files->file_lock) ||
				     atomic_read(&files->count) == 1);

The first argument is the pointer, and the second argument says that
this may be protected by either RCU (as opposed to RCU-bh, RCU-sched,
or SRCU), the files->file_lock as recorded by lockdep, or by being in
a single-threaded process as noted by the value of files->count.
(Please see http://lwn.net/Articles/368683/ for a recent patch, another
will go out soon.)

So, could you please tell me what protects the rcu_dereference() in
__sk_free() so that I can craft the appropriate form of rcu_dereference()?

							Thanx, Paul

Re: What protects rcu_dereference() in __sk_free()?

[Eric Dumazet]

Le 14/01/2010 19:41, Paul E. McKenney a écrit :
> Hello, Dmitry,
> 
> Could you please tell me what protects the rcu_dereference() in
> __sk_free()?  I am adding lockdep-based checking to RCU, and
> "git blame" said I should ask you about this one.
> 
> The current code, rcu_dereference(), assumes that this is protected only
> by RCU-bh.  My problem might be any of the following:
> 
> o	Some other flavor of RCU protects this, e.g., RCU-sched, which
> 	would require rcu_dereference_sched() in place of my current
> 	rcu_dereference_bh() for RCU-bh.
> 
> o	This is called from updates as well as from readers, and
> 	some lock protects the updates.
> 
> o	This is called during initialization, when this pointer is
> 	inaccessible to readers.
> 	
> Please note that I can add a check to cover multiple possibilities.
> For a real example in include/linux/fdtable.h:
> 
> 	file = rcu_dereference_check(fdt->fd[fd],
> 				     rcu_read_lock_held() ||
> 				     lockdep_is_held(&files->file_lock) ||
> 				     atomic_read(&files->count) == 1);
> 
> The first argument is the pointer, and the second argument says that
> this may be protected by either RCU (as opposed to RCU-bh, RCU-sched,
> or SRCU), the files->file_lock as recorded by lockdep, or by being in
> a single-threaded process as noted by the value of files->count.
> (Please see http://lwn.net/Articles/368683/ for a recent patch, another
> will go out soon.)
> 
> So, could you please tell me what protects the rcu_dereference() in
> __sk_free() so that I can craft the appropriate form of rcu_dereference()?
> 

Hi Paul

filter = rcu_dereference(sk->sk_filter);

is probably not really needed, current thread being the one doing socket destruction,
and has a writer role.

void sk_free(struct sock *sk)
{
	if (atomic_dec_and_test(&sk->sk_wmem_alloc))
		__sk_free(sk);
}

So the protection comes from the atomic_dec_and_test() that acts as a lock.

Re: What protects rcu_dereference() in __sk_free()?

[Paul E. McKenney]

On Fri, Jan 15, 2010 at 06:59:25AM +0100, Eric Dumazet wrote:
> Le 14/01/2010 19:41, Paul E. McKenney a écrit :
> > Hello, Dmitry,
> > 
> > Could you please tell me what protects the rcu_dereference() in
> > __sk_free()?  I am adding lockdep-based checking to RCU, and
> > "git blame" said I should ask you about this one.
> > 
> > The current code, rcu_dereference(), assumes that this is protected only
> > by RCU-bh.  My problem might be any of the following:
> > 
> > o	Some other flavor of RCU protects this, e.g., RCU-sched, which
> > 	would require rcu_dereference_sched() in place of my current
> > 	rcu_dereference_bh() for RCU-bh.
> > 
> > o	This is called from updates as well as from readers, and
> > 	some lock protects the updates.
> > 
> > o	This is called during initialization, when this pointer is
> > 	inaccessible to readers.
> > 	
> > Please note that I can add a check to cover multiple possibilities.
> > For a real example in include/linux/fdtable.h:
> > 
> > 	file = rcu_dereference_check(fdt->fd[fd],
> > 				     rcu_read_lock_held() ||
> > 				     lockdep_is_held(&files->file_lock) ||
> > 				     atomic_read(&files->count) == 1);
> > 
> > The first argument is the pointer, and the second argument says that
> > this may be protected by either RCU (as opposed to RCU-bh, RCU-sched,
> > or SRCU), the files->file_lock as recorded by lockdep, or by being in
> > a single-threaded process as noted by the value of files->count.
> > (Please see http://lwn.net/Articles/368683/ for a recent patch, another
> > will go out soon.)
> > 
> > So, could you please tell me what protects the rcu_dereference() in
> > __sk_free() so that I can craft the appropriate form of rcu_dereference()?
> > 
> 
> Hi Paul
> 
> filter = rcu_dereference(sk->sk_filter);
> 
> is probably not really needed, current thread being the one doing socket destruction,
> and has a writer role.
> 
> void sk_free(struct sock *sk)
> {
> 	if (atomic_dec_and_test(&sk->sk_wmem_alloc))
> 		__sk_free(sk);
> }
> 
> So the protection comes from the atomic_dec_and_test() that acts as a lock.

Thank you for the info, Eric!

One option would be to remove the rcu_dereference() from __sk_free().
Given that it was there, my thought would be to make it read as follows:

	filter = rcu_dereference_check(sk->sk_filter,
				       atomic_read(&sk->sk_wmem_alloc) == 0);

This approach would have the benefit of potentially catching some race
conditions if built with CONFIG_PROVE_RCU.  Does this seem reasonable to
you?

						Thanx, Paul

Re: What protects rcu_dereference() in __sk_free()?

[Eric Dumazet]

Le 15/01/2010 20:51, Paul E. McKenney a écrit :
> 
> Thank you for the info, Eric!
> 
> One option would be to remove the rcu_dereference() from __sk_free().
> Given that it was there, my thought would be to make it read as follows:
> 
> 	filter = rcu_dereference_check(sk->sk_filter,
> 				       atomic_read(&sk->sk_wmem_alloc) == 0);
> 
> This approach would have the benefit of potentially catching some race
> conditions if built with CONFIG_PROVE_RCU.  Does this seem reasonable to
> you?

Thats a good suggestion, this documents the thing with no runtime cost.

Thanks