Hi Andrey,
On Fri, Nov 20, 2020 at 03:29:39PM +0100, Andrey Zhizhikin wrote:
> Hello Salvatore,
>
> On Fri, Nov 20, 2020 at 2:34 PM Salvatore Bonaccorso <carnil@xxxxxxxxxx> wrote:
> >
> > Hi Andrey,
> >
> > On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote:
> > > On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@xxxxxxxxxx> wrote:
> > > >
> > > > This reverts commit 168200b6d6ea0cb5765943ec5da5b8149701f36a upstream.
> > > > (but only from 4.19.y)
> > >
> > > This revert would fail the build of 4.19.y with gcc10, I believe the
> > > original commit was introduced to address exactly this case. If this
> > > is intended behavior that 4.19.y is not compiled with newer gcc
> > > versions - then this revert is OK.
> >
> > TTBOMK, this would not regress the build for newer gcc (specifically
> > gcc10) as 4.19.158 is failing perf tool builds there as well (without
> > the above commit reverted). Just as an example v4.19.y does not have
> > cff20b3151cc ("perf tests bp_account: Make global variable static")
> > which is there in v5.6-rc6 to fix build failures with 10.0.1.
> >
> > But it did regress builds with older gcc's as for instance used in
> > Debian buster (gcc 8.3.0) since 4.19.152.
> >
> > Do I possibly miss something? If there is a solution to make it build
> > with newer GCCs and *not* regress previously working GCC versions then
> > this is surely the best outcome though.
>
> I guess (and from what I understand in Leo's reply), porting of
> 95c6fe970a01 ("perf cs-etm: Change tuple from traceID-CPU# to
> traceID-metadata") should solve the issue for both older and newer gcc
> versions.
>
> The breakage is now in
> [tools/perf/util/cs-etm-decoder/cs-etm-decoder.c] file (which uses
> traceid_list inside). This is solved with the above commit, which
> concealed traceid_list internally inside [tools/perf/util/cs-etm.c]
> file and exposed to [tools/perf/util/cs-etm-decoder/cs-etm-decoder.c]
> via cs_etm__get_cpu() call.
>
> Can you try out to port that commit to see if that would solve your
> regression?
So something like the following will compile as well with the older
gcc version.
I realize: I mainline the order of the commits was:
95c6fe970a01 ("perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata")
168200b6d6ea ("perf cs-etm: Move definition of 'traceid_list' global variable from header f
ile")
But to v4.19.y only 168200b6d6ea was backported, and while that was
done I now realize the comment was also changed including the change
fom 95c6fe970a01.
Thus the proposed backported patch would drop the change in
tools/perf/util/cs-etm.c to the comment as this was already done.
Thecnically currently the comment would be wrong, because it reads:
/* RB tree for quick conversion between traceID and metadata pointers */
but backport of 95c6fe970a01 is not included.
Would the right thing to do thus be:
- Revert b801d568c7d8 "perf cs-etm: Move definition of 'traceid_list' global variable from header file"
- Backport 95c6fe970a01 ("perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata")
- Backport 168200b6d6ea ("perf cs-etm: Move definition of 'traceid_list' global variable from header file")
?
Leo ist that what you were proposing?
Regards,
Salvatore
>From 7d6b3668d8ae5d3aea8827670ade8ac43b92db4a Mon Sep 17 00:00:00 2001
From: Leo Yan <leo.yan@xxxxxxxxxx>
Date: Tue, 29 Jan 2019 20:28:39 +0800
Subject: [PATCH] perf cs-etm: Change tuple from traceID-CPU# to
traceID-metadata
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@xxxxxxxxxx>
Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Cc: Mike Leach <mike.leach@xxxxxxxxxx>
Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
Cc: Robert Walker <robert.walker@xxxxxxx>
Cc: Suzuki K Poulouse <suzuki.poulose@xxxxxxx>
Cc: coresight ml <coresight@xxxxxxxxxxxxxxxx>
Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
Link: http://lkml.kernel.org/r/20190129122842.32041-6-leo.yan@xxxxxxxxxx
Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
[Salvatore Bonaccorso: Drop comment change in tools/perf/util/cs-etm.h
which was already changed with b801d568c7d8 ("perf cs-etm: Move
definition of 'traceid_list' global variable from header file")]
Signed-off-by: Salvatore Bonaccorso <carnil@xxxxxxxxxx>
---
.../perf/util/cs-etm-decoder/cs-etm-decoder.c | 8 +++---
tools/perf/util/cs-etm.c | 26 ++++++++++++++-----
tools/perf/util/cs-etm.h | 7 +++++
3 files changed, 30 insertions(+), 11 deletions(-)
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 ad33b99f5d21..3275b8dc9344 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -94,6 +94,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;
@@ -233,7 +247,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 */
@@ -1319,9 +1333,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) {
@@ -1387,8 +1401,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 c7ef97b198c7..97c3152f5bfd 100644
--- a/tools/perf/util/cs-etm.h
+++ b/tools/perf/util/cs-etm.h
@@ -66,6 +66,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,
@@ -73,6 +74,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
--
2.29.2
