linux-kernel - Re: [PATCH] perf record: Cache build-ID of hit DSOs only

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [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