Re: [PATCH v2 3/4] perf tools: Support reading from backward ring buffer

[Wangnan (F)]

On 2016/5/6 21:40, Wangnan (F) wrote:
> On 2016/5/6 4:07, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Apr 27, 2016 at 02:19:22AM +0000, Wang Nan escreveu:
> > > perf_evlist__mmap_read_backward() is introduced for reading backward
> > > ring buffer. Different from reading forward, before reading, caller
> > > needs to call perf_evlist__mmap_read_catchup() first.
> > >
> > > Backward ring buffer should be read from 'head' pointer, not '0'.
> > > perf_evlist__mmap_read_catchup() saves 'head' to 'md->prev', then
> > > make it remember the start position after each reading.
> > >
> > > Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
> > > Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > Cc: Zefan Li <lizefan@xxxxxxxxxx>
> > > Cc: pi3orama@xxxxxxx
> > > ---
> > >   tools/perf/util/evlist.c | 39 +++++++++++++++++++++++++++++++++++++++
> > >   tools/perf/util/evlist.h |  4 ++++
> > >   2 files changed, 43 insertions(+)
> > >
> > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > > index 17cd014..2e0b7b0 100644
> > > --- a/tools/perf/util/evlist.c
> > > +++ b/tools/perf/util/evlist.c
> > > @@ -766,6 +766,45 @@ union perf_event *perf_evlist__mmap_read(struct 
> > > perf_evlist *evlist, int idx)
> > >       return perf_mmap__read(md, evlist->overwrite, old, head, 
> > > &md->prev);
> > >   }
> > >   +union perf_event *
> > > +perf_evlist__mmap_read_backward(struct perf_evlist *evlist, int idx)
> > > +{
> > > +    struct perf_mmap *md = &evlist->mmap[idx];
> > > +    u64 head, end;
> > > +    u64 start = md->prev;
> > > +
> > > +    /*
> > > +     * Check if event was unmapped due to a POLLHUP/POLLERR.
> > > +     */
> > > +    if (!atomic_read(&md->refcnt))
> > > +        return NULL;
> > > +
> > > +    /* NOTE: head is negative in this case */
> > What is this comment for, can you ellaborate? Are you double sure this
> > arithmetic with u64, negative values, that -head below are all ok?
>
> Yes. In backward ring buffer, kernel write data from '0'. Each
> time it write a record, it minus sizeof(record) from 'head'
> pointer. So '-head' means number of bytes already written.
>
> > I've applied the first two patches in this series.
> >
> > I also need to check why we now need that catchup thing :-\
>
> I think catchup is necessary. I tried to get rid of it by setting
> md->prev to -1 and initialize it to 0 when reading forwardly and
> to head when reading backwardly, but it require too much code
> adjustments.

In addtion, reader of backward ring buffer tends to fetch the most recent
record. Since each reading gets earlier record, a catchup function is always
required.

Thank you.
> > > +    head = perf_mmap__read_head(md);
> > > +
> > > +    if (!head)
> > > +        return NULL;
> > > +
> > > +    end = head + md->mask + 1;
> > > +
> > > +    if ((end - head) > -head)
> > > +        end = 0;
> > > +
>
> This '-head' is used to detect wrapping. Never use positive pointer.
>
> > > +    return perf_mmap__read(md, false, start, end, &md->prev);
> > > +}
> > > +
> > > +void perf_evlist__mmap_read_catchup(struct perf_evlist *evlist, int 
> > > idx)
> > > +{
> > > +    struct perf_mmap *md = &evlist->mmap[idx];
> > > +    u64 head;
> > > +
> > > +    if (!atomic_read(&md->refcnt))
> > > +        return;
> > > +
> > > +    head = perf_mmap__read_head(md);
> > > +    md->prev = head;
> > > +}
> > > +
> > >   static bool perf_mmap__empty(struct perf_mmap *md)
> > >   {
> > >       return perf_mmap__read_head(md) == md->prev && 
> > > !md->auxtrace_mmap.base;
> > > diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> > > index 208897a..85d1b59 100644
> > > --- a/tools/perf/util/evlist.h
> > > +++ b/tools/perf/util/evlist.h
> > > @@ -129,6 +129,10 @@ struct perf_sample_id 
> > > *perf_evlist__id2sid(struct perf_evlist *evlist, u64 id);
> > >     union perf_event *perf_evlist__mmap_read(struct perf_evlist 
> > > *evlist, int idx);
> > >   +union perf_event *perf_evlist__mmap_read_backward(struct 
> > > perf_evlist *evlist,
> > > +                          int idx);
> > > +void perf_evlist__mmap_read_catchup(struct perf_evlist *evlist, int 
> > > idx);
> > > +
> > >   void perf_evlist__mmap_consume(struct perf_evlist *evlist, int idx);
> > >     int perf_evlist__open(struct perf_evlist *evlist);
> > > --
> > > 1.8.3.4