Messages in this thread | | | Date | Mon, 5 Jan 2009 13:58:32 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [patch] mm: fix lockless pagecache reordering bug (was Re: |
| |
On Mon, Jan 05, 2009 at 10:04:51PM +0100, Peter Zijlstra wrote: > On Mon, 2009-01-05 at 12:12 -0800, Paul E. McKenney wrote: > > On Mon, Jan 05, 2009 at 10:44:27AM -0800, Linus Torvalds wrote: > > > On Mon, 5 Jan 2009, Nick Piggin wrote: > > > > On Mon, Jan 05, 2009 at 09:30:55AM -0800, Linus Torvalds wrote: > > > > Putting an rcu_dereference there might work, but I think it misses a > > > > subtlety of this code. > > > > > > No, _you_ miss the subtlety of something that can change under you. > > > > > > Look at radix_tree_deref_slot(), and realize that without the > > > rcu_dereference(), the compiler would actually be allowed to think that it > > > can re-load anything from *pslot several times. So without my one-liner > > > patch, the compiler can actually do this: > > > > > > register = load_from_memory(pslot) > > > if (radix_tree_is_indirect_ptr(register)) > > > goto fail: > > > return load_from_memory(pslot); > > > > > > fail: > > > return RADIX_TREE_RETRY; > > > > My guess is that Nick believes that the value in *pslot cannot change > > in such as way as to cause radix_tree_is_indirect_ptr()'s return value > > to change within a given RCU grace period, and that Linus disagrees. > > Nick's belief would indeed be true IFF all modifying ops including all > uses of radix_tree_replace_slot() are serialized wrt. each other. > > However, since radix_tree_deref_slot() is the counterpart of > radix_tree_replace_slot(), one would indeed expect rcu_dereference() > therein, much like Linus suggests. > > While what Nick says is true, the lifetime management of the data > objects is arranged externally from the radix tree -- I still think we > need the rcu_dereference() even for that argument, as we want to support > RCU lifetime management as well.
Makes sense to me!
Thanx, Paul
| |