[PATCH] ARM: spinlock: avoid exclusive accesses on unlock() path

[PATCH] ARM: spinlock: avoid exclusive accesses on unlock() path

[Will Deacon]

When unlocking a spinlock, all we need to do is increment the owner
field of the lock. Since only one CPU can be performing an unlock()
operation for a given lock, this doesn't need to be exclusive.

This patch simplifies arch_spin_unlock to use non-exclusive accesses
when updating the owner field of the lock.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/spinlock.h | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h
index b4ca707..6220e9f 100644
--- a/arch/arm/include/asm/spinlock.h
+++ b/arch/arm/include/asm/spinlock.h
@@ -119,22 +119,8 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
 
 static inline void arch_spin_unlock(arch_spinlock_t *lock)
 {
-	unsigned long tmp;
-	u32 slock;
-
 	smp_mb();
-
-	__asm__ __volatile__(
-"	mov	%1, #1\n"
-"1:	ldrex	%0, [%2]\n"
-"	uadd16	%0, %0, %1\n"
-"	strex	%1, %0, [%2]\n"
-"	teq	%1, #0\n"
-"	bne	1b"
-	: "=&r" (slock), "=&r" (tmp)
-	: "r" (&lock->slock)
-	: "cc");
-
+	lock->tickets.owner++;
 	dsb_sev();
 }
 
-- 
1.8.0

[PATCH] ARM: spinlock: avoid exclusive accesses on unlock() path

[马文健]

Hi
while one cpu is doing arch_spin_unlock,another cpu doing arch_spin_lock
the first cpu's
lock->tickets.owner++;
may be over writed by the second's 
strex	%2, %1, [%3]\n
so the owner filed doesn't get increased. 

But i'm not sure about this, may be there are something i have missed. 

Thanks.

------------------ Original ------------------

[PATCH] ARM: spinlock: avoid exclusive accesses on unlock() path

[Will Deacon]

On Tue, Aug 07, 2018 at 09:59:13AM +0800, ??? wrote:
> while one cpu is doing arch_spin_unlock,another cpu doing arch_spin_lock
> the first cpu's
> lock->tickets.owner++;
> may be over writed by the second's 
> strex	%2, %1, [%3]\n
> so the owner filed doesn't get increased. 

I don't see how this can happen. If the unlocker writes to the owner field
in-between the locker's LDXR/STXR pair (which is the only way the harmful
overwrite could occur as far as I can tell), then the STXR will fail and not
update memory.

Are you actually seeing a problem here?

Will

[PATCH] ARM: spinlock: avoid exclusive accesses on unlock()path

[Will Deacon]

On Wed, Aug 08, 2018 at 08:30:28AM +0800, ??? wrote:
> On 8 Aug 2018, will.deacon wrote:
> 
> >On Tue, Aug 07, 2018 at 09:59:13AM +0800, ??? wrote:
> >> while one cpu is doing arch_spin_unlock,another cpu doing arch_spin_lock
> > >the first cpu's
> > >lock->tickets.owner++;
> > >may be over writed by the second's
> > >strex %2, %1, [%3]\n
> > >so the owner filed doesn't get increased.
> 
> >I don't see how this can happen. If the unlocker writes to the owner field
> >in-between the locker's LDXR/STXR pair (which is the only way the harmful
> >overwrite could occur as far as I can tell), then the STXR will fail and not
> >update memory.
> 
> >Are you actually seeing a problem here?
> 
> In DDI0406C_C_arm_architecture_reference_manual.pdf A3-117, It says after
> normal Store operation,  the monitor maybe in Exclusive Access or Open Access
> which is implementation defined.

You're looking at the section describing non-shareable memory. If you scroll
down to A3-119, you can see that a store to the tagged address from a
different CPU will transition the monitor to the open state.

Will