Message-ID: <CAP-5=fXvyv17hHkyovys4XPNuEu1gZ9L2in6DOgd+PBPpGx=9A@mail.gmail.com>
Date: Tue, 28 Oct 2025 08:50:34 -0700
From: Ian Rogers <irogers@...gle.com>
To: James Clark <james.clark@...aro.org>
Cc: Namhyung Kim <namhyung@...nel.org>, 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>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf dsos: Don't block when reading build IDs

On Tue, Oct 28, 2025 at 8:26 AM James Clark <james.clark@...aro.org> wrote:
>
>
>
> On 28/10/2025 3:07 pm, Ian Rogers wrote:
> > On Tue, Oct 28, 2025 at 3:33 AM James Clark <james.clark@...aro.org> wrote:
> >>
> >> On 26/10/2025 4:52 am, Ian Rogers wrote:
> >>> On Sat, Oct 25, 2025 at 4:59 PM Namhyung Kim <namhyung@...nel.org> wrote:
> >>>>
> >>>> Hello,
> >>>>
> >>>> On Fri, Oct 24, 2025 at 11:26:18AM -0700, Ian Rogers wrote:
> >>>>> On Wed, Oct 22, 2025 at 8:46 AM James Clark <james.clark@...aro.org> wrote:
> >>>>>>
> >>>>>> The following command will hang consistently when the GPU is being used
> >>>>>> due to non regular files (e.g. /dev/dri/renderD129, /dev/dri/card2)
> >>>>>> being opened to read build IDs:
> >>>>>>
> >>>>>>    $ perf record -asdg -e cpu-clock -- true
> >>>>>>
> >>>>>> Change to non blocking reads to avoid the hang here:
> >>>>>>
> >>>>>>     #0  __libc_pread64 (offset=<optimised out>, count=0, buf=0x7fffffffa4a0, fd=237) at ../sysdeps/unix/sysv/linux/pread64.c:25
> >>>>>>     #1  __libc_pread64 (fd=237, buf=0x7fffffffa4a0, count=0, offset=0) at ../sysdeps/unix/sysv/linux/pread64.c:23
> >>>>>>     #2  ?? () from /lib/x86_64-linux-gnu/libelf.so.1
> >>>>>>     #3  read_build_id (filename=0x5555562df333 "/dev/dri/card2", bid=0x7fffffffb680, block=true) at util/symbol-elf.c:880
> >>>>>>     #4  filename__read_build_id (filename=0x5555562df333 "/dev/dri/card2", bid=0x7fffffffb680, block=true) at util/symbol-elf.c:924
> >>>>>>     #5  dsos__read_build_ids_cb (dso=0x5555562df1d0, data=0x7fffffffb750) at util/dsos.c:84
> >>>>>>     #6  __dsos__for_each_dso (dsos=0x55555623de68, cb=0x5555557e7030 <dsos__read_build_ids_cb>, data=0x7fffffffb750) at util/dsos.c:59
> >>>>>>     #7  dsos__for_each_dso (dsos=0x55555623de68, cb=0x5555557e7030 <dsos__read_build_ids_cb>, data=0x7fffffffb750) at util/dsos.c:503
> >>>>>>     #8  dsos__read_build_ids (dsos=0x55555623de68, with_hits=true) at util/dsos.c:107
> >>>>>>     #9  machine__read_build_ids (machine=0x55555623da58, with_hits=true) at util/build-id.c:950
> >>>>>>     #10 perf_session__read_build_ids (session=0x55555623d840, with_hits=true) at util/build-id.c:956
> >>>>>>     #11 write_build_id (ff=0x7fffffffb958, evlist=0x5555562323d0) at util/header.c:327
> >>>>>>     #12 do_write_feat (ff=0x7fffffffb958, type=2, p=0x7fffffffb950, evlist=0x5555562323d0, fc=0x0) at util/header.c:3588
> >>>>>>     #13 perf_header__adds_write (header=0x55555623d840, evlist=0x5555562323d0, fd=3, fc=0x0) at util/header.c:3632
> >>>>>>     #14 perf_session__do_write_header (session=0x55555623d840, evlist=0x5555562323d0, fd=3, at_exit=true, fc=0x0, write_attrs_after_data=false) at util/header.c:3756
> >>>>>>     #15 perf_session__write_header (session=0x55555623d840, evlist=0x5555562323d0, fd=3, at_exit=true) at util/header.c:3796
> >>>>>>     #16 record__finish_output (rec=0x5555561838d8 <record>) at builtin-record.c:1899
> >>>>>>     #17 __cmd_record (rec=0x5555561838d8 <record>, argc=2, argv=0x7fffffffe320) at builtin-record.c:2967
> >>>>>>     #18 cmd_record (argc=2, argv=0x7fffffffe320) at builtin-record.c:4453
> >>>>>>     #19 run_builtin (p=0x55555618cbb0 <commands+288>, argc=9, argv=0x7fffffffe320) at perf.c:349
> >>>>>>     #20 handle_internal_command (argc=9, argv=0x7fffffffe320) at perf.c:401
> >>>>>>     #21 run_argv (argcp=0x7fffffffe16c, argv=0x7fffffffe160) at perf.c:445
> >>>>>>     #22 main (argc=9, argv=0x7fffffffe320) at perf.c:553
> >>>>>>
> >>>>>> Fixes: 53b00ff358dc ("perf record: Make --buildid-mmap the default")
> >>>>>> Signed-off-by: James Clark <james.clark@...aro.org>
> >>>>>> ---
> >>>>>> I'm not actually sure if this is the right fix for this. Commit
> >>>>>> 2c369d91d093 ("perf symbol: Add blocking argument to
> >>>>>> filename__read_build_id") which added the blocking argument says that it
> >>>>>> should be non-blocking reads for event synthesis, but the callstack here
> >>>>>> is when writing the header out.
> >>>>>>
> >>>>>> There was also an is_regular_file() check added in commit c21986d33d6b
> >>>>>> ("perf unwind-libdw: skip non-regular files"), which presumably falls
> >>>>>> afoul of the "which unfortunately fails for symlinks" part of the other
> >>>>>> linked commit message?
> >>>>>>
> >>>>>> So I can't tell if we should add the is_regular_file() check here too,
> >>>>>> or just set it to non-blocking, or it needs some extra state to be
> >>>>>> passed all the way down to dsos__read_build_ids_cb() to do different
> >>>>>> things.
> >>>>>
> >>>>> The fix lgtm but I worry about making all the build ID reading
> >>>>> non-blocking meaning build IDs getting lost.
> >>>>
> >>>> I'm not sure what non-blocking means for regular file system operations
> >>>> on a block device.  But it may have some impact on regular files on a
> >>>> network filesystem.
> >>
> >> It depends on the filesystem, but I think the assurances given by
> >> O_NONBLOCK are very weak anyway. It can be practically ignored or do
> >> different things in different situations. Especially as we don't handle
> >> EAGAIN or do anything fancy we shouldn't use it.
> >>
> >> For our case it looks like we should always do blocking reads but make
> >> sure to not open a non-regular file.
> >>
> >>>
> >>> Agreed. Prior to using blocking we tried to imply from the file type
> >>> from stat, but that skipped things like symlinks :-/
> >>>
> >>
> >> Am I missing something here? is_regular_file() uses stat() which returns
> >> info about the target file, rather than the symlink, so they wouldn't be
> >> skipped. lstat() is the one that returns info about the link file.
> >>
> >> I tested is_regular_file() with links, links to links, pipes, files and
> >> devices and it works as expected and would avoid the need to use O_NONBLOCK.
> >
> > I don't mind we back out the use of the is_block parameter and use
> > is_regular_file. In the code before the change to use is_block the
> > callers would call is_regular_file and then call
> > filename__read_build_id. This seemed error prone as some callers were
> > doing the is_regular_file check and others not. I think I favor just
> > getting rid of the argument but putting the is_regular_file check in
> > filename__read_build_id. I'd switched to using O_NONBLOCK rather than
>
> That makes sense to me. Should I make that change in addition to
> Namhyung's patch?
>
> > is_regular_file not just because of symlinks but just because it
> > wasn't clear that anything non-regular may still have a build id.
>
> That I'm not sure about either. Can we assume that no non-regular files
> have build IDs as we're not handling EAGAIN anyway? Or at least if they
> do it's not currently supported.

Perhaps what we want is to O_NONBLOCK just when !is_regular_file in
the filename__read_build_id functions (there's one in symbol-minimal
and another in symbol-elf). That way we read regular files without
worry that O_NONBLOCK could fail for say a network filesystem. If a
non-regular file fails to open with O_NONBLOCK then it was probably a
block device, ... If it does open then we can try to get a build-ID
just in case this is just something weird. But tbh, we could skip the
whole O_NONBLOCK thing until we have evidence it is actually a problem
somewhere. It'd be cool if you could make a patch for this.

Thanks,
Ian

> >
> > There's the separate issue of why are we writing out buildids in the
> > features when the buildid-mmaps contain the same data. It seems
> > Namhyung has a patch just about optimizing that case :-)
> >
> > Thanks,
> > Ian
> >
> >> James
> >>
> >>>>> It seems that generating
> >>>>> the build ID feature table is unnecessary if we have build ID mmap
> >>>>> events, including synthesized ones that will have read the build IDs.
> >>>>> I wonder if a better "fix" is:
> >>>>> ```
> >>>>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> >>>>> index cb52aea9607d..15f75c70eb76 100644
> >>>>> --- a/tools/perf/builtin-record.c
> >>>>> +++ b/tools/perf/builtin-record.c
> >>>>> @@ -1842,7 +1842,7 @@ static void record__init_features(struct record *rec)
> >>>>>          for (feat = HEADER_FIRST_FEATURE; feat < HEADER_LAST_FEATURE; feat++)
> >>>>>                  perf_header__set_feat(&session->header, feat);
> >>>>>
> >>>>> -       if (rec->no_buildid)
> >>>>> +       if (rec->no_buildid || rec->buildid_mmap)
> >>>>>                  perf_header__clear_feat(&session->header, HEADER_BUILD_ID);
> >>>>>
> >>>>>          if (!have_tracepoints(&rec->evlist->core.entries))
> >>>>> ```
> >>>>> that should disable the build ID feature table generation when build
> >>>>> ID mmaps are in use (the default). Having the build IDs twice in the
> >>>>> data file feels redundant. Or we could do your fix or both, wdyt?
> >>>>
> >>>> I'm ok to remove the header feature but I think it should update
> >>>> build-ID cache even with this change.
> >>>
> >>> Yeah, dropping the feature writing also impacts updating the build ID
> >>> cache because:
> >>> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.c?h=perf-tools-next#n338
> >>> It is kind of strange that writing a header feature does this. What if
> >>> I want to write a header with build IDs but not update the cache? It'd
> >>> make more sense, I think, for perf_session__cache_build_ids to be
> >>> called explicitly. There is also the global no_buildid_cache
> >>> controlling behavior:
> >>> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/build-id.c?h=perf-tools-next#n43
> >>> But that's kind of a hack as we may have >1 session such as with TPEBS.
> >>>
> >>>> I'm also curious if the device has samples actually.  It should be
> >>>> checked by dso->hit.
> >>>
> >>> In this case the header writing is happening after the samples have
> >>> been processed, but it looks like marking of samples doesn't consider
> >>> data addresses:
> >>> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/build-id.c?h=perf-tools-next#n55
> >>> just sample->ip and the callchain. If the marking was updated for data
> >>> pages then just writing build IDs for marked dsos would make sense.
> >>> There could be callers not marking dsos so they'd need altering, or
> >>> two versions of the code.
> >>>
> >>> Thanks,
> >>> Ian
> >>>
> >>>> Thanks,
> >>>> Namhyung
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>> Ian
> >>>>>
> >>>>>> ---
> >>>>>>    tools/perf/util/dsos.c | 5 +++--
> >>>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
> >>>>>> index 64c1d65b0149..5e1c815d7cb0 100644
> >>>>>> --- a/tools/perf/util/dsos.c
> >>>>>> +++ b/tools/perf/util/dsos.c
> >>>>>> @@ -81,13 +81,14 @@ static int dsos__read_build_ids_cb(struct dso *dso, void *data)
> >>>>>>                   return 0;
> >>>>>>           }
> >>>>>>           nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
> >>>>>> -       if (filename__read_build_id(dso__long_name(dso), &bid, /*block=*/true) > 0) {
> >>>>>> +       /* Don't block in case path isn't for a regular file. */
> >>>>>> +       if (filename__read_build_id(dso__long_name(dso), &bid, /*block=*/false) > 0) {
> >>>>>>                   dso__set_build_id(dso, &bid);
> >>>>>>                   args->have_build_id = true;
> >>>>>>           } else if (errno == ENOENT && dso__nsinfo(dso)) {
> >>>>>>                   char *new_name = dso__filename_with_chroot(dso, dso__long_name(dso));
> >>>>>>
> >>>>>> -               if (new_name && filename__read_build_id(new_name, &bid, /*block=*/true) > 0) {
> >>>>>> +               if (new_name && filename__read_build_id(new_name, &bid, /*block=*/false) > 0) {
> >>>>>>                           dso__set_build_id(dso, &bid);
> >>>>>>                           args->have_build_id = true;
> >>>>>>                   }
> >>>>>>
> >>>>>> ---
> >>>>>> base-commit: a1d8548c23076c66d96647f5f6f25aa43567f247
> >>>>>> change-id: 20251022-james-perf-fix-dso-block-ca1d8437bbc0
> >>>>>>
> >>>>>> Best regards,
> >>>>>> --
> >>>>>> James Clark <james.clark@...aro.org>
> >>>>>>
> >>
>

Powered by blists - more mailing lists