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

["Wangnan (F)" <wangnan0@huawei.com>]

On 2017/10/11 2:23, Wangnan (F) wrote:
>
>
> On 2017/10/11 1:20, kan.liang@intel.com wrote:
>> From: Kan Liang <kan.liang@intel.com>
>>
>> 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@intel.com>
>> ---
>>   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.