Re: [Qemu-devel] [PATCH RFC 3/4] target-ppc: use atomic_cmpxchg for ld/st reservation

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2016-09-07 at 10:17 +0530, Nikunj A Dadhania wrote:
> > David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > 
> > [ Unknown signature status ]
> > On Fri, Sep 02, 2016 at 12:02:55PM +0530, Nikunj A Dadhania wrote:
> > > 
> > > > > > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> > 
> > This really needs a comment indicating that this implementation isn't
> > strictly correct (although probably good enough in practice).
> 
> Sure. And it also does not help if someone uses any store other than
> store conditional, that isn't taken care.
> 
> Assumption here is the locking primitives use load with reservation and
> store conditional. And no other ld/st variant touch this memory.

This is an incorrect assumption. spin_unlock for example uses a normal
store.

That being said, you will observe the difference in value which should
hopefully make things work...

I *hope* we don't have anything that relies on a normal store of the same
value as the atomic breaking the reservation, I *think* we might get away
with it, but it is indeed fishy.

> > Specifically a racing store which happens to store the same value
> > which was already in memory should clobber the reservation, but won't
> > with this implementation.
> > 
> > I had a long discussion at KVM Forum with Emilio Costa about this, in
> > which I discovered just how hard it is to strictly implement
> > store-conditional semantics in terms of anything else.  So, this is
> > probably a reasonable substitute, but we should note the fact that
> > it's not 100%.
> 
> I will update the commit log.
> 
> Regards,
> Nikunj