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

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

[Leo Yan]

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.

 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");
+				evsel->core.attr.sample_type |= PERF_SAMPLE_DATA_SRC;
+				sample_type |= PERF_SAMPLE_DATA_SRC;
+			}
+		}
+
 		if (!is_pipe && !(sample_type & PERF_SAMPLE_DATA_SRC)) {
 			ui__error("Selected --mem-mode but no mem data. "
 				  "Did you call perf record without -d?\n");
-- 
2.25.1

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

[Arnaldo Carvalho de Melo]

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");
> +				evsel->core.attr.sample_type |= PERF_SAMPLE_DATA_SRC;
> +				sample_type |= PERF_SAMPLE_DATA_SRC;
> +			}
> +		}
> +
>  		if (!is_pipe && !(sample_type & PERF_SAMPLE_DATA_SRC)) {
>  			ui__error("Selected --mem-mode but no mem data. "
>  				  "Did you call perf record without -d?\n");
> -- 
> 2.25.1

-- 

- Arnaldo

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

[James Clark]

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.

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>

>> +				evsel->core.attr.sample_type |= PERF_SAMPLE_DATA_SRC;
>> +				sample_type |= PERF_SAMPLE_DATA_SRC;
>> +			}
>> +		}
>> +
>>  		if (!is_pipe && !(sample_type & PERF_SAMPLE_DATA_SRC)) {
>>  			ui__error("Selected --mem-mode but no mem data. "
>>  				  "Did you call perf record without -d?\n");
>> -- 
>> 2.25.1
> 

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

[Leo Yan]

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

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

[Arnaldo Carvalho de Melo]

Em Thu, Apr 14, 2022 at 11:29:48AM +0100, James Clark escreveu:
> 
> 
> 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.
> 
> 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, collecting your review,

- Arnaldo

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

[Arnaldo Carvalho de Melo]

Em Thu, Apr 14, 2022 at 07:01:24PM +0800, Leo Yan escreveu:
> 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.

Waiting then
 
> > 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

-- 

- Arnaldo