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

2012-09-13 Thread 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

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

2012-09-13 Thread Stephane Eranian
On Thu, Sep 13, 2012 at 12:23 PM, Peter Zijlstra 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: >> >>

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

2012-09-13 Thread 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 */ >

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

2012-09-13 Thread 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 */

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

2012-09-13 Thread Stephane Eranian
On Thu, Sep 13, 2012 at 12:23 PM, Peter Zijlstra pet...@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:

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

2012-09-13 Thread 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

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

2012-09-12 Thread Stephane Eranian
On Wed, Sep 12, 2012 at 8:52 PM, Peter Zijlstra 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

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

2012-09-12 Thread 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) > { >

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

2012-09-12 Thread Stephane Eranian
On Wed, Sep 12, 2012 at 8:17 PM, Peter Zijlstra wrote: > On Wed, 2012-09-12 at 20:00 +0200, Stephane Eranian wrote: >> On Wed, Sep 12, 2012 at 7:45 PM, Peter Zijlstra wrote: >> > On Wed, 2012-09-12 at 19:37 +0200, Peter Zijlstra wrote: >> >> Ah, so I do think EIO will re-enable LBR, >> > >> >

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

2012-09-12 Thread 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 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

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

2012-09-12 Thread Stephane Eranian
On Wed, Sep 12, 2012 at 7:45 PM, Peter Zijlstra 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 >>

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

2012-09-12 Thread 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, >

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

2012-09-12 Thread 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 =

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

2012-09-12 Thread 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

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

2012-09-12 Thread 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 */ >

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

2012-09-12 Thread Stephane Eranian
On Wed, Sep 12, 2012 at 6:22 PM, 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 */ >

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

2012-09-12 Thread 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

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

2012-09-12 Thread 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

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

2012-09-12 Thread Stephane Eranian
On Wed, Sep 12, 2012 at 6:22 PM, Peter Zijlstra pet...@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

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

2012-09-12 Thread 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);

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

2012-09-12 Thread 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

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

2012-09-12 Thread 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()

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

2012-09-12 Thread 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

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

2012-09-12 Thread Stephane Eranian
On Wed, Sep 12, 2012 at 7:45 PM, Peter Zijlstra pet...@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

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

2012-09-12 Thread 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 pet...@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

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

2012-09-12 Thread Stephane Eranian
On Wed, Sep 12, 2012 at 8:17 PM, Peter Zijlstra pet...@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 pet...@infradead.org wrote: On Wed, 2012-09-12 at 19:37 +0200, Peter Zijlstra wrote: Ah, so I do think EIO

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

2012-09-12 Thread 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

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

2012-09-12 Thread Stephane Eranian
On Wed, Sep 12, 2012 at 8:52 PM, Peter Zijlstra pet...@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: