From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932539AbdJJSgc (ORCPT ); Tue, 10 Oct 2017 14:36:32 -0400 Received: from mail.kernel.org ([198.145.29.99]:45746 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932232AbdJJSgb (ORCPT ); Tue, 10 Oct 2017 14:36:31 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2E8E8218C5 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=acme@kernel.org Date: Tue, 10 Oct 2017 15:36:28 -0300 From: Arnaldo Carvalho de Melo To: Wang Nan Cc: "Liang, Kan" , "peterz@infradead.org" , "mingo@redhat.com" , "linux-kernel@vger.kernel.org" , "jolsa@kernel.org" , "wangnan0@huawei.com" , "hekuang@huawei.com" , "namhyung@kernel.org" , "alexander.shishkin@linux.intel.com" , "Hunter, Adrian" , "ak@linux.intel.com" Subject: Re: [PATCH 03/10] perf tool: new iterfaces to read event from ring buffer Message-ID: <20171010183628.GL28623@kernel.org> References: <1507656023-177125-1-git-send-email-kan.liang@intel.com> <1507656023-177125-4-git-send-email-kan.liang@intel.com> <20171010181520.GH28623@kernel.org> <37D7C6CF3E00A74B8858931C1DB2F077537D1B11@SHSMSX103.ccr.corp.intel.com> <20171010183455.GK28623@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171010183455.GK28623@kernel.org> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.9.0 (2017-09-02) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.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 > > > > > > > > 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 > > > > --- > > > > 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