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

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:

quoted

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.

quoted

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.