From: Waiman Long <longman@redhat.com> Date: 2017-02-10 15:56:09
It was found when running fio sequential write test with a XFS ramdisk
on a VM running on a 2-socket x86-64 system, the %CPU times as reported
by perf were as follows:
69.75% 0.59% fio [k] down_write
69.15% 0.01% fio [k] call_rwsem_down_write_failed
67.12% 1.12% fio [k] rwsem_down_write_failed
63.48% 52.77% fio [k] osq_lock
9.46% 7.88% fio [k] __raw_callee_save___kvm_vcpu_is_preempt
3.93% 3.93% fio [k] __kvm_vcpu_is_preempted
Making vcpu_is_preempted() a callee-save function has a relatively
high cost on x86-64 primarily due to at least one more cacheline of
data access from the saving and restoring of registers (8 of them)
to and from stack as well as one more level of function call. As
vcpu_is_preempted() is called within the spinlock, mutex and rwsem
slowpaths, there isn't much to gain by making it callee-save. So it
is now changed to a normal function call instead.
With this patch applied on both bare-metal & KVM guest on a 2-socekt
16-core 32-thread system with 16 parallel jobs (8 on each socket), the
aggregrate bandwidth of the fio test on an XFS ramdisk were as follows:
Bare Metal KVM Guest
I/O Type w/o patch with patch w/o patch with patch
-------- --------- ---------- --------- ----------
random read 8650.5 MB/s 8560.9 MB/s 7602.9 MB/s 8196.1 MB/s
seq read 9104.8 MB/s 9397.2 MB/s 8293.7 MB/s 8566.9 MB/s
random write 1623.8 MB/s 1626.7 MB/s 1590.6 MB/s 1700.7 MB/s
seq write 1626.4 MB/s 1624.9 MB/s 1604.8 MB/s 1726.3 MB/s
The perf data (on KVM guest) now became:
70.78% 0.58% fio [k] down_write
70.20% 0.01% fio [k] call_rwsem_down_write_failed
69.70% 1.17% fio [k] rwsem_down_write_failed
59.91% 55.42% fio [k] osq_lock
10.14% 10.14% fio [k] __kvm_vcpu_is_preempted
On bare metal, the patch doesn't introduce any performance
regression. On KVM guest, it produces noticeable performance
improvement (up to 7%).
Signed-off-by: Waiman Long <longman@redhat.com>
---
v1->v2:
- Rerun the fio test on a different system on both bare-metal and a
KVM guest. Both sockets were utilized in this test.
- The commit log was updated with new performance numbers, but the
patch wasn't changed.
- Drop patch 2.
arch/x86/include/asm/paravirt.h | 2 +-
arch/x86/include/asm/paravirt_types.h | 2 +-
arch/x86/kernel/kvm.c | 7 ++-----
arch/x86/kernel/paravirt-spinlocks.c | 6 ++----
arch/x86/xen/spinlock.c | 4 +---
5 files changed, 7 insertions(+), 14 deletions(-)
@@ -309,7 +309,7 @@ struct pv_lock_ops {void(*wait)(u8*ptr,u8val);void(*kick)(intcpu);-structparavirt_callee_savevcpu_is_preempted;+bool(*vcpu_is_preempted)(intcpu);};/* This contains all the paravirt structures: we get a convenient
From: Peter Zijlstra <peterz@infradead.org> Date: 2017-02-10 16:20:40
On Fri, Feb 10, 2017 at 10:43:09AM -0500, Waiman Long wrote:
It was found when running fio sequential write test with a XFS ramdisk
on a VM running on a 2-socket x86-64 system, the %CPU times as reported
by perf were as follows:
69.75% 0.59% fio [k] down_write
69.15% 0.01% fio [k] call_rwsem_down_write_failed
67.12% 1.12% fio [k] rwsem_down_write_failed
63.48% 52.77% fio [k] osq_lock
9.46% 7.88% fio [k] __raw_callee_save___kvm_vcpu_is_preempt
3.93% 3.93% fio [k] __kvm_vcpu_is_preempted
Thinking about this again, wouldn't something like the below also work?
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2017-02-10 16:23:05
On 10/02/2017 16:43, Waiman Long wrote:
quoted hunk
It was found when running fio sequential write test with a XFS ramdisk
on a VM running on a 2-socket x86-64 system, the %CPU times as reported
by perf were as follows:
69.75% 0.59% fio [k] down_write
69.15% 0.01% fio [k] call_rwsem_down_write_failed
67.12% 1.12% fio [k] rwsem_down_write_failed
63.48% 52.77% fio [k] osq_lock
9.46% 7.88% fio [k] __raw_callee_save___kvm_vcpu_is_preempt
3.93% 3.93% fio [k] __kvm_vcpu_is_preempted
Making vcpu_is_preempted() a callee-save function has a relatively
high cost on x86-64 primarily due to at least one more cacheline of
data access from the saving and restoring of registers (8 of them)
to and from stack as well as one more level of function call. As
vcpu_is_preempted() is called within the spinlock, mutex and rwsem
slowpaths, there isn't much to gain by making it callee-save. So it
is now changed to a normal function call instead.
With this patch applied on both bare-metal & KVM guest on a 2-socekt
16-core 32-thread system with 16 parallel jobs (8 on each socket), the
aggregrate bandwidth of the fio test on an XFS ramdisk were as follows:
Bare Metal KVM Guest
I/O Type w/o patch with patch w/o patch with patch
-------- --------- ---------- --------- ----------
random read 8650.5 MB/s 8560.9 MB/s 7602.9 MB/s 8196.1 MB/s
seq read 9104.8 MB/s 9397.2 MB/s 8293.7 MB/s 8566.9 MB/s
random write 1623.8 MB/s 1626.7 MB/s 1590.6 MB/s 1700.7 MB/s
seq write 1626.4 MB/s 1624.9 MB/s 1604.8 MB/s 1726.3 MB/s
The perf data (on KVM guest) now became:
70.78% 0.58% fio [k] down_write
70.20% 0.01% fio [k] call_rwsem_down_write_failed
69.70% 1.17% fio [k] rwsem_down_write_failed
59.91% 55.42% fio [k] osq_lock
10.14% 10.14% fio [k] __kvm_vcpu_is_preempted
On bare metal, the patch doesn't introduce any performance
regression. On KVM guest, it produces noticeable performance
improvement (up to 7%).
Signed-off-by: Waiman Long <longman@redhat.com>
---
v1->v2:
- Rerun the fio test on a different system on both bare-metal and a
KVM guest. Both sockets were utilized in this test.
- The commit log was updated with new performance numbers, but the
patch wasn't changed.
- Drop patch 2.
arch/x86/include/asm/paravirt.h | 2 +-
arch/x86/include/asm/paravirt_types.h | 2 +-
arch/x86/kernel/kvm.c | 7 ++-----
arch/x86/kernel/paravirt-spinlocks.c | 6 ++----
arch/x86/xen/spinlock.c | 4 +---
5 files changed, 7 insertions(+), 14 deletions(-)
@@ -309,7 +309,7 @@ struct pv_lock_ops {void(*wait)(u8*ptr,u8val);void(*kick)(intcpu);-structparavirt_callee_savevcpu_is_preempted;+bool(*vcpu_is_preempted)(intcpu);};/* This contains all the paravirt structures: we get a convenient
From: Waiman Long <longman@redhat.com> Date: 2017-02-10 17:00:49
On 02/10/2017 11:35 AM, Waiman Long wrote:
On 02/10/2017 11:19 AM, Peter Zijlstra wrote:
quoted
On Fri, Feb 10, 2017 at 10:43:09AM -0500, Waiman Long wrote:
quoted
It was found when running fio sequential write test with a XFS ramdisk
on a VM running on a 2-socket x86-64 system, the %CPU times as reported
by perf were as follows:
69.75% 0.59% fio [k] down_write
69.15% 0.01% fio [k] call_rwsem_down_write_failed
67.12% 1.12% fio [k] rwsem_down_write_failed
63.48% 52.77% fio [k] osq_lock
9.46% 7.88% fio [k] __raw_callee_save___kvm_vcpu_is_preempt
3.93% 3.93% fio [k] __kvm_vcpu_is_preempted
Thinking about this again, wouldn't something like the below also work?
That should work for now. I have done something similar for
__pv_queued_spin_unlock. However, this has the problem of creating a
dependency on the exact layout of the steal_time structure. Maybe the
constant 16 can be passed in as a parameter offsetof(struct
kvm_steal_time, preempted) to the asm call.
Cheers,
Longman
One more thing, that will improve KVM performance, but it won't help Xen.
I looked into the assembly code for rwsem_spin_on_owner, It need to save
and restore 2 additional registers with my patch. Doing it your way,
will transfer the save and restore overhead to the assembly code.
However, __kvm_vcpu_is_preempted() is called multiple times per
invocation of rwsem_spin_on_owner. That function is simple enough that
making __kvm_vcpu_is_preempted() callee-save won't produce much compiler
optimization opportunity. The outer function rwsem_down_write_failed()
does appear to be a bit bigger (from 866 bytes to 884 bytes) though.
Cheers,
Longman
From: Waiman Long <longman@redhat.com> Date: 2017-02-10 18:06:51
On 02/10/2017 11:19 AM, Peter Zijlstra wrote:
quoted hunk
On Fri, Feb 10, 2017 at 10:43:09AM -0500, Waiman Long wrote:
quoted
It was found when running fio sequential write test with a XFS ramdisk
on a VM running on a 2-socket x86-64 system, the %CPU times as reported
by perf were as follows:
69.75% 0.59% fio [k] down_write
69.15% 0.01% fio [k] call_rwsem_down_write_failed
67.12% 1.12% fio [k] rwsem_down_write_failed
63.48% 52.77% fio [k] osq_lock
9.46% 7.88% fio [k] __raw_callee_save___kvm_vcpu_is_preempt
3.93% 3.93% fio [k] __kvm_vcpu_is_preempted
Thinking about this again, wouldn't something like the below also work?
That should work for now. I have done something similar for
__pv_queued_spin_unlock. However, this has the problem of creating a
dependency on the exact layout of the steal_time structure. Maybe the
constant 16 can be passed in as a parameter offsetof(struct
kvm_steal_time, preempted) to the asm call.
Cheers,
Longman
Could we not put the $steal_time+16 displacement as an immediate in the
cmpb and save a whole register here?
That way we'd end up with something like:
asm("
push %rdi;
movslq %edi, %rdi;
movq __per_cpu_offset(,%rdi,8), %rax;
cmpb $0, %[offset](%rax);
setne %al;
pop %rdi;
" : : [offset] "i" (((unsigned long)&steal_time) + offsetof(struct steal_time, preempted)));
And if we could get rid of the sign extend on edi we could avoid all the
push-pop nonsense, but I'm not sure I see how to do that (then again,
this asm foo isn't my strongest point).
That should work for now. I have done something similar for
__pv_queued_spin_unlock. However, this has the problem of creating a
dependency on the exact layout of the steal_time structure. Maybe the
constant 16 can be passed in as a parameter offsetof(struct
kvm_steal_time, preempted) to the asm call.
Yeah it should be well possible to pass that in. But ideally we'd have
GCC grow something like __attribute__((callee_saved)) or somesuch and it
would do all this for us.
One more thing, that will improve KVM performance, but it won't help Xen.
People still use Xen? ;-) In any case, their implementation looks very
similar and could easily crib this.
I looked into the assembly code for rwsem_spin_on_owner, It need to save
and restore 2 additional registers with my patch. Doing it your way,
will transfer the save and restore overhead to the assembly code.
However, __kvm_vcpu_is_preempted() is called multiple times per
invocation of rwsem_spin_on_owner. That function is simple enough that
making __kvm_vcpu_is_preempted() callee-save won't produce much compiler
optimization opportunity.
This is because of that noinline, right? Otherwise it would've been
folded and register pressure would be much higher.
The outer function rwsem_down_write_failed()
does appear to be a bit bigger (from 866 bytes to 884 bytes) though.
I suspect GCC is being clever and since all this is static it plays
games with the calling convention and pushes these clobbers out.
From: Peter Zijlstra <peterz@infradead.org> Date: 2017-02-13 10:54:09
On Mon, Feb 13, 2017 at 11:47:16AM +0100, Peter Zijlstra wrote:
That way we'd end up with something like:
asm("
push %rdi;
movslq %edi, %rdi;
movq __per_cpu_offset(,%rdi,8), %rax;
cmpb $0, %[offset](%rax);
setne %al;
pop %rdi;
" : : [offset] "i" (((unsigned long)&steal_time) + offsetof(struct steal_time, preempted)));
And if we could get rid of the sign extend on edi we could avoid all the
push-pop nonsense, but I'm not sure I see how to do that (then again,
this asm foo isn't my strongest point).
Could we not put the $steal_time+16 displacement as an immediate in the
cmpb and save a whole register here?
That way we'd end up with something like:
asm("
push %rdi;
movslq %edi, %rdi;
movq __per_cpu_offset(,%rdi,8), %rax;
cmpb $0, %[offset](%rax);
setne %al;
pop %rdi;
" : : [offset] "i" (((unsigned long)&steal_time) + offsetof(struct steal_time, preempted)));
And if we could get rid of the sign extend on edi we could avoid all the
push-pop nonsense, but I'm not sure I see how to do that (then again,
this asm foo isn't my strongest point).
Yes, I think that can work. I will try to ran this patch to see how
thing goes.
That should work for now. I have done something similar for
__pv_queued_spin_unlock. However, this has the problem of creating a
dependency on the exact layout of the steal_time structure. Maybe the
constant 16 can be passed in as a parameter offsetof(struct
kvm_steal_time, preempted) to the asm call.
Yeah it should be well possible to pass that in. But ideally we'd have
GCC grow something like __attribute__((callee_saved)) or somesuch and it
would do all this for us.
That will be really nice too. I am not too fond of working in assembly.
quoted
One more thing, that will improve KVM performance, but it won't help Xen.
People still use Xen? ;-) In any case, their implementation looks very
similar and could easily crib this.
In Red Hat, my focus will be on KVM performance. I do believe that there
are still Xen users out there. So we still need to keep their interest
into consideration. Given that, I am OK to make it work better in KVM
first and then think about Xen later.
quoted
I looked into the assembly code for rwsem_spin_on_owner, It need to save
and restore 2 additional registers with my patch. Doing it your way,
will transfer the save and restore overhead to the assembly code.
However, __kvm_vcpu_is_preempted() is called multiple times per
invocation of rwsem_spin_on_owner. That function is simple enough that
making __kvm_vcpu_is_preempted() callee-save won't produce much compiler
optimization opportunity.
This is because of that noinline, right? Otherwise it would've been
folded and register pressure would be much higher.
Yes, I guess so. The noinline is there so that we know what the CPU time
is for spinning rather than other activities within the slowpath.
quoted
The outer function rwsem_down_write_failed()
does appear to be a bit bigger (from 866 bytes to 884 bytes) though.
I suspect GCC is being clever and since all this is static it plays
games with the calling convention and pushes these clobbers out.
From: Waiman Long <longman@redhat.com> Date: 2017-02-13 19:42:18
On 02/13/2017 05:53 AM, Peter Zijlstra wrote:
On Mon, Feb 13, 2017 at 11:47:16AM +0100, Peter Zijlstra wrote:
quoted
That way we'd end up with something like:
asm("
push %rdi;
movslq %edi, %rdi;
movq __per_cpu_offset(,%rdi,8), %rax;
cmpb $0, %[offset](%rax);
setne %al;
pop %rdi;
" : : [offset] "i" (((unsigned long)&steal_time) + offsetof(struct steal_time, preempted)));
And if we could get rid of the sign extend on edi we could avoid all the
push-pop nonsense, but I'm not sure I see how to do that (then again,
this asm foo isn't my strongest point).
We could kill the zero or sign extend by changing the calling interface to pass an unsigned long instead of an int. It is much more likely that a zero extend is free for the caller than a sign extend.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
We could kill the zero or sign extend by changing the calling
interface to pass an unsigned long instead of an int. It is much more
likely that a zero extend is free for the caller than a sign extend.
Right, Boris and me talked about that on IRC. I was wondering if the
argument was u32 if we could assume the top 32 bits are 0 and then use
rdi without prior movzx.
That would allow reducing the thing one more instruction.
Also, PVOP_CALL_ARG#() have an (unsigned long) cast in them that doesn't
make sense. That cast ends up resulting in the calling code doing
explicit sign or zero extends into the full 64bit register for no good
reason.
If one removes that cast things still compile, but I worry something
somehow relies on this weird behaviour and will come apart.
We could kill the zero or sign extend by changing the calling interface to pass an unsigned long instead of an int. It is much more likely that a zero extend is free for the caller than a sign extend.
I have thought of that too. However, the goal is to eliminate memory
read/write from/to stack. Eliminating a register sign-extend instruction
won't help much in term of performance.
Cheers,
Longman
I have thought of that too. However, the goal is to eliminate memory
read/write from/to stack. Eliminating a register sign-extend instruction
won't help much in term of performance.
Problem here is that all instructions have dependencies, so if you can
get rid of the sign extend mov you kill a bunch of stall cycles (I would
expect).
But yes, peanuts vs the stack load/stores.