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

[Arnaldo Carvalho de Melo]

Em Tue, Oct 10, 2017 at 06:28:18PM +0000, Liang, Kan escreveu:
> 
> 
> > 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.

There is no one at the end of your patchkit? Or no user _right now_? If
there is a user now, lemme see... yeah, no user right now, so _that_ is
yet another bug, i.e. it should be used, no? If this is just a left
over, then we should just throw it away, now, its a cleanup.

But if it should've been used right now, then we should fix the above
problem you noticed in one patch, then in another make it be used, this
way we get the current code base fixed.

The rest of the patchkit is something new, paving the way to
multithreading, and that we will be discussing separately from fixing
bugs found while working on this, agreed?

- Arnaldo

> 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