Messages in this thread | | | From | Song Liu <> | Subject | Re: [PATCH v3 perf, bpf-next 1/4] perf, bpf: Introduce PERF_RECORD_BPF_EVENT | Date | Fri, 14 Dec 2018 17:10:56 +0000 |
| |
> On Dec 14, 2018, at 5:48 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Thu, Dec 13, 2018 at 09:48:57PM +0000, Song Liu escreveu: >> >> >>> On Dec 13, 2018, at 10:45 AM, Peter Zijlstra <peterz@infradead.org> wrote: >>> >>> On Wed, Dec 12, 2018 at 01:33:20PM -0500, Steven Rostedt wrote: >>>> On Wed, 12 Dec 2018 19:05:53 +0100 >>>> Peter Zijlstra <peterz@infradead.org> wrote: >>>> >>>>> On Wed, Dec 12, 2018 at 05:09:17PM +0000, Song Liu wrote: >>>>>>> And while this tracks the bpf kallsyms, it does not do all kallsyms. >>>>>>> >>>>>>> .... Oooh, I see the problem, everybody is doing their own custom >>>>>>> kallsym_{add,del}() thing, instead of having that in generic code :-( >>>>>>> >>>>>>> This, for example, doesn't track module load/unload nor ftrace >>>>>>> trampolines, even though both affect kallsyms. >>>>>> >>>>>> I think we can use PERF_RECORD_MMAP(or MMAP2) for module load/unload. >>>>>> That could be separate sets of patches. >>>>> >>>>> So I would actually like to move bpf_lock/bpf_kallsyms/bpf_tree + >>>>> bpf_prog_kallsyms_*() + __bpf_address_lookup() into kernel/kallsyms.c >>>>> and also have ftrace use that. >>>>> >>>>> Because currently the ftrace stuff is otherwise invisible. >>>>> >>>>> A generic kallsym register/unregister for any JIT. >>>> >>>> That's if it needs to look up the symbols that were recorded when init >>>> was unloaded. >>>> >>>> The ftrace kallsyms is used to save the function names of init code >>>> that was freed, but may have been recorded. With out the ftrace >>>> kallsyms the functions traced at init time would just show up as hex >>>> addresses (not very useful). >>>> >>>> I'm not sure how BPF would need those symbols unless they were executed >>>> during init (module or core) and needed to see what the symbols use to >>>> be). >>> >>> Aah, that sounds entirely dodgy and possibly quite broken. We freed that >>> init code, so BPF or your trampolines (or a tiny module) could actually >>> fit in there and insert their own kallsyms, and then we have overlapping >>> symbols, which would be pretty bad. >>> >>> I thought the ftrace kallsym stuff was for the trampolines, which would >>> be fairly similar to what BPF is doing. And why I'm trying to get a >>> generic dynamic kallsym thing sorted. There's bound the be other >>> code-gen things at some point. >> >> Hi Peter, >> >> I guess you are looking for something for all ksym add/delete events, like; >> >> /* >> * PERF_RECORD_KSYMBOL >> * >> * struct { >> * struct perf_event_header header; >> * u64 addr; >> * u32 len; >> * u16 ksym_type; >> * u16 flags; >> * char name[]; >> * struct sample_id sample_id; >> * }; >> */ > > Can't this reuse PERF_RECORD_MMAP2 with some bit in the header to mean > that the name is the symbol name, not a path to some ELF/whatever? The > ksym type could be encoded in the prot field, PROT_EXEC for functions, > PROT_READ for read only data, PROT_WRITE for rw data.
Thanks Arnaldo!
I think this works. PERF_RECORD_MMAP2 has many bits in it. We can encode a lot of details. We can even have bit to differentiate map/unmap.
> > If we do it that way older tools will show the DSO name and an > unresolved symbol, and even an indication if its a function or data, > which is better than not showing anything when processing a new > PERF_RECORD_KSYMBOL.
For compatibility, we can use attr.bpf_event bit (or attr.mmap2_plus) to turn on/off new variations of PERF_RECORD_MMAP2. Unless user runs perf-record and perf-report with different versions of perf tools, we should not see weird events.
> > New tools, seeing the perf_event_attr.header bit will know that this is > a "map" with just one symbol and will show that for both DSO name and > symbol. >
Hi Peter,
Could you please share your comments/suggestions on Arnaldo's proposal?
Thanks, Song
| |