[PATCH v2 0/4] perf arm-spe: Track pid/tid for Arm SPE samples

[PATCH v2 0/4] perf arm-spe: Track pid/tid for Arm SPE samples

[German Gomez]

The following patchset is an iteration on RFC [1] where pid/tid info is
assigned to the Arm SPE synthesized samples. Two methods of tracking
pids are considered: hardware-based (using Arm SPE CONTEXT packets), and
context-switch events (from perf) as fallback.

  - Patch #1 enables pid tracking using RECORD_SWITCH* events from perf.
  - Patch #2 updates perf-record documentation and arm-spe recording so
    that they are consistent.
  - Patch #3 saves the value of SPE CONTEXT packet to the arm_spe_record
    struct.
  - Patch #4 enables hardware-based pid tracking using SPE CONTEXT
    packets.

Changes since v1:

  - [PATCH 1/4] Fix authorship of commit.
  - [PATCH 2/4] (New patch) Updated perf-record docs to reflect the
    behavior of Arm SPE introduced by the previous patch.
  - [PATCH 3/4] update initialization of context_id field to (u64)-1.
  - [PATCH 4/4] Update handling of pid/tid tracking fallback following
    Leo Yan's suggestion. Don't consider per-thread mode on this patch.

German Gomez (3):
  perf arm-spe: Update --switch-events docs in perf-record
  perf arm-spe: Save context ID in record
  perf arm-spe: Support hardware-based PID tracing

Namhyung Kim (1):
  perf arm-spe: Track task context switch for cpu-mode events

 tools/perf/Documentation/perf-record.txt      |   2 +-
 tools/perf/arch/arm64/util/arm-spe.c          |   8 +-
 .../util/arm-spe-decoder/arm-spe-decoder.c    |   2 +
 .../util/arm-spe-decoder/arm-spe-decoder.h    |   1 +
 tools/perf/util/arm-spe.c                     | 120 ++++++++++++++----
 5 files changed, 104 insertions(+), 29 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] perf arm-spe: Track task context switch for cpu-mode events

[German Gomez]

From: Namhyung Kim <namhyung@kernel.org>

When perf report synthesize events from ARM SPE data, it refers to
current cpu, pid and tid in the machine.  But there's no place to set
them in the ARM SPE decoder.  I'm seeing all pid/tid is set to -1 and
user symbols are not resolved in the output.

  # perf record -a -e arm_spe_0/ts_enable=1/ sleep 1

  # perf report -q | head
     8.77%     8.77%  :-1      [kernel.kallsyms]  [k] format_decode
     7.02%     7.02%  :-1      [kernel.kallsyms]  [k] seq_printf
     7.02%     7.02%  :-1      [unknown]          [.] 0x0000ffff9f687c34
     5.26%     5.26%  :-1      [kernel.kallsyms]  [k] vsnprintf
     3.51%     3.51%  :-1      [kernel.kallsyms]  [k] string
     3.51%     3.51%  :-1      [unknown]          [.] 0x0000ffff9f66ae20
     3.51%     3.51%  :-1      [unknown]          [.] 0x0000ffff9f670b3c
     3.51%     3.51%  :-1      [unknown]          [.] 0x0000ffff9f67c040
     1.75%     1.75%  :-1      [kernel.kallsyms]  [k] ___cache_free
     1.75%     1.75%  :-1      [kernel.kallsyms]  [k]
__count_memcg_events

Like Intel PT, add context switch records to track task info.  As ARM
SPE support was added later than PERF_RECORD_SWITCH_CPU_WIDE, I think
we can safely set the attr.context_switch bit and use it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: German Gomez <german.gomez@arm.com>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/arch/arm64/util/arm-spe.c |  6 +++++-
 tools/perf/util/arm-spe.c            | 25 +++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index a4420d4df..58ba8d15c 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -166,8 +166,12 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	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))
+	if (!perf_cpu_map__empty(cpus)) {
 		evsel__set_sample_bit(tracking_evsel, TIME);
+		evsel__set_sample_bit(tracking_evsel, CPU);
+		/* also track task context switch */
+		tracking_evsel->core.attr.context_switch = 1;
+	}
 
 	return 0;
 }
diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
index 58b7069c5..230bc7ab2 100644
--- a/tools/perf/util/arm-spe.c
+++ b/tools/perf/util/arm-spe.c
@@ -681,6 +681,25 @@ static int arm_spe_process_timeless_queues(struct arm_spe *spe, pid_t tid,
 	return 0;
 }
 
+static int arm_spe_context_switch(struct arm_spe *spe, union perf_event *event,
+				  struct perf_sample *sample)
+{
+	pid_t pid, tid;
+	int cpu;
+
+	if (!(event->header.misc & PERF_RECORD_MISC_SWITCH_OUT))
+		return 0;
+
+	pid = event->context_switch.next_prev_pid;
+	tid = event->context_switch.next_prev_tid;
+	cpu = sample->cpu;
+
+	if (tid == -1)
+		pr_warning("context_switch event has no tid\n");
+
+	return machine__set_current_tid(spe->machine, cpu, pid, tid);
+}
+
 static int arm_spe_process_event(struct perf_session *session,
 				 union perf_event *event,
 				 struct perf_sample *sample,
@@ -718,6 +737,12 @@ static int arm_spe_process_event(struct perf_session *session,
 		}
 	} else if (timestamp) {
 		err = arm_spe_process_queues(spe, timestamp);
+		if (err)
+			return err;
+
+		if (event->header.type == PERF_RECORD_SWITCH_CPU_WIDE ||
+		    event->header.type == PERF_RECORD_SWITCH)
+			err = arm_spe_context_switch(spe, event, sample);
 	}
 
 	return err;
-- 
2.25.1

[PATCH v2 2/4] perf arm-spe: Update --switch-events docs in perf-record

[German Gomez]

Update perf-record docs and Arm SPE recording options so that they are
consistent. This includes supporting the --no-switch-events flag in Arm
SPE as well.

Signed-off-by: German Gomez <german.gomez@arm.com>
---
 tools/perf/Documentation/perf-record.txt | 2 +-
 tools/perf/arch/arm64/util/arm-spe.c     | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 2d7df8703..3cf7bac67 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -469,7 +469,7 @@ This option sets the time out limit. The default value is 500 ms.
 
 --switch-events::
 Record context switch events i.e. events of type PERF_RECORD_SWITCH or
-PERF_RECORD_SWITCH_CPU_WIDE. In some cases (e.g. Intel PT or CoreSight)
+PERF_RECORD_SWITCH_CPU_WIDE. In some cases (e.g. Intel PT, CoreSight or Arm SPE)
 switch events will be enabled automatically, which can be suppressed by
 by the option --no-switch-events.
 
diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index 58ba8d15c..725a06cd2 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -169,8 +169,10 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	if (!perf_cpu_map__empty(cpus)) {
 		evsel__set_sample_bit(tracking_evsel, TIME);
 		evsel__set_sample_bit(tracking_evsel, CPU);
+
 		/* also track task context switch */
-		tracking_evsel->core.attr.context_switch = 1;
+		if (!record_opts__no_switch_events(opts))
+			tracking_evsel->core.attr.context_switch = 1;
 	}
 
 	return 0;
-- 
2.25.1

Re: [PATCH v2 2/4] perf arm-spe: Update --switch-events docs in perf-record

[Leo Yan]

On Tue, Nov 09, 2021 at 11:50:18AM +0000, German Gomez wrote:
> Update perf-record docs and Arm SPE recording options so that they are
> consistent. This includes supporting the --no-switch-events flag in Arm
> SPE as well.
> 
> Signed-off-by: German Gomez <german.gomez@arm.com>

Reviewed-by: Leo Yan <leo.yan@linaro.org>

> ---
>  tools/perf/Documentation/perf-record.txt | 2 +-
>  tools/perf/arch/arm64/util/arm-spe.c     | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> index 2d7df8703..3cf7bac67 100644
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -469,7 +469,7 @@ This option sets the time out limit. The default value is 500 ms.
>  
>  --switch-events::
>  Record context switch events i.e. events of type PERF_RECORD_SWITCH or
> -PERF_RECORD_SWITCH_CPU_WIDE. In some cases (e.g. Intel PT or CoreSight)
> +PERF_RECORD_SWITCH_CPU_WIDE. In some cases (e.g. Intel PT, CoreSight or Arm SPE)
>  switch events will be enabled automatically, which can be suppressed by
>  by the option --no-switch-events.
>  
> diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
> index 58ba8d15c..725a06cd2 100644
> --- a/tools/perf/arch/arm64/util/arm-spe.c
> +++ b/tools/perf/arch/arm64/util/arm-spe.c
> @@ -169,8 +169,10 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
>  	if (!perf_cpu_map__empty(cpus)) {
>  		evsel__set_sample_bit(tracking_evsel, TIME);
>  		evsel__set_sample_bit(tracking_evsel, CPU);
> +
>  		/* also track task context switch */
> -		tracking_evsel->core.attr.context_switch = 1;
> +		if (!record_opts__no_switch_events(opts))
> +			tracking_evsel->core.attr.context_switch = 1;
>  	}
>  
>  	return 0;
> -- 
> 2.25.1
> 

Re: [PATCH v2 2/4] perf arm-spe: Update --switch-events docs in perf-record

[Namhyung Kim]

On Wed, Nov 10, 2021 at 11:18 PM Leo Yan <leo.yan@linaro.org> wrote:
>
> On Tue, Nov 09, 2021 at 11:50:18AM +0000, German Gomez wrote:
> > Update perf-record docs and Arm SPE recording options so that they are
> > consistent. This includes supporting the --no-switch-events flag in Arm
> > SPE as well.
> >
> > Signed-off-by: German Gomez <german.gomez@arm.com>
>
> Reviewed-by: Leo Yan <leo.yan@linaro.org>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

[PATCH v2 3/4] perf arm-spe: Save context ID in record

[German Gomez]

This patch is to save context ID in record, this will be used to set TID
for samples.

Signed-off-by: German Gomez <german.gomez@arm.com>
Reviewed-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/arm-spe-decoder/arm-spe-decoder.c | 2 ++
 tools/perf/util/arm-spe-decoder/arm-spe-decoder.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
index 32fe41835..3fc528c92 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
@@ -151,6 +151,7 @@ static int arm_spe_read_record(struct arm_spe_decoder *decoder)
 	u64 payload, ip;
 
 	memset(&decoder->record, 0x0, sizeof(decoder->record));
+	decoder->record.context_id = (u64)-1;
 
 	while (1) {
 		err = arm_spe_get_next_packet(decoder);
@@ -180,6 +181,7 @@ static int arm_spe_read_record(struct arm_spe_decoder *decoder)
 		case ARM_SPE_COUNTER:
 			break;
 		case ARM_SPE_CONTEXT:
+			decoder->record.context_id = payload;
 			break;
 		case ARM_SPE_OP_TYPE:
 			if (idx == SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC) {
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
index 59bdb7309..46a8556a9 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
@@ -38,6 +38,7 @@ struct arm_spe_record {
 	u64 timestamp;
 	u64 virt_addr;
 	u64 phys_addr;
+	u64 context_id;
 };
 
 struct arm_spe_insn;
-- 
2.25.1

Re: [PATCH v2 3/4] perf arm-spe: Save context ID in record

[Namhyung Kim]

Hello,

On Tue, Nov 9, 2021 at 3:50 AM German Gomez <german.gomez@arm.com> wrote:
>
> This patch is to save context ID in record, this will be used to set TID
> for samples.
>
> Signed-off-by: German Gomez <german.gomez@arm.com>
> Reviewed-by: Leo Yan <leo.yan@linaro.org>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> ---
>  tools/perf/util/arm-spe-decoder/arm-spe-decoder.c | 2 ++
>  tools/perf/util/arm-spe-decoder/arm-spe-decoder.h | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> index 32fe41835..3fc528c92 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> @@ -151,6 +151,7 @@ static int arm_spe_read_record(struct arm_spe_decoder *decoder)
>         u64 payload, ip;
>
>         memset(&decoder->record, 0x0, sizeof(decoder->record));
> +       decoder->record.context_id = (u64)-1;
>
>         while (1) {
>                 err = arm_spe_get_next_packet(decoder);
> @@ -180,6 +181,7 @@ static int arm_spe_read_record(struct arm_spe_decoder *decoder)
>                 case ARM_SPE_COUNTER:
>                         break;
>                 case ARM_SPE_CONTEXT:
> +                       decoder->record.context_id = payload;
>                         break;
>                 case ARM_SPE_OP_TYPE:
>                         if (idx == SPE_OP_PKT_HDR_CLASS_LD_ST_ATOMIC) {
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> index 59bdb7309..46a8556a9 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> @@ -38,6 +38,7 @@ struct arm_spe_record {
>         u64 timestamp;
>         u64 virt_addr;
>         u64 phys_addr;
> +       u64 context_id;
>  };
>
>  struct arm_spe_insn;
> --
> 2.25.1
>

[PATCH v2 4/4] perf arm-spe: Support hardware-based PID tracing

[German Gomez]

If Arm SPE traces contain CONTEXT packets with TID info, use these
values for tracking tid of samples. Otherwise fall back to using context
switch events and display a message warning the user of possible timing
inaccuracies [1].

[1] https://lore.kernel.org/lkml/f877cfa6-9b25-6445-3806-ca44a4042eaf@arm.com/

Signed-off-by: German Gomez <german.gomez@arm.com>
---
 tools/perf/util/arm-spe.c | 99 +++++++++++++++++++++++++++------------
 1 file changed, 70 insertions(+), 29 deletions(-)

diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
index 230bc7ab2..30b8bb48a 100644
--- a/tools/perf/util/arm-spe.c
+++ b/tools/perf/util/arm-spe.c
@@ -71,6 +71,7 @@ struct arm_spe {
 	u64				kernel_start;
 
 	unsigned long			num_events;
+	u8				use_ctx_pkt_for_pid;
 };
 
 struct arm_spe_queue {
@@ -226,6 +227,44 @@ static inline u8 arm_spe_cpumode(struct arm_spe *spe, u64 ip)
 		PERF_RECORD_MISC_USER;
 }
 
+static void arm_spe_set_pid_tid_cpu(struct arm_spe *spe,
+				    struct auxtrace_queue *queue)
+{
+	struct arm_spe_queue *speq = queue->priv;
+	pid_t tid;
+
+	tid = machine__get_current_tid(spe->machine, speq->cpu);
+	if (tid != -1) {
+		speq->tid = tid;
+		thread__zput(speq->thread);
+	} else
+		speq->tid = queue->tid;
+
+	if ((!speq->thread) && (speq->tid != -1)) {
+		speq->thread = machine__find_thread(spe->machine, -1,
+						    speq->tid);
+	}
+
+	if (speq->thread) {
+		speq->pid = speq->thread->pid_;
+		if (queue->cpu == -1)
+			speq->cpu = speq->thread->cpu;
+	}
+}
+
+static int arm_spe_set_tid(struct arm_spe_queue *speq, pid_t tid)
+{
+	struct arm_spe *spe = speq->spe;
+	int err = machine__set_current_tid(spe->machine, speq->cpu, tid, tid);
+
+	if (err)
+		return err;
+
+	arm_spe_set_pid_tid_cpu(spe, &spe->queues.queue_array[speq->queue_nr]);
+
+	return 0;
+}
+
 static void arm_spe_prep_sample(struct arm_spe *spe,
 				struct arm_spe_queue *speq,
 				union perf_event *event,
@@ -460,6 +499,19 @@ static int arm_spe_run_decoder(struct arm_spe_queue *speq, u64 *timestamp)
 		 * can correlate samples between Arm SPE trace data and other
 		 * perf events with correct time ordering.
 		 */
+
+		/*
+		 * Update pid/tid info.
+		 */
+		record = &speq->decoder->record;
+		if (!spe->timeless_decoding && record->context_id != (u64)-1) {
+			ret = arm_spe_set_tid(speq, record->context_id);
+			if (ret)
+				return ret;
+
+			spe->use_ctx_pkt_for_pid = true;
+		}
+
 		ret = arm_spe_sample(speq);
 		if (ret)
 			return ret;
@@ -586,31 +638,6 @@ static bool arm_spe__is_timeless_decoding(struct arm_spe *spe)
 	return timeless_decoding;
 }
 
-static void arm_spe_set_pid_tid_cpu(struct arm_spe *spe,
-				    struct auxtrace_queue *queue)
-{
-	struct arm_spe_queue *speq = queue->priv;
-	pid_t tid;
-
-	tid = machine__get_current_tid(spe->machine, speq->cpu);
-	if (tid != -1) {
-		speq->tid = tid;
-		thread__zput(speq->thread);
-	} else
-		speq->tid = queue->tid;
-
-	if ((!speq->thread) && (speq->tid != -1)) {
-		speq->thread = machine__find_thread(spe->machine, -1,
-						    speq->tid);
-	}
-
-	if (speq->thread) {
-		speq->pid = speq->thread->pid_;
-		if (queue->cpu == -1)
-			speq->cpu = speq->thread->cpu;
-	}
-}
-
 static int arm_spe_process_queues(struct arm_spe *spe, u64 timestamp)
 {
 	unsigned int queue_nr;
@@ -641,7 +668,12 @@ static int arm_spe_process_queues(struct arm_spe *spe, u64 timestamp)
 			ts = timestamp;
 		}
 
-		arm_spe_set_pid_tid_cpu(spe, queue);
+		/*
+		 * A previous context-switch event has set pid/tid in the machine's context, so
+		 * here we need to update the pid/tid in the thread and SPE queue.
+		 */
+		if (!spe->use_ctx_pkt_for_pid)
+			arm_spe_set_pid_tid_cpu(spe, queue);
 
 		ret = arm_spe_run_decoder(speq, &ts);
 		if (ret < 0) {
@@ -740,8 +772,9 @@ static int arm_spe_process_event(struct perf_session *session,
 		if (err)
 			return err;
 
-		if (event->header.type == PERF_RECORD_SWITCH_CPU_WIDE ||
-		    event->header.type == PERF_RECORD_SWITCH)
+		if (!spe->use_ctx_pkt_for_pid &&
+		    (event->header.type == PERF_RECORD_SWITCH_CPU_WIDE ||
+		    event->header.type == PERF_RECORD_SWITCH))
 			err = arm_spe_context_switch(spe, event, sample);
 	}
 
@@ -808,7 +841,15 @@ static int arm_spe_flush(struct perf_session *session __maybe_unused,
 		return arm_spe_process_timeless_queues(spe, -1,
 				MAX_TIMESTAMP - 1);
 
-	return arm_spe_process_queues(spe, MAX_TIMESTAMP);
+	ret = arm_spe_process_queues(spe, MAX_TIMESTAMP);
+	if (ret)
+		return ret;
+
+	if (!spe->use_ctx_pkt_for_pid)
+		ui__warning("Arm SPE CONTEXT packets not found in the traces.\n"
+			    "Matching of TIDs to SPE events could be inaccurate.\n");
+
+	return 0;
 }
 
 static void arm_spe_free_queue(void *priv)
-- 
2.25.1

Re: [PATCH v2 4/4] perf arm-spe: Support hardware-based PID tracing

[Namhyung Kim]

On Tue, Nov 9, 2021 at 3:50 AM German Gomez <german.gomez@arm.com> wrote:
>
> If Arm SPE traces contain CONTEXT packets with TID info, use these
> values for tracking tid of samples. Otherwise fall back to using context
> switch events and display a message warning the user of possible timing
> inaccuracies [1].
>
> [1] https://lore.kernel.org/lkml/f877cfa6-9b25-6445-3806-ca44a4042eaf@arm.com/
>
> Signed-off-by: German Gomez <german.gomez@arm.com>
> ---
>  tools/perf/util/arm-spe.c | 99 +++++++++++++++++++++++++++------------
>  1 file changed, 70 insertions(+), 29 deletions(-)
>
> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
> index 230bc7ab2..30b8bb48a 100644
> --- a/tools/perf/util/arm-spe.c
> +++ b/tools/perf/util/arm-spe.c
> @@ -71,6 +71,7 @@ struct arm_spe {
>         u64                             kernel_start;
>
>         unsigned long                   num_events;
> +       u8                              use_ctx_pkt_for_pid;
>  };
>
>  struct arm_spe_queue {
> @@ -226,6 +227,44 @@ static inline u8 arm_spe_cpumode(struct arm_spe *spe, u64 ip)
>                 PERF_RECORD_MISC_USER;
>  }
>
> +static void arm_spe_set_pid_tid_cpu(struct arm_spe *spe,
> +                                   struct auxtrace_queue *queue)
> +{
> +       struct arm_spe_queue *speq = queue->priv;
> +       pid_t tid;
> +
> +       tid = machine__get_current_tid(spe->machine, speq->cpu);
> +       if (tid != -1) {
> +               speq->tid = tid;
> +               thread__zput(speq->thread);
> +       } else
> +               speq->tid = queue->tid;
> +
> +       if ((!speq->thread) && (speq->tid != -1)) {
> +               speq->thread = machine__find_thread(spe->machine, -1,
> +                                                   speq->tid);
> +       }
> +
> +       if (speq->thread) {
> +               speq->pid = speq->thread->pid_;
> +               if (queue->cpu == -1)
> +                       speq->cpu = speq->thread->cpu;
> +       }
> +}
> +
> +static int arm_spe_set_tid(struct arm_spe_queue *speq, pid_t tid)
> +{
> +       struct arm_spe *spe = speq->spe;
> +       int err = machine__set_current_tid(spe->machine, speq->cpu, tid, tid);

I think we should pass -1 as pid as we don't know the real pid.

Thanks,
Namhyung


> +
> +       if (err)
> +               return err;
> +
> +       arm_spe_set_pid_tid_cpu(spe, &spe->queues.queue_array[speq->queue_nr]);
> +
> +       return 0;
> +}
> +

Re: [PATCH v2 4/4] perf arm-spe: Support hardware-based PID tracing

[Leo Yan]

On Wed, Nov 10, 2021 at 11:28:48PM -0800, Namhyung Kim wrote:

[...]

> > +static void arm_spe_set_pid_tid_cpu(struct arm_spe *spe,
> > +                                   struct auxtrace_queue *queue)
> > +{
> > +       struct arm_spe_queue *speq = queue->priv;
> > +       pid_t tid;
> > +
> > +       tid = machine__get_current_tid(spe->machine, speq->cpu);
> > +       if (tid != -1) {
> > +               speq->tid = tid;
> > +               thread__zput(speq->thread);
> > +       } else
> > +               speq->tid = queue->tid;
> > +
> > +       if ((!speq->thread) && (speq->tid != -1)) {
> > +               speq->thread = machine__find_thread(spe->machine, -1,
> > +                                                   speq->tid);
> > +       }
> > +
> > +       if (speq->thread) {
> > +               speq->pid = speq->thread->pid_;
> > +               if (queue->cpu == -1)
> > +                       speq->cpu = speq->thread->cpu;
> > +       }
> > +}
> > +
> > +static int arm_spe_set_tid(struct arm_spe_queue *speq, pid_t tid)
> > +{
> > +       struct arm_spe *spe = speq->spe;
> > +       int err = machine__set_current_tid(spe->machine, speq->cpu, tid, tid);
> 
> I think we should pass -1 as pid as we don't know the real pid.

AFAICT, I observe one case for machine__set_current_tid() returning error
is 'speq->cpu' is -1 (this is the case for per-thread tracing).  In
this case, if pass '-1' for pid/tid, it still will return failure.

So here should return the error as it is.  Am I missing anything?

Thanks,
Leo

Re: [PATCH v2 4/4] perf arm-spe: Support hardware-based PID tracing

[Namhyung Kim]

Hi Leo,

On Wed, Nov 10, 2021 at 11:41 PM Leo Yan <leo.yan@linaro.org> wrote:
>
> On Wed, Nov 10, 2021 at 11:28:48PM -0800, Namhyung Kim wrote:
>
> [...]
>
> > > +static void arm_spe_set_pid_tid_cpu(struct arm_spe *spe,
> > > +                                   struct auxtrace_queue *queue)
> > > +{
> > > +       struct arm_spe_queue *speq = queue->priv;
> > > +       pid_t tid;
> > > +
> > > +       tid = machine__get_current_tid(spe->machine, speq->cpu);
> > > +       if (tid != -1) {
> > > +               speq->tid = tid;
> > > +               thread__zput(speq->thread);
> > > +       } else
> > > +               speq->tid = queue->tid;
> > > +
> > > +       if ((!speq->thread) && (speq->tid != -1)) {
> > > +               speq->thread = machine__find_thread(spe->machine, -1,
> > > +                                                   speq->tid);
> > > +       }
> > > +
> > > +       if (speq->thread) {
> > > +               speq->pid = speq->thread->pid_;
> > > +               if (queue->cpu == -1)
> > > +                       speq->cpu = speq->thread->cpu;
> > > +       }
> > > +}
> > > +
> > > +static int arm_spe_set_tid(struct arm_spe_queue *speq, pid_t tid)
> > > +{
> > > +       struct arm_spe *spe = speq->spe;
> > > +       int err = machine__set_current_tid(spe->machine, speq->cpu, tid, tid);
> >
> > I think we should pass -1 as pid as we don't know the real pid.
>
> AFAICT, I observe one case for machine__set_current_tid() returning error
> is 'speq->cpu' is -1 (this is the case for per-thread tracing).  In
> this case, if pass '-1' for pid/tid, it still will return failure.
>
> So here should return the error as it is.  Am I missing anything?

I'm not saying about the error.  It's about thread status.
In the machine__set_current_tid(), it calls
machine__findnew_thread() with given pid and tid.

I suspect it can set pid to a wrong value if the thread has
no pid value at the moment.

Thanks,
Namhyung

Re: [PATCH v2 4/4] perf arm-spe: Support hardware-based PID tracing

[Leo Yan]

Hi Namhyung,

On Wed, Nov 10, 2021 at 11:59:05PM -0800, Namhyung Kim wrote:

[...]

> > > > +static void arm_spe_set_pid_tid_cpu(struct arm_spe *spe,
> > > > +                                   struct auxtrace_queue *queue)
> > > > +{
> > > > +       struct arm_spe_queue *speq = queue->priv;
> > > > +       pid_t tid;
> > > > +
> > > > +       tid = machine__get_current_tid(spe->machine, speq->cpu);
> > > > +       if (tid != -1) {
> > > > +               speq->tid = tid;
> > > > +               thread__zput(speq->thread);
> > > > +       } else
> > > > +               speq->tid = queue->tid;
> > > > +
> > > > +       if ((!speq->thread) && (speq->tid != -1)) {
> > > > +               speq->thread = machine__find_thread(spe->machine, -1,
> > > > +                                                   speq->tid);
> > > > +       }
> > > > +
> > > > +       if (speq->thread) {
> > > > +               speq->pid = speq->thread->pid_;
> > > > +               if (queue->cpu == -1)
> > > > +                       speq->cpu = speq->thread->cpu;
> > > > +       }
> > > > +}
> > > > +
> > > > +static int arm_spe_set_tid(struct arm_spe_queue *speq, pid_t tid)
> > > > +{
> > > > +       struct arm_spe *spe = speq->spe;
> > > > +       int err = machine__set_current_tid(spe->machine, speq->cpu, tid, tid);
> > >
> > > I think we should pass -1 as pid as we don't know the real pid.
> >
> > AFAICT, I observe one case for machine__set_current_tid() returning error
> > is 'speq->cpu' is -1 (this is the case for per-thread tracing).  In
> > this case, if pass '-1' for pid/tid, it still will return failure.
> >
> > So here should return the error as it is.  Am I missing anything?
> 
> I'm not saying about the error.  It's about thread status.
> In the machine__set_current_tid(), it calls
> machine__findnew_thread() with given pid and tid.
> 
> I suspect it can set pid to a wrong value if the thread has
> no pid value at the moment.

Here we should avoid to write pid '-1' with
machine__set_current_tid().

The function arm_spe_set_tid() is invoked when SPE trace data contains
context packet and it passes pid coming from the context packet.  On
the other hand, when SPE trace data doesn't contain context packet, we
relies on context switch event to set pid value.  So if we pass pid
'-1' in arm_spe_set_tid(), it will overwrite the pid value which has
been set by context switch event.

Simply say, if SPE trace data contains context packet with valid pid,
perf invokes arm_spe_set_tid() to set the pid value.  Otherwise, it
should skip this operation and roll back to use the pid value from
the context switch event.

Thanks,
Leo

Re: [PATCH v2 4/4] perf arm-spe: Support hardware-based PID tracing

[German Gomez]

Hi Leo, Namhyung,

On 11/11/2021 08:30, Leo Yan wrote:
> Hi Namhyung,
>
> On Wed, Nov 10, 2021 at 11:59:05PM -0800, Namhyung Kim wrote:
>
> [...]
>
>>>>> +static void arm_spe_set_pid_tid_cpu(struct arm_spe *spe,
>>>>> +                                   struct auxtrace_queue *queue)
>>>>> +{
>>>>> +       struct arm_spe_queue *speq = queue->priv;
>>>>> +       pid_t tid;
>>>>> +
>>>>> +       tid = machine__get_current_tid(spe->machine, speq->cpu);
>>>>> +       if (tid != -1) {
>>>>> +               speq->tid = tid;
>>>>> +               thread__zput(speq->thread);
>>>>> +       } else
>>>>> +               speq->tid = queue->tid;
>>>>> +
>>>>> +       if ((!speq->thread) && (speq->tid != -1)) {
>>>>> +               speq->thread = machine__find_thread(spe->machine, -1,
>>>>> +                                                   speq->tid);
>>>>> +       }
>>>>> +
>>>>> +       if (speq->thread) {
>>>>> +               speq->pid = speq->thread->pid_;
>>>>> +               if (queue->cpu == -1)
>>>>> +                       speq->cpu = speq->thread->cpu;
>>>>> +       }
>>>>> +}
>>>>> +
>>>>> +static int arm_spe_set_tid(struct arm_spe_queue *speq, pid_t tid)
>>>>> +{
>>>>> +       struct arm_spe *spe = speq->spe;
>>>>> +       int err = machine__set_current_tid(spe->machine, speq->cpu, tid, tid);
>>>> I think we should pass -1 as pid as we don't know the real pid.
>>> AFAICT, I observe one case for machine__set_current_tid() returning error
>>> is 'speq->cpu' is -1 (this is the case for per-thread tracing).  In
>>> this case, if pass '-1' for pid/tid, it still will return failure.
>>>
>>> So here should return the error as it is.  Am I missing anything?
>> I'm not saying about the error.  It's about thread status.
>> In the machine__set_current_tid(), it calls
>> machine__findnew_thread() with given pid and tid.
>>
>> I suspect it can set pid to a wrong value if the thread has
>> no pid value at the moment.
> Here we should avoid to write pid '-1' with
> machine__set_current_tid().

If the kernel is writing the tids to the contextidr, isn't it wrong to
assume tid == pid when decoding the context packets here? I haven't
observed any impact in the built-in commands though, so there must be
something I'm not seeing.

Thanks,
German
>
> The function arm_spe_set_tid() is invoked when SPE trace data contains
> context packet and it passes pid coming from the context packet.  On
> the other hand, when SPE trace data doesn't contain context packet, we
> relies on context switch event to set pid value.  So if we pass pid
> '-1' in arm_spe_set_tid(), it will overwrite the pid value which has
> been set by context switch event.
>
> Simply say, if SPE trace data contains context packet with valid pid,
> perf invokes arm_spe_set_tid() to set the pid value.  Otherwise, it
> should skip this operation and roll back to use the pid value from
> the context switch event.
>
> Thanks,
> Leo

Re: [PATCH v2 4/4] perf arm-spe: Support hardware-based PID tracing

[Leo Yan]

On Thu, Nov 11, 2021 at 12:23:08PM +0000, German Gomez wrote:
> On 11/11/2021 08:30, Leo Yan wrote:
> > On Wed, Nov 10, 2021 at 11:59:05PM -0800, Namhyung Kim wrote:

[...]

> >>>>> +static int arm_spe_set_tid(struct arm_spe_queue *speq, pid_t tid)
> >>>>> +{
> >>>>> +       struct arm_spe *spe = speq->spe;
> >>>>> +       int err = machine__set_current_tid(spe->machine, speq->cpu, tid, tid);
> >>>>
> >>>> I think we should pass -1 as pid as we don't know the real pid.
> >>>>
> >>> AFAICT, I observe one case for machine__set_current_tid() returning error
> >>> is 'speq->cpu' is -1 (this is the case for per-thread tracing).  In
> >>> this case, if pass '-1' for pid/tid, it still will return failure.
> >>>
> >>> So here should return the error as it is.  Am I missing anything?
> >>
> >> I'm not saying about the error.  It's about thread status.
> >> In the machine__set_current_tid(), it calls
> >> machine__findnew_thread() with given pid and tid.
> >>
> >> I suspect it can set pid to a wrong value if the thread has
> >> no pid value at the moment.
> >
> > Here we should avoid to write pid '-1' with
> > machine__set_current_tid().
> 
> If the kernel is writing the tids to the contextidr, isn't it wrong to
> assume tid == pid when decoding the context packets here? I haven't
> observed any impact in the built-in commands though, so there must be
> something I'm not seeing.

Okay, let me correct myself :)

I checked Intel-pt's implementation, I understand now that we need to 
distinguish the cases for pid/tid from context switch event and only tid
from SPE context packet.

Since the context switch event contains the correct pid and tid
values, we should set both of them, see Intel-PT's implementation [1].

As Namhyung pointed, we need to set pid as '-1' when we only know the
tid value from SPE context packet; see [2].

So we should use the same with Intel-pt.

Sorry for I didn't really understand well Namhyung's suggestion and
thanks you both pointed out the issue.

Leo

P.s. an offline topic is we should send a patch to fix cs-etm issue
as well [3].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/intel-pt.c#n2920
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/intel-pt.c#n2215
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/cs-etm.c#n1121

Re: [PATCH v2 4/4] perf arm-spe: Support hardware-based PID tracing

[German Gomez]

Hi, thanks for looking into it

On 11/11/2021 12:42, Leo Yan wrote:
> On Thu, Nov 11, 2021 at 12:23:08PM +0000, German Gomez wrote:
>> On 11/11/2021 08:30, Leo Yan wrote:
>>> On Wed, Nov 10, 2021 at 11:59:05PM -0800, Namhyung Kim wrote:
> [...]
>
>>>>>>> +static int arm_spe_set_tid(struct arm_spe_queue *speq, pid_t tid)
>>>>>>> +{
>>>>>>> +       struct arm_spe *spe = speq->spe;
>>>>>>> +       int err = machine__set_current_tid(spe->machine, speq->cpu, tid, tid);
>>>>>> I think we should pass -1 as pid as we don't know the real pid.
>>>>>>
>>>>> AFAICT, I observe one case for machine__set_current_tid() returning error
>>>>> is 'speq->cpu' is -1 (this is the case for per-thread tracing).  In
>>>>> this case, if pass '-1' for pid/tid, it still will return failure.
>>>>>
>>>>> So here should return the error as it is.  Am I missing anything?
>>>> I'm not saying about the error.  It's about thread status.
>>>> In the machine__set_current_tid(), it calls
>>>> machine__findnew_thread() with given pid and tid.
>>>>
>>>> I suspect it can set pid to a wrong value if the thread has
>>>> no pid value at the moment.
>>> Here we should avoid to write pid '-1' with
>>> machine__set_current_tid().
>> If the kernel is writing the tids to the contextidr, isn't it wrong to
>> assume tid == pid when decoding the context packets here? I haven't
>> observed any impact in the built-in commands though, so there must be
>> something I'm not seeing.
> Okay, let me correct myself :)
>
> I checked Intel-pt's implementation, I understand now that we need to 
> distinguish the cases for pid/tid from context switch event and only tid
> from SPE context packet.
>
> Since the context switch event contains the correct pid and tid
> values, we should set both of them, see Intel-PT's implementation [1].
>
> As Namhyung pointed, we need to set pid as '-1' when we only know the
> tid value from SPE context packet; see [2].

I will correct this and resend the patch for SPE.

Thanks,
German

>
> So we should use the same with Intel-pt.
>
> Sorry for I didn't really understand well Namhyung's suggestion and
> thanks you both pointed out the issue.
>
> Leo
>
> P.s. an offline topic is we should send a patch to fix cs-etm issue
> as well [3].
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/intel-pt.c#n2920
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/intel-pt.c#n2215
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/cs-etm.c#n1121

Re: [PATCH v2 0/4] perf arm-spe: Track pid/tid for Arm SPE samples

[Leo Yan]

Hi Arnaldo,

On Tue, Nov 09, 2021 at 11:50:16AM +0000, German Gomez wrote:
> The following patchset is an iteration on RFC [1] where pid/tid info is
> assigned to the Arm SPE synthesized samples. Two methods of tracking
> pids are considered: hardware-based (using Arm SPE CONTEXT packets), and
> context-switch events (from perf) as fallback.
> 
>   - Patch #1 enables pid tracking using RECORD_SWITCH* events from perf.
>   - Patch #2 updates perf-record documentation and arm-spe recording so
>     that they are consistent.
>   - Patch #3 saves the value of SPE CONTEXT packet to the arm_spe_record
>     struct.
>   - Patch #4 enables hardware-based pid tracking using SPE CONTEXT
>     packets.

I have tested this patch set, it works well on Hisilicon D06 board,
please consider to pick up.  Thanks!

Leo

Re: [PATCH v2 0/4] perf arm-spe: Track pid/tid for Arm SPE samples

[Leo Yan]

On Thu, Nov 11, 2021 at 03:27:14PM +0800, Leo Yan wrote:
> Hi Arnaldo,
> 
> On Tue, Nov 09, 2021 at 11:50:16AM +0000, German Gomez wrote:
> > The following patchset is an iteration on RFC [1] where pid/tid info is
> > assigned to the Arm SPE synthesized samples. Two methods of tracking
> > pids are considered: hardware-based (using Arm SPE CONTEXT packets), and
> > context-switch events (from perf) as fallback.
> > 
> >   - Patch #1 enables pid tracking using RECORD_SWITCH* events from perf.
> >   - Patch #2 updates perf-record documentation and arm-spe recording so
> >     that they are consistent.
> >   - Patch #3 saves the value of SPE CONTEXT packet to the arm_spe_record
> >     struct.
> >   - Patch #4 enables hardware-based pid tracking using SPE CONTEXT
> >     packets.
> 
> I have tested this patch set, it works well on Hisilicon D06 board,
> please consider to pick up.  Thanks!

Hi Arnaldo,

Please hold on this version and German will respin a new patch set for
a found issue.

Thanks,
Leo

Re: [PATCH v2 0/4] perf arm-spe: Track pid/tid for Arm SPE samples

[Arnaldo Carvalho de Melo]

Em Thu, Nov 11, 2021 at 09:26:47PM +0800, Leo Yan escreveu:
> On Thu, Nov 11, 2021 at 03:27:14PM +0800, Leo Yan wrote:
> > Hi Arnaldo,
> > 
> > On Tue, Nov 09, 2021 at 11:50:16AM +0000, German Gomez wrote:
> > > The following patchset is an iteration on RFC [1] where pid/tid info is
> > > assigned to the Arm SPE synthesized samples. Two methods of tracking
> > > pids are considered: hardware-based (using Arm SPE CONTEXT packets), and
> > > context-switch events (from perf) as fallback.
> > > 
> > >   - Patch #1 enables pid tracking using RECORD_SWITCH* events from perf.
> > >   - Patch #2 updates perf-record documentation and arm-spe recording so
> > >     that they are consistent.
> > >   - Patch #3 saves the value of SPE CONTEXT packet to the arm_spe_record
> > >     struct.
> > >   - Patch #4 enables hardware-based pid tracking using SPE CONTEXT
> > >     packets.
> > 
> > I have tested this patch set, it works well on Hisilicon D06 board,
> > please consider to pick up.  Thanks!
> 
> Hi Arnaldo,
> 
> Please hold on this version and German will respin a new patch set for
> a found issue.

Ok

- Arnaldo