Re: [PATCH perf-tools-next v9 1/6] perf ftrace: Optimise __cmd_ftrace() stack frame with strbuf streaming

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Oct 20, 2026 at 10:33:32PM -0400, Aaron Tomlin wrote:
> In __cmd_ftrace(), incoming trace data from trace_pipe was historically
> buffered in a fixed 4096-byte array allocated directly on the stack
> frame. Furthermore, reads were written directly to stdout via fwrite()
> in arbitrary chunk sizes, rather than as discrete, well-delimited
> trace lines.
> 
> Refactor __cmd_ftrace() to employ dynamic heap allocation for the read
> buffer alongside a dynamic struct strbuf to accumulate streamed
> characters into whole lines. Pre-allocating the strbuf ensures that
> line processing incurs zero heap reallocations during steady-state
> polling, while the stack frame of __cmd_ftrace() is reduced by over 96%
> (down to ~160 bytes). Robust error and EOF detection on stdout and the
> trace stream ensures polling terminates cleanly upon pipeline or pager
> exit, while retrying on non-fatal EAGAIN and EINTR conditions prevents
> premature termination on non-blocking reads or signal interruptions.

I think it's better to split the error handling into its own commit
maybe with a Fixes tag.  But I'm curious if it's possible to get EAGAIN
on read() after poll().

Thanks,
Namhyung

> 
> Signed-off-by: Aaron Tomlin <atomlin@atomlin.com>
> ---
>  tools/perf/builtin-ftrace.c | 79 +++++++++++++++++++++++++++++++------
>  1 file changed, 66 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 6017493ff179..65983259b07e 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -14,6 +14,7 @@
>  #include <poll.h>
>  #include <semaphore.h>
>  #include <signal.h>
> +#include <stdio.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <linux/capability.h>
> @@ -41,7 +42,10 @@
>  #include "util/units.h"
>  #include "util/parse-sublevel-options.h"
>  
> +#include "util/strbuf.h"
> +
>  #define DEFAULT_TRACER  "function_graph"
> +#define TRACE_BUF_SIZE  4096
>  
>  static volatile sig_atomic_t workload_exec_errno;
>  static volatile sig_atomic_t done;
> @@ -738,7 +742,8 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>  {
>  	char *trace_file;
>  	int trace_fd;
> -	char buf[4096];
> +	char *buf = NULL;
> +	struct strbuf linebuf = STRBUF_INIT;
>  	struct pollfd pollfd = {
>  		.events = POLLIN,
>  	};
> @@ -767,10 +772,18 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>  
>  	setup_pager();
>  
> +	buf = malloc(TRACE_BUF_SIZE);
> +	if (!buf) {
> +		pr_err("failed to allocate trace buffer\n");
> +		goto out_reset;
> +	}
> +	strbuf_init(&linebuf, 512);
> +	strbuf_setlen(&linebuf, 0);
> +
>  	trace_file = get_tracing_instance_file("trace_pipe");
>  	if (!trace_file) {
>  		pr_err("failed to open trace_pipe\n");
> -		goto out_reset;
> +		goto out_free;
>  	}
>  
>  	trace_fd = open(trace_file, O_RDONLY);
> @@ -779,7 +792,7 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>  
>  	if (trace_fd < 0) {
>  		pr_err("failed to open trace_pipe\n");
> -		goto out_reset;
> +		goto out_free;
>  	}
>  
>  	fcntl(trace_fd, F_SETFL, O_NONBLOCK);
> @@ -806,24 +819,42 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>  	}
>  
>  	while (!done) {
> -		if (poll(&pollfd, 1, -1) < 0)
> +		if (poll(&pollfd, 1, -1) < 0) {
> +			if (errno == EINTR)
> +				continue;
>  			break;
> +		}
>  
>  		if (pollfd.revents & POLLIN) {
> -			int n = read(trace_fd, buf, sizeof(buf));
> -			if (n < 0)
> +			int n = read(trace_fd, buf, TRACE_BUF_SIZE);
> +
> +			if (n < 0) {
> +				if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR)
> +					continue;
>  				break;
> -			if (fwrite(buf, n, 1, stdout) != 1)
> +			}
> +			if (n == 0)
>  				break;
> +			for (int i = 0; i < n; i++) {
> +				if (buf[i] == '\n') {
> +					if (fprintf(stdout, "%s\n", linebuf.buf) < 0)
> +						break;
> +					strbuf_setlen(&linebuf, 0);
> +				} else {
> +					if (strbuf_addch(&linebuf, buf[i]) < 0)
> +						goto out_close_fd;
> +				}
> +			}
>  			/* flush output since stdout is in full buffering mode due to pager */
> -			fflush(stdout);
> +			if (fflush(stdout) < 0 || ferror(stdout))
> +				break;
>  		}
>  	}
>  
>  	write_tracing_file("tracing_on", "0");
>  
>  	if (workload_exec_errno) {
> -		const char *emsg = str_error_r(workload_exec_errno, buf, sizeof(buf));
> +		const char *emsg = str_error_r(workload_exec_errno, buf, TRACE_BUF_SIZE);
>  		/* flush stdout first so below error msg appears at the end. */
>  		fflush(stdout);
>  		pr_err("workload failed: %s\n", emsg);
> @@ -831,16 +862,38 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>  	}
>  
>  	/* read remaining buffer contents */
> -	while (true) {
> -		int n = read(trace_fd, buf, sizeof(buf));
> -		if (n <= 0)
> +	while (!ferror(stdout)) {
> +		int n = read(trace_fd, buf, TRACE_BUF_SIZE);
> +
> +		if (n < 0) {
> +			if (errno == EINTR)
> +				continue;
>  			break;
> -		if (fwrite(buf, n, 1, stdout) != 1)
> +		}
> +		if (n == 0)
>  			break;
> +		for (int i = 0; i < n; i++) {
> +			if (buf[i] == '\n') {
> +				if (fprintf(stdout, "%s\n", linebuf.buf) < 0)
> +					break;
> +				strbuf_setlen(&linebuf, 0);
> +			} else {
> +				if (strbuf_addch(&linebuf, buf[i]) < 0)
> +					goto out_close_fd;
> +			}
> +		}
> +	}
> +
> +	if (!ferror(stdout) && linebuf.len > 0) {
> +		fprintf(stdout, "%s\n", linebuf.buf);
> +		strbuf_setlen(&linebuf, 0);
>  	}
>  
>  out_close_fd:
>  	close(trace_fd);
> +out_free:
> +	free(buf);
> +	strbuf_release(&linebuf);
>  out_reset:
>  	exit_tracing_instance();
>  out:
> -- 
> 2.55.0
>