Message-ID: <alpine.LRH.2.02.1406031007420.15434@file01.intranet.prod.int.rdu2.redhat.com>
Date:	Tue, 3 Jun 2014 10:18:05 -0400 (EDT)
From:	Mikulas Patocka <mpatocka@...hat.com>
To:	Peter Zijlstra <peterz@...radead.org>
cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	"James E.J. Bottomley" <jejb@...isc-linux.org>,
	Helge Deller <deller@....de>,
	John David Anglin <dave.anglin@...l.net>,
	Parisc List <linux-parisc@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	"Vinod, Chegu" <chegu_vinod@...com>,
	Waiman Long <Waiman.Long@...com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Rik van Riel <riel@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Davidlohr Bueso <davidlohr@...com>,
	Peter Anvin <hpa@...or.com>, Andi Kleen <andi@...stfloor.org>,
	"Chandramouleeswaran, Aswin" <aswin@...com>,
	"Norton, Scott J" <scott.norton@...com>,
	Jason Low <jason.low2@...com>
Subject: Re: [PATCH v2] introduce atomic_pointer to fix a race condition in
 cancelable mcs spinlocks



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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists