Re: [PATCH] Add adaptive elision to rwlocks

[Andi Kleen <ak at linux dot intel dot com>]

> __rwelision is an unsigned type, yet ELIDE_LOCK tests with <= 0.
> Make it == 0.

Thanks for the review.  I'll do these changes.

> 
> In elision_adapt you have two cases of:
> 
> 	if (var1 != var2)
> 	  va1 = var2;
> 
> If it is meaningfully worthwhile to write that instead of simply:
> 
> 	var1 = var2;

This is an important optimization to avoid unnecessary cache line
transitions and aborts. In general unnecessary writes
can be very expensive in parallel situations, as hardware
may need to do a lot of work to transition the cache line
from SHARED to EXCLUSIVE.

I can add a comment.

> 
> then it warrants a comment explaining why.
> 
> I see no reason why ELIDE_*LOCK must be macros rather than functions.
> If you really know better than the compiler it's important that they
> be inlined, then use always_inline.  Give each a comment describing
> its interface contract.  (AFAICT elision_adapt should just be rolled
> directly into elide_lock.)  
> 
> Oh, I see, you are passing in an expression to be evaluated inside.
> Can you try passing in the address of a nested function marked as
> always_inline and see if the compiler manages to inline everything?
> Hmm.  But it's the same expression every time!  So why the false
> generality?  They could just be inline functions taking the
> pthread_rwlock_t pointer directly.

The macros are generic for any kind of lock type.

It would be a different e.g. for spinlocks. There are also
some possible future optimizations where it may be different 
even for rwlocks.

So I would prefer to keep the macro. Do you insist on the
inline?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only