[PATCH v2 0/4] perf auxtrace: Support multiple AUX events

[PATCH v2 0/4] 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 are to use the 'auxtrace' flag for support multiple AUX events.
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.

Patch 02 is a cleanup for removing unused 'pmu' pointer in the
auxtrace_record structure.

Patches 03 and 04 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.

Changes from v1:
- Added comment in patch 01 for iterating AUX events (Adrian)
- Added patch 02 for removing unused field 'pmu' (Adrian)


Leo Yan (4):
  perf auxtrace: Iterate all AUX events when finish reading
  perf auxtrace: Remove unused 'pmu' pointer from struct auxtrace_record
  perf arm-spe: Extract evsel setting up
  perf arm-spe: Support multiple Arm SPE events

 tools/perf/arch/arm/util/cs-etm.c     |   1 -
 tools/perf/arch/arm64/util/arm-spe.c  | 108 +++++++++++++++-----------
 tools/perf/arch/arm64/util/hisi-ptt.c |   1 -
 tools/perf/arch/x86/util/intel-bts.c  |   1 -
 tools/perf/arch/x86/util/intel-pt.c   |   1 -
 tools/perf/util/auxtrace.c            |  25 ++++--
 tools/perf/util/auxtrace.h            |   1 -
 7 files changed, 82 insertions(+), 56 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] 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 | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index b99e72f7da88..61835a6a9ea3 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -670,18 +670,33 @@ 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)
+	if (!itr->evlist)
 		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;
+			/*
+			 * Multiple AUX events might be opened in a session.
+			 * Bail out for success case as the AUX event has been
+			 * found and enabled, otherwise, continue to check if
+			 * the next AUX event can cover the mmaped buffer with
+			 * 'idx'.
+			 */
+			ret = evlist__enable_event_idx(itr->evlist, evsel, idx);
+			if (ret >= 0)
+				return ret;
 		}
 	}
-	return -EINVAL;
+
+	/* Failed to find an event for the buffer 'idx' */
+	if (ret < 0)
+		pr_err("Failed to enable event (idx=%d): %d\n", idx, ret);
+
+	return ret;
 }
 
 /*
-- 
2.34.1

[PATCH v2 2/4] perf auxtrace: Remove unused 'pmu' pointer from struct auxtrace_record

[Leo Yan]

The 'pmu' pointer in the auxtrace_record structure is not used after
support multiple AUX events, remove it.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/arch/arm/util/cs-etm.c     | 1 -
 tools/perf/arch/arm64/util/arm-spe.c  | 1 -
 tools/perf/arch/arm64/util/hisi-ptt.c | 1 -
 tools/perf/arch/x86/util/intel-bts.c  | 1 -
 tools/perf/arch/x86/util/intel-pt.c   | 1 -
 tools/perf/util/auxtrace.h            | 1 -
 6 files changed, 6 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index da6231367993..96aeb7cdbee1 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -888,7 +888,6 @@ struct auxtrace_record *cs_etm_record_init(int *err)
 	}
 
 	ptr->cs_etm_pmu			= cs_etm_pmu;
-	ptr->itr.pmu			= cs_etm_pmu;
 	ptr->itr.parse_snapshot_options	= cs_etm_parse_snapshot_options;
 	ptr->itr.recording_options	= cs_etm_recording_options;
 	ptr->itr.info_priv_size		= cs_etm_info_priv_size;
diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index 0b52e67edb3b..622fd47573ec 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -497,7 +497,6 @@ struct auxtrace_record *arm_spe_recording_init(int *err,
 	}
 
 	sper->arm_spe_pmu = arm_spe_pmu;
-	sper->itr.pmu = arm_spe_pmu;
 	sper->itr.snapshot_start = arm_spe_snapshot_start;
 	sper->itr.snapshot_finish = arm_spe_snapshot_finish;
 	sper->itr.find_snapshot = arm_spe_find_snapshot;
diff --git a/tools/perf/arch/arm64/util/hisi-ptt.c b/tools/perf/arch/arm64/util/hisi-ptt.c
index ba97c8a562a0..eac9739c87e6 100644
--- a/tools/perf/arch/arm64/util/hisi-ptt.c
+++ b/tools/perf/arch/arm64/util/hisi-ptt.c
@@ -174,7 +174,6 @@ struct auxtrace_record *hisi_ptt_recording_init(int *err,
 	}
 
 	pttr->hisi_ptt_pmu = hisi_ptt_pmu;
-	pttr->itr.pmu = hisi_ptt_pmu;
 	pttr->itr.recording_options = hisi_ptt_recording_options;
 	pttr->itr.info_priv_size = hisi_ptt_info_priv_size;
 	pttr->itr.info_fill = hisi_ptt_info_fill;
diff --git a/tools/perf/arch/x86/util/intel-bts.c b/tools/perf/arch/x86/util/intel-bts.c
index 34696f3d3d5d..85c8186300c8 100644
--- a/tools/perf/arch/x86/util/intel-bts.c
+++ b/tools/perf/arch/x86/util/intel-bts.c
@@ -434,7 +434,6 @@ struct auxtrace_record *intel_bts_recording_init(int *err)
 	}
 
 	btsr->intel_bts_pmu = intel_bts_pmu;
-	btsr->itr.pmu = intel_bts_pmu;
 	btsr->itr.recording_options = intel_bts_recording_options;
 	btsr->itr.info_priv_size = intel_bts_info_priv_size;
 	btsr->itr.info_fill = intel_bts_info_fill;
diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c
index 4b710e875953..ea510a7486b1 100644
--- a/tools/perf/arch/x86/util/intel-pt.c
+++ b/tools/perf/arch/x86/util/intel-pt.c
@@ -1197,7 +1197,6 @@ struct auxtrace_record *intel_pt_recording_init(int *err)
 	}
 
 	ptr->intel_pt_pmu = intel_pt_pmu;
-	ptr->itr.pmu = intel_pt_pmu;
 	ptr->itr.recording_options = intel_pt_recording_options;
 	ptr->itr.info_priv_size = intel_pt_info_priv_size;
 	ptr->itr.info_fill = intel_pt_info_fill;
diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
index f99e53426528..a54f7b99ba5a 100644
--- a/tools/perf/util/auxtrace.h
+++ b/tools/perf/util/auxtrace.h
@@ -411,7 +411,6 @@ struct auxtrace_record {
 	int (*read_finish)(struct auxtrace_record *itr, int idx);
 	unsigned int alignment;
 	unsigned int default_aux_sample_size;
-	struct perf_pmu *pmu;
 	struct evlist *evlist;
 };
 
-- 
2.34.1

[PATCH v2 3/4] 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 622fd47573ec..c228b96aa78b 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 v2 4/4] 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 c228b96aa78b..2be99fdf997d 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 v2 0/4] perf auxtrace: Support multiple AUX events

[Leo Yan]

On 7/25/2024 7:46 AM, Leo Yan wrote:
> 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.

Sorry I forgot to mention this series is dependent on
Adrian's two patches:

https://lore.kernel.org/linux-perf-users/e52c54a9-d34b-49a5-8c12-c609afcea1e6@arm.com/T/#mfe08431c03277cdef9c00fa42d86f6a42588de2a
https://lore.kernel.org/linux-perf-users/e52c54a9-d34b-49a5-8c12-c609afcea1e6@arm.com/T/#m519e5b2066333eb5f0bc3b7fd2dd89a8151e7b66

Thanks,
Leo

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

[Adrian Hunter]

On 25/07/24 09:46, Leo Yan wrote:
> When finished to read AUX trace data from mmaped buffer, based on the

Here and elsewhere 'mmapped' is more common than 'mmaped' so maybe:

	mmaped -> mmapped

> 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.

Looking at this again, a couple more things came to mind - see below.

> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/util/auxtrace.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index b99e72f7da88..61835a6a9ea3 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -670,18 +670,33 @@ 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)
> +	if (!itr->evlist)
>  		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;

That will make the evsel->disabled case an error, which
might be a problem. Possibly the event can be disabled
(e.g. via control fifo) but still have data to read out.

> +			/*
> +			 * Multiple AUX events might be opened in a session.
> +			 * Bail out for success case as the AUX event has been
> +			 * found and enabled, otherwise, continue to check if
> +			 * the next AUX event can cover the mmaped buffer with
> +			 * 'idx'.
> +			 */

Thinking about this some more, raised some questions:

How do we know there is only one AUX event per mmap?  They
would have to be on different CPUs for that to be true?
And wouldn't --per-thread have all AUX events in every mmap?

> +			ret = evlist__enable_event_idx(itr->evlist, evsel, idx);
> +			if (ret >= 0)
> +				return ret;
>  		}
>  	}
> -	return -EINVAL;
> +
> +	/* Failed to find an event for the buffer 'idx' */
> +	if (ret < 0)
> +		pr_err("Failed to enable event (idx=%d): %d\n", idx, ret);
> +
> +	return ret;
>  }
>  
>  /*

Re: [PATCH v2 2/4] perf auxtrace: Remove unused 'pmu' pointer from struct auxtrace_record

[Adrian Hunter]

On 25/07/24 09:46, Leo Yan wrote:
> The 'pmu' pointer in the auxtrace_record structure is not used after
> support multiple AUX events, remove it.

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

> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/arch/arm/util/cs-etm.c     | 1 -
>  tools/perf/arch/arm64/util/arm-spe.c  | 1 -
>  tools/perf/arch/arm64/util/hisi-ptt.c | 1 -
>  tools/perf/arch/x86/util/intel-bts.c  | 1 -
>  tools/perf/arch/x86/util/intel-pt.c   | 1 -
>  tools/perf/util/auxtrace.h            | 1 -
>  6 files changed, 6 deletions(-)
> 
> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> index da6231367993..96aeb7cdbee1 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -888,7 +888,6 @@ struct auxtrace_record *cs_etm_record_init(int *err)
>  	}
>  
>  	ptr->cs_etm_pmu			= cs_etm_pmu;
> -	ptr->itr.pmu			= cs_etm_pmu;
>  	ptr->itr.parse_snapshot_options	= cs_etm_parse_snapshot_options;
>  	ptr->itr.recording_options	= cs_etm_recording_options;
>  	ptr->itr.info_priv_size		= cs_etm_info_priv_size;
> diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
> index 0b52e67edb3b..622fd47573ec 100644
> --- a/tools/perf/arch/arm64/util/arm-spe.c
> +++ b/tools/perf/arch/arm64/util/arm-spe.c
> @@ -497,7 +497,6 @@ struct auxtrace_record *arm_spe_recording_init(int *err,
>  	}
>  
>  	sper->arm_spe_pmu = arm_spe_pmu;
> -	sper->itr.pmu = arm_spe_pmu;
>  	sper->itr.snapshot_start = arm_spe_snapshot_start;
>  	sper->itr.snapshot_finish = arm_spe_snapshot_finish;
>  	sper->itr.find_snapshot = arm_spe_find_snapshot;
> diff --git a/tools/perf/arch/arm64/util/hisi-ptt.c b/tools/perf/arch/arm64/util/hisi-ptt.c
> index ba97c8a562a0..eac9739c87e6 100644
> --- a/tools/perf/arch/arm64/util/hisi-ptt.c
> +++ b/tools/perf/arch/arm64/util/hisi-ptt.c
> @@ -174,7 +174,6 @@ struct auxtrace_record *hisi_ptt_recording_init(int *err,
>  	}
>  
>  	pttr->hisi_ptt_pmu = hisi_ptt_pmu;
> -	pttr->itr.pmu = hisi_ptt_pmu;
>  	pttr->itr.recording_options = hisi_ptt_recording_options;
>  	pttr->itr.info_priv_size = hisi_ptt_info_priv_size;
>  	pttr->itr.info_fill = hisi_ptt_info_fill;
> diff --git a/tools/perf/arch/x86/util/intel-bts.c b/tools/perf/arch/x86/util/intel-bts.c
> index 34696f3d3d5d..85c8186300c8 100644
> --- a/tools/perf/arch/x86/util/intel-bts.c
> +++ b/tools/perf/arch/x86/util/intel-bts.c
> @@ -434,7 +434,6 @@ struct auxtrace_record *intel_bts_recording_init(int *err)
>  	}
>  
>  	btsr->intel_bts_pmu = intel_bts_pmu;
> -	btsr->itr.pmu = intel_bts_pmu;
>  	btsr->itr.recording_options = intel_bts_recording_options;
>  	btsr->itr.info_priv_size = intel_bts_info_priv_size;
>  	btsr->itr.info_fill = intel_bts_info_fill;
> diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c
> index 4b710e875953..ea510a7486b1 100644
> --- a/tools/perf/arch/x86/util/intel-pt.c
> +++ b/tools/perf/arch/x86/util/intel-pt.c
> @@ -1197,7 +1197,6 @@ struct auxtrace_record *intel_pt_recording_init(int *err)
>  	}
>  
>  	ptr->intel_pt_pmu = intel_pt_pmu;
> -	ptr->itr.pmu = intel_pt_pmu;
>  	ptr->itr.recording_options = intel_pt_recording_options;
>  	ptr->itr.info_priv_size = intel_pt_info_priv_size;
>  	ptr->itr.info_fill = intel_pt_info_fill;
> diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
> index f99e53426528..a54f7b99ba5a 100644
> --- a/tools/perf/util/auxtrace.h
> +++ b/tools/perf/util/auxtrace.h
> @@ -411,7 +411,6 @@ struct auxtrace_record {
>  	int (*read_finish)(struct auxtrace_record *itr, int idx);
>  	unsigned int alignment;
>  	unsigned int default_aux_sample_size;
> -	struct perf_pmu *pmu;
>  	struct evlist *evlist;
>  };
>  

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

[Leo Yan]

On 7/25/2024 4:40 PM, Adrian Hunter wrote:
> On 25/07/24 09:46, Leo Yan wrote:
>> When finished to read AUX trace data from mmaped buffer, based on the
> 
> Here and elsewhere 'mmapped' is more common than 'mmaped' so maybe:
> 
>         mmaped -> mmapped

Will fix.

>> 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.
> 
> Looking at this again, a couple more things came to mind - see below.
> 
>>
>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>> ---
>>  tools/perf/util/auxtrace.c | 25 ++++++++++++++++++++-----
>>  1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>> index b99e72f7da88..61835a6a9ea3 100644
>> --- a/tools/perf/util/auxtrace.c
>> +++ b/tools/perf/util/auxtrace.c
>> @@ -670,18 +670,33 @@ 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)
>> +     if (!itr->evlist)
>>               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;
> 
> That will make the evsel->disabled case an error, which
> might be a problem. Possibly the event can be disabled
> (e.g. via control fifo) but still have data to read out.

If so, we need to extend evlist__enable_event_idx() or create a new function
to check if the memory index is belonged to an evsel. If the idx is found for
an evsel, then we can check the evsel->disabled flag.

>> +                     /*
>> +                      * Multiple AUX events might be opened in a session.
>> +                      * Bail out for success case as the AUX event has been
>> +                      * found and enabled, otherwise, continue to check if
>> +                      * the next AUX event can cover the mmaped buffer with
>> +                      * 'idx'.
>> +                      */
> 
> Thinking about this some more, raised some questions:
> 
> How do we know there is only one AUX event per mmap?

On my test platform, there have two Arm SPE events, the first one's cpumask is
2-5 and the second SPE's cpumask is 6-7. The AUX events do not intersect CPU
maps. Therefore, a mmapped AUX buffer only binds to a AUX event.

I think we can add a checking in the function record__auxtrace_init(). After
it calls auxtrace_record__init(), we can report failure if the AUX events have
the overlapped cpumask.

> They would have to be on different CPUs for that to be true?

Yes.

> And wouldn't --per-thread have all AUX events in every mmap?

Before I roughly tested '--per-thread' mode and did not see issue. But this
time I tested '--per-thread' mode and found the failure by the kernel checking
[1] - it does not allow the different AUX events to bind to the same FD.

Here I need to dig a bit for two options, either we need to fix the perf
tool to open multiple AUX events for '--per-thread' mode, or remove the
kernel's checking to allow different AUX events to bind to same FD.

Thanks a lot for review!

Leo

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/events/core.c#n12348

>> +                     ret = evlist__enable_event_idx(itr->evlist, evsel, idx);
>> +                     if (ret >= 0)
>> +                             return ret;
>>               }
>>       }
>> -     return -EINVAL;
>> +
>> +     /* Failed to find an event for the buffer 'idx' */
>> +     if (ret < 0)
>> +             pr_err("Failed to enable event (idx=%d): %d\n", idx, ret);
>> +
>> +     return ret;
>>  }
>>
>>  /*
> 

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

[Arnaldo Carvalho de Melo]

On Mon, Jul 29, 2024 at 09:48:57AM +0100, Leo Yan wrote:
> On 7/25/2024 4:40 PM, Adrian Hunter wrote:
> > On 25/07/24 09:46, Leo Yan wrote:
> >> When finished to read AUX trace data from mmaped buffer, based on the
> > 
> > Here and elsewhere 'mmapped' is more common than 'mmaped' so maybe:
> > 
> >         mmaped -> mmapped
> 
> Will fix.
> 
> >> 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.
> > 
> > Looking at this again, a couple more things came to mind - see below.
> > 
> >>
> >> Signed-off-by: Leo Yan <leo.yan@arm.com>
> >> ---
> >>  tools/perf/util/auxtrace.c | 25 ++++++++++++++++++++-----
> >>  1 file changed, 20 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> >> index b99e72f7da88..61835a6a9ea3 100644
> >> --- a/tools/perf/util/auxtrace.c
> >> +++ b/tools/perf/util/auxtrace.c
> >> @@ -670,18 +670,33 @@ 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)
> >> +     if (!itr->evlist)
> >>               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;
> > 
> > That will make the evsel->disabled case an error, which
> > might be a problem. Possibly the event can be disabled
> > (e.g. via control fifo) but still have data to read out.
> 
> If so, we need to extend evlist__enable_event_idx() or create a new function
> to check if the memory index is belonged to an evsel. If the idx is found for
> an evsel, then we can check the evsel->disabled flag.
> 
> >> +                     /*
> >> +                      * Multiple AUX events might be opened in a session.
> >> +                      * Bail out for success case as the AUX event has been
> >> +                      * found and enabled, otherwise, continue to check if
> >> +                      * the next AUX event can cover the mmaped buffer with
> >> +                      * 'idx'.
> >> +                      */
> > 
> > Thinking about this some more, raised some questions:
> > 
> > How do we know there is only one AUX event per mmap?
> 
> On my test platform, there have two Arm SPE events, the first one's cpumask is
> 2-5 and the second SPE's cpumask is 6-7. The AUX events do not intersect CPU
> maps. Therefore, a mmapped AUX buffer only binds to a AUX event.
> 
> I think we can add a checking in the function record__auxtrace_init(). After
> it calls auxtrace_record__init(), we can report failure if the AUX events have
> the overlapped cpumask.
> 
> > They would have to be on different CPUs for that to be true?
> 
> Yes.
> 
> > And wouldn't --per-thread have all AUX events in every mmap?
> 
> Before I roughly tested '--per-thread' mode and did not see issue. But this
> time I tested '--per-thread' mode and found the failure by the kernel checking
> [1] - it does not allow the different AUX events to bind to the same FD.
> 
> Here I need to dig a bit for two options, either we need to fix the perf
> tool to open multiple AUX events for '--per-thread' mode, or remove the
> kernel's checking to allow different AUX events to bind to same FD.
> 
> Thanks a lot for review!

Ok, the two patches from Adrian are in, I'll now wait for you to refresh
this series,

Thanks,

- Arnaldo