Re: [PATCH v2] introduce atomic_pointer to fix a race condition in cancelable mcs spinlocks

[Peter Zijlstra]

On Tue, Jun 03, 2014 at 07:14:31AM -0400, Mikulas Patocka wrote:
> > So if we really want to keep supporting these platforms; I would propose
> > something like:
> > 
> > #ifdef __CHECKER__
> > #define __atomic	__attribute__((address_space(5)))
> > #else
> > #define __atomic
> > #endif
> > 
> > #define store(p, v)	(*(p) = (typeof(*(p)) __force __atomic)(v))
> > #define load(p)		((typeof(*p) __force)ACCESS_ONCE(*(p)))
> > 
> > Along with changes to xchg() and cmpxchg() that require them to take
> > pointers to __atomic.
> > 
> > That way we keep the flexibility of xchg() and cmpxchg() for being
> > (mostly) type and size invariant, and get sparse to find wrong usage.
> > 
> > Then parisc, sparc32, tile32, metag-lock1 and arc-!llsc can go implement
> > store() however they like.
> 
> Your proposal is very good because it warns about incorrect usage 
> automatically.

Exactly the point.

> Your usage is very similar to what my patch at the top of this thread 
> does:
> 
> Instead of "__atomic struct s *p;" declaration, my patch uses
> "atomic_pointer(struct s*) p;" as the declaration
> Instead of store(&p, v), my patch uses atomic_pointer_set(&p, v);
> Instead of load(&p), my patch uses atomic_pointer_get(&p);
> Instead of xchg(&p, v), my patch uses atomic_pointer_xchg(&p, v);
> Instead of cmpxchg(&p, v1, v2), my patch uses atomic_pointer_cmpxchg(&p1, v1, v2);
> 
> > But its horrible, and doesn't have any benefit afaict.
> 
> See the five cases above - why do you say that the operation on the left 
> is good and the operation on the right is horrible? To me, it looks like 
> they are both similar, they are just named differently. Both check the 
> type of the pointer and warns if the user passes incompatible pointer. If 
> I rename the operations in my patch to store(), load(), xchg(), cmpxchg(), 
> would you like it?

Nope.. because the above store,load,xchg,cmpxchg are type invariant and
work for anything of size (1),2,4,(8).

So I dislike your proposal on a number of points:

 1) its got pointer in, and while the immediate problem is indeed with
 pointers, there is no reason it always should be, so we'll keep on
 introducing new APIs;

 2) its got a fixed length, nl. sizeof(void *), if we were to find
 another case which had the same problem which used 'int' we'd have to
 again create new APIs;

 3) you only fixed the one site;

 4) I'm the lazy kind and atomic_foo_* is just too much typing, let
 alone remembering all the various new atomic_foo_ APIs resulting from
 all this.

This is the place where I really miss C++ templates; and yes before
people shoot me in the head for that, I do know about all the various
pitfalls and down sides of those too.

> My patch has advantage (over your #define __atomic 
> __attribute__((address_space(5))) ) that it checks the mismatches at 
> compile time. Your proposal only check them with sparse. But either way - 
> it is very good that the mismatches are being checked automatically.

So my proposal goes a lot further in that by making xchg() and cmpxchg()
require pointer to __atomic, all sites get coverage, not only the one
case where you found was a problem.

Yes, this requires a lot more effort, for we'll have to pretty much
audit and annotate the entire tree, but such things can be done, see for
example the introduction of __rcu.

Also, these days we get automagic emails if we introduce new sparse
fails, so it being sparse and not gcc isn't really any threshold at all.

> We need some method to catch these races automatically. There are places 
> where people xchg() or cmpxchg() with direct modifications, see for 
> example this:

Yep, so all those places will immediately stand out, the first fail will
be that those variables aren't marked __atomic, once you do that, the
direct assignment will complain about crossing the address_space marker.

Voila, sorted.

Attachment: pgpMf5jqFD0k1.pgp
Description: PGP signature