[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 11 Oct 2017 02:50:15 +0800
From: "Wangnan (F)" <wangnan0@...wei.com>
To: <kan.liang@...el.com>, <acme@...nel.org>, <peterz@...radead.org>,
<mingo@...hat.com>, <linux-kernel@...r.kernel.org>
CC: <jolsa@...nel.org>, <hekuang@...wei.com>, <namhyung@...nel.org>,
<alexander.shishkin@...ux.intel.com>, <adrian.hunter@...el.com>,
<ak@...ux.intel.com>
Subject: Re: [PATCH 02/10] perf tool: fix: Don't discard prev in backward mode
On 2017/10/11 2:23, Wangnan (F) wrote:
>
>
> On 2017/10/11 1:20, kan.liang@...el.com wrote:
>> From: Kan Liang <kan.liang@...el.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@...el.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.
Powered by blists - more mailing lists