Messages in this thread | | | Date | Mon, 5 Jan 2009 12:12:58 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [patch] mm: fix lockless pagecache reordering bug (was Re: BUG: soft lockup - is this XFS problem?) |
| |
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.
Whatever the answer, I would argue for -at- -least- a comment explaining why it is safe. I am not seeing the objection to rcu_dereference(), but I must confess that it has been awhile since I have looked closely at the radix_tree code. :-/
Thanx, Paul
| |