Re: [PATCH v4 09/23] perf annotate: Deduplicate overlapping ARM SPE events for data type profiling

[Tengda Wu]

On 2026/8/12 14:07, Adrian Hunter wrote:
> On 12/08/2026 05:28, Tengda Wu wrote:
>>
>>
>> On 2026/8/10 14:57, Adrian Hunter wrote:
>>> On 08/08/2026 15:23, Tengda Wu wrote:
>>>> When data type profiling is enabled on ARM SPE, multiple overlapping
>>>> events (e.g., l1d-miss, tlb-access) are synthesized for a single sampled
>>>> instruction, as shown below:
>>>>
>>>>   Available samples
>>>>   0 arm_spe_0/ts_enable=1,pa_enable=1,load_filter=1,store_filter=1,min_latency=30/
>>>>   0 dummy:u
>>>>   84K l1d-miss
>>>>   95K l1d-access
>>>>   77K llc-miss
>>>>   58K llc-access
>>>>   9K tlb-miss
>>>>   108K tlb-access
>>>>   0 branch
>>>>   13K remote-access
>>>>   108K memory
>>>>   108K instructions
>>>>
>>>> While 'perf report' provides an interactive menu for users to select a
>>>> specific event to prevent duplicate counting, 'perf annotate' lacks such
>>>> a mechanism. Consequently, it counts all instructions across these
>>>> overlapping events, which inflates the profile and distorts the data
>>>> type statistics.
>>>>
>>>> Although using the '--itrace' option can work around this issue (e.g.:
>>>> perf annotate --data-type --stdio --itrace=i1i), it is inconvenient for
>>>> users to specify this explicitly every time.
>>>>
>>>> To address this, introduce itrace_synth_opts.dont_overlap. Set this to true
>>>> when data type profiling is enabled and the user has not explicitly
>>>> specified an itrace option. Then, during arm_spe_process_auxtrace_info(),
>>>> adjust the synthesized event options based on the dont_overlap value to
>>>> only enable instruction event synthesis, thereby achieving automatic
>>>> deduplication.
>>>>
>>>> Signed-off-by: Tengda Wu <wutengda@xxxxxxxxxxxxxxx>
>>>> ---
>>>>  tools/perf/builtin-annotate.c |  8 ++++++++
>>>>  tools/perf/util/arm-spe.c     | 17 +++++++++++++++++
>>>>  tools/perf/util/auxtrace.h    |  2 ++
>>>>  3 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
>>>> index 69cb72b2082a..616f54bf4868 100644
>>>> --- a/tools/perf/builtin-annotate.c
>>>> +++ b/tools/perf/builtin-annotate.c
>>>> @@ -873,6 +873,14 @@ int cmd_annotate(int argc, const char **argv)
>>>>  	annotate.session = perf_session__new(&data, &annotate.tool);
>>>>  	if (IS_ERR(annotate.session))
>>>>  		return PTR_ERR(annotate.session);
>>>> +	/*
>>>> +	 * Hardware tracing (e.g.: ARM SPE) may generate overlapping events
>>>> +	 * per instruction. When data type profiling is enabled, enable
>>>> +	 * dont_overlap to deduplicate them to avoid skewed stats, but only
>>>> +	 * if user hasn't specified itrace options (respect user override).
>>>> +	 */
>>>> +	if (annotate.data_type && !itrace_synth_opts.set)
>>>> +		itrace_synth_opts.dont_overlap = true;
>>>>  
>>>>  	annotate.session->itrace_synth_opts = &itrace_synth_opts;
>>>>  
>>>> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
>>>> index 401aab529309..1721882423f6 100644
>>>> --- a/tools/perf/util/arm-spe.c
>>>> +++ b/tools/perf/util/arm-spe.c
>>>> @@ -2033,6 +2033,9 @@ int arm_spe_process_auxtrace_info(union perf_event *event,
>>>>  		/* Default nanoseconds period not supported */
>>>>  		spe->synth_opts.period_type = PERF_ITRACE_PERIOD_INSTRUCTIONS;
>>>>  		spe->synth_opts.period = 1;
>>>> +
>>>> +		if (session->itrace_synth_opts)
>>>> +			spe->synth_opts.dont_overlap = session->itrace_synth_opts->dont_overlap;
>>>>  	}
>>>>  
>>>>  	if (spe->synth_opts.period_type != PERF_ITRACE_PERIOD_INSTRUCTIONS) {
>>>> @@ -2044,6 +2047,20 @@ int arm_spe_process_auxtrace_info(union perf_event *event,
>>>>  		ui__warning("Arm SPE has a hardware-based sampling period.\n\n"
>>>>  			    "--itrace periods > 1i downsample by an interval of n SPE samples rather than n instructions.\n");
>>>>  
>>>> +	if (spe->synth_opts.dont_overlap) {
>>>> +		/*
>>>> +		 * The 'instructions' event is the most comprehensive,
>>>> +		 * synthesize it exclusively.
>>>> +		 */
>>>> +		spe->synth_opts.flc = false;
>>>> +		spe->synth_opts.llc = false;
>>>> +		spe->synth_opts.tlb = false;
>>>> +		spe->synth_opts.branches = false;
>>>> +		spe->synth_opts.remote_access = false;
>>>> +		spe->synth_opts.mem = false;
>>>> +		spe->synth_opts.instructions = true;
>>>> +	}
>>>> +
>>>>  	err = arm_spe_synth_events(spe, session);
>>>>  	if (err)
>>>>  		goto err_free_queues;
>>>> diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
>>>> index 6947f3f284c0..ebb4e9e8e574 100644
>>>> --- a/tools/perf/util/auxtrace.h
>>>> +++ b/tools/perf/util/auxtrace.h
>>>> @@ -80,6 +80,7 @@ enum itrace_period_type {
>>>>   * @intr_events: whether to synthesize interrupt events
>>>>   * @errors: whether to synthesize decoder error events
>>>>   * @dont_decode: whether to skip decoding entirely
>>>> + * @dont_overlap: whether to deduplicate overlapping events
>>>
>>> This doesn't seem to match what you are actually doing, which
>>> seems to be to choose particular default itrace options for
>>> 'perf annotate --data-type'
>>>
>>> I wonder if this should really be handled by
>>> itrace_synth_opts__set_default()?
>>>
>>
>> Placing this in itrace_synth_opts__set_default() may not be appropriate.
> 
> Your current approach introduces the concepts of
> "dont_overlap" and "deduplicate" but doesn't actually
> map them to specific functionality.  They are a mismatch
> with what you are actually doing.
> 

Agreed.

>>
>> The current approach is designed to constrain the following conditions:
>>
>> 1. Using perf annotate
>> 2. With --data-type specified
>> 3. Based on ARM SPE
>> 4. Without --itrace explicitly specified
>>
>> When these conditions are all met, we want to enable --itrace=i1i by default,
>> effectively making this option built-in.
>>
>> If we were to place the --itrace=i1i configuration inside
>> itrace_synth_opts__set_default(), conditions 1, 2, and 3 would not be
>> reachable from that context, as there's no way to propagate that information
>> into the function.
> 
> itrace_synth_opts__set_default() is only called when 4 is true.
> 
> WRT 1 and 2, you are planning to pass information via synth_opts,
> anyway, so it is the same in that regard.
> 

Agreed.

>>                    Configuring it directly there would take effect globally,
>> impacting other commands such as perf report and perf script.
> 
> Not if they did not pass the new information, same as your current
> approach.
> 

Agreed.

>>
>> Therefore, the current approach passes the dont_overlap information through
>> itrace (satisfying conditions 1, 2, and 4), and then configures --itrace=i1i
>> inside the ARM SPE driver (satisfying condition 3).
> 
> Why wouldn't that default be ok for other auxtrace implementations?
> 

To be honest, I wasn't entirely sure about the potential impact of making
'--itrace=i1i' the default for 'perf annotate --data-type' across all
architectures, so I took the most conservative approach and restricted it
to ARM SPE.

However, if another architecture's auxtrace backend also synthesizes multiple
events from a single instruction, it would in principle need --itrace=i1i as
well. I also tested this on an Intel environment for comparison, enabling
--itrace=i1i or not yielded the exact same sample count, so making it the
default should be safe:

  perf annotate --data-type --stdio --type-stat
     total 880, ok 711 (80.8%), bad 169 (19.2%)

  perf annotate --data-type --stdio --type-stat --itrace=i1i
     total 880, ok 711 (80.8%), bad 169 (19.2%)

So I agree with your point, I'll move this configuration directly into
itrace_synth_opts__set_default().

> In any case, the problem is with the mismatch between the naming,
> definition, and framing of "dont_overlap" and how it is actually used.
> 
> Perhaps instead:
> 
> 	default_single_event_per_ip
> 
> and explain the annotate data-type use-case in its kernel-doc
> description.
> 

Yes, default_single_event_per_ip is indeed much more accurate and clear.
I'll fix it.

Thanks,
Tengda

>>>
>>>>   * @log: write a decoding log
>>>>   * @calls: limit branch samples to calls (can be combined with @returns)
>>>>   * @returns: limit branch samples to returns (can be combined with @calls)
>>>> @@ -128,6 +129,7 @@ struct itrace_synth_opts {
>>>>  	bool			intr_events;
>>>>  	bool			errors;
>>>>  	bool			dont_decode;
>>>> +	bool			dont_overlap;
>>>>  	bool			log;
>>>>  	bool			calls;
>>>>  	bool			returns;
>>
>