| Message ID | 20190805233738.136357-1-yabinc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: tmc-etr: Fix updating buffer in not-snapshot mode. | expand |
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>
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 >
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
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(-)