Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753841AbbBKR3E (ORCPT ); Wed, 11 Feb 2015 12:29:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56638 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753497AbbBKR3C (ORCPT ); Wed, 11 Feb 2015 12:29:02 -0500 Date: Wed, 11 Feb 2015 18:24:34 +0100 From: Oleg Nesterov To: Jeremy Fitzhardinge Cc: Raghavendra K T , Linus Torvalds , Sasha Levin , Davidlohr Bueso , Peter Zijlstra , Thomas Gleixner , Ingo Molnar , Peter Anvin , Konrad Rzeszutek Wilk , Paolo Bonzini , Paul McKenney , Waiman Long , Dave Jones , the arch/x86 maintainers , Paul Gortmaker , Andi Kleen , Jason Wang , Linux Kernel Mailing List , KVM list , virtualization , xen-devel@lists.xenproject.org, Rik van Riel , Christian Borntraeger , Andrew Morton , Andrey Ryabinin Subject: Re: [PATCH] x86 spinlock: Fix memory corruption on completing completions Message-ID: <20150211172434.GA28689@redhat.com> References: <1423234148-13886-1-git-send-email-raghavendra.kt@linux.vnet.ibm.com> <54D7D19B.1000103@goop.org> <54D87F1E.9060307@linux.vnet.ibm.com> <20150209120227.GT21418@twins.programming.kicks-ass.net> <54D9CFC7.5020007@linux.vnet.ibm.com> <20150210132634.GA30380@redhat.com> <54DAADEE.6070506@goop.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54DAADEE.6070506@goop.org> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2000 Lines: 48 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 if we simply move SLOWPATH from .tail to .head? > > In this case arch_spin_unlock() could do xadd(tickets.head) and check > > the result > > Well, right now, "tail" is manipulated by locked instructions by CPUs > who are contending for the ticketlock, but head can be manipulated > unlocked by the CPU which currently owns the ticketlock. If SLOWPATH > moved into head, then non-owner CPUs would be touching head, requiring > everyone to use locked instructions on it. > > That's the theory, but I don't see much (any?) code which depends on that. > > Ideally we could find a way so that pv ticketlocks could use a plain > unlocked add for the unlock like the non-pv case, but I just don't see a > way to do it. 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? Anyway, I suggested this to avoid the overflow if we use xadd(), and I guess we need the locked insn anyway if we want to eliminate the unsafe read-after-unlock... > > BTW. If we move "clear slowpath" into "lock" path, then probably trylock > > should be changed too? Something like below, we just need to clear SLOWPATH > > before cmpxchg. > > How important / widely used is trylock these days? I am not saying this is that important. Just this looks more consistent imo and we can do this for free. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/