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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf ftrace: --graph-opts duration silently clobbers noduration
--

commit e23672676ea60d48741ccc720b34c8778668c668
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. Introduce support for omitting function duration by exposing the
    noduration sub-option under --graph-opts (and conversely duration=[0|1]).

> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index e11418e619378..88df6b09d9b0d 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
[ ... ]
> @@ -1754,6 +1772,8 @@ 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 },
> +		{ .name = "duration",		.value_ptr = &duration },
>  		{ .name = "verbose",		.value_ptr = &ftrace->graph_verbose },
>  		{ .name = "thresh",		.value_ptr = &ftrace->graph_thresh },
>  		{ .name = "depth",		.value_ptr = &ftrace->graph_depth },
> @@ -1769,6 +1789,9 @@ static int parse_graph_tracer_opts(const struct option *opt,
>  	if (ret)
>  		return ret;
>  
> +	if (duration != -1)
> +		ftrace->graph_noduration = !duration;
> +

[Severity: Medium]
Does this logic unconditionally overwrite the noduration parameter?

If a user executes perf ftrace with --graph-opts duration=1,noduration, the
token parsing loop in perf_parse_sublevel_options() correctly processes them
sequentially. It sets duration to 1 and ftrace->graph_noduration to 1.

However, since duration != -1 is evaluated after the loop, it overwrites
ftrace->graph_noduration with !1 (which is 0). This causes the duration
parameter to always clobber the noduration parameter, regardless of their
order in the comma-separated list.

Would it be possible to parse both directives strictly in the order they are
specified without unconditionally overriding one after the loop?

>  	if (filter_retaddr != -1) {
>  		ftrace->filter_retaddr = (filter_retaddr != 0);
>  		ftrace->filter_retaddr_set = true;

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