[PATCH] coresight: tmc-etr: Fix updating buffer in not-snapshot mode.

[PATCH] coresight: tmc-etr: Fix updating buffer in not-snapshot mode.

[Yabin Cui]

TMC etr always copies all available data to perf aux buffer, which
may exceed the available space in perf aux buffer. It isn't suitable
for not-snapshot mode, because:
1) It may overwrite previously written data.
2) It may make the perf_event_mmap_page->aux_head report having more
or less data than the reality.

Signed-off-by: Yabin Cui <yabinc@google.com>
---
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 17006705287a..697e68d492af 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1410,9 +1410,10 @@ static void tmc_free_etr_buffer(void *config)
  * tmc_etr_sync_perf_buffer: Copy the actual trace data from the hardware
  * buffer to the perf ring buffer.
  */
-static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf)
+static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf,
+				     unsigned long to_copy)
 {
-	long bytes, to_copy;
+	long bytes;
 	long pg_idx, pg_offset, src_offset;
 	unsigned long head = etr_perf->head;
 	char **dst_pages, *src_buf;
@@ -1423,7 +1424,6 @@ static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf)
 	pg_offset = head & (PAGE_SIZE - 1);
 	dst_pages = (char **)etr_perf->pages;
 	src_offset = etr_buf->offset;
-	to_copy = etr_buf->len;
 
 	while (to_copy > 0) {
 		/*
@@ -1501,7 +1501,11 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	size = etr_buf->len;
-	tmc_etr_sync_perf_buffer(etr_perf);
+	if (!etr_perf->snapshot && size > handle->size) {
+		size = handle->size;
+		lost = true;
+	}
+	tmc_etr_sync_perf_buffer(etr_perf, size);
 
 	/*
 	 * In snapshot mode we simply increment the head by the number of byte
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH] coresight: tmc-etr: Fix updating buffer in not-snapshot mode.

[Suzuki K Poulose]

Hi Yabin

On 06/08/2019 00:37, Yabin Cui wrote:
> TMC etr always copies all available data to perf aux buffer, which
> may exceed the available space in perf aux buffer. It isn't suitable
> for not-snapshot mode, because:
> 1) It may overwrite previously written data.
> 2) It may make the perf_event_mmap_page->aux_head report having more
> or less data than the reality.

Thanks for debugging and the fix.

> Signed-off-by: Yabin Cui <yabinc@google.com>
> ---
>   drivers/hwtracing/coresight/coresight-tmc-etr.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 17006705287a..697e68d492af 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1410,9 +1410,10 @@ static void tmc_free_etr_buffer(void *config)
>    * tmc_etr_sync_perf_buffer: Copy the actual trace data from the hardware
>    * buffer to the perf ring buffer.
>    */
> -static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf)
> +static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf,
> +				     unsigned long to_copy)
>   {
> -	long bytes, to_copy;
> +	long bytes;
>   	long pg_idx, pg_offset, src_offset;
>   	unsigned long head = etr_perf->head;
>   	char **dst_pages, *src_buf;
> @@ -1423,7 +1424,6 @@ static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf)
>   	pg_offset = head & (PAGE_SIZE - 1);
>   	dst_pages = (char **)etr_perf->pages;
>   	src_offset = etr_buf->offset;
> -	to_copy = etr_buf->len;
>   
>   	while (to_copy > 0) {
>   		/*
> @@ -1501,7 +1501,11 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
>   	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>   
>   	size = etr_buf->len;
> -	tmc_etr_sync_perf_buffer(etr_perf);
> +	if (!etr_perf->snapshot && size > handle->size) {
> +		size = handle->size;
> +		lost = true;
> +	}
> +	tmc_etr_sync_perf_buffer(etr_perf, size);

The patch as such looks good to me.
I was thinking if we should limit the ETR buffer to the perf ring buffer,
in case the perf buffer is smaller than the ETR configured size.
Irrespective of snapshot mode or not, we can't save more than what the ring
buffer offers us with. Even though for the cpu-wide trace, we could have "n"
such ring buffers, we end up using only one ring buffer(the last one scheduled 
out). May be we could explore if we could improve the handling of large buffer
into multiple ring buffers (which may require perf core changes).

Anyways, for this patch:

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Re: [PATCH] coresight: tmc-etr: Fix updating buffer in not-snapshot mode.

[Mathieu Poirier]

Good day Yabin,

With this patch you are addressing a long time itch I had - please read on.

On Mon, 5 Aug 2019 at 17:37, Yabin Cui <yabinc@google.com> wrote:
>
> TMC etr always copies all available data to perf aux buffer, which
> may exceed the available space in perf aux buffer. It isn't suitable
> for not-snapshot mode, because:
> 1) It may overwrite previously written data.
> 2) It may make the perf_event_mmap_page->aux_head report having more
> or less data than the reality.
>
> Signed-off-by: Yabin Cui <yabinc@google.com>
> ---
>  drivers/hwtracing/coresight/coresight-tmc-etr.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 17006705287a..697e68d492af 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1410,9 +1410,10 @@ static void tmc_free_etr_buffer(void *config)
>   * tmc_etr_sync_perf_buffer: Copy the actual trace data from the hardware
>   * buffer to the perf ring buffer.
>   */
> -static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf)
> +static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf,
> +                                    unsigned long to_copy)
>  {
> -       long bytes, to_copy;
> +       long bytes;
>         long pg_idx, pg_offset, src_offset;
>         unsigned long head = etr_perf->head;
>         char **dst_pages, *src_buf;
> @@ -1423,7 +1424,6 @@ static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf)
>         pg_offset = head & (PAGE_SIZE - 1);
>         dst_pages = (char **)etr_perf->pages;
>         src_offset = etr_buf->offset;
> -       to_copy = etr_buf->len;
>
>         while (to_copy > 0) {
>                 /*
> @@ -1501,7 +1501,11 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
>         spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
>         size = etr_buf->len;
> -       tmc_etr_sync_perf_buffer(etr_perf);
> +       if (!etr_perf->snapshot && size > handle->size) {
> +               size = handle->size;
> +               lost = true;
> +       }

Perfect - this is in line with what is done for ETB and ETF.

> +       tmc_etr_sync_perf_buffer(etr_perf, size);

Here tmc_etr_sync_perf_buffer() will copy data to the perf ring buffer
starting at @etr_perf->offset for @size, clipping the _end_ of the
trace data accumulated in the trace buffer.  This is contrary to what
is done for ETB and ETF where the equivalent of @etr_perf->offset is
moved forward (clipping the _beginning_ of the trace data) in order to
keep as much of the end as possible.

I would rather enact the same heuristic here.

Thanks,
Mathieu

>
>         /*
>          * In snapshot mode we simply increment the head by the number of byte
> --
> 2.22.0.770.g0f2c4a37fd-goog
>