Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 7 Jun 2021 14:03:21 -0600
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     Leo Yan <leo.yan@linaro.org>
Cc:     James Clark <james.clark@arm.com>, acme@kernel.org,
        coresight@lists.linaro.org, al.grant@arm.com,
        branislav.rankov@arm.com, denik@chromium.org,
        suzuki.poulose@arm.com, anshuman.khandual@arm.com,
        Mike Leach <mike.leach@linaro.org>,
        John Garry <john.garry@huawei.com>,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-arm-kernel@lists.infradead.org,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v4 1/1] perf cs-etm: Split Coresight decode by aux
 records
Message-ID: <20210607200321.GA179489@p14s>
References: <20210601130751.25750-1-james.clark@arm.com>
 <20210601130751.25750-2-james.clark@arm.com>
 <20210604052117.GI10272@leoy-ThinkPad-X240s>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210604052117.GI10272@leoy-ThinkPad-X240s>
Precedence: bulk

Hi James, 

Leo has beat me to the punch on most of the things I noticed in this set, so
this is just to complement what has already been pointed out.

On Fri, Jun 04, 2021 at 01:21:17PM +0800, Leo Yan wrote:
> Hi James,
> 
> On Tue, Jun 01, 2021 at 04:07:51PM +0300, James Clark wrote:
> > Populate the auxtrace queues using AUX records rather than whole
> > auxtrace buffers.
> > 
> > This is similar to the auxtrace_queues__process_index() ->
> > auxtrace_queues__add_indexed_event() flow where
> > perf_session__peek_event() is used to read AUXTRACE events out of random
> > positions in the file based on the auxtrace index.
> > 
> > Now there is a second pass using perf_session__peek_events() which loops
> > over all PERF_RECORD_AUX events, finds the corresponding auxtrace
> > buffer, and adds a fragment of that buffer to the auxtrace queues,
> > rather than the whole buffer.

The first loop is enacted by perf_session__peek_events() and processes all the
PERF_RECORD_AUX events.  The second loop correlates PERF_RECORD_AUX with
PERF_RECORD_AUXTRACE.

The above paragraph seems to indicate the opposite.

> > 
> > This is a completely new implementation compared to v1 and v2 of the
> > patchset where significant modifications were made to the decoding flow,
> > including saving all of the AUX records, and changing the point where
> > decoding started. These changes had effects on the outcome of decoding,
> > but with this implementation, the output of decoding is identical to
> > perf/core, except in the case where decoding failed due to the issue
> > that this change addresses. No changes to decoding were made, apart
> > from populating the auxtrace queues.
> 
> IMHO, this patch is much better than previous versions! :)
> 
> When went through the whole patch, the most code actually is general
> enough and can consider to place into into util/auxtrace.c.  If there
> have any objection, we can come back to add the code for only cs-etm
> specific.

I would leave the idea of moving this to util/auxtrace.c alone for the time
being.

> 
> > Because the decoder is already reset between each auxtrace buffer,
> > populating the queues with fragments of whole buffers based on AUX
> > records has the same affect as resetting the decoder on the AUX record
> > bounds during decode. But this approach only requires a change to one
> > part of the whole flow, rather than many parts.
> > 
> > There is still one TODO about how buffers are found. Currently there is
> > a plain loop as a search, but this could be improved with a binary
> > search or keeping a record of which buffers have already been added and
> > starting the search from the last used place. AUX records and buffers
> > will come in similar orders in the file so the latter would be simpler
> > and probably be good enough. But I'm looking for feedback about the
> > approach as a whole before ironing out all of the details.

I think for the time being what you have here will work just fine.

> > 
> > Other issues that were fixed by the previous patchsets such as fixing
> > the case where the first aux record comes before the first MMAP record
> > can now be fixed as completely independent issues.
> > 

I assume you did not expect this patch to be merged and as such wrote as much as
you could, which is fine.  The next revision should have a changelog that will
concentrate on why this patch is needed.  I think the testing section (below) is
useful and should be kept around.

> > Testing
> > =======
> > 
> > Testing was done with the following script, to diff the decoding results
> > between the patched and un-patched versions of perf:
> > 
> > 	#!/bin/bash
> > 	set -ex
> > 
> > 	$1 script -i $3 $4 > split.script
> > 	$2 script -i $3 $4 > default.script
> > 
> > 	diff split.script default.script | head -n 20
> > 
> > And it was run like this, with various itrace options depending on the
> > quantity of synthesised events:
> > 
> > 	compare.sh ./perf-patched ./perf-default perf-per-cpu-2-threads.data --itrace=i100000ns
> > 
> > No changes in output were observed in the following scenarios:
> > 
> > * Simple per-cpu
> > 	perf record -e cs_etm/@tmc_etr0/u top
> > 
> > * Per-thread, single thread
> > 	perf record -e cs_etm/@tmc_etr0/u --per-thread ./threads_C
> > 
> > * Per-thread multiple threads (but only one thread collected data):
> > 	perf record -e cs_etm/@tmc_etr0/u --per-thread --pid 4596,4597
> > 
> > * Per-thread multiple threads (both threads collected data):
> > 	perf record -e cs_etm/@tmc_etr0/u --per-thread --pid 4596,4597
> > 
> > * Per-cpu explicit threads:
> > 	perf record -e cs_etm/@tmc_etr0/u --pid 853,854
> > 
> > * System-wide (per-cpu):
> >     perf record -e cs_etm/@tmc_etr0/u -a
> > 
> > * No data collected (no aux buffers)
> > 	Can happen with any command when run for a short period
> > 
> > * Containing truncated records
> > 	Can happen with any command
> > 
> > * Containing aux records with 0 size
> > 	Can happen with any command
> > 
> > * Snapshot mode
> > 	perf record -e cs_etm/@tmc_etr0/u -a --snapshot
> > 
> > Signed-off-by: James Clark <james.clark@arm.com>
> > ---
> >  tools/perf/util/cs-etm.c | 159 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 156 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > index 64536a6ed10a..053aad4b28cf 100644
> > --- a/tools/perf/util/cs-etm.c
> > +++ b/tools/perf/util/cs-etm.c
> > @@ -2679,6 +2679,161 @@ static u64 *cs_etm__create_meta_blk(u64 *buff_in, int *buff_in_offset,
> >  	return metadata;
> >  }
> >  
> > +/**
> > + * Puts a fragment of an auxtrace buffer into the auxtrace queues based
> > + * on the bounds of aux_event, if it matches with the buffer that's at
> > + * file_offset.
> > + *
> > + * Normally, whole auxtrace buffers would be added to the queue. But we
> > + * want to reset the decoder for every PERF_RECORD_AUX event, and the decoder
> > + * is reset across each buffer, so splitting the buffers up in advance has
> > + * the same effect.
> > + */
> > +static int cs_etm__queue_aux_fragment(struct perf_session *session,
> > +						off_t file_offset, size_t sz,
> > +						struct perf_record_aux *aux_event,
> > +						struct perf_sample *sample)

The indentation doesn't work.  The 'o' of off_t should be aligned with the 's'
of struct on the first line.  

> > +{
> > +	int err;
> > +	char buf[PERF_SAMPLE_MAX_SIZE];
> > +	union perf_event *auxtrace_event_union;
> > +	struct perf_record_auxtrace *auxtrace_event;
> > +	union perf_event auxtrace_fragment;
> > +	bool matchesCpuPid;
> 
> It's better to avoid using camel naming.
> 

Leo is correct - I'm surprised checkpatch hasn't complained bitterly.

> > +	__u64 aux_offset;
> > +	struct cs_etm_auxtrace *etm = container_of(session->auxtrace,
> > +						   struct cs_etm_auxtrace,
> > +						   auxtrace);
> > +
> > +	/*
> > +	 * There should be a PERF_RECORD_AUXTRACE event at the file_offset that we got
> > +	 * from looping through the auxtrace index.
> > +	 */
> > +	err = perf_session__peek_event(session, file_offset, buf,
> > +				       PERF_SAMPLE_MAX_SIZE, &auxtrace_event_union, NULL);
> > +	if (err)
> > +		return err;
> > +	auxtrace_event = &auxtrace_event_union->auxtrace;
> > +	if (auxtrace_event->header.type != PERF_RECORD_AUXTRACE)
> > +		return -EINVAL;
> > +
> > +	if (auxtrace_event->header.size < sizeof(struct perf_record_auxtrace) ||
> > +		auxtrace_event->header.size != sz) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * In per-thread mode, CPU is set to -1, but TID will be set instead.
> > +	 * See auxtrace_mmap_params__set_idx()
> > +	 */
> > +	if (auxtrace_event->cpu == (__u32) -1)
> > +		matchesCpuPid = auxtrace_event->tid == sample->tid;
> > +	else
> > +		matchesCpuPid = auxtrace_event->cpu == sample->cpu;
> 
> If "matchesCpuPid" is false, can directly bail out at here, so can
> avoid the below unnecessary calculations.
> 
> > +
> > +	/*
> > +	 * In snapshot/overwrite mode, the head points to the end of the buffer so aux_offset needs
> > +	 * to have the size subtracted so it points to the beginning as in normal mode.
> > +	 */
> > +	if (aux_event->flags & PERF_AUX_FLAG_OVERWRITE)
> > +		aux_offset = aux_event->aux_offset - aux_event->aux_size;
> > +	else
> > +		aux_offset = aux_event->aux_offset;
> > +
> > +	if (matchesCpuPid &&
> > +	    aux_offset >= auxtrace_event->offset &&
> > +	    aux_offset + aux_event->aux_size <= auxtrace_event->offset + auxtrace_event->size) {
> 
> It's possible that an event PERF_RECORD_AUXTRACE saves AUX trace data
> for multiple events PERF_RECORD_AUX (if we consider watermark).
> 
> So here the logic makes sense for me for checking the buffer range and
> I don't find any issue.
> 
> > +		/*
> > +		 * If this AUX event was inside this buffer somewhere, create a new auxtrace event
> > +		 * based on the sizes of the aux event, and queue that fragment.
> > +		 */
> > +		auxtrace_fragment.auxtrace = *auxtrace_event;
> > +		auxtrace_fragment.auxtrace.size = aux_event->aux_size;
> > +		auxtrace_fragment.auxtrace.offset = aux_offset;
> > +		file_offset += aux_offset - auxtrace_event->offset + auxtrace_event->header.size;
> > +		return auxtrace_queues__add_event(&etm->queues,
> > +					       session,
> > +					       &auxtrace_fragment,
> > +					       file_offset,
> > +					       NULL);
> > +	}
> > +
> > +	/* Wasn't inside this buffer, but there were no parse errors. 1 == 'not found' */
> > +	return 1;
> > +}
> > +
> > +static int cs_etm__queue_aux_records_cb(struct perf_session *session,
> > +			     union perf_event *event, u64 offset __maybe_unused,
> > +			     void *data __maybe_unused)

Same comment about the indentation.

> > +{
> > +	struct perf_sample sample;
> > +	int ret;
> > +	struct auxtrace_index_entry *ent;
> > +	struct auxtrace_index *auxtrace_index;
> > +	struct evsel *evsel;
> > +	size_t i;
> > +	struct cs_etm_auxtrace *etm = container_of(session->auxtrace,
> > +						   struct cs_etm_auxtrace,
> > +						   auxtrace);
> > +
	struct cs_etm_auxtrace *etm = container_of(session->auxtrace,
						   struct cs_etm_auxtrace,
						   auxtrace);
	struct auxtrace_index *auxtrace_index;
	struct auxtrace_index_entry *ent;
	struct perf_sample sample;
	struct evsel *evsel;
	size_t i;
	int ret;

> > +	/* Don't care about any other events, we're only queuing buffers for AUX events */
> > +	if (event->header.type != PERF_RECORD_AUX)
> > +		return 0;
> > +
> > +	if (event->header.size < sizeof(struct perf_record_aux))
> > +		return -EINVAL;
> > +
> > +	/* Truncated Aux records can have 0 size and shouldn't result in anything being queued. */
> > +	if (!event->aux.aux_size)
> > +		return 0;
> > +
> > +	/*
> > +	 * Parse the sample, we need the sample_id_all data that comes after the event so that the
> > +	 * CPU or PID can be matched to an AUXTRACE buffer's CPU or PID.
> > +	 */
> > +	evsel = evlist__event2evsel(session->evlist, event);
> > +	evsel__parse_sample(evsel, event, &sample);
> 
> It's good to check the return errors from evsel__parse_sample().
> 
> > +
> > +	/*
> > +	 * Loop throuch the auxtrace index to find the buffer that matches up with this
> > +	 * aux event.
> > +	 * TODO: binary search?
> 
> For binary search, you might need to reorg the auxtrace_index and
> its entries, I personally think this is not the priority.
> 

Here too I agree with Leo.  

> > +	 */
> > +	list_for_each_entry(auxtrace_index, &session->auxtrace_index, list) {
> > +		for (i = 0; i < auxtrace_index->nr; i++) {
> > +			ent = &auxtrace_index->entries[i];
> > +			ret = cs_etm__queue_aux_fragment(session, ent->file_offset,
> > +							 ent->sz, &event->aux, &sample);
> > +			if (!ret) {
> > +				etm->data_queued = true;
> > +				return 0;
> > +			} else if (ret < 0) {
> > +				/*
> > +				 * Anything other than 1 is an error. Positive values are 'not
> > +				 * found' we just want to go onto the next one in that case.
> > +				 */
> > +				return ret;
> > +			}
> > +		}
> > +	}
> 
> It's good to add a warning at here?  If cannot find a matched
> AUXRECORD event and fails to queue the fragment, it's likely caused by
> the AUX ring buffer's overrun.
> 
> > +
> > +	/*
> > +	 * We would get here if there are no entries in the index (either no auxtrace buffers or
> > +	 * no index). Fail silently as there is the possibility of queueing them in
> > +	 * cs_etm__process_auxtrace_event() if etm->data_queued is still false;
> > +	 *
> > +	 * In that scenario, buffers will not be split by AUX records.
> > +	 */
> > +	return 0;
> > +}
> > +
> > +static int cs_etm__queue_aux_records(struct perf_session *session)
> > +{
> > +	return perf_session__peek_events(session, session->header.data_offset,
> > +					 session->header.data_size,
> > +					 cs_etm__queue_aux_records_cb, NULL);

Perfect indentation. Here cs_etm__queue_aux_records() doesn't give us much.  I
would simply call perf_session__peek_events() from
cs_etm__process_auxtrace_info().

> > +}
> > +
> >  int cs_etm__process_auxtrace_info(union perf_event *event,
> >  				  struct perf_session *session)
> >  {
> > @@ -2879,12 +3034,10 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
> >  	if (err)
> >  		goto err_delete_thread;
> >  
> > -	err = auxtrace_queues__process_index(&etm->queues, session);
> > +	err = cs_etm__queue_aux_records(session);
> >  	if (err)
> >  		goto err_delete_thread;
> >  
> > -	etm->data_queued = etm->queues.populated;
> 
> Seems to me it's no reason to remove this sentence.
> 
> "etm->queues.populated" will be set when call
> auxtrace_queues__add_event(), so we still can assign it to
> "etm->data_queued" at here.
> 
> Thanks,
> Leo
> 

I'm happy with what is done in this patch.

Thanks,
Mathieu

> > -
> >  	return 0;
> >  
> >  err_delete_thread:
> > -- 
> > 2.28.0
> >