Re: [PATCH v3 5/9] perf build-id: Mark DSO in sample callchains

[Namhyung Kim <namhyung@kernel.org>]

On Wed, May 28, 2025 at 04:11:13PM -0700, Ian Rogers wrote:
> On Wed, May 28, 2025 at 3:16 PM Namhyung Kim <namhyung@kernel.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@kernel.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.

Yeah, I just wanted to keep the original behavior to save the used
binaries in the build-ID cache.  But we can use debuginfod if it's
available as you said.

I think it's a separate topic whether we prefer the build-ID cache or
debuginfod.  We should be able to change the default behavior with a
config option later.  So let's keep the caching for now.

Thanks,
Namhyung