Re: [PATCH] perf dsos: Don't block when reading build IDs
From: James Clark
Date: Tue Oct 28 2025 - 06:33:38 EST
On 26/10/2025 4:52 am, Ian Rogers wrote:
On Sat, Oct 25, 2025 at 4:59 PM Namhyung Kim <namhyung@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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.
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@xxxxxxxxxx>