Re: [PATCH v7 10/11] pvqspinlock, x86: Enable qspinlock PV support for KVM

[Paolo Bonzini <pbonzini@redhat.com>]

Il 19/03/2014 21:14, Waiman Long ha scritto:
> This patch adds the necessary KVM specific code to allow KVM to support
> the sleeping and CPU kicking operations needed by the queue spinlock PV
> code.

The remaining problem of this patch is that you cannot get the 
generically optimal configuration: qspinlock on baremetal, PV qspinlock 
on KVM/Xen, unfair qspinlock on VMware/Hyper-V.  You really need to 
disable unfair locks in kvm_spinlock_init.

Paolo

> The XFS test had moderate spinlock contention of 1.6% whereas the
> ext4 test had heavy spinlock contention of 15.4% as reported by perf.
>
> A more interesting performance comparison is when there is
> overcommit. With both PV guests (all 20 CPUs equally shared with
> both guests - 200% overcommit) turned on and the "idle=poll" kernel
> option to simulate a busy guest. The results of running the same AIM7
> workloads are shown in the table below.
>
>   XFS Test:
>   kernel		 JPM	Real Time   Sys Time	Usr Time
>   -----			 ---	---------   --------	--------
>   PV ticketlock		597015	  29.99      424.44	 25.85
>   qspinlock		117493	 153.20	    1006.06	 59.60
>   PV qspinlock		616438	  29.20	     397.57	 23.31
>   unfair qspinlock	642398	  28.02	     396.42	 25.42
>   unfair + PV qspinlock	633803	  28.40	     400.45	 26.16
>
>   ext4 Test:
>   kernel		 JPM	Real Time   Sys Time	Usr Time
>   -----			 ---	---------   --------	--------
>   PV ticketlock		120984	 148.78     2378.98	 29.27
>   qspinlock		 54995	 327.30	    5023.58	 54.73
>   PV qspinlock		124215	 144.91	    2282.22	 28.89
>   unfair qspinlock	467411	  38.51	     481.80	 25.80
>   unfair + PV qspinlock	471080	  38.21	     482.40	 25.09
>
> The kernel build test (make -j 20) results are as follows:
>
>   kernel		Real Time   Sys Time	Usr Time
>   -----			---------   --------	--------
>   PV ticketlock		20m6.158s   41m7.167s	283m3.790s
>   qspinlock		26m41.294s  74m55.585s	346m31.981s
>   PV qspinlock		20m17.429s  41m19.434s	281m21.238s
>   unfair qspinlock	19m58.315s  40m18.011s	279m27.177s
>   unfair + PV qspinlock	20m0.030s   40m35.011s	278m50.522s
>
> With no overcommit, the PV code doesn't really do anything. The unfair
> lock does provide some performance advantage depending on the workload.
>
> In an overcommited PV guest, however, there can be a big performance
> benefit with PV and unfair lock. In term of spinlock contention,
> the ordering of the 3 workloads are:
>
>     kernel build < AIM7 disk xfs < AIM7 disk ext4
>
> With light spinlock contention, the PV ticketlock can be a bit
> faster than PV qspinlock. On moderate to high spinlock contention,
> PV qspinlock performs better. The unfair lock has the best performance
> in all cases, especially with heavy spinlock contention.
>
> Signed-off-by: Waiman Long <Waiman.Long@hp.com>
> ---
>  arch/x86/kernel/kvm.c |   82 +++++++++++++++++++++++++++++++++++++++++++++++++
>  kernel/Kconfig.locks  |    2 +-
>  2 files changed, 83 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index f318e78..c28bc1b 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -568,6 +568,7 @@ static void kvm_kick_cpu(int cpu)
>  	kvm_hypercall2(KVM_HC_KICK_CPU, flags, apicid);
>  }
>
> +#ifndef CONFIG_QUEUE_SPINLOCK
>  enum kvm_contention_stat {
>  	TAKEN_SLOW,
>  	TAKEN_SLOW_PICKUP,
> @@ -795,6 +796,82 @@ static void kvm_unlock_kick(struct arch_spinlock *lock, __ticket_t ticket)
>  		}
>  	}
>  }
> +#else /* !CONFIG_QUEUE_SPINLOCK */
> +
> +#ifdef CONFIG_KVM_DEBUG_FS
> +static struct dentry *d_spin_debug;
> +static struct dentry *d_kvm_debug;
> +static u32 kick_stats;		/* CPU kick count    */
> +static u32 hibernate_stats;	/* Hibernation count */
> +
> +static int __init kvm_spinlock_debugfs(void)
> +{
> +	d_kvm_debug = debugfs_create_dir("kvm-guest", NULL);
> +	if (!d_kvm_debug) {
> +		printk(KERN_WARNING
> +		       "Could not create 'kvm' debugfs directory\n");
> +		return -ENOMEM;
> +	}
> +	d_spin_debug = debugfs_create_dir("spinlocks", d_kvm_debug);
> +
> +	debugfs_create_u32("kick_stats", 0644, d_spin_debug, &kick_stats);
> +	debugfs_create_u32("hibernate_stats",
> +			   0644, d_spin_debug, &hibernate_stats);
> +	return 0;
> +}
> +
> +static inline void inc_kick_stats(void)
> +{
> +	add_smp(&kick_stats, 1);
> +}
> +
> +static inline void inc_hib_stats(void)
> +{
> +	add_smp(&hibernate_stats, 1);
> +}
> +
> +fs_initcall(kvm_spinlock_debugfs);
> +
> +#else /* CONFIG_KVM_DEBUG_FS */
> +static inline void inc_kick_stats(void)
> +{
> +}
> +
> +static inline void inc_hib_stats(void)
> +{
> +
> +}
> +#endif /* CONFIG_KVM_DEBUG_FS */
> +
> +static void kvm_kick_cpu_type(int cpu)
> +{
> +	kvm_kick_cpu(cpu);
> +	inc_kick_stats();
> +}
> +
> +/*
> + * Halt the current CPU & release it back to the host
> + */
> +static void kvm_hibernate(void)
> +{
> +	unsigned long flags;
> +
> +	if (in_nmi())
> +		return;
> +
> +	inc_hib_stats();
> +	/*
> +	 * Make sure an interrupt handler can't upset things in a
> +	 * partially setup state.
> +	 */
> +	local_irq_save(flags);
> +	if (arch_irqs_disabled_flags(flags))
> +		halt();
> +	else
> +		safe_halt();
> +	local_irq_restore(flags);
> +}
> +#endif /* !CONFIG_QUEUE_SPINLOCK */
>
>  /*
>   * Setup pv_lock_ops to exploit KVM_FEATURE_PV_UNHALT if present.
> @@ -807,8 +884,13 @@ void __init kvm_spinlock_init(void)
>  	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
>  		return;
>
> +#ifdef CONFIG_QUEUE_SPINLOCK
> +	pv_lock_ops.kick_cpu = kvm_kick_cpu_type;
> +	pv_lock_ops.hibernate = kvm_hibernate;
> +#else
>  	pv_lock_ops.lock_spinning = PV_CALLEE_SAVE(kvm_lock_spinning);
>  	pv_lock_ops.unlock_kick = kvm_unlock_kick;
> +#endif
>  }
>
>  static __init int kvm_spinlock_init_jump(void)
> diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
> index f185584..a70fdeb 100644
> --- a/kernel/Kconfig.locks
> +++ b/kernel/Kconfig.locks
> @@ -229,4 +229,4 @@ config ARCH_USE_QUEUE_SPINLOCK
>
>  config QUEUE_SPINLOCK
>  	def_bool y if ARCH_USE_QUEUE_SPINLOCK
> -	depends on SMP && !PARAVIRT_SPINLOCKS
> +	depends on SMP && (!PARAVIRT_SPINLOCKS || !XEN)
>