Skip to contentSkip to conversation

8bf00a529967dafbbb210b377c38a15834d1e979 - performance regression?

DISCUSSION Michael S. Tsirkin 2 participants 17 messages Oct 31, 2013 – Nov 18, 2013

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?
Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

Why that's not a large regression, it's a far cry from actually helping performance.

So are we better off not using this feature?
What kind of workload is improved by this change?

#1 lore raw
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop. Next commit d7cd97964ba6d70c5 uses add_atomic_switch_msr()/clear_atomic_switch_msr()
to switch PERF_GLOBAL_CTRL, but it does not depend on VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL support which previous patch added, if the support is not there the switching will use another mechanism which is even slower. So MSR is switched no matter if PERF_GLOBAL_CTRL is enabled or not. If you saying that using VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is slower than using generic vmentry MSR switching then I pretty much doubt it since the only purpose of special VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is to be faster then general mechanism.

--
Gleb.

author
#15 lore raw
In reply to Michael S. Tsirkin
Quoted 10 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?
Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

Quoted 5 lines

Why that's not a large regression, it's a far cry from actually helping performance.

So are we better off not using this feature?
What kind of workload is improved by this change?

Ping.
Let's revert 8bf00a529967dafbbb210b377c38a15834d1e979? I see a small performance gain from reverting it ...

Signature · 1 line
MST
#16 lore raw
Quoted 11 lines · Michael S. Tsirkin
Quoted 10 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?
Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

Quoted 7 lines
Quoted 5 lines

Why that's not a large regression, it's a far cry from actually helping performance.

So are we better off not using this feature?
What kind of workload is improved by this change?

Ping.

I answer it already.

--
Gleb.

author
#2 lore raw
In reply to Gleb Natapov
Quoted 13 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

Quoted 3 lines

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

It does add code to add_atomic_switch_msr.

Quoted 18 lines

Next commit d7cd97964ba6d70c5
uses add_atomic_switch_msr()/clear_atomic_switch_msr()
to switch PERF_GLOBAL_CTRL, but it does not depend on VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL support which previous patch added, if the support is not there the switching will use another mechanism which is even slower. So MSR is switched no matter if PERF_GLOBAL_CTRL is enabled or not. If you saying that using VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is slower than using generic vmentry MSR switching then I pretty much doubt it since the only purpose of special VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is to be faster then general mechanism.

--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 14 lines · Michael S. Tsirkin
Quoted 13 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

Quoted 6 lines
Quoted 3 lines

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

Quoted 12 lines
Quoted 12 lines

Next commit d7cd97964ba6d70c5
uses add_atomic_switch_msr()/clear_atomic_switch_msr()
to switch PERF_GLOBAL_CTRL, but it does not depend on VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL support which previous patch added, if the support is not there the switching will use another mechanism which is even slower. So MSR is switched no matter if PERF_GLOBAL_CTRL is enabled or not. If you saying that using VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is slower than using generic vmentry MSR switching then I pretty much doubt it since the only purpose of special VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is to be faster then general mechanism.

Quoted 6 lines
Quoted 6 lines

--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gleb.

author
#4 lore raw
Quoted 15 lines · Gleb Natapov
Quoted 14 lines · Michael S. Tsirkin
Quoted 13 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

It's simple: if I revert 8bf00a529967dafbbb210 then exit latency is reduced.
You seem to tell me it should be a nop, but in practice it isn't.

Quoted 21 lines
Quoted 12 lines
Quoted 12 lines

Next commit d7cd97964ba6d70c5
uses add_atomic_switch_msr()/clear_atomic_switch_msr()
to switch PERF_GLOBAL_CTRL, but it does not depend on VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL support which previous patch added, if the support is not there the switching will use another mechanism which is even slower. So MSR is switched no matter if PERF_GLOBAL_CTRL is enabled or not. If you saying that using VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is slower than using generic vmentry MSR switching then I pretty much doubt it since the only purpose of special VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is to be faster then general mechanism.

Quoted 6 lines
Quoted 6 lines

--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gleb.

#5 lore raw
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Gleb Natapov
Quoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

Quoted 9 lines
Quoted 3 lines
Quoted 2 lines

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

It's simple: if I revert 8bf00a529967dafbbb210 then exit latency is reduced.
You seem to tell me it should be a nop, but in practice it isn't.

No, if you read below I am saying that it looks like you are claiming that generic msr switch mechanism is faster and I am not buying that. If you believe this to be the case ask Intel for explanation. Your claim about "not running guest under perf" is even stranger since in this case no msr switch should happen regardless of the aforementioned commit (unless guest or host runs nmi watchdog, but then switch will happen no matter if perf is running, so again not running guest under perf" does not make sense). So, in short, you do not really know where the slow down is coming from. My guess is that it comes from the fact that we unconditionally call clear_atomic_switch_msr() in atomic_switch_perf_msrs(), but then fix that instead of reverting the patch.

Quoted 19 lines
Quoted 19 lines
Quoted 18 lines
Quoted 18 lines

Next commit d7cd97964ba6d70c5
uses add_atomic_switch_msr()/clear_atomic_switch_msr()
to switch PERF_GLOBAL_CTRL, but it does not depend on VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL support which previous patch added, if the support is not there the switching will use another mechanism which is even slower. So MSR is switched no matter if PERF_GLOBAL_CTRL is enabled or not. If you saying that using VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is slower than using generic vmentry MSR switching then I pretty much doubt it since the only purpose of special VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is to be faster then general mechanism.

--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 2 lines
Quoted 2 lines

--
Gleb.

--
Gleb.

author
#6 lore raw
Quoted 31 lines · Gleb Natapov
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Gleb Natapov
Quoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

Quoted 9 lines
Quoted 3 lines
Quoted 2 lines

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

It's simple: if I revert 8bf00a529967dafbbb210 then exit latency is reduced.
You seem to tell me it should be a nop, but in practice it isn't.

Quoted 9 lines

No, if you read below I am saying that it looks like you are claiming that generic msr switch mechanism is faster and I am not buying that. If you believe this to be the case ask Intel for explanation. Your claim about "not running guest under perf" is even stranger since in this case no msr switch should happen regardless of the aforementioned commit (unless guest or host runs nmi watchdog, but then switch will happen no matter if perf is running, so again not running guest under perf" does not make sense). So, in short, you do not really know where the slow down is coming from.

That's true.

Quoted 3 lines

My guess is that it comes from the fact that we unconditionally call clear_atomic_switch_msr() in atomic_switch_perf_msrs(), but then fix that instead of reverting the patch.

We can try, but reverting is much simpler, it removes code instead of adding code. Do you know which workload is actually improved by 8bf00a529967dafbbb210?

Quoted 24 lines
Quoted 19 lines
Quoted 19 lines
Quoted 18 lines
Quoted 18 lines

Next commit d7cd97964ba6d70c5
uses add_atomic_switch_msr()/clear_atomic_switch_msr()
to switch PERF_GLOBAL_CTRL, but it does not depend on VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL support which previous patch added, if the support is not there the switching will use another mechanism which is even slower. So MSR is switched no matter if PERF_GLOBAL_CTRL is enabled or not. If you saying that using VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is slower than using generic vmentry MSR switching then I pretty much doubt it since the only purpose of special VM_(ENTRY|EXIT)_LOAD_IA32_PERF_GLOBAL_CTRL is to be faster then general mechanism.

--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 2 lines
Quoted 2 lines

--
Gleb.

--
Gleb.

#7 lore raw
Quoted 42 lines · Michael S. Tsirkin
Quoted 31 lines · Gleb Natapov
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Gleb Natapov
Quoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

Quoted 9 lines
Quoted 3 lines
Quoted 2 lines

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

It's simple: if I revert 8bf00a529967dafbbb210 then exit latency is reduced.
You seem to tell me it should be a nop, but in practice it isn't.

Quoted 9 lines

No, if you read below I am saying that it looks like you are claiming that generic msr switch mechanism is faster and I am not buying that. If you believe this to be the case ask Intel for explanation. Your claim about "not running guest under perf" is even stranger since in this case no msr switch should happen regardless of the aforementioned commit (unless guest or host runs nmi watchdog, but then switch will happen no matter if perf is running, so again not running guest under perf" does not make sense). So, in short, you do not really know where the slow down is coming from.

Quoted 2 lines

That's true.

Then dig dipper.

Quoted 6 lines
Quoted 3 lines

My guess is that it comes from the fact that we unconditionally call clear_atomic_switch_msr() in atomic_switch_perf_msrs(), but then fix that instead of reverting the patch.

We can try, but reverting is much simpler, it removes code instead of adding code.

Well, this is such absurd statement I do not really know what to say :)

Quoted 3 lines

Do you know which workload is actually improved by
8bf00a529967dafbbb210?

Switching PERF_GLOBAL_CTRL when it needs to be switched.

--
Gleb.

#8 lore raw
Quoted 43 lines · Gleb Natapov
Quoted 42 lines · Michael S. Tsirkin
Quoted 40 lines · Gleb Natapov
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Gleb Natapov
Quoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

Quoted 9 lines
Quoted 3 lines
Quoted 2 lines

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

It's simple: if I revert 8bf00a529967dafbbb210 then exit latency is reduced.
You seem to tell me it should be a nop, but in practice it isn't.

No, if you read below I am saying that it looks like you are claiming that generic msr switch mechanism is faster and I am not buying that. If you believe this to be the case ask Intel for explanation. Your claim about "not running guest under perf" is even stranger since in this case no msr switch should happen regardless of the aforementioned commit (unless guest or host runs nmi watchdog, but then switch will happen no matter if perf is running, so again not running guest under perf" does not make sense). So, in short, you do not really know where the slow down is coming from.

Quoted 4 lines
Quoted 2 lines

That's true.

Then dig dipper.

So quick and dirty patch to not needlessly write into VM_ENTRY_CONTROLS when no PERF_GLOBAL_CTRL switching needed removes all the overhead. But we probably need more generic code to shadow entire VM_ENTRY_CONTROLS/VM_EXIT_CONTROLS.

arch/x86/kvm/vmx.c +5 -0
index e293a62..be64221 100644
@@ -413,6 +413,7 @@ struct vcpu_vmx {
 	struct shared_msr_entry *guest_msrs;
 	int                   nmsrs;
 	int                   save_nmsrs;
+	bool                  core_perf_global_ctrl;
 	unsigned long	      host_idt_base;
 #ifdef CONFIG_X86_64
 	u64 		      msr_host_kernel_gs_base;

@@ -1432,9 +1433,12 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
 		break;
 	case MSR_CORE_PERF_GLOBAL_CTRL:
 		if (cpu_has_load_perf_global_ctrl) {
+			if (vmx->core_perf_global_ctrl) {
 			clear_atomic_switch_msr_special(
 					VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
 					VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL);
+				vmx->core_perf_global_ctrl = false;
+			}
 			return;
 		}
 		break;

@@ -1488,6 +1492,7 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
 					GUEST_IA32_PERF_GLOBAL_CTRL,
 					HOST_IA32_PERF_GLOBAL_CTRL,
 					guest_val, host_val);
+			vmx->core_perf_global_ctrl = true;
 			return;
 		}
 		break;

--
Gleb.

author
#9 lore raw
Quoted 44 lines · Gleb Natapov
Quoted 43 lines · Gleb Natapov
Quoted 42 lines · Michael S. Tsirkin
Quoted 40 lines · Gleb Natapov
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Gleb Natapov
Quoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

Quoted 9 lines
Quoted 3 lines
Quoted 2 lines

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

It's simple: if I revert 8bf00a529967dafbbb210 then exit latency is reduced.
You seem to tell me it should be a nop, but in practice it isn't.

No, if you read below I am saying that it looks like you are claiming that generic msr switch mechanism is faster and I am not buying that. If you believe this to be the case ask Intel for explanation. Your claim about "not running guest under perf" is even stranger since in this case no msr switch should happen regardless of the aforementioned commit (unless guest or host runs nmi watchdog, but then switch will happen no matter if perf is running, so again not running guest under perf" does not make sense). So, in short, you do not really know where the slow down is coming from.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

That's true.

Then dig dipper.

So quick and dirty patch to not needlessly write into VM_ENTRY_CONTROLS when no PERF_GLOBAL_CTRL switching needed removes all the overhead. But we probably need more generic code to shadow entire VM_ENTRY_CONTROLS/VM_EXIT_CONTROLS.

Hmm, seems to help but less than 50 cycles, seems to be around 20 and that kind of gain is getting hard to measure.

Quoted 40 lines
arch/x86/kvm/vmx.c +5 -1
index e293a62..be64221 100644
@@ -413,6 +413,7 @@ struct vcpu_vmx {
 	struct shared_msr_entry *guest_msrs;
 	int                   nmsrs;
 	int                   save_nmsrs;
+	bool                  core_perf_global_ctrl;
 	unsigned long	      host_idt_base;
 #ifdef CONFIG_X86_64
 	u64 		      msr_host_kernel_gs_base;

@@ -1432,9 +1433,12 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
 		break;
 	case MSR_CORE_PERF_GLOBAL_CTRL:
 		if (cpu_has_load_perf_global_ctrl) {
+			if (vmx->core_perf_global_ctrl) {
 			clear_atomic_switch_msr_special(
 					VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
 					VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL);
+				vmx->core_perf_global_ctrl = false;
+			}
 			return;
 		}
 		break;

@@ -1488,6 +1492,7 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
 					GUEST_IA32_PERF_GLOBAL_CTRL,
 					HOST_IA32_PERF_GLOBAL_CTRL,
 					guest_val, host_val);
+			vmx->core_perf_global_ctrl = true;
 			return;
 		}
 		break;
--

Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#10 lore raw
Quoted 47 lines · Michael S. Tsirkin
Quoted 46 lines · Gleb Natapov
Quoted 45 lines · Gleb Natapov
Quoted 44 lines · Michael S. Tsirkin
Quoted 31 lines · Gleb Natapov
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Gleb Natapov
Quoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

Quoted 9 lines
Quoted 3 lines
Quoted 2 lines

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

It's simple: if I revert 8bf00a529967dafbbb210 then exit latency is reduced.
You seem to tell me it should be a nop, but in practice it isn't.

Quoted 9 lines

No, if you read below I am saying that it looks like you are claiming that generic msr switch mechanism is faster and I am not buying that. If you believe this to be the case ask Intel for explanation. Your claim about "not running guest under perf" is even stranger since in this case no msr switch should happen regardless of the aforementioned commit (unless guest or host runs nmi watchdog, but then switch will happen no matter if perf is running, so again not running guest under perf" does not make sense). So, in short, you do not really know where the slow down is coming from.

That's true.

Quoted 9 lines
Quoted 5 lines
Quoted 2 lines

Then dig dipper.

So quick and dirty patch to not needlessly write into VM_ENTRY_CONTROLS when no PERF_GLOBAL_CTRL switching needed removes all the overhead. But we probably need more generic code to shadow entire VM_ENTRY_CONTROLS/VM_EXIT_CONTROLS.

Hmm, seems to help but less than 50 cycles, seems to be around 20 and that kind of gain is getting hard to measure.

With the patch the code with and without MSR_CORE_PERF_GLOBAL_CTRL switch is exactly same when no switching is needed (nothing is done basically), so I doubt your measurement. For me result is exactly same (it is noisy, but noisy in the same way).

--
Gleb.

author
#11 lore raw
Quoted 54 lines · Gleb Natapov
Quoted 53 lines · Michael S. Tsirkin
Quoted 51 lines · Gleb Natapov
Quoted 43 lines · Gleb Natapov
Quoted 42 lines · Michael S. Tsirkin
Quoted 40 lines · Gleb Natapov
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Gleb Natapov
Quoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

Quoted 9 lines
Quoted 3 lines
Quoted 2 lines

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

It's simple: if I revert 8bf00a529967dafbbb210 then exit latency is reduced.
You seem to tell me it should be a nop, but in practice it isn't.

No, if you read below I am saying that it looks like you are claiming that generic msr switch mechanism is faster and I am not buying that. If you believe this to be the case ask Intel for explanation. Your claim about "not running guest under perf" is even stranger since in this case no msr switch should happen regardless of the aforementioned commit (unless guest or host runs nmi watchdog, but then switch will happen no matter if perf is running, so again not running guest under perf" does not make sense). So, in short, you do not really know where the slow down is coming from.

Quoted 4 lines
Quoted 2 lines

That's true.

Then dig dipper.

So quick and dirty patch to not needlessly write into VM_ENTRY_CONTROLS when no PERF_GLOBAL_CTRL switching needed removes all the overhead. But we probably need more generic code to shadow entire VM_ENTRY_CONTROLS/VM_EXIT_CONTROLS.

Quoted 6 lines
Quoted 3 lines

Hmm, seems to help but less than 50 cycles, seems to be around 20 and that kind of gain is getting hard to measure.

With the patch the code with and without MSR_CORE_PERF_GLOBAL_CTRL switch is exactly same when no switching is needed (nothing is done basically), so I doubt your measurement.

OK I'll retry when I have the time.

Quoted 5 lines

For me result is exactly same (it is noisy,
but noisy in the same way).

--
Gleb.

author
#12 lore raw
In reply to Gleb Natapov
Quoted 44 lines · Gleb Natapov
Quoted 43 lines · Gleb Natapov
Quoted 42 lines · Michael S. Tsirkin
Quoted 40 lines · Gleb Natapov
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Gleb Natapov
Quoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

Quoted 9 lines
Quoted 3 lines
Quoted 2 lines

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

It's simple: if I revert 8bf00a529967dafbbb210 then exit latency is reduced.
You seem to tell me it should be a nop, but in practice it isn't.

No, if you read below I am saying that it looks like you are claiming that generic msr switch mechanism is faster and I am not buying that. If you believe this to be the case ask Intel for explanation. Your claim about "not running guest under perf" is even stranger since in this case no msr switch should happen regardless of the aforementioned commit (unless guest or host runs nmi watchdog, but then switch will happen no matter if perf is running, so again not running guest under perf" does not make sense). So, in short, you do not really know where the slow down is coming from.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

That's true.

Then dig dipper.

So quick and dirty patch to not needlessly write into VM_ENTRY_CONTROLS when no PERF_GLOBAL_CTRL switching needed removes all the overhead. But we probably need more generic code to shadow entire VM_ENTRY_CONTROLS/VM_EXIT_CONTROLS.

Do you plan to complete this? Merge as is? You said this saves about 50 cycles per exit for you, did you not?

Quoted 35 lines
arch/x86/kvm/vmx.c +5 -1
index e293a62..be64221 100644
@@ -413,6 +413,7 @@ struct vcpu_vmx {
 	struct shared_msr_entry *guest_msrs;
 	int                   nmsrs;
 	int                   save_nmsrs;
+	bool                  core_perf_global_ctrl;
 	unsigned long	      host_idt_base;
 #ifdef CONFIG_X86_64
 	u64 		      msr_host_kernel_gs_base;

@@ -1432,9 +1433,12 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
 		break;
 	case MSR_CORE_PERF_GLOBAL_CTRL:
 		if (cpu_has_load_perf_global_ctrl) {
+			if (vmx->core_perf_global_ctrl) {
 			clear_atomic_switch_msr_special(
 					VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
 					VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL);
+				vmx->core_perf_global_ctrl = false;
+			}
 			return;
 		}
 		break;

@@ -1488,6 +1492,7 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
 					GUEST_IA32_PERF_GLOBAL_CTRL,
 					HOST_IA32_PERF_GLOBAL_CTRL,
 					guest_val, host_val);
+			vmx->core_perf_global_ctrl = true;
 			return;
 		}
 		break;
--

Gleb.

#13 lore raw
Quoted 47 lines · Michael S. Tsirkin
Quoted 46 lines · Gleb Natapov
Quoted 45 lines · Gleb Natapov
Quoted 44 lines · Michael S. Tsirkin
Quoted 31 lines · Gleb Natapov
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Gleb Natapov
Quoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

Quoted 9 lines
Quoted 3 lines
Quoted 2 lines

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

It's simple: if I revert 8bf00a529967dafbbb210 then exit latency is reduced.
You seem to tell me it should be a nop, but in practice it isn't.

Quoted 9 lines

No, if you read below I am saying that it looks like you are claiming that generic msr switch mechanism is faster and I am not buying that. If you believe this to be the case ask Intel for explanation. Your claim about "not running guest under perf" is even stranger since in this case no msr switch should happen regardless of the aforementioned commit (unless guest or host runs nmi watchdog, but then switch will happen no matter if perf is running, so again not running guest under perf" does not make sense). So, in short, you do not really know where the slow down is coming from.

That's true.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

Then dig dipper.

So quick and dirty patch to not needlessly write into VM_ENTRY_CONTROLS when no PERF_GLOBAL_CTRL switching needed removes all the overhead. But we probably need more generic code to shadow entire VM_ENTRY_CONTROLS/VM_EXIT_CONTROLS.

Do you plan to complete this? Merge as is?

I have a patch to shadow VM_ENTRY_CONTROLS/VM_EXIT_CONTROLS now, will post it shortly, but it will have to wait for the end of a merge window anyway.

Quoted 2 lines

You said this saves about 50 cycles per exit for you, did you not?

Let's not exaggerate :) May be 20 after removing all the noise.

Quoted 29 lines
Quoted 29 lines
arch/x86/kvm/vmx.c +4 -0
index e293a62..be64221 100644
@@ -413,6 +413,7 @@ struct vcpu_vmx {
 	struct shared_msr_entry *guest_msrs;
 	int                   nmsrs;
 	int                   save_nmsrs;
+	bool                  core_perf_global_ctrl;
 	unsigned long	      host_idt_base;
 #ifdef CONFIG_X86_64
 	u64 		      msr_host_kernel_gs_base;

@@ -1432,9 +1433,12 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
 		break;
 	case MSR_CORE_PERF_GLOBAL_CTRL:
 		if (cpu_has_load_perf_global_ctrl) {
+			if (vmx->core_perf_global_ctrl) {
 			clear_atomic_switch_msr_special(
 					VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
 					VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL);
+				vmx->core_perf_global_ctrl = false;
+			}
 			return;
 		}
 		break;

@@ -1488,6 +1492,7 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
 					GUEST_IA32_PERF_GLOBAL_CTRL,
Quoted 8 lines
Quoted 8 lines
arch/x86/kvm/vmx.c +1 -1
 					HOST_IA32_PERF_GLOBAL_CTRL,
 					guest_val, host_val);
+			vmx->core_perf_global_ctrl = true;
 			return;
 		}
 		break;
--

Gleb.

--
Gleb.

author
#14 lore raw
Quoted 55 lines · Gleb Natapov
Quoted 54 lines · Michael S. Tsirkin
Quoted 52 lines · Gleb Natapov
Quoted 43 lines · Gleb Natapov
Quoted 42 lines · Michael S. Tsirkin
Quoted 40 lines · Gleb Natapov
Quoted 20 lines · Michael S. Tsirkin
Quoted 19 lines · Gleb Natapov
Quoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Gleb Natapov
Quoted 4 lines · Michael S. Tsirkin

commit 8bf00a529967dafbbb210b377c38a15834d1e979:
" KVM: VMX: add support for switching of PERF_GLOBAL_CTRL " was
as far as I can tell supposed to bring about performance improvement on hardware that supports it?

No, it (and commits after it) supposed to fix a bug which it did.

Quoted 6 lines

Instead it seems to make the typical case (not running guest under perf) a bit slower than it used to be.
the cost of VMexit goes up by about 50 cycles
on sandy bridge where the optimization in question actually is activated.

You seams to be confused. 8bf00a529967dafbbb210 adds support for special PERF_GLOBAL_CTRL switching, but does not add code to switch anything, so the commit itself is a nop.

Quoted 9 lines
Quoted 3 lines
Quoted 2 lines

It does add code to add_atomic_switch_msr.

So what? You do not read what I wrote.

It's simple: if I revert 8bf00a529967dafbbb210 then exit latency is reduced.
You seem to tell me it should be a nop, but in practice it isn't.

No, if you read below I am saying that it looks like you are claiming that generic msr switch mechanism is faster and I am not buying that. If you believe this to be the case ask Intel for explanation. Your claim about "not running guest under perf" is even stranger since in this case no msr switch should happen regardless of the aforementioned commit (unless guest or host runs nmi watchdog, but then switch will happen no matter if perf is running, so again not running guest under perf" does not make sense). So, in short, you do not really know where the slow down is coming from.

Quoted 4 lines
Quoted 2 lines

That's true.

Then dig dipper.

So quick and dirty patch to not needlessly write into VM_ENTRY_CONTROLS when no PERF_GLOBAL_CTRL switching needed removes all the overhead. But we probably need more generic code to shadow entire VM_ENTRY_CONTROLS/VM_EXIT_CONTROLS.

Quoted 4 lines
Quoted 1 line

Do you plan to complete this? Merge as is?

I have a patch to shadow VM_ENTRY_CONTROLS/VM_EXIT_CONTROLS now, will post it shortly, but it will have to wait for the end of a merge window anyway.

No rush.

Quoted 3 lines
Quoted 2 lines

You said this saves about 50 cycles per exit for you, did you not?

Let's not exaggerate :) May be 20 after removing all the noise.

Interesting. I remember reverting the feature gave me 50 cycles. I'll have to retest, but 20 cycles on all exits is nice too.

Quoted 40 lines
Quoted 29 lines
Quoted 29 lines
arch/x86/kvm/vmx.c +4 -0
index e293a62..be64221 100644
@@ -413,6 +413,7 @@ struct vcpu_vmx {
 	struct shared_msr_entry *guest_msrs;
 	int                   nmsrs;
 	int                   save_nmsrs;
+	bool                  core_perf_global_ctrl;
 	unsigned long	      host_idt_base;
 #ifdef CONFIG_X86_64
 	u64 		      msr_host_kernel_gs_base;

@@ -1432,9 +1433,12 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
 		break;
 	case MSR_CORE_PERF_GLOBAL_CTRL:
 		if (cpu_has_load_perf_global_ctrl) {
+			if (vmx->core_perf_global_ctrl) {
 			clear_atomic_switch_msr_special(
 					VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
 					VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL);
+				vmx->core_perf_global_ctrl = false;
+			}
 			return;
 		}
 		break;

@@ -1488,6 +1492,7 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
 					GUEST_IA32_PERF_GLOBAL_CTRL,
Quoted 8 lines
Quoted 8 lines
arch/x86/kvm/vmx.c +1 -1
 					HOST_IA32_PERF_GLOBAL_CTRL,
 					guest_val, host_val);
+			vmx->core_perf_global_ctrl = true;
 			return;
 		}
 		break;
--

Gleb.

--
Gleb.