From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753915AbcGFMeR (ORCPT <rfc822;w@1wt.eu>);
	Wed, 6 Jul 2016 08:34:17 -0400
Received: from mx1.redhat.com ([209.132.183.28]:60317 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751007AbcGFMeQ (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Wed, 6 Jul 2016 08:34:16 -0400
Date: Wed, 6 Jul 2016 14:34:11 +0200
From: Jiri Olsa <jolsa@redhat.com>
To: "Wangnan (F)" <wangnan0@huawei.com>
Cc: acme@kernel.org, linux-kernel@vger.kernel.org, pi3orama@163.com,
        lizefan@huawei.com, He Kuang <hekuang@huawei.com>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Jiri Olsa <jolsa@kernel.org>, Masami Hiramatsu <mhiramat@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>, Nilay Vaish <nilayvaish@gmail.com>
Subject: Re: [PATCH v13 5/8] perf record: Read from overwritable ring buffer
Message-ID: <20160706123411.GF26517@krava>
References: <1467613209-191781-1-git-send-email-wangnan0@huawei.com>
 <1467613209-191781-6-git-send-email-wangnan0@huawei.com>
 <20160706113857.GD26517@krava>
 <577CF390.2090700@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <577CF390.2090700@huawei.com>
User-Agent: Mutt/1.6.1 (2016-04-27)
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Wed, 06 Jul 2016 12:34:15 +0000 (UTC)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Jul 06, 2016 at 08:03:28PM +0800, Wangnan (F) wrote:
> 
> 
> On 2016/7/6 19:38, Jiri Olsa wrote:
> > On Mon, Jul 04, 2016 at 06:20:06AM +0000, Wang Nan wrote:
> > 
> > SNIP
> > 
> > > +static void
> > > +record__toggle_overwrite_evsels(struct record *rec,
> > > +				enum overwrite_evt_state state)
> > > +{
> > > +	struct perf_evlist *evlist = rec->overwrite_evlist;
> > > +	enum overwrite_evt_state old_state = rec->overwrite_evt_state;
> > > +	enum action {
> > > +		NONE,
> > > +		PAUSE,
> > > +		RESUME,
> > > +	} action = NONE;
> > > +
> > > +	switch (old_state) {
> > > +	case OVERWRITE_EVT_RUNNING: {
> > > +		switch (state) {
> > > +		case OVERWRITE_EVT_DATA_PENDING:
> > > +			action = PAUSE;
> > > +			break;
> > > +		case OVERWRITE_EVT_RUNNING:
> > > +		case OVERWRITE_EVT_EMPTY:
> > > +		default:
> > > +			goto state_err;
> > > +		}
> > > +		break;
> > > +	}
> > > +	case OVERWRITE_EVT_DATA_PENDING: {
> > > +		switch (state) {
> > > +		case OVERWRITE_EVT_EMPTY:
> > > +			break;
> > > +		case OVERWRITE_EVT_RUNNING:
> > > +		case OVERWRITE_EVT_DATA_PENDING:
> > > +		default:
> > > +			goto state_err;
> > > +		}
> > > +		break;
> > > +	}
> > > +	case OVERWRITE_EVT_EMPTY: {
> > > +		switch (state) {
> > > +		case OVERWRITE_EVT_RUNNING:
> > > +			action = RESUME;
> > > +			break;
> > > +		case OVERWRITE_EVT_EMPTY:
> > > +		case OVERWRITE_EVT_DATA_PENDING:
> > > +		default:
> > > +			goto state_err;
> > > +		}
> > > +		break;
> > > +	}
> > > +	default:
> > > +		WARN_ONCE(1, "Shouldn't get there\n");
> > > +	}
> > > +
> > > +	rec->overwrite_evt_state = state;
> > > +
> > > +	if (!evlist)
> > > +		return;
> > I'd expect this check at the begining
> 
> I think even evlist is NULL the state changing is still required.
> Actually, the state machine is independent with aux evlist. Even
> we without overwritable evsels the state machine is still valid.
> So let the state machine runs unconditionally.

hum, can't see that.. it's state machine to govern overwrite evlist, right?
if there's no overwrite evlist we should keep the current processing

if it's meant to govern the mmap reading in general
we should at least rename it

jirka