[PATCH v2 0/3] coresight: Fix for snapshot mode

[PATCH v2 0/3] coresight: Fix for snapshot mode

[Leo Yan]

This patch series is to correct the pointer usages for the snapshot
mode.

Patch 01 is to polish code, it removes the redundant header maintained
in tmc-etr driver and directly uses pointer perf_output_handle::head.

Patch 02 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.  Patch 03 is
to update comments in CoreSight drivers to reflect the changes
introduced by patch 02.

This patch can be cleanly applied on the mainline kernel with:

  commit dbe69e433722 ("Merge tag 'net-next-5.14' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next")

And it has been tested on Arm64 Juno board.

Changes from v1:
- Dropped the patch "coresight: etm-perf: Correct buffer syncing for
  snapshot", after a long discussion, the patch doesn't really resolve
  any issues for snapshot mode.  And another reason for unlike this
  patch is now the CoreSight and Intel-PT have the consistent behaviour
  (Suzuki/James/Mathieu);
- Added the patch 03 to updates drivers' comments (James);
- Added Suzuki's review tag for patch 01;
- Added James' review and testing tags for patch 02.


Leo Yan (3):
  coresight: tmc-etr: Use perf_output_handle::head for AUX ring buffer
  perf cs-etm: Remove callback cs_etm_find_snapshot()
  coresight: Update comments for removing cs_etm_find_snapshot()

 drivers/hwtracing/coresight/coresight-etb10.c |   2 +-
 .../hwtracing/coresight/coresight-tmc-etf.c   |   2 +-
 .../hwtracing/coresight/coresight-tmc-etr.c   |  12 +-
 tools/perf/arch/arm/util/cs-etm.c             | 133 ------------------
 4 files changed, 6 insertions(+), 143 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] coresight: tmc-etr: Use perf_output_handle::head for AUX ring buffer

[Leo Yan]

When enable the Arm CoreSight PMU event, the context for AUX ring buffer
is prepared in the structure perf_output_handle, and its field "head"
points the head of the AUX ring buffer and it is updated after filling
AUX trace data into buffer.

Current code uses an extra field etr_perf_buffer::head to maintain the
header for the AUX ring buffer which is not necessary; alternatively,
it's better to directly perf_output_handle::head.

This patch removes the field etr_perf_buffer::head and directly used
perf_output_handle::head for the head of AUX ring buffer.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index acdb59e0e661..b22823d67680 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -32,7 +32,6 @@ struct etr_flat_buf {
  * @etr_buf		- Actual buffer used by the ETR
  * @pid			- The PID this etr_perf_buffer belongs to.
  * @snaphost		- Perf session mode
- * @head		- handle->head at the beginning of the session.
  * @nr_pages		- Number of pages in the ring buffer.
  * @pages		- Array of Pages in the ring buffer.
  */
@@ -41,7 +40,6 @@ struct etr_perf_buffer {
 	struct etr_buf		*etr_buf;
 	pid_t			pid;
 	bool			snapshot;
-	unsigned long		head;
 	int			nr_pages;
 	void			**pages;
 };
@@ -1437,16 +1435,16 @@ static void tmc_free_etr_buffer(void *config)
  * buffer to the perf ring buffer.
  */
 static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf,
+				     unsigned long head,
 				     unsigned long src_offset,
 				     unsigned long to_copy)
 {
 	long bytes;
 	long pg_idx, pg_offset;
-	unsigned long head = etr_perf->head;
 	char **dst_pages, *src_buf;
 	struct etr_buf *etr_buf = etr_perf->etr_buf;
 
-	head = etr_perf->head;
+	head = PERF_IDX2OFF(head, etr_perf);
 	pg_idx = head >> PAGE_SHIFT;
 	pg_offset = head & (PAGE_SIZE - 1);
 	dst_pages = (char **)etr_perf->pages;
@@ -1553,7 +1551,7 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
 	/* Insert barrier packets at the beginning, if there was an overflow */
 	if (lost)
 		tmc_etr_buf_insert_barrier_packet(etr_buf, offset);
-	tmc_etr_sync_perf_buffer(etr_perf, offset, size);
+	tmc_etr_sync_perf_buffer(etr_perf, handle->head, offset, size);
 
 	/*
 	 * In snapshot mode we simply increment the head by the number of byte
@@ -1605,8 +1603,6 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
 		goto unlock_out;
 	}
 
-	etr_perf->head = PERF_IDX2OFF(handle->head, etr_perf);
-
 	/*
 	 * No HW configuration is needed if the sink is already in
 	 * use for this session.
-- 
2.25.1

[PATCH v2 2/3] perf cs-etm: Remove callback cs_etm_find_snapshot()

[Leo Yan]

The callback cs_etm_find_snapshot() is invoked for snapshot mode, its
main purpose is to find the correct AUX trace data and returns "head"
and "old" (we can call "old" as "old head") to the caller, the caller
__auxtrace_mmap__read() uses these two pointers to decide the AUX trace
data size.

This patch removes cs_etm_find_snapshot() with below reasons:

- The first thing in cs_etm_find_snapshot() is to check if the head has
  wrapped around, if it is not, directly bails out.  The checking is
  pointless, this is because the "head" and "old" pointers both are
  monotonical increasing so they never wrap around.

- cs_etm_find_snapshot() adjusts the "head" and "old" pointers and
  assumes the AUX ring buffer is fully filled with the hardware trace
  data, so it always subtracts the difference "mm->len" from "head" to
  get "old".  Let's imagine the snapshot is taken in very short
  interval, the tracers only fill a small chunk of the trace data into
  the AUX ring buffer, in this case, it's wrongly to copy the whole the
  AUX ring buffer to perf file.

- As the "head" and "old" pointers are monotonically increased, the
  function __auxtrace_mmap__read() handles these two pointers properly.
  It calculates the reminders for these two pointers, and the size is
  clamped to be never more than "snapshot_size".  We can simply reply on
  the function __auxtrace_mmap__read() to calculate the correct result
  for data copying, it's not necessary to add Arm CoreSight specific
  callback.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: James Clark <james.clark@arm.com>
Tested-by: James Clark <james.clark@arm.com>
---
 tools/perf/arch/arm/util/cs-etm.c | 133 ------------------------------
 1 file changed, 133 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index d942f118d32c..85168d87b2d7 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -38,8 +38,6 @@ struct cs_etm_recording {
 	struct auxtrace_record	itr;
 	struct perf_pmu		*cs_etm_pmu;
 	struct evlist		*evlist;
-	int			wrapped_cnt;
-	bool			*wrapped;
 	bool			snapshot_mode;
 	size_t			snapshot_size;
 };
@@ -734,135 +732,6 @@ static int cs_etm_info_fill(struct auxtrace_record *itr,
 	return 0;
 }
 
-static int cs_etm_alloc_wrapped_array(struct cs_etm_recording *ptr, int idx)
-{
-	bool *wrapped;
-	int cnt = ptr->wrapped_cnt;
-
-	/* Make @ptr->wrapped as big as @idx */
-	while (cnt <= idx)
-		cnt++;
-
-	/*
-	 * Free'ed in cs_etm_recording_free().  Using realloc() to avoid
-	 * cross compilation problems where the host's system supports
-	 * reallocarray() but not the target.
-	 */
-	wrapped = realloc(ptr->wrapped, cnt * sizeof(bool));
-	if (!wrapped)
-		return -ENOMEM;
-
-	wrapped[cnt - 1] = false;
-	ptr->wrapped_cnt = cnt;
-	ptr->wrapped = wrapped;
-
-	return 0;
-}
-
-static bool cs_etm_buffer_has_wrapped(unsigned char *buffer,
-				      size_t buffer_size, u64 head)
-{
-	u64 i, watermark;
-	u64 *buf = (u64 *)buffer;
-	size_t buf_size = buffer_size;
-
-	/*
-	 * We want to look the very last 512 byte (chosen arbitrarily) in
-	 * the ring buffer.
-	 */
-	watermark = buf_size - 512;
-
-	/*
-	 * @head is continuously increasing - if its value is equal or greater
-	 * than the size of the ring buffer, it has wrapped around.
-	 */
-	if (head >= buffer_size)
-		return true;
-
-	/*
-	 * The value of @head is somewhere within the size of the ring buffer.
-	 * This can be that there hasn't been enough data to fill the ring
-	 * buffer yet or the trace time was so long that @head has numerically
-	 * wrapped around.  To find we need to check if we have data at the very
-	 * end of the ring buffer.  We can reliably do this because mmap'ed
-	 * pages are zeroed out and there is a fresh mapping with every new
-	 * session.
-	 */
-
-	/* @head is less than 512 byte from the end of the ring buffer */
-	if (head > watermark)
-		watermark = head;
-
-	/*
-	 * Speed things up by using 64 bit transactions (see "u64 *buf" above)
-	 */
-	watermark >>= 3;
-	buf_size >>= 3;
-
-	/*
-	 * If we find trace data at the end of the ring buffer, @head has
-	 * been there and has numerically wrapped around at least once.
-	 */
-	for (i = watermark; i < buf_size; i++)
-		if (buf[i])
-			return true;
-
-	return false;
-}
-
-static int cs_etm_find_snapshot(struct auxtrace_record *itr,
-				int idx, struct auxtrace_mmap *mm,
-				unsigned char *data,
-				u64 *head, u64 *old)
-{
-	int err;
-	bool wrapped;
-	struct cs_etm_recording *ptr =
-			container_of(itr, struct cs_etm_recording, itr);
-
-	/*
-	 * Allocate memory to keep track of wrapping if this is the first
-	 * time we deal with this *mm.
-	 */
-	if (idx >= ptr->wrapped_cnt) {
-		err = cs_etm_alloc_wrapped_array(ptr, idx);
-		if (err)
-			return err;
-	}
-
-	/*
-	 * Check to see if *head has wrapped around.  If it hasn't only the
-	 * amount of data between *head and *old is snapshot'ed to avoid
-	 * bloating the perf.data file with zeros.  But as soon as *head has
-	 * wrapped around the entire size of the AUX ring buffer it taken.
-	 */
-	wrapped = ptr->wrapped[idx];
-	if (!wrapped && cs_etm_buffer_has_wrapped(data, mm->len, *head)) {
-		wrapped = true;
-		ptr->wrapped[idx] = true;
-	}
-
-	pr_debug3("%s: mmap index %d old head %zu new head %zu size %zu\n",
-		  __func__, idx, (size_t)*old, (size_t)*head, mm->len);
-
-	/* No wrap has occurred, we can just use *head and *old. */
-	if (!wrapped)
-		return 0;
-
-	/*
-	 * *head has wrapped around - adjust *head and *old to pickup the
-	 * entire content of the AUX buffer.
-	 */
-	if (*head >= mm->len) {
-		*old = *head - mm->len;
-	} else {
-		*head += mm->len;
-		*old = *head - mm->len;
-	}
-
-	return 0;
-}
-
 static int cs_etm_snapshot_start(struct auxtrace_record *itr)
 {
 	struct cs_etm_recording *ptr =
@@ -900,7 +769,6 @@ static void cs_etm_recording_free(struct auxtrace_record *itr)
 	struct cs_etm_recording *ptr =
 			container_of(itr, struct cs_etm_recording, itr);
 
-	zfree(&ptr->wrapped);
 	free(ptr);
 }
 
@@ -928,7 +796,6 @@ struct auxtrace_record *cs_etm_record_init(int *err)
 	ptr->itr.recording_options	= cs_etm_recording_options;
 	ptr->itr.info_priv_size		= cs_etm_info_priv_size;
 	ptr->itr.info_fill		= cs_etm_info_fill;
-	ptr->itr.find_snapshot		= cs_etm_find_snapshot;
 	ptr->itr.snapshot_start		= cs_etm_snapshot_start;
 	ptr->itr.snapshot_finish	= cs_etm_snapshot_finish;
 	ptr->itr.reference		= cs_etm_reference;
-- 
2.25.1

[PATCH v2 3/3] coresight: Update comments for removing cs_etm_find_snapshot()

[Leo Yan]

Since the function cs_etm_find_snapshot() has been removed in the user
space, it directly uses the perf common function __auxtrace_mmap__read()
to calcualte the head and size for AUX trace data in snapshot mode.

Updates the comments in drivers to reflect the changes.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etb10.c   | 2 +-
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 2 +-
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index f775cbee12b8..1cdb627d6c38 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -557,7 +557,7 @@ static unsigned long etb_update_buffer(struct coresight_device *csdev,
 
 	/*
 	 * In snapshot mode we simply increment the head by the number of byte
-	 * that were written.  User space function  cs_etm_find_snapshot() will
+	 * that were written.  User space function __auxtrace_mmap__read() will
 	 * figure out how many bytes to get from the AUX buffer based on the
 	 * position of the head.
 	 */
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 45b85edfc690..fec8ef3694cf 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -546,7 +546,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
 
 	/*
 	 * In snapshot mode we simply increment the head by the number of byte
-	 * that were written.  User space function  cs_etm_find_snapshot() will
+	 * that were written.  User space function __auxtrace_mmap__read() will
 	 * figure out how many bytes to get from the AUX buffer based on the
 	 * position of the head.
 	 */
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index b22823d67680..960515e01171 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1555,7 +1555,7 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
 
 	/*
 	 * In snapshot mode we simply increment the head by the number of byte
-	 * that were written.  User space function  cs_etm_find_snapshot() will
+	 * that were written.  User space function __auxtrace_mmap__read() will
 	 * figure out how many bytes to get from the AUX buffer based on the
 	 * position of the head.
 	 */
-- 
2.25.1

Re: [PATCH v2 2/3] perf cs-etm: Remove callback cs_etm_find_snapshot()

[Arnaldo Carvalho de Melo]

Em Thu, Jul 01, 2021 at 05:35:36PM +0800, Leo Yan escreveu:
> The callback cs_etm_find_snapshot() is invoked for snapshot mode, its
> main purpose is to find the correct AUX trace data and returns "head"
> and "old" (we can call "old" as "old head") to the caller, the caller
> __auxtrace_mmap__read() uses these two pointers to decide the AUX trace
> data size.
> 
> This patch removes cs_etm_find_snapshot() with below reasons:
> 
> - The first thing in cs_etm_find_snapshot() is to check if the head has
>   wrapped around, if it is not, directly bails out.  The checking is
>   pointless, this is because the "head" and "old" pointers both are
>   monotonical increasing so they never wrap around.
> 
> - cs_etm_find_snapshot() adjusts the "head" and "old" pointers and
>   assumes the AUX ring buffer is fully filled with the hardware trace
>   data, so it always subtracts the difference "mm->len" from "head" to
>   get "old".  Let's imagine the snapshot is taken in very short
>   interval, the tracers only fill a small chunk of the trace data into
>   the AUX ring buffer, in this case, it's wrongly to copy the whole the
>   AUX ring buffer to perf file.
> 
> - As the "head" and "old" pointers are monotonically increased, the
>   function __auxtrace_mmap__read() handles these two pointers properly.
>   It calculates the reminders for these two pointers, and the size is
>   clamped to be never more than "snapshot_size".  We can simply reply on
>   the function __auxtrace_mmap__read() to calculate the correct result
>   for data copying, it's not necessary to add Arm CoreSight specific
>   callback.

Thanks, applied.

- Arnaldo

Re: [PATCH v2 2/3] perf cs-etm: Remove callback cs_etm_find_snapshot()

[Leo Yan]

On Thu, Jul 01, 2021 at 01:25:14PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jul 01, 2021 at 05:35:36PM +0800, Leo Yan escreveu:
> > The callback cs_etm_find_snapshot() is invoked for snapshot mode, its
> > main purpose is to find the correct AUX trace data and returns "head"
> > and "old" (we can call "old" as "old head") to the caller, the caller
> > __auxtrace_mmap__read() uses these two pointers to decide the AUX trace
> > data size.
> > 
> > This patch removes cs_etm_find_snapshot() with below reasons:
> > 
> > - The first thing in cs_etm_find_snapshot() is to check if the head has
> >   wrapped around, if it is not, directly bails out.  The checking is
> >   pointless, this is because the "head" and "old" pointers both are
> >   monotonical increasing so they never wrap around.
> > 
> > - cs_etm_find_snapshot() adjusts the "head" and "old" pointers and
> >   assumes the AUX ring buffer is fully filled with the hardware trace
> >   data, so it always subtracts the difference "mm->len" from "head" to
> >   get "old".  Let's imagine the snapshot is taken in very short
> >   interval, the tracers only fill a small chunk of the trace data into
> >   the AUX ring buffer, in this case, it's wrongly to copy the whole the
> >   AUX ring buffer to perf file.
> > 
> > - As the "head" and "old" pointers are monotonically increased, the
> >   function __auxtrace_mmap__read() handles these two pointers properly.
> >   It calculates the reminders for these two pointers, and the size is
> >   clamped to be never more than "snapshot_size".  We can simply reply on
> >   the function __auxtrace_mmap__read() to calculate the correct result
> >   for data copying, it's not necessary to add Arm CoreSight specific
> >   callback.
> 
> Thanks, applied.

Thanks a lot for picking up the patch, Arnaldo!

Hi Mathieu, I supposed to get your review before merging; since
Arnaldo moves quickly, if you want me to follow up anything relevant
to this change, please let me know.  Thanks!

Leo

Re: [PATCH v2 2/3] perf cs-etm: Remove callback cs_etm_find_snapshot()

[Mathieu Poirier]

On Thu, 1 Jul 2021 at 19:10, Leo Yan <leo.yan@linaro.org> wrote:
>
> On Thu, Jul 01, 2021 at 01:25:14PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Jul 01, 2021 at 05:35:36PM +0800, Leo Yan escreveu:
> > > The callback cs_etm_find_snapshot() is invoked for snapshot mode, its
> > > main purpose is to find the correct AUX trace data and returns "head"
> > > and "old" (we can call "old" as "old head") to the caller, the caller
> > > __auxtrace_mmap__read() uses these two pointers to decide the AUX trace
> > > data size.
> > >
> > > This patch removes cs_etm_find_snapshot() with below reasons:
> > >
> > > - The first thing in cs_etm_find_snapshot() is to check if the head has
> > >   wrapped around, if it is not, directly bails out.  The checking is
> > >   pointless, this is because the "head" and "old" pointers both are
> > >   monotonical increasing so they never wrap around.
> > >
> > > - cs_etm_find_snapshot() adjusts the "head" and "old" pointers and
> > >   assumes the AUX ring buffer is fully filled with the hardware trace
> > >   data, so it always subtracts the difference "mm->len" from "head" to
> > >   get "old".  Let's imagine the snapshot is taken in very short
> > >   interval, the tracers only fill a small chunk of the trace data into
> > >   the AUX ring buffer, in this case, it's wrongly to copy the whole the
> > >   AUX ring buffer to perf file.
> > >
> > > - As the "head" and "old" pointers are monotonically increased, the
> > >   function __auxtrace_mmap__read() handles these two pointers properly.
> > >   It calculates the reminders for these two pointers, and the size is
> > >   clamped to be never more than "snapshot_size".  We can simply reply on
> > >   the function __auxtrace_mmap__read() to calculate the correct result
> > >   for data copying, it's not necessary to add Arm CoreSight specific
> > >   callback.
> >
> > Thanks, applied.
>
> Thanks a lot for picking up the patch, Arnaldo!
>
> Hi Mathieu, I supposed to get your review before merging; since
> Arnaldo moves quickly, if you want me to follow up anything relevant
> to this change, please let me know.  Thanks!

I was going to review this set next week.  But it is fine if Arnaldo
has picked it up since enough people have already looked at and tested
this code.

Thanks,
Mathieu

>
> Leo