| Message ID | 20210528161552.654907-1-leo.yan@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | coresight: Fix for snapshot mode | expand |
Hi Leo, On Fri, May 28, 2021 at 9:16 AM Leo Yan <leo.yan@linaro.org> 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 <denik@chromium.org> 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 <leo.yan@linaro.org> 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 <suzuki.poulose@arm.com> 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 <suzuki.poulose@arm.com> 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 <leo.yan@linaro.org> 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 <suzuki.poulose@arm.com> 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
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(-)