On Wed, May 12, 2021 at 05:39:56PM +0300, James Clark wrote:

On 29/04/2021 18:00, Leo Yan wrote:

quoted

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