From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH RFC v6 09/11] pvqspinlock, x86: Add qspinlock para-virtualization support Date: Thu, 13 Mar 2014 14:57:45 +0100 Message-ID: <5321B959.5050305__39744.3527625457$1394719167$gmane$org@redhat.com> References: <1394650498-30118-1-git-send-email-Waiman.Long@hp.com> <1394650498-30118-10-git-send-email-Waiman.Long@hp.com> <5321949F.1010103@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WO68w-00022E-4f for xen-devel@lists.xenproject.org; Thu, 13 Mar 2014 13:57:54 +0000 Received: by mail-ee0-f43.google.com with SMTP id e53so431746eek.16 for ; Thu, 13 Mar 2014 06:57:51 -0700 (PDT) In-Reply-To: <5321949F.1010103@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel , Waiman Long Cc: Jeremy Fitzhardinge , Raghavendra K T , Gleb Natapov , Peter Zijlstra , virtualization@lists.linux-foundation.org, Andi Kleen , "H. Peter Anvin" , Michel Lespinasse , Alok Kataria , linux-arch@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org, Ingo Molnar , xen-devel@lists.xenproject.org, "Paul E. McKenney" , Arnd Bergmann , Scott J Norton , Steven Rostedt , Chris Wright , Thomas Gleixner , Aswin Chandramouleeswaran , Chegu Vinod , Oleg Nesterov List-Id: xen-devel@lists.xenproject.org Il 13/03/2014 12:21, David Vrabel ha scritto: > On 12/03/14 18:54, Waiman Long wrote: >> This patch adds para-virtualization support to the queue spinlock in >> the same way as was done in the PV ticket lock code. In essence, the >> lock waiters will spin for a specified number of times (QSPIN_THRESHOLD >> = 2^14) and then halted itself. The queue head waiter will spins >> 2*QSPIN_THRESHOLD times before halting itself. When it has spinned >> QSPIN_THRESHOLD times, the queue head will assume that the lock >> holder may be scheduled out and attempt to kick the lock holder CPU >> if it has the CPU number on hand. > > I don't really understand the reasoning for kicking the lock holder. I agree. If the lock holder isn't running, there's probably a good reason for that and going to sleep will not necessarily convince the scheduler to give more CPU to the lock holder. I think there are two choices: 1) use yield_to to donate part of the waiter's quantum to the lock holder? For this we probably need a new, separate hypercall interface. For KVM it would be the same as hlt in the guest but with an additional yield_to in the host. 2) do nothing, just go to sleep. Could you get (or do you have) numbers for (2)? More important, I think a barrier is missing: Lock holder --------------------------------------- // queue_spin_unlock barrier(); ACCESS_ONCE(qlock->lock) = 0; barrier(); // pv_kick_node: if (pv->cpustate != PV_CPU_HALTED) return; ACCESS_ONCE(pv->cpustate) = PV_CPU_KICKED; __queue_kick_cpu(pv->mycpu, PV_KICK_QUEUE_HEAD); Waiter ------------------------------------------- // pv_head_spin_check ACCESS_ONCE(pv->cpustate) = PV_CPU_HALTED; lockval = cmpxchg(&qlock->lock, _QSPINLOCK_LOCKED, _QSPINLOCK_LOCKED_SLOWPATH); if (lockval == 0) { /* * Can exit now as the lock is free */ ACCESS_ONCE(pv->cpustate) = PV_CPU_ACTIVE; *count = 0; return; } __queue_hibernate(); Nothing protects from writing qlock->lock before pv->cpustate is read, leading to this: Lock holder Waiter --------------------------------------------------------------- read pv->cpustate (it is PV_CPU_ACTIVE) pv->cpustate = PV_CPU_HALTED lockval = cmpxchg(...) hibernate() qlock->lock = 0 if (pv->cpustate != PV_CPU_HALTED) return; I think you need this: - if (pv->cpustate != PV_CPU_HALTED) - return; - ACCESS_ONCE(pv->cpustate) = PV_CPU_KICKED; + if (cmpxchg(pv->cpustate, PV_CPU_HALTED, PV_CPU_KICKED) + != PV_CPU_HALTED) + return; Paolo