[PATCH 0/2] perf/x86/intel/pt: VMX related updates

[PATCH 0/2] perf/x86/intel/pt: VMX related updates

[Alexander Shishkin]

Hi Peter,

On systems where PT does not coexist with VMX operation, we silently stop
scheduling in PT events while VMX is on. It is confusing to the user as
what they get in return is a perf session with no PT trace. So I added a
check for any preexisting VMX users to the event creation path, so that we
can tell the user right off the bat that it's not going to work (2/2). Also,
when we do end up with PT events and VMX coexisting, the scheduling needs to
be aware of it (1/2).

Alexander Shishkin (2):
  perf/x86/intel/pt: Fail event scheduling on conflict with VMX
  perf/x86/intel/pt: Fail event creation if VMX operation is on

 arch/x86/events/intel/pt.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

-- 
2.11.0

[PATCH 1/2] perf/x86/intel/pt: Fail event scheduling on conflict with VMX

[Alexander Shishkin]

At the moment, if VMX operation prevents PT tracing, the PMU will
silently return success to the event scheduling code, which will
track its 'on' time, etc. Instead, report failure so that perf
core knows this event is not actually on.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reported-by: Andi Kleen <ak@linux.intel.com>
Fixes: 1c5ac21a0e ("perf/x86/intel/pt: Don't die on VMXON")
---
 arch/x86/events/intel/pt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index d92a60ef08..9372fa4549 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1335,7 +1335,7 @@ static void pt_event_start(struct perf_event *event, int mode)
 	struct pt_buffer *buf;
 
 	if (READ_ONCE(pt->vmx_on))
-		return;
+		goto fail_stop;
 
 	buf = perf_aux_output_begin(&pt->handle, event);
 	if (!buf)
-- 
2.11.0

Re: [PATCH 1/2] perf/x86/intel/pt: Fail event scheduling on conflict with VMX

[Peter Zijlstra]

On Tue, Feb 14, 2017 at 03:24:15PM +0200, Alexander Shishkin wrote:
> At the moment, if VMX operation prevents PT tracing, the PMU will
> silently return success to the event scheduling code, which will
> track its 'on' time, etc. Instead, report failure so that perf
> core knows this event is not actually on.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Reported-by: Andi Kleen <ak@linux.intel.com>
> Fixes: 1c5ac21a0e ("perf/x86/intel/pt: Don't die on VMXON")
> ---
>  arch/x86/events/intel/pt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> index d92a60ef08..9372fa4549 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c
> @@ -1335,7 +1335,7 @@ static void pt_event_start(struct perf_event *event, int mode)
>  	struct pt_buffer *buf;
>  
>  	if (READ_ONCE(pt->vmx_on))
> -		return;
> +		goto fail_stop;
>  
>  	buf = perf_aux_output_begin(&pt->handle, event);
>  	if (!buf)

I'm not getting it; how does this matter to the time tracking in
event_sched_in() / event_sched_out() ?

That looks at event->state == PERF_EVENT_STATE*

This goto affects event->hw.state == PERF_HES_

The core assumes ->start() will _NOT_ fail.

Re: [PATCH 1/2] perf/x86/intel/pt: Fail event scheduling on conflict with VMX

[Alexander Shishkin]

Peter Zijlstra <peterz@infradead.org> writes:

> On Tue, Feb 14, 2017 at 03:24:15PM +0200, Alexander Shishkin wrote:
>> At the moment, if VMX operation prevents PT tracing, the PMU will
>> silently return success to the event scheduling code, which will
>> track its 'on' time, etc. Instead, report failure so that perf
>> core knows this event is not actually on.
>> 
>> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Reported-by: Andi Kleen <ak@linux.intel.com>
>> Fixes: 1c5ac21a0e ("perf/x86/intel/pt: Don't die on VMXON")
>> ---
>>  arch/x86/events/intel/pt.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
>> index d92a60ef08..9372fa4549 100644
>> --- a/arch/x86/events/intel/pt.c
>> +++ b/arch/x86/events/intel/pt.c
>> @@ -1335,7 +1335,7 @@ static void pt_event_start(struct perf_event *event, int mode)
>>  	struct pt_buffer *buf;
>>  
>>  	if (READ_ONCE(pt->vmx_on))
>> -		return;
>> +		goto fail_stop;
>>  
>>  	buf = perf_aux_output_begin(&pt->handle, event);
>>  	if (!buf)
>
> I'm not getting it; how does this matter to the time tracking in
> event_sched_in() / event_sched_out() ?
>
> That looks at event->state == PERF_EVENT_STATE*
>
> This goto affects event->hw.state == PERF_HES_
>
> The core assumes ->start() will _NOT_ fail.

This is called by pmu::add(), which checks hw.state afterwards and if it
finds HES_STOPPED, it returns an error, which event_sched_in() captures
and keeps the event in INACTIVE state. Should I add a comment about it?

Regards,
--
Alex

Re: [PATCH 1/2] perf/x86/intel/pt: Fail event scheduling on conflict with VMX

[Peter Zijlstra]

On Tue, Feb 14, 2017 at 06:17:30PM +0200, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > On Tue, Feb 14, 2017 at 03:24:15PM +0200, Alexander Shishkin wrote:
> >> At the moment, if VMX operation prevents PT tracing, the PMU will
> >> silently return success to the event scheduling code, which will
> >> track its 'on' time, etc. Instead, report failure so that perf
> >> core knows this event is not actually on.
> >> 
> >> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >> Reported-by: Andi Kleen <ak@linux.intel.com>
> >> Fixes: 1c5ac21a0e ("perf/x86/intel/pt: Don't die on VMXON")
> >> ---
> >>  arch/x86/events/intel/pt.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> >> index d92a60ef08..9372fa4549 100644
> >> --- a/arch/x86/events/intel/pt.c
> >> +++ b/arch/x86/events/intel/pt.c
> >> @@ -1335,7 +1335,7 @@ static void pt_event_start(struct perf_event *event, int mode)
> >>  	struct pt_buffer *buf;
> >>  
> >>  	if (READ_ONCE(pt->vmx_on))
> >> -		return;
> >> +		goto fail_stop;
> >>  
> >>  	buf = perf_aux_output_begin(&pt->handle, event);
> >>  	if (!buf)
> >
> > I'm not getting it; how does this matter to the time tracking in
> > event_sched_in() / event_sched_out() ?
> >
> > That looks at event->state == PERF_EVENT_STATE*
> >
> > This goto affects event->hw.state == PERF_HES_
> >
> > The core assumes ->start() will _NOT_ fail.
> 
> This is called by pmu::add(), which checks hw.state afterwards and if it
> finds HES_STOPPED, it returns an error, which event_sched_in() captures
> and keeps the event in INACTIVE state. Should I add a comment about it?

Egads... so what if ->add() succeeds but we then hit this on
->stop()/->start() due to throttle or period adjust?

Now I suppose PT will never normally hit either of those, but you can do
IOC_PERIOD on it, just for giggles.

Yes, this very much needs a comment... Also, should not this then live
in ->add() in the first place?

Re: [PATCH 1/2] perf/x86/intel/pt: Fail event scheduling on conflict with VMX

[Alexander Shishkin]

Peter Zijlstra <peterz@infradead.org> writes:

> On Tue, Feb 14, 2017 at 06:17:30PM +0200, Alexander Shishkin wrote:
>> This is called by pmu::add(), which checks hw.state afterwards and if it
>> finds HES_STOPPED, it returns an error, which event_sched_in() captures
>> and keeps the event in INACTIVE state. Should I add a comment about it?
>
> Egads... so what if ->add() succeeds but we then hit this on
> ->stop()/->start() due to throttle or period adjust?

It will hang there with hw.state==PERF_HES_STOPPED till the next
sched_out. But that will be the case anyway if VMXON kicks in while PT
is running.

> Now I suppose PT will never normally hit either of those, but you can do
> IOC_PERIOD on it, just for giggles.

True. Should we worry?

> Yes, this very much needs a comment... Also, should not this then live
> in ->add() in the first place?

The output_stop() stuff in the mmap_close path relies on
perf_aux_output_end() being in ->stop(), let me think if there's more.

Regards,
--
Alex

Re: [PATCH 1/2] perf/x86/intel/pt: Fail event scheduling on conflict with VMX

[Peter Zijlstra]

On Tue, Feb 14, 2017 at 07:21:34PM +0200, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > On Tue, Feb 14, 2017 at 06:17:30PM +0200, Alexander Shishkin wrote:
> >> This is called by pmu::add(), which checks hw.state afterwards and if it
> >> finds HES_STOPPED, it returns an error, which event_sched_in() captures
> >> and keeps the event in INACTIVE state. Should I add a comment about it?
> >
> > Egads... so what if ->add() succeeds but we then hit this on
> > ->stop()/->start() due to throttle or period adjust?
> 
> It will hang there with hw.state==PERF_HES_STOPPED till the next
> sched_out. But that will be the case anyway if VMXON kicks in while PT
> is running.

Right, so I question the whole 'lets not schedule PT when VMX' premise,
it leads to inconsistencies all over. How about we treat it like
->add() succeeded and VMX simply results in no output.

Esp. when you then emit 'fake' data into/from a vmlaunch/vmresume
instruction.

Re: [PATCH 1/2] perf/x86/intel/pt: Fail event scheduling on conflict with VMX

[Peter Zijlstra]

On Tue, Feb 14, 2017 at 07:38:07PM +0100, Peter Zijlstra wrote:

> Right, so I question the whole 'lets not schedule PT when VMX' premise,
> it leads to inconsistencies all over. How about we treat it like
> ->add() succeeded and VMX simply results in no output.
> 
> Esp. when you then emit 'fake' data into/from a vmlaunch/vmresume
> instruction.

That is, what about something like the below? (completely untested for
obvious raisins).

That should schedule PT like normal, and where VMXON will auto-clear
TraceEn for us, we make VMXOFF set it again.

---
 arch/x86/events/intel/pt.c | 38 ++++++++++++++++++++++++++------------
 arch/x86/events/intel/pt.h |  1 +
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 5900471ee508..a42fa1bef761 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -411,6 +411,7 @@ static u64 pt_config_filters(struct perf_event *event)
 
 static void pt_config(struct perf_event *event)
 {
+	struct pt *pt = this_cpu_ptr(&pt_ctx);
 	u64 reg;
 
 	if (!event->hw.itrace_started) {
@@ -429,11 +430,14 @@ static void pt_config(struct perf_event *event)
 	reg |= (event->attr.config & PT_CONFIG_MASK);
 
 	event->hw.config = reg;
-	wrmsrl(MSR_IA32_RTIT_CTL, reg);
+
+	if (!pt->vmx_on)
+		wrmsrl(MSR_IA32_RTIT_CTL, reg);
 }
 
 static void pt_config_stop(struct perf_event *event)
 {
+	struct pt *pt = this_cpu_ptr(&pt_ctx);
 	u64 ctl = READ_ONCE(event->hw.config);
 
 	/* may be already stopped by a PMI */
@@ -441,7 +445,9 @@ static void pt_config_stop(struct perf_event *event)
 		return;
 
 	ctl &= ~RTIT_CTL_TRACEEN;
-	wrmsrl(MSR_IA32_RTIT_CTL, ctl);
+
+	if (!pt->vmx_on)
+		wrmsrl(MSR_IA32_RTIT_CTL, ctl);
 
 	WRITE_ONCE(event->hw.config, ctl);
 
@@ -1174,10 +1180,12 @@ void intel_pt_interrupt(void)
 	/*
 	 * If VMX is on and PT does not support it, don't touch anything.
 	 */
-	if (READ_ONCE(pt->vmx_on))
+	if (READ_ONCE(pt->vmx_on)) {
+		WRITE_ONCE(pt->vmx_pmi_pending, 1);
 		return;
+	}
 
-	if (!event)
+	if (WARN_ON(!event)) /* should be set if handle_nmi */
 		return;
 
 	pt_config_stop(event);
@@ -1236,13 +1244,22 @@ void intel_pt_handle_vmx(int on)
 	 */
 	local_irq_save(flags);
 	WRITE_ONCE(pt->vmx_on, on);
+	if (on)
+		goto done;
 
-	if (on) {
-		/* prevent pt_config_stop() from writing RTIT_CTL */
-		event = pt->handle.event;
-		if (event)
-			event->hw.config = 0;
+	/* OTOH, if we just did VMXOFF, we need to set TraceEn again */
+	event = pt->handle.event;
+	if (!event)
+		goto done;
+
+	if (pt->vmx_pmi_pending) {
+		intel_pt_interrupt();
+		pt->vmx_pmi_pending = 0;
+	} else {
+		wrmsrl(MSR_IA32_RTIT_CTL, event->hw.config);
 	}
+
+done:
 	local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(intel_pt_handle_vmx);
@@ -1257,9 +1274,6 @@ static void pt_event_start(struct perf_event *event, int mode)
 	struct pt *pt = this_cpu_ptr(&pt_ctx);
 	struct pt_buffer *buf;
 
-	if (READ_ONCE(pt->vmx_on))
-		return;
-
 	buf = perf_aux_output_begin(&pt->handle, event);
 	if (!buf)
 		goto fail_stop;
diff --git a/arch/x86/events/intel/pt.h b/arch/x86/events/intel/pt.h
index 53473c21b554..98ed385e187b 100644
--- a/arch/x86/events/intel/pt.h
+++ b/arch/x86/events/intel/pt.h
@@ -187,6 +187,7 @@ struct pt {
 	struct pt_filters	filters;
 	int			handle_nmi;
 	int			vmx_on;
+	int			vmx_pmi_pending;
 };
 
 #endif /* __INTEL_PT_H__ */

Re: [PATCH 1/2] perf/x86/intel/pt: Fail event scheduling on conflict with VMX

[Alexander Shishkin]

Peter Zijlstra <peterz@infradead.org> writes:

> On Tue, Feb 14, 2017 at 07:38:07PM +0100, Peter Zijlstra wrote:
>
>> Right, so I question the whole 'lets not schedule PT when VMX' premise,
>> it leads to inconsistencies all over. How about we treat it like
>> ->add() succeeded and VMX simply results in no output.
>> 
>> Esp. when you then emit 'fake' data into/from a vmlaunch/vmresume
>> instruction.
>
> That is, what about something like the below? (completely untested for
> obvious raisins).
>
> That should schedule PT like normal, and where VMXON will auto-clear
> TraceEn for us, we make VMXOFF set it again.

This makes sense, but this will only make a difference if we're tracing
(the kernel side of) the process that did VMXON in the first place and
we want to see what happens immediately after VMXOFF.

>
> ---
>  arch/x86/events/intel/pt.c | 38 ++++++++++++++++++++++++++------------
>  arch/x86/events/intel/pt.h |  1 +
>  2 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> index 5900471ee508..a42fa1bef761 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c
> @@ -411,6 +411,7 @@ static u64 pt_config_filters(struct perf_event *event)
>  
>  static void pt_config(struct perf_event *event)
>  {
> +	struct pt *pt = this_cpu_ptr(&pt_ctx);
>  	u64 reg;
>  
>  	if (!event->hw.itrace_started) {
> @@ -429,11 +430,14 @@ static void pt_config(struct perf_event *event)
>  	reg |= (event->attr.config & PT_CONFIG_MASK);
>  
>  	event->hw.config = reg;
> -	wrmsrl(MSR_IA32_RTIT_CTL, reg);
> +
> +	if (!pt->vmx_on)
> +		wrmsrl(MSR_IA32_RTIT_CTL, reg);
>  }
>  
>  static void pt_config_stop(struct perf_event *event)
>  {
> +	struct pt *pt = this_cpu_ptr(&pt_ctx);
>  	u64 ctl = READ_ONCE(event->hw.config);
>  
>  	/* may be already stopped by a PMI */
> @@ -441,7 +445,9 @@ static void pt_config_stop(struct perf_event *event)
>  		return;
>  
>  	ctl &= ~RTIT_CTL_TRACEEN;
> -	wrmsrl(MSR_IA32_RTIT_CTL, ctl);
> +
> +	if (!pt->vmx_on)
> +		wrmsrl(MSR_IA32_RTIT_CTL, ctl);
>  
>  	WRITE_ONCE(event->hw.config, ctl);
>  
> @@ -1174,10 +1180,12 @@ void intel_pt_interrupt(void)
>  	/*
>  	 * If VMX is on and PT does not support it, don't touch anything.
>  	 */
> -	if (READ_ONCE(pt->vmx_on))
> +	if (READ_ONCE(pt->vmx_on)) {
> +		WRITE_ONCE(pt->vmx_pmi_pending, 1);
>  		return;
> +	}

This is even simpler: we actually need to carry out the first part of
the interrupt function anyway, which deals with updating buffer pointers
etc, thus "handling" the PMI, but we don't restart the event, which will
be then done by the intel_pt_handle_vmx(0), so we don't need the
pending_pmi thingy.

Now the fake data is worrying me much more. Consider this: we start an
event while ->vmx_on==1, which means that before we write a fake VMCS
packet, we need to write a whole bunch of other packets to establish the
context with synchronization point and kitchen sink (PSB..PSBEND), then
fake a trace start TIP.PGE, then VMCS, then TIP.PGD. Then, we should
remember that we did this once to not do it again on every sched-in.

Another corner case is when there's not enough room in the buffer and we
need to postpone the fake VMCS until there is room again. Let me see if
there's more.

Regards,
--
Alex

Re: [PATCH 1/2] perf/x86/intel/pt: Fail event scheduling on conflict with VMX

[Peter Zijlstra]

On Wed, Feb 15, 2017 at 10:11:41AM +0200, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > On Tue, Feb 14, 2017 at 07:38:07PM +0100, Peter Zijlstra wrote:
> >
> >> Right, so I question the whole 'lets not schedule PT when VMX' premise,
> >> it leads to inconsistencies all over. How about we treat it like
> >> ->add() succeeded and VMX simply results in no output.
> >> 
> >> Esp. when you then emit 'fake' data into/from a vmlaunch/vmresume
> >> instruction.
> >
> > That is, what about something like the below? (completely untested for
> > obvious raisins).
> >
> > That should schedule PT like normal, and where VMXON will auto-clear
> > TraceEn for us, we make VMXOFF set it again.
> 
> This makes sense, but this will only make a difference if we're tracing
> (the kernel side of) the process that did VMXON in the first place and
> we want to see what happens immediately after VMXOFF.

So I got annoyed by the inconsistencies in state depending on timing;
and this mucking about with pretending the event is not scheduled and
shouldn't be counted running when VMX when it bloody well is. It just so
happens it doesn't generate data.

Then I got worried when acme said he needed to kill all his VMs before
using PT; that's just wrong. You should just get no data for the VM but
the rest should very much show up.

And then when I looked at this, I couldn't figure out who would ever
re-enable PT for CPU events. VMXON will clear TraceEn, nobody will ever
set it again, because we generally don't schedule CPU events if we don't
have to.

Hence this proposal that should clear all that up.

> > @@ -1174,10 +1180,12 @@ void intel_pt_interrupt(void)
> >  	/*
> >  	 * If VMX is on and PT does not support it, don't touch anything.
> >  	 */
> > -	if (READ_ONCE(pt->vmx_on))
> > +	if (READ_ONCE(pt->vmx_on)) {
> > +		WRITE_ONCE(pt->vmx_pmi_pending, 1);
> >  		return;
> > +	}
> 
> This is even simpler: we actually need to carry out the first part of
> the interrupt function anyway, which deals with updating buffer pointers
> etc, thus "handling" the PMI, but we don't restart the event, which will
> be then done by the intel_pt_handle_vmx(0), so we don't need the
> pending_pmi thingy.

Indeed! Make it so ;-)

> Now the fake data is worrying me much more. Consider this: we start an
> event while ->vmx_on==1, which means that before we write a fake VMCS
> packet, we need to write a whole bunch of other packets to establish the
> context with synchronization point and kitchen sink (PSB..PSBEND), then
> fake a trace start TIP.PGE, then VMCS, then TIP.PGD. Then, we should
> remember that we did this once to not do it again on every sched-in.
> 
> Another corner case is when there's not enough room in the buffer and we
> need to postpone the fake VMCS until there is room again. Let me see if
> there's more.

I suppose the alternative is issuing PERF_RECORD_AUX events indicating
paused output or something.

[PATCH 2/2] perf/x86/intel/pt: Fail event creation if VMX operation is on

[Alexander Shishkin]

On systems where PT does not coexist with VMX, users get confused when
PT turns up with no data because they forgot they're running a kvm
session at the same time.

This patch adds a preemptive check for any active VMX operations that
will fail event creation. This does not provide any guarantees or
protection against racing with a kvm starting in parallel, but is
intended to serve as a hint for the user. If VMXON happens after an
event had been created, the event will still produce an empty trace.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reported-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/events/intel/pt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 9372fa4549..b1490a879c 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -1444,6 +1444,20 @@ static void pt_event_destroy(struct perf_event *event)
 
 static int pt_event_init(struct perf_event *event)
 {
+	int cpu, vmx_on = 0;
+
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		struct pt *pt = per_cpu_ptr(&pt_ctx, cpu);
+
+		if (READ_ONCE(pt->vmx_on))
+			vmx_on++;
+	}
+	put_online_cpus();
+
+	if (vmx_on)
+		return -EBUSY;
+
 	if (event->attr.type != pt_pmu.pmu.type)
 		return -ENOENT;
 
-- 
2.11.0

Re: [PATCH 2/2] perf/x86/intel/pt: Fail event creation if VMX operation is on

[Peter Zijlstra]

On Tue, Feb 14, 2017 at 03:24:16PM +0200, Alexander Shishkin wrote:
> On systems where PT does not coexist with VMX, users get confused when
> PT turns up with no data because they forgot they're running a kvm
> session at the same time.
> 
> This patch adds a preemptive check for any active VMX operations that
> will fail event creation. This does not provide any guarantees or
> protection against racing with a kvm starting in parallel, but is
> intended to serve as a hint for the user. If VMXON happens after an
> event had been created, the event will still produce an empty trace.
> 

Would it not be more sensible to write fake PT packets in
intel_pt_handle_vmx() to demarcate VMX regions in the trace?

Re: [PATCH 2/2] perf/x86/intel/pt: Fail event creation if VMX operation is on

[Arnaldo Carvalho de Melo]

Em Tue, Feb 14, 2017 at 03:24:16PM +0200, Alexander Shishkin escreveu:
> On systems where PT does not coexist with VMX, users get confused when
> PT turns up with no data because they forgot they're running a kvm
> session at the same time.
> 
> This patch adds a preemptive check for any active VMX operations that
> will fail event creation. This does not provide any guarantees or
> protection against racing with a kvm starting in parallel, but is
> intended to serve as a hint for the user. If VMXON happens after an
> event had been created, the event will still produce an empty trace.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Reported-by: Andi Kleen <ak@linux.intel.com>

Yeah, I saw that as well, and Andi told me about this limitation, so,
for quite a while now, everytime I need to test PT on the only machine I
have with it, I have to stop my kvm sessions :-\

Thanks for working on this!

- Arnaldo

> ---
>  arch/x86/events/intel/pt.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> index 9372fa4549..b1490a879c 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c
> @@ -1444,6 +1444,20 @@ static void pt_event_destroy(struct perf_event *event)
>  
>  static int pt_event_init(struct perf_event *event)
>  {
> +	int cpu, vmx_on = 0;
> +
> +	get_online_cpus();
> +	for_each_online_cpu(cpu) {
> +		struct pt *pt = per_cpu_ptr(&pt_ctx, cpu);
> +
> +		if (READ_ONCE(pt->vmx_on))
> +			vmx_on++;
> +	}
> +	put_online_cpus();
> +
> +	if (vmx_on)
> +		return -EBUSY;
> +
>  	if (event->attr.type != pt_pmu.pmu.type)
>  		return -ENOENT;
>  
> -- 
> 2.11.0

Re: [PATCH 2/2] perf/x86/intel/pt: Fail event creation if VMX operation is on

[Alexander Shishkin]

Arnaldo Carvalho de Melo <acme@kernel.org> writes:

> Em Tue, Feb 14, 2017 at 03:24:16PM +0200, Alexander Shishkin escreveu:
>> On systems where PT does not coexist with VMX, users get confused when
>> PT turns up with no data because they forgot they're running a kvm
>> session at the same time.
>> 
>> This patch adds a preemptive check for any active VMX operations that
>> will fail event creation. This does not provide any guarantees or
>> protection against racing with a kvm starting in parallel, but is
>> intended to serve as a hint for the user. If VMXON happens after an
>> event had been created, the event will still produce an empty trace.
>> 
>> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Reported-by: Andi Kleen <ak@linux.intel.com>
>
> Yeah, I saw that as well, and Andi told me about this limitation, so,
> for quite a while now, everytime I need to test PT on the only machine I
> have with it, I have to stop my kvm sessions :-\
>
> Thanks for working on this!

Well, we can't make the limitation go away. You'll still have to stop
kvms to get any 'meaningful' PT data.

Regards,
--
Alex

Re: [PATCH 2/2] perf/x86/intel/pt: Fail event creation if VMX operation is on

[Peter Zijlstra]

On Wed, Feb 15, 2017 at 10:34:58AM +0200, Alexander Shishkin wrote:

> Well, we can't make the limitation go away. You'll still have to stop
> kvms to get any 'meaningful' PT data.

Why would you need to stop all VMs in order to get your !VM data? Sure,
you get black holes where the VM runs, but we should be able to see
everything else.

Re: [PATCH 2/2] perf/x86/intel/pt: Fail event creation if VMX operation is on

[Alexander Shishkin]

Peter Zijlstra <peterz@infradead.org> writes:

> On Wed, Feb 15, 2017 at 10:34:58AM +0200, Alexander Shishkin wrote:
>
>> Well, we can't make the limitation go away. You'll still have to stop
>> kvms to get any 'meaningful' PT data.
>
> Why would you need to stop all VMs in order to get your !VM data? Sure,
> you get black holes where the VM runs, but we should be able to see
> everything else.

No, what I mean is that if you run kvm prior to starting perf record,
which I assume is the case for acme, your entire session is a black
hole. The VMXON happens pretty early on, you can open /dev/kvm,
ioctl(KVM_CREATE_VM) on it and that will do a VMXON already.

The problem is that PT (on BDW) doesn't trace inside VM root mode, not
just between VM entry/VM exit.

Regards,
--
Alex

Re: [PATCH 2/2] perf/x86/intel/pt: Fail event creation if VMX operation is on

[Arnaldo Carvalho de Melo]

Em Wed, Feb 15, 2017 at 03:05:10PM +0200, Alexander Shishkin escreveu:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > On Wed, Feb 15, 2017 at 10:34:58AM +0200, Alexander Shishkin wrote:
> >
> >> Well, we can't make the limitation go away. You'll still have to stop
> >> kvms to get any 'meaningful' PT data.
> >
> > Why would you need to stop all VMs in order to get your !VM data? Sure,
> > you get black holes where the VM runs, but we should be able to see
> > everything else.
> 
> No, what I mean is that if you run kvm prior to starting perf record,
> which I assume is the case for acme, your entire session is a black

Exactly.

> hole. The VMXON happens pretty early on, you can open /dev/kvm,
> ioctl(KVM_CREATE_VM) on it and that will do a VMXON already.
> 
> The problem is that PT (on BDW) doesn't trace inside VM root mode, not
> just between VM entry/VM exit.

Which is just unfortunate, destroys PT for a rather common use case :-\

Guess I need a <fill in the broadwell successor that allows using PT
together with VMs>

Which is?

:-)

- Arnaldo

Re: [PATCH 2/2] perf/x86/intel/pt: Fail event creation if VMX operation is on

[Peter Zijlstra]

On Wed, Feb 15, 2017 at 03:05:10PM +0200, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > On Wed, Feb 15, 2017 at 10:34:58AM +0200, Alexander Shishkin wrote:
> >
> >> Well, we can't make the limitation go away. You'll still have to stop
> >> kvms to get any 'meaningful' PT data.
> >
> > Why would you need to stop all VMs in order to get your !VM data? Sure,
> > you get black holes where the VM runs, but we should be able to see
> > everything else.
> 
> No, what I mean is that if you run kvm prior to starting perf record,
> which I assume is the case for acme, your entire session is a black
> hole. The VMXON happens pretty early on, you can open /dev/kvm,
> ioctl(KVM_CREATE_VM) on it and that will do a VMXON already.

kvm_intel has a vmm_exclusive parameter, if you set that to 0 is
_should_ behave much better.

Re: [PATCH 2/2] perf/x86/intel/pt: Fail event creation if VMX operation is on

[Arnaldo Carvalho de Melo]

Em Wed, Feb 15, 2017 at 10:34:58AM +0200, Alexander Shishkin escreveu:
> Arnaldo Carvalho de Melo <acme@kernel.org> writes:
> > Em Tue, Feb 14, 2017 at 03:24:16PM +0200, Alexander Shishkin escreveu:
> > Yeah, I saw that as well, and Andi told me about this limitation, so,
> > for quite a while now, everytime I need to test PT on the only machine I
> > have with it, I have to stop my kvm sessions :-\

> > Thanks for working on this!

> Well, we can't make the limitation go away. You'll still have to stop
> kvms to get any 'meaningful' PT data.

If we can't make the limitation go away we need to at least warn users
instead of let 'perf record' sit there doing nothing and then at the end
return as if everything went well only to when trying to use 'perf
script' nothing will appear.

- Arnaldo

Re: [PATCH 2/2] perf/x86/intel/pt: Fail event creation if VMX operation is on

[Alexander Shishkin]

Arnaldo Carvalho de Melo <acme@kernel.org> writes:

> Em Wed, Feb 15, 2017 at 10:34:58AM +0200, Alexander Shishkin escreveu:
>> Arnaldo Carvalho de Melo <acme@kernel.org> writes:
>> > Em Tue, Feb 14, 2017 at 03:24:16PM +0200, Alexander Shishkin escreveu:
>> > Yeah, I saw that as well, and Andi told me about this limitation, so,
>> > for quite a while now, everytime I need to test PT on the only machine I
>> > have with it, I have to stop my kvm sessions :-\
>
>> > Thanks for working on this!
>
>> Well, we can't make the limitation go away. You'll still have to stop
>> kvms to get any 'meaningful' PT data.
>
> If we can't make the limitation go away we need to at least warn users
> instead of let 'perf record' sit there doing nothing and then at the end
> return as if everything went well only to when trying to use 'perf
> script' nothing will appear.

I tend to like Peter's idea about PERF_RECORD_AUX with a PAUSED flag to
indicate this.

Regards,
--
Alex

Re: [PATCH 2/2] perf/x86/intel/pt: Fail event creation if VMX operation is on

[Arnaldo Carvalho de Melo]

Em Wed, Feb 15, 2017 at 03:09:25PM +0200, Alexander Shishkin escreveu:
> Arnaldo Carvalho de Melo <acme@kernel.org> writes:
> 
> > Em Wed, Feb 15, 2017 at 10:34:58AM +0200, Alexander Shishkin escreveu:
> >> Arnaldo Carvalho de Melo <acme@kernel.org> writes:
> >> > Em Tue, Feb 14, 2017 at 03:24:16PM +0200, Alexander Shishkin escreveu:
> >> > Yeah, I saw that as well, and Andi told me about this limitation, so,
> >> > for quite a while now, everytime I need to test PT on the only machine I
> >> > have with it, I have to stop my kvm sessions :-\
> >
> >> > Thanks for working on this!
> >
> >> Well, we can't make the limitation go away. You'll still have to stop
> >> kvms to get any 'meaningful' PT data.
> >
> > If we can't make the limitation go away we need to at least warn users
> > instead of let 'perf record' sit there doing nothing and then at the end
> > return as if everything went well only to when trying to use 'perf
> > script' nothing will appear.
> 
> I tend to like Peter's idea about PERF_RECORD_AUX with a PAUSED flag to
> indicate this.

Ok, you mean that I will then be able to start a intel pt session while
VMs are there, which will sample nothing, but then, with this session
still in place, as soon as I stop all VMs, samples will start appearing
on the buffer, then, at report/script/whatever analysis tool start, I'll
be able to show the blind spots and say that happened because something
prevented PT records from being produced?

Would be good to have some indication of _what_ prevented sampling, to
not have to say

  "hey, something banned PT records for a while, maybe VMs?"

Best thing I could do would be to correlated that with PERF_RECORD_MMAP
for suspecting pathnames.

- Arnaldo