Re: [PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock
From: Raghavendra K T
Date: Tue Feb 24 2015 - 13:18:35 EST
On 02/24/2015 08:17 PM, Ingo Molnar wrote:
* Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
On Tue, Feb 24, 2015 at 02:54:59PM +0530, Raghavendra K T wrote:
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: Moving slowpath flag to head, ticket_equals idea]
[PeterZ: Detailed changelog]
Reported-by: Sasha Levin <sasha.levin@xxxxxxxxxx>
Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Raghavendra K T <raghavendra.kt@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Oleg Nesterov <oleg@xxxxxxxxxx>
Acked-by: David Vrabel <david.vrabel@xxxxxxxxxx>
---
arch/x86/include/asm/spinlock.h | 94 ++++++++++++++++++++---------------------
arch/x86/kernel/kvm.c | 7 ++-
arch/x86/xen/spinlock.c | 7 ++-
3 files changed, 58 insertions(+), 50 deletions(-)
Changes for stable:
- Don't replace the ACCESS_ONCE to READ_ONCE which would cause horraneous
Compiler warnings (Linus, David Vbriel, PeterZ, Ingo)
What is the git commit id of this in Linus's tree? What
stable tree(s) do you want this applied to?
It's:
d6abfdb20223 x86/spinlocks/paravirt: Fix memory corruption on unlock
Yes, This is the original patch. Please note I have taken out the
READ_ONCE changes from the original patch to avoid build warnings
mentioned below.
(Those READ_ONCE changes were cosmetic and was not present in the
previous versions)
You'll also need this fix from Linus to avoid (harmless)
build warnings:
dd36929720f4 kernel: make READ_ONCE() valid on const arguments
So this may not be absolutely necessary with the current patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/