From: James Clark <james.clark@arm.com>
To: Mathieu Poirier <mathieu.poirier@linaro.org>,
Leo Yan <leo.yan@linaro.org>
Cc: 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,
John Garry <john.garry@huawei.com>, Will Deacon <will@kernel.org>,
Mike Leach <mike.leach@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>,
linux-arm-kernel@lists.infradead.org,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 2/2] perf cs-etm: Split --dump-raw-trace by AUX records
Date: Tue, 29 Jun 2021 09:52:28 +0100 [thread overview]
Message-ID: <b9a3ae95-987b-d1de-a242-141e7444aa83@arm.com> (raw)
In-Reply-To: <20210628200132.GB1200359@p14s>
On 28/06/2021 21:01, Mathieu Poirier wrote:
> On Mon, Jun 28, 2021 at 08:08:02PM +0800, Leo Yan wrote:
>> On Mon, Jun 28, 2021 at 11:38:34AM +0100, James Clark wrote:
>>
>> [...]
>>
>>>>> static int cs_etm__process_auxtrace_event(struct perf_session *session,
>>>>> union perf_event *event,
>>>>> struct perf_tool *tool __maybe_unused)
>>>>> @@ -2462,7 +2478,8 @@ static int cs_etm__process_auxtrace_event(struct perf_session *session,
>>>>> cs_etm__dump_event(etm, buffer);
>>>>> auxtrace_buffer__put_data(buffer);
>>>>> }
>>>>> - }
>>>>> + } else if (dump_trace)
>>>>> + dump_queued_data(etm, &event->auxtrace);
>>>>
>>>> IIUC, in the function cs_etm__process_auxtrace_event(), since
>>>> "etm->data_queued" is always true, below flow will never run:
>>>>
>>>> if (!etm->data_queued) {
>>>> ......
>>>>
>>>> if (dump_trace)
>>>> if (auxtrace_buffer__get_data(buffer, fd)) {
>>>> cs_etm__dump_event(etm, buffer);
>>>> auxtrace_buffer__put_data(buffer);
>>>> }
>>>> }
>>>>
>>>> If so, it's better to use a new patch to polish the code.
>>>>
>>>
>>> Hi Leo,
>>>
>>> I think this is not true in piped mode because there is no auxtrace index.
>>> In that mode, events are processed only in file order and cs_etm__process_auxtrace_event()
>>> is called for each buffer.
>>>
>>> You can reproduce this with something like this:
>>>
>>> ./perf record -o - ls > stdio.data
>>> cat stdio.data | ./perf report -i -
>>
>> You are right! I tried these two commands with cs_etm event, just as
>> you said, in this case, the AUX trace data is not queued; so the flow
>> for "if (!etm->data_queued)" should be kept. If so, I am very fine
>> for current change. Thanks for sharing the knowledge.
>>
>>> There are some other Coresight features that don't work as expected in this mode, like
>>> sorting timestamps between CPUs. The aux split patchset won't work either because random
>>> access isn't possible. And the TRBE patch that I'm working on now won't work, because it
>>> also requires the random access to lookup the flags on the AUX record to configure the
>>> decoder for unformatted trace.
>>
>
> There is a lot of things happening in this area. Based on the above should I
> still plan to review this set or should I wait for another revision?
From my point of view, this one is final. It looks like both Leo and I have tested
it with and without his snapshot changes and it's working as expected in both cases.
Thanks
James
>
> Thanks,
> Mathieu
>
>> Cool, looking forward for the patches :)
>>
>> Leo
next prev parent reply other threads:[~2021-06-29 8:52 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-24 16:43 [PATCH v7 0/2] perf cs-etm: Split Coresight decode by aux records James Clark
2021-06-24 16:43 ` [PATCH v7 1/2] " James Clark
2021-06-27 11:50 ` Leo Yan
2021-07-05 19:33 ` Mathieu Poirier
2021-06-24 16:43 ` [PATCH v7 2/2] perf cs-etm: Split --dump-raw-trace by AUX records James Clark
2021-06-28 1:27 ` Leo Yan
2021-06-28 10:38 ` James Clark
2021-06-28 12:08 ` Leo Yan
2021-06-28 20:01 ` Mathieu Poirier
2021-06-29 6:09 ` Leo Yan
2021-06-29 8:52 ` James Clark [this message]
2021-06-29 19:08 ` Mathieu Poirier
2021-07-05 19:33 ` Mathieu Poirier
2021-07-14 17:45 ` Arnaldo Carvalho de Melo
2021-07-19 16:33 ` Mathieu Poirier
2021-07-19 20:10 ` Arnaldo Carvalho de Melo
2021-07-20 15:15 ` 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=b9a3ae95-987b-d1de-a242-141e7444aa83@arm.com \
--to=james.clark@arm.com \
--cc=acme@kernel.org \
--cc=al.grant@arm.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=anshuman.khandual@arm.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=linux-perf-users@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).