[PATCH V4] x86 spinlock: Fix memory corruption on completing completions
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 2x0.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
Suggested-by: Linus Torvalds
Signed-off-by: Raghavendra K T
---
arch/x86/include/asm/spinlock.h | 91 -
arch/x86/kernel/kvm.c | 10 +++--
arch/x86/xen/spinlock.c | 10 +++--
3 files changed, 56 insertions(+), 55 deletions(-)
potential TODO:
* The whole patch be splitted into, 1. move slowpath flag
2. fix memory corruption in completion problem ??
Changes since V3:
- Detailed changelog (PeterZ)
- Replace ACCESS_ONCE with READ_ONCE (oleg)
- Add xen changes (Oleg)
- Correct break logic in unlock_wait() (Oleg)
Changes since V2:
- Move the slowpath flag to head, this enables xadd usage in unlock code
and inturn we can get rid of read/write after unlock (Oleg)
- usage of ticket_equals (Oleg)
Changes since V1:
- Add missing TICKET_LOCK_INC before unlock kick (fixes hang in overcommit:
Jeremy).
- Remove SLOWPATH_FLAG clearing in fast lock. (Jeremy)
- clear SLOWPATH_FLAG in arch_spin_value_unlocked during comparison.
Note: The current implementation is still based on avoid writing after unlock.
we could still have potential invalid memory read. (Sasha)
Result:
setup: 16core (32 cpu +ht sandy bridge 8GB 16vcpu guest)
base = 3_19_rc7
3_19_rc7_spinfix_v3
+---+---+---++---+
kernbench (Time taken in sec lower is better)
+---+---+---++---+
base %stdevpatched %stdev %improve
+---+---+---++---+
1x 54.2300 3.0652 54.3008 4.0366-0.13056
2x 90.1883 5.5509 90.1650 6.4336 0.02583
+---+---+---++---+
+---+---+---++---+
dbench (Throughput higher is better)
+---+---+---++---+
base %stdevpatched %stdev %improve
+---+---+---++---+
1x 7029.9188 2.5952 6905.0712 4.4737-1.77595
2x 3254.207514.8291 3233.713726.8784-0.62976
+---+---+---++---+
(here is the result I got from the patches, I believe there may
be some small overhead from xadd etc, but overall looks fine but
a thorough test may be needed)
diff --git a/arch/x86/include/asm/spinlock.h b/arch/x86/include/asm/spinlock.h
index 625660f..646a1a3 100644
--- a/arch/x86/include/asm/spinlock.h
+++ b/arch/x86/include/asm/spinlock.h
@@ -46,7 +46,7 @@ static __always_inline bool static_key_false(struct
static_key *key);
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(arch_spinlock_t
*lock,
}
#endif /* CONFIG_P
[PATCH] virtio: don't set VIRTIO_CONFIG_S_DRIVER_OK twice.
I noticed this with the console device. It's not *wrong*, just a bit weird. Signed-off-by: Rusty Russell diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index b9f70dfc4751..5ce2aa48fc6e 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -236,7 +236,10 @@ static int virtio_dev_probe(struct device *_d) if (err) goto err; - add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); + /* If probe didn't do it, mark device DRIVER_OK ourselves. */ + if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK)) + virtio_device_ready(dev); + if (drv->scan) drv->scan(dev); ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC v5 net-next 4/6] virtio-net: add basic interrupt coalescing support
"Michael S. Tsirkin" writes: > On Tue, Feb 10, 2015 at 12:02:37PM +1030, Rusty Russell wrote: >> Jason Wang writes: >> > This patch enables the interrupt coalescing setting through ethtool. >> >> The problem is that there's nothing network specific about interrupt >> coalescing. I can see other devices wanting exactly the same thing, >> which means we'd deprecate this in the next virtio standard. >> >> I think the right answer is to extend like we did with >> vring_used_event(), eg: >> >> 1) Add a new feature VIRTIO_F_RING_COALESCE. >> 2) Add another a 32-bit field after vring_used_event(), eg: >> #define vring_used_delay(vr) (*(u32 *)((vr)->avail->ring[(vr)->num + >> 2])) >> >> This loses the ability to coalesce by number of frames, but we can still >> do number of sg entries, as we do now with used_event, and we could >> change virtqueue_enable_cb_delayed() to take a precise number if we >> wanted. > > But do we expect delay to be update dynamically? > If not, why not stick it in config space? Hmm, we could update it dynamically (and will, in the case of ethtool). But it won't be common, so we could append a field to virtio_pci_common_cfg for PCI. I think MMIO and CCW would be easy to extend too, but CC'd to check. >> My feeling is that this should be a v1.0-only feature though >> (eg. feature bit 33). > > Yes, e.g. we can't extend config space for legacy virtio pci. Thanks, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] x86 spinlock: Fix memory corruption on completing completions
On 02/11, Jeremy Fitzhardinge wrote: > > On 02/11/2015 09:24 AM, Oleg Nesterov wrote: > > I agree, and I have to admit I am not sure I fully understand why > > unlock uses the locked add. Except we need a barrier to avoid the race > > with the enter_slowpath() users, of course. Perhaps this is the only > > reason? > > Right now it needs to be a locked operation to prevent read-reordering. > x86 memory ordering rules state that all writes are seen in a globally > consistent order, and are globally ordered wrt reads *on the same > addresses*, but reads to different addresses can be reordered wrt to writes. > > So, if the unlocking add were not a locked operation: > > __add(&lock->tickets.head, TICKET_LOCK_INC); /* not locked */ > > if (unlikely(lock->tickets.tail & TICKET_SLOWPATH_FLAG)) > __ticket_unlock_slowpath(lock, prev); > > Then the read of lock->tickets.tail can be reordered before the unlock, > which introduces a race: Yes, yes, thanks, but this is what I meant. We need a barrier. Even if "Every store is a release" as Linus mentioned. > This *might* be OK, but I think it's on dubious ground: > > __add(&lock->tickets.head, TICKET_LOCK_INC); /* not locked */ > > /* read overlaps write, and so is ordered */ > if (unlikely(lock->head_tail & (TICKET_SLOWPATH_FLAG << TICKET_SHIFT)) > __ticket_unlock_slowpath(lock, prev); > > because I think Intel and AMD differed in interpretation about how > overlapping but different-sized reads & writes are ordered (or it simply > isn't architecturally defined). can't comment, I simply so not know how the hardware works. > If the slowpath flag is moved to head, then it would always have to be > locked anyway, because it needs to be atomic against other CPU's RMW > operations setting the flag. Yes, this is true. But again, if we want to avoid the read-after-unlock, we need to update this lock and read SLOWPATH atomically, it seems that we can't avoid the locked insn. Oleg. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3] x86 spinlock: Fix memory corruption on completing completions
On 02/12/2015 08:30 PM, Peter Zijlstra wrote: On Thu, Feb 12, 2015 at 05:17:27PM +0530, Raghavendra K T wrote: [...] 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). 2. use xadd to avoid read/write after unlock that checks the need for unlock_kick (Linus). Maybe spend a few more words explaining these things; something like: Unlocked locks don't care about the slowpath flag; therefore we can keep it set after the last unlock, as long as we clear it again on the first (try)lock -- this removes the write after unlock. Nit: I 'll reword a bit here since slowpath flag would result in unnecessary kick but otherwise harmless IMO. something like: 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 avoid the need to access both the head and tail tickets on unlock. We can 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. Its 'obvious' now, but maybe not so much after we've all not looked at this for a few months. Absolutely correct. Thanks Peter for the detailed and very helpful writeup. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3] x86 spinlock: Fix memory corruption on completing completions
On Thu, Feb 12, 2015 at 05:17:27PM +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). > 2. use xadd to avoid read/write after unlock that checks the need for > unlock_kick (Linus). Maybe spend a few more words explaining these things; something like: Unlocked locks don't care about the slowpath flag; therefore we can keep it set after the last unlock, as long as we clear it again on the first (try)lock -- this removes the write after unlock. By moving the slowpath flag from the tail to the head ticket we avoid the need to access both the head and tail tickets on unlock. We can 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. Its 'obvious' now, but maybe not so much after we've all not looked at this for a few months. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3] x86 spinlock: Fix memory corruption on completing completions
On 02/12/2015 07:07 PM, Oleg Nesterov wrote:
On 02/12, Raghavendra K T wrote:
@@ -772,7 +773,8 @@ __visible void kvm_lock_spinning(struct arch_spinlock
*lock, __ticket_t want)
* check again make sure it didn't become free while
* we weren't looking.
*/
- if (ACCESS_ONCE(lock->tickets.head) == want) {
+ head = ACCESS_ONCE(lock->tickets.head);
+ if (__tickets_equal(head, want)) {
add_stats(TAKEN_SLOW_PICKUP, 1);
While at it, perhaps it makes sense to s/ACCESS_ONCE/READ_ONCE/ but this
is cosmetic.
yes.. will do.
We also need to change another user of enter_slow_path, xen_lock_spinning()
in arch/x86/xen/spinlock.c.
Had in mind but forgot before sending. will update resend.
Other than that looks correct at first glance... but this is up to
maintainers.
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3] x86 spinlock: Fix memory corruption on completing completions
On 02/12/2015 07:32 PM, Oleg Nesterov wrote:
Damn, sorry for noise, forgot to mention...
On 02/12, Raghavendra K T wrote:
+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);
+ }
+}
...
+clear_slowpath:
+ if (TICKET_SLOWPATH_FLAG)
+ __ticket_check_and_clear_slowpath(lock, inc.head);
I think you can remove this "if (TICKET_SLOWPATH_FLAG)" check. If it is
defined as zero, gcc should optimize out the code under "if (head & 0)".
right, the above if ( ) is unnecesary, though we would have same code
at the end, getting rid of that makes code more clean.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3] x86 spinlock: Fix memory corruption on completing completions
On 02/12/2015 07:20 PM, Oleg Nesterov wrote: On 02/12, Raghavendra K T wrote: @@ -191,8 +189,7 @@ static inline void arch_spin_unlock_wait(arch_spinlock_t *lock) * 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) || tmp.head != head) break; Ah, it seems that "tmp.head != head" should be turned into !__tickets_equal(), no? Suppose that TICKET_SLOWPATH_FLAG is set after the first ACCESS_ONCE(head), then tmp.head != head will be true before the first unlock we are waiting for. Good catch. othewise we would wrongly break out even when somebody does halt wait. And perhaps you can turn these ACCESS_ONCE into READ_ONCE as well. Yes again :) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3] x86 spinlock: Fix memory corruption on completing completions
Damn, sorry for noise, forgot to mention...
On 02/12, Raghavendra K T wrote:
>
> +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);
> + }
> +}
...
> +clear_slowpath:
> + if (TICKET_SLOWPATH_FLAG)
> + __ticket_check_and_clear_slowpath(lock, inc.head);
I think you can remove this "if (TICKET_SLOWPATH_FLAG)" check. If it is
defined as zero, gcc should optimize out the code under "if (head & 0)".
But this is purely cosmetic and subjective, feel free to ignore.
Oleg.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3] x86 spinlock: Fix memory corruption on completing completions
On 02/12, Raghavendra K T wrote: > > @@ -191,8 +189,7 @@ static inline void arch_spin_unlock_wait(arch_spinlock_t > *lock) >* 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) || tmp.head != head) > break; Ah, it seems that "tmp.head != head" should be turned into !__tickets_equal(), no? Suppose that TICKET_SLOWPATH_FLAG is set after the first ACCESS_ONCE(head), then tmp.head != head will be true before the first unlock we are waiting for. And perhaps you can turn these ACCESS_ONCE into READ_ONCE as well. Oleg. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3] x86 spinlock: Fix memory corruption on completing completions
On 02/12, Raghavendra K T wrote:
>
> @@ -772,7 +773,8 @@ __visible void kvm_lock_spinning(struct arch_spinlock
> *lock, __ticket_t want)
>* check again make sure it didn't become free while
>* we weren't looking.
>*/
> - if (ACCESS_ONCE(lock->tickets.head) == want) {
> + head = ACCESS_ONCE(lock->tickets.head);
> + if (__tickets_equal(head, want)) {
> add_stats(TAKEN_SLOW_PICKUP, 1);
While at it, perhaps it makes sense to s/ACCESS_ONCE/READ_ONCE/ but this
is cosmetic.
We also need to change another user of enter_slow_path, xen_lock_spinning()
in arch/x86/xen/spinlock.c.
Other than that looks correct at first glance... but this is up to
maintainers.
Oleg.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V3] x86 spinlock: Fix memory corruption on completing completions
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).
2. use xadd to avoid read/write after unlock that checks the need for
unlock_kick (Linus).
Result:
setup: 16core (32 cpu +ht sandy bridge 8GB 16vcpu guest)
benchmark overcommit %improve
kernbench 1x -0.13
kernbench 2x0.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]
Reported-by: Sasha Levin
Suggested-by: Linus Torvalds
Signed-off-by: Raghavendra K T
---
arch/x86/include/asm/spinlock.h | 87 -
arch/x86/kernel/kvm.c | 4 +-
2 files changed, 45 insertions(+), 46 deletions(-)
potential TODO:
* The whole patch be splitted into, 1. move slowpath flag
2. fix memory corruption in completion problem ??
* May be we could directly pass inc for __ticket_check_and_clear_slowpath
but I hope current code is more readable.
Changes since V2:
- Move the slowpath flag to head, this enables xadd usage in unlock code
and inturn we can get rid of read/write after unlock (Oleg)
- usage of ticket_equals (Oleg)
Changes since V1:
- Add missing TICKET_LOCK_INC before unlock kick (fixes hang in overcommit:
Jeremy).
- Remove SLOWPATH_FLAG clearing in fast lock. (Jeremy)
- clear SLOWPATH_FLAG in arch_spin_value_unlocked during comparison.
Note: The current implementation is still based on avoid writing after unlock.
we could still have potential invalid memory read. (Sasha)
Result:
setup: 16core (32 cpu +ht sandy bridge 8GB 16vcpu guest)
base = 3_19_rc7
3_19_rc7_spinfix_v3
+---+---+---++---+
kernbench (Time taken in sec lower is better)
+---+---+---++---+
base %stdevpatched %stdev %improve
+---+---+---++---+
1x 54.2300 3.0652 54.3008 4.0366-0.13056
2x 90.1883 5.5509 90.1650 6.4336 0.02583
+---+---+---++---+
+---+---+---++---+
dbench (Throughput higher is better)
+---+---+---++---+
base %stdevpatched %stdev %improve
+---+---+---++---+
1x 7029.9188 2.5952 6905.0712 4.4737-1.77595
2x 3254.207514.8291 3233.713726.8784-0.62976
+---+---+---++---+
(here is the result I got from the patches, I believe there may
be some small overhead from xadd etc, but overall looks fine but
a thorough test may be needed)
diff --git a/arch/x86/include/asm/spinlock.h b/arch/x86/include/asm/spinlock.h
index 625660f..9697b45 100644
--- a/arch/x86/include/asm/spinlock.h
+++ b/arch/x86/include/asm/spinlock.h
@@ -46,7 +46,7 @@ static __always_inline bool static_key_false(struct
static_key *key);
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(arch_spinlock_t
*lock,
}
#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.hea
