From: "Liang, Kan" <kan.liang@intel.com>
To: Jiri Olsa <jolsa@redhat.com>
CC: "acme@kernel.org" <acme@kernel.org>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "wangnan0@huawei.com" <wangnan0@huawei.com>,
        "jolsa@kernel.org" <jolsa@kernel.org>,
        "namhyung@kernel.org" <namhyung@kernel.org>,
        "ak@linux.intel.com" <ak@linux.intel.com>,
        "yao.jin@linux.intel.com" <yao.jin@linux.intel.com>
Subject: RE: [PATCH V4 02/15] perf mmap: introduce perf_mmap__read_init()
Thread-Topic: [PATCH V4 02/15] perf mmap: introduce perf_mmap__read_init()
Thread-Index: AQHTjj57lzEqAmelqkaOAN4u//v1JaN19MKAgADWmxA=
Date: Tue, 16 Jan 2018 18:12:41 +0000
Message-ID: <37D7C6CF3E00A74B8858931C1DB2F07753800FF1@SHSMSX103.ccr.corp.intel.com>
References: <1516047651-164336-1-git-send-email-kan.liang@intel.com>
 <1516047651-164336-3-git-send-email-kan.liang@intel.com>
 <20180116131237.GG26643@krava>
In-Reply-To: <20180116131237.GG26643@krava>
Accept-Language: zh-CN, en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-version: 11.0.0.116
dlp-reaction: no-action
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org

> On Mon, Jan 15, 2018 at 12:20:38PM -0800, kan.liang@intel.com wrote:
> > From: Kan Liang <kan.liang@intel.com>
> >
> > The perf record has specific codes to calculate the ringbuffer position
> > for both overwrite and non-overwrite mode. Now, only perf record
> > supports both modes. The perf top will support both modes later.
> > It is useful to make the specific codes generic.
> >
> > Introduce a new interface perf_mmap__read_init() to find ringbuffer
> > position. The perf_mmap__read_init() is actually factored out from
> > perf_mmap__push().
> > There are slight differences.
> > - Add a check for map->refcnt
> > - Add new return value logic, EAGAIN and EINVAL.
> 
> not helpful.. I asked to separate those changes,
> so we can clearly see what the refcnt check for
> and what's behing that EAGAIN return
> 
> please add separate:
>   1) patch that adds perf_mmap__read_init into perf_mmap__push
>      with no functional change
>   2) patch adds adds and explain the refcnt check
>   3) patch that adds and explain the EAGAIN return
>

Oh I see. I have misunderstood before.
I will change it in V5.

Thanks,
Kan

 
> thanks,
> jirka
> 
> >
> > Signed-off-by: Kan Liang <kan.liang@intel.com>
> > ---
> >  tools/perf/util/mmap.c | 43
> +++++++++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/mmap.h |  2 ++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
> > index 05076e6..414089f 100644
> > --- a/tools/perf/util/mmap.c
> > +++ b/tools/perf/util/mmap.c
> > @@ -267,6 +267,49 @@ static int overwrite_rb_find_range(void *buf, int
> mask, u64 head, u64 *start, u6
> >  	return -1;
> >  }
> >
> > +/*
> > + * Report the start and end of the available data in ringbuffer
> > + */
> > +int perf_mmap__read_init(struct perf_mmap *map, bool overwrite,
> > +			 u64 *startp, u64 *endp)
> > +{
> > +	unsigned char *data = map->base + page_size;
> > +	u64 head = perf_mmap__read_head(map);
> > +	u64 old = map->prev;
> > +	unsigned long size;
> > +
> > +	/*
> > +	 * Check if event was unmapped due to a POLLHUP/POLLERR.
> > +	 */
> > +	if (!refcount_read(&map->refcnt))
> > +		return -EINVAL;
> > +
> > +	*startp = overwrite ? head : old;
> > +	*endp = overwrite ? old : head;
> > +
> > +	if (*startp == *endp)
> > +		return -EAGAIN;
> > +
> > +	size = *endp - *startp;
> > +	if (size > (unsigned long)(map->mask) + 1) {
> > +		if (!overwrite) {
> > +			WARN_ONCE(1, "failed to keep up with mmap data.
> (warn only once)\n");
> > +
> > +			map->prev = head;
> > +			perf_mmap__consume(map, overwrite);
> > +			return -EAGAIN;
> > +		}
> > +
> > +		/*
> > +		 * Backward ring buffer is full. We still have a chance to read
> > +		 * most of data from it.
> > +		 */
> > +		if (overwrite_rb_find_range(data, map->mask, head, startp,
> endp))
> > +			return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> >  int perf_mmap__push(struct perf_mmap *md, bool overwrite,
> >  		    void *to, int push(void *to, void *buf, size_t size))
> >  {
> > diff --git a/tools/perf/util/mmap.h b/tools/perf/util/mmap.h
> > index e43d7b5..0633308 100644
> > --- a/tools/perf/util/mmap.h
> > +++ b/tools/perf/util/mmap.h
> > @@ -94,4 +94,6 @@ int perf_mmap__push(struct perf_mmap *md, bool
> backward,
> >
> >  size_t perf_mmap__mmap_len(struct perf_mmap *map);
> >
> > +int perf_mmap__read_init(struct perf_mmap *map, bool overwrite,
> > +			 u64 *startp, u64 *endp);
> >  #endif /*__PERF_MMAP_H */
> > --
> > 2.5.5
> >