From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752903AbdHJQWV (ORCPT ); Thu, 10 Aug 2017 12:22:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52706 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751982AbdHJQWU (ORCPT ); Thu, 10 Aug 2017 12:22:20 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 27622C0721DD Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=longman@redhat.com Subject: Re: [RESEND PATCH v5] locking/pvqspinlock: Relax cmpxchg's to improve performance on some archs To: Peter Zijlstra Cc: Ingo Molnar , linux-kernel@vger.kernel.org, Pan Xinhui , Boqun Feng , Andrea Parri References: <1495633108-12818-1-git-send-email-longman@redhat.com> <20170810115034.ie65wfxepiq6noew@hirez.programming.kicks-ass.net> <945c28c3-5779-c8c8-13bb-40477abd1f0e@redhat.com> <20170810161524.2wzocpcxrliy7nt6@hirez.programming.kicks-ass.net> From: Waiman Long Organization: Red Hat Message-ID: <7cb318a8-d5b9-0019-a537-1720fc5222cc@redhat.com> Date: Thu, 10 Aug 2017 12:22:19 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <20170810161524.2wzocpcxrliy7nt6@hirez.programming.kicks-ass.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 10 Aug 2017 16:22:20 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/10/2017 12:15 PM, Peter Zijlstra wrote: > On Thu, Aug 10, 2017 at 09:58:57AM -0400, Waiman Long wrote: >> On 08/10/2017 09:27 AM, Waiman Long wrote: >>> On 08/10/2017 07:50 AM, Peter Zijlstra wrote: >>>> On Wed, May 24, 2017 at 09:38:28AM -0400, Waiman Long wrote: >>>>> # of thread w/o patch with patch % Change >>>>> ----------- --------- ---------- -------- >>>>> 4 4053.3 Mop/s 4223.7 Mop/s +4.2% >>>>> 8 3310.4 Mop/s 3406.0 Mop/s +2.9% >>>>> 12 2576.4 Mop/s 2674.6 Mop/s +3.8% >>>> Waiman, could you run those numbers again but with the below 'fixed' ? >>>> >>>>> @@ -361,6 +361,13 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) >>>>> * observe its next->locked value and advance itself. >>>>> * >>>>> * Matches with smp_store_mb() and cmpxchg() in pv_wait_node() >>>>> + * >>>>> + * The write to next->locked in arch_mcs_spin_unlock_contended() >>>>> + * must be ordered before the read of pn->state in the cmpxchg() >>>>> + * below for the code to work correctly. However, this is not >>>>> + * guaranteed on all architectures when the cmpxchg() call fails. >>>>> + * Both x86 and PPC can provide that guarantee, but other >>>>> + * architectures not necessarily. >>>>> */ >>>> smp_mb(); >>>> >>>>> if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted) >>>>> return; >>>> Ideally this Power CPU can optimize back-to-back SYNC instructions, but >>>> who knows... >>> Yes, I can run the numbers again. However, the changes here is in the >>> slowpath. My current patch optimizes the fast path only and my original >>> test doesn't stress the slowpath at all, I think. I will have to make >>> some changes to stress the slowpath. >> Looking at past emails, I remember why I put the comment there. Putting >> an smp_mb() here will definitely has an negative performance impact on >> x86. So I put in the comment here to remind me that the current code may >> not work for ARM64. >> >> To fix that, my current thought is to have a cmpxchg variant that >> guarantees ordering for both success and failure, for example, >> cmpxchg_ordered(). In that way, we only need to insert the barrier for >> architectures that need it. That will be a separate patch instead of >> integrating into this one. > Might as well do an explicit: > > smp_mb__before_atomic() > cmpxchg_relaxed() > smp_mb__after_atomic() > > I suppose and not introduce new primitives. Right. I think that will work without impacting current x86 performance. Will update my patch accordingly. Thanks, Longman