On Fri, Jan 11, 2019 at 10:48:54AM -0700, Mathieu Poirier wrote:
[...]
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.
Yeah, I will try best to avoid this situation :)
* 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.
This suggestion is good to me.
By following this, I also will add a new function cs_etm__get_magic(u8 trace_id, u64 *magic) for accessing magic number in metadata.
Thanks a lot for suggestions.
[...]