Re: [PATCH -tip v5 2/5] perf: set correct value to perf_event_header.misc for BTS

[Akihiro Nagai <akihiro.nagai.hw@hitachi.com>]

(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,