Re: [PATCH v2] perf ftrace: Use process/session specific trace settings

[Namhyung Kim <namhyung@kernel.org>]

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@linux.ibm.com>
> Suggested-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> Reported-by: Alexander Egorenkov <egorenar@linux.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
>