[PATCH v1 0/6] perf auxtrace: Support multiple AUX events

[PATCH v1 0/6] perf auxtrace: Support multiple AUX events

[Leo Yan]

This series is to support multiple events with the *same* type in Perf
AUX trace. As the first enabled instance, the patch series enables
multiple Arm SPE events (e.g. arm_spe_0, arm_spe_1, etc) in AUX trace.

The solution for support multiple AUX events with same type is not
difficult. As the events are same type, the trace data shares the same
format and can be decoded by the same decoder. Essentially, this patch
series is to extend the AUX trace flow from support single PMU event to
multiple events.

Note, this series does not support a more complex case - different types
of AUX events, (e.g. Arm CoreSight event and Arm SPE events are enabled
simultaneously).

Patch 01 is a minor refactoring for dereference PMU pointer from evsel
structure.

Patches 02, 03 and 04 are to use the 'auxtrace' flag for support multiple
AUX events. Firstly, we need to set the 'auxtrace' flag for Arm and s390
AUX events (Intel PT and bts have set already it). Afterwards, by using
the evsel__is_aux_event() function, the core layer iterates the whole
evlist - which allows the buffer index can be matched to corresponding
AUX event.

Patches 05 and 06 are to configure multiple SPE event in architecture
dependent code. The old code is only initialize the first AUX event.
with this series, it initializes all SPE PMU events.

This patch series has been tested with the normal 'perf record' command
and 'perf mem record' command.  And verified for the decoding commands
'perf script' and 'perf mem report'.

I observed one prominent issue is for per-CPU profiling. For example,
when specifying option '-C 2' for profiling on CPU2, in this case the
'arm_spe_0' event supports CPU2 but the 'arm_spe_1' event does not
support the CPU. As a result, 'arm_spe_1' event reports failure. This
is likely a common issue for support Per-CPU profiling with multiple
PMU events and every PMU event only support partial CPUs. This issue
will be addressed later.


Leo Yan (6):
  perf pmu: Directly use evsel's PMU pointer
  perf auxtrace arm: Set the 'auxtrace' flag for AUX events
  perf auxtrace s390: Set the 'auxtrace' flag for AUX events
  perf auxtrace: Iterate all AUX events when finish reading
  perf arm-spe: Extract evsel setting up
  perf arm-spe: Support multiple Arm SPE events

 tools/perf/arch/arm/util/pmu.c       |   3 +
 tools/perf/arch/arm64/util/arm-spe.c | 107 ++++++++++++++++-----------
 tools/perf/arch/s390/util/auxtrace.c |   1 +
 tools/perf/util/auxtrace.c           |  15 +++-
 tools/perf/util/pmu.c                |   2 +-
 5 files changed, 78 insertions(+), 50 deletions(-)

-- 
2.34.1

[PATCH v1 1/6] perf pmu: Directly use evsel's PMU pointer

[Leo Yan]

Rather than iterating the whole PMU list for finding the associated PMU
device for an evsel, this commit optimizes to directly use evsel's 'pmu'
pointer for accessing PMU device.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/util/pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 986166bc7c78..798cd5a2ebc4 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1199,7 +1199,7 @@ void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
 
 bool evsel__is_aux_event(const struct evsel *evsel)
 {
-	struct perf_pmu *pmu = evsel__find_pmu(evsel);
+	struct perf_pmu *pmu = evsel->pmu;
 
 	return pmu && pmu->auxtrace;
 }
-- 
2.34.1

[PATCH v1 2/6] perf auxtrace arm: Set the 'auxtrace' flag for AUX events

[Leo Yan]

Originally, the 'auxtrace' flag in the PMU event is used for AUX area
sampling. It indicates a PMU event is for AUX tracing.

Set this flag for AUX trace events on Arm.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/arch/arm/util/pmu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/arch/arm/util/pmu.c b/tools/perf/arch/arm/util/pmu.c
index 1c9541d01722..b7fa1245e242 100644
--- a/tools/perf/arch/arm/util/pmu.c
+++ b/tools/perf/arch/arm/util/pmu.c
@@ -25,6 +25,7 @@ void perf_pmu__arch_init(struct perf_pmu *pmu)
 		/* add ETM default config here */
 		pmu->selectable = true;
 		pmu->perf_event_attr_init_default = cs_etm_get_default_config;
+		pmu->auxtrace = true;
 #if defined(__aarch64__)
 	} else if (strstarts(pmu->name, ARM_SPE_PMU_NAME)) {
 		pmu->selectable = true;
@@ -32,8 +33,10 @@ void perf_pmu__arch_init(struct perf_pmu *pmu)
 		pmu->perf_event_attr_init_default = arm_spe_pmu_default_config;
 		if (strstarts(pmu->name, "arm_spe_"))
 			pmu->mem_events = perf_mem_events_arm;
+		pmu->auxtrace = true;
 	} else if (strstarts(pmu->name, HISI_PTT_PMU_NAME)) {
 		pmu->selectable = true;
+		pmu->auxtrace = true;
 #endif
 	}
 #endif
-- 
2.34.1

[PATCH v1 3/6] perf auxtrace s390: Set the 'auxtrace' flag for AUX events

[Leo Yan]

Set the 'auxtrace' flag for AUX events on s390.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/arch/s390/util/auxtrace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/arch/s390/util/auxtrace.c b/tools/perf/arch/s390/util/auxtrace.c
index 5068baa3e092..d7e72413b262 100644
--- a/tools/perf/arch/s390/util/auxtrace.c
+++ b/tools/perf/arch/s390/util/auxtrace.c
@@ -99,6 +99,7 @@ struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
 		if (pos->core.attr.config == PERF_EVENT_CPUM_SF_DIAG) {
 			diagnose = 1;
 			pos->needs_auxtrace_mmap = true;
+			pos->pmu->auxtrace = true;
 			break;
 		}
 	}
-- 
2.34.1

[PATCH v1 4/6] perf auxtrace: Iterate all AUX events when finish reading

[Leo Yan]

When finished to read AUX trace data from mmaped buffer, based on the
AUX buffer index the core layer needs to search the corresponding PMU
event and re-enable it to continue tracing.

However, current code only searches the first AUX event. It misses to
search other enabled AUX events, thus, it returns failure if the buffer
index does not belong to the first AUX event.

This patch extends the auxtrace_record__read_finish() function to
search for every enabled AUX events, so all the mmaped buffer indexes
can be covered.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/util/auxtrace.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index e2f317063eec..95be330d7e10 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -670,18 +670,25 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel,
 int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx)
 {
 	struct evsel *evsel;
+	int ret = -EINVAL;
 
 	if (!itr->evlist || !itr->pmu)
 		return -EINVAL;
 
 	evlist__for_each_entry(itr->evlist, evsel) {
-		if (evsel->core.attr.type == itr->pmu->type) {
+		if (evsel__is_aux_event(evsel)) {
 			if (evsel->disabled)
-				return 0;
-			return evlist__enable_event_idx(itr->evlist, evsel, idx);
+				continue;
+			ret = evlist__enable_event_idx(itr->evlist, evsel, idx);
+			if (ret >= 0)
+				return ret;
 		}
 	}
-	return -EINVAL;
+
+	if (ret < 0)
+		pr_err("Failed to event enable event (idx=%d): %d\n", idx, ret);
+
+	return ret;
 }
 
 /*
-- 
2.34.1

[PATCH v1 5/6] perf arm-spe: Extract evsel setting up

[Leo Yan]

The evsel for Arm SPE PMU needs to be set up. Extract the setting up
into a function arm_spe_setup_evsel().

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/arch/arm64/util/arm-spe.c | 74 +++++++++++++++-------------
 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index 0b52e67edb3b..fe7942824113 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -132,6 +132,45 @@ static __u64 arm_spe_pmu__sample_period(const struct perf_pmu *arm_spe_pmu)
 	return sample_period;
 }
 
+static void arm_spe_setup_evsel(struct evsel *evsel, struct perf_cpu_map *cpus)
+{
+	u64 bit;
+
+	evsel->core.attr.freq = 0;
+	evsel->core.attr.sample_period = arm_spe_pmu__sample_period(evsel->pmu);
+	evsel->needs_auxtrace_mmap = true;
+
+	/*
+	 * To obtain the auxtrace buffer file descriptor, the auxtrace event
+	 * must come first.
+	 */
+	evlist__to_front(evsel->evlist, evsel);
+
+	/*
+	 * In the case of per-cpu mmaps, sample CPU for AUX event;
+	 * also enable the timestamp tracing for samples correlation.
+	 */
+	if (!perf_cpu_map__is_any_cpu_or_is_empty(cpus)) {
+		evsel__set_sample_bit(evsel, CPU);
+		evsel__set_config_if_unset(evsel->pmu, evsel, "ts_enable", 1);
+	}
+
+	/*
+	 * Set this only so that perf report knows that SPE generates memory info. It has no effect
+	 * on the opening of the event or the SPE data produced.
+	 */
+	evsel__set_sample_bit(evsel, DATA_SRC);
+
+	/*
+	 * The PHYS_ADDR flag does not affect the driver behaviour, it is used to
+	 * inform that the resulting output's SPE samples contain physical addresses
+	 * where applicable.
+	 */
+	bit = perf_pmu__format_bits(evsel->pmu, "pa_enable");
+	if (evsel->core.attr.config & bit)
+		evsel__set_sample_bit(evsel, PHYS_ADDR);
+}
+
 static int arm_spe_recording_options(struct auxtrace_record *itr,
 				     struct evlist *evlist,
 				     struct record_opts *opts)
@@ -144,7 +183,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	bool privileged = perf_event_paranoid_check(-1);
 	struct evsel *tracking_evsel;
 	int err;
-	u64 bit;
 
 	sper->evlist = evlist;
 
@@ -154,9 +192,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 				pr_err("There may be only one " ARM_SPE_PMU_NAME "x event\n");
 				return -EINVAL;
 			}
-			evsel->core.attr.freq = 0;
-			evsel->core.attr.sample_period = arm_spe_pmu__sample_period(arm_spe_pmu);
-			evsel->needs_auxtrace_mmap = true;
 			arm_spe_evsel = evsel;
 			opts->full_auxtrace = true;
 		}
@@ -222,36 +257,7 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 		pr_debug2("%sx snapshot size: %zu\n", ARM_SPE_PMU_NAME,
 			  opts->auxtrace_snapshot_size);
 
-	/*
-	 * To obtain the auxtrace buffer file descriptor, the auxtrace event
-	 * must come first.
-	 */
-	evlist__to_front(evlist, arm_spe_evsel);
-
-	/*
-	 * In the case of per-cpu mmaps, sample CPU for AUX event;
-	 * also enable the timestamp tracing for samples correlation.
-	 */
-	if (!perf_cpu_map__is_any_cpu_or_is_empty(cpus)) {
-		evsel__set_sample_bit(arm_spe_evsel, CPU);
-		evsel__set_config_if_unset(arm_spe_pmu, arm_spe_evsel,
-					   "ts_enable", 1);
-	}
-
-	/*
-	 * Set this only so that perf report knows that SPE generates memory info. It has no effect
-	 * on the opening of the event or the SPE data produced.
-	 */
-	evsel__set_sample_bit(arm_spe_evsel, DATA_SRC);
-
-	/*
-	 * The PHYS_ADDR flag does not affect the driver behaviour, it is used to
-	 * inform that the resulting output's SPE samples contain physical addresses
-	 * where applicable.
-	 */
-	bit = perf_pmu__format_bits(arm_spe_pmu, "pa_enable");
-	if (arm_spe_evsel->core.attr.config & bit)
-		evsel__set_sample_bit(arm_spe_evsel, PHYS_ADDR);
+	arm_spe_setup_evsel(arm_spe_evsel, cpus);
 
 	/* Add dummy event to keep tracking */
 	err = parse_event(evlist, "dummy:u");
-- 
2.34.1

[PATCH v1 6/6] perf arm-spe: Support multiple Arm SPE events

[Leo Yan]

As the flag 'auxtrace' has been set for Arm SPE events, now it is ready
to use evsel__is_aux_event() to check if an event is AUX trace event or
not. Use this function to replace the old checking for only the first
Arm SPE event.

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

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index fe7942824113..d59f6ca499f2 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -8,6 +8,7 @@
 #include <linux/types.h>
 #include <linux/bitops.h>
 #include <linux/log2.h>
+#include <linux/string.h>
 #include <linux/zalloc.h>
 #include <time.h>
 
@@ -177,8 +178,7 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 {
 	struct arm_spe_recording *sper =
 			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 evsel *evsel, *tmp;
 	struct perf_cpu_map *cpus = evlist->core.user_requested_cpus;
 	bool privileged = perf_event_paranoid_check(-1);
 	struct evsel *tracking_evsel;
@@ -187,12 +187,12 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	sper->evlist = evlist;
 
 	evlist__for_each_entry(evlist, evsel) {
-		if (evsel->core.attr.type == arm_spe_pmu->type) {
-			if (arm_spe_evsel) {
-				pr_err("There may be only one " ARM_SPE_PMU_NAME "x event\n");
+		if (evsel__is_aux_event(evsel)) {
+			if (!strstarts(evsel->pmu_name, ARM_SPE_PMU_NAME)) {
+				pr_err("Found unexpected auxtrace event: %s\n",
+				       evsel->pmu_name);
 				return -EINVAL;
 			}
-			arm_spe_evsel = evsel;
 			opts->full_auxtrace = true;
 		}
 	}
@@ -257,7 +257,10 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 		pr_debug2("%sx snapshot size: %zu\n", ARM_SPE_PMU_NAME,
 			  opts->auxtrace_snapshot_size);
 
-	arm_spe_setup_evsel(arm_spe_evsel, cpus);
+	evlist__for_each_entry_safe(evlist, tmp, evsel) {
+		if (evsel__is_aux_event(evsel))
+			arm_spe_setup_evsel(evsel, cpus);
+	}
 
 	/* Add dummy event to keep tracking */
 	err = parse_event(evlist, "dummy:u");
@@ -307,12 +310,16 @@ static int arm_spe_snapshot_start(struct auxtrace_record *itr)
 	struct arm_spe_recording *ptr =
 			container_of(itr, struct arm_spe_recording, itr);
 	struct evsel *evsel;
+	int ret = -EINVAL;
 
 	evlist__for_each_entry(ptr->evlist, evsel) {
-		if (evsel->core.attr.type == ptr->arm_spe_pmu->type)
-			return evsel__disable(evsel);
+		if (evsel__is_aux_event(evsel)) {
+			ret = evsel__disable(evsel);
+			if (ret < 0)
+				return ret;
+		}
 	}
-	return -EINVAL;
+	return ret;
 }
 
 static int arm_spe_snapshot_finish(struct auxtrace_record *itr)
@@ -320,12 +327,16 @@ static int arm_spe_snapshot_finish(struct auxtrace_record *itr)
 	struct arm_spe_recording *ptr =
 			container_of(itr, struct arm_spe_recording, itr);
 	struct evsel *evsel;
+	int ret = -EINVAL;
 
 	evlist__for_each_entry(ptr->evlist, evsel) {
-		if (evsel->core.attr.type == ptr->arm_spe_pmu->type)
-			return evsel__enable(evsel);
+		if (evsel__is_aux_event(evsel)) {
+			ret = evsel__enable(evsel);
+			if (ret < 0)
+				return ret;
+		}
 	}
-	return -EINVAL;
+	return ret;
 }
 
 static int arm_spe_alloc_wrapped_array(struct arm_spe_recording *ptr, int idx)
-- 
2.34.1

Re: [PATCH v1 1/6] perf pmu: Directly use evsel's PMU pointer

[Adrian Hunter]

On 21/07/24 23:21, Leo Yan wrote:
> Rather than iterating the whole PMU list for finding the associated PMU
> device for an evsel, this commit optimizes to directly use evsel's 'pmu'
> pointer for accessing PMU device.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/util/pmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 986166bc7c78..798cd5a2ebc4 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1199,7 +1199,7 @@ void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
>  
>  bool evsel__is_aux_event(const struct evsel *evsel)
>  {
> -	struct perf_pmu *pmu = evsel__find_pmu(evsel);
> +	struct perf_pmu *pmu = evsel->pmu;

Assumes event parser has populated evsel->pmu for auxtrace events.
Could use a comment to that effect.

>  
>  	return pmu && pmu->auxtrace;
>  }

Re: [PATCH v1 2/6] perf auxtrace arm: Set the 'auxtrace' flag for AUX events

[Adrian Hunter]

On 21/07/24 23:21, Leo Yan wrote:
> Originally, the 'auxtrace' flag in the PMU event is used for AUX area
> sampling. It indicates a PMU event is for AUX tracing.
> 
> Set this flag for AUX trace events on Arm.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>

Note same as:

https://lore.kernel.org/all/20240715160712.127117-6-adrian.hunter@intel.com/

Either should be fine:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  tools/perf/arch/arm/util/pmu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/arch/arm/util/pmu.c b/tools/perf/arch/arm/util/pmu.c
> index 1c9541d01722..b7fa1245e242 100644
> --- a/tools/perf/arch/arm/util/pmu.c
> +++ b/tools/perf/arch/arm/util/pmu.c
> @@ -25,6 +25,7 @@ void perf_pmu__arch_init(struct perf_pmu *pmu)
>  		/* add ETM default config here */
>  		pmu->selectable = true;
>  		pmu->perf_event_attr_init_default = cs_etm_get_default_config;
> +		pmu->auxtrace = true;
>  #if defined(__aarch64__)
>  	} else if (strstarts(pmu->name, ARM_SPE_PMU_NAME)) {
>  		pmu->selectable = true;
> @@ -32,8 +33,10 @@ void perf_pmu__arch_init(struct perf_pmu *pmu)
>  		pmu->perf_event_attr_init_default = arm_spe_pmu_default_config;
>  		if (strstarts(pmu->name, "arm_spe_"))
>  			pmu->mem_events = perf_mem_events_arm;
> +		pmu->auxtrace = true;
>  	} else if (strstarts(pmu->name, HISI_PTT_PMU_NAME)) {
>  		pmu->selectable = true;
> +		pmu->auxtrace = true;
>  #endif
>  	}
>  #endif

Re: [PATCH v1 3/6] perf auxtrace s390: Set the 'auxtrace' flag for AUX events

[Adrian Hunter]

On 21/07/24 23:21, Leo Yan wrote:
> Set the 'auxtrace' flag for AUX events on s390.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/arch/s390/util/auxtrace.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/perf/arch/s390/util/auxtrace.c b/tools/perf/arch/s390/util/auxtrace.c
> index 5068baa3e092..d7e72413b262 100644
> --- a/tools/perf/arch/s390/util/auxtrace.c
> +++ b/tools/perf/arch/s390/util/auxtrace.c
> @@ -99,6 +99,7 @@ struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
>  		if (pos->core.attr.config == PERF_EVENT_CPUM_SF_DIAG) {
>  			diagnose = 1;
>  			pos->needs_auxtrace_mmap = true;
> +			pos->pmu->auxtrace = true;

This is probably too late. See:

https://lore.kernel.org/all/20240715160712.127117-7-adrian.hunter@intel.com/

>  			break;
>  		}
>  	}

Re: [PATCH v1 4/6] perf auxtrace: Iterate all AUX events when finish reading

[Adrian Hunter]

On 21/07/24 23:21, Leo Yan wrote:
> When finished to read AUX trace data from mmaped buffer, based on the
> AUX buffer index the core layer needs to search the corresponding PMU
> event and re-enable it to continue tracing.
> 
> However, current code only searches the first AUX event. It misses to
> search other enabled AUX events, thus, it returns failure if the buffer
> index does not belong to the first AUX event.
> 
> This patch extends the auxtrace_record__read_finish() function to
> search for every enabled AUX events, so all the mmaped buffer indexes
> can be covered.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/util/auxtrace.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index e2f317063eec..95be330d7e10 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -670,18 +670,25 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel,
>  int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx)
>  {
>  	struct evsel *evsel;
> +	int ret = -EINVAL;
>  
>  	if (!itr->evlist || !itr->pmu)
>  		return -EINVAL;
>  
>  	evlist__for_each_entry(itr->evlist, evsel) {
> -		if (evsel->core.attr.type == itr->pmu->type) {
> +		if (evsel__is_aux_event(evsel)) {

If the type is the same, then there is no need to
change the logic here?

Otherwise, maybe that should be a separate patch

>  			if (evsel->disabled)
> -				return 0;
> -			return evlist__enable_event_idx(itr->evlist, evsel, idx);
> +				continue;
> +			ret = evlist__enable_event_idx(itr->evlist, evsel, idx);
> +			if (ret >= 0)

Should this be:

			if (ret < 0)

> +				return ret;

And will need a common error path for the pr_err() below.

>  		}
>  	}
> -	return -EINVAL;
> +
> +	if (ret < 0)
> +		pr_err("Failed to event enable event (idx=%d): %d\n", idx, ret);
> +
> +	return ret;
>  }
>  
>  /*

Re: [PATCH v1 1/6] perf pmu: Directly use evsel's PMU pointer

[Leo Yan]

Hi Adrian,

On 7/22/24 11:40, Adrian Hunter wrote:

[...]

> On 21/07/24 23:21, Leo Yan wrote:
>> Rather than iterating the whole PMU list for finding the associated PMU
>> device for an evsel, this commit optimizes to directly use evsel's 'pmu'
>> pointer for accessing PMU device.
>>
>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>> ---
>>   tools/perf/util/pmu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 986166bc7c78..798cd5a2ebc4 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -1199,7 +1199,7 @@ void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
>>
>>   bool evsel__is_aux_event(const struct evsel *evsel)
>>   {
>> -     struct perf_pmu *pmu = evsel__find_pmu(evsel);
>> +     struct perf_pmu *pmu = evsel->pmu;
> 
> Assumes event parser has populated evsel->pmu for auxtrace events.
> Could use a comment to that effect.

Sure, will add a comment for this.

Thanks,
Leo

>>
>>        return pmu && pmu->auxtrace;
>>   }
> 

Re: [PATCH v1 2/6] perf auxtrace arm: Set the 'auxtrace' flag for AUX events

[Leo Yan]

On 7/22/24 11:49, Adrian Hunter wrote:
> On 21/07/24 23:21, Leo Yan wrote:
>> Originally, the 'auxtrace' flag in the PMU event is used for AUX area
>> sampling. It indicates a PMU event is for AUX tracing.
>>
>> Set this flag for AUX trace events on Arm.
>>
>> Signed-off-by: Leo Yan <leo.yan@arm.com>
> 
> Note same as:
> 
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20240715160712.127117-6-adrian.hunter%40intel.com%2F&data=05%7C02%7Cleo.yan%40arm.com%7C909e738ccfd84b1ad1dc08dcaa3bf922%7Cf34e597957d94aaaad4db122a662184d%7C0%7C0%7C638572421778104310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=rJq9SLjbFOrZRkWIGLj8Xj682h%2BW%2FD8O0IL3ac0UcO4%3D&reserved=0
> 
> Either should be fine:
> 
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Thank you for the patch, I will drop my one.

Leo
>> ---
>>   tools/perf/arch/arm/util/pmu.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/tools/perf/arch/arm/util/pmu.c b/tools/perf/arch/arm/util/pmu.c
>> index 1c9541d01722..b7fa1245e242 100644
>> --- a/tools/perf/arch/arm/util/pmu.c
>> +++ b/tools/perf/arch/arm/util/pmu.c
>> @@ -25,6 +25,7 @@ void perf_pmu__arch_init(struct perf_pmu *pmu)
>>                /* add ETM default config here */
>>                pmu->selectable = true;
>>                pmu->perf_event_attr_init_default = cs_etm_get_default_config;
>> +             pmu->auxtrace = true;
>>   #if defined(__aarch64__)
>>        } else if (strstarts(pmu->name, ARM_SPE_PMU_NAME)) {
>>                pmu->selectable = true;
>> @@ -32,8 +33,10 @@ void perf_pmu__arch_init(struct perf_pmu *pmu)
>>                pmu->perf_event_attr_init_default = arm_spe_pmu_default_config;
>>                if (strstarts(pmu->name, "arm_spe_"))
>>                        pmu->mem_events = perf_mem_events_arm;
>> +             pmu->auxtrace = true;
>>        } else if (strstarts(pmu->name, HISI_PTT_PMU_NAME)) {
>>                pmu->selectable = true;
>> +             pmu->auxtrace = true;
>>   #endif
>>        }
>>   #endif
> 

Re: [PATCH v1 3/6] perf auxtrace s390: Set the 'auxtrace' flag for AUX events

[Leo Yan]

On 7/22/24 11:54, Adrian Hunter wrote:

[...]

> On 21/07/24 23:21, Leo Yan wrote:
>> Set the 'auxtrace' flag for AUX events on s390.
>>
>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>> ---
>>   tools/perf/arch/s390/util/auxtrace.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/perf/arch/s390/util/auxtrace.c b/tools/perf/arch/s390/util/auxtrace.c
>> index 5068baa3e092..d7e72413b262 100644
>> --- a/tools/perf/arch/s390/util/auxtrace.c
>> +++ b/tools/perf/arch/s390/util/auxtrace.c
>> @@ -99,6 +99,7 @@ struct auxtrace_record *auxtrace_record__init(struct evlist *evlist,
>>                if (pos->core.attr.config == PERF_EVENT_CPUM_SF_DIAG) {
>>                        diagnose = 1;
>>                        pos->needs_auxtrace_mmap = true;
>> +                     pos->pmu->auxtrace = true;
> 
> This is probably too late. See:
> 
> https://lore.kernel.org/all/20240715160712.127117-7-adrian.hunter@intel.com/

Okay, I will drop this patch.

Thanks,
Leo


> 
>>                        break;
>>                }
>>        }
> 

Re: [PATCH v1 4/6] perf auxtrace: Iterate all AUX events when finish reading

[Leo Yan]

On 7/22/24 12:13, Adrian Hunter wrote:

[...]

> On 21/07/24 23:21, Leo Yan wrote:
>> When finished to read AUX trace data from mmaped buffer, based on the
>> AUX buffer index the core layer needs to search the corresponding PMU
>> event and re-enable it to continue tracing.
>>
>> However, current code only searches the first AUX event. It misses to
>> search other enabled AUX events, thus, it returns failure if the buffer
>> index does not belong to the first AUX event.
>>
>> This patch extends the auxtrace_record__read_finish() function to
>> search for every enabled AUX events, so all the mmaped buffer indexes
>> can be covered.
>>
>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>> ---
>>   tools/perf/util/auxtrace.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>> index e2f317063eec..95be330d7e10 100644
>> --- a/tools/perf/util/auxtrace.c
>> +++ b/tools/perf/util/auxtrace.c
>> @@ -670,18 +670,25 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel,
>>   int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx)
>>   {
>>        struct evsel *evsel;
>> +     int ret = -EINVAL;
>>
>>        if (!itr->evlist || !itr->pmu)
>>                return -EINVAL;
>>
>>        evlist__for_each_entry(itr->evlist, evsel) {
>> -             if (evsel->core.attr.type == itr->pmu->type) {
>> +             if (evsel__is_aux_event(evsel)) {
> 
> If the type is the same, then there is no need to
> change the logic here?

No, the type is not same for AUX events. Every event has its own type
value, this is likely related to recent refactoring.

As a result, 'itr->pmu' only maintains the first registered AUX event,
comparing to it the tool will find _only_ one AUX event. This is why here
changes to use the evsel__is_aux_event() to detect AUX event.

> Otherwise, maybe that should be a separate patch

Could you explain what is a separate patch for?

After this change, the field 'itr->pmu' will be redundant (at least this
is the case for Arm SPE). I am preparing a refactoring patches for cleaning up
and see if can totally remove the field 'itr->pmu' (if all AUX events
have no issue.

> 
>>                        if (evsel->disabled)
>> -                             return 0;
>> -                     return evlist__enable_event_idx(itr->evlist, evsel, idx);
>> +                             continue;
>> +                     ret = evlist__enable_event_idx(itr->evlist, evsel, idx);
>> +                     if (ret >= 0)
> 
> Should this be:
> 
>                          if (ret < 0)

Here the logic is to iterate all AUX events, even if an AUX event fails to
find the buffer index, it will continue to next AUX event.

So it directly bails out for success (as we have found the matched AUX
event and enabled it). For the failure cause, it will continue for checking
next event - until all events have been checked and no event is matched
for buffer index, the failure will be handled at the end of the function.

Thanks,
Leo

> 
>> +                             return ret;
> 
> And will need a common error path for the pr_err() below.
> 
>>                }
>>        }
>> -     return -EINVAL;
>> +
>> +     if (ret < 0)
>> +             pr_err("Failed to event enable event (idx=%d): %d\n", idx, ret);
>> +
>> +     return ret;
>>   }
>>
>>   /*
> 

Re: [PATCH v1 4/6] perf auxtrace: Iterate all AUX events when finish reading

[Adrian Hunter]

On 22/07/24 18:09, Leo Yan wrote:
> On 7/22/24 12:13, Adrian Hunter wrote:
> 
> [...]
> 
>> On 21/07/24 23:21, Leo Yan wrote:
>>> When finished to read AUX trace data from mmaped buffer, based on the
>>> AUX buffer index the core layer needs to search the corresponding PMU
>>> event and re-enable it to continue tracing.
>>>
>>> However, current code only searches the first AUX event. It misses to
>>> search other enabled AUX events, thus, it returns failure if the buffer
>>> index does not belong to the first AUX event.
>>>
>>> This patch extends the auxtrace_record__read_finish() function to
>>> search for every enabled AUX events, so all the mmaped buffer indexes
>>> can be covered.
>>>
>>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>>> ---
>>>   tools/perf/util/auxtrace.c | 15 +++++++++++----
>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>>> index e2f317063eec..95be330d7e10 100644
>>> --- a/tools/perf/util/auxtrace.c
>>> +++ b/tools/perf/util/auxtrace.c
>>> @@ -670,18 +670,25 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel,
>>>   int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx)
>>>   {
>>>        struct evsel *evsel;
>>> +     int ret = -EINVAL;
>>>
>>>        if (!itr->evlist || !itr->pmu)
>>>                return -EINVAL;
>>>
>>>        evlist__for_each_entry(itr->evlist, evsel) {
>>> -             if (evsel->core.attr.type == itr->pmu->type) {
>>> +             if (evsel__is_aux_event(evsel)) {
>>
>> If the type is the same, then there is no need to
>> change the logic here?
> 
> No, the type is not same for AUX events. Every event has its own type
> value, this is likely related to recent refactoring.
> 
> As a result, 'itr->pmu' only maintains the first registered AUX event,
> comparing to it the tool will find _only_ one AUX event. This is why here
> changes to use the evsel__is_aux_event() to detect AUX event.
> 
>> Otherwise, maybe that should be a separate patch
> 
> Could you explain what is a separate patch for?

No need.

> 
> After this change, the field 'itr->pmu' will be redundant (at least this
> is the case for Arm SPE). I am preparing a refactoring patches for cleaning up
> and see if can totally remove the field 'itr->pmu' (if all AUX events
> have no issue.

For this function, 'itr->pmu' could be removed in this patch
since it is not used anymore.

> 
>>
>>>                        if (evsel->disabled)
>>> -                             return 0;
>>> -                     return evlist__enable_event_idx(itr->evlist, evsel, idx);
>>> +                             continue;
>>> +                     ret = evlist__enable_event_idx(itr->evlist, evsel, idx);
>>> +                     if (ret >= 0)
>>
>> Should this be:
>>
>>                          if (ret < 0)
> 
> Here the logic is to iterate all AUX events, even if an AUX event fails to
> find the buffer index, it will continue to next AUX event.
> 
> So it directly bails out for success (as we have found the matched AUX
> event and enabled it). For the failure cause, it will continue for checking
> next event - until all events have been checked and no event is matched
> for buffer index, the failure will be handled at the end of the function.

Thanks for the explanation. Could probably use a small comment.

> 
> Thanks,
> Leo
> 
>>
>>> +                             return ret;
>>
>> And will need a common error path for the pr_err() below.
>>
>>>                }
>>>        }
>>> -     return -EINVAL;
>>> +
>>> +     if (ret < 0)
>>> +             pr_err("Failed to event enable event (idx=%d): %d\n", idx, ret);
>>> +
>>> +     return ret;
>>>   }
>>>
>>>   /*
>>

Re: [PATCH v1 1/6] perf pmu: Directly use evsel's PMU pointer

[Ian Rogers]

On Sun, Jul 21, 2024 at 1:21 PM Leo Yan <leo.yan@arm.com> wrote:
>
> Rather than iterating the whole PMU list for finding the associated PMU
> device for an evsel, this commit optimizes to directly use evsel's 'pmu'
> pointer for accessing PMU device.

The code doesn't do that:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmus.c?h=perf-tools-next#n698
```
struct perf_pmu *evsel__find_pmu(const struct evsel *evsel)
{
       struct perf_pmu *pmu = evsel->pmu;

       if (!pmu) {
               pmu = perf_pmus__find_by_type(evsel->core.attr.type);
               ((struct evsel *)evsel)->pmu = pmu;
       }
       return pmu;
}
```
That is, if the evsel->pmu is not NULL then just return it, otherwise
find the pmu using the type from the attribute. Any linear such should
happen at most once unless the pmu is NULL from event parsing or
perf_pmus__find_by_type.  The PMU may be NULL for legacy events and if
sysfs isn't mounted. If you are encountering that then maybe we need a
flag to say don't find the PMU for this evsel as it is known NULL.

Thanks,
Ian

> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/util/pmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 986166bc7c78..798cd5a2ebc4 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1199,7 +1199,7 @@ void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
>
>  bool evsel__is_aux_event(const struct evsel *evsel)
>  {
> -       struct perf_pmu *pmu = evsel__find_pmu(evsel);
> +       struct perf_pmu *pmu = evsel->pmu;
>
>         return pmu && pmu->auxtrace;
>  }
> --
> 2.34.1
>

Re: [PATCH v1 4/6] perf auxtrace: Iterate all AUX events when finish reading

[Leo Yan]

On 7/22/2024 4:59 PM, Adrian Hunter wrote:

[...]

>>>> @@ -670,18 +670,25 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel,
>>>>   int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx)
>>>>   {
>>>>        struct evsel *evsel;
>>>> +     int ret = -EINVAL;
>>>>
>>>>        if (!itr->evlist || !itr->pmu)
>>>>                return -EINVAL;
>>>>
>>>>        evlist__for_each_entry(itr->evlist, evsel) {
>>>> -             if (evsel->core.attr.type == itr->pmu->type) {
>>>> +             if (evsel__is_aux_event(evsel)) {
>>>
>>> If the type is the same, then there is no need to
>>> change the logic here?
>>
>> No, the type is not same for AUX events. Every event has its own type
>> value, this is likely related to recent refactoring.
>>
>> As a result, 'itr->pmu' only maintains the first registered AUX event,
>> comparing to it the tool will find _only_ one AUX event. This is why here
>> changes to use the evsel__is_aux_event() to detect AUX event.
>>
>>> Otherwise, maybe that should be a separate patch
>>
>> Could you explain what is a separate patch for?
> 
> No need.
> 
>>
>> After this change, the field 'itr->pmu' will be redundant (at least this
>> is the case for Arm SPE). I am preparing a refactoring patches for cleaning up
>> and see if can totally remove the field 'itr->pmu' (if all AUX events
>> have no issue.
> 
> For this function, 'itr->pmu' could be removed in this patch
> since it is not used anymore.

Thanks for confirmation. I will use a separate patch for removing 'itr-pmu'
after it is not used anymore.

>>>>                        if (evsel->disabled)
>>>> -                             return 0;
>>>> -                     return evlist__enable_event_idx(itr->evlist, evsel, idx);
>>>> +                             continue;
>>>> +                     ret = evlist__enable_event_idx(itr->evlist, evsel, idx);
>>>> +                     if (ret >= 0)
>>>
>>> Should this be:
>>>
>>>                          if (ret < 0)
>>
>> Here the logic is to iterate all AUX events, even if an AUX event fails to
>> find the buffer index, it will continue to next AUX event.
>>
>> So it directly bails out for success (as we have found the matched AUX
>> event and enabled it). For the failure cause, it will continue for checking
>> next event - until all events have been checked and no event is matched
>> for buffer index, the failure will be handled at the end of the function.
> 
> Thanks for the explanation. Could probably use a small comment.

Will do.

Thanks,
Leo

Re: [PATCH v1 1/6] perf pmu: Directly use evsel's PMU pointer

[Leo Yan]

On 7/22/2024 5:16 PM, Ian Rogers wrote:
> On Sun, Jul 21, 2024 at 1:21 PM Leo Yan <leo.yan@arm.com> wrote:
>>
>> Rather than iterating the whole PMU list for finding the associated PMU
>> device for an evsel, this commit optimizes to directly use evsel's 'pmu'
>> pointer for accessing PMU device.
> 
> The code doesn't do that:
> ```
> struct perf_pmu *evsel__find_pmu(const struct evsel *evsel)
> {
>        struct perf_pmu *pmu = evsel->pmu;
> 
>        if (!pmu) {
>                pmu = perf_pmus__find_by_type(evsel->core.attr.type);
>                ((struct evsel *)evsel)->pmu = pmu;
>        }
>        return pmu;
> }
> ```
> That is, if the evsel->pmu is not NULL then just return it, otherwise
> find the pmu using the type from the attribute. Any linear such should
> happen at most once unless the pmu is NULL from event parsing or
> perf_pmus__find_by_type.

So evsel__find_pmu() is good enough.

> The PMU may be NULL for legacy events and if
> sysfs isn't mounted. If you are encountering that then maybe we need a
> flag to say don't find the PMU for this evsel as it is known NULL.

I don't see a case of the PMU pointer is NULL. So don't need this flag.

My bad for misreading the code :\  Thanks a lot for pointing out.

Leo

Re: [PATCH v1 4/6] perf auxtrace: Iterate all AUX events when finish reading

[Ian Rogers]

Just a heads up. Arnaldo added this to tmp.perf-tools-next and it
caused the intel-pt tests to start failing:
```
$ perf test 118 -v
118: Miscellaneous Intel PT testing:
--- start ---
test child forked, pid 148999
--- Test system-wide sideband ---
Checking for CPU-wide recording on CPU 0
OK
Checking for CPU-wide recording on CPU 1
OK
Linux
Failed to enable event (idx=0): -22
Failed to record MMAP events on CPU 1 when tracing CPU 0
...
```
It's likely Adrian's comments already address this but you may also
want to double check this test is passing with v2.

Thanks,
Ian

Re: [PATCH v1 4/6] perf auxtrace: Iterate all AUX events when finish reading

[Arnaldo Carvalho de Melo]

On Wed, Jul 31, 2024 at 06:38:59PM -0700, Ian Rogers wrote:
> Just a heads up. Arnaldo added this to tmp.perf-tools-next and it
> caused the intel-pt tests to start failing:

My plan right now is just to remove that cset that Ian bisected since it
is not on perf-tools-next, just on the scratch branch
tmp.perf-tools-next.

Trying to do that now as it will help us with bisection in the future.

- Arnaldo

> ```
> $ perf test 118 -v
> 118: Miscellaneous Intel PT testing:
> --- start ---
> test child forked, pid 148999
> --- Test system-wide sideband ---
> Checking for CPU-wide recording on CPU 0
> OK
> Checking for CPU-wide recording on CPU 1
> OK
> Linux
> Failed to enable event (idx=0): -22
> Failed to record MMAP events on CPU 1 when tracing CPU 0
> ...
> ```
> It's likely Adrian's comments already address this but you may also
> want to double check this test is passing with v2.
> 
> Thanks,
> Ian

Re: [PATCH v1 4/6] perf auxtrace: Iterate all AUX events when finish reading

[Leo Yan]

On 8/1/24 16:04, Arnaldo Carvalho de Melo wrote:
> On Wed, Jul 31, 2024 at 06:38:59PM -0700, Ian Rogers wrote:
>> Just a heads up. Arnaldo added this to tmp.perf-tools-next and it
>> caused the intel-pt tests to start failing:

Sorry for causing regression and thanks for reporting the issue.

> My plan right now is just to remove that cset that Ian bisected since it
> is not on perf-tools-next, just on the scratch branch
> tmp.perf-tools-next.

Please leave the patch 04 out. I have sent out v2 but Adrian pointed out a 
concern for per-thread mode, now I am working on this and after ready I will 
send new patches.

I have verified the latest perf-tools-next branch, it works well on Arm SPE 
(thanks for picking up patches 05 and 06).

> Trying to do that now as it will help us with bisection in the future.

I am not clear this. Could you elaborate a bit what I should follow up?

>> ```
>> $ perf test 118 -v
>> 118: Miscellaneous Intel PT testing:
>> --- start ---
>> test child forked, pid 148999
>> --- Test system-wide sideband ---
>> Checking for CPU-wide recording on CPU 0
>> OK
>> Checking for CPU-wide recording on CPU 1
>> OK
>> Linux
>> Failed to enable event (idx=0): -22
>> Failed to record MMAP events on CPU 1 when tracing CPU 0
>> ...
>> ```
>> It's likely Adrian's comments already address this but you may also
>> want to double check this test is passing with v2.

Sure. I will give a test for this test when I send new patches.

Thanks,
Leo