Skip to contentSkip to conversation

3.19.1-stable review

PATCH In review Greg Kroah-Hartman 7 participants 171 messages Mar 4, 2015 – May 11, 2015 Page 2 of 2

Subsystems (7)
Stable branch x86 KVM MIPS +3 more subsystems
lore
Share & tools
lore
author maintainer
#165 lore raw

PATCH 174/175 x86/spinlocks/paravirt: Fix memory corruption on unlock

In reply to Greg Kroah-Hartman

3.19-stable review patch. If anyone has any objections, please let me know.

------------------

From: Raghavendra K T <[email protected]>

commit d6abfdb2022368d8c6c4be3f11a06656601a6cc2 upstream.

Paravirt spinlock clears slowpath flag after doing unlock. As explained by Linus currently it does:

                prev = *lock;
                add_smp(&lock->tickets.head, TICKET_LOCK_INC);

                /* add_smp() is a full mb() */

                if (unlikely(lock->tickets.tail & TICKET_SLOWPATH_FLAG))
                        __ticket_unlock_slowpath(lock, prev);

which is *exactly* the kind of things you cannot do with spinlocks, because after you've done the "add_smp()" and released the spinlock for the fast-path, you can't access the spinlock any more. Exactly because a fast-path lock might come in, and release the whole data structure.

Linus suggested that we should not do any writes to lock after unlock(), and we can move slowpath clearing to fastpath lock.

So this patch implements the fix with:

1. Moving slowpath flag to head (Oleg):
Unlocked locks don't care about the slowpath flag; therefore we can keep it set after the last unlock, and clear it again on the first (try)lock. -- this removes the write after unlock. note that keeping slowpath flag would result in unnecessary kicks.
By moving the slowpath flag from the tail to the head ticket we also avoid the need to access both the head and tail tickets on unlock.

2. use xadd to avoid read/write after unlock that checks the need for unlock_kick (Linus):
We further avoid the need for a read-after-release by using xadd; the prev head value will include the slowpath flag and indicate if we need to do PV kicking of suspended spinners -- on modern chips xadd isn't (much) more expensive than an add + load.

Result:
setup: 16core (32 cpu +ht sandy bridge 8GB 16vcpu guest) benchmark overcommit %improve

 kernbench  1x           -0.13
 kernbench  2x            0.02
 dbench     1x           -1.77
 dbench     2x           -0.63

[Jeremy: Hinted missing TICKET_LOCK_INC for kick] [Oleg: Moved slowpath flag to head, ticket_equals idea] [PeterZ: Added detailed changelog]

---
 arch/x86/include/asm/spinlock.h |   94 +++++++++++++++++++---------------------
 arch/x86/kernel/kvm.c           |   13 +++--
 arch/x86/xen/spinlock.c         |   13 +++--
 3 files changed, 64 insertions(+), 56 deletions(-)
arch/x86/include/asm/spinlock.h +46 -48
@@ -46,7 +46,7 @@ static __always_inline bool static_key_f
 
 static inline void __ticket_enter_slowpath(arch_spinlock_t *lock)
 {
-	set_bit(0, (volatile unsigned long *)&lock->tickets.tail);
+	set_bit(0, (volatile unsigned long *)&lock->tickets.head);
 }
 
 #else  /* !CONFIG_PARAVIRT_SPINLOCKS */
@@ -60,10 +60,30 @@ static inline void __ticket_unlock_kick(
 }
 
 #endif /* CONFIG_PARAVIRT_SPINLOCKS */
+static inline int  __tickets_equal(__ticket_t one, __ticket_t two)
+{
+	return !((one ^ two) & ~TICKET_SLOWPATH_FLAG);
+}
+
+static inline void __ticket_check_and_clear_slowpath(arch_spinlock_t *lock,
+							__ticket_t head)
+{
+	if (head & TICKET_SLOWPATH_FLAG) {
+		arch_spinlock_t old, new;
+
+		old.tickets.head = head;
+		new.tickets.head = head & ~TICKET_SLOWPATH_FLAG;
+		old.tickets.tail = new.tickets.head + TICKET_LOCK_INC;
+		new.tickets.tail = old.tickets.tail;
+
+		/* try to clear slowpath flag when there are no contenders */
+		cmpxchg(&lock->head_tail, old.head_tail, new.head_tail);
+	}
+}
 
 static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
 {
-	return lock.tickets.head == lock.tickets.tail;
+	return __tickets_equal(lock.tickets.head, lock.tickets.tail);
 }
 
 /*
@@ -87,18 +107,21 @@ static __always_inline void arch_spin_lo
 	if (likely(inc.head == inc.tail))
 		goto out;
 
-	inc.tail &= ~TICKET_SLOWPATH_FLAG;
 	for (;;) {
 		unsigned count = SPIN_THRESHOLD;
 
 		do {
-			if (READ_ONCE(lock->tickets.head) == inc.tail)
-				goto out;
+			inc.head = READ_ONCE(lock->tickets.head);
+			if (__tickets_equal(inc.head, inc.tail))
+				goto clear_slowpath;
 			cpu_relax();
 		} while (--count);
 		__ticket_lock_spinning(lock, inc.tail);
 	}
-out:	barrier();	/* make sure nothing creeps before the lock is taken */
+clear_slowpath:
+	__ticket_check_and_clear_slowpath(lock, inc.head);
+out:
+	barrier();	/* make sure nothing creeps before the lock is taken */
 }
 
 static __always_inline int arch_spin_trylock(arch_spinlock_t *lock)
@@ -106,56 +129,30 @@ static __always_inline int arch_spin_try
 	arch_spinlock_t old, new;
 
 	old.tickets = READ_ONCE(lock->tickets);
-	if (old.tickets.head != (old.tickets.tail & ~TICKET_SLOWPATH_FLAG))
+	if (!__tickets_equal(old.tickets.head, old.tickets.tail))
 		return 0;
 
 	new.head_tail = old.head_tail + (TICKET_LOCK_INC << TICKET_SHIFT);
+	new.head_tail &= ~TICKET_SLOWPATH_FLAG;
 
 	/* cmpxchg is a full barrier, so nothing can move before it */
 	return cmpxchg(&lock->head_tail, old.head_tail, new.head_tail) == old.head_tail;
 }
 
-static inline void __ticket_unlock_slowpath(arch_spinlock_t *lock,
-					    arch_spinlock_t old)
-{
-	arch_spinlock_t new;
-
-	BUILD_BUG_ON(((__ticket_t)NR_CPUS) != NR_CPUS);
-
-	/* Perform the unlock on the "before" copy */
-	old.tickets.head += TICKET_LOCK_INC;
-
-	/* Clear the slowpath flag */
-	new.head_tail = old.head_tail & ~(TICKET_SLOWPATH_FLAG << TICKET_SHIFT);
-
-	/*
-	 * If the lock is uncontended, clear the flag - use cmpxchg in
-	 * case it changes behind our back though.
-	 */
-	if (new.tickets.head != new.tickets.tail ||
-	    cmpxchg(&lock->head_tail, old.head_tail,
-					new.head_tail) != old.head_tail) {
-		/*
-		 * Lock still has someone queued for it, so wake up an
-		 * appropriate waiter.
-		 */
-		__ticket_unlock_kick(lock, old.tickets.head);
-	}
-}
-
 static __always_inline void arch_spin_unlock(arch_spinlock_t *lock)
 {
 	if (TICKET_SLOWPATH_FLAG &&
-	    static_key_false(&paravirt_ticketlocks_enabled)) {
-		arch_spinlock_t prev;
+		static_key_false(&paravirt_ticketlocks_enabled)) {
+		__ticket_t head;
 
-		prev = *lock;
-		add_smp(&lock->tickets.head, TICKET_LOCK_INC);
+		BUILD_BUG_ON(((__ticket_t)NR_CPUS) != NR_CPUS);
 
-		/* add_smp() is a full mb() */
+		head = xadd(&lock->tickets.head, TICKET_LOCK_INC);
 
-		if (unlikely(lock->tickets.tail & TICKET_SLOWPATH_FLAG))
-			__ticket_unlock_slowpath(lock, prev);
+		if (unlikely(head & TICKET_SLOWPATH_FLAG)) {
+			head &= ~TICKET_SLOWPATH_FLAG;
+			__ticket_unlock_kick(lock, (head + TICKET_LOCK_INC));
+		}
 	} else
 		__add(&lock->tickets.head, TICKET_LOCK_INC, UNLOCK_LOCK_PREFIX);
 }
@@ -164,14 +161,15 @@ static inline int arch_spin_is_locked(ar
 {
 	struct __raw_tickets tmp = READ_ONCE(lock->tickets);
 
-	return tmp.tail != tmp.head;
+	return !__tickets_equal(tmp.tail, tmp.head);
 }
 
 static inline int arch_spin_is_contended(arch_spinlock_t *lock)
 {
 	struct __raw_tickets tmp = READ_ONCE(lock->tickets);
 
-	return (__ticket_t)(tmp.tail - tmp.head) > TICKET_LOCK_INC;
+	tmp.head &= ~TICKET_SLOWPATH_FLAG;
+	return (tmp.tail - tmp.head) > TICKET_LOCK_INC;
 }
 #define arch_spin_is_contended	arch_spin_is_contended
 
@@ -183,16 +181,16 @@ static __always_inline void arch_spin_lo
 
 static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
 {
-	__ticket_t head = ACCESS_ONCE(lock->tickets.head);
+	__ticket_t head = READ_ONCE(lock->tickets.head);
 
 	for (;;) {
-		struct __raw_tickets tmp = ACCESS_ONCE(lock->tickets);
+		struct __raw_tickets tmp = READ_ONCE(lock->tickets);
 		/*
 		 * We need to check "unlocked" in a loop, tmp.head == head
 		 * can be false positive because of overflow.
 		 */
-		if (tmp.head == (tmp.tail & ~TICKET_SLOWPATH_FLAG) ||
-		    tmp.head != head)
+		if (__tickets_equal(tmp.head, tmp.tail) ||
+				!__tickets_equal(tmp.head, head))
 			break;
 
 		cpu_relax();
arch/x86/kernel/kvm.c +9 -4
@@ -609,7 +609,7 @@ static inline void check_zero(void)
 	u8 ret;
 	u8 old;
 
-	old = ACCESS_ONCE(zero_stats);
+	old = READ_ONCE(zero_stats);
 	if (unlikely(old)) {
 		ret = cmpxchg(&zero_stats, old, 0);
 		/* This ensures only one fellow resets the stat */
@@ -727,6 +727,7 @@ __visible void kvm_lock_spinning(struct
 	int cpu;
 	u64 start;
 	unsigned long flags;
+	__ticket_t head;
 
 	if (in_nmi())
 		return;
@@ -768,11 +769,15 @@ __visible void kvm_lock_spinning(struct
 	 */
 	__ticket_enter_slowpath(lock);
 
+	/* make sure enter_slowpath, which is atomic does not cross the read */
+	smp_mb__after_atomic();
+
 	/*
 	 * check again make sure it didn't become free while
 	 * we weren't looking.
 	 */
-	if (ACCESS_ONCE(lock->tickets.head) == want) {
+	head = READ_ONCE(lock->tickets.head);
+	if (__tickets_equal(head, want)) {
 		add_stats(TAKEN_SLOW_PICKUP, 1);
 		goto out;
 	}
@@ -803,8 +808,8 @@ static void kvm_unlock_kick(struct arch_
 	add_stats(RELEASED_SLOW, 1);
 	for_each_cpu(cpu, &waiting_cpus) {
 		const struct kvm_lock_waiting *w = &per_cpu(klock_waiting, cpu);
-		if (ACCESS_ONCE(w->lock) == lock &&
-		    ACCESS_ONCE(w->want) == ticket) {
+		if (READ_ONCE(w->lock) == lock &&
+		    READ_ONCE(w->want) == ticket) {
 			add_stats(RELEASED_SLOW_KICKED, 1);
 			kvm_kick_cpu(cpu);
 			break;
arch/x86/xen/spinlock.c +9 -4
@@ -41,7 +41,7 @@ static u8 zero_stats;
 static inline void check_zero(void)
 {
 	u8 ret;
-	u8 old = ACCESS_ONCE(zero_stats);
+	u8 old = READ_ONCE(zero_stats);
 	if (unlikely(old)) {
 		ret = cmpxchg(&zero_stats, old, 0);
 		/* This ensures only one fellow resets the stat */
@@ -112,6 +112,7 @@ __visible void xen_lock_spinning(struct
 	struct xen_lock_waiting *w = this_cpu_ptr(&lock_waiting);
 	int cpu = smp_processor_id();
 	u64 start;
+	__ticket_t head;
 	unsigned long flags;
 
 	/* If kicker interrupts not initialized yet, just spin */
@@ -159,11 +160,15 @@ __visible void xen_lock_spinning(struct
 	 */
 	__ticket_enter_slowpath(lock);
 
+	/* make sure enter_slowpath, which is atomic does not cross the read */
+	smp_mb__after_atomic();
+
 	/*
 	 * check again make sure it didn't become free while
 	 * we weren't looking
 	 */
-	if (ACCESS_ONCE(lock->tickets.head) == want) {
+	head = READ_ONCE(lock->tickets.head);
+	if (__tickets_equal(head, want)) {
 		add_stats(TAKEN_SLOW_PICKUP, 1);
 		goto out;
 	}
@@ -204,8 +209,8 @@ static void xen_unlock_kick(struct arch_
 		const struct xen_lock_waiting *w = &per_cpu(lock_waiting, cpu);
 
 		/* Make sure we read lock before want */
-		if (ACCESS_ONCE(w->lock) == lock &&
-		    ACCESS_ONCE(w->want) == next) {
+		if (READ_ONCE(w->lock) == lock &&
+		    READ_ONCE(w->want) == next) {
 			add_stats(RELEASED_SLOW_KICKED, 1);
 			xen_send_IPI_one(cpu, XEN_SPIN_UNLOCK_VECTOR);
 			break;
author maintainer
#166 lore raw

PATCH 175/175 x86/xen/p2m: Replace ACCESS_ONCE with READ_ONCE

In reply to Greg Kroah-Hartman

3.19-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Borntraeger <[email protected]>

commit 1760f1eb7ec485197bd3a8a9c13e4160bb740275 upstream.

ACCESS_ONCE does not work reliably on non-scalar types. For example gcc 4.6 and 4.7 might remove the volatile tag for such accesses during the SRA (scalar replacement of aggregates) step
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)

Change the p2m code to replace ACCESS_ONCE with READ_ONCE.

---
 arch/x86/xen/p2m.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/xen/p2m.c +1 -1
@@ -554,7 +554,7 @@ static bool alloc_p2m(unsigned long pfn)
 		mid_mfn = NULL;
 	}
 
-	p2m_pfn = pte_pfn(ACCESS_ONCE(*ptep));
+	p2m_pfn = pte_pfn(READ_ONCE(*ptep));
 	if (p2m_pfn == PFN_DOWN(__pa(p2m_identity)) ||
 	    p2m_pfn == PFN_DOWN(__pa(p2m_missing))) {
 		/* p2m leaf page is missing */
#167 lore raw
In reply to Greg Kroah-Hartman
Quoted 8 lines · Greg Kroah-Hartman

This is the start of the stable review cycle for the 3.19.1 release. There are 175 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know.

Responses should be made by Fri Mar 6 06:09:09 UTC 2015. Anything received after that time might be too late.

Build results:

	total: 121 pass: 108 fail: 13
Failed builds:
	powerpc:defconfig
	powerpc:allmodconfig
	powerpc:cell_defconfig
	powerpc:ppc6xx_defconfig
	powerpc:mpc83xx_defconfig
	powerpc:mpc85xx_defconfig
	powerpc:mpc85xx_smp_defconfig
	powerpc:tqm8xx_defconfig
	powerpc:85xx/sbc8548_defconfig
	powerpc:83xx/mpc834x_mds_defconfig
	powerpc:86xx/sbc8641d_defconfig
	sh:dreamcast_defconfig
	sh:microdev_defconfig
Qemu tests:
	total: 30 pass: 26 fail: 4

Failed tests:
powerpc:ppc_book3s_defconfig
powerpc:ppc_book3s_smp_defconfig
sh:sh_defconfig
sh:sheb_defconfig

Culprit for the failed builds (both powerpc and sh) is 'kernel: tighten rules for ACCESS ONCE' according to bisect.

Details are available at http://server.roeck-us.net:8010/builders.

Guenter

author maintainer
#168 lore raw
Quoted 28 lines · Guenter Roeck
Quoted 8 lines · Greg Kroah-Hartman

This is the start of the stable review cycle for the 3.19.1 release. There are 175 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know.

Responses should be made by Fri Mar 6 06:09:09 UTC 2015. Anything received after that time might be too late.

Build results:

	total: 121 pass: 108 fail: 13
Failed builds:
	powerpc:defconfig
	powerpc:allmodconfig
	powerpc:cell_defconfig
	powerpc:ppc6xx_defconfig
	powerpc:mpc83xx_defconfig
	powerpc:mpc85xx_defconfig
	powerpc:mpc85xx_smp_defconfig
	powerpc:tqm8xx_defconfig
	powerpc:85xx/sbc8548_defconfig
	powerpc:83xx/mpc834x_mds_defconfig
	powerpc:86xx/sbc8641d_defconfig
	sh:dreamcast_defconfig
	sh:microdev_defconfig
Qemu tests:
	total: 30 pass: 26 fail: 4
Quoted 8 lines

Failed tests:
powerpc:ppc_book3s_defconfig
powerpc:ppc_book3s_smp_defconfig
sh:sh_defconfig
sh:sheb_defconfig

Culprit for the failed builds (both powerpc and sh) is 'kernel: tighten rules for ACCESS ONCE' according to bisect.

Ok, thanks, I should have this fixed now.

greg k-h

#169 lore raw
In reply to Greg Kroah-Hartman
Quoted 8 lines · Greg Kroah-Hartman

This is the start of the stable review cycle for the 3.19.1 release. There are 175 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know.

Responses should be made by Fri Mar 6 06:09:09 UTC 2015. Anything received after that time might be too late.

Quoted 8 lines

The whole patch series can be found in one patch at:

	kernel.org/pub/linux/kernel/v3.0/stable-review/patch-3.19.1-rc1.gz

and the diffstat can be found below.

thanks,

greg k-h

Compiled and booted on my test system. No dmesg regressions.

thanks,
-- Shuah

Signature · 5 lines
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
[email protected] | (970) 217-8978
author maintainer
#170 lore raw
Quoted 13 lines · Shuah Khan
Quoted 12 lines · Greg Kroah-Hartman

This is the start of the stable review cycle for the 3.19.1 release. There are 175 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know.

Responses should be made by Fri Mar 6 06:09:09 UTC 2015. Anything received after that time might be too late.

The whole patch series can be found in one patch at:

	kernel.org/pub/linux/kernel/v3.0/stable-review/patch-3.19.1-rc1.gz

and the diffstat can be found below.

Quoted 6 lines
Quoted 4 lines

thanks,

greg k-h

Compiled and booted on my test system. No dmesg regressions.

Thanks for testing all 4 of these and letting me know.

greg k-h