| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Yb8ubHK38NXDQE+O@krava>
Date: Sun, 19 Dec 2021 14:06:52 +0100
From: Jiri Olsa <jolsa@...hat.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: lkml <linux-kernel@...r.kernel.org>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Ingo Molnar <mingo@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Namhyung Kim <namhyung@...nel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Michael Petlan <mpetlan@...hat.com>,
Ian Rogers <irogers@...gle.com>,
linux-perf-users@...r.kernel.org,
"Frank Ch. Eigler" <fche@...hat.com>
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 <jolsa@...nel.org>
> ---
> .../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
>
Powered by blists - more mailing lists