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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c288baa5-abb1-c34c-713c-b9d11873d34a@intel.com>
Date:   Wed, 21 Sep 2022 10:26:05 +0300
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ian Rogers <irogers@...gle.com>,
        linux-perf-users <linux-perf-users@...r.kernel.org>
Subject: Re: [PATCH 3/4] perf record: Save DSO build-ID for synthesizing

On 20/09/22 21:30, Namhyung Kim wrote:
> On Tue, Sep 20, 2022 at 7:00 AM Adrian Hunter <adrian.hunter@...el.com> 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@...nel.org>
>>> ---
>>>  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 ?

>>> +
>>> +     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,
>>