Messages in this thread Patch in this message | | | Date | Fri, 16 Mar 2012 11:22:45 +0900 | From | Akihiro Nagai <> | Subject | Re: [PATCH -tip v5 2/5] perf: set correct value to perf_event_header.misc for BTS |
| |
(2012/03/07 2:32), Peter Zijlstra wrote: > On Tue, 2012-02-21 at 14:39 +0900, Akihiro Nagai wrote: >> (2012/01/30 18:35), Peter Zijlstra wrote: >>> On Mon, 2012-01-30 at 13:43 +0900, Akihiro Nagai wrote: >>>> @@ -330,6 +342,14 @@ int intel_pmu_drain_bts_buffer(void) >>>> return 1; >>>> >>>> for (; at< top; at++) { >>>> + /* >>>> + * To resolve user space symbols and DSOs correctly, set >>>> + * PERF_RECORD_MISC_USER if from_addr or to_addr is user space. >>>> + */ >>>> + if (!kernel_ip(data.ip) || !kernel_ip(data.addr)) { >>>> + header.misc&= ~PERF_RECORD_MISC_CPUMODE_MASK; >>>> + header.misc |= PERF_RECORD_MISC_USER; >>>> + } >>>> data.ip = at->from; >>>> data.addr = at->to; >>> >>> Why not key off of the from? If its a jump from userspace its a user >>> event, its a jump from kernel space its a kernel event? >> >> Of course, originally, perf does that. > > I don't think it does. > >> And, in those cases, >> BTS records the both addresses of kernel and user in one >> perf_sample on branches from kernel to user. > > Sorry, I don't get this. > >> Current perf sets PERF_RECORD_MISC_KERNEL to all BTS events, > > It doesn't, it does something far more stupid.. it sets the state to > wherever we were when the BTS overflow interrupt happens. If that was in > kernel space, we mark all of them as in-kernel, if that was in > user-space we mark all of then in-user.
Normally, perf does. However, BTS (intel_pmu_drain_bts_buffer) uses local-defined and not-initialized pt_regs to decide PERF_RECORD_MISC_KERNEL or USER. Accordingly, almost all BTS events are set PERF_RECORD_MISC_KERNEL.
BTW, I think it's a bug that uses not-initialized pt_regs. So, it needs initialize: - struct pt_regs regs; + struct pt_regs regs = {0};
Or, intel_pmu_drain_bts_buffer() get pt_regs from a function argument. - int intel_pmu_drain_bts_buffer(void) + int intel_pmu_drain_bts_buffer(struct pt_regs *regs)
> >> and >> perf-script doesn't resolve symbols and DSOs about the >> user-space address, because it is a kernel event. > > Well, that's a perf-script problem, not something you should change the > kernel for.
Exactly. I'm going to fix perf-script.
> > So how about something like this? > > --- > arch/x86/kernel/cpu/perf_event_intel_ds.c | 7 +++++++ > 1 files changed, 7 insertions(+), 0 deletions(-) > > diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c > b/arch/x86/kernel/cpu/perf_event_intel_ds.c > index d6bd49f..81e788c 100644 > --- a/arch/x86/kernel/cpu/perf_event_intel_ds.c > +++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c > @@ -333,6 +333,13 @@ int intel_pmu_drain_bts_buffer(void) > data.ip = at->from; > data.addr = at->to; > > + /* XXX doesn't do virt muck properly */ > + header.misc&= ~PERF_RECORD_MISC_CPUMODE_MASK; > + if (kernel_ip(at->from)) > + header.misc |= PERF_RECORD_MISC_KERNEL; > + else > + header.misc |= PERF_RECORD_MISC_USER; > + > perf_output_sample(&handle,&header,&data, event); > }
It looks good. I think it's correct semantically to set PERF_RECORD_MISC_KERNEL or USER by the from-address. However, this patch needs perf-script fix too. I'm going to send next patches including this patch.
Thank you,
| |