From: James Clark <james.clark@arm.com>
To: coresight@lists.linaro.org
Cc: al.grant@arm.com, branislav.rankov@arm.com, denik@chromium.org,
suzuki.poulose@arm.com, Mike Leach <mike.leach@linaro.org>,
Leo Yan <leo.yan@linaro.org>, Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
John Garry <john.garry@huawei.com>, Will Deacon <will@kernel.org>,
Mathieu Poirier <mathieu.poirier@linaro.org>,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/7] perf cs-etm: Only search timestamp in current sample's queue.
Date: Thu, 6 May 2021 13:45:59 +0300 [thread overview]
Message-ID: <69b81af7-8d94-a6fa-b2e3-ca4e8483d5c0@arm.com> (raw)
In-Reply-To: <20210212144513.31765-3-james.clark@arm.com>
On 12/02/2021 16:45, James Clark wrote:
> Change initial timestamp search to only operate on the queue
> related to the current event. In a later change the bounds
> of the aux record will also be used to reset the decoder and
> the record is only relevant to a single queue.
>
> This change makes some files that had coresight data
> but didn't syntesise any events start working and generating
> events. I'm not sure of the reason for that. I'd expect this
> change to only affect the ordering of events.
I've gotten to the bottom of this mystery of why decoding starts working because of this change.
Currently:
* _All_ decoding happens on the first AUX record
* Decoding depends on binary data (so also depends on MMAP records)
* Ordering of AUX records and MMAP records is timing sensitive
So there are two scenarios:
1) The perf.data file contains MMAPs followed by AUX records. Everything works
2) The perf.data file contains an AUX record, followed by MMAPS, then further AUX records. Decoding never worked.
Per-thread mode (timeless) always worked because we wait for EXIT rather than AUX to start the decode, which is after MMAPS.
Per-cpu mode was always at the mercy of the ordering of events. So it's not a regression that this patchset changes the behaviour
here and it's doing more of 'the right thing' now.
As a separate change I will add a warning to cs_etm__mem_access() when it fails to find the right binary as this is a current sore point.
James
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> tools/perf/util/cs-etm.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 27894facae5e..8f8b448632fb 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -97,7 +97,7 @@ struct cs_etm_queue {
> /* RB tree for quick conversion between traceID and metadata pointers */
> static struct intlist *traceid_list;
>
> -static int cs_etm__update_queues(struct cs_etm_auxtrace *etm);
> +static int cs_etm__update_queues(struct cs_etm_auxtrace *etm, int cpu);
> static int cs_etm__process_queues(struct cs_etm_auxtrace *etm);
> static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm,
> pid_t tid);
> @@ -524,7 +524,6 @@ static void cs_etm__dump_event(struct cs_etm_auxtrace *etm,
> static int cs_etm__flush_events(struct perf_session *session,
> struct perf_tool *tool)
> {
> - int ret;
> struct cs_etm_auxtrace *etm = container_of(session->auxtrace,
> struct cs_etm_auxtrace,
> auxtrace);
> @@ -534,11 +533,6 @@ static int cs_etm__flush_events(struct perf_session *session,
> if (!tool->ordered_events)
> return -EINVAL;
>
> - ret = cs_etm__update_queues(etm);
> -
> - if (ret < 0)
> - return ret;
> -
> if (etm->timeless_decoding)
> return cs_etm__process_timeless_queues(etm, -1);
>
> @@ -851,10 +845,7 @@ static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm,
> etmq->queue_nr = queue_nr;
> etmq->offset = 0;
>
> - if (etm->timeless_decoding)
> - return 0;
> - else
> - return cs_etm__search_first_timestamp(etmq);
> + return 0;
> }
>
> static int cs_etm__setup_queues(struct cs_etm_auxtrace *etm)
> @@ -874,14 +865,20 @@ static int cs_etm__setup_queues(struct cs_etm_auxtrace *etm)
> return 0;
> }
>
> -static int cs_etm__update_queues(struct cs_etm_auxtrace *etm)
> +static int cs_etm__update_queues(struct cs_etm_auxtrace *etm, int cpu)
> {
> + int ret;
> if (etm->queues.new_data) {
> etm->queues.new_data = false;
> - return cs_etm__setup_queues(etm);
> + ret = cs_etm__setup_queues(etm);
> + if (ret)
> + return ret;
> }
>
> - return 0;
> + if (!etm->timeless_decoding)
> + return cs_etm__search_first_timestamp(etm->queues.queue_array[cpu].priv);
> + else
> + return 0;
> }
>
> static inline
> @@ -2358,8 +2355,9 @@ static int cs_etm__process_event(struct perf_session *session,
> else
> timestamp = 0;
>
> - if (timestamp || etm->timeless_decoding) {
> - err = cs_etm__update_queues(etm);
> + if ((timestamp || etm->timeless_decoding)
> + && event->header.type == PERF_RECORD_AUX) {
> + err = cs_etm__update_queues(etm, sample->cpu);
> if (err)
> return err;
> }
>
_______________________________________________
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:[~2021-05-06 10:48 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-12 14:45 [PATCH 0/7] Split Coresight decode by aux records James Clark
2021-02-12 14:45 ` [PATCH 1/7] perf cs-etm: Split up etm queue setup function James Clark
2021-02-20 8:11 ` Leo Yan
2021-02-12 14:45 ` [PATCH 2/7] perf cs-etm: Only search timestamp in current sample's queue James Clark
2021-02-20 11:50 ` Leo Yan
2021-03-01 15:28 ` James Clark
2021-03-02 11:52 ` Leo Yan
2021-05-06 10:45 ` James Clark [this message]
2021-02-12 14:45 ` [PATCH 3/7] perf cs-etm: Save aux records in each etm queue James Clark
2021-02-27 7:10 ` Leo Yan
2021-03-01 15:43 ` James Clark
2021-03-02 12:03 ` Leo Yan
2021-02-12 14:45 ` [PATCH 4/7] perf cs-etm: don't process queues until cs_etm__flush_events James Clark
2021-02-12 14:45 ` [PATCH 5/7] perf cs-etm: split decode by aux records James Clark
2021-02-12 14:45 ` [PATCH 6/7] perf cs-etm: Use existing decode code path for --dump-raw-trace James Clark
2021-02-12 14:45 ` [PATCH 7/7] perf cs-etm: Suppress printing when resetting decoder James Clark
2021-02-24 16:13 ` [PATCH 0/7] Split Coresight decode by aux records Mathieu Poirier
2021-03-01 14:05 ` James Clark
2021-04-15 20:37 ` Mathieu Poirier
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=69b81af7-8d94-a6fa-b2e3-ca4e8483d5c0@arm.com \
--to=james.clark@arm.com \
--cc=al.grant@arm.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=branislav.rankov@arm.com \
--cc=coresight@lists.linaro.org \
--cc=denik@chromium.org \
--cc=john.garry@huawei.com \
--cc=jolsa@redhat.com \
--cc=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mathieu.poirier@linaro.org \
--cc=mike.leach@linaro.org \
--cc=namhyung@kernel.org \
--cc=suzuki.poulose@arm.com \
--cc=will@kernel.org \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).