Re: [PATCH v3 perf, bpf-next 1/4] perf, bpf: Introduce PERF_RECORD_BPF_EVENT

[Peter Zijlstra]

On Fri, Dec 14, 2018 at 10:48:57AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Dec 13, 2018 at 09:48:57PM +0000, Song Liu escreveu:

> > 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;
> >        * };
> >        */

Yes, something like that.

> 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.
> 
> 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.
> 
> 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.

That confuses me; the DSO for ksyms is [kernel|$modname] after all. And
BPF would like to have multiple symbols per 'program', so I can imagine
it would want to do something like:

	[bpf-progname1] function1
	[bpf-progname1] function2
	[bpf-progname2] progname2

The first being an bpf proglet with multiple functions, the second a
'legacy' bpf proglet with only a single function.

Trouble is; both PERF_RECORD_KSYM and MMAP* only have a single name[]
field. Now, I suppose we could add:

	char modname[MODULE_NAME_LEN]

or:

	u16 modlen;
	char modname[modlen];

or something along those lines.

Similarly; I would not expect the ftrace trampolines to all have a
different module name.

> > We can use ksym_type to encode BPF_EVENT, trampolines, or other type of ksym.
> > We can use flags or header.misc to encode ksym add/delete. Is this right?
> > 
> > If we go this direction, shall we reserve a few more bytes in it for different
> > types to use, like:
> > 
> >       /*
> >        * PERF_RECORD_KSYMBOL
> >        *
> >        * struct {
> >        *      struct perf_event_header header;
> >        *      u64                             addr;
> >        *      u32                             len;
> >        *      u16                             ksym_type;
> >        *      u16                             flags;
> >        *      u64                             data[2];
> >        *      char                            name[];
> >        *      struct sample_id                sample_id;
> >        * };
> >        */

Right; elsewhere you proposed keeping PERF_RECORD_BPF_EVENT for that;
which I think is clearer.

I think you can keep much of the current patches for that in fact.