lkml.org 
[lkml]   [2017]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH 02/10] perf tool: fix: Don't discard prev in backward mode
    Date
    > 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.
    >

    \
     
     \ /
      Last update: 2017-10-10 21:51    [W:4.526 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site