Hello Salvatore,
On Fri, Nov 20, 2020 at 4:53 PM Salvatore Bonaccorso <carnil@xxxxxxxxxx> wrote:
>
> 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")
Yes, I believe this would be the correct course of action here; this
should cover the regression you've encountered and should ensure that
perf builds on both the "old" and "new" gcc versions.
>
> ?
>
> 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
>
--
Regards,
Andrey.
