From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751557AbdBMCYs (ORCPT ); Sun, 12 Feb 2017 21:24:48 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:57843 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751193AbdBMCYq (ORCPT ); Sun, 12 Feb 2017 21:24:46 -0500 Subject: Re: [PATCH v2] locking/pvqspinlock: Relax cmpxchg's to improve performance on some archs To: Waiman Long , Xinhui Pan References: <1482697561-23848-1-git-send-email-longman@redhat.com> <778926a5-cf9f-586b-6bc4-b9453d88aabb@redhat.com> Cc: Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, Boqun Feng From: panxinhui Date: Mon, 13 Feb 2017 10:24:38 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <778926a5-cf9f-586b-6bc4-b9453d88aabb@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 17021302-0020-0000-0000-00000B585272 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006606; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000203; SDB=6.00821118; UDB=6.00401557; IPR=6.00598574; BA=6.00005131; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00014259; XFM=3.00000011; UTC=2017-02-13 02:24:43 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17021302-0021-0000-0000-00005A0BAF8C Message-Id: <1f8d7eaf-ac3b-81b8-0d8f-12f60436cc48@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-02-12_19:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1612050000 definitions=main-1702130023 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 在 2017/2/10 上午4:53, Waiman Long 写道: > On 02/07/2017 10:39 PM, Xinhui Pan wrote: >> >> >> 2016-12-26 4:26 GMT+08:00 Waiman Long >: >> >> A number of cmpxchg calls in qspinlock_paravirt.h were replaced by more >> relaxed versions to improve performance on architectures that use LL/SC. >> >> All the locking related cmpxchg's are replaced with the _acquire >> variants: >> - pv_queued_spin_steal_lock() >> - trylock_clear_pending() >> >> The cmpxchg's related to hashing are replaced by either by the _release >> or the _relaxed variants. See the inline comment for details. >> >> Signed-off-by: Waiman Long > >> >> v1->v2: >> - Add comments in changelog and code for the rationale of the change. >> >> --- >> kernel/locking/qspinlock_paravirt.h | 50 ++++++++++++++++++++++++------------- >> 1 file changed, 33 insertions(+), 17 deletions(-) >> >> >> @@ -323,8 +329,14 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) >> * If pv_kick_node() changed us to vcpu_hashed, retain that >> * value so that pv_wait_head_or_lock() knows to not also try >> * to hash this lock. >> + * >> + * The smp_store_mb() and control dependency above will ensure >> + * that state change won't happen before that. Synchronizing >> + * with pv_kick_node() wrt hashing by this waiter or by the >> + * lock holder is done solely by the state variable. There is >> + * no other ordering requirement. >> */ >> - cmpxchg(&pn->state, vcpu_halted, vcpu_running); >> + cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_running); >> >> /* >> * If the locked flag is still not set after wakeup, it is a >> @@ -360,9 +372,12 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) >> * pv_wait_node(). If OTOH this fails, the vCPU was running and will >> * observe its next->locked value and advance itself. >> * >> - * Matches with smp_store_mb() and cmpxchg() in pv_wait_node() >> + * Matches with smp_store_mb() and cmpxchg_relaxed() in pv_wait_node(). >> + * A release barrier is used here to ensure that node->locked is >> + * always set before changing the state. See comment in pv_wait_node(). >> */ >> - if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted) >> + if (cmpxchg_release(&pn->state, vcpu_halted, vcpu_hashed) >> + != vcpu_halted) >> return; >> >> hi, Waiman >> We can't use _release here, a full barrier is needed. >> >> There is pv_kick_node vs pv_wait_head_or_lock >> >> [w] l->locked = _Q_SLOW_VAL //reordered here >> if (READ_ONCE(pn->state) == vcpu_hashed) //False. >> lp = (struct qspinlock **)1; >> >> [STORE] pn->state = vcpu_hashed lp = pv_hash(lock, pn); >> pv_hash() if (xchg(&l->locked, _Q_SLOW_VAL) == 0) // fasle, not unhashed. >> >> Then the same lock has hashed twice but only unhashed once. So at last as the hash table grows big, we hit RCU stall. >> >> I hit RCU stall when I run netperf benchmark >> >> thanks >> xinhui >> >> >> -- >> 1.8.3.1 >> >> > Yes, I know I am being too aggressive in this patch. I am going to tone it down a bit. I just don't have time to run a performance test on PPC system to verify the gain yet. I am planning to send an updated patch soon. > hi, All I guess I have found the scenario that causes the RCU stall. pv_wait_node [L] pn->state // this load is reordered from cmxchg_release. smp_store_mb(pn->state, vcpu_halted); if (!READ_ONCE(node->locked)) arch_mcs_spin_unlock_contended(&next->locked); pv_kick_node [-L]cmpxchg_release(&pn->state, vcpu_halted, vcpu_hashed) //cmpxchg_release fails, so pn->state keep as it is. pv_wait(&pn->state, vcpu_halted); //on PPC, It will not return until pn->state != vcpu_halted. And when rcu stall hit, I fire an BUG(), and enter debug mode, it seems most cpus are in pv_wait... So the soltuion to solve this problems is simple, keep the cmpxchg as it is in pv_kick_node, cmpxchg on ppc provides full barriers. thanks xinhui > Cheers, > Longman >