Re: [PATCH 02/10] perf tool: fix: Don't discard prev in backward mode

[Wangnan (F)]

On 2017/10/11 2:23, Wangnan (F) wrote:
> On 2017/10/11 1:20, kan.liang@xxxxxxxxx wrote:
> > From: Kan Liang <kan.liang@xxxxxxxxx>
> >
> > Perf record can switch output. The new output should only store the data
> > after switching. However, in overwrite backward mode, the new output
> > still have the data from old output.
> >
> > At the end of mmap_read, the position of processed ring buffer is saved
> > in md->prev. Next mmap_read should be end in md->prev.
> > However, the md->prev is discarded. So next mmap_read has to process
> > whole valid ring buffer, which definitely include the old processed
> > data.
> >
> > Set the prev as the end of the range in backward mode.
> >
> > Signed-off-by: Kan Liang <kan.liang@xxxxxxxxx>
> > ---
> >   tools/perf/util/evlist.c | 14 +++++++++++++-
> >   1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 33b8837..7d23cf5 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -742,13 +742,25 @@ static int
> >   rb_find_range(void *data, int mask, u64 head, u64 old,
> >             u64 *start, u64 *end, bool backward)
> >   {
> > +    int ret;
> > +
> >       if (!backward) {
> >           *start = old;
> >           *end = head;
> >           return 0;
> >       }
> >   -    return backward_rb_find_range(data, mask, head, start, end);
> > +    ret = backward_rb_find_range(data, mask, head, start, end);
> > +
> > +    /*
> > +     * The start and end from backward_rb_find_range is the range 
> > for all
> > +     * valid data in ring buffer.
> > +     * However, part of the data is processed previously.
> > +     * Reset the end to drop the processed data
> > +     */
> > +    *end = old;
> > +

[SNIP]

> If you really want to avoid record duplication, you need to changes 
> record__mmap_read()'s
> logic. Now it complains "failed to keep up with mmap data" and avoid 
> dumping data when
> size of newly generated data is larger than the size of the ring 
> buffer. It is reasonable
> for forward ring buffer because in this case you lost the head of the 
> first record, the
> whole ring buffer is unparseable. However, it is wrong in backward 
> case. What you
> should do in this case is dumping the whole ring buffer.

I think what you want should be something like this: (not tested)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ee7d0a8..f621a8e 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -173,7 +173,9 @@ rb_find_range(void *data, int mask, u64 head, u64 old,
         return 0;
     }

-    return backward_rb_find_range(data, mask, head, start, end);
+    *start = head;
+    *end = old;
+    return 0;
 }

 static int
@@ -199,10 +201,15 @@ record__mmap_read(struct record *rec, struct 
perf_mmap *md,

     size = end - start;
     if (size > (unsigned long)(md->mask) + 1) {
-        WARN_ONCE(1, "failed to keep up with mmap data. (warn only 
once)\n");
+        if (!backward) {
+            WARN_ONCE(1, "failed to keep up with mmap data. (warn only 
once)\n");

-        md->prev = head;
-        perf_mmap__consume(md, overwrite || backward);
+            md->prev = head;
+            perf_mmap__consume(md, overwrite || backward);
+        } else {
+            backward_rb_find_range(data, md->mask, head, start, end);
+            /* FIXME: error processing */
+        }
         return 0;
     }


Use 'head' and 'old' to locate data position in ring buffer by default. 
If overwrite happen, use
backward_rb_find_range() to fetch the last available record and dump the 
whole ring buffer.

Thank you.