Messages in this thread | | | From | Denis Vlasenko <> | Subject | Re: [patch 00/21] mutex subsystem, -V14 | Date | Fri, 6 Jan 2006 09:34:37 +0200 |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |