On Sat, Jan 19, 2019 at 09:43:44AM +0800, Leo Yan wrote:
> 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.
>
> For this reason, a reliable way for accessing metadata array is to use
> traceID to find associated metadata; by accessing metadata content we
> can know not only the CPU number but also for ETM version, which can be
> used for sequential change for setting sample flags for exception
> packets.
This paragraph is not needed to understand why this patch is needed. Please
remove.
>
> 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....@linaro.org>
> ---
> .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 8 ++--
> tools/perf/util/cs-etm.c | 37 ++++++++++++++++---
> tools/perf/util/cs-etm.h | 4 +-
> 3 files changed, 37 insertions(+), 12 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 294efa76c9e3..cdd38ffd10d2 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -305,14 +305,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;
> @@ -322,7 +320,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder
> *decoder,
>
> decoder->packet_buffer[et].sample_type = sample_type;
> decoder->packet_buffer[et].isa = CS_ETM_ISA_UNKNOWN;
> - 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;
> decoder->packet_buffer[et].instr_count = 0;
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 1aa29633ce77..e89989fe0a5c 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -97,6 +97,20 @@ static u32 cs_etm__get_v7_protocol_version(u32 etmidr)
> return CS_ETM_PROTO_ETMV3;
> }
>
> +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;
> @@ -252,7 +266,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 */
> @@ -1519,9 +1533,20 @@ 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.
> + *
> + * The conversion between traceID and CPU logic ID number has to
> + * be made for each packet that gets decoded: firstly retrieve
> + * metadata pointer from trace ID by using traceID-metadata tuple,
> + * then read CPU logic ID number in metadata.
> + *
> + * It's not safe to directly use CPU logic ID number as index to
> + * access metadata sequential array, e.g. when system have
> + * hotplugged out CPUs, the metadata array are only allocated for
> + * online CPUs but not offline CPUs, thus the CPU logic number is
> + * not consistent with its index in the arrary. For this reason,
> + * we need to fallback to use TraceID-metadata tuple as a reliable
> + * method to access metadata.
Why adding this long comment? To me all that is needed is
s/traceID-CPU#/traceID-metadata .
> */
> traceid_list = intlist__new(NULL);
> if (!traceid_list) {
> @@ -1587,8 +1612,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..5d70d10f3907 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)
> @@ -78,4 +78,6 @@ cs_etm__process_auxtrace_info(union perf_event *event
> __maybe_unused,
> }
> #endif
>
> +int cs_etm__get_cpu(u8 trace_chan_id, int *cpu);
This function is part of a public header that can theoretically be included by
any other file. As such it has to be defined within the HAVE_CSTRACE_SUPPORT
define.
> +
> #endif
> --
> 2.17.1
>
