Re: [PATCH V2 11/19] csky: Atomic operations

On Thu, Jul 05, 2018 at 07:59:02PM +0200, Peter Zijlstra wrote:

On Mon, Jul 02, 2018 at 01:30:14AM +0800, Guo Ren wrote:

quoted

+static inline void arch_spin_lock(arch_spinlock_t *lock)
+{
+	unsigned int *p = &lock->lock;
+	unsigned int tmp;
+
+	asm volatile (
+		"1:	ldex.w		%0, (%1) \n"
+		"	bnez		%0, 1b   \n"
+		"	movi		%0, 1    \n"
+		"	stex.w		%0, (%1) \n"
+		"	bez		%0, 1b   \n"
+		: "=&r" (tmp)
+		: "r"(p)
+		: "memory");
+	smp_mb();
+}

Test-and-set with MB acting as ACQUIRE, ok.

Em ... Ok, I'll try to use test-and-set function instead of it.

quoted

+static inline void arch_spin_unlock(arch_spinlock_t *lock)
+{
+	unsigned int *p = &lock->lock;
+	unsigned int tmp;
+
+	smp_mb();
+	asm volatile (
+		"1:	ldex.w		%0, (%1) \n"
+		"	movi		%0, 0    \n"
+		"	stex.w		%0, (%1) \n"
+		"	bez		%0, 1b   \n"
+		: "=&r" (tmp)
+		: "r"(p)
+		: "memory");
+}

MB acting for RELEASE, but _why_ are you using a LDEX/STEX to clear the
lock word? Would not a normal store work?

Normal store is enough, I'll fixup it in next version patch.
 

Also, the fact that you need MB for release implies your LDEX does not
in fact imply anything and your xchg/cmpxchg implementation is broken.

xchg/cmxchg broken without 1th smp_mb()? Why we need protect the
instructions flow before the ldex.w?

quoted

+static inline int arch_spin_trylock(arch_spinlock_t *lock)
+{
+	unsigned int *p = &lock->lock;
+	unsigned int tmp;
+
+	asm volatile (
+		"1:	ldex.w		%0, (%1) \n"
+		"	bnez		%0, 2f   \n"
+		"	movi		%0, 1    \n"
+		"	stex.w		%0, (%1) \n"
+		"	bez		%0, 1b   \n"
+		"	movi		%0, 0    \n"
+		"2:				 \n"
+		: "=&r" (tmp)
+		: "r"(p)
+		: "memory");
+	smp_mb();
+
+	return !tmp;
+}

Strictly speaking you can avoid the MB on failure. You only need to
provide ACQUIRE semantics on success.

That said, I would really suggest you implement a ticket lock instead of
a test-and-set lock. They're not really all that complicated and do
provide better worst case behaviour.

Ok, I'll try to implement ticket lock in next version patch.

quoted

+/****** read lock/unlock/trylock ******/

Please have a look at using qrwlock -- esp. if you implement a ticket
lock, then the rwlock comes for 'free'.

Ok, I'll try it.

 Guo Ren