Re: [PATCH 6/6] Optimise d_find_alias() [try #6]

[Nick Piggin <nickpiggin@yahoo.com.au>]

David Howells wrote:
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
> 
>>Having the smp_rmb() here implies there is some sort of memory barrier
>>based synchronisation protocol at a higher level (than this function),
>>because you don't actually do anything before them smp_rmb() here.
>>
>>So can you comment what that is?
> 
> 
> Hmmm... That probably shouldn't be there. I don't think Andrew Morton and I
> actually came to an agreement as to whether it's necessary. I don't think it
> is, he thought that it was, though he may have changed his mind.
> 

I think this is easy to argue that the optimisation is OK _if_ the condition
being moved outside the locks is atomic.

The reasoning: if there is _no_ higher synchronisation which guarantees the
condition may be flipped one way or the other, then it is equally uncertain
of whether the spinlock will be taken before or after any critical sections
that modify the value. If there _is_ some higher synchronisation, then the
value is going to be correct anyway -- *unless* you are using some consistency
based lockless synchronisation (ie. if this value is 10 then that must be
false), which is what an rmb could be covering.

Now in this case, the condition isn't exactly atomic but I think that's still
OK, because it shouldn't be able to get a false negative or positive during
some transient operation (ie. if the list was _never_ empty within our known
synchronisation, it will return false, if it was _always_ empty, true).

So I guess the only question is whether some code relies on another value to
"know" the result ( if (inode->nr_aliases > 0) {d_find_alias(inode);} ), in
which case I _think_ an rmb there can get you out of trouble. But you'd need
a comment.

Also: I might be talking complete crap here, so anyone feel free to ridicule
me if I'm wrong.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com