[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CF447261-1B9C-423B-9508-60B32EAC4A5C@fb.com>
Date: Tue, 22 Jan 2019 19:24:43 +0000
From: Song Liu <songliubraving@...com>
To: Jiri Olsa <jolsa@...hat.com>
CC: Arnaldo Carvalho de Melo <acme@...nel.org>,
Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"peterz@...radead.org" <peterz@...radead.org>,
"ast@...nel.org" <ast@...nel.org>,
"daniel@...earbox.net" <daniel@...earbox.net>,
Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v11 perf, bpf-next 7/9] perf tools: synthesize
PERF_RECORD_* for loaded BPF programs
> On Jan 22, 2019, at 11:15 AM, Jiri Olsa <jolsa@...hat.com> wrote:
>
> On Tue, Jan 22, 2019 at 06:38:56PM +0000, Song Liu wrote:
>
> SNIP
>
>>>> in perf_session__process_event, this happens right when processing
>>>> buildids in 'perf record', and also in 'perf report', so that is
>>>> something badly synthesized that hits perf.data for PERF_RECORD_KSYMBOL.
>>>
>>> it's reproducible with simple:
>>> perf record -e cycles,instructions ls
>>>
>>> as you said on irc, it's the machine->id_hdr_size size missing
>>> there's one more glitch, attached patch fixes that for me
>>> you can't use sizeof(struct ksymbol_event), because it includes
>>> the name as well.. which screws the size
>>>
>>> but I don't know that code that much.. might be still something
>>> missing
>>>
>>> jirka
>>
>> Hi Arnaldo and Jiri,
>>
>> Thanks for catching and fixing the bug.
>>
>> I guess the following is OK?
>>
>> *bpf_event = (struct bpf_event){
>> .header = {
>> .type = PERF_RECORD_BPF_EVENT,
>> .size = sizeof(struct bpf_event),
>> },
>> .type = PERF_BPF_EVENT_PROG_LOAD,
>> .flags = 0,
>> .id = info.id,
>> };
>>
>> as struct bpf_event doesn't have variable length name:
>>
>> struct bpf_event {
>> struct perf_event_header header;
>> u16 type;
>> u16 flags;
>> u32 id;
>>
>> /* for bpf_prog types */
>> u8 tag[BPF_TAG_SIZE]; // prog tag
>> };
>>
>> Or we need similar fix?
>
> yep, looks good.. also don't forget to add the 'machine->id_hdr_size'
>
> jirka
So we still need something like?
diff --git i/tools/perf/util/bpf-event.c w/tools/perf/util/bpf-event.c
index 01e1dc1bb7fb..d7bf45485820 100644
--- i/tools/perf/util/bpf-event.c
+++ w/tools/perf/util/bpf-event.c
@@ -4,6 +4,7 @@
#include <bpf/bpf.h>
#include <bpf/btf.h>
#include <linux/btf.h>
+#include "machine.h"
#include "bpf-event.h"
#include "debug.h"
#include "symbol.h"
@@ -187,7 +188,7 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_tool *tool,
*bpf_event = (struct bpf_event){
.header = {
.type = PERF_RECORD_BPF_EVENT,
- .size = sizeof(struct bpf_event),
+ .size = sizeof(struct bpf_event) + machine->id_hdr_size,
},
.type = PERF_BPF_EVENT_PROG_LOAD,
.flags = 0,
Would you send the official patch? Or would you prefer me sending it?
Thanks,
Song
Powered by blists - more mailing lists