From: Mathieu Poirier <mathieu.poirier@linaro.org> To: Leo Yan <leo.yan@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: Wed, 23 Jan 2019 14:13:00 -0700 [thread overview] Message-ID: <20190123211300.GE620@xps15> (raw) In-Reply-To: <20190119014347.27441-6-leo.yan@linaro.org> 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.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 . > */ > 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 >
WARNING: multiple messages have this Message-ID (diff)
From: Mathieu Poirier <mathieu.poirier@linaro.org> To: Leo Yan <leo.yan@linaro.org> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Coresight ML <coresight@lists.linaro.org>, linux-kernel@vger.kernel.org, Arnaldo Carvalho de Melo <acme@kernel.org>, Namhyung Kim <namhyung@kernel.org>, Robert Walker <robert.walker@arm.com>, Jiri Olsa <jolsa@redhat.com>, linux-arm-kernel@lists.infradead.org, Mike Leach <mike.leach@linaro.org> Subject: Re: [PATCH v6 5/8] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata Date: Wed, 23 Jan 2019 14:13:00 -0700 [thread overview] Message-ID: <20190123211300.GE620@xps15> (raw) In-Reply-To: <20190119014347.27441-6-leo.yan@linaro.org> 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.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 . > */ > 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-01-23 21:13 UTC|newest] Thread overview: 42+ 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 ` Leo Yan 2019-01-19 1:43 ` [PATCH v6 1/8] perf cs-etm: Add last instruction information in packet Leo Yan 2019-01-19 1:43 ` Leo Yan 2019-01-23 21:03 ` Mathieu Poirier 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-19 1:43 ` Leo Yan 2019-01-23 21:03 ` Mathieu Poirier 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-19 1:43 ` Leo Yan 2019-01-23 21:04 ` Mathieu Poirier 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-19 1:43 ` Leo Yan 2019-01-23 21:05 ` Mathieu Poirier 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-19 1:43 ` Leo Yan 2019-01-23 21:13 ` Mathieu Poirier [this message] 2019-01-23 21:13 ` Mathieu Poirier 2019-01-23 23:45 ` Leo Yan 2019-01-23 23:45 ` Leo Yan 2019-01-19 1:43 ` [PATCH v6 6/8] perf cs-etm: Add traceID in packet Leo Yan 2019-01-19 1:43 ` Leo Yan 2019-01-23 21:23 ` Mathieu Poirier 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-19 1:43 ` Leo Yan 2019-01-23 21:39 ` Mathieu Poirier 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-19 1:43 ` Leo Yan 2019-01-23 21:51 ` Mathieu Poirier 2019-01-23 21:51 ` Mathieu Poirier 2019-01-23 23:36 ` Leo Yan 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 0:22 ` Mathieu Poirier 2019-01-24 4:00 ` Leo Yan 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=20190123211300.GE620@xps15 \ --to=mathieu.poirier@linaro.org \ --cc=acme@kernel.org \ --cc=alexander.shishkin@linux.intel.com \ --cc=coresight@lists.linaro.org \ --cc=jolsa@redhat.com \ --cc=leo.yan@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.