From: Pan Xinhui <hidden> Date: 2016-11-02 05:13:36
This patch support to fix lock holder preemption issue.
For kernel users, we could use bool vcpu_is_preempted(int cpu) to detect
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 preempted check.
Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Pan Xinhui <redacted>
Acked-by: Christian Borntraeger <redacted>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Juergen Gross <jgross@suse.com>
---
include/linux/sched.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
From: Pan Xinhui <hidden> Date: 2016-11-02 05:13:41
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 detect 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>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Juergen Gross <jgross@suse.com>
---
kernel/locking/osq_lock.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
@@ -362,8 +366,12 @@ 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.+*/+if(!owner->on_cpu||need_resched()||+vcpu_is_preempted(task_cpu(owner))){rcu_read_unlock();returnfalse;}
From: Pan Xinhui <hidden> Date: 2016-11-02 05:13:54
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 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 is 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(+)
From: Pan Xinhui <hidden> Date: 2016-11-02 05:13:59
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-11-02 05:14:04
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 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>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
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-11-02 05:14:10
It allows us to update some status or field of one struct partially.
We can also save one kvm_read_guest_cached if we just update one filed
of the struct regardless of its current value.
Signed-off-by: Pan Xinhui <redacted>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/linux/kvm_host.h | 2 ++
virt/kvm/kvm_main.c | 20 ++++++++++++++------
2 files changed, 16 insertions(+), 6 deletions(-)
From: Pan Xinhui <hidden> Date: 2016-11-02 05:14:17
Support the vcpu_is_preempted() functionality under KVM. 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.
Use one field of struct kvm_steal_time ::preempted to indicate that if
one vcpu is running or not.
Signed-off-by: Pan Xinhui <redacted>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
arch/x86/include/uapi/asm/kvm_para.h | 4 +++-
arch/x86/kvm/x86.c | 16 ++++++++++++++++
2 files changed, 19 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,22 @@ 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;++vcpu->arch.st.steal.preempted=1;++kvm_write_guest_offset_cached(vcpu->kvm,&vcpu->arch.st.stime,+&vcpu->arch.st.steal.preempted,+offsetof(structkvm_steal_time,preempted),+sizeof(vcpu->arch.st.steal.preempted));+}+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();
From: Pan Xinhui <hidden> Date: 2016-11-02 05:14:34
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 means the vcpu has been preempted.
Signed-off-by: Pan Xinhui <redacted>
Acked-by: Radim Krčmář <redacted>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
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: Pan Xinhui <hidden> Date: 2016-11-02 05:14:56
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(-)
So that ends up with a full function call in the native case. I did
something like the below on top, completely untested, not been near a
compiler etc..
It doesn't get rid of the branch, but at least it avoids the function
call, and hardware should have no trouble predicting a constant
condition.
Also, it looks like you end up not setting vcpu_is_preempted when KVM
doesn't support steal clock, which would end up in an instant NULL
deref. Fixed that too.
---
@@ -309,7 +309,7 @@ struct pv_lock_ops {void(*wait)(u8*ptr,u8val);void(*kick)(intcpu);-bool(*vcpu_is_preempted)(intcpu);+structparavirt_callee_savevcpu_is_preempted;};/* This contains all the paravirt structures: we get a convenient---a/arch/x86/include/asm/qspinlock.h+++b/arch/x86/include/asm/qspinlock.h
So that ends up with a full function call in the native case. I did
something like the below on top, completely untested, not been near a
compiler etc..
Hi, Peter.
I think we can avoid a function call in a simpler way. How about below
static inline bool vcpu_is_preempted(int cpu)
{
/* only set in pv case*/
if (pv_lock_ops.vcpu_is_preempted)
return pv_lock_ops.vcpu_is_preempted(cpu);
return false;
}
It doesn't get rid of the branch, but at least it avoids the function
call, and hardware should have no trouble predicting a constant
condition.
Also, it looks like you end up not setting vcpu_is_preempted when KVM
doesn't support steal clock, which would end up in an instant NULL
deref. Fixed that too.
maybe not true. There is .vcpu_is_preempted = native_vcpu_is_preempted when we define pv_lock_ops.
your patch is a good example for any people who want to add any native/pv function. :)
thanks
xinhui
@@ -309,7 +309,7 @@ struct pv_lock_ops {void(*wait)(u8*ptr,u8val);void(*kick)(intcpu);-bool(*vcpu_is_preempted)(intcpu);+structparavirt_callee_savevcpu_is_preempted;};/* This contains all the paravirt structures: we get a convenient---a/arch/x86/include/asm/qspinlock.h+++b/arch/x86/include/asm/qspinlock.h
@@ -66,6 +68,12 @@ unsigned native_patch(u8 type, u16 clobb end = end_pv_lock_ops_queued_spin_unlock; goto patch_site; }+ 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;+ } #endif default:--- a/arch/x86/xen/spinlock.c+++ b/arch/x86/xen/spinlock.c
@@ -114,6 +114,8 @@ void xen_uninit_lock_cpu(int cpu) per_cpu(irq_name, cpu) = NULL; }+PV_CALLEE_SAVE_REGS_THUNK(xen_vcpu_stolen);+ /* * Our init of PV spinlocks is split in two init functions due to us * using paravirt patching and jump labels patching and having to do
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-11-16 10:24:32
On Wed, Nov 16, 2016 at 12:19:09PM +0800, Pan Xinhui wrote:
Hi, Peter.
I think we can avoid a function call in a simpler way. How about below
static inline bool vcpu_is_preempted(int cpu)
{
/* only set in pv case*/
if (pv_lock_ops.vcpu_is_preempted)
return pv_lock_ops.vcpu_is_preempted(cpu);
return false;
}
That is still more expensive. It needs to do an actual load and makes it
hard to predict the branch, you'd have to actually wait for the load to
complete etc.
Also, it generates more code.
Paravirt muck should strive to be as cheap as possible when ran on
native hardware.
From: Christian Borntraeger <hidden> Date: 2016-11-16 11:29:58
On 11/16/2016 11:23 AM, Peter Zijlstra wrote:
On Wed, Nov 16, 2016 at 12:19:09PM +0800, Pan Xinhui wrote:
quoted
Hi, Peter.
I think we can avoid a function call in a simpler way. How about below
static inline bool vcpu_is_preempted(int cpu)
{
/* only set in pv case*/
if (pv_lock_ops.vcpu_is_preempted)
return pv_lock_ops.vcpu_is_preempted(cpu);
return false;
}
That is still more expensive. It needs to do an actual load and makes it
hard to predict the branch, you'd have to actually wait for the load to
complete etc.
Out of curiosity, why is that hard to predict?
On s390 the branch prediction runs asynchronously ahead of the downstream
pipeline (e.g. search for "IBM z Systems Processor Optimization Primer" page 11).
given enough capacity, I would assume that modern x86 processors would do the same
and be able to predict this is as soon as it becomes hot (and otherwise you would
not notice the branch miss anyway). Is x86 behaving differently here?
Also, it generates more code.
Paravirt muck should strive to be as cheap as possible when ran on
native hardware.
As I am interested in this series from the s390 point of view, this is
the only thing that block this series?
Is there a chance to add a static key around the paravirt ops somehow?
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-11-16 11:43:48
On Wed, Nov 16, 2016 at 12:29:44PM +0100, Christian Borntraeger wrote:
On 11/16/2016 11:23 AM, Peter Zijlstra wrote:
quoted
On Wed, Nov 16, 2016 at 12:19:09PM +0800, Pan Xinhui wrote:
quoted
Hi, Peter.
I think we can avoid a function call in a simpler way. How about below
static inline bool vcpu_is_preempted(int cpu)
{
/* only set in pv case*/
if (pv_lock_ops.vcpu_is_preempted)
return pv_lock_ops.vcpu_is_preempted(cpu);
return false;
}
That is still more expensive. It needs to do an actual load and makes it
hard to predict the branch, you'd have to actually wait for the load to
complete etc.
Out of curiosity, why is that hard to predict?
On s390 the branch prediction runs asynchronously ahead of the downstream
pipeline (e.g. search for "IBM z Systems Processor Optimization Primer" page 11).
given enough capacity, I would assume that modern x86 processors would do the same
and be able to predict this is as soon as it becomes hot (and otherwise you would
not notice the branch miss anyway). Is x86 behaving differently here?
Not sure how exactly it works, but it seems to me that an immediate
assignment to the value you're going to compare would leave very little
doubt.
Then again, maybe cores aren't that smart and only look at the
hysterical btb for prediction.
quoted
Also, it generates more code.
Paravirt muck should strive to be as cheap as possible when ran on
native hardware.
As I am interested in this series from the s390 point of view, this is
the only thing that block this series?
Ingo was rewriting the changelog, other than that, no, I can do this on
top. Just spotted this because Ingo and me talked it over.
Is there a chance to add a static key around the paravirt ops somehow?
More code generation still, replacing the call with an immediate
assignment to the return register is the shortest possible option I
think.
From: Pan Xinhui <hidden> Date: 2016-11-17 05:16:50
在 2016/11/16 18:23, Peter Zijlstra 写道:
On Wed, Nov 16, 2016 at 12:19:09PM +0800, Pan Xinhui wrote:
quoted
Hi, Peter.
I think we can avoid a function call in a simpler way. How about below
static inline bool vcpu_is_preempted(int cpu)
{
/* only set in pv case*/
if (pv_lock_ops.vcpu_is_preempted)
return pv_lock_ops.vcpu_is_preempted(cpu);
return false;
}
That is still more expensive. It needs to do an actual load and makes it
hard to predict the branch, you'd have to actually wait for the load to
complete etc.
yes, one more load in native case. I think this is acceptable as vcpu_is_preempted is not a critical function.
however if we use pv_callee_save_regs_thunk, more unnecessary registers might be save/resotred in pv case.
that will introduce a little overhead.
but I think I am okay with your idea. I can make another patch based on this patchset with your suggested-by.
thanks
xinhui
Also, it generates more code.
Paravirt muck should strive to be as cheap as possible when ran on
native hardware.