Messages in this thread | | | From | "Liang, Kan" <> | Subject | RE: [PATCH 02/10] perf tool: fix: Don't discard prev in backward mode | Date | Tue, 10 Oct 2017 19:50:16 +0000 |
| |
> 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) >
No. That's not what I want. My test code never trigger the WARN_ONCE.
I think you will see the problem, if you simply run the command as below. sudo ./perf record -e cycles:P -C0 --overwrite --switch-output=1s
The output size keep increasing. Because the new output always include the old outputs. What I want is the 'start' and 'end' for the increase, not everything.
> 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. >
| |