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

[Adrian Hunter]

On 21/09/22 10:26, Adrian Hunter wrote:
> On 20/09/22 21:30, Namhyung Kim wrote:
>> 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;
>>>> +     }
> 
> Also I guess the dsos optimization could be a separate patch ?

Disregard that - I thought there were other changes too.