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 -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
