From mboxrd@z Thu Jan 1 00:00:00 1970 From: Waiman Long Subject: Re: [PATCH v16 13/14] pvqspinlock: Improve slowpath performance by avoiding cmpxchg Date: Thu, 30 Apr 2015 14:49:26 -0400 Message-ID: <55427936.4080900@hp.com> 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"; Format="flowed" Content-Transfer-Encoding: 7bit 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 To: Peter Zijlstra Cc: linux-arch@vger.kernel.org, Rik van Riel , Raghavendra K T , Oleg Nesterov , kvm@vger.kernel.org, Konrad Rzeszutek Wilk , Daniel J Blueman , x86@kernel.org, Paolo Bonzini , linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Scott J Norton , Ingo Molnar , David Vrabel , "H. Peter Anvin" , xen-devel@lists.xenproject.org, Thomas Gleixner , "Paul E. McKenney" , Linus Torvalds , Boris Ostrovsky , Douglas Hatch List-Id: linux-arch.vger.kernel.org On 04/29/2015 02:11 PM, 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 do think the code is OK. However, you are right that if my reasoning is incorrect, the resulting bug will be really subtle. So I am going to withdraw this particular patch as it has no functional impact to the overall patch series. Please let me know if you have any other comments on other parts of the series and I will send send out a new series without this particular patch. Cheers, Longman From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from g2t1383g.austin.hp.com ([15.217.136.92]:39005 "EHLO g2t1383g.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752626AbbD3Std (ORCPT ); Thu, 30 Apr 2015 14:49:33 -0400 Message-ID: <55427936.4080900@hp.com> Date: Thu, 30 Apr 2015 14:49:26 -0400 From: Waiman Long MIME-Version: 1.0 Subject: Re: [PATCH v16 13/14] pvqspinlock: Improve slowpath performance by avoiding cmpxchg 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> In-Reply-To: <20150429181112.GI23123@twins.programming.kicks-ass.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Peter Zijlstra Cc: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , linux-arch@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, xen-devel@lists.xenproject.org, kvm@vger.kernel.org, Paolo Bonzini , Konrad Rzeszutek Wilk , Boris Ostrovsky , "Paul E. McKenney" , Rik van Riel , Linus Torvalds , Raghavendra K T , David Vrabel , Oleg Nesterov , Daniel J Blueman , Scott J Norton , Douglas Hatch Message-ID: <20150430184926.mfrAFGZGQo5C-q7XAEADCEcWED7x_qstlXuuYuG3alc@z> On 04/29/2015 02:11 PM, 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 do think the code is OK. However, you are right that if my reasoning is incorrect, the resulting bug will be really subtle. So I am going to withdraw this particular patch as it has no functional impact to the overall patch series. Please let me know if you have any other comments on other parts of the series and I will send send out a new series without this particular patch. Cheers, Longman