Le mercredi 10 mars 2010 à 10:41 +0100, Arnd Bergmann a écrit :

On Wednesday 10 March 2010 03:14:10 Paul E. McKenney wrote:

quoted

On Tue, Mar 09, 2010 at 10:12:59PM +0100, Arnd Bergmann wrote:

quoted

I've just tried annotating net/ipv4/route.c like this and did not get
very far, because the same pointers are used for rcu and rcu_bh.
Could you check if this is a false positive or an actual finding?

Hmmm...  I am only seeing a call_rcu_bh() here, so unless I am missing
something, this is a real problem in TREE_PREEMPT_RCU kernels.  The
call_rcu_bh() only interacts with the rcu_read_lock_bh() readers, not
the rcu_read_lock() readers.

One approach is to run freed blocks through both types of grace periods,
I suppose.

Well, if I introduce different __rcu and __rcu_bh address space annotations,
sparse would still not like that, because then you can only pass the annotated
pointers into either rcu_dereference or rcu_dereference_bh.

What the code seems to be doing here is in some places

	local_bh_disable();
	...
	rcu_read_lock();
	rcu_dereference(rt_hash_table[h].chain);
	rcu_read_unlock();
	...
	local_bh_enable();

and in others

	rcu_read_lock_bh();
	rcu_dereference_bh(rt_hash_table[h].chain);
	rcu_read_unlock_bh();

When rt_hash_table[h].chain gets the __rcu_bh annotation, we'd have to
turn first rcu_dereference into rcu_dereference_bh in order to have a clean
build with sparse. Would that change be
a) correct from RCU perspective,
b) desirable for code inspection, and
c) lockdep-clean?

Its really rcu_dereference_bh() that could/should be used:
I see no problem changing


        local_bh_disable();
        ...
        rcu_read_lock();
        rcu_dereference(rt_hash_table[h].chain);
        rcu_read_unlock();
        ...
        local_bh_enable();


to


        local_bh_disable();
        ...
        rcu_read_lock();
        rcu_dereference_bh(rt_hash_table[h].chain);
        rcu_read_unlock();
        ...
        local_bh_enable();