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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Oct 2017 18:28:18 +0000
From:   "Liang, Kan" <kan.liang@...el.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
CC:     "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jolsa@...nel.org" <jolsa@...nel.org>,
        "wangnan0@...wei.com" <wangnan0@...wei.com>,
        "hekuang@...wei.com" <hekuang@...wei.com>,
        "namhyung@...nel.org" <namhyung@...nel.org>,
        "alexander.shishkin@...ux.intel.com" 
        <alexander.shishkin@...ux.intel.com>,
        "Hunter, Adrian" <adrian.hunter@...el.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>
Subject: RE: [PATCH 03/10] perf tool: new iterfaces to read event from ring
 buffer



> Em Tue, Oct 10, 2017 at 10:20:16AM -0700, kan.liang@...el.com escreveu:
> > From: Kan Liang <kan.liang@...el.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.
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@...el.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