This patch series is to correct the pointer usages for the snapshot
mode.
Patch 01 allows the AUX trace in the free run mode and only syncs the
AUX ring buffer when taking snapshot.
Patch 02 is to polish code, it removes the redundant header maintained
in tmc-etr driver and directly uses pointer perf_output_handle::head.
Patch 03 removes the callback cs_etm_find_snapshot() which wrongly
calculates the buffer headers; we can simply use the perf's common
function __auxtrace_mmap__read() for headers calculation.
This patch can be cleanly applied on the mainline kernel with:
commit 97e5bf604b7a ("Merge branch 'for-5.13-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/dennis/percpu")
And it has been tested on Arm64 Juno board.
Leo Yan (3):
coresight: etm-perf: Correct buffer syncing for snapshot
coresight: tmc-etr: Use perf_output_handle::head for AUX ring buffer
perf cs-etm: Remove callback cs_etm_find_snapshot()
.../hwtracing/coresight/coresight-etm-perf.c | 30 +++-
.../hwtracing/coresight/coresight-etm-perf.h | 2 +
.../hwtracing/coresight/coresight-tmc-etr.c | 10 +-
tools/perf/arch/arm/util/cs-etm.c | 133 ------------------
4 files changed, 32 insertions(+), 143 deletions(-)
--
2.25.1
Hi Leo,
On Fri, May 28, 2021 at 9:16 AM Leo Yan <[email protected]> wrote:
>
> This patch series is to correct the pointer usages for the snapshot
> mode.
>
> Patch 01 allows the AUX trace in the free run mode and only syncs the
> AUX ring buffer when taking snapshot.
>
> Patch 02 is to polish code, it removes the redundant header maintained
> in tmc-etr driver and directly uses pointer perf_output_handle::head.
>
> Patch 03 removes the callback cs_etm_find_snapshot() which wrongly
> calculates the buffer headers; we can simply use the perf's common
> function __auxtrace_mmap__read() for headers calculation.
>
> This patch can be cleanly applied on the mainline kernel with:
>
> commit 97e5bf604b7a ("Merge branch 'for-5.13-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/dennis/percpu")
>
> And it has been tested on Arm64 Juno board.
I have verified the patches on Chrome OS Trogdor device.
They fixed the problem discussed in
https://lists.linaro.org/pipermail/coresight/2021-May/006411.html.
Tested-by: Denis Nikitin <[email protected]>
Thanks,
Denis
>
>
> Leo Yan (3):
> coresight: etm-perf: Correct buffer syncing for snapshot
> coresight: tmc-etr: Use perf_output_handle::head for AUX ring buffer
> perf cs-etm: Remove callback cs_etm_find_snapshot()
>
> .../hwtracing/coresight/coresight-etm-perf.c | 30 +++-
> .../hwtracing/coresight/coresight-etm-perf.h | 2 +
> .../hwtracing/coresight/coresight-tmc-etr.c | 10 +-
> tools/perf/arch/arm/util/cs-etm.c | 133 ------------------
> 4 files changed, 32 insertions(+), 143 deletions(-)
>
> --
> 2.25.1
>
Hi Denis
On 10/06/2021 07:43, Denis Nikitin wrote:
> Hi Leo,
>
> On Fri, May 28, 2021 at 9:16 AM Leo Yan <[email protected]> wrote:
>>
>> This patch series is to correct the pointer usages for the snapshot
>> mode.
>>
>> Patch 01 allows the AUX trace in the free run mode and only syncs the
>> AUX ring buffer when taking snapshot.
>>
>> Patch 02 is to polish code, it removes the redundant header maintained
>> in tmc-etr driver and directly uses pointer perf_output_handle::head.
>>
>> Patch 03 removes the callback cs_etm_find_snapshot() which wrongly
>> calculates the buffer headers; we can simply use the perf's common
>> function __auxtrace_mmap__read() for headers calculation.
>>
>> This patch can be cleanly applied on the mainline kernel with:
>>
>> commit 97e5bf604b7a ("Merge branch 'for-5.13-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/dennis/percpu")
>>
>> And it has been tested on Arm64 Juno board.
>
> I have verified the patches on Chrome OS Trogdor device.
> They fixed the problem discussed in
> https://lists.linaro.org/pipermail/coresight/2021-May/006411.html.
Are you able to confirm if the patch 3 alone fixes the above issue ?
I am not convinced that Patch 1 is necessary.
Suzuki
Hi Suzuki,
On Thu, Jun 10, 2021 at 9:04 AM Suzuki K Poulose <[email protected]> wrote:
>
[...]
>
> Are you able to confirm if the patch 3 alone fixes the above issue ?
> I am not convinced that Patch 1 is necessary.
>
Yes. "perf cs-etm: Remove callback cs_etm_find_snapshot()" alone fixes
the issue.
- Denis
> Suzuki
On Fri, Jun 11, 2021 at 01:31:41AM -0700, Denis Nikitin wrote:
> Hi Suzuki,
>
> On Thu, Jun 10, 2021 at 9:04 AM Suzuki K Poulose <[email protected]> wrote:
> >
> [...]
> >
> > Are you able to confirm if the patch 3 alone fixes the above issue ?
> > I am not convinced that Patch 1 is necessary.
> >
>
> Yes. "perf cs-etm: Remove callback cs_etm_find_snapshot()" alone fixes
> the issue.
Based on current testing result, we should give high priority for
patches 2 and 3.
The patch 1 is controversial for how to handle the trace data kept
in multiple AUX buffers; essentially it's up to how we understand the
snapshot definition. I confirmed Intel-PT and CoreSight have the same
behaviour for capturing trace data from multiple AUX ring buffers when
snapshot occurs.
I'd like to leave patch 1/3 out, and resend it if we get conclusion.
At the meantime, @Denis, if you have observed any profiling result
(or profiling quality) difference caused by patch 1, the feedback would
be very valuable.
Thanks a lot for Denis' testing and insight review from Suzuki!
Leo
Hi Leo,
On Fri, Jun 11, 2021 at 5:27 PM Leo Yan <[email protected]> wrote:
>
> On Fri, Jun 11, 2021 at 01:31:41AM -0700, Denis Nikitin wrote:
> > Hi Suzuki,
> >
> > On Thu, Jun 10, 2021 at 9:04 AM Suzuki K Poulose <[email protected]> wrote:
> > >
> > [...]
> > >
> > > Are you able to confirm if the patch 3 alone fixes the above issue ?
> > > I am not convinced that Patch 1 is necessary.
> > >
> >
> > Yes. "perf cs-etm: Remove callback cs_etm_find_snapshot()" alone fixes
> > the issue.
>
> Based on current testing result, we should give high priority for
> patches 2 and 3.
>
> The patch 1 is controversial for how to handle the trace data kept
> in multiple AUX buffers; essentially it's up to how we understand the
> snapshot definition. I confirmed Intel-PT and CoreSight have the same
> behaviour for capturing trace data from multiple AUX ring buffers when
> snapshot occurs.
>
> I'd like to leave patch 1/3 out, and resend it if we get conclusion.
> At the meantime, @Denis, if you have observed any profiling result
> (or profiling quality) difference caused by patch 1, the feedback would
> be very valuable.
I evaluated AutoFDO profiles with benchmarks but I was only focused
on the system-wide mode. And as I understood patch 1 fixes the issue
in non system-wide mode.
Currently I'm OoO so I won't be able to do further evaluation.
- Denis
>
>
> Thanks a lot for Denis' testing and insight review from Suzuki!
>
> Leo
Hi Denis,
On Sun, Jun 20, 2021 at 10:21:57PM -1000, Denis Nikitin wrote:
[...]
> > I'd like to leave patch 1/3 out, and resend it if we get conclusion.
> > At the meantime, @Denis, if you have observed any profiling result
> > (or profiling quality) difference caused by patch 1, the feedback would
> > be very valuable.
>
> I evaluated AutoFDO profiles with benchmarks but I was only focused
> on the system-wide mode. And as I understood patch 1 fixes the issue
> in non system-wide mode.
Yes, patch 1 doesn't work for the system-wide mode. In the system-wide
mode, CoreSight driver has its own reference counter to only allow the
last CPU to fill trace data to AUX buffer.
> Currently I'm OoO so I won't be able to do further evaluation.
No problem, thanks for the feedback!
Leo