Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752727AbcDOQtb (ORCPT <rfc822;w@1wt.eu>);
	Fri, 15 Apr 2016 12:49:31 -0400
Received: from szxga02-in.huawei.com ([119.145.14.65]:6530 "EHLO
	szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752456AbcDOQta (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 15 Apr 2016 12:49:30 -0400
Subject: Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping
To: Arnaldo Carvalho de Melo <acme@kernel.org>,
        Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
References: <1460535673-159866-1-git-send-email-wangnan0@huawei.com>
 <20160415104058.GA2970@krava> <5710C63D.30104@huawei.com>
 <5710D33C.8000006@huawei.com> <20160415130932.GM9056@kernel.org>
 <20160415162654.GR9056@kernel.org>
CC: Jiri Olsa <jolsa@redhat.com>, <linux-kernel@vger.kernel.org>,
        <pi3orama@163.com>, He Kuang <hekuang@huawei.com>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>,
        Namhyung Kim <namhyung@kernel.org>, Zefan Li <lizefan@huawei.com>
From: "Wangnan (F)" <wangnan0@huawei.com>
Message-ID: <57111B64.8030609@huawei.com>
Date: Sat, 16 Apr 2016 00:48:36 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.5.0
MIME-Version: 1.0
In-Reply-To: <20160415162654.GR9056@kernel.org>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-Originating-IP: [10.111.66.109]
X-CFilter-Loop: Reflected
X-Mirapoint-Virus-RAPID-Raw: score=unknown(0),
	refid=str=0001.0A090201.57111B85.00A4,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0,
	ip=0.0.0.0,
	so=2013-06-18 04:22:30,
	dmn=2013-03-21 17:37:32
X-Mirapoint-Loop-Id: 79dc89b13a1f7777bb264453ef10bc8e
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3897
Lines: 105



On 2016/4/16 0:26, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 15, 2016 at 10:09:32AM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Fri, Apr 15, 2016 at 07:40:44PM +0800, Wangnan (F) escreveu:
>>> On 2016/4/15 18:45, Wangnan (F) wrote:
>>>> On 2016/4/15 18:40, Jiri Olsa wrote:
>>>>> On Wed, Apr 13, 2016 at 08:21:03AM +0000, Wang Nan wrote:
>>> [SNIP]
>>>
>>>>> I did not get 3/10 patch and the patchset did not apply cleanly,
>>>>> git am failed.. would you have it in a branch somewhere?
>>>> Sorry, you are not in the CC list. 'git send-email' failed to extract your
>>>> email address from the Acked-by tag.
>>>>
>>>> I'll inform you after I putting them into a git branch. Please wait.
>>>>
>>>> Thank you.
>>> I just realized Arnaldo has already collected these patches set into
>>> his perf/core. Please see it in his git tree [1]. You can also have a look
>>> at my git tree [2] if you want :)
>> I haven't pushed them to Ingo yet, so I can fix up things if Jiri has
>> any fixes or other requests,
> I moved those patches to a separate branch, perf/switch_output, till we get a
> bit more review, I think I was too fast on tentatively processing this patchset
> and have some questions, for instance, this part I thin really confusing, in
> the main record loop:
>
>         	switch_output_enable();
>          for (;;) {
>                  unsigned long long hits = rec->samples;
>
>                  if (record__mmap_read_all(rec) < 0) {
>                          auxtrace_snapshot_disable();
>                          err = -1;
>                          goto out_child;
>                  }
> <SNIP>
>                  if (switch_output_is_disabled()) {
>                          switch_output_enable();
>
>                          if (!quiet)
>                                  fprintf(stderr, "[ perf record: dump data: Woken up %ld times ]\n",
>                                          waking);
>                          waking = 0;
>                          fd = record__switch_output(rec, false);
>                          if (fd < 0) {
>                                  pr_err("Failed to switch to new file\n");
>                                  err = fd;
>                                  goto out_child;
>                          }
>                  }
> <SNIP>
> 	}
>
> That switch_output_enable() one we can't get to because it is part of that
> trigger_ thing, so just by looking here we think switch_output is being enabled
> unconditionally, when in fact it will check if it is "OFF" and if so, will not
> "enable", then when we see switch_output_is_disabled() the question will return
> false if it is "OFF", but what we read is "hey, this is not disabled, so it
> must be enabled, no? Confusing :-\

You are right. I think we should change the naming in trigger stuff:

TRIGGER_OFF:       this trigger is turned off
TRIGGER_RELEASED:   preparing to be triggered
TRIGGER_TOGGLED: things happened

actions:

OFF -> RELEASED    : on
RELEASED -> TOGGLED: toggle
TOGGLED-> RELEASED : release

conditions:

is_released()
is_toggled()

I'll send a v3 soon.

Thank you.

> Perhaps we should have multiple record loops, one really simple, the original
> one, one for auxtrace, that, from what we've discussed so far, doesn't lok will
> be supported together with output switch, and one for output switch?
>
> Would be something like:
>
> 	if (switch_output)
> 		err = record__switch_output_read_events()
> 	else if (auxtrace)
> 		err = record__auxtrace_read_events()
> 	else
> 		err = record__read_events();
>
> And then each of these loops don't need to have all those branches per mmap_read.
>
> - Arnaldo
>   
>> - Arnaldo
>>   
>>> [1] https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=c0bdc1c461dd5b2e492c0746708a3e0da6b13515
>>> [2] https://git.kernel.org/cgit/linux/kernel/git/pi3orama/linux.git/log/?h=perf/overwrite