From: Leo Yan <leo.yan@linaro.org>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
Mike Leach <mike.leach@linaro.org>,
Robert Walker <robert.walker@arm.com>,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org,
Coresight ML <coresight@lists.linaro.org>
Subject: Re: [PATCH v6 5/8] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata
Date: Thu, 24 Jan 2019 07:45:14 +0800 [thread overview]
Message-ID: <20190123234514.GB3268@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <20190123211300.GE620@xps15>
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.yan@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
> >
next prev parent reply other threads:[~2019-01-23 23:45 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-19 1:43 [PATCH v6 0/8] perf cs-etm: Add support for sample flags Leo Yan
2019-01-19 1:43 ` [PATCH v6 1/8] perf cs-etm: Add last instruction information in packet Leo Yan
2019-01-23 21:03 ` Mathieu Poirier
2019-01-19 1:43 ` [PATCH v6 2/8] perf cs-etm: Set sample flags for instruction range packet Leo Yan
2019-01-23 21:03 ` Mathieu Poirier
2019-01-19 1:43 ` [PATCH v6 3/8] perf cs-etm: Set sample flags for trace discontinuity Leo Yan
2019-01-23 21:04 ` Mathieu Poirier
2019-01-19 1:43 ` [PATCH v6 4/8] perf cs-etm: Add exception number in exception packet Leo Yan
2019-01-23 21:05 ` Mathieu Poirier
2019-01-19 1:43 ` [PATCH v6 5/8] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata Leo Yan
2019-01-23 21:13 ` Mathieu Poirier
2019-01-23 23:45 ` Leo Yan [this message]
2019-01-19 1:43 ` [PATCH v6 6/8] perf cs-etm: Add traceID in packet Leo Yan
2019-01-23 21:23 ` Mathieu Poirier
2019-01-19 1:43 ` [PATCH v6 7/8] perf cs-etm: Set sample flags for exception packet Leo Yan
2019-01-23 21:39 ` Mathieu Poirier
2019-01-19 1:43 ` [PATCH v6 8/8] perf cs-etm: Set sample flags for exception return packet Leo Yan
2019-01-23 21:51 ` Mathieu Poirier
2019-01-23 23:36 ` Leo Yan
2019-01-24 0:22 ` [PATCH v6 0/8] perf cs-etm: Add support for sample flags Mathieu Poirier
2019-01-24 4:00 ` Leo Yan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190123234514.GB3268@leoy-ThinkPad-X240s \
--to=leo.yan@linaro.org \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=coresight@lists.linaro.org \
--cc=jolsa@redhat.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=mike.leach@linaro.org \
--cc=namhyung@kernel.org \
--cc=robert.walker@arm.com \
--cc=suzuki.poulose@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).