Re: [PATCH 2/3] perf: collect BPF metadata from existing BPF programs

[Namhyung Kim]

Hi Blake,

On Tue, Jun 03, 2025 at 02:27:53PM -0700, Blake Jones wrote:
> Hi Namhyung,
> 
> On Tue, Jun 3, 2025 at 1:15 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > On Wed, May 21, 2025 at 03:27:24PM -0700, Blake Jones wrote:
> > > Look for .rodata maps, find ones with 'bpf_metadata_' variables, extract
> > > their values as strings, and create a new PERF_RECORD_BPF_METADATA
> > > synthetic event using that data. The code gets invoked from the existing
> > > routine perf_event__synthesize_one_bpf_prog().
> >
> > It would be great if you can show an example how those metadata is
> > constructed and shared between BPF programs.
> 
> I've added the following to my commit message:
> 
> | For example, a BPF program with the following variables:
> |
> |     const char bpf_metadata_version[] SEC(".rodata") = "3.14159";
> |     int bpf_metadata_value[] SEC(".rodata") = 42;
> |
> | would generate a PERF_RECORD_BPF_METADATA record with:
> |
> |     .prog_name        = <BPF program name, e.g. "bpf_prog_a1b2c3_foo">
> |     .nr_entries       = 2
> |     .entries[0].key   = "version"
> |     .entries[0].value = "3.14159"
> |     .entries[1].key   = "value"
> |     .entries[1].value = "42"
> |
> | Each of the BPF programs and subprograms that share those variables would
> | get a distinct PERF_RECORD_BPF_METADATA record, with the ".prog_name" showing
> | the name of each program or subprogram. The prog_name is deliberately the
> | same as the ".name" field in the corresponding PERF_RECORD_KSYMBOL record.

Thanks!

> 
> > IIUC the metadata is collected for each BPF program which may have
> > multiple subprograms.  Then this patch creates multiple PERF_RECORD_
> > BPF_METADATA for each subprogram, right?
> >
> > Can it be shared using the BPF program ID?
> 
> In theory, yes, it could be shared. But I want to be able to correlate them
> with the corresponding PERF_RECORD_KSYMBOL events, and KSYMBOL events for
> subprograms don't have the full-program ID, so I wouldn't be able to do that.

It's unfortunate that KSYMBOL doesn't have the program ID, but IIRC the
following BPF_EVENT should have it.  I think it's safe to think KSYMBOLs
belong to the BPF_EVENT when they are from the same thread.

> 
> > > +     rodata = calloc(1, map_info.value_size);
> >
> > You can use 'zalloc()' instead, in other places too.
> 
> Fixed, thanks.
> 
> > > +void bpf_metadata_free(struct bpf_metadata *metadata)
> > > +{
> > > +     if (metadata == NULL)
> > > +             return;
> > > +     for (__u32 index = 0; index < metadata->nr_prog_names; index++)
> > > +             free(metadata->prog_names[index]);
> > > +     if (metadata->prog_names != NULL)
> > > +             free(metadata->prog_names);
> > > +     if (metadata->event != NULL)
> > > +             free(metadata->event);
> >
> > No need to NULL change for free().
> 
> I've removed the NULL checks.
> 
> > > +static int synthesize_perf_record_bpf_metadata(
> > > [...]
> > > +     for (__u32 index = 0; index < metadata->nr_prog_names; index++) {
> > > +             memcpy(event->bpf_metadata.prog_name,
> > > +                    metadata->prog_names[index], BPF_PROG_NAME_LEN);
> >
> > Is it possible to call synthesize_bpf_prog_name() directly to the
> > event->bpf_metadata.prog_name instead of saving it metadata->prog_names?
> 
> Not with the way the code is currently structured - we need the BTF data
> to call synthesize_bpf_prog_name(), and that's allocated and freed inside
> of bpf_metadata_create().

I see.  You already freed the map data and BTF.  Ok, it's not a big deal
and probably not needed if we can switch to BPF ID.

Thanks,
Namhyung