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:
pgputQ3Xnfc84.pgp
Description: PGP signature
