[PATCH v1 0/3] perf arm-spe: Correct recording configurations

[PATCH v1 0/3] perf arm-spe: Correct recording configurations

[Leo Yan]

This patch series is to correct Arm SPE recording configurations.

As found by James Clark, it's not reasonable for the current code for
sample flags CPU/TIME/TID which are hard coded.  For TIME sample flag,
since it's always enabled, then Arm SPE has no chance for timeless
tracing; for CPU sample flag, it's not needed for per-thread mode;
for TID sample flag, it's redundant for AUX and dummy events.

This series corrects the sample flags setting, and it enables
timestamp for per-cpu mode tracing by default.

This patch set has been tested on Arm64 Hisilicon D06 platform.


Leo Yan (3):
  perf arm-spe: Correct sample flags for SPE event
  perf arm-spe: Correct sample flags for dummy event
  perf arm-spe: Enable timestamp for per-cpu mode

 tools/perf/arch/arm64/util/arm-spe.c | 66 +++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 16 deletions(-)

-- 
2.25.1

[PATCH v1 0/3] perf arm-spe: Correct recording configurations

[Leo Yan]

This patch series is to correct Arm SPE recording configurations.

As found by James Clark, it's not reasonable for the current code for
sample flags CPU/TIME/TID which are hard coded.  For TIME sample flag,
since it's always enabled, then Arm SPE has no chance for timeless
tracing; for CPU sample flag, it's not needed for per-thread mode;
for TID sample flag, it's redundant for AUX and dummy events.

This series corrects the sample flags setting, and it enables
timestamp for per-cpu mode tracing by default.

This patch set has been tested on Arm64 Hisilicon D06 platform.


Leo Yan (3):
  perf arm-spe: Correct sample flags for SPE event
  perf arm-spe: Correct sample flags for dummy event
  perf arm-spe: Enable timestamp for per-cpu mode

 tools/perf/arch/arm64/util/arm-spe.c | 66 +++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 16 deletions(-)

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/3] perf arm-spe: Correct sample flags for SPE event

[Leo Yan]

Now it's hard code to set sample flags for CPU, TIME and TID for SPE
event, which is pointless.

The CPU is useful for sampling only for per-mmap case, it is used to
indicate the AUX trace is associated to which CPU.

The TIME sample is not needed for AUX event, since the time for AUX
event is not really used and this time is a different thing from the
timestamp in Arm SPE trace, the timestamp tracing which is controlled
by Arm SPE's config bit.

The TID sample is not useful for AUX event.

This patch corrects the sample flags for SPE event, it only set CPU
sample bit for per-cpu mmap case.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/arch/arm64/util/arm-spe.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index 414c8a5584b1..902e73a64184 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -68,6 +68,7 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 			container_of(itr, struct arm_spe_recording, itr);
 	struct perf_pmu *arm_spe_pmu = sper->arm_spe_pmu;
 	struct evsel *evsel, *arm_spe_evsel = NULL;
+	struct perf_cpu_map *cpus = evlist->core.cpus;
 	bool privileged = perf_event_paranoid_check(-1);
 	struct evsel *tracking_evsel;
 	int err;
@@ -120,9 +121,9 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	 */
 	evlist__to_front(evlist, arm_spe_evsel);
 
-	evsel__set_sample_bit(arm_spe_evsel, CPU);
-	evsel__set_sample_bit(arm_spe_evsel, TIME);
-	evsel__set_sample_bit(arm_spe_evsel, TID);
+	/* In the case of per-cpu mmaps, sample CPU for AUX event. */
+	if (!perf_cpu_map__empty(cpus))
+		evsel__set_sample_bit(arm_spe_evsel, CPU);
 
 	/* Add dummy event to keep tracking */
 	err = parse_events(evlist, "dummy:u", NULL);
-- 
2.25.1

[PATCH v1 1/3] perf arm-spe: Correct sample flags for SPE event

[Leo Yan]

Now it's hard code to set sample flags for CPU, TIME and TID for SPE
event, which is pointless.

The CPU is useful for sampling only for per-mmap case, it is used to
indicate the AUX trace is associated to which CPU.

The TIME sample is not needed for AUX event, since the time for AUX
event is not really used and this time is a different thing from the
timestamp in Arm SPE trace, the timestamp tracing which is controlled
by Arm SPE's config bit.

The TID sample is not useful for AUX event.

This patch corrects the sample flags for SPE event, it only set CPU
sample bit for per-cpu mmap case.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/arch/arm64/util/arm-spe.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index 414c8a5584b1..902e73a64184 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -68,6 +68,7 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 			container_of(itr, struct arm_spe_recording, itr);
 	struct perf_pmu *arm_spe_pmu = sper->arm_spe_pmu;
 	struct evsel *evsel, *arm_spe_evsel = NULL;
+	struct perf_cpu_map *cpus = evlist->core.cpus;
 	bool privileged = perf_event_paranoid_check(-1);
 	struct evsel *tracking_evsel;
 	int err;
@@ -120,9 +121,9 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	 */
 	evlist__to_front(evlist, arm_spe_evsel);
 
-	evsel__set_sample_bit(arm_spe_evsel, CPU);
-	evsel__set_sample_bit(arm_spe_evsel, TIME);
-	evsel__set_sample_bit(arm_spe_evsel, TID);
+	/* In the case of per-cpu mmaps, sample CPU for AUX event. */
+	if (!perf_cpu_map__empty(cpus))
+		evsel__set_sample_bit(arm_spe_evsel, CPU);
 
 	/* Add dummy event to keep tracking */
 	err = parse_events(evlist, "dummy:u", NULL);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[Leo Yan]

The dummy event is mainly used for mmap, the TIME sample is only needed
for per-cpu case so that the perf tool can rely on the correct timing
for parsing symbols.  And the CPU sample is useless for mmap.

This patch enables TIME sample for per-cpu mmap and doesn't enable CPU
sample.  For later extension (e.g. support multiple AUX events), it sets
the dummy event when the condition "opts->full_auxtrace" is true.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index 902e73a64184..f6eec0900604 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	struct evsel *evsel, *arm_spe_evsel = NULL;
 	struct perf_cpu_map *cpus = evlist->core.cpus;
 	bool privileged = perf_event_paranoid_check(-1);
-	struct evsel *tracking_evsel;
 	int err;
 
 	sper->evlist = evlist;
@@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 		evsel__set_sample_bit(arm_spe_evsel, CPU);
 
 	/* Add dummy event to keep tracking */
-	err = parse_events(evlist, "dummy:u", NULL);
-	if (err)
-		return err;
-
-	tracking_evsel = evlist__last(evlist);
-	evlist__set_tracking_event(evlist, tracking_evsel);
-
-	tracking_evsel->core.attr.freq = 0;
-	tracking_evsel->core.attr.sample_period = 1;
-	evsel__set_sample_bit(tracking_evsel, TIME);
-	evsel__set_sample_bit(tracking_evsel, CPU);
-	evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK);
+	if (opts->full_auxtrace) {
+		struct evsel *tracking_evsel;
+
+		err = parse_events(evlist, "dummy:u", NULL);
+		if (err)
+			return err;
+
+		tracking_evsel = evlist__last(evlist);
+		evlist__set_tracking_event(evlist, tracking_evsel);
+
+		tracking_evsel->core.attr.freq = 0;
+		tracking_evsel->core.attr.sample_period = 1;
+
+		/* In per-cpu case, always need the time of mmap events etc */
+		if (!perf_cpu_map__empty(cpus))
+			evsel__set_sample_bit(tracking_evsel, TIME);
+	}
 
 	return 0;
 }
-- 
2.25.1

[PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[Leo Yan]

The dummy event is mainly used for mmap, the TIME sample is only needed
for per-cpu case so that the perf tool can rely on the correct timing
for parsing symbols.  And the CPU sample is useless for mmap.

This patch enables TIME sample for per-cpu mmap and doesn't enable CPU
sample.  For later extension (e.g. support multiple AUX events), it sets
the dummy event when the condition "opts->full_auxtrace" is true.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index 902e73a64184..f6eec0900604 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	struct evsel *evsel, *arm_spe_evsel = NULL;
 	struct perf_cpu_map *cpus = evlist->core.cpus;
 	bool privileged = perf_event_paranoid_check(-1);
-	struct evsel *tracking_evsel;
 	int err;
 
 	sper->evlist = evlist;
@@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 		evsel__set_sample_bit(arm_spe_evsel, CPU);
 
 	/* Add dummy event to keep tracking */
-	err = parse_events(evlist, "dummy:u", NULL);
-	if (err)
-		return err;
-
-	tracking_evsel = evlist__last(evlist);
-	evlist__set_tracking_event(evlist, tracking_evsel);
-
-	tracking_evsel->core.attr.freq = 0;
-	tracking_evsel->core.attr.sample_period = 1;
-	evsel__set_sample_bit(tracking_evsel, TIME);
-	evsel__set_sample_bit(tracking_evsel, CPU);
-	evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK);
+	if (opts->full_auxtrace) {
+		struct evsel *tracking_evsel;
+
+		err = parse_events(evlist, "dummy:u", NULL);
+		if (err)
+			return err;
+
+		tracking_evsel = evlist__last(evlist);
+		evlist__set_tracking_event(evlist, tracking_evsel);
+
+		tracking_evsel->core.attr.freq = 0;
+		tracking_evsel->core.attr.sample_period = 1;
+
+		/* In per-cpu case, always need the time of mmap events etc */
+		if (!perf_cpu_map__empty(cpus))
+			evsel__set_sample_bit(tracking_evsel, TIME);
+	}
 
 	return 0;
 }
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[James Clark]

On 29/04/2021 18:00, Leo Yan wrote:
> The dummy event is mainly used for mmap, the TIME sample is only needed
> for per-cpu case so that the perf tool can rely on the correct timing
> for parsing symbols.  And the CPU sample is useless for mmap.
> 
> This patch enables TIME sample for per-cpu mmap and doesn't enable CPU
> sample.  For later extension (e.g. support multiple AUX events), it sets
> the dummy event when the condition "opts->full_auxtrace" is true.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
> index 902e73a64184..f6eec0900604 100644
> --- a/tools/perf/arch/arm64/util/arm-spe.c
> +++ b/tools/perf/arch/arm64/util/arm-spe.c
> @@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
>  	struct evsel *evsel, *arm_spe_evsel = NULL;
>  	struct perf_cpu_map *cpus = evlist->core.cpus;
>  	bool privileged = perf_event_paranoid_check(-1);
> -	struct evsel *tracking_evsel;
>  	int err;
>  
>  	sper->evlist = evlist;
> @@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
>  		evsel__set_sample_bit(arm_spe_evsel, CPU);
>  
>  	/* Add dummy event to keep tracking */
> -	err = parse_events(evlist, "dummy:u", NULL);
> -	if (err)
> -		return err;
> -
> -	tracking_evsel = evlist__last(evlist);
> -	evlist__set_tracking_event(evlist, tracking_evsel);
> -
> -	tracking_evsel->core.attr.freq = 0;
> -	tracking_evsel->core.attr.sample_period = 1;
> -	evsel__set_sample_bit(tracking_evsel, TIME);
> -	evsel__set_sample_bit(tracking_evsel, CPU);
> -	evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK);
> +	if (opts->full_auxtrace) {
> +		struct evsel *tracking_evsel;

Hi Leo,

I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace
files, but I don't think it does anything because there is this at the top
of the function:

   	if (!opts->full_auxtrace)
		return 0;

The same applies for other usages of "full_auxtrace" in the same function.
They are all always true. I'm also not sure if it's ever defined what
full_auxtrace means.

James

> +
> +		err = parse_events(evlist, "dummy:u", NULL);
> +		if (err)
> +			return err;
> +
> +		tracking_evsel = evlist__last(evlist);
> +		evlist__set_tracking_event(evlist, tracking_evsel);
> +
> +		tracking_evsel->core.attr.freq = 0;
> +		tracking_evsel->core.attr.sample_period = 1;
> +
> +		/* In per-cpu case, always need the time of mmap events etc */
> +		if (!perf_cpu_map__empty(cpus))
> +			evsel__set_sample_bit(tracking_evsel, TIME);
> +	}
>  
>  	return 0;
>  }
> 

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[James Clark]

On 29/04/2021 18:00, Leo Yan wrote:
> The dummy event is mainly used for mmap, the TIME sample is only needed
> for per-cpu case so that the perf tool can rely on the correct timing
> for parsing symbols.  And the CPU sample is useless for mmap.
> 
> This patch enables TIME sample for per-cpu mmap and doesn't enable CPU
> sample.  For later extension (e.g. support multiple AUX events), it sets
> the dummy event when the condition "opts->full_auxtrace" is true.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
> index 902e73a64184..f6eec0900604 100644
> --- a/tools/perf/arch/arm64/util/arm-spe.c
> +++ b/tools/perf/arch/arm64/util/arm-spe.c
> @@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
>  	struct evsel *evsel, *arm_spe_evsel = NULL;
>  	struct perf_cpu_map *cpus = evlist->core.cpus;
>  	bool privileged = perf_event_paranoid_check(-1);
> -	struct evsel *tracking_evsel;
>  	int err;
>  
>  	sper->evlist = evlist;
> @@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
>  		evsel__set_sample_bit(arm_spe_evsel, CPU);
>  
>  	/* Add dummy event to keep tracking */
> -	err = parse_events(evlist, "dummy:u", NULL);
> -	if (err)
> -		return err;
> -
> -	tracking_evsel = evlist__last(evlist);
> -	evlist__set_tracking_event(evlist, tracking_evsel);
> -
> -	tracking_evsel->core.attr.freq = 0;
> -	tracking_evsel->core.attr.sample_period = 1;
> -	evsel__set_sample_bit(tracking_evsel, TIME);
> -	evsel__set_sample_bit(tracking_evsel, CPU);
> -	evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK);
> +	if (opts->full_auxtrace) {
> +		struct evsel *tracking_evsel;

Hi Leo,

I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace
files, but I don't think it does anything because there is this at the top
of the function:

   	if (!opts->full_auxtrace)
		return 0;

The same applies for other usages of "full_auxtrace" in the same function.
They are all always true. I'm also not sure if it's ever defined what
full_auxtrace means.

James

> +
> +		err = parse_events(evlist, "dummy:u", NULL);
> +		if (err)
> +			return err;
> +
> +		tracking_evsel = evlist__last(evlist);
> +		evlist__set_tracking_event(evlist, tracking_evsel);
> +
> +		tracking_evsel->core.attr.freq = 0;
> +		tracking_evsel->core.attr.sample_period = 1;
> +
> +		/* In per-cpu case, always need the time of mmap events etc */
> +		if (!perf_cpu_map__empty(cpus))
> +			evsel__set_sample_bit(tracking_evsel, TIME);
> +	}
>  
>  	return 0;
>  }
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[James Clark]

On 12/05/2021 17:39, James Clark wrote:
> 
> 
> On 29/04/2021 18:00, Leo Yan wrote:
>> The dummy event is mainly used for mmap, the TIME sample is only needed
[...]
>> -	tracking_evsel->core.attr.freq = 0;
>> -	tracking_evsel->core.attr.sample_period = 1;
>> -	evsel__set_sample_bit(tracking_evsel, TIME);
>> -	evsel__set_sample_bit(tracking_evsel, CPU);
>> -	evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK);
>> +	if (opts->full_auxtrace) {
>> +		struct evsel *tracking_evsel;
> 
> Hi Leo,
> 
> I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace
> files, but I don't think it does anything because there is this at the top
> of the function:
> 
>    	if (!opts->full_auxtrace)
> 		return 0;
> 
> The same applies for other usages of "full_auxtrace" in the same function.
> They are all always true. I'm also not sure if it's ever defined what
> full_auxtrace means.
> 
> James

Apart from this issue above, I've tested the full patchset with various combinations
of --per-thread, -a and --timestamp and don't see any issues like missing command names
or decode issues. (Apart from -a where Command is reported as '-1', but this issue is
present before and after this patchset so is unrelated.)

I think it makes sense to unify the behaviour to make it more like Coresight and others
so this is a good change.

Reviewed-by: James Clark <james.clark@arm.com>
Tested-by: James Clark <james.clark@arm.com>

> 
>> +
>> +		err = parse_events(evlist, "dummy:u", NULL);
>> +		if (err)
>> +			return err;
>> +
>> +		tracking_evsel = evlist__last(evlist);
>> +		evlist__set_tracking_event(evlist, tracking_evsel);
>> +
>> +		tracking_evsel->core.attr.freq = 0;
>> +		tracking_evsel->core.attr.sample_period = 1;
>> +
>> +		/* In per-cpu case, always need the time of mmap events etc */
>> +		if (!perf_cpu_map__empty(cpus))
>> +			evsel__set_sample_bit(tracking_evsel, TIME);
>> +	}
>>  
>>  	return 0;
>>  }
>>

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[James Clark]

On 12/05/2021 17:39, James Clark wrote:
> 
> 
> On 29/04/2021 18:00, Leo Yan wrote:
>> The dummy event is mainly used for mmap, the TIME sample is only needed
[...]
>> -	tracking_evsel->core.attr.freq = 0;
>> -	tracking_evsel->core.attr.sample_period = 1;
>> -	evsel__set_sample_bit(tracking_evsel, TIME);
>> -	evsel__set_sample_bit(tracking_evsel, CPU);
>> -	evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK);
>> +	if (opts->full_auxtrace) {
>> +		struct evsel *tracking_evsel;
> 
> Hi Leo,
> 
> I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace
> files, but I don't think it does anything because there is this at the top
> of the function:
> 
>    	if (!opts->full_auxtrace)
> 		return 0;
> 
> The same applies for other usages of "full_auxtrace" in the same function.
> They are all always true. I'm also not sure if it's ever defined what
> full_auxtrace means.
> 
> James

Apart from this issue above, I've tested the full patchset with various combinations
of --per-thread, -a and --timestamp and don't see any issues like missing command names
or decode issues. (Apart from -a where Command is reported as '-1', but this issue is
present before and after this patchset so is unrelated.)

I think it makes sense to unify the behaviour to make it more like Coresight and others
so this is a good change.

Reviewed-by: James Clark <james.clark@arm.com>
Tested-by: James Clark <james.clark@arm.com>

> 
>> +
>> +		err = parse_events(evlist, "dummy:u", NULL);
>> +		if (err)
>> +			return err;
>> +
>> +		tracking_evsel = evlist__last(evlist);
>> +		evlist__set_tracking_event(evlist, tracking_evsel);
>> +
>> +		tracking_evsel->core.attr.freq = 0;
>> +		tracking_evsel->core.attr.sample_period = 1;
>> +
>> +		/* In per-cpu case, always need the time of mmap events etc */
>> +		if (!perf_cpu_map__empty(cpus))
>> +			evsel__set_sample_bit(tracking_evsel, TIME);
>> +	}
>>  
>>  	return 0;
>>  }
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[Leo Yan]

Hi James,

On Wed, May 12, 2021 at 06:17:03PM +0300, James Clark wrote:

[...]

> Apart from this issue above, I've tested the full patchset with various combinations
> of --per-thread, -a and --timestamp and don't see any issues like missing command names
> or decode issues. (Apart from -a where Command is reported as '-1', but this issue is
> present before and after this patchset so is unrelated.)

Good finding!  Will take a look for the failure issue with option '-a'.

> I think it makes sense to unify the behaviour to make it more like Coresight and others
> so this is a good change.
> 
> Reviewed-by: James Clark <james.clark@arm.com>
> Tested-by: James Clark <james.clark@arm.com>

Thanks a lot for the testing,

Leo

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[Leo Yan]

Hi James,

On Wed, May 12, 2021 at 06:17:03PM +0300, James Clark wrote:

[...]

> Apart from this issue above, I've tested the full patchset with various combinations
> of --per-thread, -a and --timestamp and don't see any issues like missing command names
> or decode issues. (Apart from -a where Command is reported as '-1', but this issue is
> present before and after this patchset so is unrelated.)

Good finding!  Will take a look for the failure issue with option '-a'.

> I think it makes sense to unify the behaviour to make it more like Coresight and others
> so this is a good change.
> 
> Reviewed-by: James Clark <james.clark@arm.com>
> Tested-by: James Clark <james.clark@arm.com>

Thanks a lot for the testing,

Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[Leo Yan]

On Wed, May 12, 2021 at 05:39:56PM +0300, James Clark wrote:
> 
> 
> On 29/04/2021 18:00, Leo Yan wrote:
> > The dummy event is mainly used for mmap, the TIME sample is only needed
> > for per-cpu case so that the perf tool can rely on the correct timing
> > for parsing symbols.  And the CPU sample is useless for mmap.
> > 
> > This patch enables TIME sample for per-cpu mmap and doesn't enable CPU
> > sample.  For later extension (e.g. support multiple AUX events), it sets
> > the dummy event when the condition "opts->full_auxtrace" is true.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 13 deletions(-)
> > 
> > diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
> > index 902e73a64184..f6eec0900604 100644
> > --- a/tools/perf/arch/arm64/util/arm-spe.c
> > +++ b/tools/perf/arch/arm64/util/arm-spe.c
> > @@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
> >  	struct evsel *evsel, *arm_spe_evsel = NULL;
> >  	struct perf_cpu_map *cpus = evlist->core.cpus;
> >  	bool privileged = perf_event_paranoid_check(-1);
> > -	struct evsel *tracking_evsel;
> >  	int err;
> >  
> >  	sper->evlist = evlist;
> > @@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
> >  		evsel__set_sample_bit(arm_spe_evsel, CPU);
> >  
> >  	/* Add dummy event to keep tracking */
> > -	err = parse_events(evlist, "dummy:u", NULL);
> > -	if (err)
> > -		return err;
> > -
> > -	tracking_evsel = evlist__last(evlist);
> > -	evlist__set_tracking_event(evlist, tracking_evsel);
> > -
> > -	tracking_evsel->core.attr.freq = 0;
> > -	tracking_evsel->core.attr.sample_period = 1;
> > -	evsel__set_sample_bit(tracking_evsel, TIME);
> > -	evsel__set_sample_bit(tracking_evsel, CPU);
> > -	evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK);
> > +	if (opts->full_auxtrace) {
> > +		struct evsel *tracking_evsel;
> 
> Hi Leo,
> 
> I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace
> files, but I don't think it does anything because there is this at the top
> of the function:
> 
>    	if (!opts->full_auxtrace)
> 		return 0;
> 
> The same applies for other usages of "full_auxtrace" in the same function.
> They are all always true. I'm also not sure if it's ever defined what
> full_auxtrace means.

Good pointing.  TBH, I also stuggled for handling "opts->full_auxtrace"
when wrote the patch; IIUC, "opts->full_auxtrace" is also used in
builtin-record.c to indicate if the recording contains AUX tracing.

Will follow your suggestion to respin the patch (and refine the code)
to remove the redundant condition checking for "opts->full_auxtrace".

Thanks,
Leo

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[Leo Yan]

On Wed, May 12, 2021 at 05:39:56PM +0300, James Clark wrote:
> 
> 
> On 29/04/2021 18:00, Leo Yan wrote:
> > The dummy event is mainly used for mmap, the TIME sample is only needed
> > for per-cpu case so that the perf tool can rely on the correct timing
> > for parsing symbols.  And the CPU sample is useless for mmap.
> > 
> > This patch enables TIME sample for per-cpu mmap and doesn't enable CPU
> > sample.  For later extension (e.g. support multiple AUX events), it sets
> > the dummy event when the condition "opts->full_auxtrace" is true.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 13 deletions(-)
> > 
> > diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
> > index 902e73a64184..f6eec0900604 100644
> > --- a/tools/perf/arch/arm64/util/arm-spe.c
> > +++ b/tools/perf/arch/arm64/util/arm-spe.c
> > @@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
> >  	struct evsel *evsel, *arm_spe_evsel = NULL;
> >  	struct perf_cpu_map *cpus = evlist->core.cpus;
> >  	bool privileged = perf_event_paranoid_check(-1);
> > -	struct evsel *tracking_evsel;
> >  	int err;
> >  
> >  	sper->evlist = evlist;
> > @@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
> >  		evsel__set_sample_bit(arm_spe_evsel, CPU);
> >  
> >  	/* Add dummy event to keep tracking */
> > -	err = parse_events(evlist, "dummy:u", NULL);
> > -	if (err)
> > -		return err;
> > -
> > -	tracking_evsel = evlist__last(evlist);
> > -	evlist__set_tracking_event(evlist, tracking_evsel);
> > -
> > -	tracking_evsel->core.attr.freq = 0;
> > -	tracking_evsel->core.attr.sample_period = 1;
> > -	evsel__set_sample_bit(tracking_evsel, TIME);
> > -	evsel__set_sample_bit(tracking_evsel, CPU);
> > -	evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK);
> > +	if (opts->full_auxtrace) {
> > +		struct evsel *tracking_evsel;
> 
> Hi Leo,
> 
> I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace
> files, but I don't think it does anything because there is this at the top
> of the function:
> 
>    	if (!opts->full_auxtrace)
> 		return 0;
> 
> The same applies for other usages of "full_auxtrace" in the same function.
> They are all always true. I'm also not sure if it's ever defined what
> full_auxtrace means.

Good pointing.  TBH, I also stuggled for handling "opts->full_auxtrace"
when wrote the patch; IIUC, "opts->full_auxtrace" is also used in
builtin-record.c to indicate if the recording contains AUX tracing.

Will follow your suggestion to respin the patch (and refine the code)
to remove the redundant condition checking for "opts->full_auxtrace".

Thanks,
Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[Arnaldo Carvalho de Melo]

Em Wed, May 12, 2021 at 11:23:30PM +0800, Leo Yan escreveu:
> On Wed, May 12, 2021 at 05:39:56PM +0300, James Clark wrote:
> > 
> > 
> > On 29/04/2021 18:00, Leo Yan wrote:
> > > The dummy event is mainly used for mmap, the TIME sample is only needed
> > > for per-cpu case so that the perf tool can rely on the correct timing
> > > for parsing symbols.  And the CPU sample is useless for mmap.
> > > 
> > > This patch enables TIME sample for per-cpu mmap and doesn't enable CPU
> > > sample.  For later extension (e.g. support multiple AUX events), it sets
> > > the dummy event when the condition "opts->full_auxtrace" is true.
> > > 
> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > ---
> > >  tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------
> > >  1 file changed, 17 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
> > > index 902e73a64184..f6eec0900604 100644
> > > --- a/tools/perf/arch/arm64/util/arm-spe.c
> > > +++ b/tools/perf/arch/arm64/util/arm-spe.c
> > > @@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
> > >  	struct evsel *evsel, *arm_spe_evsel = NULL;
> > >  	struct perf_cpu_map *cpus = evlist->core.cpus;
> > >  	bool privileged = perf_event_paranoid_check(-1);
> > > -	struct evsel *tracking_evsel;
> > >  	int err;
> > >  
> > >  	sper->evlist = evlist;
> > > @@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
> > >  		evsel__set_sample_bit(arm_spe_evsel, CPU);
> > >  
> > >  	/* Add dummy event to keep tracking */
> > > -	err = parse_events(evlist, "dummy:u", NULL);
> > > -	if (err)
> > > -		return err;
> > > -
> > > -	tracking_evsel = evlist__last(evlist);
> > > -	evlist__set_tracking_event(evlist, tracking_evsel);
> > > -
> > > -	tracking_evsel->core.attr.freq = 0;
> > > -	tracking_evsel->core.attr.sample_period = 1;
> > > -	evsel__set_sample_bit(tracking_evsel, TIME);
> > > -	evsel__set_sample_bit(tracking_evsel, CPU);
> > > -	evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK);
> > > +	if (opts->full_auxtrace) {
> > > +		struct evsel *tracking_evsel;
> > 
> > Hi Leo,
> > 
> > I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace
> > files, but I don't think it does anything because there is this at the top
> > of the function:
> > 
> >    	if (!opts->full_auxtrace)
> > 		return 0;
> > 
> > The same applies for other usages of "full_auxtrace" in the same function.
> > They are all always true. I'm also not sure if it's ever defined what
> > full_auxtrace means.
> 
> Good pointing.  TBH, I also stuggled for handling "opts->full_auxtrace"
> when wrote the patch; IIUC, "opts->full_auxtrace" is also used in
> builtin-record.c to indicate if the recording contains AUX tracing.
> 
> Will follow your suggestion to respin the patch (and refine the code)
> to remove the redundant condition checking for "opts->full_auxtrace".

Ok, so please collect his Tested-by and Reviewed-by and I'll wait for v2
then,

- Arnaldo

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[Arnaldo Carvalho de Melo]

Em Wed, May 12, 2021 at 11:23:30PM +0800, Leo Yan escreveu:
> On Wed, May 12, 2021 at 05:39:56PM +0300, James Clark wrote:
> > 
> > 
> > On 29/04/2021 18:00, Leo Yan wrote:
> > > The dummy event is mainly used for mmap, the TIME sample is only needed
> > > for per-cpu case so that the perf tool can rely on the correct timing
> > > for parsing symbols.  And the CPU sample is useless for mmap.
> > > 
> > > This patch enables TIME sample for per-cpu mmap and doesn't enable CPU
> > > sample.  For later extension (e.g. support multiple AUX events), it sets
> > > the dummy event when the condition "opts->full_auxtrace" is true.
> > > 
> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > ---
> > >  tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------
> > >  1 file changed, 17 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
> > > index 902e73a64184..f6eec0900604 100644
> > > --- a/tools/perf/arch/arm64/util/arm-spe.c
> > > +++ b/tools/perf/arch/arm64/util/arm-spe.c
> > > @@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
> > >  	struct evsel *evsel, *arm_spe_evsel = NULL;
> > >  	struct perf_cpu_map *cpus = evlist->core.cpus;
> > >  	bool privileged = perf_event_paranoid_check(-1);
> > > -	struct evsel *tracking_evsel;
> > >  	int err;
> > >  
> > >  	sper->evlist = evlist;
> > > @@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
> > >  		evsel__set_sample_bit(arm_spe_evsel, CPU);
> > >  
> > >  	/* Add dummy event to keep tracking */
> > > -	err = parse_events(evlist, "dummy:u", NULL);
> > > -	if (err)
> > > -		return err;
> > > -
> > > -	tracking_evsel = evlist__last(evlist);
> > > -	evlist__set_tracking_event(evlist, tracking_evsel);
> > > -
> > > -	tracking_evsel->core.attr.freq = 0;
> > > -	tracking_evsel->core.attr.sample_period = 1;
> > > -	evsel__set_sample_bit(tracking_evsel, TIME);
> > > -	evsel__set_sample_bit(tracking_evsel, CPU);
> > > -	evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK);
> > > +	if (opts->full_auxtrace) {
> > > +		struct evsel *tracking_evsel;
> > 
> > Hi Leo,
> > 
> > I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace
> > files, but I don't think it does anything because there is this at the top
> > of the function:
> > 
> >    	if (!opts->full_auxtrace)
> > 		return 0;
> > 
> > The same applies for other usages of "full_auxtrace" in the same function.
> > They are all always true. I'm also not sure if it's ever defined what
> > full_auxtrace means.
> 
> Good pointing.  TBH, I also stuggled for handling "opts->full_auxtrace"
> when wrote the patch; IIUC, "opts->full_auxtrace" is also used in
> builtin-record.c to indicate if the recording contains AUX tracing.
> 
> Will follow your suggestion to respin the patch (and refine the code)
> to remove the redundant condition checking for "opts->full_auxtrace".

Ok, so please collect his Tested-by and Reviewed-by and I'll wait for v2
then,

- Arnaldo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[Leo Yan]

On Tue, May 18, 2021 at 09:54:02AM -0300, Arnaldo Carvalho de Melo wrote:

[...]

> > Will follow your suggestion to respin the patch (and refine the code)
> > to remove the redundant condition checking for "opts->full_auxtrace".
> 
> Ok, so please collect his Tested-by and Reviewed-by and I'll wait for v2
> then,

Sure, will send the patches in tomorrow, sorry for some delay.

And thanks for reminding!

Re: [PATCH v1 2/3] perf arm-spe: Correct sample flags for dummy event

[Leo Yan]

On Tue, May 18, 2021 at 09:54:02AM -0300, Arnaldo Carvalho de Melo wrote:

[...]

> > Will follow your suggestion to respin the patch (and refine the code)
> > to remove the redundant condition checking for "opts->full_auxtrace".
> 
> Ok, so please collect his Tested-by and Reviewed-by and I'll wait for v2
> then,

Sure, will send the patches in tomorrow, sorry for some delay.

And thanks for reminding!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/3] perf arm-spe: Enable timestamp for per-cpu mode

[Leo Yan]

For per-cpu mmap, it should enable timestamp tracing for Arm SPE; this
is helpful for samples correlation.

To automatically enable the timestamp, a helper arm_spe_set_timestamp()
is introduced for setting "ts_enable" format bit.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/arch/arm64/util/arm-spe.c | 33 ++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index f6eec0900604..dae115dd458e 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -14,6 +14,7 @@
 #include "../../../util/cpumap.h"
 #include "../../../util/event.h"
 #include "../../../util/evsel.h"
+#include "../../../util/evsel_config.h"
 #include "../../../util/evlist.h"
 #include "../../../util/session.h"
 #include <internal/lib.h> // page_size
@@ -32,6 +33,29 @@ struct arm_spe_recording {
 	struct evlist		*evlist;
 };
 
+static void arm_spe_set_timestamp(struct auxtrace_record *itr,
+				  struct evsel *evsel)
+{
+	struct arm_spe_recording *ptr;
+	struct perf_pmu *arm_spe_pmu;
+	struct evsel_config_term *term = evsel__get_config_term(evsel, CFG_CHG);
+	u64 user_bits = 0, bit;
+
+	ptr = container_of(itr, struct arm_spe_recording, itr);
+	arm_spe_pmu = ptr->arm_spe_pmu;
+
+	if (term)
+		user_bits = term->val.cfg_chg;
+
+	bit = perf_pmu__format_bits(&arm_spe_pmu->format, "ts_enable");
+
+	/* Skip if user has set it */
+	if (bit & user_bits)
+		return;
+
+	evsel->core.attr.config |= bit;
+}
+
 static size_t
 arm_spe_info_priv_size(struct auxtrace_record *itr __maybe_unused,
 		       struct evlist *evlist __maybe_unused)
@@ -120,9 +144,14 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	 */
 	evlist__to_front(evlist, arm_spe_evsel);
 
-	/* In the case of per-cpu mmaps, sample CPU for AUX event. */
-	if (!perf_cpu_map__empty(cpus))
+	/*
+	 * In the case of per-cpu mmaps, sample CPU for AUX event;
+	 * also enable the timestamp tracing for samples correlation.
+	 */
+	if (!perf_cpu_map__empty(cpus)) {
 		evsel__set_sample_bit(arm_spe_evsel, CPU);
+		arm_spe_set_timestamp(itr, arm_spe_evsel);
+	}
 
 	/* Add dummy event to keep tracking */
 	if (opts->full_auxtrace) {
-- 
2.25.1

[PATCH v1 3/3] perf arm-spe: Enable timestamp for per-cpu mode

[Leo Yan]

For per-cpu mmap, it should enable timestamp tracing for Arm SPE; this
is helpful for samples correlation.

To automatically enable the timestamp, a helper arm_spe_set_timestamp()
is introduced for setting "ts_enable" format bit.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/arch/arm64/util/arm-spe.c | 33 ++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index f6eec0900604..dae115dd458e 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -14,6 +14,7 @@
 #include "../../../util/cpumap.h"
 #include "../../../util/event.h"
 #include "../../../util/evsel.h"
+#include "../../../util/evsel_config.h"
 #include "../../../util/evlist.h"
 #include "../../../util/session.h"
 #include <internal/lib.h> // page_size
@@ -32,6 +33,29 @@ struct arm_spe_recording {
 	struct evlist		*evlist;
 };
 
+static void arm_spe_set_timestamp(struct auxtrace_record *itr,
+				  struct evsel *evsel)
+{
+	struct arm_spe_recording *ptr;
+	struct perf_pmu *arm_spe_pmu;
+	struct evsel_config_term *term = evsel__get_config_term(evsel, CFG_CHG);
+	u64 user_bits = 0, bit;
+
+	ptr = container_of(itr, struct arm_spe_recording, itr);
+	arm_spe_pmu = ptr->arm_spe_pmu;
+
+	if (term)
+		user_bits = term->val.cfg_chg;
+
+	bit = perf_pmu__format_bits(&arm_spe_pmu->format, "ts_enable");
+
+	/* Skip if user has set it */
+	if (bit & user_bits)
+		return;
+
+	evsel->core.attr.config |= bit;
+}
+
 static size_t
 arm_spe_info_priv_size(struct auxtrace_record *itr __maybe_unused,
 		       struct evlist *evlist __maybe_unused)
@@ -120,9 +144,14 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	 */
 	evlist__to_front(evlist, arm_spe_evsel);
 
-	/* In the case of per-cpu mmaps, sample CPU for AUX event. */
-	if (!perf_cpu_map__empty(cpus))
+	/*
+	 * In the case of per-cpu mmaps, sample CPU for AUX event;
+	 * also enable the timestamp tracing for samples correlation.
+	 */
+	if (!perf_cpu_map__empty(cpus)) {
 		evsel__set_sample_bit(arm_spe_evsel, CPU);
+		arm_spe_set_timestamp(itr, arm_spe_evsel);
+	}
 
 	/* Add dummy event to keep tracking */
 	if (opts->full_auxtrace) {
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel