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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CAM9d7ci__tkn5qo9B00jvt=WXmM1czMGX1-mc=8m=xsOFw3VYQ@mail.gmail.com>
Date: Fri, 1 Aug 2025 10:14:48 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, 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 <linux-perf-users@...r.kernel.org>
Subject: Re: [PATCH] perf record: Cache build-ID of hit DSOs only

Hi Ian,

On Thu, Jul 31, 2025 at 5:10 PM Ian Rogers <irogers@...gle.com> wrote:
>
>
>
> On Fri, Aug 1, 2025, 1:29 AM Namhyung Kim <namhyung@...nel.org> wrote:
>>
>> 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.
>
>
> Sorry for the issue, I hope the market dsos is correct. I'm not sure the fixes is correct as that patch called out not populating the .debug. we should make sure that's still the case for people already using --buildid-mmap, they shouldn't have the .debug filled in. Sorry for my lack of a plain text email.

That's guarded by rec->no_buildid so it shouldn't change the behavior.

Thanks,
Namhyung