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

[Nikunj A Dadhania]

Benjamin Herrenschmidt <address@hidden> writes:

> On Wed, 2016-09-07 at 10:17 +0530, Nikunj A Dadhania wrote:
>> > David Gibson <address@hidden> writes:
>> 
>> > 
>> > [ Unknown signature status ]
>> > On Fri, Sep 02, 2016 at 12:02:55PM +0530, Nikunj A Dadhania wrote:
>> > > 
>> > > > > > Signed-off-by: Nikunj A Dadhania <address@hidden>
>> > 
>> > 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.

In arch/powerpc/include/asm/spinlock.h: 

We have __arch_spin_trylock() which uses lwarx and "stwcx." instructions
to acquire lock. And later during the unlock, a normal store will do.
IMHO, that is the reason for it work.

Regards,
Nikunj