| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <404a9fab-03ff-174e-4ece-79932a6f302c@suse.com>
Date: Wed, 1 Nov 2017 16:51:43 +0100
From: Juergen Gross <jgross@...e.com>
To: Waiman Long <longman@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>, Jonathan Corbet <corbet@....net>
Cc: x86@...nel.org, virtualization@...ts.linux-foundation.org,
kvm@...r.kernel.org, xen-devel@...ts.xenproject.org,
linux-kernel@...r.kernel.org, Alok Kataria <akataria@...are.com>,
Rusty Russell <rusty@...tcorp.com.au>,
Paolo Bonzini <pbonzini@...hat.com>,
Radim Krčmář <rkrcmar@...hat.com>,
Boris Ostrovsky <boris.ostrovsky@...cle.com>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] x86/paravirt: Add kernel parameter to choose paravirt
lock type
On 01/11/17 16:32, Waiman Long wrote:
> Currently, there are 3 different lock types that can be chosen for
> the x86 architecture:
>
> - qspinlock
> - pvqspinlock
> - unfair lock
>
> One of the above lock types will be chosen at boot time depending on
> a number of different factors.
>
> Ideally, the hypervisors should be able to pick the best performing
> lock type for the current VM configuration. That is not currently
> the case as the performance of each lock type are affected by many
> different factors like the number of vCPUs in the VM, the amount vCPU
> overcommitment, the CPU type and so on.
>
> Generally speaking, unfair lock performs well for VMs with a small
> number of vCPUs. Native qspinlock may perform better than pvqspinlock
> if there is vCPU pinning and there is no vCPU over-commitment.
>
> This patch adds a new kernel parameter to allow administrator to
> choose the paravirt spinlock type to be used. VM administrators can
> experiment with the different lock types and choose one that can best
> suit their need, if they want to. Hypervisor developers can also use
> that to experiment with different lock types so that they can come
> up with a better algorithm to pick the best lock type.
>
> The hypervisor paravirt spinlock code will override this new parameter
> in determining if pvqspinlock should be used. The parameter, however,
> will override Xen's xen_nopvspin in term of disabling unfair lock.
Hmm, I'm not sure we need pvlock_type _and_ xen_nopvspin. What do others
think?
>
> Signed-off-by: Waiman Long <longman@...hat.com>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 7 +++++
> arch/x86/include/asm/paravirt.h | 9 ++++++
> arch/x86/kernel/kvm.c | 4 +++
> arch/x86/kernel/paravirt.c | 40 ++++++++++++++++++++++++-
> arch/x86/xen/spinlock.c | 6 ++--
> 5 files changed, 62 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index f7df49d..c98d9c7 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3275,6 +3275,13 @@
> [KNL] Number of legacy pty's. Overwrites compiled-in
> default number.
>
> + pvlock_type= [X86,PV_OPS]
> + Specify the paravirt spinlock type to be used.
> + Options are:
> + queued - native queued spinlock
> + pv - paravirt queued spinlock
> + unfair - simple TATAS unfair lock
> +
> quiet [KNL] Disable most log messages
>
> r128= [HW,DRM]
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index 12deec7..941a046 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -690,6 +690,15 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
>
> #endif /* SMP && PARAVIRT_SPINLOCKS */
>
> +enum pv_spinlock_type {
> + locktype_auto,
> + locktype_queued,
> + locktype_paravirt,
> + locktype_unfair,
> +};
> +
> +extern enum pv_spinlock_type pv_spinlock_type;
> +
> #ifdef CONFIG_X86_32
> #define PV_SAVE_REGS "pushl %ecx; pushl %edx;"
> #define PV_RESTORE_REGS "popl %edx; popl %ecx;"
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 8bb9594..3a5d3ec4 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -646,6 +646,10 @@ void __init kvm_spinlock_init(void)
> if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
> return;
>
> + if ((pv_spinlock_type == locktype_queued) ||
> + (pv_spinlock_type == locktype_unfair))
> + return;
> +
> __pv_init_lock_hash();
> pv_lock_ops.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
> pv_lock_ops.queued_spin_unlock = PV_CALLEE_SAVE(__pv_queued_spin_unlock);
> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> index 041096b..ca35cd3 100644
> --- a/arch/x86/kernel/paravirt.c
> +++ b/arch/x86/kernel/paravirt.c
> @@ -115,11 +115,48 @@ unsigned paravirt_patch_jmp(void *insnbuf, const void *target,
> return 5;
> }
>
> +/*
> + * The kernel argument "pvlock_type=" can be used to explicitly specify
> + * which type of spinlocks to be used. Currently, there are 3 options:
> + * 1) queued - the native queued spinlock
> + * 2) pv - the paravirt queued spinlock (if CONFIG_PARAVIRT_SPINLOCKS)
> + * 3) unfair - the simple TATAS unfair lock
> + *
> + * If this argument is not specified, the kernel will automatically choose
> + * an appropriate one depending on X86_FEATURE_HYPERVISOR and hypervisor
> + * specific settings.
> + */
> +enum pv_spinlock_type __read_mostly pv_spinlock_type = locktype_auto;
> +
> +static int __init pvlock_setup(char *s)
> +{
> + if (!s)
> + return -EINVAL;
> +
> + if (!strcmp(s, "queued"))
> + pv_spinlock_type = locktype_queued;
> + else if (!strcmp(s, "pv"))
> + pv_spinlock_type = locktype_paravirt;
> + else if (!strcmp(s, "unfair"))
> + pv_spinlock_type = locktype_unfair;
> + else
> + return -EINVAL;
> +
> + pr_info("PV lock type = %s (%d)\n", s, pv_spinlock_type);
> + return 0;
> +}
> +
> +early_param("pvlock_type", pvlock_setup);
> +
> DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
>
> void __init native_pv_lock_init(void)
> {
> - if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> + if (pv_spinlock_type == locktype_unfair)
> + return;
> +
> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
> + (pv_spinlock_type != locktype_auto))
> static_branch_disable(&virt_spin_lock_key);
Really? I don't think locktype_paravirt should disable the static key.
Juergen
Powered by blists - more mailing lists