From: Pan Xinhui <hidden> Date: 2016-10-20 17:31:26
An over-committed guest with more vCPUs than pCPUs has a heavy overload in
osq_lock().
This is because vCPU A hold the osq lock and yield out, vCPU B wait per_cpu
node->locked to be set. IOW, vCPU B wait vCPU A to run and unlock the osq
lock.
Kernel has an interface bool vcpu_is_preempted(int cpu) to see if a vCPU is
currently running or not. So break the spin loops on true condition.
test case:
perf record -a perf bench sched messaging -g 400 -p && perf report
before patch:
18.09% sched-messaging [kernel.vmlinux] [k] osq_lock
12.28% sched-messaging [kernel.vmlinux] [k] rwsem_spin_on_owner
5.27% sched-messaging [kernel.vmlinux] [k] mutex_unlock
3.89% sched-messaging [kernel.vmlinux] [k] wait_consider_task
3.64% sched-messaging [kernel.vmlinux] [k] _raw_write_lock_irq
3.41% sched-messaging [kernel.vmlinux] [k] mutex_spin_on_owner.is
2.49% sched-messaging [kernel.vmlinux] [k] system_call
after patch:
20.68% sched-messaging [kernel.vmlinux] [k] mutex_spin_on_owner
8.45% sched-messaging [kernel.vmlinux] [k] mutex_unlock
4.12% sched-messaging [kernel.vmlinux] [k] system_call
3.01% sched-messaging [kernel.vmlinux] [k] system_call_common
2.83% sched-messaging [kernel.vmlinux] [k] copypage_power7
2.64% sched-messaging [kernel.vmlinux] [k] rwsem_spin_on_owner
2.00% sched-messaging [kernel.vmlinux] [k] osq_lock
Suggested-by: Boqun Feng <redacted>
Signed-off-by: Pan Xinhui <redacted>
Acked-by: Christian Borntraeger <redacted>
Tested-by: Juergen Gross <jgross@suse.com>
---
kernel/locking/osq_lock.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
From: Pan Xinhui <hidden> Date: 2016-10-20 17:31:27
This patch support to fix lock holder preemption issue.
For kernel users, we could use bool vcpu_is_preempted(int cpu) to detech if
one vcpu is preempted or not.
The default implementation is a macro defined by false. So compiler can
wrap it out if arch dose not support such vcpu pteempted check.
Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Pan Xinhui <redacted>
Acked-by: Christian Borntraeger <redacted>
Tested-by: Juergen Gross <jgross@suse.com>
---
include/linux/sched.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -362,8 +366,14 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)*/barrier();-/* abort spinning when need_resched or owner is not running */-if(!owner->on_cpu||need_resched()){+/*+*abortspinningwhenneed_reschedorownerisnotrunningor+*owner'scpuispreempted.vcpu_is_preemptedisamacro+*definedbyfalseifarchdoesnotsupportvcpupreempted+*check+*/+if(!owner->on_cpu||need_resched()||+vcpu_is_preempted(task_cpu(owner))){rcu_read_unlock();returnfalse;}
From: Pan Xinhui <hidden> Date: 2016-10-20 17:31:37
This is to fix some lock holder preemption issues. Some other locks
implementation do a spin loop before acquiring the lock itself.
Currently kernel has an interface of bool vcpu_is_preempted(int cpu). It
takes the cpu as parameter and return true if the cpu is preempted. Then
kernel can break the spin loops upon on the retval of vcpu_is_preempted.
As kernel has used this interface, So lets support it.
Only pSeries need support it. And the fact is powerNV are built into
same kernel image with pSeries. So we need return false if we are runnig
as powerNV. The another fact is that lppaca->yiled_count keeps zero on
powerNV. So we can just skip the machine type check.
Suggested-by: Boqun Feng <redacted>
Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Pan Xinhui <redacted>
---
arch/powerpc/include/asm/spinlock.h | 8 ++++++++
1 file changed, 8 insertions(+)
@@ -52,6 +52,14 @@#define SYNC_IO#endif+#ifdef CONFIG_PPC_PSERIES+#define vcpu_is_preempted vcpu_is_preempted+staticinlineboolvcpu_is_preempted(intcpu)+{+return!!(be32_to_cpu(lppaca_of(cpu).yield_count)&1);+}+#endif+#if defined(CONFIG_PPC_SPLPAR)/* We only yield to the hypervisor if we are in shared processor mode */#define SHARED_PROCESSOR (lppaca_shared_proc(local_paca->lppaca_ptr))
From: Pan Xinhui <hidden> Date: 2016-10-20 17:31:47
This is to fix some lock holder preemption issues. Some other locks
implementation do a spin loop before acquiring the lock itself.
Currently kernel has an interface of bool vcpu_is_preempted(int cpu). It
takes the cpu as parameter and return true if the cpu is preempted.
Then kernel can break the spin loops upon on the retval of
vcpu_is_preempted.
As kernel has used this interface, So lets support it.
To deal with kernel and kvm/xen, add vcpu_is_preempted into struct
pv_lock_ops.
Then kvm or xen could provide their own implementation to support
vcpu_is_preempted.
Signed-off-by: Pan Xinhui <redacted>
---
arch/x86/include/asm/paravirt_types.h | 2 ++
arch/x86/include/asm/spinlock.h | 8 ++++++++
arch/x86/kernel/paravirt-spinlocks.c | 6 ++++++
3 files changed, 16 insertions(+)
@@ -310,6 +310,8 @@ struct pv_lock_ops {void(*wait)(u8*ptr,u8val);void(*kick)(intcpu);++bool(*vcpu_is_preempted)(intcpu);};/* This contains all the paravirt structures: we get a convenient
From: Pan Xinhui <hidden> Date: 2016-10-20 17:32:13
Commit ("x86, kvm: support vcpu preempted check") add one field "__u8
preempted" into struct kvm_steal_time. This field tells if one vcpu is
running or not.
It is zero if 1) some old KVM deos not support this filed. 2) the vcpu is
preempted. Other values means the vcpu has been preempted.
Signed-off-by: Pan Xinhui <redacted>
---
Documentation/virtual/kvm/msr.txt | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
@@ -208,7 +208,8 @@ MSR_KVM_STEAL_TIME: 0x4b564d03 __u64 steal; __u32 version; __u32 flags;- __u32 pad[12];+ __u8 preempted;+ __u32 pad[11]; } whose data will be filled in by the hypervisor periodically. Only one
@@ -232,6 +233,11 @@ MSR_KVM_STEAL_TIME: 0x4b564d03 nanoseconds. Time during which the vcpu is idle, will not be reported as steal time.+ preempted: indicate the VCPU who owns this struct is running or+ not. Non-zero values mean the VCPU has been preempted. Zero+ means the VCPU is not preempted. NOTE, it is always zero if the+ the hypervisor doesn't support this field.+ MSR_KVM_EOI_EN: 0x4b564d04 data: Bit 0 is 1 when PV end of interrupt is enabled on the vcpu; 0 when disabled. Bit 1 is reserved and must be zero. When PV end of
From: Pan Xinhui <hidden> Date: 2016-10-20 17:32:16
From: Christian Borntraeger <redacted>
this implements the s390 backend for commit
"kernel/sched: introduce vcpu preempted check interface"
by reworking the existing smp_vcpu_scheduled into
arch_vcpu_is_preempted. We can then also get rid of the
local cpu_is_preempted function by moving the
CIF_ENABLED_WAIT test into arch_vcpu_is_preempted.
Signed-off-by: Christian Borntraeger <redacted>
Acked-by: Heiko Carstens <redacted>
---
arch/s390/include/asm/spinlock.h | 8 ++++++++
arch/s390/kernel/smp.c | 9 +++++++--
arch/s390/lib/spinlock.c | 25 ++++++++-----------------
3 files changed, 23 insertions(+), 19 deletions(-)
@@ -37,15 +37,6 @@ static inline void _raw_compare_and_delay(unsigned int *lock, unsigned int old)asm(".insn rsy,0xeb0000000022,%0,0,%1"::"d"(old),"Q"(*lock));}-staticinlineintcpu_is_preempted(intcpu)-{-if(test_cpu_flag_of(CIF_ENABLED_WAIT,cpu))-return0;-if(smp_vcpu_scheduled(cpu))-return0;-return1;-}-voidarch_spin_lock_wait(arch_spinlock_t*lp){unsignedintcpu=SPINLOCK_LOCKVAL;
@@ -62,7 +53,7 @@ void arch_spin_lock_wait(arch_spinlock_t *lp)continue;}/* First iteration: check if the lock owner is running. */-if(first_diag&&cpu_is_preempted(~owner)){+if(first_diag&&arch_vcpu_is_preempted(~owner)){smp_yield_cpu(~owner);first_diag=0;continue;
@@ -108,7 +99,7 @@ void arch_spin_lock_wait_flags(arch_spinlock_t *lp, unsigned long flags)continue;}/* Check if the lock owner is running. */-if(first_diag&&cpu_is_preempted(~owner)){+if(first_diag&&arch_vcpu_is_preempted(~owner)){smp_yield_cpu(~owner);first_diag=0;continue;
@@ -127,7 +118,7 @@ void arch_spin_lock_wait_flags(arch_spinlock_t *lp, unsigned long flags)*yieldtheCPUunconditionally.ForLPARrelyonthe*senserunningstatus.*/-if(!MACHINE_IS_LPAR||cpu_is_preempted(~owner)){+if(!MACHINE_IS_LPAR||arch_vcpu_is_preempted(~owner)){smp_yield_cpu(~owner);first_diag=0;}
From: Pan Xinhui <hidden> Date: 2016-10-20 17:32:19
From: Juergen Gross <jgross@suse.com>
Support the vcpu_is_preempted() functionality under Xen. This will
enhance lock performance on overcommitted hosts (more runnable vcpus
than physical cpus in the system) as doing busy waits for preempted
vcpus will hurt system performance far worse than early yielding.
A quick test (4 vcpus on 1 physical cpu doing a parallel build job
with "make -j 8") reduced system time by about 5% with this patch.
Signed-off-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Pan Xinhui <redacted>
---
arch/x86/xen/spinlock.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -2057,6 +2057,8 @@ static void record_steal_time(struct kvm_vcpu *vcpu)&vcpu->arch.st.steal,sizeof(structkvm_steal_time))))return;+vcpu->arch.st.steal.preempted=0;+if(vcpu->arch.st.steal.version&1)vcpu->arch.st.steal.version+=1;/* first time write, random junk */
@@ -2810,8 +2812,24 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)kvm_make_request(KVM_REQ_STEAL_UPDATE,vcpu);}+staticvoidkvm_steal_time_set_preempted(structkvm_vcpu*vcpu)+{+if(!(vcpu->arch.st.msr_val&KVM_MSR_ENABLED))+return;++if(unlikely(kvm_read_guest_cached(vcpu->kvm,&vcpu->arch.st.stime,+&vcpu->arch.st.steal,sizeof(structkvm_steal_time))))+return;++vcpu->arch.st.steal.preempted=1;++kvm_write_guest_cached(vcpu->kvm,&vcpu->arch.st.stime,+&vcpu->arch.st.steal,sizeof(structkvm_steal_time));+}+voidkvm_arch_vcpu_put(structkvm_vcpu*vcpu){+kvm_steal_time_set_preempted(vcpu);kvm_x86_ops->vcpu_put(vcpu);kvm_put_guest_fpu(vcpu);vcpu->arch.last_host_tsc=rdtsc();
On Thu, Oct 20, 2016 at 05:27:54PM -0400, Pan Xinhui wrote:
Commit ("x86, kvm: support vcpu preempted check") add one field "__u8
preempted" into struct kvm_steal_time. This field tells if one vcpu is
running or not.
It is zero if 1) some old KVM deos not support this filed. 2) the vcpu is
preempted. Other values means the vcpu has been preempted.
^^^^^^^^^
s/preempted/not preempted
And better to fix other typos in the commit log ;-)
Maybe you can try aspell? That works for me.
Regards,
Boqun
@@ -208,7 +208,8 @@ MSR_KVM_STEAL_TIME: 0x4b564d03 __u64 steal; __u32 version; __u32 flags;- __u32 pad[12];+ __u8 preempted;+ __u32 pad[11]; } whose data will be filled in by the hypervisor periodically. Only one
@@ -232,6 +233,11 @@ MSR_KVM_STEAL_TIME: 0x4b564d03 nanoseconds. Time during which the vcpu is idle, will not be reported as steal time.+ preempted: indicate the VCPU who owns this struct is running or+ not. Non-zero values mean the VCPU has been preempted. Zero+ means the VCPU is not preempted. NOTE, it is always zero if the+ the hypervisor doesn't support this field.+ MSR_KVM_EOI_EN: 0x4b564d04 data: Bit 0 is 1 when PV end of interrupt is enabled on the vcpu; 0 when disabled. Bit 1 is reserved and must be zero. When PV end of
From: Pan Xinhui <hidden> Date: 2016-10-21 01:42:36
在 2016/10/21 09:23, Boqun Feng 写道:
On Thu, Oct 20, 2016 at 05:27:54PM -0400, Pan Xinhui wrote:
quoted
Commit ("x86, kvm: support vcpu preempted check") add one field "__u8
preempted" into struct kvm_steal_time. This field tells if one vcpu is
running or not.
It is zero if 1) some old KVM deos not support this filed. 2) the vcpu is
preempted. Other values means the vcpu has been preempted.
^^^^^^^^^
s/preempted/not preempted
yes. the less of *not* definitely sould be avoided..
And better to fix other typos in the commit log ;-)
Maybe you can try aspell? That works for me.
@@ -208,7 +208,8 @@ MSR_KVM_STEAL_TIME: 0x4b564d03 __u64 steal; __u32 version; __u32 flags;- __u32 pad[12];+ __u8 preempted;+ __u32 pad[11]; } whose data will be filled in by the hypervisor periodically. Only one
@@ -232,6 +233,11 @@ MSR_KVM_STEAL_TIME: 0x4b564d03 nanoseconds. Time during which the vcpu is idle, will not be reported as steal time.+ preempted: indicate the VCPU who owns this struct is running or+ not. Non-zero values mean the VCPU has been preempted. Zero+ means the VCPU is not preempted. NOTE, it is always zero if the+ the hypervisor doesn't support this field.+ MSR_KVM_EOI_EN: 0x4b564d04 data: Bit 0 is 1 when PV end of interrupt is enabled on the vcpu; 0 when disabled. Bit 1 is reserved and must be zero. When PV end of--
Corrected xen-devel mailing list address, added other Xen maintainers
On 20/10/16 23:27, Pan Xinhui wrote:
quoted hunk
From: Juergen Gross <jgross@suse.com>
Support the vcpu_is_preempted() functionality under Xen. This will
enhance lock performance on overcommitted hosts (more runnable vcpus
than physical cpus in the system) as doing busy waits for preempted
vcpus will hurt system performance far worse than early yielding.
A quick test (4 vcpus on 1 physical cpu doing a parallel build job
with "make -j 8") reduced system time by about 5% with this patch.
Signed-off-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Pan Xinhui <redacted>
---
arch/x86/xen/spinlock.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: David Laight <hidden> Date: 2016-10-21 11:30:37
From: Pan Xinhui
quoted hunk
Sent: 20 October 2016 22:28
Commit ("x86, kvm: support vcpu preempted check") add one field "__u8
preempted" into struct kvm_steal_time. This field tells if one vcpu is
running or not.
=20
It is zero if 1) some old KVM deos not support this filed. 2) the vcpu is
preempted. Other values means the vcpu has been preempted.
=20
Signed-off-by: Pan Xinhui <redacted>
---
Documentation/virtual/kvm/msr.txt | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
=20
Sent: 20 October 2016 22:28
Commit ("x86, kvm: support vcpu preempted check") add one field "__u8
preempted" into struct kvm_steal_time. This field tells if one vcpu is
running or not.
It is zero if 1) some old KVM deos not support this filed. 2) the vcpu is
preempted. Other values means the vcpu has been preempted.
Signed-off-by: Pan Xinhui <redacted>
---
Documentation/virtual/kvm/msr.txt | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
Sent: 20 October 2016 22:28
Commit ("x86, kvm: support vcpu preempted check") add one field "__u8
preempted" into struct kvm_steal_time. This field tells if one vcpu is
running or not.
It is zero if 1) some old KVM deos not support this filed. 2) the vcpu is
preempted. Other values means the vcpu has been preempted.
Signed-off-by: Pan Xinhui <redacted>
---
Documentation/virtual/kvm/msr.txt | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
Sent: 20 October 2016 22:28
Commit ("x86, kvm: support vcpu preempted check") add one field "__u8
preempted" into struct kvm_steal_time. This field tells if one vcpu is
running or not.
It is zero if 1) some old KVM deos not support this filed. 2) the vcpu is
preempted. Other values means the vcpu has been preempted.
Signed-off-by: Pan Xinhui <redacted>
---
Documentation/virtual/kvm/msr.txt | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
@@ -2057,6 +2057,8 @@ static void record_steal_time(struct kvm_vcpu *vcpu)&vcpu->arch.st.steal,sizeof(structkvm_steal_time))))return;+vcpu->arch.st.steal.preempted=0;+if(vcpu->arch.st.steal.version&1)vcpu->arch.st.steal.version+=1;/* first time write, random junk */
@@ -2810,8 +2812,24 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)kvm_make_request(KVM_REQ_STEAL_UPDATE,vcpu);}+staticvoidkvm_steal_time_set_preempted(structkvm_vcpu*vcpu)+{+if(!(vcpu->arch.st.msr_val&KVM_MSR_ENABLED))+return;++if(unlikely(kvm_read_guest_cached(vcpu->kvm,&vcpu->arch.st.stime,+&vcpu->arch.st.steal,sizeof(structkvm_steal_time))))+return;++vcpu->arch.st.steal.preempted=1;++kvm_write_guest_cached(vcpu->kvm,&vcpu->arch.st.stime,+&vcpu->arch.st.steal,sizeof(structkvm_steal_time));+}+voidkvm_arch_vcpu_put(structkvm_vcpu*vcpu){+kvm_steal_time_set_preempted(vcpu);kvm_x86_ops->vcpu_put(vcpu);kvm_put_guest_fpu(vcpu);vcpu->arch.last_host_tsc=rdtsc();
This is new version for [PATCH v6 9/9] Documentation: virtual: kvm: Support vcpu preempted check
change:
an explicit pad[3] after __u8 preempted.
a typo fix in the commit log.
From defac64d7c6a50d5f18ef64a7c776af3e21e8b68 Mon Sep 17 00:00:00 2001
From: Pan Xinhui <redacted>
Date: Thu, 20 Oct 2016 09:33:36 -0400
Subject: [PATCH v6 9/9] Documentation: virtual: kvm: Support vcpu preempted check
Commit ("x86, kvm: support vcpu preempted check") add one field "__u8
preempted" into struct kvm_steal_time. This field tells if one vcpu is
running or not.
It is zero if 1) some old KVM deos not support this filed. 2) the vcpu is
not preempted. Other values mean the vcpu has been preempted.
Signed-off-by: Pan Xinhui <redacted>
Acked-by: Radim Krčmář <redacted>
---
Documentation/virtual/kvm/msr.txt | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
@@ -208,7 +208,9 @@ MSR_KVM_STEAL_TIME: 0x4b564d03 __u64 steal; __u32 version; __u32 flags;- __u32 pad[12];+ __u8 preempted;+ __u8 u8_pad[3];+ __u32 pad[11]; } whose data will be filled in by the hypervisor periodically. Only one
@@ -232,6 +234,11 @@ MSR_KVM_STEAL_TIME: 0x4b564d03 nanoseconds. Time during which the vcpu is idle, will not be reported as steal time.+ preempted: indicate the VCPU who owns this struct is running or+ not. Non-zero values mean the VCPU has been preempted. Zero+ means the VCPU is not preempted. NOTE, it is always zero if the+ the hypervisor doesn't support this field.+ MSR_KVM_EOI_EN: 0x4b564d04 data: Bit 0 is 1 when PV end of interrupt is enabled on the vcpu; 0 when disabled. Bit 1 is reserved and must be zero. When PV end of
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2016-10-24 14:42:56
On 21/10/2016 20:39, rkrcmar@redhat.com wrote:
2016-10-21 11:27+0000, David Laight:
quoted
From: Pan Xinhui
quoted
Sent: 20 October 2016 22:28
Commit ("x86, kvm: support vcpu preempted check") add one field "__u8
preempted" into struct kvm_steal_time. This field tells if one vcpu is
running or not.
It is zero if 1) some old KVM deos not support this filed. 2) the vcpu is
preempted. Other values means the vcpu has been preempted.
Signed-off-by: Pan Xinhui <redacted>
---
Documentation/virtual/kvm/msr.txt | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
I think I'd be explicit about the 3 pad bytes you've left.
Seconded.
With that change are all KVM bits
Acked-by: Radim Krčmář <redacted>
Saw this after replying to the previous message. If you need to post v6
of the full series, it would be nice if you removed the
kvm_read_guest_cached. But anyway it wasn't my intention to override Radim.
Paolo
Sent: 20 October 2016 22:28
Commit ("x86, kvm: support vcpu preempted check") add one field "__u8
preempted" into struct kvm_steal_time. This field tells if one vcpu is
running or not.
It is zero if 1) some old KVM deos not support this filed. 2) the vcpu is
preempted. Other values means the vcpu has been preempted.
Signed-off-by: Pan Xinhui <redacted>
---
Documentation/virtual/kvm/msr.txt | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
I think I'd be explicit about the 3 pad bytes you've left.
Seconded.
With that change are all KVM bits
Acked-by: Radim Krčmář <redacted>
Saw this after replying to the previous message. If you need to post v6
of the full series, it would be nice if you removed the
kvm_read_guest_cached. But anyway it wasn't my intention to override Radim.
The patch was acceptable to me even now, so I definitely wouldn't mind
if it were even nicer. :)