[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aIuoDHBGeoFYF2fh@google.com>
Date: Thu, 31 Jul 2025 10:29:48 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Ian Rogers <irogers@...gle.com>, Kan Liang <kan.liang@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] perf record: Cache build-ID of hit DSOs only
Hi Arnaldo,
On Thu, Jul 31, 2025 at 11:27:57AM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, Jul 31, 2025 at 12:03:30AM -0700, Namhyung Kim wrote:
> > It post-processes samples to find which DSO has samples. Based on that
> > info, it can save used DSOs in the build-ID cache directory. But for
> > some reason, it saves all DSOs without checking the hit mark. Skipping
> > unused DSOs can give some speedup especially with --buildid-mmap being
> > default.
>
> > On my idle machine, `time perf record -a sleep 1` goes down from 3 sec
> > to 1.5 sec with this change.
>
> Good stuff, and this is in line with the original intent, don't cache
> uninteresting things.
>
> But now I have do some digging, as this should've been the case since
> the start, why would we go to the trouble of traversing perf.data,
> processing all samples, yadda, yadda to then not look at it when caching
> files?
>
> The whole process of reading the build ids at the end is done here:
>
> bool dsos__read_build_ids(struct dsos *dsos, bool with_hits)
> {
> struct dsos__read_build_ids_cb_args args = {
> .with_hits = with_hits,
> .have_build_id = false,
> };
>
> dsos__for_each_dso(dsos, dsos__read_build_ids_cb, &args);
> return args.have_build_id;
> }
>
> And that dsos__read_build_ids_cb thing specifically looks at:
>
> static int dsos__read_build_ids_cb(struct dso *dso, void *data)
> {
> struct dsos__read_build_ids_cb_args *args = data;
> struct nscookie nsc;
> struct build_id bid = { .size = 0, };
>
> if (args->with_hits && !dso__hit(dso) && !dso__is_vdso(dso))
> return 0;
> <SNIP>
>
> So it will not try to read the build id if that DSO has no samples.
>
> But, that was written before PERF_RECORD_MMAP* came with a build-id, so
> it _will_ have a build-id and thus checking if it has hits is needed.
>
> In the past DSOs without hits wouldn't have a build-id because
> dsos__read_build_ids_cb() would not read that build-id from the ELF
> file.
Yep, I think that's the reason.
>
> Ok, now that makes sense:
>
> Reviewed-by: Arnaldo Carvalho de Melo <acme@...hat.com>
Thanks!
>
> This could have a Fixes attached to it, one that doesn't fixes something
> that is not working, but speeds up processing by overcoming an oversight
> when adding build-ids to MMAP records, so I think a:
>
> Fixes: e29386c8f7d71fa5 ("perf record: Add --buildid-mmap option to enable PERF_RECORD_MMAP2's build id")
>
> Is worth.
Sure, will add.
Thanks,
Namhyung
Powered by blists - more mailing lists