Messages in this thread Patch in this message | | | Date | Wed, 23 Nov 2016 10:49:51 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [PATCH] Add support for disabling Intel PT trace in ftrace |
| |
On Tue, 22 Nov 2016, Steven Rostedt wrote: > On Fri, 18 Nov 2016 08:55:24 -0800 > Andi Kleen <andi@firstfloor.org> wrote: > > > From: Andi Kleen <ak@linux.intel.com> > > > > ftrace has powerfull trigger functions. Intel PT on modern Intel CPUs > > can trace execution flow. > > > > For debugging I found it useful to disable the PT trace from ftrace triggers, > > for example when specific kernel functions are hit, which indicate > > a problem. Then we can see the exact execution trace up to this point. > > > > This patch adds a "ptoff" ftrace trigger/function that disables the trace > > on the current function. The PT trace still has to be set up with perf > > > > % perf record -e intel_pt// -a ... & > > % cd /sys/kernel/debug/tracing > > % echo do_page_fault:ptoff > set_ftrace_filter > > ... > > % cd - > > % kill %1 > > % perf script --itrace=i0ns > > > > I only implemented local disabling. Enabling would be much more complicated > > and require a black list of functions to avoid recursion. Global > > disabling with IPIs would be possible, but also risk some deadlock > > scenarios. Local disabling is very easy and can be done without > > accessing any special state, so there are no such problems. It is > > usually good enough for debugging purposes. The trace can be always > > reenabled from perf. > > > > This patch adds "ptoff" both as ftrace trigger and ftrace functions. > > This makes it work from "set_ftrace_filter" and through the trigger > > field of trace points.
This changelog reads like a fairy tale and certainly needs some care.
> > The PT driver exports a pt_disable() function for this that can be also > > used for manual instrumentation.
> > diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt > > index 185c39fea2a0..5dc8ec658678 100644 > > --- a/Documentation/trace/ftrace.txt > > +++ b/Documentation/trace/ftrace.txt > > @@ -2549,6 +2549,11 @@ The following commands are supported: > > command, it only prints out the contents of the ring buffer for the > > CPU that executed the function that triggered the dump. > > > > +- ptoff > > + When the function is hit disable Intel PT trace. The Intel PT > > + trace has to be set up earlier with perf record -a -e intel_pt// ... > > + This disables the trace on the current CPU only.
When the function is hit, Intel PT trace is disabled on the current CPU; PT trace on other CPUs is unaffected. The Intel PT trace has to be set up earlier with: perf record -e intel_pt// ...
> > +/* > > + * Disable the PT trace for debugging purposes. > > + */ > > +void pt_disable(void) > > +{ > > + u64 val; > > + > > + if (!boot_cpu_has(X86_FEATURE_INTEL_PT)) > > + return; > > + > > + rdmsrl_safe(MSR_IA32_RTIT_CTL, &val);
Why is this using rdmsrl_safe()? If that's required then the return value should be checked and acted upon.
> > + val &= ~RTIT_CTL_TRACEEN; > > + wrmsrl_safe(MSR_IA32_RTIT_CTL, val);
How does this interact with perf, the PT nmi handler, context switch etc.? I can't see anything, but the implications of this need to be documented.
> > +} > > +EXPORT_SYMBOL(pt_disable);
This export is not required for the trace trigger. This wants to be a seperate patch, EXPORT_SYMBOl_GPL and an in tree user as any other export..
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > > index 4741ecdb9817..a408d288298b 100644 > > --- a/include/linux/perf_event.h > > +++ b/include/linux/perf_event.h > > @@ -1387,4 +1387,6 @@ int perf_event_exit_cpu(unsigned int cpu); > > #define perf_event_exit_cpu NULL > > #endif > > > > +void pt_disable(void);
This is the wrong header file to declare the function. Aside of that pt_disable() is a too generic name. Something like x86_intel_pt_disable() makes it clear what this is about.
Thanks,
tglx
| |