From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751977AbdIEObv (ORCPT ); Tue, 5 Sep 2017 10:31:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43074 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751420AbdIEObt (ORCPT ); Tue, 5 Sep 2017 10:31:49 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B8AA51F583 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=longman@redhat.com Subject: Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function From: Waiman Long To: Juergen Gross , linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org, x86@kernel.org, virtualization@lists.linux-foundation.org Cc: jeremy@goop.org, chrisw@sous-sol.org, akataria@vmware.com, rusty@rustcorp.com.au, boris.ostrovsky@oracle.com, hpa@zytor.com, tglx@linutronix.de, mingo@redhat.com, peterz@infradead.org References: <20170905132444.7163-1-jgross@suse.com> <20170905132444.7163-4-jgross@suse.com> <42918500-4487-f0d8-e6fd-99e4d0d7f1bf@suse.com> <3a2eefe7-5c0b-9768-2bb8-03c947133b06@redhat.com> Organization: Red Hat Message-ID: Date: Tue, 5 Sep 2017 10:31:46 -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: <3a2eefe7-5c0b-9768-2bb8-03c947133b06@redhat.com> 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.30]); Tue, 05 Sep 2017 14:31:48 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/05/2017 10:24 AM, Waiman Long wrote: > On 09/05/2017 10:18 AM, Juergen Gross wrote: >> On 05/09/17 16:10, Waiman Long wrote: >>> On 09/05/2017 09:24 AM, Juergen Gross wrote: >>>> There are cases where a guest tries to switch spinlocks to bare metal >>>> behavior (e.g. by setting "xen_nopvspin" boot parameter). Today this >>>> has the downside of falling back to unfair test and set scheme for >>>> qspinlocks due to virt_spin_lock() detecting the virtualized >>>> environment. >>>> >>>> Make virt_spin_lock() a paravirt operation in order to enable users >>>> to select an explicit behavior like bare metal. >>>> >>>> Signed-off-by: Juergen Gross >>>> --- >>>> arch/x86/include/asm/paravirt.h | 5 ++++ >>>> arch/x86/include/asm/paravirt_types.h | 1 + >>>> arch/x86/include/asm/qspinlock.h | 48 ++++++++++++++++++++++++----------- >>>> arch/x86/kernel/paravirt-spinlocks.c | 14 ++++++++++ >>>> arch/x86/kernel/smpboot.c | 2 ++ >>>> 5 files changed, 55 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h >>>> index c25dd22f7c70..d9e954fb37df 100644 >>>> --- a/arch/x86/include/asm/paravirt.h >>>> +++ b/arch/x86/include/asm/paravirt.h >>>> @@ -725,6 +725,11 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu) >>>> return PVOP_CALLEE1(bool, pv_lock_ops.vcpu_is_preempted, cpu); >>>> } >>>> >>>> +static __always_inline bool pv_virt_spin_lock(struct qspinlock *lock) >>>> +{ >>>> + return PVOP_CALLEE1(bool, pv_lock_ops.virt_spin_lock, lock); >>>> +} >>>> + >>>> #endif /* SMP && PARAVIRT_SPINLOCKS */ >>>> >>>> #ifdef CONFIG_X86_32 >>>> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h >>>> index 19efefc0e27e..928f5e7953a7 100644 >>>> --- a/arch/x86/include/asm/paravirt_types.h >>>> +++ b/arch/x86/include/asm/paravirt_types.h >>>> @@ -319,6 +319,7 @@ struct pv_lock_ops { >>>> void (*kick)(int cpu); >>>> >>>> struct paravirt_callee_save vcpu_is_preempted; >>>> + struct paravirt_callee_save virt_spin_lock; >>>> } __no_randomize_layout; >>>> >>>> /* This contains all the paravirt structures: we get a convenient >>>> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h >>>> index 48a706f641f2..fbd98896385c 100644 >>>> --- a/arch/x86/include/asm/qspinlock.h >>>> +++ b/arch/x86/include/asm/qspinlock.h >>>> @@ -17,6 +17,25 @@ static inline void native_queued_spin_unlock(struct qspinlock *lock) >>>> smp_store_release((u8 *)lock, 0); >>>> } >>>> >>>> +static inline bool native_virt_spin_lock(struct qspinlock *lock) >>>> +{ >>>> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR)) >>>> + return false; >>>> + >>>> + /* >>>> + * On hypervisors without PARAVIRT_SPINLOCKS support we fall >>>> + * back to a Test-and-Set spinlock, because fair locks have >>>> + * horrible lock 'holder' preemption issues. >>>> + */ >>>> + >>>> + do { >>>> + while (atomic_read(&lock->val) != 0) >>>> + cpu_relax(); >>>> + } while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0); >>>> + >>>> + return true; >>>> +} >>>> + >>>> #ifdef CONFIG_PARAVIRT_SPINLOCKS >>>> extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val); >>>> extern void __pv_init_lock_hash(void); >>>> @@ -38,33 +57,32 @@ static inline bool vcpu_is_preempted(long cpu) >>>> { >>>> return pv_vcpu_is_preempted(cpu); >>>> } >>>> + >>>> +void native_pv_lock_init(void) __init; >>>> #else >>>> static inline void queued_spin_unlock(struct qspinlock *lock) >>>> { >>>> native_queued_spin_unlock(lock); >>>> } >>>> + >>>> +static inline void native_pv_lock_init(void) >>>> +{ >>>> +} >>>> #endif >>>> >>>> #ifdef CONFIG_PARAVIRT >>>> #define virt_spin_lock virt_spin_lock >>>> +#ifdef CONFIG_PARAVIRT_SPINLOCKS >>>> static inline bool virt_spin_lock(struct qspinlock *lock) >>>> { >>>> - if (!static_cpu_has(X86_FEATURE_HYPERVISOR)) >>>> - return false; >>> Have you consider just add one more jump label here to skip >>> virt_spin_lock when KVM or Xen want to do so? >> Why? Did you look at patch 4? This is the way to do it... > I asked this because of my performance concern as stated later in the email. For clarification, I was actually asking if you consider just adding one more jump label to skip it for Xen/KVM instead of making virt_spin_lock() a pv-op. Cheers, Longman