Re: [PATCH v5 10/26] perf annotate: Default to --itrace=i1i for data type profiling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The default_single_event_per_ip option is explicitly ignored for Intel PT, Intel BTS, and CS-ETM decoders, leading to skewed data type profiling statistics on those PMUs.
--

commit 177095456668310745d79faec9f1ed30c358ec07
Author: Tengda Wu <wutengda@huaweicloud.com>

perf annotate: Default to --itrace=i1i for data type profiling

This commit introduces a `default_single_event_per_ip` option to the
`itrace_synth_opts` structure. It aims to prevent duplicate sample counts when
data type profiling is enabled by synthesizing at most one event per IP,
avoiding skewed statistics on hardware tracing PMUs.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 114b3cd2da495..2d1ab34f7b6b5 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -3626,7 +3626,7 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
>  		etm->synth_opts = *session->itrace_synth_opts;
>  	} else {
>  		itrace_synth_opts__set_default(&etm->synth_opts,
> -				session->itrace_synth_opts->default_no_sample);
> +				session->itrace_synth_opts->default_no_sample, false);

[Severity: Medium]
Does hardcoding false here defeat the purpose of the fix for CS-ETM traces?

When perf annotate --data-type runs without an explicit --itrace option,
cs_etm__process_auxtrace_info_full() ignores the new
default_single_event_per_ip configuration.

This causes CS-ETM to synthesize both instructions and branches for a single IP,
duplicating sample counts and skewing data type profiling statistics.

Should it pass session->itrace_synth_opts->default_single_event_per_ip instead?

[ ... ]

> diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
> index 02df3e4604891..4c9cca3acb6ca 100644
> --- a/tools/perf/util/intel-bts.c
> +++ b/tools/perf/util/intel-bts.c
> @@ -877,7 +877,7 @@ int intel_bts_process_auxtrace_info(union perf_event *event,
>  		bts->synth_opts = *session->itrace_synth_opts;
>  	} else {
>  		itrace_synth_opts__set_default(&bts->synth_opts,
> -				session->itrace_synth_opts->default_no_sample);
> +				session->itrace_synth_opts->default_no_sample, false);

[Severity: Medium]
Similarly, is there a specific reason to hardcode false for Intel BTS in
intel_bts_process_auxtrace_info()? 

This explicitly ignores the single event per IP option for BTS profiles, which
may also lead to skewed statistics.

[ ... ]

> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index 9d6628169fd95..8c21c9f52d572 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -4463,7 +4463,8 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
>  	} else {
>  		struct itrace_synth_opts *opts = session->itrace_synth_opts;
>  
> -		itrace_synth_opts__set_default(&pt->synth_opts, opts->default_no_sample);
> +		itrace_synth_opts__set_default(&pt->synth_opts,
> +					opts->default_no_sample, false);

[Severity: Medium]
Does Intel PT also need to support this new option?

Hardcoding false here in intel_pt_process_auxtrace_info() forces Intel PT to
ignore opts->default_single_event_per_ip. This allows it to synthesize
multiple events per instruction (such as transactions or ptwrites), which can
distort data type profiles just like ARM SPE did before this patch.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788872630.git.wutengda@huaweicloud.com?part=10