Message-ID: <aCYSEMrXIwptT0d6@x1>
Date: Thu, 15 May 2025 13:10:56 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Thomas Richter <tmricht@...ux.ibm.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ian Rogers <irogers@...gle.com>
Cc: linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
	linux-perf-users@...r.kernel.org, namhyung@...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] perf ftrace: Restore to original trace settings on exit

On Thu, May 15, 2025 at 08:34:07AM +0200, Thomas Richter wrote:
> 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 looks fragile as it takes a snapshot in time of what are the files
in some particular directory to save them and then restore it at the
end.

The tool may at some point in the future go and touch other (added in
the future) files in that directory, etc.

I _think_ that instead we should move to use some "session mode" ftrace,
which I _think_ is already available for quite some time, i.e. instead
of touching the global ftrace files (which probably are there for
historical reasons), we should use, lemme find the reference...

I think the keyword to lookup is /sys/kernel/debug/tracing/instances/

Ian did a lot of work on having 'perf test' run shell tests in parallel,
so we need to think about ways of allowing for that by not touching
global state.

tldr; great idea, avoid global state.

- Arnaldo
 
> The root cause is function reset_tracing_files() which
> writes zero (0) into file tracing_on.
> 
> Read tracing files on start of the program and
> restore these values just before exit.
> 
> Signed-off-by: Thomas Richter <tmricht@...ux.ibm.com>
> Suggested-by: Sumanth Korikkar <sumanthk@...ux.ibm.com>
> Reviewed-by: Sumanth Korikkar <sumanthk@...ux.ibm.com>
> Reported-by: Alexander Egorenkov <egorenar@...ux.ibm.com>
> ---
>  tools/perf/builtin-ftrace.c | 226 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 226 insertions(+)
> 
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 7caa18d5ffc3..11ead75fe0f7 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -299,6 +299,228 @@ static int reset_tracing_files(struct perf_ftrace *ftrace __maybe_unused)
>  	return 0;
>  }
>  
> +static int read_tracing_file(const char *name, char *buf, size_t size)
> +{
> +	int ret = -1;
> +	char *file;
> +	int fd;
> +
> +	file = get_tracing_file(name);
> +	if (!file) {
> +		pr_debug("cannot get tracing file: %s\n", name);
> +		return -1;
> +	}
> +
> +	fd = open(file, O_RDONLY);
> +	if (fd < 0) {
> +		pr_debug("cannot open tracing file: %s: %s\n",
> +			 name, str_error_r(errno, buf, size));
> +		goto out;
> +	}
> +
> +	/* read contents to stdout */
> +	while (true) {
> +		int n = read(fd, buf, size);
> +
> +		if (n == 0)
> +			break;
> +		else if (n < 0)
> +			goto out_close;
> +		buf += n;
> +		size -= n;
> +	}
> +	ret = 0;
> +
> +out_close:
> +	close(fd);
> +out:
> +	put_tracing_file(file);
> +	return ret;
> +}
> +
> +static int read_tracing_option_file(const char *name, char *val, size_t size)
> +{
> +	char *file;
> +	int ret;
> +
> +	if (asprintf(&file, "options/%s", name) < 0)
> +		return -1;
> +
> +	ret = read_tracing_file(file, val, size);
> +	free(file);
> +	return ret;
> +}
> +
> +/*
> + * Save the initial trace file setting to restore them after the tests.
> + * This ensures the setting are the same as before the invocation
> + * of the program.
> + */
> +static struct trace_file_list {		/* List of tracing files */
> +	const char *filename;		/* File name */
> +	char *contents;			/* Contents to restore */
> +	int (*read_fct)(const char *fn, char *buf, size_t buf_sz);		/* Read function */
> +	int (*write_fct)(const char *fn, const char *buf);		/* Write function */
> +} trace_file_list[] = {
> +	[0] = {
> +		.filename = "tracing_on",
> +		.read_fct = read_tracing_file,
> +		.write_fct = write_tracing_file,
> +	},
> +	[1] = {
> +		.filename = "current_tracer",
> +		.read_fct = read_tracing_file,
> +		.write_fct = write_tracing_file,
> +	},
> +	[2] = {
> +		.filename = "set_ftrace_pid",
> +		.read_fct = read_tracing_file,
> +		.write_fct = write_tracing_file,
> +	},
> +	[3] = {
> +		.write_fct = write_tracing_file,
> +		.read_fct = read_tracing_file,
> +		.filename = "max_graph_depth",
> +	},
> +	[4] = {
> +		.filename = "tracing_thresh",
> +		.read_fct = read_tracing_file,
> +		.write_fct = write_tracing_file,
> +	},
> +	[5] = {
> +		.filename = "tracing_cpumask",
> +		.read_fct = read_tracing_file,
> +		.write_fct = write_tracing_file,
> +	},
> +	[6] = {
> +		.filename = "set_ftrace_filter",
> +		.read_fct = read_tracing_file,
> +		.write_fct = write_tracing_file,
> +	},
> +	[7] = {
> +		.filename = "set_ftrace_notrace",
> +		.read_fct = read_tracing_file,
> +		.write_fct = write_tracing_file,
> +	},
> +	[8] = {
> +		.filename = "set_graph_function",
> +		.read_fct = read_tracing_file,
> +		.write_fct = write_tracing_file,
> +	},
> +	[9] = {
> +		.filename = "set_graph_notrace",
> +		.read_fct = read_tracing_file,
> +		.write_fct = write_tracing_file,
> +	},
> +			/* Files in .../options/ directory */
> +	[10] = {
> +		.filename = "function-fork",
> +		.read_fct = read_tracing_option_file,
> +		.write_fct = write_tracing_option_file,
> +	},
> +	[11] = {
> +		.filename = "func_stack_trace",
> +		.read_fct = read_tracing_option_file,
> +		.write_fct = write_tracing_option_file,
> +	},
> +	[12] = {
> +		.filename = "sleep-time",
> +		.read_fct = read_tracing_option_file,
> +		.write_fct = write_tracing_option_file,
> +	},
> +	[13] = {
> +		.filename = "funcgraph-irqs",
> +		.read_fct = read_tracing_option_file,
> +		.write_fct = write_tracing_option_file,
> +	},
> +	[14] = {
> +		.filename = "funcgraph-proc",
> +		.read_fct = read_tracing_option_file,
> +		.write_fct = write_tracing_option_file,
> +	},
> +	[15] = {
> +		.filename = "funcgraph-abstime",
> +		.read_fct = read_tracing_option_file,
> +		.write_fct = write_tracing_option_file,
> +	},
> +	[16] = {
> +		.filename = "funcgraph-tail",
> +		.read_fct = read_tracing_option_file,
> +		.write_fct = write_tracing_option_file,
> +	},
> +	[17] = {
> +		.filename = "latency-format",
> +		.read_fct = read_tracing_option_file,
> +		.write_fct = write_tracing_option_file,
> +	},
> +	[18] = {
> +		.filename = "irq-info",
> +		.read_fct = read_tracing_option_file,
> +		.write_fct = write_tracing_option_file,
> +	},
> +};
> +
> +static void free_tracing_content(void)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(trace_file_list); ++i)
> +		zfree(&trace_file_list[i].contents);
> +}
> +
> +/*
> + * Return a copy of the input string.
> + * Remove a trailing newline. It will be appended in the write
> + * function when values are restored before program termination.
> + * Change "no pid" or comment sign '#' at the beginning and replace it
> + * by an empty string. This resets to the default behavior indicated
> + * by the output. Those strings are not accepted as file input.
> + */
> +static char *copy_tracing_file(char *buf)
> +{
> +	char *c = strrchr(buf, '\n');
> +
> +	if (c)
> +		*c = '\0';
> +	if (*buf == '#' || !strncmp(buf, "no pid", 6))
> +		*buf = '\0';
> +	return strdup(buf);
> +}
> +
> +static int save_tracing_files(void)
> +{
> +	char buf[4096];
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(trace_file_list); ++i) {
> +		struct trace_file_list *tp = &trace_file_list[i];
> +
> +		memset(buf, 0, sizeof(buf));
> +		if ((*tp->read_fct)(tp->filename, buf, sizeof(buf)) < 0)
> +			goto out;
> +		tp->contents = copy_tracing_file(buf);
> +		if (!tp->contents)
> +			goto out;
> +	}
> +	return 0;
> +
> +out:
> +	free_tracing_content();
> +	return -1;
> +}
> +
> +static void restore_tracing_files(void)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(trace_file_list); ++i) {
> +		struct trace_file_list *tp = &trace_file_list[i];
> +
> +		(*tp->write_fct)(tp->filename, tp->contents);
> +	}
> +	free_tracing_content();
> +}
> +
>  static int set_tracing_pid(struct perf_ftrace *ftrace)
>  {
>  	int i;
> @@ -1687,6 +1909,9 @@ int cmd_ftrace(int argc, const char **argv)
>  	};
>  	enum perf_ftrace_subcommand subcmd = PERF_FTRACE_NONE;
>  
> +	if (save_tracing_files())
> +		return -1;
> +
>  	INIT_LIST_HEAD(&ftrace.filters);
>  	INIT_LIST_HEAD(&ftrace.notrace);
>  	INIT_LIST_HEAD(&ftrace.graph_funcs);
> @@ -1839,5 +2064,6 @@ int cmd_ftrace(int argc, const char **argv)
>  	delete_filter_func(&ftrace.graph_funcs);
>  	delete_filter_func(&ftrace.nograph_funcs);
>  
> +	restore_tracing_files();
>  	return ret;
>  }
> -- 
> 2.49.0

Powered by blists - more mailing lists