LinuxLists.cc - [RFC] perf record: Disable debuginfod by default

2021-12-09 20:04:33

Subject: [RFC] perf record: Disable debuginfod by default

hi,
after migrating to fedora 35 I found perf record hanging on exit
and it's because fedora 35 sets DEBUGINFOD_URLS that triggers
debuginfod query which might take long time to process.

I discussed this briefly with Frank and I'm sending the change
to disable debuginfod by default in perf record.

Frank had other idea we could discuss here to fork or just spawn
"/usr/bin/debuginfod-find ...." into background after perf record.

Perhaps there are other ways as well, hence this is RFC ;-)

thanks,
jirka

---
Fedora 35 sets by default DEBUGINFOD_URLS, which might lead
to unexpected stalls in perf record exit path, when we try
to cache profiled binaries.

# DEBUGINFOD_PROGRESS=1 ./perf record -a
^C[ perf record: Woken up 1 times to write data ]
Downloading from https://debuginfod.fedoraproject.org/ 447069
Downloading from https://debuginfod.fedoraproject.org/ 1502175
Downloading \^Z

Disabling DEBUGINFOD_URLS by default in perf record and adding
debuginfod option and .perfconfig variable support to enable id.

Default without debuginfo processing:
# perf record -a

Using system debuginfod setup:
# perf record -a --debuginfod

Using custom debuginfd url:
# perf record -a --debuginfod='https://evenbetterdebuginfodserver.krava'

Adding single perf_debuginfod_setup function and using
it also in perf buildid-cache command.

Signed-off-by: Jiri Olsa <[email protected]>
---
.../perf/Documentation/perf-buildid-cache.txt | 5 +++-
tools/perf/Documentation/perf-config.txt | 9 +++++++
tools/perf/Documentation/perf-record.txt | 9 +++++++
tools/perf/builtin-buildid-cache.c | 25 +++++++++++--------
tools/perf/builtin-record.c | 13 ++++++++++
tools/perf/util/util.c | 15 +++++++++++
tools/perf/util/util.h | 6 +++++
7 files changed, 70 insertions(+), 12 deletions(-)

diff --git a/tools/perf/Documentation/perf-buildid-cache.txt b/tools/perf/Documentation/perf-buildid-cache.txt
index cd8ce6e8ec12..7e44b419d301 100644
--- a/tools/perf/Documentation/perf-buildid-cache.txt
+++ b/tools/perf/Documentation/perf-buildid-cache.txt
@@ -74,12 +74,15 @@ OPTIONS
used when creating a uprobe for a process that resides in a
different mount namespace from the perf(1) utility.

---debuginfod=URLs::
+--debuginfod[=URLs]::
Specify debuginfod URL to be used when retrieving perf.data binaries,
it follows the same syntax as the DEBUGINFOD_URLS variable, like:

buildid-cache.debuginfod=http://192.168.122.174:8002

+ If the URLs is not specified, the value of DEBUGINFOD_URLS
+ system environment variable is used.
+
SEE ALSO
--------
linkperf:perf-record[1], linkperf:perf-report[1], linkperf:perf-buildid-list[1]
diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
index 3bb75c1f25e8..0420e71698ee 100644
--- a/tools/perf/Documentation/perf-config.txt
+++ b/tools/perf/Documentation/perf-config.txt
@@ -587,6 +587,15 @@ record.*::
Use 'n' control blocks in asynchronous (Posix AIO) trace writing
mode ('n' default: 1, max: 4).

+ record.debuginfod::
+ Specify debuginfod URL to be used when cacheing perf.data binaries,
+ it follows the same syntax as the DEBUGINFOD_URLS variable, like:
+
+ http://192.168.122.174:8002
+
+ If the URLs is 'system', the value of DEBUGINFOD_URLS system environment
+ variable is used.
+
diff.*::
diff.order::
This option sets the number of columns to sort the result.
diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 55df7b073a55..9ccc75935bc5 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -715,6 +715,15 @@ measurements:

include::intel-hybrid.txt[]

+--debuginfod[=URLs]::
+ Specify debuginfod URL to be used when cacheing perf.data binaries,
+ it follows the same syntax as the DEBUGINFOD_URLS variable, like:
+
+ http://192.168.122.174:8002
+
+ If the URLs is not specified, the value of DEBUGINFOD_URLS
+ system environment variable is used.
+
SEE ALSO
--------
linkperf:perf-stat[1], linkperf:perf-list[1], linkperf:perf-intel-pt[1]
diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index 0db3cfc04c47..cd381693658b 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -351,10 +351,14 @@ static int build_id_cache__show_all(void)

static int perf_buildid_cache_config(const char *var, const char *value, void *cb)
{
- const char **debuginfod = cb;
+ struct perf_debuginfod *di = cb;

- if (!strcmp(var, "buildid-cache.debuginfod"))
- *debuginfod = strdup(value);
+ if (!strcmp(var, "buildid-cache.debuginfod")) {
+ di->urls = strdup(value);
+ if (!di->urls)
+ return -ENOMEM;
+ di->set = true;
+ }

return 0;
}
@@ -373,8 +377,8 @@ int cmd_buildid_cache(int argc, const char **argv)
*purge_name_list_str = NULL,
*missing_filename = NULL,
*update_name_list_str = NULL,
- *kcore_filename = NULL,
- *debuginfod = NULL;
+ *kcore_filename = NULL;
+ struct perf_debuginfod debuginfod = { };
char sbuf[STRERR_BUFSIZE];

struct perf_data data = {
@@ -399,8 +403,10 @@ int cmd_buildid_cache(int argc, const char **argv)
OPT_BOOLEAN('f', "force", &force, "don't complain, do it"),
OPT_STRING('u', "update", &update_name_list_str, "file list",
"file(s) to update"),
- OPT_STRING(0, "debuginfod", &debuginfod, "debuginfod url",
- "set debuginfod url"),
+ OPT_STRING_OPTARG_SET(0, "debuginfod", &debuginfod.urls,
+ &debuginfod.set, "debuginfod urls",
+ "Enable debuginfod data retrieval from DEBUGINFOD_URLS or specified urls",
+ "system"),
OPT_INCR('v', "verbose", &verbose, "be more verbose"),
OPT_INTEGER(0, "target-ns", &ns_id, "target pid for namespace context"),
OPT_END()
@@ -425,10 +431,7 @@ int cmd_buildid_cache(int argc, const char **argv)
if (argc || !(list_files || opts_flag))
usage_with_options(buildid_cache_usage, buildid_cache_options);

- if (debuginfod) {
- pr_debug("DEBUGINFOD_URLS=%s\n", debuginfod);
- setenv("DEBUGINFOD_URLS", debuginfod, 1);
- }
+ perf_debuginfod_setup(&debuginfod);

/* -l is exclusive. It can not be used with other options. */
if (list_files && opts_flag) {
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 0338b813585a..2da2a6acbb8c 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -111,6 +111,7 @@ struct record {
unsigned long long samples;
struct mmap_cpu_mask affinity_mask;
unsigned long output_max_size; /* = 0: unlimited */
+ struct perf_debuginfod debuginfod;
};

static volatile int done;
@@ -2177,6 +2178,12 @@ static int perf_record_config(const char *var, const char *value, void *cb)
rec->opts.nr_cblocks = nr_cblocks_default;
}
#endif
+ if (!strcmp(var, "record.debuginfod")) {
+ rec->debuginfod.urls = strdup(value);
+ if (!rec->debuginfod.urls)
+ return -ENOMEM;
+ rec->debuginfod.set = true;
+ }

return 0;
}
@@ -2663,6 +2670,10 @@ static struct option __record_options[] = {
parse_control_option),
OPT_CALLBACK(0, "synth", &record.opts, "no|all|task|mmap|cgroup",
"Fine-tune event synthesis: default=all", parse_record_synth_option),
+ OPT_STRING_OPTARG_SET(0, "debuginfod", &record.debuginfod.urls,
+ &record.debuginfod.set, "debuginfod urls",
+ "Enable debuginfod data retrieval from DEBUGINFOD_URLS or specified urls",
+ "system"),
OPT_END()
};

@@ -2716,6 +2727,8 @@ int cmd_record(int argc, const char **argv)
if (err)
return err;

+ perf_debuginfod_setup(&record.debuginfod);
+
/* Make system wide (-a) the default target. */
if (!argc && target__none(&rec->opts.target))
rec->opts.target.system_wide = true;
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index df3c4671be72..fb4f6616b5fa 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -416,3 +416,18 @@ char *perf_exe(char *buf, int len)
}
return strcpy(buf, "perf");
}
+
+void perf_debuginfod_setup(struct perf_debuginfod *di)
+{
+ /*
+ * By default '!di->set' we clear DEBUGINFOD_URLS, so debuginfod
+ * processing is not triggered, otherwise we set it to 'di->urls'
+ * value. If 'di->urls' is "system" we keep DEBUGINFOD_URLS value.
+ */
+ if (!di->set)
+ setenv("DEBUGINFOD_URLS", "", 1);
+ else if (di->urls && strcmp(di->urls, "system"))
+ setenv("DEBUGINFOD_URLS", di->urls, 1);
+
+ pr_debug("DEBUGINFOD_URLS=%s\n", getenv("DEBUGINFOD_URLS"));
+}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 9f0d36ba77f2..4359f5af2de7 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -68,4 +68,10 @@ void test_attr__init(void);
struct perf_event_attr;
void test_attr__open(struct perf_event_attr *attr, pid_t pid, int cpu,
int fd, int group_fd, unsigned long flags);
+
+struct perf_debuginfod {
+ const char *urls;
+ bool set;
+};
+void perf_debuginfod_setup(struct perf_debuginfod *di);
#endif /* GIT_COMPAT_UTIL_H */
--
2.33.1

2021-12-09 23:39:37

by Namhyung Kim

[permalink] [raw]

Subject: Re: [RFC] perf record: Disable debuginfod by default

Hi Jiri,

On Thu, Dec 9, 2021 at 12:04 PM Jiri Olsa <[email protected]> wrote:
>
> hi,
> after migrating to fedora 35 I found perf record hanging on exit
> and it's because fedora 35 sets DEBUGINFOD_URLS that triggers
> debuginfod query which might take long time to process.
>
> I discussed this briefly with Frank and I'm sending the change
> to disable debuginfod by default in perf record.
>
> Frank had other idea we could discuss here to fork or just spawn
> "/usr/bin/debuginfod-find ...." into background after perf record.
>
> Perhaps there are other ways as well, hence this is RFC ;-)

I thought the debuginfod was for perf report, not record.
Maybe I'm missing something but how about moving it to
report? We can talk to debuginfod after checking the local
build-id cache and binary on the system.

Still, we can have perf buildid-cache to move it from the
debuginfod to local cache.

Thanks,
Namhyung

2021-12-10 08:04:43

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [RFC] perf record: Disable debuginfod by default

2021-12-10 12:23:39

by Jiri Olsa

[permalink] [raw]

Subject: Re: [RFC] perf record: Disable debuginfod by default

On Thu, Dec 09, 2021 at 03:39:20PM -0800, Namhyung Kim wrote:
> Hi Jiri,
>
> On Thu, Dec 9, 2021 at 12:04 PM Jiri Olsa <[email protected]> wrote:
> >
> > hi,
> > after migrating to fedora 35 I found perf record hanging on exit
> > and it's because fedora 35 sets DEBUGINFOD_URLS that triggers
> > debuginfod query which might take long time to process.
> >
> > I discussed this briefly with Frank and I'm sending the change
> > to disable debuginfod by default in perf record.
> >
> > Frank had other idea we could discuss here to fork or just spawn
> > "/usr/bin/debuginfod-find ...." into background after perf record.
> >
> > Perhaps there are other ways as well, hence this is RFC ;-)
>
> I thought the debuginfod was for perf report, not record.
> Maybe I'm missing something but how about moving it to
> report? We can talk to debuginfod after checking the local
> build-id cache and binary on the system.

at the end of the perf record we populate buildid cache
with profiled binaries for the current perf.data

**IF** there's DEBUGINFOD_URLS defined, that code will
also ask debuginfod for binaries it could not find on
the system

>
> Still, we can have perf buildid-cache to move it from the
> debuginfod to local cache.

yep, we have that already

jirka

2021-12-10 12:24:48

by Jiri Olsa

[permalink] [raw]

Subject: Re: [RFC] perf record: Disable debuginfod by default

On Fri, Dec 10, 2021 at 09:04:25AM +0100, Peter Zijlstra wrote:
> On Thu, Dec 09, 2021 at 09:04:25PM +0100, Jiri Olsa wrote:
> > hi,
> > after migrating to fedora 35 I found perf record hanging on exit
> > and it's because fedora 35 sets DEBUGINFOD_URLS that triggers
> > debuginfod query which might take long time to process.
> >
> > I discussed this briefly with Frank and I'm sending the change
> > to disable debuginfod by default in perf record.
> >
> > Frank had other idea we could discuss here to fork or just spawn
> > "/usr/bin/debuginfod-find ...." into background after perf record.
> >
> > Perhaps there are other ways as well, hence this is RFC ;-)
> >
> > thanks,
> > jirka
> >
> >
> > ---
> > Fedora 35 sets by default DEBUGINFOD_URLS, which might lead
> > to unexpected stalls in perf record exit path, when we try
> > to cache profiled binaries.
> >
> > # DEBUGINFOD_PROGRESS=1 ./perf record -a
> > ^C[ perf record: Woken up 1 times to write data ]
> > Downloading from https://debuginfod.fedoraproject.org/ 447069
> > Downloading from https://debuginfod.fedoraproject.org/ 1502175
> > Downloading \^Z
> >
> > Disabling DEBUGINFOD_URLS by default in perf record and adding
> > debuginfod option and .perfconfig variable support to enable id.
> >
> > Default without debuginfo processing:
> > # perf record -a
> >
> > Using system debuginfod setup:
> > # perf record -a --debuginfod
> >
> > Using custom debuginfd url:
> > # perf record -a --debuginfod='https://evenbetterdebuginfodserver.krava'
> >
> > Adding single perf_debuginfod_setup function and using
> > it also in perf buildid-cache command.
>
> I'm still running with --no-buildid --no-buildid-cache or something like
> that by default. As long as that remains working I'm good.

you're good ;-) that will bypass the problem completely

jirka

2021-12-10 13:33:55

by Arnaldo Carvalho de Melo

[permalink] [raw]

Subject: Re: [RFC] perf record: Disable debuginfod by default

Em Fri, Dec 10, 2021 at 01:24:40PM +0100, Jiri Olsa escreveu:
> On Fri, Dec 10, 2021 at 09:04:25AM +0100, Peter Zijlstra wrote:
> > On Thu, Dec 09, 2021 at 09:04:25PM +0100, Jiri Olsa wrote:
> > > Adding single perf_debuginfod_setup function and using
> > > it also in perf buildid-cache command.

> > I'm still running with --no-buildid --no-buildid-cache or something like
> > that by default. As long as that remains working I'm good.

> you're good ;-) that will bypass the problem completely

And these days buildids come in PERF_RECORD_MMAP records when possible
(kernel new enough), so no extra step at the end for traversing
PERF_RECORD_MMAP records, read the DSO, find the build id, etc:

$ git log --pretty=fuller -1 --author=jolsa kernel/events/
commit 88a16a1309333e43d328621ece3e9fa37027e8eb
Author: Jiri Olsa <[email protected]>
AuthorDate: Thu Jan 14 14:40:44 2021 +0100
Commit: Alexei Starovoitov <[email protected]>
CommitDate: Thu Jan 14 19:29:58 2021 -0800

perf: Add build id data in mmap2 event

Adding support to carry build id data in mmap2 event.

The build id data replaces maj/min/ino/ino_generation
fields, which are also used to identify map's binary,
so it's ok to replace them with build id data:

union {
struct {
u32 maj;
u32 min;
u64 ino;
u64 ino_generation;
};
struct {
u8 build_id_size;
u8 __reserved_1;
u16 __reserved_2;
u8 build_id[20];
};
};

Replaced maj/min/ino/ino_generation fields give us size
of 24 bytes. We use 20 bytes for build id data, 1 byte
for size and rest is unused.

There's new misc bit for mmap2 to signal there's build
id data in it:

#define PERF_RECORD_MISC_MMAP_BUILD_ID (1 << 14)

Signed-off-by: Jiri Olsa <[email protected]>
Signed-off-by: Alexei Starovoitov <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lore.kernel.org/bpf/[email protected]
$

- Arnaldo

2021-12-10 16:50:22

by Frank Ch. Eigler

[permalink] [raw]

Subject: Re: [RFC] perf record: Disable debuginfod by default

Hi -

On Fri, Dec 10, 2021 at 01:23:32PM +0100, Jiri Olsa wrote:
> [...]
> at the end of the perf record we populate buildid cache
> with profiled binaries for the current perf.data
>
> **IF** there's DEBUGINFOD_URLS defined, that code will
> also ask debuginfod for binaries it could not find on
> the system

Consider doing this only at the end of the run, and in the background,
just as a prefetch for the perf report step? The main downside there
could be if one runs many perf record jobs in close proximity,
overlapping larger prefetch download tasks. That might waste some
network traffic.

- FChE

2021-12-10 18:42:04

by Namhyung Kim

[permalink] [raw]

Subject: Re: [RFC] perf record: Disable debuginfod by default

On Fri, Dec 10, 2021 at 4:23 AM Jiri Olsa <[email protected]> wrote:
>
> On Thu, Dec 09, 2021 at 03:39:20PM -0800, Namhyung Kim wrote:
> > Hi Jiri,
> >
> > On Thu, Dec 9, 2021 at 12:04 PM Jiri Olsa <[email protected]> wrote:
> > >
> > > hi,
> > > after migrating to fedora 35 I found perf record hanging on exit
> > > and it's because fedora 35 sets DEBUGINFOD_URLS that triggers
> > > debuginfod query which might take long time to process.
> > >
> > > I discussed this briefly with Frank and I'm sending the change
> > > to disable debuginfod by default in perf record.
> > >
> > > Frank had other idea we could discuss here to fork or just spawn
> > > "/usr/bin/debuginfod-find ...." into background after perf record.
> > >
> > > Perhaps there are other ways as well, hence this is RFC ;-)
> >
> > I thought the debuginfod was for perf report, not record.
> > Maybe I'm missing something but how about moving it to
> > report? We can talk to debuginfod after checking the local
> > build-id cache and binary on the system.
>
> at the end of the perf record we populate buildid cache
> with profiled binaries for the current perf.data
>
> **IF** there's DEBUGINFOD_URLS defined, that code will
> also ask debuginfod for binaries it could not find on
> the system

Yeah, I know what you're doing. But I guess debuginfod
contains binaries for the distro and they'd be available for
a while. Then I think we don't need to do it at perf record.

Thanks,
Namhyung

2021-12-11 19:57:10

by Jiri Olsa

[permalink] [raw]

Subject: Re: [RFC] perf record: Disable debuginfod by default

On Fri, Dec 10, 2021 at 10:41:49AM -0800, Namhyung Kim wrote:
> On Fri, Dec 10, 2021 at 4:23 AM Jiri Olsa <[email protected]> wrote:
> >
> > On Thu, Dec 09, 2021 at 03:39:20PM -0800, Namhyung Kim wrote:
> > > Hi Jiri,
> > >
> > > On Thu, Dec 9, 2021 at 12:04 PM Jiri Olsa <[email protected]> wrote:
> > > >
> > > > hi,
> > > > after migrating to fedora 35 I found perf record hanging on exit
> > > > and it's because fedora 35 sets DEBUGINFOD_URLS that triggers
> > > > debuginfod query which might take long time to process.
> > > >
> > > > I discussed this briefly with Frank and I'm sending the change
> > > > to disable debuginfod by default in perf record.
> > > >
> > > > Frank had other idea we could discuss here to fork or just spawn
> > > > "/usr/bin/debuginfod-find ...." into background after perf record.
> > > >
> > > > Perhaps there are other ways as well, hence this is RFC ;-)
> > >
> > > I thought the debuginfod was for perf report, not record.
> > > Maybe I'm missing something but how about moving it to
> > > report? We can talk to debuginfod after checking the local
> > > build-id cache and binary on the system.
> >
> > at the end of the perf record we populate buildid cache
> > with profiled binaries for the current perf.data
> >
> > **IF** there's DEBUGINFOD_URLS defined, that code will
> > also ask debuginfod for binaries it could not find on
> > the system
>
> Yeah, I know what you're doing. But I guess debuginfod
> contains binaries for the distro and they'd be available for
> a while. Then I think we don't need to do it at perf record.

well there's also profiling of non system binaries, which you want
to cache, because you might want to check that profile later

so I think we should still do that in perf record, just to have
some control over debuginfod, which is executed as part of
build_id_cache__find_debug function

jirka

2021-12-19 13:04:53

by Jiri Olsa

[permalink] [raw]

Subject: Re: [RFC] perf record: Disable debuginfod by default

On Fri, Dec 10, 2021 at 11:50:10AM -0500, Frank Ch. Eigler wrote:
> Hi -
>
> On Fri, Dec 10, 2021 at 01:23:32PM +0100, Jiri Olsa wrote:
> > [...]
> > at the end of the perf record we populate buildid cache
> > with profiled binaries for the current perf.data
> >
> > **IF** there's DEBUGINFOD_URLS defined, that code will
> > also ask debuginfod for binaries it could not find on
> > the system
>
> Consider doing this only at the end of the run, and in the background,
> just as a prefetch for the perf report step? The main downside there
> could be if one runs many perf record jobs in close proximity,
> overlapping larger prefetch download tasks. That might waste some
> network traffic.

right, IMO it's too much for default behaviour but I think could
do this under new (config) option, if there's a need for that

thanks,
jirka

2021-12-19 13:07:05

by Jiri Olsa

[permalink] [raw]

Subject: Re: [RFC] perf record: Disable debuginfod by default

On Thu, Dec 09, 2021 at 09:04:25PM +0100, Jiri Olsa wrote:
> hi,
> after migrating to fedora 35 I found perf record hanging on exit
> and it's because fedora 35 sets DEBUGINFOD_URLS that triggers
> debuginfod query which might take long time to process.
>
> I discussed this briefly with Frank and I'm sending the change
> to disable debuginfod by default in perf record.
>
> Frank had other idea we could discuss here to fork or just spawn
> "/usr/bin/debuginfod-find ...." into background after perf record.
>
> Perhaps there are other ways as well, hence this is RFC ;-)
>
> thanks,
> jirka

Arnaldo,
looks like there are no more comments, do you need me to repost
it as normal patch? there's no change after rebasing on top of
your perf/core branch

thanks,
jirka

>
>
> ---
> Fedora 35 sets by default DEBUGINFOD_URLS, which might lead
> to unexpected stalls in perf record exit path, when we try
> to cache profiled binaries.
>
> # DEBUGINFOD_PROGRESS=1 ./perf record -a
> ^C[ perf record: Woken up 1 times to write data ]
> Downloading from https://debuginfod.fedoraproject.org/ 447069
> Downloading from https://debuginfod.fedoraproject.org/ 1502175
> Downloading \^Z
>
> Disabling DEBUGINFOD_URLS by default in perf record and adding
> debuginfod option and .perfconfig variable support to enable id.
>
> Default without debuginfo processing:
> # perf record -a
>
> Using system debuginfod setup:
> # perf record -a --debuginfod
>
> Using custom debuginfd url:
> # perf record -a --debuginfod='https://evenbetterdebuginfodserver.krava'
>
> Adding single perf_debuginfod_setup function and using
> it also in perf buildid-cache command.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> .../perf/Documentation/perf-buildid-cache.txt | 5 +++-
> tools/perf/Documentation/perf-config.txt | 9 +++++++
> tools/perf/Documentation/perf-record.txt | 9 +++++++
> tools/perf/builtin-buildid-cache.c | 25 +++++++++++--------
> tools/perf/builtin-record.c | 13 ++++++++++
> tools/perf/util/util.c | 15 +++++++++++
> tools/perf/util/util.h | 6 +++++
> 7 files changed, 70 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-buildid-cache.txt b/tools/perf/Documentation/perf-buildid-cache.txt
> index cd8ce6e8ec12..7e44b419d301 100644
> --- a/tools/perf/Documentation/perf-buildid-cache.txt
> +++ b/tools/perf/Documentation/perf-buildid-cache.txt
> @@ -74,12 +74,15 @@ OPTIONS
> used when creating a uprobe for a process that resides in a
> different mount namespace from the perf(1) utility.
>
> ---debuginfod=URLs::
> +--debuginfod[=URLs]::
> Specify debuginfod URL to be used when retrieving perf.data binaries,
> it follows the same syntax as the DEBUGINFOD_URLS variable, like:
>
> buildid-cache.debuginfod=http://192.168.122.174:8002
>
> + If the URLs is not specified, the value of DEBUGINFOD_URLS
> + system environment variable is used.
> +
> SEE ALSO
> --------
> linkperf:perf-record[1], linkperf:perf-report[1], linkperf:perf-buildid-list[1]
> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> index 3bb75c1f25e8..0420e71698ee 100644
> --- a/tools/perf/Documentation/perf-config.txt
> +++ b/tools/perf/Documentation/perf-config.txt
> @@ -587,6 +587,15 @@ record.*::
> Use 'n' control blocks in asynchronous (Posix AIO) trace writing
> mode ('n' default: 1, max: 4).
>
> + record.debuginfod::
> + Specify debuginfod URL to be used when cacheing perf.data binaries,
> + it follows the same syntax as the DEBUGINFOD_URLS variable, like:
> +
> + http://192.168.122.174:8002
> +
> + If the URLs is 'system', the value of DEBUGINFOD_URLS system environment
> + variable is used.
> +
> diff.*::
> diff.order::
> This option sets the number of columns to sort the result.
> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> index 55df7b073a55..9ccc75935bc5 100644
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -715,6 +715,15 @@ measurements:
>
> include::intel-hybrid.txt[]
>
> +--debuginfod[=URLs]::
> + Specify debuginfod URL to be used when cacheing perf.data binaries,
> + it follows the same syntax as the DEBUGINFOD_URLS variable, like:
> +
> + http://192.168.122.174:8002
> +
> + If the URLs is not specified, the value of DEBUGINFOD_URLS
> + system environment variable is used.
> +
> SEE ALSO
> --------
> linkperf:perf-stat[1], linkperf:perf-list[1], linkperf:perf-intel-pt[1]
> diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
> index 0db3cfc04c47..cd381693658b 100644
> --- a/tools/perf/builtin-buildid-cache.c
> +++ b/tools/perf/builtin-buildid-cache.c
> @@ -351,10 +351,14 @@ static int build_id_cache__show_all(void)
>
> static int perf_buildid_cache_config(const char *var, const char *value, void *cb)
> {
> - const char **debuginfod = cb;
> + struct perf_debuginfod *di = cb;
>
> - if (!strcmp(var, "buildid-cache.debuginfod"))
> - *debuginfod = strdup(value);
> + if (!strcmp(var, "buildid-cache.debuginfod")) {
> + di->urls = strdup(value);
> + if (!di->urls)
> + return -ENOMEM;
> + di->set = true;
> + }
>
> return 0;
> }
> @@ -373,8 +377,8 @@ int cmd_buildid_cache(int argc, const char **argv)
> *purge_name_list_str = NULL,
> *missing_filename = NULL,
> *update_name_list_str = NULL,
> - *kcore_filename = NULL,
> - *debuginfod = NULL;
> + *kcore_filename = NULL;
> + struct perf_debuginfod debuginfod = { };
> char sbuf[STRERR_BUFSIZE];
>
> struct perf_data data = {
> @@ -399,8 +403,10 @@ int cmd_buildid_cache(int argc, const char **argv)
> OPT_BOOLEAN('f', "force", &force, "don't complain, do it"),
> OPT_STRING('u', "update", &update_name_list_str, "file list",
> "file(s) to update"),
> - OPT_STRING(0, "debuginfod", &debuginfod, "debuginfod url",
> - "set debuginfod url"),
> + OPT_STRING_OPTARG_SET(0, "debuginfod", &debuginfod.urls,
> + &debuginfod.set, "debuginfod urls",
> + "Enable debuginfod data retrieval from DEBUGINFOD_URLS or specified urls",
> + "system"),
> OPT_INCR('v', "verbose", &verbose, "be more verbose"),
> OPT_INTEGER(0, "target-ns", &ns_id, "target pid for namespace context"),
> OPT_END()
> @@ -425,10 +431,7 @@ int cmd_buildid_cache(int argc, const char **argv)
> if (argc || !(list_files || opts_flag))
> usage_with_options(buildid_cache_usage, buildid_cache_options);
>
> - if (debuginfod) {
> - pr_debug("DEBUGINFOD_URLS=%s\n", debuginfod);
> - setenv("DEBUGINFOD_URLS", debuginfod, 1);
> - }
> + perf_debuginfod_setup(&debuginfod);
>
> /* -l is exclusive. It can not be used with other options. */
> if (list_files && opts_flag) {
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 0338b813585a..2da2a6acbb8c 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -111,6 +111,7 @@ struct record {
> unsigned long long samples;
> struct mmap_cpu_mask affinity_mask;
> unsigned long output_max_size; /* = 0: unlimited */
> + struct perf_debuginfod debuginfod;
> };
>
> static volatile int done;
> @@ -2177,6 +2178,12 @@ static int perf_record_config(const char *var, const char *value, void *cb)
> rec->opts.nr_cblocks = nr_cblocks_default;
> }
> #endif
> + if (!strcmp(var, "record.debuginfod")) {
> + rec->debuginfod.urls = strdup(value);
> + if (!rec->debuginfod.urls)
> + return -ENOMEM;
> + rec->debuginfod.set = true;
> + }
>
> return 0;
> }
> @@ -2663,6 +2670,10 @@ static struct option __record_options[] = {
> parse_control_option),
> OPT_CALLBACK(0, "synth", &record.opts, "no|all|task|mmap|cgroup",
> "Fine-tune event synthesis: default=all", parse_record_synth_option),
> + OPT_STRING_OPTARG_SET(0, "debuginfod", &record.debuginfod.urls,
> + &record.debuginfod.set, "debuginfod urls",
> + "Enable debuginfod data retrieval from DEBUGINFOD_URLS or specified urls",
> + "system"),
> OPT_END()
> };
>
> @@ -2716,6 +2727,8 @@ int cmd_record(int argc, const char **argv)
> if (err)
> return err;
>
> + perf_debuginfod_setup(&record.debuginfod);
> +
> /* Make system wide (-a) the default target. */
> if (!argc && target__none(&rec->opts.target))
> rec->opts.target.system_wide = true;
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index df3c4671be72..fb4f6616b5fa 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -416,3 +416,18 @@ char *perf_exe(char *buf, int len)
> }
> return strcpy(buf, "perf");
> }
> +
> +void perf_debuginfod_setup(struct perf_debuginfod *di)
> +{
> + /*
> + * By default '!di->set' we clear DEBUGINFOD_URLS, so debuginfod
> + * processing is not triggered, otherwise we set it to 'di->urls'
> + * value. If 'di->urls' is "system" we keep DEBUGINFOD_URLS value.
> + */
> + if (!di->set)
> + setenv("DEBUGINFOD_URLS", "", 1);
> + else if (di->urls && strcmp(di->urls, "system"))
> + setenv("DEBUGINFOD_URLS", di->urls, 1);
> +
> + pr_debug("DEBUGINFOD_URLS=%s\n", getenv("DEBUGINFOD_URLS"));
> +}
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 9f0d36ba77f2..4359f5af2de7 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -68,4 +68,10 @@ void test_attr__init(void);
> struct perf_event_attr;
> void test_attr__open(struct perf_event_attr *attr, pid_t pid, int cpu,
> int fd, int group_fd, unsigned long flags);
> +
> +struct perf_debuginfod {
> + const char *urls;
> + bool set;
> +};
> +void perf_debuginfod_setup(struct perf_debuginfod *di);
> #endif /* GIT_COMPAT_UTIL_H */
> --
> 2.33.1
>

2022-01-16 16:22:55

by Arnaldo Carvalho de Melo

[permalink] [raw]

Subject: Re: [RFC] perf record: Disable debuginfod by default

Em Sun, Dec 19, 2021 at 02:06:52PM +0100, Jiri Olsa escreveu:
> On Thu, Dec 09, 2021 at 09:04:25PM +0100, Jiri Olsa wrote:
> > hi,
> > after migrating to fedora 35 I found perf record hanging on exit
> > and it's because fedora 35 sets DEBUGINFOD_URLS that triggers
> > debuginfod query which might take long time to process.
> >
> > I discussed this briefly with Frank and I'm sending the change
> > to disable debuginfod by default in perf record.
> >
> > Frank had other idea we could discuss here to fork or just spawn
> > "/usr/bin/debuginfod-find ...." into background after perf record.
> >
> > Perhaps there are other ways as well, hence this is RFC ;-)
> >
> > thanks,
> > jirka
>
> Arnaldo,
> looks like there are no more comments, do you need me to repost
> it as normal patch? there's no change after rebasing on top of
> your perf/core branch

Applying now, after fixing up minor fuzzes.

> thanks,
> jirka
>
> >
> >
> > ---
> > Fedora 35 sets by default DEBUGINFOD_URLS, which might lead
> > to unexpected stalls in perf record exit path, when we try
> > to cache profiled binaries.
> >
> > # DEBUGINFOD_PROGRESS=1 ./perf record -a
> > ^C[ perf record: Woken up 1 times to write data ]
> > Downloading from https://debuginfod.fedoraproject.org/ 447069
> > Downloading from https://debuginfod.fedoraproject.org/ 1502175
> > Downloading \^Z
> >
> > Disabling DEBUGINFOD_URLS by default in perf record and adding
> > debuginfod option and .perfconfig variable support to enable id.
> >
> > Default without debuginfo processing:
> > # perf record -a
> >
> > Using system debuginfod setup:
> > # perf record -a --debuginfod
> >
> > Using custom debuginfd url:
> > # perf record -a --debuginfod='https://evenbetterdebuginfodserver.krava'
> >
> > Adding single perf_debuginfod_setup function and using
> > it also in perf buildid-cache command.
> >
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
> > .../perf/Documentation/perf-buildid-cache.txt | 5 +++-
> > tools/perf/Documentation/perf-config.txt | 9 +++++++
> > tools/perf/Documentation/perf-record.txt | 9 +++++++
> > tools/perf/builtin-buildid-cache.c | 25 +++++++++++--------
> > tools/perf/builtin-record.c | 13 ++++++++++
> > tools/perf/util/util.c | 15 +++++++++++
> > tools/perf/util/util.h | 6 +++++
> > 7 files changed, 70 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/perf/Documentation/perf-buildid-cache.txt b/tools/perf/Documentation/perf-buildid-cache.txt
> > index cd8ce6e8ec12..7e44b419d301 100644
> > --- a/tools/perf/Documentation/perf-buildid-cache.txt
> > +++ b/tools/perf/Documentation/perf-buildid-cache.txt
> > @@ -74,12 +74,15 @@ OPTIONS
> > used when creating a uprobe for a process that resides in a
> > different mount namespace from the perf(1) utility.
> >
> > ---debuginfod=URLs::
> > +--debuginfod[=URLs]::
> > Specify debuginfod URL to be used when retrieving perf.data binaries,
> > it follows the same syntax as the DEBUGINFOD_URLS variable, like:
> >
> > buildid-cache.debuginfod=http://192.168.122.174:8002
> >
> > + If the URLs is not specified, the value of DEBUGINFOD_URLS
> > + system environment variable is used.
> > +
> > SEE ALSO
> > --------
> > linkperf:perf-record[1], linkperf:perf-report[1], linkperf:perf-buildid-list[1]
> > diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> > index 3bb75c1f25e8..0420e71698ee 100644
> > --- a/tools/perf/Documentation/perf-config.txt
> > +++ b/tools/perf/Documentation/perf-config.txt
> > @@ -587,6 +587,15 @@ record.*::
> > Use 'n' control blocks in asynchronous (Posix AIO) trace writing
> > mode ('n' default: 1, max: 4).
> >
> > + record.debuginfod::
> > + Specify debuginfod URL to be used when cacheing perf.data binaries,
> > + it follows the same syntax as the DEBUGINFOD_URLS variable, like:
> > +
> > + http://192.168.122.174:8002
> > +
> > + If the URLs is 'system', the value of DEBUGINFOD_URLS system environment
> > + variable is used.
> > +
> > diff.*::
> > diff.order::
> > This option sets the number of columns to sort the result.
> > diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> > index 55df7b073a55..9ccc75935bc5 100644
> > --- a/tools/perf/Documentation/perf-record.txt
> > +++ b/tools/perf/Documentation/perf-record.txt
> > @@ -715,6 +715,15 @@ measurements:
> >
> > include::intel-hybrid.txt[]
> >
> > +--debuginfod[=URLs]::
> > + Specify debuginfod URL to be used when cacheing perf.data binaries,
> > + it follows the same syntax as the DEBUGINFOD_URLS variable, like:
> > +
> > + http://192.168.122.174:8002
> > +
> > + If the URLs is not specified, the value of DEBUGINFOD_URLS
> > + system environment variable is used.
> > +
> > SEE ALSO
> > --------
> > linkperf:perf-stat[1], linkperf:perf-list[1], linkperf:perf-intel-pt[1]
> > diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
> > index 0db3cfc04c47..cd381693658b 100644
> > --- a/tools/perf/builtin-buildid-cache.c
> > +++ b/tools/perf/builtin-buildid-cache.c
> > @@ -351,10 +351,14 @@ static int build_id_cache__show_all(void)
> >
> > static int perf_buildid_cache_config(const char *var, const char *value, void *cb)
> > {
> > - const char **debuginfod = cb;
> > + struct perf_debuginfod *di = cb;
> >
> > - if (!strcmp(var, "buildid-cache.debuginfod"))
> > - *debuginfod = strdup(value);
> > + if (!strcmp(var, "buildid-cache.debuginfod")) {
> > + di->urls = strdup(value);
> > + if (!di->urls)
> > + return -ENOMEM;
> > + di->set = true;
> > + }
> >
> > return 0;
> > }
> > @@ -373,8 +377,8 @@ int cmd_buildid_cache(int argc, const char **argv)
> > *purge_name_list_str = NULL,
> > *missing_filename = NULL,
> > *update_name_list_str = NULL,
> > - *kcore_filename = NULL,
> > - *debuginfod = NULL;
> > + *kcore_filename = NULL;
> > + struct perf_debuginfod debuginfod = { };
> > char sbuf[STRERR_BUFSIZE];
> >
> > struct perf_data data = {
> > @@ -399,8 +403,10 @@ int cmd_buildid_cache(int argc, const char **argv)
> > OPT_BOOLEAN('f', "force", &force, "don't complain, do it"),
> > OPT_STRING('u', "update", &update_name_list_str, "file list",
> > "file(s) to update"),
> > - OPT_STRING(0, "debuginfod", &debuginfod, "debuginfod url",
> > - "set debuginfod url"),
> > + OPT_STRING_OPTARG_SET(0, "debuginfod", &debuginfod.urls,
> > + &debuginfod.set, "debuginfod urls",
> > + "Enable debuginfod data retrieval from DEBUGINFOD_URLS or specified urls",
> > + "system"),
> > OPT_INCR('v', "verbose", &verbose, "be more verbose"),
> > OPT_INTEGER(0, "target-ns", &ns_id, "target pid for namespace context"),
> > OPT_END()
> > @@ -425,10 +431,7 @@ int cmd_buildid_cache(int argc, const char **argv)
> > if (argc || !(list_files || opts_flag))
> > usage_with_options(buildid_cache_usage, buildid_cache_options);
> >
> > - if (debuginfod) {
> > - pr_debug("DEBUGINFOD_URLS=%s\n", debuginfod);
> > - setenv("DEBUGINFOD_URLS", debuginfod, 1);
> > - }
> > + perf_debuginfod_setup(&debuginfod);
> >
> > /* -l is exclusive. It can not be used with other options. */
> > if (list_files && opts_flag) {
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index 0338b813585a..2da2a6acbb8c 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -111,6 +111,7 @@ struct record {
> > unsigned long long samples;
> > struct mmap_cpu_mask affinity_mask;
> > unsigned long output_max_size; /* = 0: unlimited */
> > + struct perf_debuginfod debuginfod;
> > };
> >
> > static volatile int done;
> > @@ -2177,6 +2178,12 @@ static int perf_record_config(const char *var, const char *value, void *cb)
> > rec->opts.nr_cblocks = nr_cblocks_default;
> > }
> > #endif
> > + if (!strcmp(var, "record.debuginfod")) {
> > + rec->debuginfod.urls = strdup(value);
> > + if (!rec->debuginfod.urls)
> > + return -ENOMEM;
> > + rec->debuginfod.set = true;
> > + }
> >
> > return 0;
> > }
> > @@ -2663,6 +2670,10 @@ static struct option __record_options[] = {
> > parse_control_option),
> > OPT_CALLBACK(0, "synth", &record.opts, "no|all|task|mmap|cgroup",
> > "Fine-tune event synthesis: default=all", parse_record_synth_option),
> > + OPT_STRING_OPTARG_SET(0, "debuginfod", &record.debuginfod.urls,
> > + &record.debuginfod.set, "debuginfod urls",
> > + "Enable debuginfod data retrieval from DEBUGINFOD_URLS or specified urls",
> > + "system"),
> > OPT_END()
> > };
> >
> > @@ -2716,6 +2727,8 @@ int cmd_record(int argc, const char **argv)
> > if (err)
> > return err;
> >
> > + perf_debuginfod_setup(&record.debuginfod);
> > +
> > /* Make system wide (-a) the default target. */
> > if (!argc && target__none(&rec->opts.target))
> > rec->opts.target.system_wide = true;
> > diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> > index df3c4671be72..fb4f6616b5fa 100644
> > --- a/tools/perf/util/util.c
> > +++ b/tools/perf/util/util.c
> > @@ -416,3 +416,18 @@ char *perf_exe(char *buf, int len)
> > }
> > return strcpy(buf, "perf");
> > }
> > +
> > +void perf_debuginfod_setup(struct perf_debuginfod *di)
> > +{
> > + /*
> > + * By default '!di->set' we clear DEBUGINFOD_URLS, so debuginfod
> > + * processing is not triggered, otherwise we set it to 'di->urls'
> > + * value. If 'di->urls' is "system" we keep DEBUGINFOD_URLS value.
> > + */
> > + if (!di->set)
> > + setenv("DEBUGINFOD_URLS", "", 1);
> > + else if (di->urls && strcmp(di->urls, "system"))
> > + setenv("DEBUGINFOD_URLS", di->urls, 1);
> > +
> > + pr_debug("DEBUGINFOD_URLS=%s\n", getenv("DEBUGINFOD_URLS"));
> > +}
> > diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> > index 9f0d36ba77f2..4359f5af2de7 100644
> > --- a/tools/perf/util/util.h
> > +++ b/tools/perf/util/util.h
> > @@ -68,4 +68,10 @@ void test_attr__init(void);
> > struct perf_event_attr;
> > void test_attr__open(struct perf_event_attr *attr, pid_t pid, int cpu,
> > int fd, int group_fd, unsigned long flags);
> > +
> > +struct perf_debuginfod {
> > + const char *urls;
> > + bool set;
> > +};
> > +void perf_debuginfod_setup(struct perf_debuginfod *di);
> > #endif /* GIT_COMPAT_UTIL_H */
> > --
> > 2.33.1
> >

--

- Arnaldo