Message-ID: <CAP-5=fWJMFYBtwPeH8DhzUG2jbjJ865sLojDtEc1+HDQZdpPoA@mail.gmail.com>
Date: Wed, 28 May 2025 16:11:13 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	Athira Rajeev <atrajeev@...ux.ibm.com>, Kajol Jain <kjain@...ux.ibm.com>, 
	Li Huafei <lihuafei1@...wei.com>, "Steinar H. Gunderson" <sesse@...gle.com>, 
	James Clark <james.clark@...aro.org>, Stephen Brennan <stephen.s.brennan@...cle.com>, 
	Andi Kleen <ak@...ux.intel.com>, Dmitry Vyukov <dvyukov@...gle.com>, 
	Zhongqiu Han <quic_zhonhan@...cinc.com>, Yicong Yang <yangyicong@...ilicon.com>, 
	Krzysztof Łopatowski <krzysztof.m.lopatowski@...il.com>, 
	"Dr. David Alan Gilbert" <linux@...blig.org>, Zixian Cai <fzczx123@...il.com>, 
	Steve Clevenger <scclevenger@...amperecomputing.com>, 
	Thomas Falcon <thomas.falcon@...el.com>, Martin Liska <martin.liska@....com>, 
	Martin Liška <m.liska@...link.cz>, 
	Song Liu <song@...nel.org>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/9] perf build-id: Mark DSO in sample callchains

On Wed, May 28, 2025 at 3:16 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Wed, May 28, 2025 at 01:54:41PM -0700, Ian Rogers wrote:
> > On Wed, May 28, 2025 at 1:41 PM Namhyung Kim <namhyung@...nel.org> wrote:
> > >
> > > On Mon, Apr 28, 2025 at 02:34:04PM -0700, Ian Rogers wrote:
> > > > Previously only the sample IP's map DSO would be marked hit for the
> > > > purposes of populating the build ID cache. Walk the call chain to mark
> > > > all IPs and DSOs.
> > >
> > > I think this is correct, but I'm afraid it'd also increase the processing
> > > time.  Do you happen to have any numbers?
> >
> > It increases time spent processing the data file but to get a large
> > data file I had to run for multiple seconds and I struggled to get the
> > performance cost of this to be in the milliseconds (ie a tiny fraction
> > of the record time). Ultimately I found the change imperceptible and
> > couldn't think of a good command line to make it perceptible.
>
> The worst case would be dwarf unwinding.  Maybe we can skip the
> processing if it takes too long..

This doesn't sound unreasonable but is somewhat beyond the scope of
what I wanted to do here, which relates to migrating from inodes to
buildids as identifiers for DSOs. It would be useful to get a bug
report on this being too slow.

> >
> > If the time is spent populating ~/.debug because more DSOs are marked
> > then this is fixing a bug and isn't a problem with the patch.
>
> Right, it's a good thing.
>
> >
> > My personal opinion is that it is somewhat surprising `perf record` is
> > post-processing the perf.data file at all, and -B and -N would be my
> > expected defaults - just as --buildid-mmap implies --no-buildid (-B).
>
> Otherwise nobody will run perf buildid-cache to add the info. :)

Right, but we know it is high overhead as we run a number of tests with -B:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/record_sideband.sh?h=perf-tools-next#n25
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/test_arm_spe.sh?h=perf-tools-next#n94

I agree that populating the buildid cache immediately after perf
record minimizes a gap where DSOs may change prior to perf report. If
the DSO changes midway through perf record it doesn't help as the
buildid information is only computed at the end of perf record.

Isn't there an argument that because we have build IDs we don't care
about the record to report race any more as debuginfod can find the
prior version by means of the build ID? What I mean:

Current perf default way:
1) Run perf record with mmap2 inode rather the buildid data
1.1) To avoid any DSOs in the perf.data from not being available
populate the buildid header in the perf.data and the buildid cache at
the end of perf record
2) Replace some DSO that's got a sample in the perf.data with a different DSO
3) Run perf report
3.1) The DSO's buildid is known via the header and the buildid cache
already contains the DSO

Current way with -N:
1) Run perf record with mmap2 inode rather the buildid data
1.1) To ensure DSOs have buildid data populate the buildid header in
the perf.data  at the end of perf record
2) Replace some DSO that's got a sample in the perf.data with a different DSO
3) Run perf report
3.1) The DSO's buildid is known via the header, debuginfod can
populate the buildid cache as needed

With --buildid-mmap:
1) Run perf record with mmap2 buildid data
1.1) No need to post process file to gather buildids
2) Replace some DSO that's got a sample in the perf.data with a different DSO
3) Run perf report
3.1) The DSO's buildid is known via the mmap2 events, debuginfod can
populate the buildid cache as needed

With the current way and the current way with -N there is a race with
the DSO changing midway through perf record. The buildid mmap closes
the race.
With -N and --buildid-mmap the buildid cache is populated based on use
by a tool trying to read the DSO, rather than ahead of time at the end
of perf record.

Is the risk of the race that much of an issue? I'm not sure, that's
why I'd say default to using -B (if we didn't switch to buildid mmaps
by default, but this series does that). You could opt into to covering
the race by adding a flag so the data is processed at the end of perf
record. You could use perf inject to add the build IDs.

As you say there's the cost at the end of perf record and I'm not sure
it is worth it, which is why I'd expect the default to be to opt into
having the cost. With --buildid-mmap I'm not seeing a race to cover
but this series populates the buildid cache as I know you've argued
that perf record should do this by default.

Thanks,
Ian

> > I didn't want to modify existing behaviors in these changes, however,
> > in this case I was just trying to make the existing behavior correct,
> > similar to fixing the same bug in `perf inject`.
>
> Thanks for your work,
> Namhyung
>

Powered by blists - more mailing lists