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

From: Peter Zijlstra
Date: Tue Jun 03 2014 - 09:25:24 EST


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