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

From: Arnaldo Carvalho de Melo
Date: Sat Jan 15 2022 - 15:22:20 EST

Next message: Sergey Shtylyov: "Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional"
Previous message: Fiona Hill: "[no subject]"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <jolsa@xxxxxxxxxx>
> > ---
> > .../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

Next message: Sergey Shtylyov: "Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional"
Previous message: Fiona Hill: "[no subject]"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]