From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Torvalds Subject: Re: [PATCH v16 13/14] pvqspinlock: Improve slowpath performance by avoiding cmpxchg Date: Wed, 29 Apr 2015 11:27:14 -0700 Message-ID: References: <1429901803-29771-1-git-send-email-Waiman.Long@hp.com> <1429901803-29771-14-git-send-email-Waiman.Long@hp.com> <20150429181112.GI23123@twins.programming.kicks-ass.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Waiman Long , "linux-arch@vger.kernel.org" , Rik van Riel , Raghavendra K T , Oleg Nesterov , KVM list , Konrad Rzeszutek Wilk , Daniel J Blueman , the arch/x86 maintainers , Paolo Bonzini , Linux Kernel Mailing List , virtualization , Scott J Norton , Ingo Molnar , David Vrabel , "H. Peter Anvin" , xen-devel@lists.xenproject.org, Thomas Gleixner , "Paul E. McKenney" , Boris Ostrovsky , Douglas Hatch To: Peter Zijlstra Return-path: In-Reply-To: <20150429181112.GI23123@twins.programming.kicks-ass.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org List-Id: kvm.vger.kernel.org On Wed, Apr 29, 2015 at 11:11 AM, Peter Zijlstra wrote: > On Fri, Apr 24, 2015 at 02:56:42PM -0400, Waiman Long wrote: >> In the pv_scan_next() function, the slow cmpxchg atomic operation is >> performed even if the other CPU is not even close to being halted. This >> extra cmpxchg can harm slowpath performance. >> >> This patch introduces the new mayhalt flag to indicate if the other >> spinning CPU is close to being halted or not. The current threshold >> for x86 is 2k cpu_relax() calls. If this flag is not set, the other >> spinning CPU will have at least 2k more cpu_relax() calls before >> it can enter the halt state. This should give enough time for the >> setting of the locked flag in struct mcs_spinlock to propagate to >> that CPU without using atomic op. > > Yuck! I'm not at all sure you can make assumptions like that. And the > worst part is, if it goes wrong the borkage is subtle and painful.\ I have to agree with Peter. But it goes beyond this particular patch. Patterns like this: xchg(&pn->mayhalt, true); are just evil and disgusting. Even befoe this patch, that code had (void)xchg(&pn->state, vcpu_halted); which is *wrong* and should never be done. If you want it to be "set_mb()" (which sets a value and has a memory barrier), then use set_mb(). Yes, it happens to use a "xchg()" to do so, but dammit, it documents that whole "this is a memory barrier" in the name. Also, anybody who does this should damn well document why the memory barrier is needed. The xchg(&pn->state, vcpu_halted) at least is preceded by a comment about the barriers. The new mayhalt has no sane comment in it, and the reason seems to be that no sane comment is possible. The xchg() seems to be some black magic thing. Let's not introduce magic stuff in our locking primitives. At least not undocumented magic that makes no sense. Linus