[PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-06 Thread Eduardo Valentin 
Currently, the existing qspinlock implementation will fallback to
test-and-set if the hypervisor has not set the PV_UNHALT flag.

This patch gives the opportunity to guest kernels to select
between test-and-set and the regular queueu fair lock implementation
based on the PV_DEDICATED KVM feature flag. When the PV_DEDICATED
flag is not set, the code will still fall back to test-and-set,
but when the PV_DEDICATED flag is set, the code will use
the regular queue spinlock implementation.

With this patch, when in autoselect mode, the guest will
use the default spinlock implementation based on host feature
flags as follows:

PV_DEDICATED = 1, PV_UNHALT = anything: default is qspinlock
PV_DEDICATED = 0, PV_UNHALT = 1: default is pvqspinlock
PV_DEDICATED = 0, PV_UNHALT = 0: default is tas

Cc: Paolo Bonzini 
Cc: "Radim Krčmář" 
Cc: Jonathan Corbet 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: x...@kernel.org
Cc: Peter Zijlstra 
Cc: Waiman Long 
Cc: k...@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Cc: Jan H. Schoenherr 
Cc: Anthony Liguori 
Suggested-by: Matt Wilson 
Signed-off-by: Eduardo Valentin 
---
V3:
 - When PV_DEDICATED is set (1), qspinlock is selected,
   regardless of the value of PV_UNHAULT. Suggested by Paolo Bonzini. 
 - Refreshed on top of tip/master.
V2:
 - rebase on top of tip/master

 Documentation/virtual/kvm/cpuid.txt  | 6 ++
 arch/x86/include/asm/qspinlock.h | 4 
 arch/x86/include/uapi/asm/kvm_para.h | 1 +
 arch/x86/kernel/kvm.c| 2 ++
 4 files changed, 13 insertions(+)

diff --git a/Documentation/virtual/kvm/cpuid.txt 
b/Documentation/virtual/kvm/cpuid.txt
index 3c65feb..117066a 100644
--- a/Documentation/virtual/kvm/cpuid.txt
+++ b/Documentation/virtual/kvm/cpuid.txt
@@ -54,6 +54,12 @@ KVM_FEATURE_PV_UNHALT  || 7 || guest checks 
this feature bit
||   || before enabling paravirtualized
||   || spinlock support.
 --
+KVM_FEATURE_PV_DEDICATED   || 8 || guest checks this feature bit
+   ||   || to determine if they run on
+   ||   || dedicated vCPUs, allowing opti-
+   ||   || mizations such as usage of
+   ||   || qspinlocks.
+--
 KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||24 || host will warn if no guest-side
||   || per-cpu warps are expected in
||   || kvmclock.
diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
index 5e16b5d..de42694 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -3,6 +3,8 @@
 #define _ASM_X86_QSPINLOCK_H
 
 #include 
+#include 
+
 #include 
 #include 
 #include 
@@ -58,6 +60,8 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
if (!static_branch_likely(&virt_spin_lock_key))
return false;
 
+   if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
+   return false;
/*
 * On hypervisors without PARAVIRT_SPINLOCKS support we fall
 * back to a Test-and-Set spinlock, because fair locks have
diff --git a/arch/x86/include/uapi/asm/kvm_para.h 
b/arch/x86/include/uapi/asm/kvm_para.h
index 554aa8f..85a9875 100644
--- a/arch/x86/include/uapi/asm/kvm_para.h
+++ b/arch/x86/include/uapi/asm/kvm_para.h
@@ -25,6 +25,7 @@
 #define KVM_FEATURE_STEAL_TIME 5
 #define KVM_FEATURE_PV_EOI 6
 #define KVM_FEATURE_PV_UNHALT  7
+#define KVM_FEATURE_PV_DEDICATED   8
 
 /* The last 8 bits are used to indicate how to interpret the flags field
  * in pvclock structure. If no bits are set, all flags are ignored.
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 8bb9594..dacd7cf 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -642,6 +642,8 @@ void __init kvm_spinlock_init(void)
 {
if (!kvm_para_available())
return;
+   if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
+   return;
/* Does host kernel support KVM_FEATURE_PV_UNHALT? */
if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
return;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-07 Thread Paolo Bonzini 
On 06/11/2017 21:26, Eduardo Valentin wrote:
> Currently, the existing qspinlock implementation will fallback to
> test-and-set if the hypervisor has not set the PV_UNHALT flag.
> 
> This patch gives the opportunity to guest kernels to select
> between test-and-set and the regular queueu fair lock implementation
> based on the PV_DEDICATED KVM feature flag. When the PV_DEDICATED
> flag is not set, the code will still fall back to test-and-set,
> but when the PV_DEDICATED flag is set, the code will use
> the regular queue spinlock implementation.
> 
> With this patch, when in autoselect mode, the guest will
> use the default spinlock implementation based on host feature
> flags as follows:
> 
> PV_DEDICATED = 1, PV_UNHALT = anything: default is qspinlock
> PV_DEDICATED = 0, PV_UNHALT = 1: default is pvqspinlock
> PV_DEDICATED = 0, PV_UNHALT = 0: default is tas

Hi Eduardo,

besides the suggestion to use a separate word than the one for features,
is this still needed after Waiman's patch to adaptively switch between
tas and pvqspinlock?

Paolo

> Cc: Paolo Bonzini 
> Cc: "Radim Krčmář" 
> Cc: Jonathan Corbet 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: "H. Peter Anvin" 
> Cc: x...@kernel.org
> Cc: Peter Zijlstra 
> Cc: Waiman Long 
> Cc: k...@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Cc: Jan H. Schoenherr 
> Cc: Anthony Liguori 
> Suggested-by: Matt Wilson 
> Signed-off-by: Eduardo Valentin 
> ---
> V3:
>  - When PV_DEDICATED is set (1), qspinlock is selected,
>regardless of the value of PV_UNHAULT. Suggested by Paolo Bonzini. 
>  - Refreshed on top of tip/master.
> V2:
>  - rebase on top of tip/master
> 
>  Documentation/virtual/kvm/cpuid.txt  | 6 ++
>  arch/x86/include/asm/qspinlock.h | 4 
>  arch/x86/include/uapi/asm/kvm_para.h | 1 +
>  arch/x86/kernel/kvm.c| 2 ++
>  4 files changed, 13 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/cpuid.txt 
> b/Documentation/virtual/kvm/cpuid.txt
> index 3c65feb..117066a 100644
> --- a/Documentation/virtual/kvm/cpuid.txt
> +++ b/Documentation/virtual/kvm/cpuid.txt
> @@ -54,6 +54,12 @@ KVM_FEATURE_PV_UNHALT  || 7 || guest 
> checks this feature bit
> ||   || before enabling 
> paravirtualized
> ||   || spinlock support.
>  
> --
> +KVM_FEATURE_PV_DEDICATED   || 8 || guest checks this feature bit
> +   ||   || to determine if they run on
> +   ||   || dedicated vCPUs, allowing 
> opti-
> +   ||   || mizations such as usage of
> +   ||   || qspinlocks.
> +--
>  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||24 || host will warn if no 
> guest-side
> ||   || per-cpu warps are expected in
> ||   || kvmclock.
> diff --git a/arch/x86/include/asm/qspinlock.h 
> b/arch/x86/include/asm/qspinlock.h
> index 5e16b5d..de42694 100644
> --- a/arch/x86/include/asm/qspinlock.h
> +++ b/arch/x86/include/asm/qspinlock.h
> @@ -3,6 +3,8 @@
>  #define _ASM_X86_QSPINLOCK_H
>  
>  #include 
> +#include 
> +
>  #include 
>  #include 
>  #include 
> @@ -58,6 +60,8 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
>   if (!static_branch_likely(&virt_spin_lock_key))
>   return false;
>  
> + if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
> + return false;
>   /*
>* On hypervisors without PARAVIRT_SPINLOCKS support we fall
>* back to a Test-and-Set spinlock, because fair locks have
> diff --git a/arch/x86/include/uapi/asm/kvm_para.h 
> b/arch/x86/include/uapi/asm/kvm_para.h
> index 554aa8f..85a9875 100644
> --- a/arch/x86/include/uapi/asm/kvm_para.h
> +++ b/arch/x86/include/uapi/asm/kvm_para.h
> @@ -25,6 +25,7 @@
>  #define KVM_FEATURE_STEAL_TIME   5
>  #define KVM_FEATURE_PV_EOI   6
>  #define KVM_FEATURE_PV_UNHALT7
> +#define KVM_FEATURE_PV_DEDICATED 8
>  
>  /* The last 8 bits are used to indicate how to interpret the flags field
>   * in pvclock structure. If no bits are set, all flags are ignored.
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 8bb9594..dacd7cf 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -642,6 +642,8 @@ void __init kvm_spinlock_init(void)
>  {
>   if (!kvm_para_available())
>   return;
> + if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
> + return;
>   /* Does host kernel support KVM_FEATURE_PV_UNHALT? */
>   if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
>   return;
> 

--
To unsubscribe from this list: sen

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-07 Thread Eduardo Valentin 
On Tue, Nov 07, 2017 at 01:23:56PM +0100, Paolo Bonzini wrote:
> On 06/11/2017 21:26, Eduardo Valentin wrote:
> > Currently, the existing qspinlock implementation will fallback to
> > test-and-set if the hypervisor has not set the PV_UNHALT flag.
> > 
> > This patch gives the opportunity to guest kernels to select
> > between test-and-set and the regular queueu fair lock implementation
> > based on the PV_DEDICATED KVM feature flag. When the PV_DEDICATED
> > flag is not set, the code will still fall back to test-and-set,
> > but when the PV_DEDICATED flag is set, the code will use
> > the regular queue spinlock implementation.
> > 
> > With this patch, when in autoselect mode, the guest will
> > use the default spinlock implementation based on host feature
> > flags as follows:
> > 
> > PV_DEDICATED = 1, PV_UNHALT = anything: default is qspinlock
> > PV_DEDICATED = 0, PV_UNHALT = 1: default is pvqspinlock
> > PV_DEDICATED = 0, PV_UNHALT = 0: default is tas
> 
> Hi Eduardo,
> 
> besides the suggestion to use a separate word than the one for features,

Ok. I will take a look.

> is this still needed after Waiman's patch to adaptively switch between
> tas and pvqspinlock?

Can you please point me to it ? Is it already in tip/master?

> 
> Paolo
> 
> > Cc: Paolo Bonzini 
> > Cc: "Radim Krčmář" 
> > Cc: Jonathan Corbet 
> > Cc: Thomas Gleixner 
> > Cc: Ingo Molnar 
> > Cc: "H. Peter Anvin" 
> > Cc: x...@kernel.org
> > Cc: Peter Zijlstra 
> > Cc: Waiman Long 
> > Cc: k...@vger.kernel.org
> > Cc: linux-doc@vger.kernel.org
> > Cc: linux-ker...@vger.kernel.org
> > Cc: Jan H. Schoenherr 
> > Cc: Anthony Liguori 
> > Suggested-by: Matt Wilson 
> > Signed-off-by: Eduardo Valentin 
> > ---
> > V3:
> >  - When PV_DEDICATED is set (1), qspinlock is selected,
> >regardless of the value of PV_UNHAULT. Suggested by Paolo Bonzini. 
> >  - Refreshed on top of tip/master.
> > V2:
> >  - rebase on top of tip/master
> > 
> >  Documentation/virtual/kvm/cpuid.txt  | 6 ++
> >  arch/x86/include/asm/qspinlock.h | 4 
> >  arch/x86/include/uapi/asm/kvm_para.h | 1 +
> >  arch/x86/kernel/kvm.c| 2 ++
> >  4 files changed, 13 insertions(+)
> > 
> > diff --git a/Documentation/virtual/kvm/cpuid.txt 
> > b/Documentation/virtual/kvm/cpuid.txt
> > index 3c65feb..117066a 100644
> > --- a/Documentation/virtual/kvm/cpuid.txt
> > +++ b/Documentation/virtual/kvm/cpuid.txt
> > @@ -54,6 +54,12 @@ KVM_FEATURE_PV_UNHALT  || 7 || guest 
> > checks this feature bit
> > ||   || before enabling 
> > paravirtualized
> > ||   || spinlock support.
> >  
> > --
> > +KVM_FEATURE_PV_DEDICATED   || 8 || guest checks this feature 
> > bit
> > +   ||   || to determine if they run on
> > +   ||   || dedicated vCPUs, allowing 
> > opti-
> > +   ||   || mizations such as usage of
> > +   ||   || qspinlocks.
> > +--
> >  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||24 || host will warn if no 
> > guest-side
> > ||   || per-cpu warps are expected 
> > in
> > ||   || kvmclock.
> > diff --git a/arch/x86/include/asm/qspinlock.h 
> > b/arch/x86/include/asm/qspinlock.h
> > index 5e16b5d..de42694 100644
> > --- a/arch/x86/include/asm/qspinlock.h
> > +++ b/arch/x86/include/asm/qspinlock.h
> > @@ -3,6 +3,8 @@
> >  #define _ASM_X86_QSPINLOCK_H
> >  
> >  #include 
> > +#include 
> > +
> >  #include 
> >  #include 
> >  #include 
> > @@ -58,6 +60,8 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
> > if (!static_branch_likely(&virt_spin_lock_key))
> > return false;
> >  
> > +   if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
> > +   return false;
> > /*
> >  * On hypervisors without PARAVIRT_SPINLOCKS support we fall
> >  * back to a Test-and-Set spinlock, because fair locks have
> > diff --git a/arch/x86/include/uapi/asm/kvm_para.h 
> > b/arch/x86/include/uapi/asm/kvm_para.h
> > index 554aa8f..85a9875 100644
> > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > @@ -25,6 +25,7 @@
> >  #define KVM_FEATURE_STEAL_TIME 5
> >  #define KVM_FEATURE_PV_EOI 6
> >  #define KVM_FEATURE_PV_UNHALT  7
> > +#define KVM_FEATURE_PV_DEDICATED   8
> >  
> >  /* The last 8 bits are used to indicate how to interpret the flags field
> >   * in pvclock structure. If no bits are set, all flags are ignored.
> > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> > index 8bb9594..dacd7cf 100644
> > --- a/arch/x86/kernel/kvm.c
> > +++ b/arch/x86/kernel/kvm.c
> > @@ -642

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-07 Thread Paolo Bonzini 
On 07/11/2017 13:39, Eduardo Valentin wrote:
>> is this still needed after Waiman's patch to adaptively switch between
>> tas and pvqspinlock?
> Can you please point me to it ? Is it already in tip/master?
> 

No, he just posted it:

https://marc.info/?l=linux-kernel&m=150972337909996&w=2

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-08 Thread Eduardo Valentin 
Paolo,

On Tue, Nov 07, 2017 at 01:43:15PM +0100, Paolo Bonzini wrote:
> On 07/11/2017 13:39, Eduardo Valentin wrote:
> >> is this still needed after Waiman's patch to adaptively switch between
> >> tas and pvqspinlock?
> > Can you please point me to it ? Is it already in tip/master?
> > 
> 
> No, he just posted it:
> 
> https://marc.info/?l=linux-kernel&m=150972337909996&w=2

OK, Thanks. I've reviewed his V2. I think the patch to have PV_DEDICATED is 
still interesting to have,
for the case the guest is in autoselect mode and honors what the host gives as 
hints.

> 
> Paolo

-- 
All the best,
Eduardo Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-08 Thread Radim Krčmář 
2017-11-06 12:26-0800, Eduardo Valentin:
> Currently, the existing qspinlock implementation will fallback to
> test-and-set if the hypervisor has not set the PV_UNHALT flag.
> 
> This patch gives the opportunity to guest kernels to select
> between test-and-set and the regular queueu fair lock implementation
> based on the PV_DEDICATED KVM feature flag. When the PV_DEDICATED
> flag is not set, the code will still fall back to test-and-set,
> but when the PV_DEDICATED flag is set, the code will use
> the regular queue spinlock implementation.
> 
> With this patch, when in autoselect mode, the guest will
> use the default spinlock implementation based on host feature
> flags as follows:
> 
> PV_DEDICATED = 1, PV_UNHALT = anything: default is qspinlock
> PV_DEDICATED = 0, PV_UNHALT = 1: default is pvqspinlock
> PV_DEDICATED = 0, PV_UNHALT = 0: default is tas
> 
> Cc: Paolo Bonzini 
> Cc: "Radim Krčmář" 
> Cc: Jonathan Corbet 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: "H. Peter Anvin" 
> Cc: x...@kernel.org
> Cc: Peter Zijlstra 
> Cc: Waiman Long 
> Cc: k...@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Cc: Jan H. Schoenherr 
> Cc: Anthony Liguori 
> Suggested-by: Matt Wilson 
> Signed-off-by: Eduardo Valentin 
> ---
> V3:
>  - When PV_DEDICATED is set (1), qspinlock is selected,
>regardless of the value of PV_UNHAULT. Suggested by Paolo Bonzini. 
>  - Refreshed on top of tip/master.
> V2:
>  - rebase on top of tip/master
> 
>  Documentation/virtual/kvm/cpuid.txt  | 6 ++
>  arch/x86/include/asm/qspinlock.h | 4 
>  arch/x86/include/uapi/asm/kvm_para.h | 1 +
>  arch/x86/kernel/kvm.c| 2 ++
>  4 files changed, 13 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/cpuid.txt 
> b/Documentation/virtual/kvm/cpuid.txt
> index 3c65feb..117066a 100644
> --- a/Documentation/virtual/kvm/cpuid.txt
> +++ b/Documentation/virtual/kvm/cpuid.txt
> @@ -54,6 +54,12 @@ KVM_FEATURE_PV_UNHALT  || 7 || guest 
> checks this feature bit
> ||   || before enabling 
> paravirtualized
> ||   || spinlock support.
>  
> --
> +KVM_FEATURE_PV_DEDICATED   || 8 || guest checks this feature bit
> +   ||   || to determine if they run on
> +   ||   || dedicated vCPUs, allowing 
> opti-
> +   ||   || mizations such as usage of
> +   ||   || qspinlocks.
> +--
>  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||24 || host will warn if no 
> guest-side
> ||   || per-cpu warps are expected in
> ||   || kvmclock.
> diff --git a/arch/x86/include/asm/qspinlock.h 
> b/arch/x86/include/asm/qspinlock.h
> index 5e16b5d..de42694 100644
> --- a/arch/x86/include/asm/qspinlock.h
> +++ b/arch/x86/include/asm/qspinlock.h
> @@ -3,6 +3,8 @@
>  #define _ASM_X86_QSPINLOCK_H
>  
>  #include 
> +#include 
> +
>  #include 
>  #include 
>  #include 
> @@ -58,6 +60,8 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
>   if (!static_branch_likely(&virt_spin_lock_key))
>   return false;
>  
> + if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
> + return false;

Hm, every spinlock slowpath calls cpuid, which causes a VM exit, so I
wouldn't expect it to be faster than the existing implementations.
(Using the static key would be better.)

How does this patch perform compared to user-forced qspinlock and hybrid
pvqspinlock?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Eduardo Valentin 
Hello,

On Wed, Nov 08, 2017 at 06:36:52PM +0100, Radim Krčmář wrote:
> 2017-11-06 12:26-0800, Eduardo Valentin:
> > Currently, the existing qspinlock implementation will fallback to
> > test-and-set if the hypervisor has not set the PV_UNHALT flag.
> > 
> > This patch gives the opportunity to guest kernels to select
> > between test-and-set and the regular queueu fair lock implementation
> > based on the PV_DEDICATED KVM feature flag. When the PV_DEDICATED
> > flag is not set, the code will still fall back to test-and-set,
> > but when the PV_DEDICATED flag is set, the code will use
> > the regular queue spinlock implementation.
> > 
> > With this patch, when in autoselect mode, the guest will
> > use the default spinlock implementation based on host feature
> > flags as follows:
> > 
> > PV_DEDICATED = 1, PV_UNHALT = anything: default is qspinlock
> > PV_DEDICATED = 0, PV_UNHALT = 1: default is pvqspinlock
> > PV_DEDICATED = 0, PV_UNHALT = 0: default is tas
> > 
> > Cc: Paolo Bonzini 
> > Cc: "Radim Krčmář" 
> > Cc: Jonathan Corbet 
> > Cc: Thomas Gleixner 
> > Cc: Ingo Molnar 
> > Cc: "H. Peter Anvin" 
> > Cc: x...@kernel.org
> > Cc: Peter Zijlstra 
> > Cc: Waiman Long 
> > Cc: k...@vger.kernel.org
> > Cc: linux-doc@vger.kernel.org
> > Cc: linux-ker...@vger.kernel.org
> > Cc: Jan H. Schoenherr 
> > Cc: Anthony Liguori 
> > Suggested-by: Matt Wilson 
> > Signed-off-by: Eduardo Valentin 
> > ---
> > V3:
> >  - When PV_DEDICATED is set (1), qspinlock is selected,
> >regardless of the value of PV_UNHAULT. Suggested by Paolo Bonzini. 
> >  - Refreshed on top of tip/master.
> > V2:
> >  - rebase on top of tip/master
> > 
> >  Documentation/virtual/kvm/cpuid.txt  | 6 ++
> >  arch/x86/include/asm/qspinlock.h | 4 
> >  arch/x86/include/uapi/asm/kvm_para.h | 1 +
> >  arch/x86/kernel/kvm.c| 2 ++
> >  4 files changed, 13 insertions(+)
> > 
> > diff --git a/Documentation/virtual/kvm/cpuid.txt 
> > b/Documentation/virtual/kvm/cpuid.txt
> > index 3c65feb..117066a 100644
> > --- a/Documentation/virtual/kvm/cpuid.txt
> > +++ b/Documentation/virtual/kvm/cpuid.txt
> > @@ -54,6 +54,12 @@ KVM_FEATURE_PV_UNHALT  || 7 || guest 
> > checks this feature bit
> > ||   || before enabling 
> > paravirtualized
> > ||   || spinlock support.
> >  
> > --
> > +KVM_FEATURE_PV_DEDICATED   || 8 || guest checks this feature 
> > bit
> > +   ||   || to determine if they run on
> > +   ||   || dedicated vCPUs, allowing 
> > opti-
> > +   ||   || mizations such as usage of
> > +   ||   || qspinlocks.
> > +--
> >  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||24 || host will warn if no 
> > guest-side
> > ||   || per-cpu warps are expected 
> > in
> > ||   || kvmclock.
> > diff --git a/arch/x86/include/asm/qspinlock.h 
> > b/arch/x86/include/asm/qspinlock.h
> > index 5e16b5d..de42694 100644
> > --- a/arch/x86/include/asm/qspinlock.h
> > +++ b/arch/x86/include/asm/qspinlock.h
> > @@ -3,6 +3,8 @@
> >  #define _ASM_X86_QSPINLOCK_H
> >  
> >  #include 
> > +#include 
> > +
> >  #include 
> >  #include 
> >  #include 
> > @@ -58,6 +60,8 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
> > if (!static_branch_likely(&virt_spin_lock_key))
> > return false;
> >  
> > +   if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
> > +   return false;
> 
> Hm, every spinlock slowpath calls cpuid, which causes a VM exit, so I
> wouldn't expect it to be faster than the existing implementations.
> (Using the static key would be better.)
> 
> How does this patch perform compared to user-forced qspinlock and hybrid
> pvqspinlock?

This patch should have same effect as user-forced qspinlock. However, the key 
aspect
here is this patch gives a way for the host to instruct the guest to use 
qspinlock.
Even with Longman's patch which allows guest to select the spinlock 
implementation,
there should still be the auto-select mode. In such mode, PV_DEDICATED should
allow the host to get the guest to use qspinlock, without, the guest will 
fallback
to tas when PV_UNHALT == 0.

> 
> Thanks.
> 

-- 
All the best,
Eduardo Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Wanpeng Li 
2017-11-07 4:26 GMT+08:00 Eduardo Valentin :
> Currently, the existing qspinlock implementation will fallback to
> test-and-set if the hypervisor has not set the PV_UNHALT flag.
>
> This patch gives the opportunity to guest kernels to select
> between test-and-set and the regular queueu fair lock implementation
> based on the PV_DEDICATED KVM feature flag. When the PV_DEDICATED
> flag is not set, the code will still fall back to test-and-set,
> but when the PV_DEDICATED flag is set, the code will use
> the regular queue spinlock implementation.
>
> With this patch, when in autoselect mode, the guest will
> use the default spinlock implementation based on host feature
> flags as follows:
>
> PV_DEDICATED = 1, PV_UNHALT = anything: default is qspinlock
> PV_DEDICATED = 0, PV_UNHALT = 1: default is pvqspinlock
> PV_DEDICATED = 0, PV_UNHALT = 0: default is tas
>
> Cc: Paolo Bonzini 
> Cc: "Radim Krčmář" 
> Cc: Jonathan Corbet 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: "H. Peter Anvin" 
> Cc: x...@kernel.org
> Cc: Peter Zijlstra 
> Cc: Waiman Long 
> Cc: k...@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Cc: Jan H. Schoenherr 
> Cc: Anthony Liguori 
> Suggested-by: Matt Wilson 
> Signed-off-by: Eduardo Valentin 
> ---
> V3:
>  - When PV_DEDICATED is set (1), qspinlock is selected,
>regardless of the value of PV_UNHAULT. Suggested by Paolo Bonzini.
>  - Refreshed on top of tip/master.
> V2:
>  - rebase on top of tip/master
>
>  Documentation/virtual/kvm/cpuid.txt  | 6 ++
>  arch/x86/include/asm/qspinlock.h | 4 
>  arch/x86/include/uapi/asm/kvm_para.h | 1 +
>  arch/x86/kernel/kvm.c| 2 ++
>  4 files changed, 13 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/cpuid.txt 
> b/Documentation/virtual/kvm/cpuid.txt
> index 3c65feb..117066a 100644
> --- a/Documentation/virtual/kvm/cpuid.txt
> +++ b/Documentation/virtual/kvm/cpuid.txt
> @@ -54,6 +54,12 @@ KVM_FEATURE_PV_UNHALT  || 7 || guest 
> checks this feature bit
> ||   || before enabling 
> paravirtualized
> ||   || spinlock support.
>  
> --
> +KVM_FEATURE_PV_DEDICATED   || 8 || guest checks this feature bit
> +   ||   || to determine if they run on
> +   ||   || dedicated vCPUs, allowing 
> opti-
> +   ||   || mizations such as usage of
> +   ||   || qspinlocks.
> +--
>  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||24 || host will warn if no 
> guest-side
> ||   || per-cpu warps are expected in
> ||   || kvmclock.
> diff --git a/arch/x86/include/asm/qspinlock.h 
> b/arch/x86/include/asm/qspinlock.h
> index 5e16b5d..de42694 100644
> --- a/arch/x86/include/asm/qspinlock.h
> +++ b/arch/x86/include/asm/qspinlock.h
> @@ -3,6 +3,8 @@
>  #define _ASM_X86_QSPINLOCK_H
>
>  #include 
> +#include 
> +
>  #include 
>  #include 
>  #include 
> @@ -58,6 +60,8 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
> if (!static_branch_likely(&virt_spin_lock_key))
> return false;
>
> +   if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
> +   return false;
> /*
>  * On hypervisors without PARAVIRT_SPINLOCKS support we fall
>  * back to a Test-and-Set spinlock, because fair locks have
> diff --git a/arch/x86/include/uapi/asm/kvm_para.h 
> b/arch/x86/include/uapi/asm/kvm_para.h
> index 554aa8f..85a9875 100644
> --- a/arch/x86/include/uapi/asm/kvm_para.h
> +++ b/arch/x86/include/uapi/asm/kvm_para.h
> @@ -25,6 +25,7 @@
>  #define KVM_FEATURE_STEAL_TIME 5
>  #define KVM_FEATURE_PV_EOI 6
>  #define KVM_FEATURE_PV_UNHALT  7
> +#define KVM_FEATURE_PV_DEDICATED   8
>
>  /* The last 8 bits are used to indicate how to interpret the flags field
>   * in pvclock structure. If no bits are set, all flags are ignored.
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 8bb9594..dacd7cf 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -642,6 +642,8 @@ void __init kvm_spinlock_init(void)
>  {
> if (!kvm_para_available())
> return;
> +   if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
> +   return;
> /* Does host kernel support KVM_FEATURE_PV_UNHALT? */
> if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
> return;
> --
> 2.7.4
>

You should also add a cpuid flag in kvm part.

Regards,
Wanpeng Li
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majo

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Radim Krčmář 
2017-11-09 00:55-0800, Eduardo Valentin:
> Hello,
> 
> On Wed, Nov 08, 2017 at 06:36:52PM +0100, Radim Krčmář wrote:
> > 2017-11-06 12:26-0800, Eduardo Valentin:
> > > Currently, the existing qspinlock implementation will fallback to
> > > test-and-set if the hypervisor has not set the PV_UNHALT flag.
> > > 
> > > This patch gives the opportunity to guest kernels to select
> > > between test-and-set and the regular queueu fair lock implementation
> > > based on the PV_DEDICATED KVM feature flag. When the PV_DEDICATED
> > > flag is not set, the code will still fall back to test-and-set,
> > > but when the PV_DEDICATED flag is set, the code will use
> > > the regular queue spinlock implementation.
> > > 
> > > With this patch, when in autoselect mode, the guest will
> > > use the default spinlock implementation based on host feature
> > > flags as follows:
> > > 
> > > PV_DEDICATED = 1, PV_UNHALT = anything: default is qspinlock
> > > PV_DEDICATED = 0, PV_UNHALT = 1: default is pvqspinlock
> > > PV_DEDICATED = 0, PV_UNHALT = 0: default is tas
> > > 
> > > Cc: Paolo Bonzini 
> > > Cc: "Radim Krčmář" 
> > > Cc: Jonathan Corbet 
> > > Cc: Thomas Gleixner 
> > > Cc: Ingo Molnar 
> > > Cc: "H. Peter Anvin" 
> > > Cc: x...@kernel.org
> > > Cc: Peter Zijlstra 
> > > Cc: Waiman Long 
> > > Cc: k...@vger.kernel.org
> > > Cc: linux-doc@vger.kernel.org
> > > Cc: linux-ker...@vger.kernel.org
> > > Cc: Jan H. Schoenherr 
> > > Cc: Anthony Liguori 
> > > Suggested-by: Matt Wilson 
> > > Signed-off-by: Eduardo Valentin 
> > > ---
> > > V3:
> > >  - When PV_DEDICATED is set (1), qspinlock is selected,
> > >regardless of the value of PV_UNHAULT. Suggested by Paolo Bonzini. 
> > >  - Refreshed on top of tip/master.
> > > V2:
> > >  - rebase on top of tip/master
> > > 
> > >  Documentation/virtual/kvm/cpuid.txt  | 6 ++
> > >  arch/x86/include/asm/qspinlock.h | 4 
> > >  arch/x86/include/uapi/asm/kvm_para.h | 1 +
> > >  arch/x86/kernel/kvm.c| 2 ++
> > >  4 files changed, 13 insertions(+)
> > > 
> > > diff --git a/Documentation/virtual/kvm/cpuid.txt 
> > > b/Documentation/virtual/kvm/cpuid.txt
> > > index 3c65feb..117066a 100644
> > > --- a/Documentation/virtual/kvm/cpuid.txt
> > > +++ b/Documentation/virtual/kvm/cpuid.txt
> > > @@ -54,6 +54,12 @@ KVM_FEATURE_PV_UNHALT  || 7 || guest 
> > > checks this feature bit
> > > ||   || before enabling 
> > > paravirtualized
> > > ||   || spinlock support.
> > >  
> > > --
> > > +KVM_FEATURE_PV_DEDICATED   || 8 || guest checks this feature 
> > > bit
> > > +   ||   || to determine if they run 
> > > on
> > > +   ||   || dedicated vCPUs, allowing 
> > > opti-
> > > +   ||   || mizations such as usage of
> > > +   ||   || qspinlocks.
> > > +--
> > >  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||24 || host will warn if no 
> > > guest-side
> > > ||   || per-cpu warps are 
> > > expected in
> > > ||   || kvmclock.
> > > diff --git a/arch/x86/include/asm/qspinlock.h 
> > > b/arch/x86/include/asm/qspinlock.h
> > > index 5e16b5d..de42694 100644
> > > --- a/arch/x86/include/asm/qspinlock.h
> > > +++ b/arch/x86/include/asm/qspinlock.h
> > > @@ -3,6 +3,8 @@
> > >  #define _ASM_X86_QSPINLOCK_H
> > >  
> > >  #include 
> > > +#include 
> > > +
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -58,6 +60,8 @@ static inline bool virt_spin_lock(struct qspinlock 
> > > *lock)
> > >   if (!static_branch_likely(&virt_spin_lock_key))
> > >   return false;
> > >  
> > > + if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
> > > + return false;
> > 
> > Hm, every spinlock slowpath calls cpuid, which causes a VM exit, so I
> > wouldn't expect it to be faster than the existing implementations.
> > (Using the static key would be better.)
> > 
> > How does this patch perform compared to user-forced qspinlock and hybrid
> > pvqspinlock?
> 
> This patch should have same effect as user-forced qspinlock.

This is what I'm doubting, because the patch is adding about two
thousand cycles to every spinlock-taken path.
Doesn't this patch yield better results?

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 3df743b60c80..d9225e48c11a 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -676,6 +676,12 @@ void __init kvm_spinlock_init(void)
 {
if (!kvm_para_available())
return;
+
+   if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED)) {
+   static_branch_disable(&virt_spin_lock_key);
+   return;
+   }
+

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Pankaj Gupta 


> 2017-11-07 4:26 GMT+08:00 Eduardo Valentin :
> > Currently, the existing qspinlock implementation will fallback to
> > test-and-set if the hypervisor has not set the PV_UNHALT flag.
> >
> > This patch gives the opportunity to guest kernels to select
> > between test-and-set and the regular queueu fair lock implementation
> > based on the PV_DEDICATED KVM feature flag. When the PV_DEDICATED
> > flag is not set, the code will still fall back to test-and-set,
> > but when the PV_DEDICATED flag is set, the code will use
> > the regular queue spinlock implementation.
> >
> > With this patch, when in autoselect mode, the guest will
> > use the default spinlock implementation based on host feature
> > flags as follows:
> >
> > PV_DEDICATED = 1, PV_UNHALT = anything: default is qspinlock
> > PV_DEDICATED = 0, PV_UNHALT = 1: default is pvqspinlock
> > PV_DEDICATED = 0, PV_UNHALT = 0: default is tas
> >
> > Cc: Paolo Bonzini 
> > Cc: "Radim Krčmář" 
> > Cc: Jonathan Corbet 
> > Cc: Thomas Gleixner 
> > Cc: Ingo Molnar 
> > Cc: "H. Peter Anvin" 
> > Cc: x...@kernel.org
> > Cc: Peter Zijlstra 
> > Cc: Waiman Long 
> > Cc: k...@vger.kernel.org
> > Cc: linux-doc@vger.kernel.org
> > Cc: linux-ker...@vger.kernel.org
> > Cc: Jan H. Schoenherr 
> > Cc: Anthony Liguori 
> > Suggested-by: Matt Wilson 
> > Signed-off-by: Eduardo Valentin 
> > ---
> > V3:
> >  - When PV_DEDICATED is set (1), qspinlock is selected,
> >regardless of the value of PV_UNHAULT. Suggested by Paolo Bonzini.
> >  - Refreshed on top of tip/master.
> > V2:
> >  - rebase on top of tip/master
> >
> >  Documentation/virtual/kvm/cpuid.txt  | 6 ++
> >  arch/x86/include/asm/qspinlock.h | 4 
> >  arch/x86/include/uapi/asm/kvm_para.h | 1 +
> >  arch/x86/kernel/kvm.c| 2 ++
> >  4 files changed, 13 insertions(+)
> >
> > diff --git a/Documentation/virtual/kvm/cpuid.txt
> > b/Documentation/virtual/kvm/cpuid.txt
> > index 3c65feb..117066a 100644
> > --- a/Documentation/virtual/kvm/cpuid.txt
> > +++ b/Documentation/virtual/kvm/cpuid.txt
> > @@ -54,6 +54,12 @@ KVM_FEATURE_PV_UNHALT  || 7 || guest
> > checks this feature bit
> > ||   || before enabling
> > ||   || paravirtualized
> > ||   || spinlock support.
> >  
> > --
> > +KVM_FEATURE_PV_DEDICATED   || 8 || guest checks this feature
> > bit
> > +   ||   || to determine if they run on
> > +   ||   || dedicated vCPUs, allowing
> > opti-
> > +   ||   || mizations such as usage of
> > +   ||   || qspinlocks.
> > +--
> >  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||24 || host will warn if no
> >  guest-side
> > ||   || per-cpu warps are expected
> > ||   || in
> > ||   || kvmclock.
> > diff --git a/arch/x86/include/asm/qspinlock.h
> > b/arch/x86/include/asm/qspinlock.h
> > index 5e16b5d..de42694 100644
> > --- a/arch/x86/include/asm/qspinlock.h
> > +++ b/arch/x86/include/asm/qspinlock.h
> > @@ -3,6 +3,8 @@
> >  #define _ASM_X86_QSPINLOCK_H
> >
> >  #include 
> > +#include 
> > +
> >  #include 
> >  #include 
> >  #include 
> > @@ -58,6 +60,8 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
> > if (!static_branch_likely(&virt_spin_lock_key))
> > return false;
> >
> > +   if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
> > +   return false;
> > /*
> >  * On hypervisors without PARAVIRT_SPINLOCKS support we fall
> >  * back to a Test-and-Set spinlock, because fair locks have
> > diff --git a/arch/x86/include/uapi/asm/kvm_para.h
> > b/arch/x86/include/uapi/asm/kvm_para.h
> > index 554aa8f..85a9875 100644
> > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > @@ -25,6 +25,7 @@
> >  #define KVM_FEATURE_STEAL_TIME 5
> >  #define KVM_FEATURE_PV_EOI 6
> >  #define KVM_FEATURE_PV_UNHALT  7
> > +#define KVM_FEATURE_PV_DEDICATED   8
> >
> >  /* The last 8 bits are used to indicate how to interpret the flags field
> >   * in pvclock structure. If no bits are set, all flags are ignored.
> > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> > index 8bb9594..dacd7cf 100644
> > --- a/arch/x86/kernel/kvm.c
> > +++ b/arch/x86/kernel/kvm.c
> > @@ -642,6 +642,8 @@ void __init kvm_spinlock_init(void)
> >  {
> > if (!kvm_para_available())
> > return;
> > +   if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
> > +   return;
> > /* Does host kernel

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Radim Krcmar 
2017-11-09 20:43+0800, Wanpeng Li:
> 2017-11-07 4:26 GMT+08:00 Eduardo Valentin :
> > Currently, the existing qspinlock implementation will fallback to
> > test-and-set if the hypervisor has not set the PV_UNHALT flag.
> >
> > This patch gives the opportunity to guest kernels to select
> > between test-and-set and the regular queueu fair lock implementation
> > based on the PV_DEDICATED KVM feature flag. When the PV_DEDICATED
> > flag is not set, the code will still fall back to test-and-set,
> > but when the PV_DEDICATED flag is set, the code will use
> > the regular queue spinlock implementation.
> >
> > With this patch, when in autoselect mode, the guest will
> > use the default spinlock implementation based on host feature
> > flags as follows:
> >
> > PV_DEDICATED = 1, PV_UNHALT = anything: default is qspinlock
> > PV_DEDICATED = 0, PV_UNHALT = 1: default is pvqspinlock
> > PV_DEDICATED = 0, PV_UNHALT = 0: default is tas
> >
> > Cc: Paolo Bonzini 
> > Cc: "Radim Krčmář" 
> > Cc: Jonathan Corbet 
> > Cc: Thomas Gleixner 
> > Cc: Ingo Molnar 
> > Cc: "H. Peter Anvin" 
> > Cc: x...@kernel.org
> > Cc: Peter Zijlstra 
> > Cc: Waiman Long 
> > Cc: k...@vger.kernel.org
> > Cc: linux-doc@vger.kernel.org
> > Cc: linux-ker...@vger.kernel.org
> > Cc: Jan H. Schoenherr 
> > Cc: Anthony Liguori 
> > Suggested-by: Matt Wilson 
> > Signed-off-by: Eduardo Valentin 
> > ---
> 
> You should also add a cpuid flag in kvm part.

It is better without that.  The flag has no dependency on KVM (kernel
hypervisor) code.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Radim Krcmar 
2017-11-09 10:53-0500, Pankaj Gupta:
> 
> 
> > 2017-11-07 4:26 GMT+08:00 Eduardo Valentin :
> > > Currently, the existing qspinlock implementation will fallback to
> > > test-and-set if the hypervisor has not set the PV_UNHALT flag.
> > >
> > > This patch gives the opportunity to guest kernels to select
> > > between test-and-set and the regular queueu fair lock implementation
> > > based on the PV_DEDICATED KVM feature flag. When the PV_DEDICATED
> > > flag is not set, the code will still fall back to test-and-set,
> > > but when the PV_DEDICATED flag is set, the code will use
> > > the regular queue spinlock implementation.
> > >
> > > With this patch, when in autoselect mode, the guest will
> > > use the default spinlock implementation based on host feature
> > > flags as follows:
> > >
> > > PV_DEDICATED = 1, PV_UNHALT = anything: default is qspinlock
> > > PV_DEDICATED = 0, PV_UNHALT = 1: default is pvqspinlock
> > > PV_DEDICATED = 0, PV_UNHALT = 0: default is tas
> > >
> > > Cc: Paolo Bonzini 
> > > Cc: "Radim Krčmář" 
> > > Cc: Jonathan Corbet 
> > > Cc: Thomas Gleixner 
> > > Cc: Ingo Molnar 
> > > Cc: "H. Peter Anvin" 
> > > Cc: x...@kernel.org
> > > Cc: Peter Zijlstra 
> > > Cc: Waiman Long 
> > > Cc: k...@vger.kernel.org
> > > Cc: linux-doc@vger.kernel.org
> > > Cc: linux-ker...@vger.kernel.org
> > > Cc: Jan H. Schoenherr 
> > > Cc: Anthony Liguori 
> > > Suggested-by: Matt Wilson 
> > > Signed-off-by: Eduardo Valentin 
> > > ---
> > > V3:
> > >  - When PV_DEDICATED is set (1), qspinlock is selected,
> > >regardless of the value of PV_UNHAULT. Suggested by Paolo Bonzini.
> > >  - Refreshed on top of tip/master.
> > > V2:
> > >  - rebase on top of tip/master
> > >
> > >  Documentation/virtual/kvm/cpuid.txt  | 6 ++
> > >  arch/x86/include/asm/qspinlock.h | 4 
> > >  arch/x86/include/uapi/asm/kvm_para.h | 1 +
> > >  arch/x86/kernel/kvm.c| 2 ++
> > >  4 files changed, 13 insertions(+)
> > >
> > > diff --git a/Documentation/virtual/kvm/cpuid.txt
> > > b/Documentation/virtual/kvm/cpuid.txt
> > > index 3c65feb..117066a 100644
> > > --- a/Documentation/virtual/kvm/cpuid.txt
> > > +++ b/Documentation/virtual/kvm/cpuid.txt
> > > @@ -54,6 +54,12 @@ KVM_FEATURE_PV_UNHALT  || 7 || guest
> > > checks this feature bit
> > > ||   || before enabling
> > > ||   || paravirtualized
> > > ||   || spinlock support.
> > >  
> > > --
> > > +KVM_FEATURE_PV_DEDICATED   || 8 || guest checks this feature
> > > bit
> > > +   ||   || to determine if they run 
> > > on
> > > +   ||   || dedicated vCPUs, allowing
> > > opti-
> > > +   ||   || mizations such as usage of
> > > +   ||   || qspinlocks.
> > > +--
> > >  KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||24 || host will warn if no
> > >  guest-side
> > > ||   || per-cpu warps are expected
> > > ||   || in
> > > ||   || kvmclock.
> > > diff --git a/arch/x86/include/asm/qspinlock.h
> > > b/arch/x86/include/asm/qspinlock.h
> > > index 5e16b5d..de42694 100644
> > > --- a/arch/x86/include/asm/qspinlock.h
> > > +++ b/arch/x86/include/asm/qspinlock.h
> > > @@ -3,6 +3,8 @@
> > >  #define _ASM_X86_QSPINLOCK_H
> > >
> > >  #include 
> > > +#include 
> > > +
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -58,6 +60,8 @@ static inline bool virt_spin_lock(struct qspinlock 
> > > *lock)
> > > if (!static_branch_likely(&virt_spin_lock_key))
> > > return false;
> > >
> > > +   if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED))
> > > +   return false;
> > > /*
> > >  * On hypervisors without PARAVIRT_SPINLOCKS support we fall
> > >  * back to a Test-and-Set spinlock, because fair locks have
> > > diff --git a/arch/x86/include/uapi/asm/kvm_para.h
> > > b/arch/x86/include/uapi/asm/kvm_para.h
> > > index 554aa8f..85a9875 100644
> > > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > > @@ -25,6 +25,7 @@
> > >  #define KVM_FEATURE_STEAL_TIME 5
> > >  #define KVM_FEATURE_PV_EOI 6
> > >  #define KVM_FEATURE_PV_UNHALT  7
> > > +#define KVM_FEATURE_PV_DEDICATED   8
> > >
> > >  /* The last 8 bits are used to indicate how to interpret the flags field
> > >   * in pvclock structure. If no bits are set, all flags are ignored.
> > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> > > index 8bb9594..dacd7cf 100644
> > > --- a/arch/x86/kernel/kvm.c
> > > +++

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Peter Zijlstra 
On Thu, Nov 09, 2017 at 05:05:36PM +0100, Radim Krcmar wrote:
> 2017-11-09 10:53-0500, Pankaj Gupta:
> > 2] PV TLB should also behave as per option PV_DEDICATED for better 
> > performance.
> 
> Right,

Shouldn't KVM do flush_tlb_other() in any case? Not sure how
PV_DEDICATED can help with that.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Pankaj Gupta 

> Subject: Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when 
> PV_DEDICATED is set
> 
> On Thu, Nov 09, 2017 at 05:05:36PM +0100, Radim Krcmar wrote:
> > 2017-11-09 10:53-0500, Pankaj Gupta:
> > > 2] PV TLB should also behave as per option PV_DEDICATED for better
> > > performance.
> > 
> > Right,
> 
> Shouldn't KVM do flush_tlb_other() in any case? Not sure how
> PV_DEDICATED can help with that.

Yes.

If I understand it correctly, It will just avoid traversing all the 
cpus another time just to check/set 'KVM_VCPU_PREEMPTED'/FLUSH value and 
clear the cpu mask.

Thanks,
Pankaj
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Radim Krcmar 
2017-11-09 17:17+0100, Peter Zijlstra:
> On Thu, Nov 09, 2017 at 05:05:36PM +0100, Radim Krcmar wrote:
> > 2017-11-09 10:53-0500, Pankaj Gupta:
> > > 2] PV TLB should also behave as per option PV_DEDICATED for better 
> > > performance.
> > 
> > Right,
> 
> Shouldn't KVM do flush_tlb_other() in any case? Not sure how
> PV_DEDICATED can help with that.

It will, the suggestion was based on recent extension of the
flush_tlb_others implementaion, https://lkml.org/lkml/2017/11/8/1146.

PV_TLB_FLUSH allows a guest to set a flush bit instead of sending flush
IPI if the target VCPU is not running.  This would be a waste of time
with PV_DEDICATED as all VCPUs are expected to always running.

With PV_DEDICATED, the guest should keep using native_flush_tlb_others.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Peter Zijlstra 
On Thu, Nov 09, 2017 at 05:45:23PM +0100, Radim Krcmar wrote:
> 2017-11-09 17:17+0100, Peter Zijlstra:
> > On Thu, Nov 09, 2017 at 05:05:36PM +0100, Radim Krcmar wrote:
> > > 2017-11-09 10:53-0500, Pankaj Gupta:
> > > > 2] PV TLB should also behave as per option PV_DEDICATED for better 
> > > > performance.
> > > 
> > > Right,
> > 
> > Shouldn't KVM do flush_tlb_other() in any case? Not sure how
> > PV_DEDICATED can help with that.
> 
> It will, the suggestion was based on recent extension of the
> flush_tlb_others implementaion, https://lkml.org/lkml/2017/11/8/1146.
> 
> PV_TLB_FLUSH allows a guest to set a flush bit instead of sending flush
> IPI if the target VCPU is not running.  This would be a waste of time
> with PV_DEDICATED as all VCPUs are expected to always running.
> 
> With PV_DEDICATED, the guest should keep using native_flush_tlb_others.

Is saving that for_each_cpu() really worth the effort compared to the
cost of actually doing the IPIs and CR3 write?

Also, you should not put cpumask_t on stack, that's 'broken'.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Peter Zijlstra 
On Thu, Nov 09, 2017 at 06:12:41PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 09, 2017 at 05:45:23PM +0100, Radim Krcmar wrote:
> > 2017-11-09 17:17+0100, Peter Zijlstra:
> > > On Thu, Nov 09, 2017 at 05:05:36PM +0100, Radim Krcmar wrote:
> > > > 2017-11-09 10:53-0500, Pankaj Gupta:
> > > > > 2] PV TLB should also behave as per option PV_DEDICATED for better 
> > > > > performance.
> > > > 
> > > > Right,
> > > 
> > > Shouldn't KVM do flush_tlb_other() in any case? Not sure how
> > > PV_DEDICATED can help with that.
> > 
> > It will, the suggestion was based on recent extension of the
> > flush_tlb_others implementaion, https://lkml.org/lkml/2017/11/8/1146.
> > 
> > PV_TLB_FLUSH allows a guest to set a flush bit instead of sending flush
> > IPI if the target VCPU is not running.  This would be a waste of time
> > with PV_DEDICATED as all VCPUs are expected to always running.
> > 
> > With PV_DEDICATED, the guest should keep using native_flush_tlb_others.
> 
> Is saving that for_each_cpu() really worth the effort compared to the
> cost of actually doing the IPIs and CR3 write?
> 
> Also, you should not put cpumask_t on stack, that's 'broken'.

Also, you'll want to use __cpumask_clear_cpu() there.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Peter Zijlstra 
On Thu, Nov 09, 2017 at 06:15:11PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 09, 2017 at 06:12:41PM +0100, Peter Zijlstra wrote:
> > On Thu, Nov 09, 2017 at 05:45:23PM +0100, Radim Krcmar wrote:
> > > 2017-11-09 17:17+0100, Peter Zijlstra:
> > > > On Thu, Nov 09, 2017 at 05:05:36PM +0100, Radim Krcmar wrote:
> > > > > 2017-11-09 10:53-0500, Pankaj Gupta:
> > > > > > 2] PV TLB should also behave as per option PV_DEDICATED for better 
> > > > > > performance.
> > > > > 
> > > > > Right,
> > > > 
> > > > Shouldn't KVM do flush_tlb_other() in any case? Not sure how
> > > > PV_DEDICATED can help with that.
> > > 
> > > It will, the suggestion was based on recent extension of the
> > > flush_tlb_others implementaion, https://lkml.org/lkml/2017/11/8/1146.
> > > 
> > > PV_TLB_FLUSH allows a guest to set a flush bit instead of sending flush
> > > IPI if the target VCPU is not running.  This would be a waste of time
> > > with PV_DEDICATED as all VCPUs are expected to always running.
> > > 
> > > With PV_DEDICATED, the guest should keep using native_flush_tlb_others.
> > 
> > Is saving that for_each_cpu() really worth the effort compared to the
> > cost of actually doing the IPIs and CR3 write?
> > 
> > Also, you should not put cpumask_t on stack, that's 'broken'.
> 
> Also, you'll want to use __cpumask_clear_cpu() there.

Also^2, that patch split is crazy, after patch 2/3 your machine is
broken due to lost TLB flushes. You have to first add the SHOULD_FLUSH
handling and then clear CPUs from the native_flush_tlb_other() mask.


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Radim Krcmar 
2017-11-09 18:28+0100, Peter Zijlstra:
> On Thu, Nov 09, 2017 at 06:15:11PM +0100, Peter Zijlstra wrote:
> > On Thu, Nov 09, 2017 at 06:12:41PM +0100, Peter Zijlstra wrote:
> > > On Thu, Nov 09, 2017 at 05:45:23PM +0100, Radim Krcmar wrote:
> > > > 2017-11-09 17:17+0100, Peter Zijlstra:
> > > > > On Thu, Nov 09, 2017 at 05:05:36PM +0100, Radim Krcmar wrote:
> > > > > > 2017-11-09 10:53-0500, Pankaj Gupta:
> > > > > > > 2] PV TLB should also behave as per option PV_DEDICATED for 
> > > > > > > better performance.
> > > > > > 
> > > > > > Right,
> > > > > 
> > > > > Shouldn't KVM do flush_tlb_other() in any case? Not sure how
> > > > > PV_DEDICATED can help with that.
> > > > 
> > > > It will, the suggestion was based on recent extension of the
> > > > flush_tlb_others implementaion, https://lkml.org/lkml/2017/11/8/1146.
> > > > 
> > > > PV_TLB_FLUSH allows a guest to set a flush bit instead of sending flush
> > > > IPI if the target VCPU is not running.  This would be a waste of time
> > > > with PV_DEDICATED as all VCPUs are expected to always running.
> > > > 
> > > > With PV_DEDICATED, the guest should keep using native_flush_tlb_others.
> > > 
> > > Is saving that for_each_cpu() really worth the effort compared to the
> > > cost of actually doing the IPIs and CR3 write?
> > > 
> > > Also, you should not put cpumask_t on stack, that's 'broken'.
> > 
> > Also, you'll want to use __cpumask_clear_cpu() there.
> 
> Also^2, that patch split is crazy, after patch 2/3 your machine is
> broken due to lost TLB flushes. You have to first add the SHOULD_FLUSH
> handling and then clear CPUs from the native_flush_tlb_other() mask.

That should be fixed in v2 -- [2/3] must not enable this feature if the
host has not exposed it and [3/3] has to expose it.  (The ordering of
those two doesn't matter as they are separate kernel.)
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Radim Krcmar 
2017-11-09 18:12+0100, Peter Zijlstra:
> On Thu, Nov 09, 2017 at 05:45:23PM +0100, Radim Krcmar wrote:
> > 2017-11-09 17:17+0100, Peter Zijlstra:
> > > On Thu, Nov 09, 2017 at 05:05:36PM +0100, Radim Krcmar wrote:
> > > > 2017-11-09 10:53-0500, Pankaj Gupta:
> > > > > 2] PV TLB should also behave as per option PV_DEDICATED for better 
> > > > > performance.
> > > > 
> > > > Right,
> > > 
> > > Shouldn't KVM do flush_tlb_other() in any case? Not sure how
> > > PV_DEDICATED can help with that.
> > 
> > It will, the suggestion was based on recent extension of the
> > flush_tlb_others implementaion, https://lkml.org/lkml/2017/11/8/1146.
> > 
> > PV_TLB_FLUSH allows a guest to set a flush bit instead of sending flush
> > IPI if the target VCPU is not running.  This would be a waste of time
> > with PV_DEDICATED as all VCPUs are expected to always running.
> > 
> > With PV_DEDICATED, the guest should keep using native_flush_tlb_others.
> 
> Is saving that for_each_cpu() really worth the effort compared to the
> cost of actually doing the IPIs and CR3 write?

It is one line for a few percent (hopefully better for AMD with AVIC).

Still, keeping the decision completely on userspace would be cleaner.

> Also, you should not put cpumask_t on stack, that's 'broken'.

Good catch, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Wanpeng Li 
2017-11-10 1:15 GMT+08:00 Peter Zijlstra :
> On Thu, Nov 09, 2017 at 06:12:41PM +0100, Peter Zijlstra wrote:
>> On Thu, Nov 09, 2017 at 05:45:23PM +0100, Radim Krcmar wrote:
>> > 2017-11-09 17:17+0100, Peter Zijlstra:
>> > > On Thu, Nov 09, 2017 at 05:05:36PM +0100, Radim Krcmar wrote:
>> > > > 2017-11-09 10:53-0500, Pankaj Gupta:
>> > > > > 2] PV TLB should also behave as per option PV_DEDICATED for better 
>> > > > > performance.
>> > > >
>> > > > Right,
>> > >
>> > > Shouldn't KVM do flush_tlb_other() in any case? Not sure how
>> > > PV_DEDICATED can help with that.
>> >
>> > It will, the suggestion was based on recent extension of the
>> > flush_tlb_others implementaion, https://lkml.org/lkml/2017/11/8/1146.
>> >
>> > PV_TLB_FLUSH allows a guest to set a flush bit instead of sending flush
>> > IPI if the target VCPU is not running.  This would be a waste of time
>> > with PV_DEDICATED as all VCPUs are expected to always running.
>> >
>> > With PV_DEDICATED, the guest should keep using native_flush_tlb_others.
>>
>> Is saving that for_each_cpu() really worth the effort compared to the
>> cost of actually doing the IPIs and CR3 write?
>>
>> Also, you should not put cpumask_t on stack, that's 'broken'.

Thanks pointing out this. I found a useful comments in arch/x86/kernel/irq.c:

/* These two declarations are only used in check_irq_vectors_for_cpu_disable()
 * below, which is protected by stop_machine().  Putting them on the stack
 * results in a stack frame overflow.  Dynamically allocating could result in a
 * failure so declare these two cpumasks as global.
 */
static struct cpumask affinity_new, online_new;

>
> Also, you'll want to use __cpumask_clear_cpu() there.

Will do.

Regards,
Wanpeng Li
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Wanpeng Li 
2017-11-10 0:00 GMT+08:00 Radim Krcmar :
> 2017-11-09 20:43+0800, Wanpeng Li:
>> 2017-11-07 4:26 GMT+08:00 Eduardo Valentin :
>> > Currently, the existing qspinlock implementation will fallback to
>> > test-and-set if the hypervisor has not set the PV_UNHALT flag.
>> >
>> > This patch gives the opportunity to guest kernels to select
>> > between test-and-set and the regular queueu fair lock implementation
>> > based on the PV_DEDICATED KVM feature flag. When the PV_DEDICATED
>> > flag is not set, the code will still fall back to test-and-set,
>> > but when the PV_DEDICATED flag is set, the code will use
>> > the regular queue spinlock implementation.
>> >
>> > With this patch, when in autoselect mode, the guest will
>> > use the default spinlock implementation based on host feature
>> > flags as follows:
>> >
>> > PV_DEDICATED = 1, PV_UNHALT = anything: default is qspinlock
>> > PV_DEDICATED = 0, PV_UNHALT = 1: default is pvqspinlock
>> > PV_DEDICATED = 0, PV_UNHALT = 0: default is tas
>> >
>> > Cc: Paolo Bonzini 
>> > Cc: "Radim Krčmář" 
>> > Cc: Jonathan Corbet 
>> > Cc: Thomas Gleixner 
>> > Cc: Ingo Molnar 
>> > Cc: "H. Peter Anvin" 
>> > Cc: x...@kernel.org
>> > Cc: Peter Zijlstra 
>> > Cc: Waiman Long 
>> > Cc: k...@vger.kernel.org
>> > Cc: linux-doc@vger.kernel.org
>> > Cc: linux-ker...@vger.kernel.org
>> > Cc: Jan H. Schoenherr 
>> > Cc: Anthony Liguori 
>> > Suggested-by: Matt Wilson 
>> > Signed-off-by: Eduardo Valentin 
>> > ---
>>
>> You should also add a cpuid flag in kvm part.
>
> It is better without that.  The flag has no dependency on KVM (kernel
> hypervisor) code.

Do you mean -cpu host, +，I think it will result in "warning: host
doesn't support requested feature: CPUID.4001H:eax.XX"

Regards,
Wanpeng Li
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-09 Thread Peter Zijlstra 
On Fri, Nov 10, 2017 at 10:07:56AM +0800, Wanpeng Li wrote:

> >> Also, you should not put cpumask_t on stack, that's 'broken'.
> 
> Thanks pointing out this. I found a useful comments in arch/x86/kernel/irq.c:
> 
> /* These two declarations are only used in check_irq_vectors_for_cpu_disable()
>  * below, which is protected by stop_machine().  Putting them on the stack
>  * results in a stack frame overflow.  Dynamically allocating could result in 
> a
>  * failure so declare these two cpumasks as global.
>  */
> static struct cpumask affinity_new, online_new;

That code no longer exists.. Also not entirely sure how it would be
helpful.

What you probably want to do is have a per-cpu cpumask, since
flush_tlb_others() is called with preemption disabled. But you probably
don't want an unconditionally allocated one, since most kernels will not
in fact be PV.

So you'll want something like:

static DEFINE_PER_CPU(cpumask_var_t, __pv_tlb_mask);

And then you need something like:

for_each_possible_cpu(cpu) {
zalloc_cpumask_var_node(per_cpu_ptr(&__pb_tlb_mask, cpu),
GFP_KERNEL, cpu_to_node(cpu));
}

before you set the pv-op or so.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-10 Thread Wanpeng Li 
2017-11-10 15:59 GMT+08:00 Peter Zijlstra :
> On Fri, Nov 10, 2017 at 10:07:56AM +0800, Wanpeng Li wrote:
>
>> >> Also, you should not put cpumask_t on stack, that's 'broken'.
>>
>> Thanks pointing out this. I found a useful comments in arch/x86/kernel/irq.c:
>>
>> /* These two declarations are only used in 
>> check_irq_vectors_for_cpu_disable()
>>  * below, which is protected by stop_machine().  Putting them on the stack
>>  * results in a stack frame overflow.  Dynamically allocating could result 
>> in a
>>  * failure so declare these two cpumasks as global.
>>  */
>> static struct cpumask affinity_new, online_new;
>
> That code no longer exists.. Also not entirely sure how it would be
> helpful.
>
> What you probably want to do is have a per-cpu cpumask, since
> flush_tlb_others() is called with preemption disabled. But you probably
> don't want an unconditionally allocated one, since most kernels will not
> in fact be PV.
>
> So you'll want something like:
>
> static DEFINE_PER_CPU(cpumask_var_t, __pv_tlb_mask);
>
> And then you need something like:
>
> for_each_possible_cpu(cpu) {
> zalloc_cpumask_var_node(per_cpu_ptr(&__pb_tlb_mask, cpu),
> GFP_KERNEL, cpu_to_node(cpu));
> }
>
> before you set the pv-op or so.

Thanks Peterz, :) I will have a try.

Regards,
Wanpeng Li
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-10 Thread Paolo Bonzini 
On 10/11/2017 07:07, Wanpeng Li wrote:
>>> You should also add a cpuid flag in kvm part.
>> It is better without that.  The flag has no dependency on KVM (kernel
>> hypervisor) code.
> Do you mean -cpu host, +，I think it will result in "warning: host
> doesn't support requested feature: CPUID.4001H:eax.XX"

There are some exceptions where QEMU overrides the values of
KVM_GET_SUPPORTED_CPUID.

I think it is better to add the flag to KVM *and* to QEMU's override in
kvm_arch_get_supported_cpuid (target/i386/kvm.c).

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-15 Thread Eduardo Valentin 
Hey Radim,

On Thu, Nov 09, 2017 at 03:17:33PM +0100, Radim Krčmář wrote:



> 
> This is what I'm doubting, because the patch is adding about two
> thousand cycles to every spinlock-taken path.
> Doesn't this patch yield better results?
> 
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 3df743b60c80..d9225e48c11a 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -676,6 +676,12 @@ void __init kvm_spinlock_init(void)
>  {
>   if (!kvm_para_available())
>   return;
> +
> + if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED)) {
> + static_branch_disable(&virt_spin_lock_key);
> + return;
> + }
> +

Yes, the above suggestion is a much better approach. The code has probably 
changed from the time I wrote the first version. I will refresh with the above 
suggestion.


>   /* Does host kernel support KVM_FEATURE_PV_UNHALT? */
>   if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
>   return;
> 
> >  However, the 
> > key aspect
> > here is this patch gives a way for the host to instruct the guest to use 
> > qspinlock.
> > Even with Longman's patch which allows guest to select the spinlock 
> > implementation,
> > there should still be the auto-select mode. In such mode, PV_DEDICATED 
> > should
> > allow the host to get the guest to use qspinlock, without, the guest will 
> > fallback
> > to tas when PV_UNHALT == 0.
> 
> I agree that a flag can be useful for certains setups.

Cool!

> 

-- 
All the best,
Eduardo Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] locking/qspinlock/x86: Avoid test-and-set when PV_DEDICATED is set

2017-11-26 Thread Wanpeng Li 
Hi Eduardo,
2017-11-16 12:54 GMT+08:00 Eduardo Valentin :
> Hey Radim,
>
> On Thu, Nov 09, 2017 at 03:17:33PM +0100, Radim Krčmář wrote:
>
> 
>
>>
>> This is what I'm doubting, because the patch is adding about two
>> thousand cycles to every spinlock-taken path.
>> Doesn't this patch yield better results?
>>
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index 3df743b60c80..d9225e48c11a 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -676,6 +676,12 @@ void __init kvm_spinlock_init(void)
>>  {
>>   if (!kvm_para_available())
>>   return;
>> +
>> + if (kvm_para_has_feature(KVM_FEATURE_PV_DEDICATED)) {
>> + static_branch_disable(&virt_spin_lock_key);
>> + return;
>> + }
>> +
>
> Yes, the above suggestion is a much better approach. The code has probably 
> changed from the time I wrote the first version. I will refresh with the 
> above suggestion.

Do you mind to send a new version since the merge window is closed?

Regards,
Wanpeng Li

>
>
>>   /* Does host kernel support KVM_FEATURE_PV_UNHALT? */
>>   if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
>>   return;
>>
>> >  However, the 
>> > key aspect
>> > here is this patch gives a way for the host to instruct the guest to use 
>> > qspinlock.
>> > Even with Longman's patch which allows guest to select the spinlock 
>> > implementation,
>> > there should still be the auto-select mode. In such mode, PV_DEDICATED 
>> > should
>> > allow the host to get the guest to use qspinlock, without, the guest will 
>> > fallback
>> > to tas when PV_UNHALT == 0.
>>
>> I agree that a flag can be useful for certains setups.
>
> Cool!
>
>>
>
> --
> All the best,
> Eduardo Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html