Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-12 Thread Oleg Nesterov
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

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-11 Thread Oleg Nesterov
On 02/10, Jeremy Fitzhardinge wrote: On 02/10/2015 05:26 AM, Oleg Nesterov wrote: On 02/10, Raghavendra K T wrote: Unfortunately xadd could result in head overflow as tail is high. The other option was repeated cmpxchg which is bad I believe. Any suggestions? Stupid question... what

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-11 Thread Raghavendra K T
On 02/11/2015 11:08 PM, Oleg Nesterov wrote: On 02/11, Raghavendra K T wrote: On 02/10/2015 06:56 PM, Oleg Nesterov wrote: In this case __ticket_check_and_clear_slowpath() really needs to cmpxchg the whole .head_tail. Plus obviously more boring changes. This needs a separate patch even _if_

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-11 Thread Oleg Nesterov
On 02/11, Raghavendra K T wrote: On 02/10/2015 06:56 PM, Oleg Nesterov wrote: In this case __ticket_check_and_clear_slowpath() really needs to cmpxchg the whole .head_tail. Plus obviously more boring changes. This needs a separate patch even _if_ this can work. Correct, but apart from

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-11 Thread Linus Torvalds
On Feb 11, 2015 3:15 PM, Jeremy Fitzhardinge jer...@goop.org wrote: 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

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-11 Thread Jeremy Fitzhardinge
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

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-11 Thread Jeremy Fitzhardinge
On 02/11/2015 03:28 PM, Linus Torvalds wrote: On Feb 11, 2015 3:15 PM, Jeremy Fitzhardinge jer...@goop.org mailto:jer...@goop.org wrote: 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

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-11 Thread Raghavendra K T
On 02/10/2015 06:56 PM, Oleg Nesterov wrote: On 02/10, Raghavendra K T wrote: On 02/10/2015 06:23 AM, Linus Torvalds wrote: add_smp(lock-tickets.head, TICKET_LOCK_INC); if (READ_ONCE(lock-tickets.tail) TICKET_SLOWPATH_FLAG) .. into something like val =

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-10 Thread Oleg Nesterov
On 02/10, Denys Vlasenko wrote: # define HEAD_MASK (TICKET_SLOWPATH_FLAG-1) ... unlock_again: val = xadd((lock-ticket.head_tail, TICKET_LOCK_INC); if (unlikely(!(val HEAD_MASK))) { /* overflow. we inadvertently incremented the tail word. * tail's lsb is TICKET_SLOWPATH_FLAG.

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-10 Thread Jeremy Fitzhardinge
On 02/10/2015 05:26 AM, Oleg Nesterov wrote: On 02/10, Raghavendra K T wrote: On 02/10/2015 06:23 AM, Linus Torvalds wrote: add_smp(lock-tickets.head, TICKET_LOCK_INC); if (READ_ONCE(lock-tickets.tail) TICKET_SLOWPATH_FLAG) .. into something like val =

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-10 Thread Raghavendra K T
On 02/10/2015 06:23 AM, Linus Torvalds wrote: On Mon, Feb 9, 2015 at 4:02 AM, Peter Zijlstra pet...@infradead.org wrote: On Mon, Feb 09, 2015 at 03:04:22PM +0530, Raghavendra K T wrote: So we have 3 choices, 1. xadd 2. continue with current approach. 3. a read before unlock and also after

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-10 Thread Oleg Nesterov
On 02/10, Raghavendra K T wrote: On 02/10/2015 06:23 AM, Linus Torvalds wrote: add_smp(lock-tickets.head, TICKET_LOCK_INC); if (READ_ONCE(lock-tickets.tail) TICKET_SLOWPATH_FLAG) .. into something like val = xadd((lock-ticket.head_tail, TICKET_LOCK_INC

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-10 Thread Denys Vlasenko
On Tue, Feb 10, 2015 at 10:30 AM, Raghavendra K T raghavendra...@linux.vnet.ibm.com wrote: On 02/10/2015 06:23 AM, Linus Torvalds wrote: add_smp(lock-tickets.head, TICKET_LOCK_INC); if (READ_ONCE(lock-tickets.tail) TICKET_SLOWPATH_FLAG) .. into something like val

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-10 Thread Sasha Levin
On 02/10/2015 04:30 AM, Raghavendra K T wrote: So I think Raghavendra's last version (which hopefully fixes the lockup problem that Sasha reported) together with changing that V2 did pass the stress, but getting confirmation Sasha would help. I've been running it for the last two days, and

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-09 Thread Linus Torvalds
On Mon, Feb 9, 2015 at 4:02 AM, Peter Zijlstra pet...@infradead.org wrote: On Mon, Feb 09, 2015 at 03:04:22PM +0530, Raghavendra K T wrote: So we have 3 choices, 1. xadd 2. continue with current approach. 3. a read before unlock and also after that. For the truly paranoid we have

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-09 Thread Peter Zijlstra
On Mon, Feb 09, 2015 at 03:04:22PM +0530, Raghavendra K T wrote: So we have 3 choices, 1. xadd 2. continue with current approach. 3. a read before unlock and also after that. For the truly paranoid we have probe_kernel_address(), suppose the lock was in module space and the module just got

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-09 Thread Raghavendra K T
On 02/09/2015 05:32 PM, Peter Zijlstra wrote: On Mon, Feb 09, 2015 at 03:04:22PM +0530, Raghavendra K T wrote: So we have 3 choices, 1. xadd 2. continue with current approach. 3. a read before unlock and also after that. For the truly paranoid we have probe_kernel_address(), suppose the lock

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-08 Thread Raghavendra K T
On 02/06/2015 09:55 PM, Linus Torvalds wrote: On Fri, Feb 6, 2015 at 6:49 AM, Raghavendra K T raghavendra...@linux.vnet.ibm.com wrote: Paravirt spinlock clears slowpath flag after doing unlock. [ fix edited out ] So I'm not going to be applying this for 3.19, because it's much too late and

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-08 Thread Oleg Nesterov
On 02/06, Sasha Levin wrote: Can we modify it slightly to avoid potentially accessing invalid memory: diff --git a/arch/x86/include/asm/spinlock.h b/arch/x86/include/asm/spinlock.h index 5315887..cd22d73 100644 --- a/arch/x86/include/asm/spinlock.h +++ b/arch/x86/include/asm/spinlock.h @@

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-08 Thread Raghavendra K T
On 02/07/2015 12:27 AM, Sasha Levin wrote: On 02/06/2015 09:49 AM, 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);

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-08 Thread Jeremy Fitzhardinge
On 02/06/2015 06:49 AM, 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() */

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-06 Thread Sasha Levin
On 02/06/2015 02:42 PM, Davidlohr Bueso wrote: On Fri, 2015-02-06 at 08:25 -0800, Linus Torvalds wrote: On Fri, Feb 6, 2015 at 6:49 AM, Raghavendra K T raghavendra...@linux.vnet.ibm.com wrote: Paravirt spinlock clears slowpath flag after doing unlock. [ fix edited out ] So I'm not going to

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-06 Thread Sasha Levin
On 02/06/2015 09:49 AM, 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() */

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-06 Thread Davidlohr Bueso
On Fri, 2015-02-06 at 08:25 -0800, Linus Torvalds wrote: On Fri, Feb 6, 2015 at 6:49 AM, Raghavendra K T raghavendra...@linux.vnet.ibm.com wrote: Paravirt spinlock clears slowpath flag after doing unlock. [ fix edited out ] So I'm not going to be applying this for 3.19, because it's much

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-06 Thread Davidlohr Bueso
On Fri, 2015-02-06 at 16:15 -0500, Sasha Levin wrote: On 02/06/2015 02:42 PM, Davidlohr Bueso wrote: On Fri, 2015-02-06 at 08:25 -0800, Linus Torvalds wrote: On Fri, Feb 6, 2015 at 6:49 AM, Raghavendra K T raghavendra...@linux.vnet.ibm.com wrote: Paravirt spinlock clears slowpath flag

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

2015-02-06 Thread Andrey Ryabinin
On 02/06/2015 07:15 PM, Linus Torvalds wrote: On Fri, Feb 6, 2015 at 7:20 AM, Sasha Levin sasha.le...@oracle.com wrote: Can we modify it slightly to avoid potentially accessing invalid memory: So I think there's a race with that. And I'll warn you: the kernel does do speculative reads of