Re: [PATCH 3/4] perf record: Save DSO build-ID for synthesizing

[Namhyung Kim]

On Tue, Sep 20, 2022 at 7:00 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>
> On 16/09/22 20:59, Namhyung Kim wrote:
> > When synthesizing MMAP2 with build-id, it'd read the same file repeatedly as
> > it has no idea if it's done already.  Maintain a dsos to check that and skip
> > the file access if possible.
> >
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> >  tools/perf/util/synthetic-events.c | 49 +++++++++++++++++++++++++-----
> >  1 file changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index 9d4f5dacd154..e6978b2dee8f 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c
> > @@ -4,6 +4,7 @@
> >  #include "util/data.h"
> >  #include "util/debug.h"
> >  #include "util/dso.h"
> > +#include "util/dsos.h"
> >  #include "util/event.h"
> >  #include "util/evlist.h"
> >  #include "util/machine.h"
> > @@ -47,12 +48,25 @@
> >
> >  unsigned int proc_map_timeout = DEFAULT_PROC_MAP_PARSE_TIMEOUT;
> >
> > +static bool synth_init_done;
> > +static struct dsos synth_dsos;
> > +
> >  void perf_event__synthesize_start(void)
> >  {
> > +     if (synth_init_done)
> > +             return;
> > +
> > +     dsos__init(&synth_dsos);
> > +
> > +     synth_init_done = true;
> >  }
> >
> >  void perf_event__synthesize_stop(void)
> >  {
> > +     if (!synth_init_done)
> > +             return;
> > +
> > +     dsos__exit(&synth_dsos);
> >  }
> >
> >  int perf_tool__process_synth_event(struct perf_tool *tool,
> > @@ -374,26 +388,47 @@ static bool read_proc_maps_line(struct io *io, __u64 *start, __u64 *end,
> >  static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
> >                                            bool is_kernel)
> >  {
> > -     struct build_id bid;
> > +     struct build_id _bid, *bid = &_bid;
> > +     struct dso *dso = NULL;
> > +     struct dso_id id;
> >       int rc;
> >
> > -     if (is_kernel)
> > -             rc = sysfs__read_build_id("/sys/kernel/notes", &bid);
> > -     else
> > -             rc = filename__read_build_id(event->filename, &bid) > 0 ? 0 : -1;
> > +     if (is_kernel) {
> > +             rc = sysfs__read_build_id("/sys/kernel/notes", bid);
> > +             goto out;
> > +     }
> > +
> > +     id.maj = event->maj;
> > +     id.min = event->min;
> > +     id.ino = event->ino;
> > +     id.ino_generation = event->ino_generation;
> >
>
> There might be some paths missing perf_event__synthesize_start()
> e.g. perf trace
> So it would probably be safer to use lazy initialization for
> synth_dsos.

I thought about that too, but it'd need a cleanup routine at the end
anyway.  So I ended up having a pair of start/stop routines.

>
> Also, callers of perf_record_mmap2__read_build_id() have a struct
> machine so I wonder about putting synth_dsos in struct machine ?
> Or even just using machine->dsos ?

My intention was to use minimal info from struct dso - name, id and
build-id.  But as it already uses dsos/dso routines, it's not much
different from using the existing machine->dsos.

So yeah, I think it's good to reuse the existing one as it'd benefit
the build-id processing at the end.  Will change.

Thanks,
Namhyung


>
> > +     dso = dsos__findnew_id(&synth_dsos, event->filename, &id);
> > +     if (dso && dso->has_build_id) {
> > +             bid = &dso->bid;
> > +             rc = 0;
> > +             goto out;
> > +     }
> > +
> > +     rc = filename__read_build_id(event->filename, bid) > 0 ? 0 : -1;
> > +
> > +out:
> >       if (rc == 0) {
> > -             memcpy(event->build_id, bid.data, sizeof(bid.data));
> > -             event->build_id_size = (u8) bid.size;
> > +             memcpy(event->build_id, bid->data, sizeof(bid->data));
> > +             event->build_id_size = (u8) bid->size;
> >               event->header.misc |= PERF_RECORD_MISC_MMAP_BUILD_ID;
> >               event->__reserved_1 = 0;
> >               event->__reserved_2 = 0;
> > +
> > +             if (dso && !dso->has_build_id)
> > +                     dso__set_build_id(dso, bid);
> >       } else {
> >               if (event->filename[0] == '/') {
> >                       pr_debug2("Failed to read build ID for %s\n",
> >                                 event->filename);
> >               }
> >       }
> > +     dso__put(dso);
> >  }
> >
> >  int perf_event__synthesize_mmap_events(struct perf_tool *tool,
>