[4/5] perf tools: Properly set the value of 'old' in snapshot mode
diff mbox series

Message ID 20190501175052.29667-5-mathieu.poirier@linaro.org
State New
Headers show
Series
  • coresight: Fix snapshot mode
Related show

Commit Message

Mathieu Poirier May 1, 2019, 5:50 p.m. UTC
In snapshot mode the value of the 'old' pointer needs to be adjusted when
'head' has wrapped around in order to get the latest information in the
buffer and be compatible with the generic AUX ring buffer mechanic.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Leo Yan May 7, 2019, 8:44 a.m. UTC | #1
On Wed, May 01, 2019 at 11:50:51AM -0600, Mathieu Poirier wrote:
> In snapshot mode the value of the 'old' pointer needs to be adjusted when
> 'head' has wrapped around in order to get the latest information in the
> buffer and be compatible with the generic AUX ring buffer mechanic.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  tools/perf/arch/arm/util/cs-etm.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> index 911426721170..4e73fe1a6978 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -541,11 +541,19 @@ static int cs_etm_find_snapshot(struct auxtrace_record *itr __maybe_unused,
>  				unsigned char *data __maybe_unused,
>  				u64 *head, u64 *old)
>  {
> +	bool wrapped;
> +
>  	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);
>  
> -	*old = *head;
> -	*head += mm->len;
> +	/*
> +	 * If the last byte in the ring buffer isn't zero, the head has
> +	 * wrapped around.
> +	 */
> +	wrapped = !!(data[mm->len - 1]);

This is confused for me since I can think out two cases might break
this checking.

The first case is the trace data stream might be zero at the end of the
buffer; the second case is that the buffer is not really wrapped around
at this time but the end of buffer contains the stale data by previous
time.

Could you confirm both cases will not happen?

Will do more testing for this patch set.

Thanks,
Leo Yan

> +
> +	if (wrapped)
> +		*old = *head - mm->len;
>  
>  	return 0;
>  }
> -- 
> 2.17.1
>
Mathieu Poirier May 7, 2019, 5:59 p.m. UTC | #2
On Tue, 7 May 2019 at 02:44, Leo Yan <leo.yan@linaro.org> wrote:
>
> On Wed, May 01, 2019 at 11:50:51AM -0600, Mathieu Poirier wrote:
> > In snapshot mode the value of the 'old' pointer needs to be adjusted when
> > 'head' has wrapped around in order to get the latest information in the
> > buffer and be compatible with the generic AUX ring buffer mechanic.
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  tools/perf/arch/arm/util/cs-etm.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> > index 911426721170..4e73fe1a6978 100644
> > --- a/tools/perf/arch/arm/util/cs-etm.c
> > +++ b/tools/perf/arch/arm/util/cs-etm.c
> > @@ -541,11 +541,19 @@ static int cs_etm_find_snapshot(struct auxtrace_record *itr __maybe_unused,
> >                               unsigned char *data __maybe_unused,
> >                               u64 *head, u64 *old)
> >  {
> > +     bool wrapped;
> > +
> >       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);
> >
> > -     *old = *head;
> > -     *head += mm->len;
> > +     /*
> > +      * If the last byte in the ring buffer isn't zero, the head has
> > +      * wrapped around.
> > +      */
> > +     wrapped = !!(data[mm->len - 1]);
>
> This is confused for me since I can think out two cases might break
> this checking.
>
> The first case is the trace data stream might be zero at the end of the
> buffer;

I just realized there is a better way to do this - since "*head" is
continiously incrementing I will simply compare it to mm->len.  If it
is equal of bigger, the head has wrapped around.

>the second case is that the buffer is not really wrapped around
> at this time but the end of buffer contains the stale data by previous
> time.

That would mean the snapshots were really close together.  In that
case we'd simply get the tail end of the previous snapshot, which is
fine since it is was close enough that we do want that data.

>
> Could you confirm both cases will not happen?
>
> Will do more testing for this patch set.
>
> Thanks,
> Leo Yan
>
> > +
> > +     if (wrapped)
> > +             *old = *head - mm->len;
> >
> >       return 0;
> >  }
> > --
> > 2.17.1
> >

Patch
diff mbox series

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index 911426721170..4e73fe1a6978 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -541,11 +541,19 @@  static int cs_etm_find_snapshot(struct auxtrace_record *itr __maybe_unused,
 				unsigned char *data __maybe_unused,
 				u64 *head, u64 *old)
 {
+	bool wrapped;
+
 	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);
 
-	*old = *head;
-	*head += mm->len;
+	/*
+	 * If the last byte in the ring buffer isn't zero, the head has
+	 * wrapped around.
+	 */
+	wrapped = !!(data[mm->len - 1]);
+
+	if (wrapped)
+		*old = *head - mm->len;
 
 	return 0;
 }