On Wed, Jan 23, 2019 at 02:13:00PM -0700, Mathieu Poirier wrote:
> 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.
Will do.
> > 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 .
Here just want to give more information for why we create
traceID-metadata tuple (and hope can give the reason why we need
to use traceID-metadata rather than traceID-CPU# tuple).
If you think it's redundant, will drop it.
> > */
> > 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.
My stupid. Will move it into HAVE_CSTRACE_SUPPORT blocks.
Thanks a lot for reviewing.
> > +
> > #endif
> > --
> > 2.17.1
> >
