Messages in this thread Patch in this message | | | Subject | Re: [PATCH 02/10] perf tool: fix: Don't discard prev in backward mode | From | "Wangnan (F)" <> | Date | Wed, 11 Oct 2017 02:50:15 +0800 |
| |
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.
| |