Thanks Peter! > On Jan 17, 2019, at 5:09 AM, Peter Zijlstra <pet...@infradead.org> wrote: > > On Wed, Jan 16, 2019 at 08:29:25AM -0800, Song Liu wrote: >> + /* >> + * Record bpf events: >> + * enum perf_bpf_event_type { >> + * PERF_BPF_EVENT_UNKNOWN = 0, >> + * PERF_BPF_EVENT_PROG_LOAD = 1, >> + * PERF_BPF_EVENT_PROG_UNLOAD = 2, >> + * }; >> + * >> + * struct { >> + * struct perf_event_header header; >> + * u16 type; >> + * u16 flags; >> + * u32 id; >> + * u8 tag[BPF_TAG_SIZE]; > > This does forever fix BPF_TAG_SIZE; is that intentional? We could easily > make that a variable length field like with the other event. Or is that > value already part of the eBPF ABI?
Yes, BPF_TAG_SIZE is already part of eBPF ABI. Song > >> + * struct sample_id sample_id; >> + * }; >> + */ >> + PERF_RECORD_BPF_EVENT = 18, >> @@ -7744,6 +7747,121 @@ void perf_event_ksymbol(u16 ksym_type, u64 addr, u32 >> len, bool unregister, >> WARN_ONCE(1, "%s: Invalid KSYMBOL type 0x%x\n", __func__, ksym_type); >> } >> >> +struct perf_bpf_event { >> + struct bpf_prog *prog; >> + struct { >> + struct perf_event_header header; >> + u16 type; >> + u16 flags; >> + u32 id; >> + u8 tag[BPF_TAG_SIZE]; >> + } event_id; >> +}; > >> +static void perf_event_bpf_emit_ksymbols(struct bpf_prog *prog, >> + enum perf_bpf_event_type type) >> +{ >> + bool unregister = type == PERF_BPF_EVENT_PROG_UNLOAD; >> + int i; >> + >> + if (prog->aux->func_cnt == 0) { >> + perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_BPF, >> + (u64)(unsigned long)prog->bpf_func, >> + prog->jited_len, unregister, >> + perf_event_bpf_get_name, prog); >> + } else { >> + for (i = 0; i < prog->aux->func_cnt; i++) { >> + struct bpf_prog *subprog = prog->aux->func[i]; >> + >> + perf_event_ksymbol( >> + PERF_RECORD_KSYMBOL_TYPE_BPF, >> + (u64)(unsigned long)subprog->bpf_func, >> + subprog->jited_len, unregister, >> + perf_event_bpf_get_name, subprog); >> + } >> + } >> +} > > I still think this is a weird place to do this.. :-) See them patches I > just send. > >> +void perf_event_bpf_event(struct bpf_prog *prog, >> + enum perf_bpf_event_type type, >> + u16 flags) >> +{ >> + struct perf_bpf_event bpf_event; >> + >> + if (type <= PERF_BPF_EVENT_UNKNOWN || >> + type >= PERF_BPF_EVENT_MAX) >> + return; >> + >> + switch (type) { >> + case PERF_BPF_EVENT_PROG_LOAD: >> + case PERF_BPF_EVENT_PROG_UNLOAD: >> + if (atomic_read(&nr_ksymbol_events)) >> + perf_event_bpf_emit_ksymbols(prog, type); >> + break; >> + default: >> + break; >> + } >> + >> + if (!atomic_read(&nr_bpf_events)) >> + return; >> + >> + bpf_event = (struct perf_bpf_event){ >> + .prog = prog, >> + .event_id = { >> + .header = { >> + .type = PERF_RECORD_BPF_EVENT, >> + .size = sizeof(bpf_event.event_id), >> + }, >> + .type = type, >> + .flags = flags, >> + .id = prog->aux->id, >> + }, >> + }; > > BUILD_BUG_ON(BPF_TAG_SIZE % sizeof(u64)); > >> + memcpy(bpf_event.event_id.tag, prog->tag, BPF_TAG_SIZE); >> + perf_iterate_sb(perf_event_bpf_output, &bpf_event, NULL); >> +} > > Anyway, small nits only: > > Acked-by: Peter Zijlstra (Intel) <pet...@infradeaed.org>