* [PATCH 0/4] make virt_spin_lock() a pvops function
@ 2017-09-05 13:24 ` Juergen Gross
0 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, chrisw, mingo, tglx, hpa, longman,
akataria, boris.ostrovsky
With virt_spin_lock() being a pvops function the bare metal case can be
optimized by patching the call away completely. In case a kernel running
as a guest it can decide whether to use paravitualized spinlocks, the
current fallback to the unfair test-and-set scheme, or to mimic the
bare metal behavior.
Juergen Gross (4):
paravirt: add generic _paravirt_false() function
paravirt: switch vcpu_is_preempted to use _paravirt_false() on bare
metal
paravirt: add virt_spin_lock pvops function
paravirt,xen: correct xen_nopvspin case
arch/x86/include/asm/paravirt.h | 5 ++++
arch/x86/include/asm/paravirt_types.h | 3 +++
arch/x86/include/asm/qspinlock.h | 48 ++++++++++++++++++++++++-----------
arch/x86/kernel/paravirt-spinlocks.c | 22 ++++++++--------
arch/x86/kernel/paravirt.c | 7 +++++
arch/x86/kernel/paravirt_patch_32.c | 18 ++++++-------
arch/x86/kernel/paravirt_patch_64.c | 17 +++++--------
arch/x86/kernel/smpboot.c | 2 ++
arch/x86/xen/spinlock.c | 2 ++
9 files changed, 79 insertions(+), 45 deletions(-)
--
2.12.3
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 1/4] paravirt: add generic _paravirt_false() function
2017-09-05 13:24 ` Juergen Gross
(?)
@ 2017-09-05 13:24 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo, peterz, longman, Juergen Gross
Add a _paravirt_false() default function returning always false which
can be used for cases where a boolean pvops replacement should just
say "no".
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/include/asm/paravirt_types.h | 2 ++
arch/x86/kernel/paravirt.c | 7 +++++++
arch/x86/kernel/paravirt_patch_32.c | 8 ++++++++
arch/x86/kernel/paravirt_patch_64.c | 7 +++++++
4 files changed, 24 insertions(+)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 6b64fc6367f2..19efefc0e27e 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -377,6 +377,7 @@ extern struct pv_lock_ops pv_lock_ops;
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len);
unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len);
+unsigned paravirt_patch_false(void *insnbuf, unsigned len);
unsigned paravirt_patch_call(void *insnbuf,
const void *target, u16 tgt_clobbers,
unsigned long addr, u16 site_clobbers,
@@ -682,6 +683,7 @@ void paravirt_flush_lazy_mmu(void);
void _paravirt_nop(void);
u32 _paravirt_ident_32(u32);
u64 _paravirt_ident_64(u64);
+bool _paravirt_false(void);
#define paravirt_nop ((void *)_paravirt_nop)
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index a14df9eecfed..94105773c00c 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -66,6 +66,11 @@ u64 notrace _paravirt_ident_64(u64 x)
return x;
}
+bool notrace _paravirt_false(void)
+{
+ return false;
+}
+
void __init default_banner(void)
{
printk(KERN_INFO "Booting paravirtualized kernel on %s\n",
@@ -149,6 +154,8 @@ unsigned paravirt_patch_default(u8 type, u16 clobbers, void *insnbuf,
ret = paravirt_patch_ident_32(insnbuf, len);
else if (opfunc == _paravirt_ident_64)
ret = paravirt_patch_ident_64(insnbuf, len);
+ else if (opfunc == _paravirt_false)
+ ret = paravirt_patch_false(insnbuf, len);
else if (type == PARAVIRT_PATCH(pv_cpu_ops.iret) ||
type == PARAVIRT_PATCH(pv_cpu_ops.usergs_sysret64))
diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c
index 553acbbb4d32..287c7b9735de 100644
--- a/arch/x86/kernel/paravirt_patch_32.c
+++ b/arch/x86/kernel/paravirt_patch_32.c
@@ -9,6 +9,8 @@ DEF_NATIVE(pv_mmu_ops, read_cr2, "mov %cr2, %eax");
DEF_NATIVE(pv_mmu_ops, write_cr3, "mov %eax, %cr3");
DEF_NATIVE(pv_mmu_ops, read_cr3, "mov %cr3, %eax");
+DEF_NATIVE(, xor, "xor %eax, %eax");
+
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%eax)");
DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %eax, %eax");
@@ -26,6 +28,12 @@ unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len)
return 0;
}
+unsigned paravirt_patch_false(void *insnbuf, unsigned len)
+{
+ return paravirt_patch_insns(insnbuf, len,
+ start__xor, end__xor);
+}
+
extern bool pv_is_native_spin_unlock(void);
extern bool pv_is_native_vcpu_is_preempted(void);
diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index 11aaf1eaa0e4..8ab4379ceea9 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -17,6 +17,7 @@ DEF_NATIVE(pv_cpu_ops, swapgs, "swapgs");
DEF_NATIVE(, mov32, "mov %edi, %eax");
DEF_NATIVE(, mov64, "mov %rdi, %rax");
+DEF_NATIVE(, xor, "xor %rax, %rax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%rdi)");
@@ -35,6 +36,12 @@ unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len)
start__mov64, end__mov64);
}
+unsigned paravirt_patch_false(void *insnbuf, unsigned len)
+{
+ return paravirt_patch_insns(insnbuf, len,
+ start__xor, end__xor);
+}
+
extern bool pv_is_native_spin_unlock(void);
extern bool pv_is_native_vcpu_is_preempted(void);
--
2.12.3
^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 1/4] paravirt: add generic _paravirt_false() function
2017-09-05 13:24 ` Juergen Gross
(?)
(?)
@ 2017-09-05 13:24 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, chrisw, mingo, tglx, hpa, longman,
akataria, boris.ostrovsky
Add a _paravirt_false() default function returning always false which
can be used for cases where a boolean pvops replacement should just
say "no".
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/include/asm/paravirt_types.h | 2 ++
arch/x86/kernel/paravirt.c | 7 +++++++
arch/x86/kernel/paravirt_patch_32.c | 8 ++++++++
arch/x86/kernel/paravirt_patch_64.c | 7 +++++++
4 files changed, 24 insertions(+)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 6b64fc6367f2..19efefc0e27e 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -377,6 +377,7 @@ extern struct pv_lock_ops pv_lock_ops;
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len);
unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len);
+unsigned paravirt_patch_false(void *insnbuf, unsigned len);
unsigned paravirt_patch_call(void *insnbuf,
const void *target, u16 tgt_clobbers,
unsigned long addr, u16 site_clobbers,
@@ -682,6 +683,7 @@ void paravirt_flush_lazy_mmu(void);
void _paravirt_nop(void);
u32 _paravirt_ident_32(u32);
u64 _paravirt_ident_64(u64);
+bool _paravirt_false(void);
#define paravirt_nop ((void *)_paravirt_nop)
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index a14df9eecfed..94105773c00c 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -66,6 +66,11 @@ u64 notrace _paravirt_ident_64(u64 x)
return x;
}
+bool notrace _paravirt_false(void)
+{
+ return false;
+}
+
void __init default_banner(void)
{
printk(KERN_INFO "Booting paravirtualized kernel on %s\n",
@@ -149,6 +154,8 @@ unsigned paravirt_patch_default(u8 type, u16 clobbers, void *insnbuf,
ret = paravirt_patch_ident_32(insnbuf, len);
else if (opfunc == _paravirt_ident_64)
ret = paravirt_patch_ident_64(insnbuf, len);
+ else if (opfunc == _paravirt_false)
+ ret = paravirt_patch_false(insnbuf, len);
else if (type == PARAVIRT_PATCH(pv_cpu_ops.iret) ||
type == PARAVIRT_PATCH(pv_cpu_ops.usergs_sysret64))
diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c
index 553acbbb4d32..287c7b9735de 100644
--- a/arch/x86/kernel/paravirt_patch_32.c
+++ b/arch/x86/kernel/paravirt_patch_32.c
@@ -9,6 +9,8 @@ DEF_NATIVE(pv_mmu_ops, read_cr2, "mov %cr2, %eax");
DEF_NATIVE(pv_mmu_ops, write_cr3, "mov %eax, %cr3");
DEF_NATIVE(pv_mmu_ops, read_cr3, "mov %cr3, %eax");
+DEF_NATIVE(, xor, "xor %eax, %eax");
+
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%eax)");
DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %eax, %eax");
@@ -26,6 +28,12 @@ unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len)
return 0;
}
+unsigned paravirt_patch_false(void *insnbuf, unsigned len)
+{
+ return paravirt_patch_insns(insnbuf, len,
+ start__xor, end__xor);
+}
+
extern bool pv_is_native_spin_unlock(void);
extern bool pv_is_native_vcpu_is_preempted(void);
diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index 11aaf1eaa0e4..8ab4379ceea9 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -17,6 +17,7 @@ DEF_NATIVE(pv_cpu_ops, swapgs, "swapgs");
DEF_NATIVE(, mov32, "mov %edi, %eax");
DEF_NATIVE(, mov64, "mov %rdi, %rax");
+DEF_NATIVE(, xor, "xor %rax, %rax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%rdi)");
@@ -35,6 +36,12 @@ unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len)
start__mov64, end__mov64);
}
+unsigned paravirt_patch_false(void *insnbuf, unsigned len)
+{
+ return paravirt_patch_insns(insnbuf, len,
+ start__xor, end__xor);
+}
+
extern bool pv_is_native_spin_unlock(void);
extern bool pv_is_native_vcpu_is_preempted(void);
--
2.12.3
^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 1/4] paravirt: add generic _paravirt_false() function
2017-09-05 13:24 ` Juergen Gross
` (2 preceding siblings ...)
(?)
@ 2017-09-05 13:24 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, rusty, chrisw, mingo, tglx, hpa,
longman, akataria, boris.ostrovsky
Add a _paravirt_false() default function returning always false which
can be used for cases where a boolean pvops replacement should just
say "no".
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/include/asm/paravirt_types.h | 2 ++
arch/x86/kernel/paravirt.c | 7 +++++++
arch/x86/kernel/paravirt_patch_32.c | 8 ++++++++
arch/x86/kernel/paravirt_patch_64.c | 7 +++++++
4 files changed, 24 insertions(+)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 6b64fc6367f2..19efefc0e27e 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -377,6 +377,7 @@ extern struct pv_lock_ops pv_lock_ops;
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len);
unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len);
+unsigned paravirt_patch_false(void *insnbuf, unsigned len);
unsigned paravirt_patch_call(void *insnbuf,
const void *target, u16 tgt_clobbers,
unsigned long addr, u16 site_clobbers,
@@ -682,6 +683,7 @@ void paravirt_flush_lazy_mmu(void);
void _paravirt_nop(void);
u32 _paravirt_ident_32(u32);
u64 _paravirt_ident_64(u64);
+bool _paravirt_false(void);
#define paravirt_nop ((void *)_paravirt_nop)
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index a14df9eecfed..94105773c00c 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -66,6 +66,11 @@ u64 notrace _paravirt_ident_64(u64 x)
return x;
}
+bool notrace _paravirt_false(void)
+{
+ return false;
+}
+
void __init default_banner(void)
{
printk(KERN_INFO "Booting paravirtualized kernel on %s\n",
@@ -149,6 +154,8 @@ unsigned paravirt_patch_default(u8 type, u16 clobbers, void *insnbuf,
ret = paravirt_patch_ident_32(insnbuf, len);
else if (opfunc == _paravirt_ident_64)
ret = paravirt_patch_ident_64(insnbuf, len);
+ else if (opfunc == _paravirt_false)
+ ret = paravirt_patch_false(insnbuf, len);
else if (type == PARAVIRT_PATCH(pv_cpu_ops.iret) ||
type == PARAVIRT_PATCH(pv_cpu_ops.usergs_sysret64))
diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c
index 553acbbb4d32..287c7b9735de 100644
--- a/arch/x86/kernel/paravirt_patch_32.c
+++ b/arch/x86/kernel/paravirt_patch_32.c
@@ -9,6 +9,8 @@ DEF_NATIVE(pv_mmu_ops, read_cr2, "mov %cr2, %eax");
DEF_NATIVE(pv_mmu_ops, write_cr3, "mov %eax, %cr3");
DEF_NATIVE(pv_mmu_ops, read_cr3, "mov %cr3, %eax");
+DEF_NATIVE(, xor, "xor %eax, %eax");
+
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%eax)");
DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %eax, %eax");
@@ -26,6 +28,12 @@ unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len)
return 0;
}
+unsigned paravirt_patch_false(void *insnbuf, unsigned len)
+{
+ return paravirt_patch_insns(insnbuf, len,
+ start__xor, end__xor);
+}
+
extern bool pv_is_native_spin_unlock(void);
extern bool pv_is_native_vcpu_is_preempted(void);
diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index 11aaf1eaa0e4..8ab4379ceea9 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -17,6 +17,7 @@ DEF_NATIVE(pv_cpu_ops, swapgs, "swapgs");
DEF_NATIVE(, mov32, "mov %edi, %eax");
DEF_NATIVE(, mov64, "mov %rdi, %rax");
+DEF_NATIVE(, xor, "xor %rax, %rax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%rdi)");
@@ -35,6 +36,12 @@ unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len)
start__mov64, end__mov64);
}
+unsigned paravirt_patch_false(void *insnbuf, unsigned len)
+{
+ return paravirt_patch_insns(insnbuf, len,
+ start__xor, end__xor);
+}
+
extern bool pv_is_native_spin_unlock(void);
extern bool pv_is_native_vcpu_is_preempted(void);
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 2/4] paravirt: switch vcpu_is_preempted to use _paravirt_false() on bare metal
2017-09-05 13:24 ` Juergen Gross
@ 2017-09-05 13:24 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo, peterz, longman, Juergen Gross
Instead of special casing pv_lock_ops.vcpu_is_preempted when patching
use _paravirt_false() on bare metal.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/kernel/paravirt-spinlocks.c | 14 +-------------
arch/x86/kernel/paravirt_patch_32.c | 10 ----------
arch/x86/kernel/paravirt_patch_64.c | 10 ----------
3 files changed, 1 insertion(+), 33 deletions(-)
diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 8f2d1c9d43a8..26e4bd92f309 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -20,25 +20,13 @@ bool pv_is_native_spin_unlock(void)
__raw_callee_save___native_queued_spin_unlock;
}
-__visible bool __native_vcpu_is_preempted(long cpu)
-{
- return false;
-}
-PV_CALLEE_SAVE_REGS_THUNK(__native_vcpu_is_preempted);
-
-bool pv_is_native_vcpu_is_preempted(void)
-{
- return pv_lock_ops.vcpu_is_preempted.func ==
- __raw_callee_save___native_vcpu_is_preempted;
-}
-
struct pv_lock_ops pv_lock_ops = {
#ifdef CONFIG_SMP
.queued_spin_lock_slowpath = native_queued_spin_lock_slowpath,
.queued_spin_unlock = PV_CALLEE_SAVE(__native_queued_spin_unlock),
.wait = paravirt_nop,
.kick = paravirt_nop,
- .vcpu_is_preempted = PV_CALLEE_SAVE(__native_vcpu_is_preempted),
+ .vcpu_is_preempted = __PV_IS_CALLEE_SAVE(_paravirt_false),
#endif /* SMP */
};
EXPORT_SYMBOL(pv_lock_ops);
diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c
index 287c7b9735de..ea311a3563e3 100644
--- a/arch/x86/kernel/paravirt_patch_32.c
+++ b/arch/x86/kernel/paravirt_patch_32.c
@@ -13,7 +13,6 @@ DEF_NATIVE(, xor, "xor %eax, %eax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%eax)");
-DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %eax, %eax");
#endif
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len)
@@ -35,7 +34,6 @@ unsigned paravirt_patch_false(void *insnbuf, unsigned len)
}
extern bool pv_is_native_spin_unlock(void);
-extern bool pv_is_native_vcpu_is_preempted(void);
unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
unsigned long addr, unsigned len)
@@ -65,14 +63,6 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
goto patch_site;
}
goto patch_default;
-
- case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
- if (pv_is_native_vcpu_is_preempted()) {
- start = start_pv_lock_ops_vcpu_is_preempted;
- end = end_pv_lock_ops_vcpu_is_preempted;
- goto patch_site;
- }
- goto patch_default;
#endif
default:
diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index 8ab4379ceea9..64dffe4499b4 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -21,7 +21,6 @@ DEF_NATIVE(, xor, "xor %rax, %rax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%rdi)");
-DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %rax, %rax");
#endif
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len)
@@ -43,7 +42,6 @@ unsigned paravirt_patch_false(void *insnbuf, unsigned len)
}
extern bool pv_is_native_spin_unlock(void);
-extern bool pv_is_native_vcpu_is_preempted(void);
unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
unsigned long addr, unsigned len)
@@ -76,14 +74,6 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
goto patch_site;
}
goto patch_default;
-
- case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
- if (pv_is_native_vcpu_is_preempted()) {
- start = start_pv_lock_ops_vcpu_is_preempted;
- end = end_pv_lock_ops_vcpu_is_preempted;
- goto patch_site;
- }
- goto patch_default;
#endif
default:
--
2.12.3
^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 2/4] paravirt: switch vcpu_is_preempted to use _paravirt_false() on bare metal
@ 2017-09-05 13:24 ` Juergen Gross
0 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, rusty, chrisw, mingo, tglx, hpa,
longman, akataria, boris.ostrovsky
Instead of special casing pv_lock_ops.vcpu_is_preempted when patching
use _paravirt_false() on bare metal.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/kernel/paravirt-spinlocks.c | 14 +-------------
arch/x86/kernel/paravirt_patch_32.c | 10 ----------
arch/x86/kernel/paravirt_patch_64.c | 10 ----------
3 files changed, 1 insertion(+), 33 deletions(-)
diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 8f2d1c9d43a8..26e4bd92f309 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -20,25 +20,13 @@ bool pv_is_native_spin_unlock(void)
__raw_callee_save___native_queued_spin_unlock;
}
-__visible bool __native_vcpu_is_preempted(long cpu)
-{
- return false;
-}
-PV_CALLEE_SAVE_REGS_THUNK(__native_vcpu_is_preempted);
-
-bool pv_is_native_vcpu_is_preempted(void)
-{
- return pv_lock_ops.vcpu_is_preempted.func ==
- __raw_callee_save___native_vcpu_is_preempted;
-}
-
struct pv_lock_ops pv_lock_ops = {
#ifdef CONFIG_SMP
.queued_spin_lock_slowpath = native_queued_spin_lock_slowpath,
.queued_spin_unlock = PV_CALLEE_SAVE(__native_queued_spin_unlock),
.wait = paravirt_nop,
.kick = paravirt_nop,
- .vcpu_is_preempted = PV_CALLEE_SAVE(__native_vcpu_is_preempted),
+ .vcpu_is_preempted = __PV_IS_CALLEE_SAVE(_paravirt_false),
#endif /* SMP */
};
EXPORT_SYMBOL(pv_lock_ops);
diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c
index 287c7b9735de..ea311a3563e3 100644
--- a/arch/x86/kernel/paravirt_patch_32.c
+++ b/arch/x86/kernel/paravirt_patch_32.c
@@ -13,7 +13,6 @@ DEF_NATIVE(, xor, "xor %eax, %eax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%eax)");
-DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %eax, %eax");
#endif
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len)
@@ -35,7 +34,6 @@ unsigned paravirt_patch_false(void *insnbuf, unsigned len)
}
extern bool pv_is_native_spin_unlock(void);
-extern bool pv_is_native_vcpu_is_preempted(void);
unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
unsigned long addr, unsigned len)
@@ -65,14 +63,6 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
goto patch_site;
}
goto patch_default;
-
- case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
- if (pv_is_native_vcpu_is_preempted()) {
- start = start_pv_lock_ops_vcpu_is_preempted;
- end = end_pv_lock_ops_vcpu_is_preempted;
- goto patch_site;
- }
- goto patch_default;
#endif
default:
diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index 8ab4379ceea9..64dffe4499b4 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -21,7 +21,6 @@ DEF_NATIVE(, xor, "xor %rax, %rax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%rdi)");
-DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %rax, %rax");
#endif
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len)
@@ -43,7 +42,6 @@ unsigned paravirt_patch_false(void *insnbuf, unsigned len)
}
extern bool pv_is_native_spin_unlock(void);
-extern bool pv_is_native_vcpu_is_preempted(void);
unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
unsigned long addr, unsigned len)
@@ -76,14 +74,6 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
goto patch_site;
}
goto patch_default;
-
- case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
- if (pv_is_native_vcpu_is_preempted()) {
- start = start_pv_lock_ops_vcpu_is_preempted;
- end = end_pv_lock_ops_vcpu_is_preempted;
- goto patch_site;
- }
- goto patch_default;
#endif
default:
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 2/4] paravirt: switch vcpu_is_preempted to use _paravirt_false() on bare metal
2017-09-05 13:24 ` Juergen Gross
` (4 preceding siblings ...)
(?)
@ 2017-09-05 13:24 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, chrisw, mingo, tglx, hpa, longman,
akataria, boris.ostrovsky
Instead of special casing pv_lock_ops.vcpu_is_preempted when patching
use _paravirt_false() on bare metal.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/kernel/paravirt-spinlocks.c | 14 +-------------
arch/x86/kernel/paravirt_patch_32.c | 10 ----------
arch/x86/kernel/paravirt_patch_64.c | 10 ----------
3 files changed, 1 insertion(+), 33 deletions(-)
diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 8f2d1c9d43a8..26e4bd92f309 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -20,25 +20,13 @@ bool pv_is_native_spin_unlock(void)
__raw_callee_save___native_queued_spin_unlock;
}
-__visible bool __native_vcpu_is_preempted(long cpu)
-{
- return false;
-}
-PV_CALLEE_SAVE_REGS_THUNK(__native_vcpu_is_preempted);
-
-bool pv_is_native_vcpu_is_preempted(void)
-{
- return pv_lock_ops.vcpu_is_preempted.func ==
- __raw_callee_save___native_vcpu_is_preempted;
-}
-
struct pv_lock_ops pv_lock_ops = {
#ifdef CONFIG_SMP
.queued_spin_lock_slowpath = native_queued_spin_lock_slowpath,
.queued_spin_unlock = PV_CALLEE_SAVE(__native_queued_spin_unlock),
.wait = paravirt_nop,
.kick = paravirt_nop,
- .vcpu_is_preempted = PV_CALLEE_SAVE(__native_vcpu_is_preempted),
+ .vcpu_is_preempted = __PV_IS_CALLEE_SAVE(_paravirt_false),
#endif /* SMP */
};
EXPORT_SYMBOL(pv_lock_ops);
diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c
index 287c7b9735de..ea311a3563e3 100644
--- a/arch/x86/kernel/paravirt_patch_32.c
+++ b/arch/x86/kernel/paravirt_patch_32.c
@@ -13,7 +13,6 @@ DEF_NATIVE(, xor, "xor %eax, %eax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%eax)");
-DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %eax, %eax");
#endif
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len)
@@ -35,7 +34,6 @@ unsigned paravirt_patch_false(void *insnbuf, unsigned len)
}
extern bool pv_is_native_spin_unlock(void);
-extern bool pv_is_native_vcpu_is_preempted(void);
unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
unsigned long addr, unsigned len)
@@ -65,14 +63,6 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
goto patch_site;
}
goto patch_default;
-
- case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
- if (pv_is_native_vcpu_is_preempted()) {
- start = start_pv_lock_ops_vcpu_is_preempted;
- end = end_pv_lock_ops_vcpu_is_preempted;
- goto patch_site;
- }
- goto patch_default;
#endif
default:
diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index 8ab4379ceea9..64dffe4499b4 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -21,7 +21,6 @@ DEF_NATIVE(, xor, "xor %rax, %rax");
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%rdi)");
-DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %rax, %rax");
#endif
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len)
@@ -43,7 +42,6 @@ unsigned paravirt_patch_false(void *insnbuf, unsigned len)
}
extern bool pv_is_native_spin_unlock(void);
-extern bool pv_is_native_vcpu_is_preempted(void);
unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
unsigned long addr, unsigned len)
@@ -76,14 +74,6 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
goto patch_site;
}
goto patch_default;
-
- case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
- if (pv_is_native_vcpu_is_preempted()) {
- start = start_pv_lock_ops_vcpu_is_preempted;
- end = end_pv_lock_ops_vcpu_is_preempted;
- goto patch_site;
- }
- goto patch_default;
#endif
default:
--
2.12.3
^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:24 ` Juergen Gross
` (5 preceding siblings ...)
(?)
@ 2017-09-05 13:24 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, rusty, chrisw, mingo, tglx, hpa,
longman, akataria, boris.ostrovsky
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 <jgross@suse.com>
---
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;
-
- /*
- * 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;
+ return pv_virt_spin_lock(lock);
+}
+#else
+static inline bool virt_spin_lock(struct qspinlock *lock)
+{
+ return native_virt_spin_lock(lock);
}
+#endif /* CONFIG_PARAVIRT_SPINLOCKS */
#endif /* CONFIG_PARAVIRT */
#include <asm-generic/qspinlock.h>
diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 26e4bd92f309..1be187ef8a38 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
__raw_callee_save___native_queued_spin_unlock;
}
+__visible bool __native_virt_spin_lock(struct qspinlock *lock)
+{
+ return native_virt_spin_lock(lock);
+}
+PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
+
struct pv_lock_ops pv_lock_ops = {
#ifdef CONFIG_SMP
.queued_spin_lock_slowpath = native_queued_spin_lock_slowpath,
@@ -27,6 +33,14 @@ struct pv_lock_ops pv_lock_ops = {
.wait = paravirt_nop,
.kick = paravirt_nop,
.vcpu_is_preempted = __PV_IS_CALLEE_SAVE(_paravirt_false),
+ .virt_spin_lock = PV_CALLEE_SAVE(__native_virt_spin_lock),
#endif /* SMP */
};
EXPORT_SYMBOL(pv_lock_ops);
+
+void __init native_pv_lock_init(void)
+{
+ if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
+ pv_lock_ops.virt_spin_lock =
+ __PV_IS_CALLEE_SAVE(_paravirt_false);
+}
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 54b9e89d4d6b..21500d3ba359 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -77,6 +77,7 @@
#include <asm/i8259.h>
#include <asm/realmode.h>
#include <asm/misc.h>
+#include <asm/qspinlock.h>
/* Number of siblings per CPU package */
int smp_num_siblings = 1;
@@ -1381,6 +1382,7 @@ void __init native_smp_prepare_boot_cpu(void)
/* already set me in cpu_online_mask in boot_cpu_init() */
cpumask_set_cpu(me, cpu_callout_mask);
cpu_set_state_online(me);
+ native_pv_lock_init();
}
void __init native_smp_cpus_done(unsigned int max_cpus)
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:24 ` Juergen Gross
` (6 preceding siblings ...)
(?)
@ 2017-09-05 13:24 ` Juergen Gross
2017-09-05 13:55 ` Peter Zijlstra
` (7 more replies)
-1 siblings, 8 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo, peterz, longman, Juergen Gross
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 <jgross@suse.com>
---
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;
-
- /*
- * 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;
+ return pv_virt_spin_lock(lock);
+}
+#else
+static inline bool virt_spin_lock(struct qspinlock *lock)
+{
+ return native_virt_spin_lock(lock);
}
+#endif /* CONFIG_PARAVIRT_SPINLOCKS */
#endif /* CONFIG_PARAVIRT */
#include <asm-generic/qspinlock.h>
diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 26e4bd92f309..1be187ef8a38 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
__raw_callee_save___native_queued_spin_unlock;
}
+__visible bool __native_virt_spin_lock(struct qspinlock *lock)
+{
+ return native_virt_spin_lock(lock);
+}
+PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
+
struct pv_lock_ops pv_lock_ops = {
#ifdef CONFIG_SMP
.queued_spin_lock_slowpath = native_queued_spin_lock_slowpath,
@@ -27,6 +33,14 @@ struct pv_lock_ops pv_lock_ops = {
.wait = paravirt_nop,
.kick = paravirt_nop,
.vcpu_is_preempted = __PV_IS_CALLEE_SAVE(_paravirt_false),
+ .virt_spin_lock = PV_CALLEE_SAVE(__native_virt_spin_lock),
#endif /* SMP */
};
EXPORT_SYMBOL(pv_lock_ops);
+
+void __init native_pv_lock_init(void)
+{
+ if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
+ pv_lock_ops.virt_spin_lock =
+ __PV_IS_CALLEE_SAVE(_paravirt_false);
+}
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 54b9e89d4d6b..21500d3ba359 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -77,6 +77,7 @@
#include <asm/i8259.h>
#include <asm/realmode.h>
#include <asm/misc.h>
+#include <asm/qspinlock.h>
/* Number of siblings per CPU package */
int smp_num_siblings = 1;
@@ -1381,6 +1382,7 @@ void __init native_smp_prepare_boot_cpu(void)
/* already set me in cpu_online_mask in boot_cpu_init() */
cpumask_set_cpu(me, cpu_callout_mask);
cpu_set_state_online(me);
+ native_pv_lock_init();
}
void __init native_smp_cpus_done(unsigned int max_cpus)
--
2.12.3
^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:24 ` Juergen Gross
@ 2017-09-05 13:55 ` Peter Zijlstra
2017-09-05 13:55 ` Peter Zijlstra
` (6 subsequent siblings)
7 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-05 13:55 UTC (permalink / raw)
To: Juergen Gross
Cc: linux-kernel, xen-devel, x86, virtualization, jeremy, chrisw,
akataria, rusty, boris.ostrovsky, hpa, tglx, mingo, longman
On Tue, Sep 05, 2017 at 03:24:43PM +0200, Juergen Gross wrote:
> 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);
> }
>
Should this not have:
#ifdef CONFIG_PARAVIRT
?
> +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;
> +}
#endif
> +
> #ifdef CONFIG_PARAVIRT_SPINLOCKS
> extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
> extern void __pv_init_lock_hash(void);
> #ifdef CONFIG_PARAVIRT
> #define virt_spin_lock virt_spin_lock
> +#ifdef CONFIG_PARAVIRT_SPINLOCKS
> static inline bool virt_spin_lock(struct qspinlock *lock)
> {
> + return pv_virt_spin_lock(lock);
> +}
> +#else
> +static inline bool virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> }
> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
> #endif /* CONFIG_PARAVIRT */
Because I think the above only ever uses native_virt_spin_lock() when
PARAVIRT.
> @@ -1381,6 +1382,7 @@ void __init native_smp_prepare_boot_cpu(void)
> /* already set me in cpu_online_mask in boot_cpu_init() */
> cpumask_set_cpu(me, cpu_callout_mask);
> cpu_set_state_online(me);
> + native_pv_lock_init();
> }
Aah, this is where that goes.. OK that works too.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
@ 2017-09-05 13:55 ` Peter Zijlstra
0 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-05 13:55 UTC (permalink / raw)
To: Juergen Gross
Cc: jeremy, x86, linux-kernel, virtualization, chrisw, mingo, tglx,
longman, hpa, xen-devel, akataria, boris.ostrovsky
On Tue, Sep 05, 2017 at 03:24:43PM +0200, Juergen Gross wrote:
> 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);
> }
>
Should this not have:
#ifdef CONFIG_PARAVIRT
?
> +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;
> +}
#endif
> +
> #ifdef CONFIG_PARAVIRT_SPINLOCKS
> extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
> extern void __pv_init_lock_hash(void);
> #ifdef CONFIG_PARAVIRT
> #define virt_spin_lock virt_spin_lock
> +#ifdef CONFIG_PARAVIRT_SPINLOCKS
> static inline bool virt_spin_lock(struct qspinlock *lock)
> {
> + return pv_virt_spin_lock(lock);
> +}
> +#else
> +static inline bool virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> }
> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
> #endif /* CONFIG_PARAVIRT */
Because I think the above only ever uses native_virt_spin_lock() when
PARAVIRT.
> @@ -1381,6 +1382,7 @@ void __init native_smp_prepare_boot_cpu(void)
> /* already set me in cpu_online_mask in boot_cpu_init() */
> cpumask_set_cpu(me, cpu_callout_mask);
> cpu_set_state_online(me);
> + native_pv_lock_init();
> }
Aah, this is where that goes.. OK that works too.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:55 ` Peter Zijlstra
@ 2017-09-05 14:01 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 14:01 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, xen-devel, x86, virtualization, jeremy, chrisw,
akataria, rusty, boris.ostrovsky, hpa, tglx, mingo, longman
On 05/09/17 15:55, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 03:24:43PM +0200, Juergen Gross wrote:
>> 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);
>> }
>>
>
>
> Should this not have:
>
> #ifdef CONFIG_PARAVIRT
>
> ?
I can add it if you want.
Juergen
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
@ 2017-09-05 14:01 ` Juergen Gross
0 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 14:01 UTC (permalink / raw)
To: Peter Zijlstra
Cc: jeremy, x86, linux-kernel, virtualization, chrisw, mingo, tglx,
longman, hpa, xen-devel, akataria, boris.ostrovsky
On 05/09/17 15:55, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 03:24:43PM +0200, Juergen Gross wrote:
>> 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);
>> }
>>
>
>
> Should this not have:
>
> #ifdef CONFIG_PARAVIRT
>
> ?
I can add it if you want.
Juergen
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:55 ` Peter Zijlstra
(?)
(?)
@ 2017-09-05 14:01 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 14:01 UTC (permalink / raw)
To: Peter Zijlstra
Cc: jeremy, rusty, x86, linux-kernel, virtualization, chrisw, mingo,
tglx, longman, hpa, xen-devel, akataria, boris.ostrovsky
On 05/09/17 15:55, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 03:24:43PM +0200, Juergen Gross wrote:
>> 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);
>> }
>>
>
>
> Should this not have:
>
> #ifdef CONFIG_PARAVIRT
>
> ?
I can add it if you want.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:24 ` Juergen Gross
2017-09-05 13:55 ` Peter Zijlstra
@ 2017-09-05 13:55 ` Peter Zijlstra
2017-09-05 14:02 ` Waiman Long
` (5 subsequent siblings)
7 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-05 13:55 UTC (permalink / raw)
To: Juergen Gross
Cc: jeremy, rusty, x86, linux-kernel, virtualization, chrisw, mingo,
tglx, longman, hpa, xen-devel, akataria, boris.ostrovsky
On Tue, Sep 05, 2017 at 03:24:43PM +0200, Juergen Gross wrote:
> 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);
> }
>
Should this not have:
#ifdef CONFIG_PARAVIRT
?
> +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;
> +}
#endif
> +
> #ifdef CONFIG_PARAVIRT_SPINLOCKS
> extern void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
> extern void __pv_init_lock_hash(void);
> #ifdef CONFIG_PARAVIRT
> #define virt_spin_lock virt_spin_lock
> +#ifdef CONFIG_PARAVIRT_SPINLOCKS
> static inline bool virt_spin_lock(struct qspinlock *lock)
> {
> + return pv_virt_spin_lock(lock);
> +}
> +#else
> +static inline bool virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> }
> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
> #endif /* CONFIG_PARAVIRT */
Because I think the above only ever uses native_virt_spin_lock() when
PARAVIRT.
> @@ -1381,6 +1382,7 @@ void __init native_smp_prepare_boot_cpu(void)
> /* already set me in cpu_online_mask in boot_cpu_init() */
> cpumask_set_cpu(me, cpu_callout_mask);
> cpu_set_state_online(me);
> + native_pv_lock_init();
> }
Aah, this is where that goes.. OK that works too.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:24 ` Juergen Gross
2017-09-05 13:55 ` Peter Zijlstra
2017-09-05 13:55 ` Peter Zijlstra
@ 2017-09-05 14:02 ` Waiman Long
2017-09-05 14:02 ` Waiman Long
` (4 subsequent siblings)
7 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:02 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, rusty, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
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 <jgross@suse.com>
> ---
> 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;
> +
I think you can take the above if statement out as you has done test in
native_pv_lock_init(). So the test will also be false here.
As this patch series is x86 specific, you should probably add "x86/" in
front of paravirt in the patch titles.
Cheers,
Longman
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:24 ` Juergen Gross
` (2 preceding siblings ...)
2017-09-05 14:02 ` Waiman Long
@ 2017-09-05 14:02 ` Waiman Long
2017-09-05 14:08 ` Peter Zijlstra
2017-09-05 14:08 ` Peter Zijlstra
2017-09-05 14:02 ` Waiman Long
` (3 subsequent siblings)
7 siblings, 2 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:02 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo, peterz
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 <jgross@suse.com>
> ---
> 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;
> +
I think you can take the above if statement out as you has done test in
native_pv_lock_init(). So the test will also be false here.
As this patch series is x86 specific, you should probably add "x86/" in
front of paravirt in the patch titles.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:02 ` Waiman Long
@ 2017-09-05 14:08 ` Peter Zijlstra
2017-09-05 14:08 ` Peter Zijlstra
1 sibling, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-05 14:08 UTC (permalink / raw)
To: Waiman Long
Cc: Juergen Gross, linux-kernel, xen-devel, x86, virtualization,
jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo
On Tue, Sep 05, 2017 at 10:02:57AM -0400, Waiman Long wrote:
> On 09/05/2017 09:24 AM, Juergen Gross wrote:
> > +static inline bool native_virt_spin_lock(struct qspinlock *lock)
> > +{
> > + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> > + return false;
> > +
>
> I think you can take the above if statement out as you has done test in
> native_pv_lock_init(). So the test will also be false here.
That does mean we'll run a test-and-set spinlock until paravirt patching
happens though. I prefer to not do that.
One important point.. we must not be holding any locks when we switch
over between the two locks. Back then I spend some time making sure that
didn't happen with the X86 feature flag muck.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
@ 2017-09-05 14:08 ` Peter Zijlstra
0 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-05 14:08 UTC (permalink / raw)
To: Waiman Long
Cc: Juergen Gross, jeremy, x86, linux-kernel, virtualization, chrisw,
mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
On Tue, Sep 05, 2017 at 10:02:57AM -0400, Waiman Long wrote:
> On 09/05/2017 09:24 AM, Juergen Gross wrote:
> > +static inline bool native_virt_spin_lock(struct qspinlock *lock)
> > +{
> > + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> > + return false;
> > +
>
> I think you can take the above if statement out as you has done test in
> native_pv_lock_init(). So the test will also be false here.
That does mean we'll run a test-and-set spinlock until paravirt patching
happens though. I prefer to not do that.
One important point.. we must not be holding any locks when we switch
over between the two locks. Back then I spend some time making sure that
didn't happen with the X86 feature flag muck.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:08 ` Peter Zijlstra
(?)
@ 2017-09-05 14:19 ` Waiman Long
-1 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:19 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, jeremy, x86, linux-kernel, virtualization, chrisw,
mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
On 09/05/2017 10:08 AM, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 10:02:57AM -0400, Waiman Long wrote:
>> On 09/05/2017 09:24 AM, Juergen Gross wrote:
>>> +static inline bool native_virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>> + return false;
>>> +
>> I think you can take the above if statement out as you has done test in
>> native_pv_lock_init(). So the test will also be false here.
> That does mean we'll run a test-and-set spinlock until paravirt patching
> happens though. I prefer to not do that.
>
> One important point.. we must not be holding any locks when we switch
> over between the two locks. Back then I spend some time making sure that
> didn't happen with the X86 feature flag muck.
AFAICT, native_pv_lock_init() is called before SMP init. So it shouldn't
matter.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:08 ` Peter Zijlstra
(?)
(?)
@ 2017-09-05 14:19 ` Waiman Long
-1 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:19 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, jeremy, rusty, x86, linux-kernel, virtualization,
chrisw, mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
On 09/05/2017 10:08 AM, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 10:02:57AM -0400, Waiman Long wrote:
>> On 09/05/2017 09:24 AM, Juergen Gross wrote:
>>> +static inline bool native_virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>> + return false;
>>> +
>> I think you can take the above if statement out as you has done test in
>> native_pv_lock_init(). So the test will also be false here.
> That does mean we'll run a test-and-set spinlock until paravirt patching
> happens though. I prefer to not do that.
>
> One important point.. we must not be holding any locks when we switch
> over between the two locks. Back then I spend some time making sure that
> didn't happen with the X86 feature flag muck.
AFAICT, native_pv_lock_init() is called before SMP init. So it shouldn't
matter.
Cheers,
Longman
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:08 ` Peter Zijlstra
` (2 preceding siblings ...)
(?)
@ 2017-09-05 14:19 ` Waiman Long
-1 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:19 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, linux-kernel, xen-devel, x86, virtualization,
jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo
On 09/05/2017 10:08 AM, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 10:02:57AM -0400, Waiman Long wrote:
>> On 09/05/2017 09:24 AM, Juergen Gross wrote:
>>> +static inline bool native_virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
>>> + return false;
>>> +
>> I think you can take the above if statement out as you has done test in
>> native_pv_lock_init(). So the test will also be false here.
> That does mean we'll run a test-and-set spinlock until paravirt patching
> happens though. I prefer to not do that.
>
> One important point.. we must not be holding any locks when we switch
> over between the two locks. Back then I spend some time making sure that
> didn't happen with the X86 feature flag muck.
AFAICT, native_pv_lock_init() is called before SMP init. So it shouldn't
matter.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:02 ` Waiman Long
2017-09-05 14:08 ` Peter Zijlstra
@ 2017-09-05 14:08 ` Peter Zijlstra
1 sibling, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-05 14:08 UTC (permalink / raw)
To: Waiman Long
Cc: Juergen Gross, jeremy, rusty, x86, linux-kernel, virtualization,
chrisw, mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
On Tue, Sep 05, 2017 at 10:02:57AM -0400, Waiman Long wrote:
> On 09/05/2017 09:24 AM, Juergen Gross wrote:
> > +static inline bool native_virt_spin_lock(struct qspinlock *lock)
> > +{
> > + if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
> > + return false;
> > +
>
> I think you can take the above if statement out as you has done test in
> native_pv_lock_init(). So the test will also be false here.
That does mean we'll run a test-and-set spinlock until paravirt patching
happens though. I prefer to not do that.
One important point.. we must not be holding any locks when we switch
over between the two locks. Back then I spend some time making sure that
didn't happen with the X86 feature flag muck.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:24 ` Juergen Gross
` (3 preceding siblings ...)
2017-09-05 14:02 ` Waiman Long
@ 2017-09-05 14:02 ` Waiman Long
2017-09-05 14:10 ` Waiman Long
` (2 subsequent siblings)
7 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:02 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria, boris.ostrovsky
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 <jgross@suse.com>
> ---
> 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;
> +
I think you can take the above if statement out as you has done test in
native_pv_lock_init(). So the test will also be false here.
As this patch series is x86 specific, you should probably add "x86/" in
front of paravirt in the patch titles.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:24 ` Juergen Gross
` (4 preceding siblings ...)
2017-09-05 14:02 ` Waiman Long
@ 2017-09-05 14:10 ` Waiman Long
2017-09-05 14:10 ` Waiman Long
2017-09-05 14:10 ` Waiman Long
7 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:10 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria, boris.ostrovsky
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 <jgross@suse.com>
> ---
> 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?
> -
> - /*
> - * 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;
> + return pv_virt_spin_lock(lock);
> +}
> +#else
> +static inline bool virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> }
> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
> #endif /* CONFIG_PARAVIRT */
>
> #include <asm-generic/qspinlock.h>
> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
> index 26e4bd92f309..1be187ef8a38 100644
> --- a/arch/x86/kernel/paravirt-spinlocks.c
> +++ b/arch/x86/kernel/paravirt-spinlocks.c
> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
> __raw_callee_save___native_queued_spin_unlock;
> }
>
> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> +}
> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
I have some concern about the overhead of register saving/restoring have
on spin lock performance in case the kernel is under a non-KVM/Xen
hypervisor.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:24 ` Juergen Gross
` (5 preceding siblings ...)
2017-09-05 14:10 ` Waiman Long
@ 2017-09-05 14:10 ` Waiman Long
2017-09-05 14:10 ` Waiman Long
7 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:10 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, rusty, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
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 <jgross@suse.com>
> ---
> 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?
> -
> - /*
> - * 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;
> + return pv_virt_spin_lock(lock);
> +}
> +#else
> +static inline bool virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> }
> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
> #endif /* CONFIG_PARAVIRT */
>
> #include <asm-generic/qspinlock.h>
> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
> index 26e4bd92f309..1be187ef8a38 100644
> --- a/arch/x86/kernel/paravirt-spinlocks.c
> +++ b/arch/x86/kernel/paravirt-spinlocks.c
> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
> __raw_callee_save___native_queued_spin_unlock;
> }
>
> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> +}
> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
I have some concern about the overhead of register saving/restoring have
on spin lock performance in case the kernel is under a non-KVM/Xen
hypervisor.
Cheers,
Longman
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:24 ` Juergen Gross
` (6 preceding siblings ...)
2017-09-05 14:10 ` Waiman Long
@ 2017-09-05 14:10 ` Waiman Long
2017-09-05 14:18 ` Juergen Gross
` (2 more replies)
7 siblings, 3 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:10 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo, peterz
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 <jgross@suse.com>
> ---
> 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?
> -
> - /*
> - * 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;
> + return pv_virt_spin_lock(lock);
> +}
> +#else
> +static inline bool virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> }
> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
> #endif /* CONFIG_PARAVIRT */
>
> #include <asm-generic/qspinlock.h>
> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
> index 26e4bd92f309..1be187ef8a38 100644
> --- a/arch/x86/kernel/paravirt-spinlocks.c
> +++ b/arch/x86/kernel/paravirt-spinlocks.c
> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
> __raw_callee_save___native_queued_spin_unlock;
> }
>
> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
> +{
> + return native_virt_spin_lock(lock);
> +}
> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
I have some concern about the overhead of register saving/restoring have
on spin lock performance in case the kernel is under a non-KVM/Xen
hypervisor.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:10 ` Waiman Long
@ 2017-09-05 14:18 ` Juergen Gross
2017-09-05 14:18 ` Juergen Gross
2017-09-05 14:18 ` Juergen Gross
2 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 14:18 UTC (permalink / raw)
To: Waiman Long, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, rusty, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
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 <jgross@suse.com>
>> ---
>> 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...
>
>> -
>> - /*
>> - * 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;
>> + return pv_virt_spin_lock(lock);
>> +}
>> +#else
>> +static inline bool virt_spin_lock(struct qspinlock *lock)
>> +{
>> + return native_virt_spin_lock(lock);
>> }
>> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
>> #endif /* CONFIG_PARAVIRT */
>>
>> #include <asm-generic/qspinlock.h>
>> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
>> index 26e4bd92f309..1be187ef8a38 100644
>> --- a/arch/x86/kernel/paravirt-spinlocks.c
>> +++ b/arch/x86/kernel/paravirt-spinlocks.c
>> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
>> __raw_callee_save___native_queued_spin_unlock;
>> }
>>
>> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
>> +{
>> + return native_virt_spin_lock(lock);
>> +}
>> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
>
> I have some concern about the overhead of register saving/restoring have
> on spin lock performance in case the kernel is under a non-KVM/Xen
> hypervisor.
We are on the slow path already.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:10 ` Waiman Long
2017-09-05 14:18 ` Juergen Gross
@ 2017-09-05 14:18 ` Juergen Gross
2017-09-05 14:18 ` Juergen Gross
2 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 14:18 UTC (permalink / raw)
To: Waiman Long, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria, boris.ostrovsky
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 <jgross@suse.com>
>> ---
>> 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...
>
>> -
>> - /*
>> - * 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;
>> + return pv_virt_spin_lock(lock);
>> +}
>> +#else
>> +static inline bool virt_spin_lock(struct qspinlock *lock)
>> +{
>> + return native_virt_spin_lock(lock);
>> }
>> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
>> #endif /* CONFIG_PARAVIRT */
>>
>> #include <asm-generic/qspinlock.h>
>> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
>> index 26e4bd92f309..1be187ef8a38 100644
>> --- a/arch/x86/kernel/paravirt-spinlocks.c
>> +++ b/arch/x86/kernel/paravirt-spinlocks.c
>> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
>> __raw_callee_save___native_queued_spin_unlock;
>> }
>>
>> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
>> +{
>> + return native_virt_spin_lock(lock);
>> +}
>> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
>
> I have some concern about the overhead of register saving/restoring have
> on spin lock performance in case the kernel is under a non-KVM/Xen
> hypervisor.
We are on the slow path already.
Juergen
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:10 ` Waiman Long
2017-09-05 14:18 ` Juergen Gross
2017-09-05 14:18 ` Juergen Gross
@ 2017-09-05 14:18 ` Juergen Gross
2017-09-05 14:24 ` Waiman Long
2017-09-05 14:24 ` Waiman Long
2 siblings, 2 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 14:18 UTC (permalink / raw)
To: Waiman Long, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo, peterz
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 <jgross@suse.com>
>> ---
>> 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...
>
>> -
>> - /*
>> - * 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;
>> + return pv_virt_spin_lock(lock);
>> +}
>> +#else
>> +static inline bool virt_spin_lock(struct qspinlock *lock)
>> +{
>> + return native_virt_spin_lock(lock);
>> }
>> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
>> #endif /* CONFIG_PARAVIRT */
>>
>> #include <asm-generic/qspinlock.h>
>> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
>> index 26e4bd92f309..1be187ef8a38 100644
>> --- a/arch/x86/kernel/paravirt-spinlocks.c
>> +++ b/arch/x86/kernel/paravirt-spinlocks.c
>> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
>> __raw_callee_save___native_queued_spin_unlock;
>> }
>>
>> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
>> +{
>> + return native_virt_spin_lock(lock);
>> +}
>> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
>
> I have some concern about the overhead of register saving/restoring have
> on spin lock performance in case the kernel is under a non-KVM/Xen
> hypervisor.
We are on the slow path already.
Juergen
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:18 ` Juergen Gross
@ 2017-09-05 14:24 ` Waiman Long
2017-09-05 14:24 ` Waiman Long
1 sibling, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:24 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, rusty, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
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 <jgross@suse.com>
>>> ---
>>> 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.
>>> -
>>> - /*
>>> - * 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;
>>> + return pv_virt_spin_lock(lock);
>>> +}
>>> +#else
>>> +static inline bool virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + return native_virt_spin_lock(lock);
>>> }
>>> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
>>> #endif /* CONFIG_PARAVIRT */
>>>
>>> #include <asm-generic/qspinlock.h>
>>> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
>>> index 26e4bd92f309..1be187ef8a38 100644
>>> --- a/arch/x86/kernel/paravirt-spinlocks.c
>>> +++ b/arch/x86/kernel/paravirt-spinlocks.c
>>> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
>>> __raw_callee_save___native_queued_spin_unlock;
>>> }
>>>
>>> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + return native_virt_spin_lock(lock);
>>> +}
>>> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
>> I have some concern about the overhead of register saving/restoring have
>> on spin lock performance in case the kernel is under a non-KVM/Xen
>> hypervisor.
> We are on the slow path already.
That is true, but I still still believe there will be performance impact
on lock contention behavior where the slowpath will be used.
Cheers,
Longman
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:18 ` Juergen Gross
@ 2017-09-05 14:24 ` Waiman Long
2017-09-05 14:24 ` Waiman Long
1 sibling, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:24 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo, peterz
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 <jgross@suse.com>
>>> ---
>>> 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.
>>> -
>>> - /*
>>> - * 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;
>>> + return pv_virt_spin_lock(lock);
>>> +}
>>> +#else
>>> +static inline bool virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + return native_virt_spin_lock(lock);
>>> }
>>> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
>>> #endif /* CONFIG_PARAVIRT */
>>>
>>> #include <asm-generic/qspinlock.h>
>>> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
>>> index 26e4bd92f309..1be187ef8a38 100644
>>> --- a/arch/x86/kernel/paravirt-spinlocks.c
>>> +++ b/arch/x86/kernel/paravirt-spinlocks.c
>>> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
>>> __raw_callee_save___native_queued_spin_unlock;
>>> }
>>>
>>> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + return native_virt_spin_lock(lock);
>>> +}
>>> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
>> I have some concern about the overhead of register saving/restoring have
>> on spin lock performance in case the kernel is under a non-KVM/Xen
>> hypervisor.
> We are on the slow path already.
That is true, but I still still believe there will be performance impact
on lock contention behavior where the slowpath will be used.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
@ 2017-09-05 14:24 ` Waiman Long
0 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:24 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria, boris.ostrovsky
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 <jgross@suse.com>
>>> ---
>>> 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.
>>> -
>>> - /*
>>> - * 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;
>>> + return pv_virt_spin_lock(lock);
>>> +}
>>> +#else
>>> +static inline bool virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + return native_virt_spin_lock(lock);
>>> }
>>> +#endif /* CONFIG_PARAVIRT_SPINLOCKS */
>>> #endif /* CONFIG_PARAVIRT */
>>>
>>> #include <asm-generic/qspinlock.h>
>>> diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
>>> index 26e4bd92f309..1be187ef8a38 100644
>>> --- a/arch/x86/kernel/paravirt-spinlocks.c
>>> +++ b/arch/x86/kernel/paravirt-spinlocks.c
>>> @@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
>>> __raw_callee_save___native_queued_spin_unlock;
>>> }
>>>
>>> +__visible bool __native_virt_spin_lock(struct qspinlock *lock)
>>> +{
>>> + return native_virt_spin_lock(lock);
>>> +}
>>> +PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
>> I have some concern about the overhead of register saving/restoring have
>> on spin lock performance in case the kernel is under a non-KVM/Xen
>> hypervisor.
> We are on the slow path already.
That is true, but I still still believe there will be performance impact
on lock contention behavior where the slowpath will be used.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:24 ` Waiman Long
@ 2017-09-05 14:31 ` Waiman Long
-1 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:31 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo, peterz
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 <jgross@suse.com>
>>>> ---
>>>> 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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
@ 2017-09-05 14:31 ` Waiman Long
0 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:31 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria, boris.ostrovsky
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 <jgross@suse.com>
>>>> ---
>>>> 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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:31 ` Waiman Long
(?)
@ 2017-09-05 14:42 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 14:42 UTC (permalink / raw)
To: Waiman Long, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, chrisw, mingo, tglx, hpa, akataria, boris.ostrovsky
On 05/09/17 16:31, Waiman Long wrote:
> 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 <jgross@suse.com>
>>>>> ---
>>>>> 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.
Aah, okay.
Bare metal could make use of this jump label, too.
I'll have a try how it looks like.
Juergen
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:31 ` Waiman Long
(?)
(?)
@ 2017-09-05 14:42 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 14:42 UTC (permalink / raw)
To: Waiman Long, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo, peterz
On 05/09/17 16:31, Waiman Long wrote:
> 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 <jgross@suse.com>
>>>>> ---
>>>>> 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.
Aah, okay.
Bare metal could make use of this jump label, too.
I'll have a try how it looks like.
Juergen
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:31 ` Waiman Long
` (2 preceding siblings ...)
(?)
@ 2017-09-05 14:42 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 14:42 UTC (permalink / raw)
To: Waiman Long, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, rusty, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
On 05/09/17 16:31, Waiman Long wrote:
> 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 <jgross@suse.com>
>>>>> ---
>>>>> 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.
Aah, okay.
Bare metal could make use of this jump label, too.
I'll have a try how it looks like.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:31 ` Waiman Long
` (3 preceding siblings ...)
(?)
@ 2017-09-06 7:08 ` Peter Zijlstra
-1 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-06 7:08 UTC (permalink / raw)
To: Waiman Long
Cc: Juergen Gross, jeremy, rusty, x86, linux-kernel, virtualization,
chrisw, mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
Guys, please trim email.
On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
> 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.
I don't understand. What performance are you worried about. Native will
now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:31 ` Waiman Long
` (4 preceding siblings ...)
(?)
@ 2017-09-06 7:08 ` Peter Zijlstra
-1 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-06 7:08 UTC (permalink / raw)
To: Waiman Long
Cc: Juergen Gross, jeremy, x86, linux-kernel, virtualization, chrisw,
mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
Guys, please trim email.
On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
> 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.
I don't understand. What performance are you worried about. Native will
now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:31 ` Waiman Long
` (5 preceding siblings ...)
(?)
@ 2017-09-06 7:08 ` Peter Zijlstra
2017-09-06 12:44 ` Waiman Long
` (2 more replies)
-1 siblings, 3 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-06 7:08 UTC (permalink / raw)
To: Waiman Long
Cc: Juergen Gross, linux-kernel, xen-devel, x86, virtualization,
jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo
Guys, please trim email.
On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
> 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.
I don't understand. What performance are you worried about. Native will
now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-06 7:08 ` Peter Zijlstra
@ 2017-09-06 12:44 ` Waiman Long
2017-09-06 12:44 ` Waiman Long
2017-09-06 12:44 ` Waiman Long
2 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-06 12:44 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, jeremy, rusty, x86, linux-kernel, virtualization,
chrisw, mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
On 09/06/2017 03:08 AM, Peter Zijlstra wrote:
> Guys, please trim email.
>
> On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
>> 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.
> I don't understand. What performance are you worried about. Native will
> now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
It is not native that I am talking about. I am worry about VM with
non-Xen/KVM hypervisor where virt_spin_lock() will actually be called.
Now that function will become a callee-saved function call instead of
being inlined into the native slowpath function.
Cheers,
Longman
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-06 7:08 ` Peter Zijlstra
2017-09-06 12:44 ` Waiman Long
@ 2017-09-06 12:44 ` Waiman Long
2017-09-06 13:06 ` Peter Zijlstra
` (2 more replies)
2017-09-06 12:44 ` Waiman Long
2 siblings, 3 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-06 12:44 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, linux-kernel, xen-devel, x86, virtualization,
jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo
On 09/06/2017 03:08 AM, Peter Zijlstra wrote:
> Guys, please trim email.
>
> On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
>> 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.
> I don't understand. What performance are you worried about. Native will
> now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
It is not native that I am talking about. I am worry about VM with
non-Xen/KVM hypervisor where virt_spin_lock() will actually be called.
Now that function will become a callee-saved function call instead of
being inlined into the native slowpath function.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-06 12:44 ` Waiman Long
@ 2017-09-06 13:06 ` Peter Zijlstra
2017-09-06 13:06 ` Peter Zijlstra
2017-09-06 13:06 ` Peter Zijlstra
2 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-06 13:06 UTC (permalink / raw)
To: Waiman Long
Cc: Juergen Gross, jeremy, x86, linux-kernel, virtualization, chrisw,
mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
On Wed, Sep 06, 2017 at 08:44:09AM -0400, Waiman Long wrote:
> On 09/06/2017 03:08 AM, Peter Zijlstra wrote:
> > Guys, please trim email.
> >
> > On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
> >> 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.
> > I don't understand. What performance are you worried about. Native will
> > now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
>
> It is not native that I am talking about. I am worry about VM with
> non-Xen/KVM hypervisor where virt_spin_lock() will actually be called.
> Now that function will become a callee-saved function call instead of
> being inlined into the native slowpath function.
But only if we actually end up using the test-and-set thing, because if
you have paravirt we end up using that.
And the test-and-set thing sucks anyway. But yes, you're right, that
case gets worse.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-06 12:44 ` Waiman Long
2017-09-06 13:06 ` Peter Zijlstra
@ 2017-09-06 13:06 ` Peter Zijlstra
2017-09-06 13:06 ` Peter Zijlstra
2 siblings, 0 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-06 13:06 UTC (permalink / raw)
To: Waiman Long
Cc: Juergen Gross, jeremy, rusty, x86, linux-kernel, virtualization,
chrisw, mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
On Wed, Sep 06, 2017 at 08:44:09AM -0400, Waiman Long wrote:
> On 09/06/2017 03:08 AM, Peter Zijlstra wrote:
> > Guys, please trim email.
> >
> > On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
> >> 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.
> > I don't understand. What performance are you worried about. Native will
> > now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
>
> It is not native that I am talking about. I am worry about VM with
> non-Xen/KVM hypervisor where virt_spin_lock() will actually be called.
> Now that function will become a callee-saved function call instead of
> being inlined into the native slowpath function.
But only if we actually end up using the test-and-set thing, because if
you have paravirt we end up using that.
And the test-and-set thing sucks anyway. But yes, you're right, that
case gets worse.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-06 12:44 ` Waiman Long
2017-09-06 13:06 ` Peter Zijlstra
2017-09-06 13:06 ` Peter Zijlstra
@ 2017-09-06 13:06 ` Peter Zijlstra
2017-09-06 13:11 ` Waiman Long
` (2 more replies)
2 siblings, 3 replies; 56+ messages in thread
From: Peter Zijlstra @ 2017-09-06 13:06 UTC (permalink / raw)
To: Waiman Long
Cc: Juergen Gross, linux-kernel, xen-devel, x86, virtualization,
jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo
On Wed, Sep 06, 2017 at 08:44:09AM -0400, Waiman Long wrote:
> On 09/06/2017 03:08 AM, Peter Zijlstra wrote:
> > Guys, please trim email.
> >
> > On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
> >> 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.
> > I don't understand. What performance are you worried about. Native will
> > now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
>
> It is not native that I am talking about. I am worry about VM with
> non-Xen/KVM hypervisor where virt_spin_lock() will actually be called.
> Now that function will become a callee-saved function call instead of
> being inlined into the native slowpath function.
But only if we actually end up using the test-and-set thing, because if
you have paravirt we end up using that.
And the test-and-set thing sucks anyway. But yes, you're right, that
case gets worse.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-06 13:06 ` Peter Zijlstra
@ 2017-09-06 13:11 ` Waiman Long
2017-09-06 13:11 ` Waiman Long
2017-09-06 13:11 ` Waiman Long
2 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-06 13:11 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, jeremy, rusty, x86, linux-kernel, virtualization,
chrisw, mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
On 09/06/2017 09:06 AM, Peter Zijlstra wrote:
> On Wed, Sep 06, 2017 at 08:44:09AM -0400, Waiman Long wrote:
>> On 09/06/2017 03:08 AM, Peter Zijlstra wrote:
>>> Guys, please trim email.
>>>
>>> On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
>>>> 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.
>>> I don't understand. What performance are you worried about. Native will
>>> now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
>> It is not native that I am talking about. I am worry about VM with
>> non-Xen/KVM hypervisor where virt_spin_lock() will actually be called.
>> Now that function will become a callee-saved function call instead of
>> being inlined into the native slowpath function.
> But only if we actually end up using the test-and-set thing, because if
> you have paravirt we end up using that.
I am talking about scenario like a kernel with paravirt spinlock running
in a VM managed by VMware, for example. We may not want to penalize them
while there are alternatives with less penalty.
Cheers,
Longman
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-06 13:06 ` Peter Zijlstra
2017-09-06 13:11 ` Waiman Long
@ 2017-09-06 13:11 ` Waiman Long
2017-09-06 13:11 ` Waiman Long
2 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-06 13:11 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, linux-kernel, xen-devel, x86, virtualization,
jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo
On 09/06/2017 09:06 AM, Peter Zijlstra wrote:
> On Wed, Sep 06, 2017 at 08:44:09AM -0400, Waiman Long wrote:
>> On 09/06/2017 03:08 AM, Peter Zijlstra wrote:
>>> Guys, please trim email.
>>>
>>> On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
>>>> 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.
>>> I don't understand. What performance are you worried about. Native will
>>> now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
>> It is not native that I am talking about. I am worry about VM with
>> non-Xen/KVM hypervisor where virt_spin_lock() will actually be called.
>> Now that function will become a callee-saved function call instead of
>> being inlined into the native slowpath function.
> But only if we actually end up using the test-and-set thing, because if
> you have paravirt we end up using that.
I am talking about scenario like a kernel with paravirt spinlock running
in a VM managed by VMware, for example. We may not want to penalize them
while there are alternatives with less penalty.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-06 13:06 ` Peter Zijlstra
2017-09-06 13:11 ` Waiman Long
2017-09-06 13:11 ` Waiman Long
@ 2017-09-06 13:11 ` Waiman Long
2 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-06 13:11 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, jeremy, x86, linux-kernel, virtualization, chrisw,
mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
On 09/06/2017 09:06 AM, Peter Zijlstra wrote:
> On Wed, Sep 06, 2017 at 08:44:09AM -0400, Waiman Long wrote:
>> On 09/06/2017 03:08 AM, Peter Zijlstra wrote:
>>> Guys, please trim email.
>>>
>>> On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
>>>> 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.
>>> I don't understand. What performance are you worried about. Native will
>>> now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
>> It is not native that I am talking about. I am worry about VM with
>> non-Xen/KVM hypervisor where virt_spin_lock() will actually be called.
>> Now that function will become a callee-saved function call instead of
>> being inlined into the native slowpath function.
> But only if we actually end up using the test-and-set thing, because if
> you have paravirt we end up using that.
I am talking about scenario like a kernel with paravirt spinlock running
in a VM managed by VMware, for example. We may not want to penalize them
while there are alternatives with less penalty.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-06 7:08 ` Peter Zijlstra
2017-09-06 12:44 ` Waiman Long
2017-09-06 12:44 ` Waiman Long
@ 2017-09-06 12:44 ` Waiman Long
2 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-06 12:44 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Juergen Gross, jeremy, x86, linux-kernel, virtualization, chrisw,
mingo, tglx, hpa, xen-devel, akataria, boris.ostrovsky
On 09/06/2017 03:08 AM, Peter Zijlstra wrote:
> Guys, please trim email.
>
> On Tue, Sep 05, 2017 at 10:31:46AM -0400, Waiman Long wrote:
>> 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.
> I don't understand. What performance are you worried about. Native will
> now do: "xor rax,rax; jnz some_cold_label" that's fairly trival code.
It is not native that I am talking about. I am worry about VM with
non-Xen/KVM hypervisor where virt_spin_lock() will actually be called.
Now that function will become a callee-saved function call instead of
being inlined into the native slowpath function.
Cheers,
Longman
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 14:24 ` Waiman Long
(?)
(?)
@ 2017-09-05 14:31 ` Waiman Long
-1 siblings, 0 replies; 56+ messages in thread
From: Waiman Long @ 2017-09-05 14:31 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, peterz, rusty, chrisw, mingo, tglx, hpa, akataria,
boris.ostrovsky
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 <jgross@suse.com>
>>>> ---
>>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 3/4] paravirt: add virt_spin_lock pvops function
2017-09-05 13:24 ` Juergen Gross
` (7 preceding siblings ...)
(?)
@ 2017-09-05 13:24 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, chrisw, mingo, tglx, hpa, longman,
akataria, boris.ostrovsky
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 <jgross@suse.com>
---
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;
-
- /*
- * 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;
+ return pv_virt_spin_lock(lock);
+}
+#else
+static inline bool virt_spin_lock(struct qspinlock *lock)
+{
+ return native_virt_spin_lock(lock);
}
+#endif /* CONFIG_PARAVIRT_SPINLOCKS */
#endif /* CONFIG_PARAVIRT */
#include <asm-generic/qspinlock.h>
diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 26e4bd92f309..1be187ef8a38 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -20,6 +20,12 @@ bool pv_is_native_spin_unlock(void)
__raw_callee_save___native_queued_spin_unlock;
}
+__visible bool __native_virt_spin_lock(struct qspinlock *lock)
+{
+ return native_virt_spin_lock(lock);
+}
+PV_CALLEE_SAVE_REGS_THUNK(__native_virt_spin_lock);
+
struct pv_lock_ops pv_lock_ops = {
#ifdef CONFIG_SMP
.queued_spin_lock_slowpath = native_queued_spin_lock_slowpath,
@@ -27,6 +33,14 @@ struct pv_lock_ops pv_lock_ops = {
.wait = paravirt_nop,
.kick = paravirt_nop,
.vcpu_is_preempted = __PV_IS_CALLEE_SAVE(_paravirt_false),
+ .virt_spin_lock = PV_CALLEE_SAVE(__native_virt_spin_lock),
#endif /* SMP */
};
EXPORT_SYMBOL(pv_lock_ops);
+
+void __init native_pv_lock_init(void)
+{
+ if (!static_cpu_has(X86_FEATURE_HYPERVISOR))
+ pv_lock_ops.virt_spin_lock =
+ __PV_IS_CALLEE_SAVE(_paravirt_false);
+}
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 54b9e89d4d6b..21500d3ba359 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -77,6 +77,7 @@
#include <asm/i8259.h>
#include <asm/realmode.h>
#include <asm/misc.h>
+#include <asm/qspinlock.h>
/* Number of siblings per CPU package */
int smp_num_siblings = 1;
@@ -1381,6 +1382,7 @@ void __init native_smp_prepare_boot_cpu(void)
/* already set me in cpu_online_mask in boot_cpu_init() */
cpumask_set_cpu(me, cpu_callout_mask);
cpu_set_state_online(me);
+ native_pv_lock_init();
}
void __init native_smp_cpus_done(unsigned int max_cpus)
--
2.12.3
^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 4/4] paravirt,xen: correct xen_nopvspin case
2017-09-05 13:24 ` Juergen Gross
` (8 preceding siblings ...)
(?)
@ 2017-09-05 13:24 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, rusty, chrisw, mingo, tglx, hpa,
longman, akataria, boris.ostrovsky
With the boot parameter "xen_nopvspin" specified a Xen guest should not
make use of paravirt spinlocks, but behave as if running on bare
metal. This is not true, however, as the qspinlock code will fall back
to a test-and-set scheme when it is detecting a hypervisor.
In order to avoid this set the virt_spin_lock pvops function to
_paravirt_false() in case xen_nopvspin has been specified.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/xen/spinlock.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 25a7c4302ce7..c01cedfa745d 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -129,6 +129,8 @@ void __init xen_init_spinlocks(void)
if (!xen_pvspin) {
printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
+ pv_lock_ops.virt_spin_lock =
+ __PV_IS_CALLEE_SAVE(_paravirt_false);
return;
}
printk(KERN_DEBUG "xen: PV spinlocks enabled\n");
--
2.12.3
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 4/4] paravirt,xen: correct xen_nopvspin case
2017-09-05 13:24 ` Juergen Gross
` (9 preceding siblings ...)
(?)
@ 2017-09-05 13:24 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: jeremy, chrisw, akataria, rusty, boris.ostrovsky, hpa, tglx,
mingo, peterz, longman, Juergen Gross
With the boot parameter "xen_nopvspin" specified a Xen guest should not
make use of paravirt spinlocks, but behave as if running on bare
metal. This is not true, however, as the qspinlock code will fall back
to a test-and-set scheme when it is detecting a hypervisor.
In order to avoid this set the virt_spin_lock pvops function to
_paravirt_false() in case xen_nopvspin has been specified.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/xen/spinlock.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 25a7c4302ce7..c01cedfa745d 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -129,6 +129,8 @@ void __init xen_init_spinlocks(void)
if (!xen_pvspin) {
printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
+ pv_lock_ops.virt_spin_lock =
+ __PV_IS_CALLEE_SAVE(_paravirt_false);
return;
}
printk(KERN_DEBUG "xen: PV spinlocks enabled\n");
--
2.12.3
^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 4/4] paravirt,xen: correct xen_nopvspin case
2017-09-05 13:24 ` Juergen Gross
` (10 preceding siblings ...)
(?)
@ 2017-09-05 13:24 ` Juergen Gross
-1 siblings, 0 replies; 56+ messages in thread
From: Juergen Gross @ 2017-09-05 13:24 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, jeremy, peterz, chrisw, mingo, tglx, hpa, longman,
akataria, boris.ostrovsky
With the boot parameter "xen_nopvspin" specified a Xen guest should not
make use of paravirt spinlocks, but behave as if running on bare
metal. This is not true, however, as the qspinlock code will fall back
to a test-and-set scheme when it is detecting a hypervisor.
In order to avoid this set the virt_spin_lock pvops function to
_paravirt_false() in case xen_nopvspin has been specified.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/xen/spinlock.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 25a7c4302ce7..c01cedfa745d 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -129,6 +129,8 @@ void __init xen_init_spinlocks(void)
if (!xen_pvspin) {
printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
+ pv_lock_ops.virt_spin_lock =
+ __PV_IS_CALLEE_SAVE(_paravirt_false);
return;
}
printk(KERN_DEBUG "xen: PV spinlocks enabled\n");
--
2.12.3
^ permalink raw reply related [flat|nested] 56+ messages in thread