Re: [patch 00/21] mutex subsystem, -V14

[Denis Vlasenko <vda@ilport.com.ua>]

On Thursday 05 January 2006 18:21, Linus Torvalds wrote:
> On Thu, 5 Jan 2006, Ingo Molnar wrote:
> > 
> > the patch below adds the barriers to the asm-generic mutex routines, so 
> > it's not like i'm lazy ;), but i really think this is unnecessary.  
> > Adding this patch would add a second, unnecessary barrier for all the 
> > arches that have barrier-less atomic ops.
> > 
> > it also makes sense: the moment you are interested in the 'previous 
> > value' of the atomic counter in an atomic fashion, you very likely want 
> > to use it for a critical section. (e.g. all the put-the-resource ops 
> > that use atomic_dec_test() rely on this implicit barrier.)
>
> So I _think_ your argument is bogus, and your patch is bogus. The use of 
> "atomic_dec_return()" in a mutex is _not_ the same barrier as using it for 
> reference counting. Not at all. Memory barriers aren't just one thing: 
> they are semi-permeable things in two different directions and with two 
> different operations: there are several different kinds of them.
> 
> >  #define __mutex_fastpath_lock(count, fail_fn)				\
> >  do {									\
> > +	smp_mb__before_atomic_dec();					\
> >  	if (unlikely(atomic_dec_return(count) < 0))			\
> >  		fail_fn(count);						\
> >  } while (0)
> 
> So I think the barrier has to come _after_ the atomic decrement (or 
> exchange). 
> 
> Because as it is written now, any writes in the locked region could 
> percolate up to just before the atomic dec - ie _outside_ the region. 
> Which is against the whole point of a lock - it would allow another CPU to 
> see the write even before it sees that the lock was successful, as far as
> I can tell.
> 
> But memory ordering is subtle, so maybe I'm missing something..

We mere humans^W device driver people get more confused with barriers
every day, as CPUs get more subtle in their out-of-order-ness.

I think adding longer-named-but-self-explanatory aliases for memory and io
barrier functions can help.

mmiowb => barrier_memw_iow
.... => barrier_memw_memw (a store-store barrier to mem)
....

General template for the name may be something like 

	[smp]barrier_{mem,io,memio}{r,w,rw}_{mem,io,memio}{r,w,rw}

Are there even more subtle cases?
--
vda