perf with precise attribute kills all KVM based VMs

Quoted 6 lines · Peter Zijlstra

Quoted 4 lines · Gleb Natapov
Yes, this is knows problem that I can't find time to fix. The crash is cause by CPU using host PEBS virtual address while guest is running which causes guest memory corruption. We should disable evens that use PEBS at the guest entry.

Quoted 6 lines

Whoops.. so the hardware is reading the DS address as programmed by the host (host linear address) and using it as a guest linear address?
Quality stuff..
Disabling PEBS events for guests isn't pretty though..

We already have atomic MSR switching at guest entry/exit time. So it's not pretty in terms of not getting full profiling, but the code won't be too hard. Basically we just have to exclude_guest any pebs event.

Quoted 3 lines

but I guess the
only alternative is mapping the DS into the guest and reprogramming MSR_IA32_DS_AREA which is all a little involved I suppose?

Way too involved, especially as it's virtual addresses and we don't control the guest cr3. Note that the hardware won't fail gracefully.

Signature · 1 line

error compiling committee.c: too many arguments to function

David Ahern

author

2012-07-09 14:47 UTC #5 lore raw

Quoted 10 lines · Avi Kivity

Quoted 9 lines · Peter Zijlstra
Quoted 4 lines · Gleb Natapov
Yes, this is knows problem that I can't find time to fix. The crash is cause by CPU using host PEBS virtual address while guest is running which causes guest memory corruption. We should disable evens that use PEBS at the guest entry.
Whoops.. so the hardware is reading the DS address as programmed by the host (host linear address) and using it as a guest linear address?

Quoted 7 lines

Quoted 3 lines
Quality stuff..
Disabling PEBS events for guests isn't pretty though..
We already have atomic MSR switching at guest entry/exit time. So it's not pretty in terms of not getting full profiling, but the code won't be too hard. Basically we just have to exclude_guest any pebs event.

I found this testing changes to perf-kvm, but found the problem extends to just perf-record. With perf-record exclude_guest defaults to 1. See tools/perf/util/util.c, event_attr_init().

Quoted 8 lines

Quoted 4 lines
but I guess the
only alternative is mapping the DS into the guest and reprogramming MSR_IA32_DS_AREA which is all a little involved I suppose?
Way too involved, especially as it's virtual addresses and we don't control the guest cr3. Note that the hardware won't fail gracefully.

Peter Zijlstra

maintainer

2012-07-09 14:47 UTC #33 lore raw

Quoted 5 lines · Avi Kivity

Quoted 1 line
Disabling PEBS events for guests isn't pretty though..
We already have atomic MSR switching at guest entry/exit time. So it's not pretty in terms of not getting full profiling, but the code won't be too hard. Basically we just have to exclude_guest any pebs event.

OK, so ideally we'd do something like the below, except of course that that isn't backwards compatible and will break the world :/

bugger that

---

arch/x86/kernel/cpu/perf_event.c +6 -0

@@ -336,6 +338,9 @@ int x86_setup_perfctr(struct perf_event *event)
                /* BTS is currently only allowed for user-mode. */
                if (!attr->exclude_kernel)
                        return -EOPNOTSUPP;
+
+               if (!attr->exclude_guest)
+                       return -EOPNOTSUPP;
        }
 
        hwc->config |= config;

@@ -378,6 +383,9 @@ int x86_pmu_hw_config(struct perf_event *event)
        if (event->attr.precise_ip) {
                int precise = 0;
 
+               if (!event->attr.exclude_guest)
+                       return -EOPNOTSUPP;
+
                /* Support for constant skid */
                if (x86_pmu.pebs_active && !x86_pmu.pebs_broken) {
                        precise++;

Peter Zijlstra

maintainer

2012-07-09 14:50 UTC #6 lore raw

Quoted 4 lines · David Ahern

I found this testing changes to perf-kvm, but found the problem extends to just perf-record. With perf-record exclude_guest defaults to 1. See tools/perf/util/util.c, event_attr_init().

You lost me there.. so perf-record defaults to exclude_guest=1 (which would make the proposed patch I just send actually possible), but its still going *bang* ?

Avi Kivity

2012-07-09 14:51 UTC #7 lore raw

Quoted 9 lines · Peter Zijlstra

Quoted 4 lines · David Ahern
I found this testing changes to perf-kvm, but found the problem extends to just perf-record. With perf-record exclude_guest defaults to 1. See tools/perf/util/util.c, event_attr_init().
You lost me there.. so perf-record defaults to exclude_guest=1 (which would make the proposed patch I just send actually possible), but its still going *bang* ?

It's possible that the DS writes overshoot the MSR... or that there's a bug somewhere.

Signature · 1 line

error compiling committee.c: too many arguments to function

David Ahern

author

2012-07-09 14:52 UTC #30 lore raw

Quoted 10 lines · Peter Zijlstra

Quoted 4 lines · David Ahern
I found this testing changes to perf-kvm, but found the problem extends to just perf-record. With perf-record exclude_guest defaults to 1. See tools/perf/util/util.c, event_attr_init().
You lost me there.. so perf-record defaults to exclude_guest=1 (which would make the proposed patch I just send actually possible), but its still going *bang* ?

I can do the formality of testing your proposed patch, but yes I think so. exclude_guest defaults to 1 because perf_guest defaults to false.

David

Gleb Natapov

2012-07-09 14:54 UTC #8 lore raw

Quoted 7 lines · Avi Kivity

Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · David Ahern
I found this testing changes to perf-kvm, but found the problem extends to just perf-record. With perf-record exclude_guest defaults to 1. See tools/perf/util/util.c, event_attr_init().

Quoted 7 lines

Quoted 3 lines
You lost me there.. so perf-record defaults to exclude_guest=1 (which would make the proposed patch I just send actually possible), but its still going *bang* ?
It's possible that the DS writes overshoot the MSR... or that there's a bug somewhere.

I hope it is the later, because we can do nothing with the former.

--
Gleb.

Gleb Natapov

2012-07-09 14:57 UTC #9 lore raw

Quoted 8 lines · Gleb Natapov

Quoted 7 lines · Avi Kivity
Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · David Ahern
I found this testing changes to perf-kvm, but found the problem extends to just perf-record. With perf-record exclude_guest defaults to 1. See tools/perf/util/util.c, event_attr_init().

Quoted 9 lines

Quoted 7 lines
Quoted 3 lines
You lost me there.. so perf-record defaults to exclude_guest=1 (which would make the proposed patch I just send actually possible), but its still going *bang* ?
It's possible that the DS writes overshoot the MSR... or that there's a bug somewhere.
I hope it is the later, because we can do nothing with the former.

Actually we can disable DS before guest entry.

--
Gleb.

David Ahern

author

2012-07-09 14:58 UTC #31 lore raw

Quoted 7 lines · David Ahern

Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · David Ahern
I found this testing changes to perf-kvm, but found the problem extends to just perf-record. With perf-record exclude_guest defaults to 1. See tools/perf/util/util.c, event_attr_init().

Quoted 8 lines

Quoted 4 lines
You lost me there.. so perf-record defaults to exclude_guest=1 (which would make the proposed patch I just send actually possible), but its still going *bang* ?
I can do the formality of testing your proposed patch, but yes I think so. exclude_guest defaults to 1 because perf_guest defaults to false.

Let me dig for a few minutes. exclude_user is getting reset somewhere. Setting a breakpoint on sys_perf_event_open and dumping the attribute:

$1 = {type = 0, size = 80, config = 0, {sample_period = 4000, sample_freq = 4000}, sample_type = 263, read_format = 7,
disabled = 1, inherit = 1, pinned = 0, exclusive = 0, exclude_user =
0, exclude_kernel = 0, exclude_hv = 0, exclude_idle =

    0, mmap = 1, comm = 1, freq = 1, inherit_stat = 0, enable_on_exec = 
1, task = 0, watermark = 0, precise_ip = 1,
  mmap_data = 0, sample_id_all = 1, exclude_host = 0, exclude_guest = 
0, __reserved_1 = 0, {wakeup_events = 0,
    wakeup_watermark = 0}, bp_type = 0, {bp_addr = 0, config1 = 0},

{bp_len = 0, config2 = 0}, branch_sample_type = 0}

Peter Zijlstra

maintainer

2012-07-09 15:00 UTC #10 lore raw

Quoted 7 lines · Avi Kivity

Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · David Ahern
I found this testing changes to perf-kvm, but found the problem extends to just perf-record. With perf-record exclude_guest defaults to 1. See tools/perf/util/util.c, event_attr_init().

Quoted 6 lines

Quoted 3 lines
You lost me there.. so perf-record defaults to exclude_guest=1 (which would make the proposed patch I just send actually possible), but its still going *bang* ?
It's possible that the DS writes overshoot the MSR... or that there's a bug somewhere.

OK, so the first thing to stare at is in what order the MSRs are touched, I think you first need to stop the counter, then clear the PEBS bits, then clear the DS_AREA one.

David Ahern

author

2012-07-09 15:18 UTC #32 lore raw

Quoted 25 lines · David Ahern

Quoted 7 lines · David Ahern
Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · David Ahern
I found this testing changes to perf-kvm, but found the problem extends to just perf-record. With perf-record exclude_guest defaults to 1. See tools/perf/util/util.c, event_attr_init().
Quoted 8 lines
Quoted 4 lines
You lost me there.. so perf-record defaults to exclude_guest=1 (which would make the proposed patch I just send actually possible), but its still going *bang* ?
I can do the formality of testing your proposed patch, but yes I think so. exclude_guest defaults to 1 because perf_guest defaults to false.
Let me dig for a few minutes. exclude_user is getting reset somewhere. Setting a breakpoint on sys_perf_event_open and dumping the attribute:
$1 = {type = 0, size = 80, config = 0, {sample_period = 4000, sample_freq = 4000}, sample_type = 263, read_format = 7,
disabled = 1, inherit = 1, pinned = 0, exclusive = 0, exclude_user =
0, exclude_kernel = 0, exclude_hv = 0, exclude_idle =

Quoted 8 lines

     0, mmap = 1, comm = 1, freq = 1, inherit_stat = 0, enable_on_exec =
1, task = 0, watermark = 0, precise_ip = 1,
   mmap_data = 0, sample_id_all = 1, exclude_host = 0, exclude_guest =
0, __reserved_1 = 0, {wakeup_events = 0,
     wakeup_watermark = 0}, bp_type = 0, {bp_addr = 0, config1 = 0},

{bp_len = 0, config2 = 0}, branch_sample_type = 0}

Offending function is parse_events_modifier(). It resets exclude_guest to 0. That's a separate thread/bug.

Undoing that (hardcoding exclude_guest to 1) and verifying in gdb that it is set for all cpus:

(gdb) p *attr
$17 = {type = 0, size = 80, config = 0, {sample_period = 4000, sample_freq = 4000}, sample_type = 423, read_format = 7,
disabled = 1, inherit = 1, pinned = 0, exclusive = 0, exclude_user =
0, exclude_kernel = 0, exclude_hv = 0, exclude_idle =

    0, mmap = 1, comm = 1, freq = 1, inherit_stat = 0, enable_on_exec = 
0, task = 0, watermark = 0, precise_ip = 1,
  mmap_data = 0, sample_id_all = 1, exclude_host = 0, exclude_guest = 
1, __reserved_1 = 0, {wakeup_events = 0,
    wakeup_watermark = 0}, bp_type = 0, {bp_addr = 0, config1 = 0},

{bp_len = 0, config2 = 0}, branch_sample_type = 0}

It still bombs and the VM dies.

David

David Ahern

author

2012-07-10 23:38 UTC #11 lore raw

Quoted 12 lines · Peter Zijlstra

Quoted 11 lines · Avi Kivity
Quoted 9 lines · Peter Zijlstra
Quoted 4 lines · David Ahern
I found this testing changes to perf-kvm, but found the problem extends to just perf-record. With perf-record exclude_guest defaults to 1. See tools/perf/util/util.c, event_attr_init().
You lost me there.. so perf-record defaults to exclude_guest=1 (which would make the proposed patch I just send actually possible), but its still going *bang* ?

Quoted 7 lines

Quoted 2 lines
It's possible that the DS writes overshoot the MSR... or that there's a bug somewhere.
OK, so the first thing to stare at is in what order the MSRs are touched, I think you first need to stop the counter, then clear the PEBS bits, then clear the DS_AREA one.

Any updates on the options for this or is there still squinting going on with MSR orders?

I guess it's a good thing Ingo's request for :pp to be the default was not implemented:
https://lkml.org/lkml/2012/5/14/26

David

Gleb Natapov

2012-07-11 07:10 UTC #12 lore raw

Quoted 16 lines · David Ahern

Quoted 15 lines · Peter Zijlstra
Quoted 13 lines · Avi Kivity
Quoted 9 lines · Peter Zijlstra
Quoted 4 lines · David Ahern
I found this testing changes to perf-kvm, but found the problem extends to just perf-record. With perf-record exclude_guest defaults to 1. See tools/perf/util/util.c, event_attr_init().
You lost me there.. so perf-record defaults to exclude_guest=1 (which would make the proposed patch I just send actually possible), but its still going *bang* ?
It's possible that the DS writes overshoot the MSR... or that there's a bug somewhere.

Quoted 8 lines

Quoted 4 lines
OK, so the first thing to stare at is in what order the MSRs are touched, I think you first need to stop the counter, then clear the PEBS bits, then clear the DS_AREA one.
Any updates on the options for this or is there still squinting going on with MSR orders?

Looks like Avi is right about the overshoot. Can you test something like this?

arch/x86/kernel/cpu/perf_event_intel.c +4 -1

index 166546e..5fb371a 100644
@@ -1374,8 +1374,11 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr)
 	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;
 	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
 	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
+	arr[1].msr = MSR_IA32_PEBS_ENABLE;
+	arr[1].host = cpuc->pebs_enabled;
+	arr[1].guest = 0;
+	*nr = 2;
 
-	*nr = 1;
 	return arr;
 }

Quoted 5 lines

I guess it's a good thing Ingo's request for :pp to be the default was not implemented:
https://lkml.org/lkml/2012/5/14/26
David

--
Gleb.

Peter Zijlstra

maintainer

2012-07-11 09:49 UTC #13 lore raw

Quoted 8 lines · Gleb Natapov

Looks like Avi is right about the overshoot. Can you test something like this?
arch/x86/kernel/cpu/perf_event_intel.c +0 -0
index 166546e..5fb371a 100644
@@ -1374,8 +1374,11 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr)
 	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;

arch/x86/kernel/cpu/perf_event_intel.c static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr) · line 1383

Quoted 10 lines

arch/x86/kernel/cpu/perf_event_intel.c +4 -1

 	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
 	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
+	arr[1].msr = MSR_IA32_PEBS_ENABLE;
+	arr[1].host = cpuc->pebs_enabled;
+	arr[1].guest = 0;
+	*nr = 2;
 
-	*nr = 1;
 	return arr;
 }

You also need to clear TR, BTS, BTINT from MSR_IA32_DEBUGCTLMSR and ideally you'd also clear MSR_IA32_DS_AREA so that any write will be a proper NULL deref or such.

Gleb Natapov

2012-07-11 09:53 UTC #14 lore raw

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Gleb Natapov

Looks like Avi is right about the overshoot. Can you test something like this?

arch/x86/kernel/cpu/perf_event_intel.c +4 -0

index 166546e..5fb371a 100644
@@ -1374,8 +1374,11 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr)
 	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;
 	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
 	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
+	arr[1].msr = MSR_IA32_PEBS_ENABLE;
+	arr[1].host = cpuc->pebs_enabled;
+	arr[1].guest = 0;
+	*nr = 2;

Quoted 8 lines

arch/x86/kernel/cpu/perf_event_intel.c static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr) · line 1383
Quoted 3 lines
arch/x86/kernel/cpu/perf_event_intel.c +0 -1
-	*nr = 1;
 	return arr;
 }
You also need to clear TR, BTS, BTINT from MSR_IA32_DEBUGCTLMSR and ideally you'd also clear MSR_IA32_DS_AREA so that any write will be a proper NULL deref or such.

Yes. With the patch above :pp modifier does not crash guest for me, but in theory it should since BTS are still written to DS. May be BTS writes do not overshoot guest entry. Will have to ask Intel for clarification.

--
Gleb.

David Ahern

author

2012-07-11 13:34 UTC #15 lore raw

Quoted 4 lines · Gleb Natapov

Quoted 3 lines · Peter Zijlstra
Quoted 1 line · Gleb Natapov
Looks like Avi is right about the overshoot. Can you test something like this?

I head to the airport in a few minutes; I'll try it out tonight or tomorrow morning.

David

Quoted 28 lines

Quoted 13 lines
Quoted 13 lines
arch/x86/kernel/cpu/perf_event_intel.c +4 -0
index 166546e..5fb371a 100644
@@ -1374,8 +1374,11 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr)
  	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;
  	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
  	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
+	arr[1].msr = MSR_IA32_PEBS_ENABLE;
+	arr[1].host = cpuc->pebs_enabled;
+	arr[1].guest = 0;
+	*nr = 2;
 
Quoted 8 lines
arch/x86/kernel/cpu/perf_event_intel.c static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr) · line 1383
Quoted 3 lines
arch/x86/kernel/cpu/perf_event_intel.c +0 -1
-	*nr = 1;
  	return arr;
  }
You also need to clear TR, BTS, BTINT from MSR_IA32_DEBUGCTLMSR and ideally you'd also clear MSR_IA32_DS_AREA so that any write will be a proper NULL deref or such.
Yes. With the patch above :pp modifier does not crash guest for me, but in theory it should since BTS are still written to DS. May be BTS writes do not overshoot guest entry. Will have to ask Intel for clarification.
--
Gleb.

David Ahern

author

2012-07-12 04:12 UTC #16 lore raw

Quoted 18 lines · Gleb Natapov

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Gleb Natapov

Looks like Avi is right about the overshoot. Can you test something like this?

arch/x86/kernel/cpu/perf_event_intel.c +4 -0

index 166546e..5fb371a 100644
@@ -1374,8 +1374,11 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr)
  	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;
  	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
  	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
+	arr[1].msr = MSR_IA32_PEBS_ENABLE;
+	arr[1].host = cpuc->pebs_enabled;
+	arr[1].guest = 0;
+	*nr = 2;

Quoted 4 lines

Quoted 4 lines
arch/x86/kernel/cpu/perf_event_intel.c static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr) · line 1383
Quoted 3 lines
arch/x86/kernel/cpu/perf_event_intel.c +0 -1
-	*nr = 1;
  	return arr;
  }

So far the 64-bit Fedora 10 VM with both a Fedora 10 stock kernel and a 2.6.38 kernel have not faired well - and that's the only VM I have tried at the moment. Using -e cycles:pp I have been able to lock up the VM 3 times out of 3 series of tests with perf-kvm that includes network traffic (e.g., netperf), disk I/O (dd based to create a file with dsync flag) and pure userspace cpu bound (openssl speed). May or may not be related.

Also, I noted that 'perf kvm --guest record -e cycles:pp' does not generate a whole lot of samples -- like < 100 in a 20-second sample -- despite the fact that the guest is rather busy.

I won't have much time over the next few days to run much in the way of tests; I'll come back to it Sunday night.

David

Quoted 7 lines

Quoted 4 lines
You also need to clear TR, BTS, BTINT from MSR_IA32_DEBUGCTLMSR and ideally you'd also clear MSR_IA32_DS_AREA so that any write will be a proper NULL deref or such.
Yes. With the patch above :pp modifier does not crash guest for me, but in theory it should since BTS are still written to DS. May be BTS writes do not overshoot guest entry. Will have to ask Intel for clarification.

Gleb Natapov

2012-07-12 04:29 UTC #17 lore raw

Quoted 24 lines · David Ahern

Quoted 18 lines · Gleb Natapov
Quoted 17 lines · Peter Zijlstra
Quoted 15 lines · Gleb Natapov
Looks like Avi is right about the overshoot. Can you test something like this?
arch/x86/kernel/cpu/perf_event_intel.c +4 -0
index 166546e..5fb371a 100644
@@ -1374,8 +1374,11 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr)
 	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;
 	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
 	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
+	arr[1].msr = MSR_IA32_PEBS_ENABLE;
+	arr[1].host = cpuc->pebs_enabled;
+	arr[1].guest = 0;
+	*nr = 2;
 
Quoted 4 lines
Quoted 4 lines
arch/x86/kernel/cpu/perf_event_intel.c static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr) · line 1383
Quoted 3 lines
arch/x86/kernel/cpu/perf_event_intel.c +0 -1
-	*nr = 1;
 	return arr;
 }

Quoted 8 lines

So far the 64-bit Fedora 10 VM with both a Fedora 10 stock kernel and a 2.6.38 kernel have not faired well - and that's the only VM I have tried at the moment. Using -e cycles:pp I have been able to lock up the VM 3 times out of 3 series of tests with perf-kvm that includes network traffic (e.g., netperf), disk I/O (dd based to create a file with dsync flag) and pure userspace cpu bound (openssl speed). May or may not be related.

OK that's may be BTSes. What about -e cycles:p? BTW are you using your patch to set exclude_guest parameter? If not use -e cycles:Hp.

Quoted 4 lines

Also, I noted that 'perf kvm --guest record -e cycles:pp' does not generate a whole lot of samples -- like < 100 in a 20-second sample -- despite the fact that the guest is rather busy.

Host events do not suppose to generate events while guest is running.

Quoted 12 lines

I won't have much time over the next few days to run much in the way of tests; I'll come back to it Sunday night.
David
Quoted 7 lines
Quoted 4 lines
You also need to clear TR, BTS, BTINT from MSR_IA32_DEBUGCTLMSR and ideally you'd also clear MSR_IA32_DS_AREA so that any write will be a proper NULL deref or such.
Yes. With the patch above :pp modifier does not crash guest for me, but in theory it should since BTS are still written to DS. May be BTS writes do not overshoot guest entry. Will have to ask Intel for clarification.

--
Gleb.

David Ahern

author

2012-07-12 15:20 UTC #18 lore raw

Quoted 33 lines · Gleb Natapov

Quoted 24 lines · David Ahern
Quoted 18 lines · Gleb Natapov
Quoted 17 lines · Peter Zijlstra
Quoted 15 lines · Gleb Natapov
Looks like Avi is right about the overshoot. Can you test something like this?
arch/x86/kernel/cpu/perf_event_intel.c +4 -0
index 166546e..5fb371a 100644
@@ -1374,8 +1374,11 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr)
  	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;
  	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
  	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
+	arr[1].msr = MSR_IA32_PEBS_ENABLE;
+	arr[1].host = cpuc->pebs_enabled;
+	arr[1].guest = 0;
+	*nr = 2;
 
Quoted 4 lines
Quoted 4 lines
arch/x86/kernel/cpu/perf_event_intel.c static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr) · line 1383
Quoted 3 lines
arch/x86/kernel/cpu/perf_event_intel.c +0 -1
-	*nr = 1;
  	return arr;
  }
Quoted 8 lines
So far the 64-bit Fedora 10 VM with both a Fedora 10 stock kernel and a 2.6.38 kernel have not faired well - and that's the only VM I have tried at the moment. Using -e cycles:pp I have been able to lock up the VM 3 times out of 3 series of tests with perf-kvm that includes network traffic (e.g., netperf), disk I/O (dd based to create a file with dsync flag) and pure userspace cpu bound (openssl speed). May or may not be related.

Quoted 2 lines

OK that's may be BTSes. What about -e cycles:p? BTW are you using your patch to set exclude_guest parameter? If not use -e cycles:Hp.

I started with cycles:pp; should not really matter - they all need to work without blowing up VMs (cycles:p, cycles:pH, cycles:pG, cycles:pp, cycles:ppH, cycles:ppG).

For grins I ran a quick test while reading emails this morning. This time a fedora 17 VM with 3.4.0-1.fc17.x86_64 kernel. It too locks up pretty quickly - just a couple of iterations of perf:

perf kvm --guestmount=/tmp/guest-mount record -fo /tmp/perf.data -a -v -e cycles:pH -- sleep 60

Note the :pH this time.

I did not have netserver installed in the VM so used a ping flood for network traffic.

Quoted 6 lines

Quoted 5 lines
Also, I noted that 'perf kvm --guest record -e cycles:pp' does not generate a whole lot of samples -- like < 100 in a 20-second sample -- despite the fact that the guest is rather busy.
Host events do not suppose to generate events while guest is running.

My server has 16 cpus and the VM has only 2 vcpus; with the -a I would expect some host sampling. Note: in the above case :pp resets the exclude-host modifier set by the perf-kvm part, so hosts samples are not excluded. See parse_events_modifier().

So, is the idea of your patch to not enable the PEBS in guest mode?

David

Gleb Natapov

2012-07-12 16:06 UTC #19 lore raw

Quoted 34 lines · David Ahern

Quoted 33 lines · Gleb Natapov
Quoted 32 lines · David Ahern
Quoted 18 lines · Gleb Natapov
Quoted 17 lines · Peter Zijlstra
Quoted 15 lines · Gleb Natapov
Looks like Avi is right about the overshoot. Can you test something like this?
arch/x86/kernel/cpu/perf_event_intel.c +4 -0
index 166546e..5fb371a 100644
@@ -1374,8 +1374,11 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr)
 	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;
 	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
 	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
+	arr[1].msr = MSR_IA32_PEBS_ENABLE;
+	arr[1].host = cpuc->pebs_enabled;
+	arr[1].guest = 0;
+	*nr = 2;
 
Quoted 4 lines
Quoted 4 lines
arch/x86/kernel/cpu/perf_event_intel.c static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr) · line 1383
Quoted 3 lines
arch/x86/kernel/cpu/perf_event_intel.c +0 -1
-	*nr = 1;
 	return arr;
 }
So far the 64-bit Fedora 10 VM with both a Fedora 10 stock kernel and a 2.6.38 kernel have not faired well - and that's the only VM I have tried at the moment. Using -e cycles:pp I have been able to lock up the VM 3 times out of 3 series of tests with perf-kvm that includes network traffic (e.g., netperf), disk I/O (dd based to create a file with dsync flag) and pure userspace cpu bound (openssl speed). May or may not be related.

Quoted 6 lines

Quoted 2 lines
OK that's may be BTSes. What about -e cycles:p? BTW are you using your patch to set exclude_guest parameter? If not use -e cycles:Hp.
I started with cycles:pp; should not really matter - they all need to work without blowing up VMs (cycles:p, cycles:pH, cycles:pG, cycles:pp, cycles:ppH, cycles:ppG).

cycles:ppG and cycles:pG should be illegal. Peter's patch takes care of this. Others should set exclude guest and have to work without blowing up VMs.

Quoted 7 lines

For grins I ran a quick test while reading emails this morning. This time a fedora 17 VM with 3.4.0-1.fc17.x86_64 kernel. It too locks up pretty quickly - just a couple of iterations of perf:
perf kvm --guestmount=/tmp/guest-mount record -fo /tmp/perf.data -a -v -e cycles:pH -- sleep 60

Do not run perf kvm. It does not set exclude_guest and :p and :pp is not compatible with guest profiling and should be disallowed. Again Peter's patch takes care of this.

Run perf top -e cycles:pH or similar.

Quoted 2 lines

Note the :pH this time.

I am not sure what perf kvm does with :pH modifier, but H modifier does not make sense with perf kvm and should be reported as an error by perf tool.

Quoted 9 lines

I did not have netserver installed in the VM so used a ping flood for network traffic.
Quoted 5 lines
Quoted 5 lines
Also, I noted that 'perf kvm --guest record -e cycles:pp' does not generate a whole lot of samples -- like < 100 in a 20-second sample -- despite the fact that the guest is rather busy.

Quoted 6 lines

Quoted 1 line
Host events do not suppose to generate events while guest is running.
My server has 16 cpus and the VM has only 2 vcpus; with the -a I would expect some host sampling. Note: in the above case :pp resets the exclude-host modifier set by the perf-kvm part, so hosts samples are not excluded. See parse_events_modifier().

Isn't this a bug? Why anything at all resets exclude-host set by perf-kvm?

Quoted 3 lines

So, is the idea of your patch to not enable the PEBS in guest mode?

Yes, the idea of my patch is to disable PEBS and the guest entry.

--
Gleb.

Gleb Natapov

2012-07-12 16:14 UTC #20 lore raw

Quoted 5 lines · Gleb Natapov

Quoted 3 lines
So, is the idea of your patch to not enable the PEBS in guest mode?
Yes, the idea of my patch is to disable PEBS and the guest entry.

BTW what is you host cpu? My patch works only for those that have global control register to disable PMU events, but that should cover most of moder cpus.

--
Gleb.

Peter Zijlstra

maintainer

2012-07-12 16:58 UTC #21 lore raw

Quoted 3 lines · Gleb Natapov

BTW what is you host cpu? My patch works only for those that have global control register to disable PMU events, but that should cover most of moder cpus.

That covers everything but core and p4, and I don't think we support PEBS on either of those.

that said, you might want to also clear EN on the individual counters, just in case the PEBS clear and DS_AREA clear aren't working.

The SDM is pretty clear on what you _should_ do to enable/disable PEBS, doing less _might_ work but...

Avi Kivity

2012-07-15 08:07 UTC #23 lore raw

Quoted 4 lines · Gleb Natapov

Quoted 2 lines
Note the :pH this time.
I am not sure what perf kvm does with :pH modifier, but H modifier does not make sense with perf kvm and should be reported as an error by perf tool.

Maybe it should refer to the guest vs. the nested guest...

Well that's a horrible UI but we do need some way to distinguish among the two. Samples from the nested guest's kernel will make no sense when looking at the guest kernel. I guess we should default to exclude nested guest mode.

Signature · 1 line

error compiling committee.c: too many arguments to function

David Ahern

author

2012-07-15 13:00 UTC #24 lore raw

Quoted 7 lines · Avi Kivity

Quoted 4 lines · Gleb Natapov
Quoted 2 lines
Note the :pH this time.
I am not sure what perf kvm does with :pH modifier, but H modifier does not make sense with perf kvm and should be reported as an error by perf tool.
Maybe it should refer to the guest vs. the nested guest...

:H = host mode; you are thinking of :h for hypervisor mode. From perf-list documentation:

"Modifiers allow the user to restrict when events are counted with 'u' for user-space, 'k' for kernel, 'h' for hypervisor. Additional modifiers are 'G' for guest counting (in KVM guests) and 'H' for host counting (not in KVM guests)."

Quoted 6 lines

Well that's a horrible UI but we do need some way to distinguish among the two. Samples from the nested guest's kernel will make no sense when looking at the guest kernel. I guess we should default to exclude nested guest mode.

I catch a return flight in a couple of hours. I will send a summary email tonight to make sure we are on the same page with the problem, uses cases and patches.

David

Avi Kivity

2012-07-15 13:03 UTC #25 lore raw

Quoted 10 lines · David Ahern

Quoted 8 lines · Avi Kivity
Quoted 5 lines · Gleb Natapov
Quoted 2 lines
Note the :pH this time.
I am not sure what perf kvm does with :pH modifier, but H modifier does not make sense with perf kvm and should be reported as an error by perf tool.
Maybe it should refer to the guest vs. the nested guest...

Quoted 7 lines

:H = host mode; you are thinking of :h for hypervisor mode. From perf-list documentation:
"Modifiers allow the user to restrict when events are counted with 'u' for user-space, 'k' for kernel, 'h' for hypervisor. Additional modifiers are 'G' for guest counting (in KVM guests) and 'H' for host counting (not in KVM guests)."

No, it's an additional distinction. A kvm guest can be in kernel mode, user mode, or in a nested guest mode (which has its own user mode and kernel mode). Currently we have no way of distinguishing between guest kernel mode and nested guest kernel mode.

I assume 'h' means profiling the hypervisor from a guest (i.e. xen dom0)?

Signature · 1 line

error compiling committee.c: too many arguments to function

David Ahern

author

2012-07-16 01:52 UTC #22 lore raw

Quoted 9 lines · Gleb Natapov

Quoted 5 lines · Gleb Natapov
Quoted 3 lines
So, is the idea of your patch to not enable the PEBS in guest mode?
Yes, the idea of my patch is to disable PEBS and the guest entry.
BTW what is you host cpu? My patch works only for those that have global control register to disable PMU events, but that should cover most of moder cpus.

So far all tests have been done on an HP with 2 Nehalem processors (E5540); I have another server with a couple of Westmere's (E5620) that I will test at some point.

David

David Ahern

author

2012-07-16 01:52 UTC #26 lore raw

Quoted 1 line · Avi Kivity

I assume 'h' means profiling the hypervisor from a guest (i.e. xen dom0)?

I do not think so. It does not appear to do anything on x86.

David Ahern

author

2012-07-16 02:20 UTC #27 lore raw

Quoted 6 lines · Gleb Natapov

Quoted 3 lines
I started with cycles:pp; should not really matter - they all need to work without blowing up VMs (cycles:p, cycles:pH, cycles:pG, cycles:pp, cycles:ppH, cycles:ppG).
cycles:ppG and cycles:pG should be illegal. Peter's patch takes care of this. Others should set exclude guest and have to work without blowing up VMs.

I was only testing your patch; I'll add Peter's going forward.

---8<---

Quoted 5 lines

Quoted 2 lines
perf kvm --guestmount=/tmp/guest-mount record -fo /tmp/perf.data -a -v -e cycles:pH -- sleep 60
Do not run perf kvm. It does not set exclude_guest and :p and :pp is not compatible with guest profiling and should be disallowed. Again Peter's patch takes care of this.

Well, perf-kvm needs to work too - and yes you can set exclude_guest with it (doesn't make sense to use perf-kvm with exclude_guest, but then VMs should not die because of it). If someone adds -e <hardware event>:p{p} it needs to be handled cleanly. The G and H event modifiers can be used explicitly to exclude host or guest and that case too needs to be handled properly from whatever the context is.

Peter's patch handles kernel side checks requiring exclude_guest if the pebs is to be used. A user running perf-kvm, perf-record and perf-top deserves something better than a confusing 'not supported error' which includes a message suggesting perf events is not enabled. I have some local patches for record and top to cover this.

Quoted 7 lines

Run perf top -e cycles:pH or similar.
Quoted 2 lines
Note the :pH this time.
I am not sure what perf kvm does with :pH modifier, but H modifier does not make sense with perf kvm and should be reported as an error by perf tool.

perf-kvm passes everything from record on to the builtin record command. So, 'perf kvm record -e cycles:pH' effectively becomes 'perf record -e cycles:pH' with 2 global flags modified (perf_guest set and perf_host unset). There is nothing invalid about the perf-kvm command.

---8<---

Quoted 6 lines

Quoted 4 lines
My server has 16 cpus and the VM has only 2 vcpus; with the -a I would expect some host sampling. Note: in the above case :pp resets the exclude-host modifier set by the perf-kvm part, so hosts samples are not excluded. See parse_events_modifier().
Isn't this a bug? Why anything at all resets exclude-host set by perf-kvm?

Arguably yes and arguably no. As with any linux command later arguments can unset/reset earlier ones. In this case -e <event>:<modifier> comes after the perf-kvm --guest --host settings. I can whip up a patch for perf {record,top} to spit out a debug message if perf_{guest,host} are not consistent with excl_{guest,host} if that's wanted.

Don't forget -- perf-kvm has the --host flag to enable host side monitoring and --no-guest can be used to disable guest side monitoring.

David

David Ahern

author Tested-by

2012-07-20 23:34 UTC #28 lore raw

Quoted 8 lines · Gleb Natapov

Looks like Avi is right about the overshoot. Can you test something like this?
arch/x86/kernel/cpu/perf_event_intel.c +0 -0
index 166546e..5fb371a 100644
@@ -1374,8 +1374,11 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr)
  	arr[0].msr = MSR_CORE_PERF_GLOBAL_CTRL;

arch/x86/kernel/cpu/perf_event_intel.c static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr) · line 1383

Quoted 10 lines

arch/x86/kernel/cpu/perf_event_intel.c +4 -1

  	arr[0].host = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
  	arr[0].guest = x86_pmu.intel_ctrl & ~cpuc->intel_ctrl_host_mask;
+	arr[1].msr = MSR_IA32_PEBS_ENABLE;
+	arr[1].host = cpuc->pebs_enabled;
+	arr[1].guest = 0;
+	*nr = 2;
 
-	*nr = 1;
  	return arr;
  }

Tested-by David Ahern <[email protected]>

Peter's patch is required as well. It's the combination that fixes the problem.

David

David Ahern

author Tested-by

2012-07-20 23:35 UTC #34 lore raw

Quoted 27 lines · Peter Zijlstra

Quoted 5 lines · Avi Kivity
Quoted 1 line
Disabling PEBS events for guests isn't pretty though..
We already have atomic MSR switching at guest entry/exit time. So it's not pretty in terms of not getting full profiling, but the code won't be too hard. Basically we just have to exclude_guest any pebs event.
OK, so ideally we'd do something like the below, except of course that that isn't backwards compatible and will break the world :/
bugger that
---
arch/x86/kernel/cpu/perf_event.c +3 -0
@@ -336,6 +338,9 @@ int x86_setup_perfctr(struct perf_event *event)
                 /* BTS is currently only allowed for user-mode. */
                 if (!attr->exclude_kernel)
                         return -EOPNOTSUPP;
+
+               if (!attr->exclude_guest)
+                       return -EOPNOTSUPP;
         }
 
         hwc->config |= config;

arch/x86/kernel/cpu/perf_event.c int x86_pmu_hw_config(struct perf_event *event) · line 391

Quoted 9 lines

arch/x86/kernel/cpu/perf_event.c +3 -0

@@ -378,6 +383,9 @@ int x86_pmu_hw_config(struct perf_event *event)
         if (event->attr.precise_ip) {
                 int precise = 0;
 
+               if (!event->attr.exclude_guest)
+                       return -EOPNOTSUPP;
+
                 /* Support for constant skid */
                 if (x86_pmu.pebs_active && !x86_pmu.pebs_broken) {
                         precise++;

Tested-by David Ahern <[email protected]>

Gleb's patch is required as well. It takes both to fix the problem. And my userspace patch keeps compatibility for users who currently use -e cycles:pp (they won't be forced to add 'H'). (lkml is slow to show it so I can't pull a link for reference, but it's in your email ahead of this one).

Be nice to get this set into stable releases as well.

Avi Kivity

2012-07-22 09:53 UTC #29 lore raw