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

Mikulas Patocka
2014-06-03T14:19:30Z

On Tue, 3 Jun 2014, Peter Zijlstra wrote:

> 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.

I originally wanted to remove PA-RISC xchg and cmpxchg, force compile
failure on places where it is used and convert them to atomic operations.
But there's a lot of such places, the patch would be big and it would
probably trigger some compile failures in configurations that I can't
test.

So, I agree that your approch with sparse tagging is better, it only warns
about unsafe use and it won't be breaking compilation for so many people.

Mikulas