RE: [PATCH 03/10] perf tool: new iterfaces to read event from ring buffer

[Liang, Kan]

> Em Tue, Oct 10, 2017 at 10:20:16AM -0700, kan.liang@xxxxxxxxx escreveu:
> > From: Kan Liang <kan.liang@xxxxxxxxx>
> >
> > The perf_evlist__mmap_read only support forward mode. It needs a
> > common function to support both forward and backward mode.
> 
> > The perf_evlist__mmap_read_backward is buggy.
> 
> So, what is the bug? You state that it is buggy, but don't spell out the bug,
> please do so.
> 

union perf_event *perf_evlist__mmap_read_backward(struct perf_evlist *evlist, int idx)
{
	struct perf_mmap *md = &evlist->mmap[idx];  <--- it should be backward_mmap

> If it fixes an existing bug, then it should go separate from this patchkit, right?

There is no one use perf_evlist__mmap_read_backward. So it doesn't trigger any issue.
I just simply drop the codes in patch 10.

Thanks,
Kan
> 
> - Arnaldo
> 
> > Introduce a new mmap read interface to support both forward and
> > backward mode. It can read event one by one from the specific range of
> > ring buffer.
> >
> > The standard process to mmap__read event would be as below.
> >  perf_mmap__read_init
> >  while (event = perf_mmap__read_event) {
> >    //process the event
> >    perf_mmap__consume
> >  }
> >  perf_mmap__read_done
> >
> > The following patches will use it to replace the old interfaces.
> >
> > Signed-off-by: Kan Liang <kan.liang@xxxxxxxxx>
> > ---
> >  tools/perf/util/evlist.c | 41
> > +++++++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/evlist.h |  2 ++
> >  2 files changed, 43 insertions(+)
> >
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index
> > 7d23cf5..b36211e 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -897,6 +897,47 @@ perf_mmap__read(struct perf_mmap *md, bool
> check_messup, u64 start,
> >  	return event;
> >  }
> >
> > +/*
> > + * Read the first event in the specified range of the ring buffer.
> > + * Used by most of the perf tools and tests  */ union perf_event *
> > +perf_mmap__read_event(struct perf_mmap_read *read) {
> > +	struct perf_mmap *md = read->md;
> > +	union perf_event *event;
> > +
> > +	/*
> > +	 * Check if event was unmapped due to a POLLHUP/POLLERR.
> > +	 */
> > +	if (!refcount_read(&md->refcnt))
> > +		return NULL;
> > +
> > +	/* In backward, the ringbuffer is already paused. */
> > +	if (!read->backward) {
> > +		read->end = perf_mmap__read_head(md);
> > +		if (!read->end)
> > +			return NULL;
> > +	}
> > +
> > +	event = perf_mmap__read(md, !read->backward && read-
> >overwrite,
> > +				read->start, read->end, &md->prev);
> > +	read->start = md->prev;
> > +	return event;
> > +}
> > +
> > +/*
> > + * Mandatory for backward
> > + * The direction of backward mmap__read_event is from head to tail.
> > + * The last mmap__read_event will set tail to md->prev.
> > + * Need to correct the md->prev.
> > + */
> > +void
> > +perf_mmap__read_done(struct perf_mmap_read *read) {
> > +	read->md->prev = read->head;
> > +}
> > +
> >  union perf_event *perf_mmap__read_forward(struct perf_mmap *md,
> bool
> > check_messup)  {
> >  	u64 head;
> > diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h index
> > 1ce4857..53baf26 100644
> > --- a/tools/perf/util/evlist.h
> > +++ b/tools/perf/util/evlist.h
> > @@ -207,6 +207,8 @@ int perf_mmap__read_init(struct perf_mmap *md,
> struct perf_mmap_read *read,
> >  			 bool overwrite, bool backward);
> >  int perf_mmap__read_to_file(struct perf_mmap_read *read,
> >  			    struct perf_data_file *file);
> > +union perf_event *perf_mmap__read_event(struct perf_mmap_read
> *read);
> > +void perf_mmap__read_done(struct perf_mmap_read *read);
> >
> >  int perf_evlist__open(struct perf_evlist *evlist);  void
> > perf_evlist__close(struct perf_evlist *evlist);
> > --
> > 2.5.5