Date: Thu, 21 Sep 2017 09:36:53 -0400
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Marcelo Tosatti <mtosatti@redhat.com>, peterz@infradead.org,
        mingo@redhat.com
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [patch 3/3] x86: kvm guest side support for KVM_HC_RT_PRIO
 hypercall
Message-ID: <20170921133653.GO26248@char.us.oracle.com>
References: <20170921113835.031375194@redhat.com>
 <20170921114039.466130276@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170921114039.466130276@redhat.com>
User-Agent: Mutt/1.8.3 (2017-05-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4132
Lines: 133

On Thu, Sep 21, 2017 at 08:38:38AM -0300, Marcelo Tosatti wrote:
> Add hypercalls to spinlock/unlock to set/unset FIFO priority
> for the vcpu, protected by a static branch to avoid performance
> increase in the normal kernels.
> 
> Enable option by "kvmfifohc" kernel command line parameter (disabled
> by default).

Wouldn't be better if there was a global 'kvm=' which could have the
various overrides?

> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> ---
>  arch/x86/kernel/kvm.c            |   31 +++++++++++++++++++++++++++++++
>  include/linux/spinlock.h         |    2 +-
>  include/linux/spinlock_api_smp.h |   17 +++++++++++++++++

Hm. It looks like you forgot to CC the maintainers:

$ scripts/get_maintainer.pl -f include/linux/spinlock.h
Peter Zijlstra <peterz@infradead.org> (maintainer:LOCKING PRIMITIVES)
Ingo Molnar <mingo@redhat.com> (maintainer:LOCKING PRIMITIVES)
linux-kernel@vger.kernel.org (open list:LOCKING PRIMITIVES)

Doing that for you.

>  3 files changed, 49 insertions(+), 1 deletion(-)
> 
> Index: kvm.fifopriohc-submit/arch/x86/kernel/kvm.c
> ===================================================================
> --- kvm.fifopriohc-submit.orig/arch/x86/kernel/kvm.c
> +++ kvm.fifopriohc-submit/arch/x86/kernel/kvm.c
> @@ -37,6 +37,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/nmi.h>
>  #include <linux/swait.h>
> +#include <linux/static_key.h>
>  #include <asm/timer.h>
>  #include <asm/cpu.h>
>  #include <asm/traps.h>
> @@ -321,6 +322,36 @@ static notrace void kvm_guest_apic_eoi_w
>  	apic->native_eoi_write(APIC_EOI, APIC_EOI_ACK);
>  }
>  
> +static int kvmfifohc;
> +
> +static int parse_kvmfifohc(char *arg)
> +{
> +	kvmfifohc = 1;
> +	return 0;
> +}
> +
> +early_param("kvmfifohc", parse_kvmfifohc);
> +
> +DEFINE_STATIC_KEY_FALSE(kvm_fifo_hc_key);
> +
> +static void kvm_init_fifo_hc(void)
> +{
> +	long ret;
> +
> +	ret = kvm_hypercall1(KVM_HC_RT_PRIO, 0);
> +
> +	if (ret == 0 && kvmfifohc == 1)
> +		static_branch_enable(&kvm_fifo_hc_key);
> +}
> +
> +static __init int kvmguest_late_init(void)
> +{
> +	kvm_init_fifo_hc();
> +	return 0;
> +}
> +
> +late_initcall(kvmguest_late_init);
> +
>  static void kvm_guest_cpu_init(void)
>  {
>  	if (!kvm_para_available())
> Index: kvm.fifopriohc-submit/include/linux/spinlock_api_smp.h
> ===================================================================
> --- kvm.fifopriohc-submit.orig/include/linux/spinlock_api_smp.h
> +++ kvm.fifopriohc-submit/include/linux/spinlock_api_smp.h
> @@ -136,11 +136,28 @@ static inline void __raw_spin_lock_bh(ra
>  	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
>  }
>  
> +#ifdef CONFIG_KVM_GUEST
> +DECLARE_STATIC_KEY_FALSE(kvm_fifo_hc_key);
> +#endif
> +
>  static inline void __raw_spin_lock(raw_spinlock_t *lock)
>  {
>  	preempt_disable();
> +
> +#if defined(CONFIG_KVM_GUEST) && defined(CONFIG_SMP)
> +	/* enable FIFO priority */
> +	if (static_branch_unlikely(&kvm_fifo_hc_key))
> +		kvm_hypercall1(KVM_HC_RT_PRIO, 0x1);
> +#endif

I am assuming the reason you choose not to wrap this in a pvops
or any other structure that is more of hypervisor agnostic is
that only KVM exposes this. But what if other hypervisors expose
something similar? Or some other mechanism similar to this?

> +
>  	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
>  	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> +
> +#if defined(CONFIG_KVM_GUEST) && defined(CONFIG_SMP)
> +	/* disable FIFO priority */
> +	if (static_branch_unlikely(&kvm_fifo_hc_key))
> +		kvm_hypercall1(KVM_HC_RT_PRIO, 0);
> +#endif
>  }
>  
>  #endif /* !CONFIG_GENERIC_LOCKBREAK || CONFIG_DEBUG_LOCK_ALLOC */
> Index: kvm.fifopriohc-submit/include/linux/spinlock.h
> ===================================================================
> --- kvm.fifopriohc-submit.orig/include/linux/spinlock.h
> +++ kvm.fifopriohc-submit/include/linux/spinlock.h
> @@ -56,7 +56,7 @@
>  #include <linux/stringify.h>
>  #include <linux/bottom_half.h>
>  #include <asm/barrier.h>
> -
> +#include <uapi/linux/kvm_para.h>
>  
>  /*
>   * Must define these before including other files, inline functions need them
> 
>