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 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.

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

- 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