On Fri, Jan 11, 2019 at 10:43:52PM +0800, Leo Yan wrote:
On Thu, Jan 10, 2019 at 03:51:47PM -0700, Mathieu Poirier wrote:
On Wed, Jan 09, 2019 at 07:37:04PM +0800, Leo Yan wrote:
If packet processing wants to check ETM version, it needs to access metadata to decide that; but we cannot simply to use CPU logic ID number as index to access metadata sequential array, especially when system have hotplugged out 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 more reliable way to access metadata array is to use traceID to find 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 patch is to change tuple from traceID-CPU# to traceID-metadata, thus it can use the tuple to retrieve metadata pointer according to traceID.
Signed-off-by: Leo Yan leo.yan@linaro.org
.../perf/util/cs-etm-decoder/cs-etm-decoder.c | 5 +++- tools/perf/util/cs-etm.c | 23 ++++++++++++++----- tools/perf/util/cs-etm.h | 2 +- 3 files changed, 22 insertions(+), 8 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 05830fe1fb70..a537e13c45a0 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -305,6 +305,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, { u32 et = 0; struct int_node *inode = NULL;
- u64 *metadata;
if (decoder->packet_count >= MAX_BUFFER - 1) return OCSD_RESP_FATAL_SYS_ERR; @@ -314,6 +315,8 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, if (!inode) return OCSD_RESP_FATAL_SYS_ERR;
- metadata = inode->priv;
- et = decoder->tail; et = (et + 1) & (MAX_BUFFER - 1); decoder->tail = et;
@@ -321,7 +324,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 = (int)metadata[CS_ETM_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 f07ebc4f57bc..7c1399eda135 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -252,7 +252,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 */
@@ -1521,9 +1521,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 retrives/retrive/retrieve
Checkpacth give me a big warning about it... I'm surprised you didn't get it on your side.
Sorry, my bad. Will fix it and later will ensure to use checkpatch before send out.
The last time someone sent me patches without using checkpatch, I deleted their set without reviewing it and I didn't tell them. Consider yourself warned.
* 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.@@ -1589,8 +1600,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];Ouch - I understand why you're doing this but freely dabbing in the metadata array makes me queasy. Could we record the traceID in the packets and make the tuple traceID-index instead?
Changing to traceID-index tuple seems better for me. But have one question for it:
If we change to use traceID-index tuple, this will introduce trouble for function cs_etm_decoder__buffer_packet(). In this function, we have trace_chan_id and need to use it to get CPU number; after using traceID-index tuple we can get index value, but we cannot get cs_etm_auxtrace::metadata due structure cs_etm_auxtrace is only a private structure in cs-etm.c and doesn't export to cs-etm-decoder/cs-etm-decoder.c.
Maybe we can keep traceID-CPU# tuple and it's only used for traceID to CPU number conversion in cs-etm.c, and add a new tuple traceID-index and this can be used as you suggested. Or other ideas?
My first thought was to move the needed structures to cs-etm.h but after taking a second look I think we can do better. First let's keep your idea of having a traceID-metadata tuple. But to access the information we could use helper functions - here is an example for CPUs:
int cs_etm__get_cpu(u8 traceid, *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]; }
By proceeding this way we remove code duplication associated with getting the inode and functions that need access to CPU and ETM architecture information don't play directly with the metadata array.
Let me know if you find a way to improve on this.
Mathieu
} /* diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index 37f8d48179ca..a145aafb020a 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)
2.17.1