On Tue, Oct 16, 2018 at 10:58:30AM +0800, Guo Ren wrote:

	WRITE_ONCE(lock->tickets.owner, lock->tickets.owner + 1);

+}
+
+static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
+{
+	return lock.tickets.owner == lock.tickets.next;
+}
+
+static inline int arch_spin_is_locked(arch_spinlock_t *lock)
+{
+	return !arch_spin_value_unlocked(READ_ONCE(*lock));
+}
+
+static inline int arch_spin_is_contended(arch_spinlock_t *lock)
+{
+	struct __raw_tickets tickets = READ_ONCE(lock->tickets);
+
+	return (tickets.next - tickets.owner) > 1;
+}
+#define arch_spin_is_contended	arch_spin_is_contended
+
+#include <asm/qrwlock.h>
+
+/* See include/linux/spinlock.h */
+#define smp_mb__after_spinlock()	smp_mb()
+
+#else /* CONFIG_QUEUED_RWLOCKS */
+
+/*
+ * Test-and-set spin-locking.
+ */

I'm still not entirely sure why you want to have two spinlock
implementations; to me that is just extra maintenance overhead.

+static inline void arch_spin_lock(arch_spinlock_t *lock)
+{
+	u32 *p = &lock->lock;
+	u32 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)
+		: "cc");
+	smp_mb();
+}
+
+static inline void arch_spin_unlock(arch_spinlock_t *lock)
+{
+	u32 *p = &lock->lock;
+	u32 tmp;
+
+	smp_mb();
+	asm volatile (
+		"	movi		%0, 0    \n"
+		"	stw		%0, (%1) \n"
+		: "=&r" (tmp)
+		: "r"(p)
+		: "cc");

	WRITE_ONCE(lock->lock, 0);
?

+}
+
+static inline int arch_spin_trylock(arch_spinlock_t *lock)
+{
+	u32 *p = &lock->lock;
+	u32 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)
+		: "cc");
+
+	if (!tmp)
+		smp_mb();
+
+	return !tmp;
+}
+
+#define arch_spin_is_locked(x)	(READ_ONCE((x)->lock) != 0)
+
+/*
+ * read lock/unlock/trylock
+ */

Idem, why do you want a second rwlock_t implementation?

+/*
+ * write lock/unlock/trylock
+ */
+static inline void arch_write_lock(arch_rwlock_t *lock)
+{
+	u32 *p = &lock->lock;
+	u32 tmp;
+
+	asm volatile (
+		"1:	ldex.w		%0, (%1) \n"
+		"	bnez		%0, 1b   \n"
+		"	subi		%0, 1    \n"
+		"	stex.w		%0, (%1) \n"
+		"	bez		%0, 1b   \n"
+		: "=&r" (tmp)
+		: "r"(p)
+		: "cc");
+	smp_mb();
+}
+
+static inline void arch_write_unlock(arch_rwlock_t *lock)
+{
+	u32 *p = &lock->lock;
+	u32 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)
+		: "cc");

Isn't that:

	WRITE_ONCE(lock->lock, 0);

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

I don't understand why you have this; if the CPU has ll/sc, why do you
need syscall support?

In any case, nothing terminally broken; so I suppose that's good enough
for starters. I just really don't understand some decisions (like having
two lock implementations and having that cmpxchg syscall when you have
hardware ll/sc).

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>