Re: [PATCH perf-tools-next v3 4/5] perf ftrace: Support omitting execution duration in function graph tracer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] perf: formatting corruption in make_blank_prefix() with verbose enabled
- [Low] perf: profile subcommand silently drops records with noduration
--

commit d0f3a0ada6164d2372b897fe5b75d0f425aca5e4
Author: Aaron Tomlin <atomlin@atomlin.com>

perf ftrace: Support omitting execution duration in function graph tracer

The Linux kernel's function graph tracer displays the execution duration
of completed functions by default via the funcgraph-duration trace
option. Whilst informative, in scenarios where the user is solely
interested in the call graph topology, call flow sequence, or inlined
hierarchy, the duration column can add visual clutter and widen each
trace line.

[ ... ]

> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 504007064e240..926726c5d9a88 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c

[ ... ]

> @@ -1754,6 +1780,7 @@ static int parse_graph_tracer_opts(const struct option *opt,
>  		{ .name = "retaddr",		.value_ptr = &ftrace->graph_retaddr },
>  		{ .name = "nosleep-time",	.value_ptr = &ftrace->graph_nosleep_time },
>  		{ .name = "noirqs",		.value_ptr = &ftrace->graph_noirqs },
> +		{ .name = "noduration",		.value_ptr = &ftrace->graph_noduration },

[Severity: Low]
Does adding noduration here allow it to be used with the profile subcommand?

The profile subcommand inherently requires function duration to compute
statistics. When the duration column is omitted, parse_func_duration()
sees no " us" suffix and silently drops the record:

tools/perf/builtin-ftrace.c:parse_func_duration() {
    ...
	if (strncmp(p, " us", 3)) {
		pr_debug("non-usec time found.. ignoring\n");
		return 0;
	}
    ...
}

This results in an empty output instead of rejecting the invalid option
combination. Should the option be rejected when used with profile?

>  		{ .name = "verbose",		.value_ptr = &ftrace->graph_verbose },
>  		{ .name = "thresh",		.value_ptr = &ftrace->graph_thresh },
>  		{ .name = "depth",		.value_ptr = &ftrace->graph_depth },

[ ... ]

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> index 8737bb52921e1..5cae01a28529f 100644
> --- a/tools/perf/util/ftrace.c
> +++ b/tools/perf/util/ftrace.c

[ ... ]

> @@ -346,9 +348,14 @@ static const char *find_func_column(const struct perf_ftrace *ftrace,
>  			return NULL;
>  	}
>  
> -	p = strchr(p + 1, '|');
> -	if (!p)
> +	if (!ftrace->graph_noduration) {
> +		p = strchr(p + 1, '|');
> +		if (!p)
> +			return NULL;
> +	} else if (strstr(p, " => ")) {
> +		/* Context switch marker: " CPU) comm-pid => comm-pid" */
>  		return NULL;
> +	}

[Severity: Low]
Will this corrupt the comm-pid column when verbose is enabled alongside
noduration?

When both verbose and noduration are enabled, find_func_column() returns
the CPU ')' or the separator after it. make_blank_prefix() uses the final
pipe_char and overwrites everything after the ')' with spaces:

tools/perf/util/ftrace.c:make_blank_prefix() {
    ...
	/* Replace duration between ')' and '|' with spaces */
	if (paren && paren < line + len) {
		char *end = buf + (pipe_char - line);
    ...
		for (c = buf + (paren - line) + 1; c < end; c++)
			*c = ' ';
	}
    ...
}

Since there is no duration field, does this blindly overwrite the comm-pid
column and its trailing separator?

>  
>  	*cpu = (int)val;
>  	return p;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012003016.263063-1-atomlin@atomlin.com?part=4