[PATCH] perf dsos: Don't block when reading build IDs

[PATCH] perf dsos: Don't block when reading build IDs

[James Clark]

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@linaro.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.
---
 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@linaro.org>

Re: [PATCH] perf dsos: Don't block when reading build IDs

[Ian Rogers]

On Wed, Oct 22, 2025 at 8:46 AM James Clark <james.clark@linaro.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@linaro.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. 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?

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@linaro.org>
>

Re: [PATCH] perf dsos: Don't block when reading build IDs

[Namhyung Kim]

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@linaro.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@linaro.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 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.

I'm also curious if the device has samples actually.  It should be
checked by dso->hit.

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@linaro.org>
> >

Re: [PATCH] perf dsos: Don't block when reading build IDs

[Ian Rogers]

On Sat, Oct 25, 2025 at 4:59 PM Namhyung Kim <namhyung@kernel.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@linaro.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@linaro.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.

Agreed. Prior to using blocking we tried to imply from the file type
from stat, but that skipped things like symlinks :-/

> > 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@linaro.org>
> > >

Re: [PATCH] perf dsos: Don't block when reading build IDs

[Namhyung Kim]

On Sat, Oct 25, 2025 at 09:52:01PM -0700, Ian Rogers wrote:
> On Sat, Oct 25, 2025 at 4:59 PM Namhyung Kim <namhyung@kernel.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@linaro.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@linaro.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.
> 
> Agreed. Prior to using blocking we tried to imply from the file type
> from stat, but that skipped things like symlinks :-/
> 
> > > 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

There's -N/--no-buildid-cache option for that. :)


> 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.

Ouch.

> 
> > I'm also curious if the device has samples actually.  It should be
> > checked by dso->hit.

Anyway, --buildid-all would trigger the problem too.

> 
> 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.

Makes sense.

> There could be callers not marking dsos so they'd need altering, or
> two versions of the code.

Hmm.. I think --buildid-mmap should enable caching by default while
skips the header feature.

How about this?


diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index cb52aea9607d4612..ea778343f3c2d525 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1890,7 +1890,7 @@ record__finish_output(struct record *rec)
 	}
 
 	/* Buildid scanning disabled or build ID in kernel and synthesized map events. */
-	if (!rec->no_buildid) {
+	if (!rec->no_buildid || !rec->no_buildid_cache) {
 		process_buildids(rec);
 
 		if (rec->buildid_all)
@@ -3083,7 +3083,7 @@ static int perf_record_config(const char *var, const char *value, void *cb)
 		else if (!strcmp(value, "no-cache"))
 			rec->no_buildid_cache = true;
 		else if (!strcmp(value, "skip"))
-			rec->no_buildid = true;
+			rec->no_buildid = rec->no_buildid_cache = true;
 		else if (!strcmp(value, "mmap"))
 			rec->buildid_mmap = true;
 		else if (!strcmp(value, "no-mmap"))
@@ -4192,16 +4192,6 @@ int cmd_record(int argc, const char **argv)
 		record.opts.record_switch_events = true;
 	}
 
-	if (!rec->buildid_mmap) {
-		pr_debug("Disabling build id in synthesized mmap2 events.\n");
-		symbol_conf.no_buildid_mmap2 = true;
-	} else if (rec->buildid_mmap_set) {
-		/*
-		 * Explicitly passing --buildid-mmap disables buildid processing
-		 * and cache generation.
-		 */
-		rec->no_buildid = true;
-	}
 	if (rec->buildid_mmap && !perf_can_record_build_id()) {
 		pr_warning("Missing support for build id in kernel mmap events.\n"
 			   "Disable this warning with --no-buildid-mmap\n");
@@ -4210,6 +4200,16 @@ int cmd_record(int argc, const char **argv)
 	if (rec->buildid_mmap) {
 		/* Enable perf_event_attr::build_id bit. */
 		rec->opts.build_id = true;
+		/* Disable build-ID table in the header */
+		rec->no_buildid = true;
+	} else {
+		pr_debug("Disabling build id in synthesized mmap2 events.\n");
+		symbol_conf.no_buildid_mmap2 = true;
+	}
+
+	if (rec->no_buildid_set && rec->no_buildid) {
+		/* -B implies -N for historic reason */
+		rec->no_buildid_cache = true;
 	}
 
 	if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
@@ -4306,7 +4306,7 @@ int cmd_record(int argc, const char **argv)
 
 	err = -ENOMEM;
 
-	if (rec->no_buildid_cache || rec->no_buildid) {
+	if (rec->no_buildid_cache) {
 		disable_buildid_cache();
 	} else if (rec->switch_output.enabled) {
 		/*

Re: [PATCH] perf dsos: Don't block when reading build IDs

[James Clark]

On 26/10/2025 4:52 am, Ian Rogers wrote:
> On Sat, Oct 25, 2025 at 4:59 PM Namhyung Kim <namhyung@kernel.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@linaro.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@linaro.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.

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@linaro.org>
>>>>

Re: [PATCH] perf dsos: Don't block when reading build IDs

[Ian Rogers]

On Tue, Oct 28, 2025 at 3:33 AM James Clark <james.clark@linaro.org> wrote:
>
> On 26/10/2025 4:52 am, Ian Rogers wrote:
> > On Sat, Oct 25, 2025 at 4:59 PM Namhyung Kim <namhyung@kernel.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@linaro.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@linaro.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
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.

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@linaro.org>
> >>>>
>

Re: [PATCH] perf dsos: Don't block when reading build IDs

[James Clark]

On 28/10/2025 3:07 pm, Ian Rogers wrote:
> On Tue, Oct 28, 2025 at 3:33 AM James Clark <james.clark@linaro.org> wrote:
>>
>> On 26/10/2025 4:52 am, Ian Rogers wrote:
>>> On Sat, Oct 25, 2025 at 4:59 PM Namhyung Kim <namhyung@kernel.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@linaro.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@linaro.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.

> 
> 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@linaro.org>
>>>>>>
>>

Re: [PATCH] perf dsos: Don't block when reading build IDs

[Ian Rogers]

On Tue, Oct 28, 2025 at 8:26 AM James Clark <james.clark@linaro.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@linaro.org> wrote:
> >>
> >> On 26/10/2025 4:52 am, Ian Rogers wrote:
> >>> On Sat, Oct 25, 2025 at 4:59 PM Namhyung Kim <namhyung@kernel.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@linaro.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@linaro.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@linaro.org>
> >>>>>>
> >>
>