[RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Peter Zijlstra]

Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI
context is problematic since the only way to change the various
unrelated bits in there is:

  debugctl = get_debugctlmsr()
  /* frob flags in debugctl */
  update_debugctlmsr(debugctl);

Which is entirely unsafe if we prod at the MSR from NMI context.

In particular the path that is responsible is:

  x86_pmu_handle_irq() (NMI handler)
    x86_pmu_stop()
      x86_pmu.disable -> intel_pmu_disable_event()
        intel_pmu_lbr_disable()
          __intel_pmu_lbr_disable()
            wrmsrl(MSR_IA32_DEBUGCTLMSR,... );

So remove intel_pmu_lbr_{en,dis}able() from
intel_pmu_{en,dis}able_event() and stick them in
intel_{get,put}_event_constraints() which are only ever called from
regular contexts.

We combine intel_pmu_needs_lbr_smpl(), which tells us if the events
wants LBR data, with event->hw.branch_reg.alloc, which given
intel_shared_regs_constraints() is set if our event got the shared
resource, to give us a correctly balanced condition in
{get,put}_event_constraints() for intel_pmu_lbr_{en,dis}able().

Also change core_pmu to only use x86_get_event_constraints since it
doesn't support any of the fancy DS (BTS,PEBS), LBR and OFFCORE features
that make up intel_{get,put}_event_constraints. 

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 arch/x86/kernel/cpu/perf_event_intel.c | 48 ++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 0d3d63a..ef4cd36 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -821,6 +821,11 @@ static inline bool intel_pmu_needs_lbr_smpl(struct perf_event *event)
 	return false;
 }
 
+static inline bool intel_pmu_has_lbr(struct perf_event *event)
+{
+	return intel_pmu_needs_lbr_smpl(event) && event->hw.branch_reg.alloc;
+}
+
 static void intel_pmu_disable_all(void)
 {
 	struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
@@ -975,13 +980,6 @@ static void intel_pmu_disable_event(struct perf_event *event)
 	cpuc->intel_ctrl_guest_mask &= ~(1ull << hwc->idx);
 	cpuc->intel_ctrl_host_mask &= ~(1ull << hwc->idx);
 
-	/*
-	 * must disable before any actual event
-	 * because any event may be combined with LBR
-	 */
-	if (intel_pmu_needs_lbr_smpl(event))
-		intel_pmu_lbr_disable(event);
-
 	if (unlikely(hwc->config_base == MSR_ARCH_PERFMON_FIXED_CTR_CTRL)) {
 		intel_pmu_disable_fixed(hwc);
 		return;
@@ -1036,12 +1034,6 @@ static void intel_pmu_enable_event(struct perf_event *event)
 		intel_pmu_enable_bts(hwc->config);
 		return;
 	}
-	/*
-	 * must enabled before any actual event
-	 * because any event may be combined with LBR
-	 */
-	if (intel_pmu_needs_lbr_smpl(event))
-		intel_pmu_lbr_enable(event);
 
 	if (event->attr.exclude_host)
 		cpuc->intel_ctrl_guest_mask |= (1ull << hwc->idx);
@@ -1382,17 +1374,28 @@ intel_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event
 
 	c = intel_bts_constraints(event);
 	if (c)
-		return c;
+		goto got_constraint;
 
 	c = intel_pebs_constraints(event);
 	if (c)
-		return c;
+		goto got_constraint;
 
 	c = intel_shared_regs_constraints(cpuc, event);
 	if (c)
-		return c;
+		goto got_constraint;
+
+	c = x86_get_event_constraints(cpuc, event);
+
+got_constraint:
 
-	return x86_get_event_constraints(cpuc, event);
+	/*
+	 * Must enabled before any actual event because any event may be
+	 * combined with LBR.
+	 */
+	if (intel_pmu_has_lbr(event))
+		intel_pmu_lbr_enable(event);
+
+	return c;
 }
 
 static void
@@ -1413,6 +1416,14 @@ intel_put_shared_regs_event_constraints(struct cpu_hw_events *cpuc,
 static void intel_put_event_constraints(struct cpu_hw_events *cpuc,
 					struct perf_event *event)
 {
+	/*
+	 * Must disabled after any actual event because any event may be
+	 * combined with LBR, but must be done before releasing the shared
+	 * regs resource since that protects the LBR state.
+	 */
+	if (intel_pmu_has_lbr(event))
+		intel_pmu_lbr_disable(event);
+
 	intel_put_shared_regs_event_constraints(cpuc, event);
 }
 
@@ -1623,8 +1634,7 @@ static __initconst const struct x86_pmu core_pmu = {
 	 * the generic event period:
 	 */
 	.max_period		= (1ULL << 31) - 1,
-	.get_event_constraints	= intel_get_event_constraints,
-	.put_event_constraints	= intel_put_event_constraints,
+	.get_event_constraints	= x86_get_event_constraints,
 	.event_constraints	= intel_core_event_constraints,
 	.guest_get_msrs		= core_guest_get_msrs,
 	.format_attrs		= intel_arch_formats_attr,

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Stephane Eranian]

On Wed, Sep 12, 2012 at 6:22 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI
> context is problematic since the only way to change the various
> unrelated bits in there is:
>
>   debugctl = get_debugctlmsr()
>   /* frob flags in debugctl */
>   update_debugctlmsr(debugctl);
>
> Which is entirely unsafe if we prod at the MSR from NMI context.
>
> In particular the path that is responsible is:
>
>   x86_pmu_handle_irq() (NMI handler)
>     x86_pmu_stop()
>       x86_pmu.disable -> intel_pmu_disable_event()
>         intel_pmu_lbr_disable()
>           __intel_pmu_lbr_disable()
>             wrmsrl(MSR_IA32_DEBUGCTLMSR,... );
>
> So remove intel_pmu_lbr_{en,dis}able() from
> intel_pmu_{en,dis}able_event() and stick them in
> intel_{get,put}_event_constraints() which are only ever called from
> regular contexts.
>
Not sure this is going to work for LBR.

We use FREEZE_LBR_ON_PMI to sync LBR data with counter overflows.
That means, LBR is already frozen by the time we get to the handler. But
that means we need to re-enable LBR when we leave the handler. I don't
think EOI is going to magically re-enable it. So we need to touch DEBUGCTL
in the irq handler.



> We combine intel_pmu_needs_lbr_smpl(), which tells us if the events
> wants LBR data, with event->hw.branch_reg.alloc, which given
> intel_shared_regs_constraints() is set if our event got the shared
> resource, to give us a correctly balanced condition in
> {get,put}_event_constraints() for intel_pmu_lbr_{en,dis}able().
>
> Also change core_pmu to only use x86_get_event_constraints since it
> doesn't support any of the fancy DS (BTS,PEBS), LBR and OFFCORE features
> that make up intel_{get,put}_event_constraints.
>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  arch/x86/kernel/cpu/perf_event_intel.c | 48 ++++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index 0d3d63a..ef4cd36 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -821,6 +821,11 @@ static inline bool intel_pmu_needs_lbr_smpl(struct perf_event *event)
>         return false;
>  }
>
> +static inline bool intel_pmu_has_lbr(struct perf_event *event)
> +{
> +       return intel_pmu_needs_lbr_smpl(event) && event->hw.branch_reg.alloc;
> +}
> +
>  static void intel_pmu_disable_all(void)
>  {
>         struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> @@ -975,13 +980,6 @@ static void intel_pmu_disable_event(struct perf_event *event)
>         cpuc->intel_ctrl_guest_mask &= ~(1ull << hwc->idx);
>         cpuc->intel_ctrl_host_mask &= ~(1ull << hwc->idx);
>
> -       /*
> -        * must disable before any actual event
> -        * because any event may be combined with LBR
> -        */
> -       if (intel_pmu_needs_lbr_smpl(event))
> -               intel_pmu_lbr_disable(event);
> -
>         if (unlikely(hwc->config_base == MSR_ARCH_PERFMON_FIXED_CTR_CTRL)) {
>                 intel_pmu_disable_fixed(hwc);
>                 return;
> @@ -1036,12 +1034,6 @@ static void intel_pmu_enable_event(struct perf_event *event)
>                 intel_pmu_enable_bts(hwc->config);
>                 return;
>         }
> -       /*
> -        * must enabled before any actual event
> -        * because any event may be combined with LBR
> -        */
> -       if (intel_pmu_needs_lbr_smpl(event))
> -               intel_pmu_lbr_enable(event);
>
>         if (event->attr.exclude_host)
>                 cpuc->intel_ctrl_guest_mask |= (1ull << hwc->idx);
> @@ -1382,17 +1374,28 @@ intel_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event
>
>         c = intel_bts_constraints(event);
>         if (c)
> -               return c;
> +               goto got_constraint;
>
>         c = intel_pebs_constraints(event);
>         if (c)
> -               return c;
> +               goto got_constraint;
>
>         c = intel_shared_regs_constraints(cpuc, event);
>         if (c)
> -               return c;
> +               goto got_constraint;
> +
> +       c = x86_get_event_constraints(cpuc, event);
> +
> +got_constraint:
>
> -       return x86_get_event_constraints(cpuc, event);
> +       /*
> +        * Must enabled before any actual event because any event may be
> +        * combined with LBR.
> +        */
> +       if (intel_pmu_has_lbr(event))
> +               intel_pmu_lbr_enable(event);
> +
> +       return c;
>  }
>
>  static void
> @@ -1413,6 +1416,14 @@ intel_put_shared_regs_event_constraints(struct cpu_hw_events *cpuc,
>  static void intel_put_event_constraints(struct cpu_hw_events *cpuc,
>                                         struct perf_event *event)
>  {
> +       /*
> +        * Must disabled after any actual event because any event may be
> +        * combined with LBR, but must be done before releasing the shared
> +        * regs resource since that protects the LBR state.
> +        */
> +       if (intel_pmu_has_lbr(event))
> +               intel_pmu_lbr_disable(event);
> +
>         intel_put_shared_regs_event_constraints(cpuc, event);
>  }
>
> @@ -1623,8 +1634,7 @@ static __initconst const struct x86_pmu core_pmu = {
>          * the generic event period:
>          */
>         .max_period             = (1ULL << 31) - 1,
> -       .get_event_constraints  = intel_get_event_constraints,
> -       .put_event_constraints  = intel_put_event_constraints,
> +       .get_event_constraints  = x86_get_event_constraints,
>         .event_constraints      = intel_core_event_constraints,
>         .guest_get_msrs         = core_guest_get_msrs,
>         .format_attrs           = intel_arch_formats_attr,
>

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Oleg Nesterov]

On 09/12, Peter Zijlstra wrote:
>
> Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI
> context is problematic since the only way to change the various
> unrelated bits in there is:
>
>   debugctl = get_debugctlmsr()
>   /* frob flags in debugctl */
>   update_debugctlmsr(debugctl);
>
> Which is entirely unsafe if we prod at the MSR from NMI context.
>
> In particular the path that is responsible is:
>
>   x86_pmu_handle_irq() (NMI handler)
>     x86_pmu_stop()
>       x86_pmu.disable -> intel_pmu_disable_event()
>         intel_pmu_lbr_disable()
>           __intel_pmu_lbr_disable()
>             wrmsrl(MSR_IA32_DEBUGCTLMSR,... );

Not only.

x86_pmu_handle_irq() does intel_pmu_disable_all() and intel_pmu_enable_all(),
this leads to intel_pmu_enable_bts() and intel_pmu_disable_bts().

And those intel_pmu_*_bts() are also called by intel_pmu_disable_event()
and intel_pmu_enable_event(), the latter is probably fine.

Oleg.

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Peter Zijlstra]

On Wed, 2012-09-12 at 18:42 +0200, Stephane Eranian wrote:
> We use FREEZE_LBR_ON_PMI to sync LBR data with counter overflows.
> That means, LBR is already frozen by the time we get to the handler. But
> that means we need to re-enable LBR when we leave the handler. I don't
> think EOI is going to magically re-enable it. So we need to touch DEBUGCTL
> in the irq handler. 

Ah, so I do think EIO will re-enable LBR, also the handler is wrapped in
x86_pmu::{dis,en}able_all() which does end up calling
intel_pmu_lbr_{dis,en}able_all(). However that leaves the MSR in the
exact same state on exit as it was on enter, so that's not a problem for
the: read-modify-write change.

Its just that the x86_pmu::disable() for the throttle could leave the
MSR in a different state on exit than it entered with. This patch avoids
that.

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Peter Zijlstra]

On Wed, 2012-09-12 at 19:36 +0200, Oleg Nesterov wrote:
> On 09/12, Peter Zijlstra wrote:
> >
> > Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI
> > context is problematic since the only way to change the various
> > unrelated bits in there is:
> >
> >   debugctl = get_debugctlmsr()
> >   /* frob flags in debugctl */
> >   update_debugctlmsr(debugctl);
> >
> > Which is entirely unsafe if we prod at the MSR from NMI context.
> >
> > In particular the path that is responsible is:
> >
> >   x86_pmu_handle_irq() (NMI handler)
> >     x86_pmu_stop()
> >       x86_pmu.disable -> intel_pmu_disable_event()
> >         intel_pmu_lbr_disable()
> >           __intel_pmu_lbr_disable()
> >             wrmsrl(MSR_IA32_DEBUGCTLMSR,... );
> 
> Not only.
> 
> x86_pmu_handle_irq() does intel_pmu_disable_all() and intel_pmu_enable_all(),
> this leads to intel_pmu_enable_bts() and intel_pmu_disable_bts().
> 
> And those intel_pmu_*_bts() are also called by intel_pmu_disable_event()
> and intel_pmu_enable_event(), the latter is probably fine.

As written in the email to Stephane just now, the {dis,en}able_all()
things are symmetric and don't change the visible MSR state. But you're
right, I missed that BTS frobbed that MSR as well. 

I'll have to see if there's a DS programming that effectively disables
the BTS nonsense.

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Peter Zijlstra]

On Wed, 2012-09-12 at 19:37 +0200, Peter Zijlstra wrote:
> Ah, so I do think EIO will re-enable LBR,

OK, it does not, but the:

>  also the handler is wrapped in
> x86_pmu::{dis,en}able_all() which does end up calling
> intel_pmu_lbr_{dis,en}able_all().

Thing does the re-enable for us,

>  However that leaves the MSR in the
> exact same state on exit as it was on enter, so that's not a problem for
> the: read-modify-write change. 

in a safe way.

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Stephane Eranian]

On Wed, Sep 12, 2012 at 7:45 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, 2012-09-12 at 19:37 +0200, Peter Zijlstra wrote:
>> Ah, so I do think EIO will re-enable LBR,
>
> OK, it does not, but the:
>
>>  also the handler is wrapped in
>> x86_pmu::{dis,en}able_all() which does end up calling
>> intel_pmu_lbr_{dis,en}able_all().
>
> Thing does the re-enable for us,
>

>>  However that leaves the MSR in the
>> exact same state on exit as it was on enter, so that's not a problem for
>> the: read-modify-write change.
>
> in a safe way.
Well, I think it does even when we have to stop events (x86_pmu_stop)
because the buffer is full. Looks like we always re-enable lbr. So looks
like the handler is a wash for debugctl.

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Peter Zijlstra]

On Wed, 2012-09-12 at 20:00 +0200, Stephane Eranian wrote:
> On Wed, Sep 12, 2012 at 7:45 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Wed, 2012-09-12 at 19:37 +0200, Peter Zijlstra wrote:
> >> Ah, so I do think EIO will re-enable LBR,
> >
> > OK, it does not, but the:
> >
> >>  also the handler is wrapped in
> >> x86_pmu::{dis,en}able_all() which does end up calling
> >> intel_pmu_lbr_{dis,en}able_all().
> >
> > Thing does the re-enable for us,
> >
> 
> >>  However that leaves the MSR in the
> >> exact same state on exit as it was on enter, so that's not a problem for
> >> the: read-modify-write change.
> >
> > in a safe way.
> Well, I think it does even when we have to stop events (x86_pmu_stop)
> because the buffer is full. Looks like we always re-enable lbr.

How so, without the proposed patch, the intel_pmu_disable_event() can do
intel_pmu_lbr_disable() which decrements cpuc->lbr_users, so the final
intel_pmu_enable_all()->intel_pmu_lbr_enable_all() will be a NOP,
leaving LBR disabled, where we entered the NMI with LBR enabled.

>  So looks like the handler is a wash for debugctl.

As for BTS, it looks like we don't throttle the thing at all, so we
shouldn't ever get to the asymmetric thing, right?

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Stephane Eranian]

On Wed, Sep 12, 2012 at 8:17 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, 2012-09-12 at 20:00 +0200, Stephane Eranian wrote:
>> On Wed, Sep 12, 2012 at 7:45 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Wed, 2012-09-12 at 19:37 +0200, Peter Zijlstra wrote:
>> >> Ah, so I do think EIO will re-enable LBR,
>> >
>> > OK, it does not, but the:
>> >
>> >>  also the handler is wrapped in
>> >> x86_pmu::{dis,en}able_all() which does end up calling
>> >> intel_pmu_lbr_{dis,en}able_all().
>> >
>> > Thing does the re-enable for us,
>> >
>>
>> >>  However that leaves the MSR in the
>> >> exact same state on exit as it was on enter, so that's not a problem for
>> >> the: read-modify-write change.
>> >
>> > in a safe way.
>> Well, I think it does even when we have to stop events (x86_pmu_stop)
>> because the buffer is full. Looks like we always re-enable lbr.
>
> How so, without the proposed patch, the intel_pmu_disable_event() can do
> intel_pmu_lbr_disable() which decrements cpuc->lbr_users, so the final
> intel_pmu_enable_all()->intel_pmu_lbr_enable_all() will be a NOP,
> leaving LBR disabled, where we entered the NMI with LBR enabled.
>
You're right. I looked at the wrong x86_pmu struct.
So yes, it is not symmetrical in all cases. So that's a problem
for the race condition.

>>  So looks like the handler is a wash for debugctl.
>
> As for BTS, it looks like we don't throttle the thing at all, so we
> shouldn't ever get to the asymmetric thing, right?
No you do, in the same function:
static void intel_pmu_disable_event(struct perf_event *event)
{
        struct hw_perf_event *hwc = &event->hw;
        struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);

        if (unlikely(hwc->idx == INTEL_PMC_IDX_FIXED_BTS)) {
                intel_pmu_disable_bts();
                intel_pmu_drain_bts_buffer();
                return;
        }

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Peter Zijlstra]

On Wed, 2012-09-12 at 20:50 +0200, Stephane Eranian wrote:

> > As for BTS, it looks like we don't throttle the thing at all, so we
> > shouldn't ever get to the asymmetric thing, right?
> No you do, in the same function:
> static void intel_pmu_disable_event(struct perf_event *event)
> {
>         struct hw_perf_event *hwc = &event->hw;
>         struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> 
>         if (unlikely(hwc->idx == INTEL_PMC_IDX_FIXED_BTS)) {
>                 intel_pmu_disable_bts();
>                 intel_pmu_drain_bts_buffer();
>                 return;
>         }

Right, but the main event loop in intel_pmu_handle_irq() is over the
MSR_CORE_PERF_GLOBAL_STATUS status bits, BTS is not included in those,
so we'd never end up calling x86_pmu_stop() on the associated event.

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Stephane Eranian]

On Wed, Sep 12, 2012 at 8:52 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, 2012-09-12 at 20:50 +0200, Stephane Eranian wrote:
>
>> > As for BTS, it looks like we don't throttle the thing at all, so we
>> > shouldn't ever get to the asymmetric thing, right?
>> No you do, in the same function:
>> static void intel_pmu_disable_event(struct perf_event *event)
>> {
>>         struct hw_perf_event *hwc = &event->hw;
>>         struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
>>
>>         if (unlikely(hwc->idx == INTEL_PMC_IDX_FIXED_BTS)) {
>>                 intel_pmu_disable_bts();
>>                 intel_pmu_drain_bts_buffer();
>>                 return;
>>         }
>
> Right, but the main event loop in intel_pmu_handle_irq() is over the
> MSR_CORE_PERF_GLOBAL_STATUS status bits, BTS is not included in those,
> so we'd never end up calling x86_pmu_stop() on the associated event.

True, it does not use a counter. But it interrupts when the buffer becomes full.
We catch this at the beginning of intel_pmu_handle_irq(). I think there is an
issue there with throtlling. Could be that the BTS buffer + workload could
caused PMU interrupts at a rate > max_rate. But yeah, it does not go
into the ovfl_status loop, so it cannot be stopped in an asymmetrical
way.

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Peter Zijlstra]

On Wed, 2012-09-12 at 18:22 +0200, Peter Zijlstra wrote:
> Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI
> context is problematic since the only way to change the various
> unrelated bits in there is:
> 
>   debugctl = get_debugctlmsr()
>   /* frob flags in debugctl */
>   update_debugctlmsr(debugctl);
> 
> Which is entirely unsafe if we prod at the MSR from NMI context.
> 
> In particular the path that is responsible is:
> 
>   x86_pmu_handle_irq() (NMI handler)
>     x86_pmu_stop()
>       x86_pmu.disable -> intel_pmu_disable_event()
>         intel_pmu_lbr_disable()
>           __intel_pmu_lbr_disable()
>             wrmsrl(MSR_IA32_DEBUGCTLMSR,... );
> 
> So remove intel_pmu_lbr_{en,dis}able() from
> intel_pmu_{en,dis}able_event() and stick them in
> intel_{get,put}_event_constraints() which are only ever called from
> regular contexts.
> 
> We combine intel_pmu_needs_lbr_smpl(), which tells us if the events
> wants LBR data, with event->hw.branch_reg.alloc, which given
> intel_shared_regs_constraints() is set if our event got the shared
> resource, to give us a correctly balanced condition in
> {get,put}_event_constraints() for intel_pmu_lbr_{en,dis}able().
> 
> Also change core_pmu to only use x86_get_event_constraints since it
> doesn't support any of the fancy DS (BTS,PEBS), LBR and OFFCORE features
> that make up intel_{get,put}_event_constraints. 

OK, so with the added stipulation that touching the MSR from the NMI
isn't a problem as long as we ensure we leave the MSR in the same state
that we found it in, and the above is the only path found so far that
could cause this to be violated, the patch is fine?

In particular we could note that both LBR and BTS use the DEBUGCTL MSR,
but BTS doesn't throttle, so the LBR code is the only thing needing
attention as per the above description.

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Stephane Eranian]

On Thu, Sep 13, 2012 at 12:23 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, 2012-09-12 at 18:22 +0200, Peter Zijlstra wrote:
>> Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI
>> context is problematic since the only way to change the various
>> unrelated bits in there is:
>>
>>   debugctl = get_debugctlmsr()
>>   /* frob flags in debugctl */
>>   update_debugctlmsr(debugctl);
>>
>> Which is entirely unsafe if we prod at the MSR from NMI context.
>>
>> In particular the path that is responsible is:
>>
>>   x86_pmu_handle_irq() (NMI handler)
>>     x86_pmu_stop()
>>       x86_pmu.disable -> intel_pmu_disable_event()
>>         intel_pmu_lbr_disable()
>>           __intel_pmu_lbr_disable()
>>             wrmsrl(MSR_IA32_DEBUGCTLMSR,... );
>>
>> So remove intel_pmu_lbr_{en,dis}able() from
>> intel_pmu_{en,dis}able_event() and stick them in
>> intel_{get,put}_event_constraints() which are only ever called from
>> regular contexts.
>>
>> We combine intel_pmu_needs_lbr_smpl(), which tells us if the events
>> wants LBR data, with event->hw.branch_reg.alloc, which given
>> intel_shared_regs_constraints() is set if our event got the shared
>> resource, to give us a correctly balanced condition in
>> {get,put}_event_constraints() for intel_pmu_lbr_{en,dis}able().
>>
>> Also change core_pmu to only use x86_get_event_constraints since it
>> doesn't support any of the fancy DS (BTS,PEBS), LBR and OFFCORE features
>> that make up intel_{get,put}_event_constraints.
>
> OK, so with the added stipulation that touching the MSR from the NMI
> isn't a problem as long as we ensure we leave the MSR in the same state
> that we found it in, and the above is the only path found so far that
> could cause this to be violated, the patch is fine?
>
Should be, though it is pretty ugly to stash all of this in the
put/get constraints.

I will run some tests.

I wonder what this does when you come in to get/put with a fake cpuc. You don't
want to perturb the local lbr which may be in use.

> In particular we could note that both LBR and BTS use the DEBUGCTL MSR,
> but BTS doesn't throttle, so the LBR code is the only thing needing
> attention as per the above description.
>
>

Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

[Peter Zijlstra]

On Thu, 2012-09-13 at 13:49 +0200, Stephane Eranian wrote:
> Should be, though it is pretty ugly to stash all of this in the
> put/get constraints.

Agreed, I almost added two extra functions for it but when I went to
look at where to call them I ended up next to get/put constraints.

> I will run some tests.
> 
> I wonder what this does when you come in to get/put with a fake cpuc. You don't
> want to perturb the local lbr which may be in use. 

Fake cpu will never set ->alloc and thus intel_pmu_has_lbr() should fail
and we don't do anything.