Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 24 Jan 2019 07:45:14 +0800
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
Message-ID: <20190123234514.GB3268@leoy-ThinkPad-X240s>
References: <20190119014347.27441-1-leo.yan@linaro.org>
 <20190119014347.27441-6-leo.yan@linaro.org>
 <20190123211300.GE620@xps15>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190123211300.GE620@xps15>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
> >