Re: [PATCH v2] perf report: Set PERF_SAMPLE_DATA_SRC bit for Arm SPE event

[Leo Yan <leo.yan@linaro.org>]

On Thu, Apr 14, 2022 at 11:29:48AM +0100, James Clark wrote:
> On 14/04/2022 02:27, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Apr 13, 2022 at 05:23:17PM +0800, Leo Yan escreveu:
> >> Since commit bb30acae4c4d ("perf report: Bail out --mem-mode if mem info
> >> is not available") "perf mem report" and "perf report --mem-mode"
> >> don't report result if the PERF_SAMPLE_DATA_SRC bit is missed in sample
> >> type.
> >>
> >> The commit ffab48705205 ("perf: arm-spe: Fix perf report --mem-mode")
> >> partially fixes the issue.  It adds PERF_SAMPLE_DATA_SRC bit for Arm SPE
> >> event, this allows the perf data file generated by kernel v5.18-rc1 or
> >> later version can be reported properly.
> >>
> >> On the other hand, perf tool still fails to be backward compatibility
> >> for a data file recorded by an older version's perf which contains Arm
> >> SPE trace data.  This patch is a workaround in reporting phase, when
> >> detects ARM SPE PMU event and without PERF_SAMPLE_DATA_SRC bit, it will
> >> force to set the bit in the sample type and give a warning info.
> >>
> >> Fixes: bb30acae4c4d ("perf report: Bail out --mem-mode if mem info is not available")
> >> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> >> Tested-by: German Gomez <german.gomez@arm.com>
> >> ---
> >> v2: Change event name from "arm_spe_" to "arm_spe";
> >>     Add German's test tag.
> > 
> > Tentatively applied, would be great to have James' and Ravi's
> > Acked-by/Reviewed-by, which I'll add before pushing this out if provided
> > in time.
> > 
> > - Arnaldo
> >  
> >>  tools/perf/builtin-report.c | 16 ++++++++++++++++
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> >> index 1ad75c7ba074..acb07a4a9b67 100644
> >> --- a/tools/perf/builtin-report.c
> >> +++ b/tools/perf/builtin-report.c
> >> @@ -353,6 +353,7 @@ static int report__setup_sample_type(struct report *rep)
> >>  	struct perf_session *session = rep->session;
> >>  	u64 sample_type = evlist__combined_sample_type(session->evlist);
> >>  	bool is_pipe = perf_data__is_pipe(session->data);
> >> +	struct evsel *evsel;
> >>  
> >>  	if (session->itrace_synth_opts->callchain ||
> >>  	    session->itrace_synth_opts->add_callchain ||
> >> @@ -407,6 +408,21 @@ static int report__setup_sample_type(struct report *rep)
> >>  	}
> >>  
> >>  	if (sort__mode == SORT_MODE__MEMORY) {
> >> +		/*
> >> +		 * FIXUP: prior to kernel 5.18, Arm SPE missed to set
> >> +		 * PERF_SAMPLE_DATA_SRC bit in sample type.  For backward
> >> +		 * compatibility, set the bit if it's an old perf data file.
> >> +		 */
> >> +		evlist__for_each_entry(session->evlist, evsel) {
> >> +			if (strstr(evsel->name, "arm_spe") &&
> >> +				!(sample_type & PERF_SAMPLE_DATA_SRC)) {
> >> +				ui__warning("PERF_SAMPLE_DATA_SRC bit is not set "
> >> +					    "for Arm SPE event.\n");
> 
> Looks ok to me. Personally I would remove the warning, otherwise people are going to start
> thinking that they need to do something about it or something bad has happened.
> 
> But because we've fixed it up there shouldn't really need to be a warning or any action.

Understand.  The warning is not bad for developers but it might
introduce confusion for users, and if we really want to check the sample
type then we can use 'perf evlist' command, so it's not very useful for
the warning.

I will remove the warning and resend a new patch.

> I don't feel too strongly about this though, so I will leave it up to Leo to make the
> final decision:
> 
> Reviewed-by: James Clark <james.clark@arm.com>

Thanks a lot for reviewing.
Leo