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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aIt9bTQhAo8G3oqH@x1>
Date: Thu, 31 Jul 2025 11:27:57 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Namhyung Kim <namhyung@...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

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.

Ok, now that makes sense:

Reviewed-by: Arnaldo Carvalho de Melo <acme@...hat.com>

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.

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
>  tools/perf/util/build-id.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index e2b295fe4d2fe552..a7018a3b0437a699 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -872,7 +872,7 @@ static int dso__cache_build_id(struct dso *dso, struct machine *machine,
>  	char *allocated_name = NULL;
>  	int ret = 0;
>  
> -	if (!dso__has_build_id(dso))
> +	if (!dso__has_build_id(dso) || !dso__hit(dso))
>  		return 0;
>  
>  	if (dso__is_kcore(dso)) {
> -- 
> 2.50.1