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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Tue, Oct 10, 2017 at 03:34:55PM -0300, Arnaldo Carvalho de Melo escreveu:
> 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@intel.com escreveu:
> > > > From: Kan Liang <kan.liang@intel.com>
> > > >
> > > > 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.

Wang, can you take a look at these two issues? 

- Arnaldo

> 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@intel.com>
> > > > ---
> > > >  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