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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
> ---
> 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
>