| 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: <aCvGF547Y3eteLRK@google.com>
Date: Mon, 19 May 2025 17:00:23 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Thomas Richter <tmricht@...ux.ibm.com>
Cc: linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
linux-perf-users@...r.kernel.org, acme@...nel.org,
agordeev@...ux.ibm.com, gor@...ux.ibm.com, sumanthk@...ux.ibm.com,
hca@...ux.ibm.com, Alexander Egorenkov <egorenar@...ux.ibm.com>
Subject: Re: [PATCH v2] perf ftrace: Use process/session specific trace
settings
Hello,
On Mon, May 19, 2025 at 04:52:35PM +0200, Thomas Richter wrote:
> V1 --> V2: Add Suggestion from Arnaldo Cavalho de Melo confirmed by
> Steven Rostedt. Use rmdir(../tracing/instances/dir) to stop
> process/session specific tracing and delete all
> process/session specific setings.
>
> Executing perf ftrace commands ftrace, profile and latency
> leave tracing disabled as can seen in this output:
>
> # echo 1 > /sys/kernel/debug/tracing/tracing_on
> # cat /sys/kernel/debug/tracing/tracing_on
> 1
> # perf ftrace trace --graph-opts depth=5 sleep 0.1 > /dev/null
> # cat /sys/kernel/debug/tracing/tracing_on
> 0
> #
>
> The tracing_on file is not restored to its value before the command.
> Fix this behavior and restore the trace setting to what
> is was before the invocation of the command.
> On Fedora 41 and 42 tracing is turned on by default.
>
> This patch use the .../tracing/instances/XXX subdirectory feature.
> Each perf ftrace invocation creates its own session/process
> specific subdirectory and does not change the global state
> in the .../tracing directory itself.
>
> Signed-off-by: Thomas Richter <tmricht@...ux.ibm.com>
> Suggested-by: Arnaldo Carvalho de Melo <acme@...nel.org>
> Reported-by: Alexander Egorenkov <egorenar@...ux.ibm.com>
> ---
> tools/perf/builtin-ftrace.c | 105 +++++++++++++++++++++++++++++++-----
> 1 file changed, 91 insertions(+), 14 deletions(-)
>
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 7caa18d5ffc3..3faf96e7185e 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -38,6 +38,8 @@
> #include "util/units.h"
> #include "util/parse-sublevel-options.h"
>
> +#include <sys/stat.h>
The standard header files are placed in the above. Let's put it there.
> +
> #define DEFAULT_TRACER "function_graph"
>
> static volatile sig_atomic_t workload_exec_errno;
> @@ -45,6 +47,8 @@ static volatile sig_atomic_t done;
>
> static struct stats latency_stats; /* for tracepoints */
>
> +static char tracing_instance[PATH_MAX]; /* Trace instance directory */
> +
> static void sig_handler(int sig __maybe_unused)
> {
> done = true;
> @@ -100,6 +104,34 @@ static bool is_ftrace_supported(void)
> return supported;
> }
>
> +/*
> + * Wrapper to test if a file in directory .../tracing/instances/XXX
> + * exists. If so return the .../tracing/instances/XXX file for use.
> + * Otherwise the file exists only in directory .../tracing and
> + * is applicable to all instances, for example file available_filter_functions.
> + * Return that file name in this case.
Not sure if it's needed. Can we call get_tracing_file() directly for
the global files?
> + *
> + * This functions works similar to get_tracing_file() and expects its caller
> + * to free the returned file name.
> + *
> + * The global variable tracing_instance is set in init_tracing_instance()
> + * called a the beginning to a process specific tracing subdirectory.
s/called a /called at / ?
> + */
> +static char *get_tracing_instance_file(const char *name)
> +{
> + char *file;
> +
> + if (asprintf(&file, "%s/%s", tracing_instance, name) < 0)
> + return NULL;
> +
> + if (!access(file, F_OK))
> + return file;
> +
> + put_tracing_file(file);
This didn't use the get_tracing_file() API. Just call free().
> + file = get_tracing_file(name);
> + return file;
> +}
> +
> static int __write_tracing_file(const char *name, const char *val, bool append)
> {
> char *file;
> @@ -109,7 +141,7 @@ static int __write_tracing_file(const char *name, const char *val, bool append)
> char errbuf[512];
> char *val_copy;
>
> - file = get_tracing_file(name);
> + file = get_tracing_instance_file(name);
> if (!file) {
> pr_debug("cannot get tracing file: %s\n", name);
> return -1;
> @@ -167,7 +199,7 @@ static int read_tracing_file_to_stdout(const char *name)
> int fd;
> int ret = -1;
>
> - file = get_tracing_file(name);
> + file = get_tracing_instance_file(name);
> if (!file) {
> pr_debug("cannot get tracing file: %s\n", name);
> return -1;
> @@ -209,7 +241,7 @@ static int read_tracing_file_by_line(const char *name,
> char *file;
> FILE *fp;
>
> - file = get_tracing_file(name);
> + file = get_tracing_instance_file(name);
> if (!file) {
> pr_debug("cannot get tracing file: %s\n", name);
> return -1;
> @@ -299,6 +331,36 @@ static int reset_tracing_files(struct perf_ftrace *ftrace __maybe_unused)
> return 0;
> }
>
> +/* Remove .../tracing/instances/XXX subdirectory created with
> + * init_tracing_instance().
> + */
> +static void exit_tracing_instance(void)
> +{
> + rmdir(tracing_instance);
Can we check the return value and print a message if failed? I think
it'd succeed mostly but let's prepare for errors.
> +}
> +
> +/* Create subdirectory within .../tracing/instances/XXX to have session
> + * or process specific setup. To delete this setup, simply remove the
> + * subdirectory.
> + */
> +static int init_tracing_instance(void)
> +{
> + char dirname[] = "instances/perf-ftrace-XXXXXX";
> + char *path;
> +
> + path = get_tracing_file(dirname);
> + if (!path)
> + return -1;
> + strcpy(tracing_instance, path);
strncpy() instead?
> + put_tracing_file(path);
> + path = mkdtemp(tracing_instance);
> + if (!path) {
> + pr_err("failed to create tracing/instances directory\n");
> + return -1;
> + }
> + return 0;
> +}
> +
> static int set_tracing_pid(struct perf_ftrace *ftrace)
> {
> int i;
> @@ -629,14 +691,19 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>
> select_tracer(ftrace);
>
> + if (init_tracing_instance() < 0) {
> + pr_err("failed to create tracing/instances\n");
Duplicated error message.
> + goto out;
> + }
> +
> if (reset_tracing_files(ftrace) < 0) {
> pr_err("failed to reset ftrace\n");
> - goto out;
> + goto out_reset;
> }
>
> /* reset ftrace buffer */
> if (write_tracing_file("trace", "0") < 0)
> - goto out;
> + goto out_reset;
>
> if (set_tracing_options(ftrace) < 0)
> goto out_reset;
> @@ -648,7 +715,7 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>
> setup_pager();
>
> - trace_file = get_tracing_file("trace_pipe");
> + trace_file = get_tracing_instance_file("trace_pipe");
> if (!trace_file) {
> pr_err("failed to open trace_pipe\n");
> goto out_reset;
> @@ -723,7 +790,7 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
> out_close_fd:
> close(trace_fd);
> out_reset:
> - reset_tracing_files(ftrace);
> + exit_tracing_instance();
> out:
> return (done && !workload_exec_errno) ? 0 : -1;
> }
> @@ -924,6 +991,11 @@ static int prepare_func_latency(struct perf_ftrace *ftrace)
> if (ftrace->target.use_bpf)
> return perf_ftrace__latency_prepare_bpf(ftrace);
>
> + if (init_tracing_instance() < 0) {
> + pr_err("failed to create tracing/instances\n");
Ditto.
> + return -1;
> + }
> +
> if (reset_tracing_files(ftrace) < 0) {
> pr_err("failed to reset ftrace\n");
> return -1;
> @@ -942,7 +1014,7 @@ static int prepare_func_latency(struct perf_ftrace *ftrace)
> return -1;
> }
>
> - trace_file = get_tracing_file("trace_pipe");
> + trace_file = get_tracing_instance_file("trace_pipe");
> if (!trace_file) {
> pr_err("failed to open trace_pipe\n");
> return -1;
> @@ -993,7 +1065,7 @@ static int cleanup_func_latency(struct perf_ftrace *ftrace)
> if (ftrace->target.use_bpf)
> return perf_ftrace__latency_cleanup_bpf(ftrace);
>
> - reset_tracing_files(ftrace);
> + exit_tracing_instance();
> return 0;
> }
>
> @@ -1304,17 +1376,22 @@ static int __cmd_profile(struct perf_ftrace *ftrace)
> goto out;
> }
>
> + if (init_tracing_instance() < 0) {
> + pr_err("failed to create tracing/instances\n");
Ditto.
Thanks,
Namhyung
> + goto out;
> + }
> +
> if (reset_tracing_files(ftrace) < 0) {
> pr_err("failed to reset ftrace\n");
> - goto out;
> + goto out_reset;
> }
>
> /* reset ftrace buffer */
> if (write_tracing_file("trace", "0") < 0)
> - goto out;
> + goto out_reset;
>
> if (set_tracing_options(ftrace) < 0)
> - return -1;
> + goto out_reset;
>
> if (write_tracing_file("current_tracer", ftrace->tracer) < 0) {
> pr_err("failed to set current_tracer to %s\n", ftrace->tracer);
> @@ -1323,7 +1400,7 @@ static int __cmd_profile(struct perf_ftrace *ftrace)
>
> setup_pager();
>
> - trace_file = get_tracing_file("trace_pipe");
> + trace_file = get_tracing_instance_file("trace_pipe");
> if (!trace_file) {
> pr_err("failed to open trace_pipe\n");
> goto out_reset;
> @@ -1385,7 +1462,7 @@ static int __cmd_profile(struct perf_ftrace *ftrace)
> out_close_fd:
> close(trace_fd);
> out_reset:
> - reset_tracing_files(ftrace);
> + exit_tracing_instance();
> out:
> return (done && !workload_exec_errno) ? 0 : -1;
> }
> --
> 2.49.0
>
Powered by blists - more mailing lists