diff mbox series

[1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata

Message ID 20201125201215.26455-1-carnil@debian.org (mailing list archive)
State New, archived
Headers show
Series [1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata | expand

Commit Message

Salvatore Bonaccorso Nov. 25, 2020, 8:12 p.m. UTC 
From: Leo Yan <leo.yan@linaro.org>

commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 upstream.

If packet processing wants to know the packet is bound with which ETM
version, it needs to access metadata to decide that based on metadata
magic number; but we cannot simply to use CPU logic ID number as index
to access metadata sequential array, especially when system have
hotplugged off CPUs, the metadata array are only allocated for online
CPUs but not offline CPUs, so the CPU logic number doesn't match with
its index in the array.

This patch is to change tuple from traceID-CPU# to traceID-metadata,
thus it can use the tuple to retrieve metadata pointer according to
traceID.

For safe accessing metadata fields, this patch provides helper function
cs_etm__get_cpu() which is used to return CPU number according to
traceID; cs_etm_decoder__buffer_packet() is the first consumer for this
helper function.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Robert Walker <robert.walker@arm.com>
Cc: Suzuki K Poulouse <suzuki.poulose@arm.com>
Cc: coresight ml <coresight@lists.linaro.org>
Cc: linux-arm-kernel@lists.infradead.org
Link: http://lkml.kernel.org/r/20190129122842.32041-6-leo.yan@linaro.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
[Salvatore Bonaccorso: Adjust for context changes in
tools/perf/util/cs-etm-decoder/cs-etm-decoder.c]
Signed-off-by: Salvatore Bonaccorso <carnil@debian.org>
---
 .../perf/util/cs-etm-decoder/cs-etm-decoder.c |  8 +++---
 tools/perf/util/cs-etm.c                      | 26 ++++++++++++++-----
 tools/perf/util/cs-etm.h                      |  9 ++++++-
 3 files changed, 31 insertions(+), 12 deletions(-)

Comments

Salvatore Bonaccorso Nov. 25, 2020, 8:23 p.m. UTC | #1 
On Wed, Nov 25, 2020 at 09:12:14PM +0100, Salvatore Bonaccorso wrote:
> From: Leo Yan <leo.yan@linaro.org>
> 
> commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 upstream.
> 
> If packet processing wants to know the packet is bound with which ETM
> version, it needs to access metadata to decide that based on metadata
> magic number; but we cannot simply to use CPU logic ID number as index
> to access metadata sequential array, especially when system have
> hotplugged off CPUs, the metadata array are only allocated for online
> CPUs but not offline CPUs, so the CPU logic number doesn't match with
> its index in the array.
> 
> This patch is to change tuple from traceID-CPU# to traceID-metadata,
> thus it can use the tuple to retrieve metadata pointer according to
> traceID.
> 
> For safe accessing metadata fields, this patch provides helper function
> cs_etm__get_cpu() which is used to return CPU number according to
> traceID; cs_etm_decoder__buffer_packet() is the first consumer for this
> helper function.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Robert Walker <robert.walker@arm.com>
> Cc: Suzuki K Poulouse <suzuki.poulose@arm.com>
> Cc: coresight ml <coresight@lists.linaro.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Link: http://lkml.kernel.org/r/20190129122842.32041-6-leo.yan@linaro.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> [Salvatore Bonaccorso: Adjust for context changes in
> tools/perf/util/cs-etm-decoder/cs-etm-decoder.c]
> Signed-off-by: Salvatore Bonaccorso <carnil@debian.org>

That's a fallout on my end. Should have said: This is for 4.19
specifically to be queued.

Background: in
https://lore.kernel.org/stable/20201014135627.GA3698844@kroah.com/
168200b6d6ea0cb5765943ec5da5b8149701f36a was queued up for v4.19.y but
the prerequeisite commit was not included and so resulted in build
failures with gcc 8.3.0.

The commit was later on reverted but in this thread it was asked to
actually make it possible to compile the file as well with more recent
gcc versions.

Those two patches to be applied in 4.19.y only pick up a backport of
the rerequisite commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 (PATCH
1) followed up by the cherry-pick of
168200b6d6ea0cb5765943ec5da5b8149701f36a again.

Regards,
Salvatore
Leo Yan Nov. 26, 2020, 1:35 a.m. UTC | #2 
On Wed, Nov 25, 2020 at 09:23:23PM +0100, Salvatore Bonaccorso wrote:
> On Wed, Nov 25, 2020 at 09:12:14PM +0100, Salvatore Bonaccorso wrote:
> > From: Leo Yan <leo.yan@linaro.org>
> > 
> > commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 upstream.

[...]

> That's a fallout on my end. Should have said: This is for 4.19
> specifically to be queued.
> 
> Background: in
> https://lore.kernel.org/stable/20201014135627.GA3698844@kroah.com/
> 168200b6d6ea0cb5765943ec5da5b8149701f36a was queued up for v4.19.y but
> the prerequeisite commit was not included and so resulted in build
> failures with gcc 8.3.0.
> 
> The commit was later on reverted but in this thread it was asked to
> actually make it possible to compile the file as well with more recent
> gcc versions.
> 
> Those two patches to be applied in 4.19.y only pick up a backport of
> the rerequisite commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 (PATCH
> 1) followed up by the cherry-pick of
> 168200b6d6ea0cb5765943ec5da5b8149701f36a again.

Since the patch 01 is minor tweaked due to context difference, I
manually compared it with original patch and looks good to me.

Thank you for the back porting,
Leo
Salvatore Bonaccorso Nov. 26, 2020, 4:52 a.m. UTC | #3 
Hi Leo,

On Thu, Nov 26, 2020 at 09:35:22AM +0800, Leo Yan wrote:
> On Wed, Nov 25, 2020 at 09:23:23PM +0100, Salvatore Bonaccorso wrote:
> > On Wed, Nov 25, 2020 at 09:12:14PM +0100, Salvatore Bonaccorso wrote:
> > > From: Leo Yan <leo.yan@linaro.org>
> > > 
> > > commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 upstream.
> 
> [...]
> 
> > That's a fallout on my end. Should have said: This is for 4.19
> > specifically to be queued.
> > 
> > Background: in
> > https://lore.kernel.org/stable/20201014135627.GA3698844@kroah.com/
> > 168200b6d6ea0cb5765943ec5da5b8149701f36a was queued up for v4.19.y but
> > the prerequeisite commit was not included and so resulted in build
> > failures with gcc 8.3.0.
> > 
> > The commit was later on reverted but in this thread it was asked to
> > actually make it possible to compile the file as well with more recent
> > gcc versions.
> > 
> > Those two patches to be applied in 4.19.y only pick up a backport of
> > the rerequisite commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 (PATCH
> > 1) followed up by the cherry-pick of
> > 168200b6d6ea0cb5765943ec5da5b8149701f36a again.
> 
> Since the patch 01 is minor tweaked due to context difference, I
> manually compared it with original patch and looks good to me.
> 
> Thank you for the back porting,

Welcome, given I'm unfamiliar with the codebasis for perf the explicit
acknowledgement nothing looks wrong was appreciated.

Regards,
Salvatore
diff mbox series

Patch

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
index 938def6d0bb9..f540037eb705 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -278,14 +278,12 @@  cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder,
 			      enum cs_etm_sample_type sample_type)
 {
 	u32 et = 0;
-	struct int_node *inode = NULL;
+	int cpu;
 
 	if (decoder->packet_count >= MAX_BUFFER - 1)
 		return OCSD_RESP_FATAL_SYS_ERR;
 
-	/* Search the RB tree for the cpu associated with this traceID */
-	inode = intlist__find(traceid_list, trace_chan_id);
-	if (!inode)
+	if (cs_etm__get_cpu(trace_chan_id, &cpu) < 0)
 		return OCSD_RESP_FATAL_SYS_ERR;
 
 	et = decoder->tail;
@@ -296,7 +294,7 @@  cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder,
 	decoder->packet_buffer[et].sample_type = sample_type;
 	decoder->packet_buffer[et].exc = false;
 	decoder->packet_buffer[et].exc_ret = false;
-	decoder->packet_buffer[et].cpu = *((int *)inode->priv);
+	decoder->packet_buffer[et].cpu = cpu;
 	decoder->packet_buffer[et].start_addr = CS_ETM_INVAL_ADDR;
 	decoder->packet_buffer[et].end_addr = CS_ETM_INVAL_ADDR;
 
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 7b5e15cc6b71..5cde3956e19a 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -91,6 +91,20 @@  static int cs_etm__update_queues(struct cs_etm_auxtrace *etm);
 static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm,
 					   pid_t tid, u64 time_);
 
+int cs_etm__get_cpu(u8 trace_chan_id, int *cpu)
+{
+	struct int_node *inode;
+	u64 *metadata;
+
+	inode = intlist__find(traceid_list, trace_chan_id);
+	if (!inode)
+		return -EINVAL;
+
+	metadata = inode->priv;
+	*cpu = (int)metadata[CS_ETM_CPU];
+	return 0;
+}
+
 static void cs_etm__packet_dump(const char *pkt_string)
 {
 	const char *color = PERF_COLOR_BLUE;
@@ -230,7 +244,7 @@  static void cs_etm__free(struct perf_session *session)
 	cs_etm__free_events(session);
 	session->auxtrace = NULL;
 
-	/* First remove all traceID/CPU# nodes for the RB tree */
+	/* First remove all traceID/metadata nodes for the RB tree */
 	intlist__for_each_entry_safe(inode, tmp, traceid_list)
 		intlist__remove(traceid_list, inode);
 	/* Then the RB tree itself */
@@ -1316,9 +1330,9 @@  int cs_etm__process_auxtrace_info(union perf_event *event,
 				    0xffffffff);
 
 	/*
-	 * Create an RB tree for traceID-CPU# tuple. Since the conversion has
-	 * to be made for each packet that gets decoded, optimizing access in
-	 * anything other than a sequential array is worth doing.
+	 * Create an RB tree for traceID-metadata tuple.  Since the conversion
+	 * has to be made for each packet that gets decoded, optimizing access
+	 * in anything other than a sequential array is worth doing.
 	 */
 	traceid_list = intlist__new(NULL);
 	if (!traceid_list) {
@@ -1384,8 +1398,8 @@  int cs_etm__process_auxtrace_info(union perf_event *event,
 			err = -EINVAL;
 			goto err_free_metadata;
 		}
-		/* All good, associate the traceID with the CPU# */
-		inode->priv = &metadata[j][CS_ETM_CPU];
+		/* All good, associate the traceID with the metadata pointer */
+		inode->priv = metadata[j];
 	}
 
 	/*
diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
index 37f8d48179ca..fb5fc6538b7f 100644
--- a/tools/perf/util/cs-etm.h
+++ b/tools/perf/util/cs-etm.h
@@ -53,7 +53,7 @@  enum {
 	CS_ETMV4_PRIV_MAX,
 };
 
-/* RB tree for quick conversion between traceID and CPUs */
+/* RB tree for quick conversion between traceID and metadata pointers */
 struct intlist *traceid_list;
 
 #define KiB(x) ((x) * 1024)
@@ -69,6 +69,7 @@  static const u64 __perf_cs_etmv4_magic   = 0x4040404040404040ULL;
 #ifdef HAVE_CSTRACE_SUPPORT
 int cs_etm__process_auxtrace_info(union perf_event *event,
 				  struct perf_session *session);
+int cs_etm__get_cpu(u8 trace_chan_id, int *cpu);
 #else
 static inline int
 cs_etm__process_auxtrace_info(union perf_event *event __maybe_unused,
@@ -76,6 +77,12 @@  cs_etm__process_auxtrace_info(union perf_event *event __maybe_unused,
 {
 	return -1;
 }
+
+static inline int cs_etm__get_cpu(u8 trace_chan_id __maybe_unused,
+				  int *cpu __maybe_unused)
+{
+	return -1;
+}
 #endif
 
 #endif