From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932133AbbJMUp7 (ORCPT ); Tue, 13 Oct 2015 16:45:59 -0400 Received: from g2t4620.austin.hp.com ([15.73.212.81]:33886 "EHLO g2t4620.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753079AbbJMUp5 (ORCPT ); Tue, 13 Oct 2015 16:45:57 -0400 Message-ID: <561D6D82.5000409@hpe.com> Date: Tue, 13 Oct 2015 16:45:54 -0400 From: Waiman Long User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.12) Gecko/20130109 Thunderbird/10.0.12 MIME-Version: 1.0 To: Peter Zijlstra CC: Ingo Molnar , Thomas Gleixner , "H. Peter Anvin" , x86@kernel.org, linux-kernel@vger.kernel.org, Scott J Norton , Douglas Hatch , Davidlohr Bueso Subject: Re: [PATCH v7 4/5] locking/pvqspinlock: Allow 1 lock stealing attempt References: <1442955044-43895-1-git-send-email-Waiman.Long@hpe.com> <1442955044-43895-5-git-send-email-Waiman.Long@hpe.com> <20151013193928.GX3816@twins.programming.kicks-ass.net> In-Reply-To: <20151013193928.GX3816@twins.programming.kicks-ass.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/13/2015 03:39 PM, Peter Zijlstra wrote: > On Tue, Sep 22, 2015 at 04:50:43PM -0400, Waiman Long wrote: >> This patch allows one attempt for the lock waiter to steal the lock >> when entering the PV slowpath. This helps to reduce the performance >> penalty caused by lock waiter preemption while not having much of >> the downsides of a real unfair lock. >> > Changelog does not explain the implementation, which is subtle enough to > warrant a few words. Will add more information into the changelog. > >> @@ -417,7 +415,8 @@ queue: >> * does not imply a full barrier. >> * >> */ >> - pv_wait_head(lock, node); >> + if (pv_wait_head_and_lock(lock, node, tail)) >> + goto release; > That's very much: pv_wait_head_or_lock(), maybe _or_steal() is even > better. I am not very good at naming function. Changing it to _or_steal() is fine for me. >> while ((val = smp_load_acquire(&lock->val.counter))& _Q_LOCKED_PENDING_MASK) >> cpu_relax(); >> >> @@ -454,7 +453,6 @@ queue: >> cpu_relax(); >> >> arch_mcs_spin_unlock_contended(&next->locked); >> - pv_kick_node(lock, next); > Not sure about removing that, breaks symmetry. > >> /* >> + * Allow one unfair trylock when entering the PV slowpath to reduce the >> + * performance impact of lock waiter preemption (either explicitly via >> + * pv_wait or implicitly via PLE). This function will be called once when >> + * a lock waiter enter the slowpath before being queued. >> + * >> + * A little bit of unfairness here can improve performance without many >> + * of the downsides of a real unfair lock. >> + */ >> +#define queued_spin_trylock(l) pv_queued_spin_trylock_unfair(l) >> +static inline bool pv_queued_spin_trylock_unfair(struct qspinlock *lock) >> +{ >> + struct __qspinlock *l = (void *)lock; >> + >> + if (READ_ONCE(l->locked)) >> + return 0; >> + /* >> + * Wait a bit here to ensure that an actively spinning queue head vCPU >> + * has a fair chance of getting the lock. >> + */ >> + cpu_relax(); >> + >> + return cmpxchg(&l->locked, 0, _Q_LOCKED_VAL) == 0; >> +} > This doesn't seem to make any sense.. Its also very much distinct from > the rest of the patch and can easily be added in a separate patch with > separate performance numbers to show it does (or does not) make a > difference. If you mean I don't need an extra cpu_relax() here, I can take that out. It was there to make the active queue head vCPU having a higher chance of getting the lock, but it is not essential. Cheers, Longman