Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754221AbcDNIfZ (ORCPT ); Thu, 14 Apr 2016 04:35:25 -0400 Received: from mga02.intel.com ([134.134.136.20]:2243 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753324AbcDNIeo (ORCPT ); Thu, 14 Apr 2016 04:34:44 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,484,1455004800"; d="scan'208";a="784669117" Subject: Re: [PATCH 03/10] perf record: Turns auxtrace_snapshot_enable into 3 states To: "Wangnan (F)" , Arnaldo Carvalho de Melo References: <1460535673-159866-1-git-send-email-wangnan0@huawei.com> <1460535673-159866-4-git-send-email-wangnan0@huawei.com> <20160413155533.GG9056@kernel.org> <570F4393.6080908@intel.com> <570F4BAE.7090808@huawei.com> Cc: linux-kernel@vger.kernel.org, pi3orama@163.com, He Kuang , Arnaldo Carvalho de Melo , Masami Hiramatsu , Namhyung Kim , Zefan Li From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki Message-ID: <570F5541.50407@intel.com> Date: Thu, 14 Apr 2016 11:30:57 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <570F4BAE.7090808@huawei.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5893 Lines: 170 On 14/04/16 10:50, Wangnan (F) wrote: > > > On 2016/4/14 15:15, Adrian Hunter wrote: >> On 13/04/16 18:55, Arnaldo Carvalho de Melo wrote: >>> Em Wed, Apr 13, 2016 at 08:21:06AM +0000, Wang Nan escreveu: >>>> auxtrace_snapshot_enable has only two states (0/1). Turns it into a >>>> triple states enum so SIGUSR2 handler can safely do other works without >>>> triggering auxtrace snapshot. >>> Adrian, can you take a look at this? Is it ok with you? >> Please forgive me if these are stupid questions: >> >> First I am wondering why we wouldn't want to snapshot auxtrace data at the >> same time as the perf buffer? > > This patch doesn't prevent taking snapshot when receiving SIGUSR2. So it was a stupid question ;-) > > If both --snapshot and --switch-outupt is provided, when SIGUSR2 received, > perf takes auxtrace snapshot and other perf buffer together. How do you keep from losing tracking information like MMAP events? Are they is a different buffer? > > Thank you. > > >> For that we would need continuous tracking information like MMAPS etc, but >> isn't that needed anyway? >> >> >>> Thanks, >>> >>> - Arnaldo >>> >>>> Signed-off-by: Wang Nan >>>> Signed-off-by: He Kuang >>>> Acked-by: Jiri Olsa >>>> Cc: Arnaldo Carvalho de Melo >>>> Cc: Masami Hiramatsu >>>> Cc: Namhyung Kim >>>> Cc: Zefan Li >>>> Cc: pi3orama@163.com >>>> --- >>>> tools/perf/builtin-record.c | 59 >>>> +++++++++++++++++++++++++++++++++++++-------- >>>> 1 file changed, 49 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c >>>> index eb6a199..480033f 100644 >>>> --- a/tools/perf/builtin-record.c >>>> +++ b/tools/perf/builtin-record.c >>>> @@ -125,7 +125,43 @@ out: >>>> static volatile int done; >>>> static volatile int signr = -1; >>>> static volatile int child_finished; >>>> -static volatile int auxtrace_snapshot_enabled; >>>> + >>>> +static volatile enum { >>>> + AUXTRACE_SNAPSHOT_OFF = -1, >>>> + AUXTRACE_SNAPSHOT_DISABLED = 0, >>>> + AUXTRACE_SNAPSHOT_ENABLED = 1, >>>> +} auxtrace_snapshot_state = AUXTRACE_SNAPSHOT_OFF; >>>> + >>>> +static inline void >>>> +auxtrace_snapshot_on(void) >>>> +{ >>>> + auxtrace_snapshot_state = AUXTRACE_SNAPSHOT_DISABLED; >>>> +} >>>> + >>>> +static inline void >>>> +auxtrace_snapshot_enable(void) >>>> +{ >>>> + if (auxtrace_snapshot_state == AUXTRACE_SNAPSHOT_OFF) >>>> + return; >>>> + auxtrace_snapshot_state = AUXTRACE_SNAPSHOT_ENABLED; >>>> +} >>>> + >>>> +static inline void >>>> +auxtrace_snapshot_disable(void) >>>> +{ >>>> + if (auxtrace_snapshot_state == AUXTRACE_SNAPSHOT_OFF) >>>> + return; >>>> + auxtrace_snapshot_state = AUXTRACE_SNAPSHOT_DISABLED; >>>> +} >>>> + >>>> +static inline bool >>>> +auxtrace_snapshot_is_enabled(void) >>>> +{ >>>> + if (auxtrace_snapshot_state == AUXTRACE_SNAPSHOT_OFF) >>>> + return false; >>>> + return auxtrace_snapshot_state == AUXTRACE_SNAPSHOT_ENABLED; >>>> +} >>>> + >>>> static volatile int auxtrace_snapshot_err; >>>> static volatile int auxtrace_record__snapshot_started; >>>> @@ -249,7 +285,7 @@ static void record__read_auxtrace_snapshot(struct >>>> record *rec) >>>> } else { >>>> auxtrace_snapshot_err = >>>> auxtrace_record__snapshot_finish(rec->itr); >>>> if (!auxtrace_snapshot_err) >>>> - auxtrace_snapshot_enabled = 1; >>>> + auxtrace_snapshot_enable(); >>>> } >>>> } >>>> @@ -615,10 +651,13 @@ static int __cmd_record(struct record *rec, int >>>> argc, const char **argv) >>>> signal(SIGCHLD, sig_handler); >>>> signal(SIGINT, sig_handler); >>>> signal(SIGTERM, sig_handler); >>>> - if (rec->opts.auxtrace_snapshot_mode) >>>> + >>>> + if (rec->opts.auxtrace_snapshot_mode) { >>>> signal(SIGUSR2, snapshot_sig_handler); >>>> - else >>>> + auxtrace_snapshot_on(); >>>> + } else { >>>> signal(SIGUSR2, SIG_IGN); >>>> + } >>>> session = perf_session__new(file, false, tool); >>>> if (session == NULL) { >>>> @@ -744,12 +783,12 @@ static int __cmd_record(struct record *rec, int >>>> argc, const char **argv) >>>> perf_evlist__enable(rec->evlist); >>>> } >>>> - auxtrace_snapshot_enabled = 1; >>>> + auxtrace_snapshot_enable(); >>>> for (;;) { >>>> unsigned long long hits = rec->samples; >>>> if (record__mmap_read_all(rec) < 0) { >>>> - auxtrace_snapshot_enabled = 0; >>>> + auxtrace_snapshot_disable(); >>>> err = -1; >>>> goto out_child; >>>> } >>>> @@ -787,12 +826,12 @@ static int __cmd_record(struct record *rec, int >>>> argc, const char **argv) >>>> * disable events in this case. >>>> */ >>>> if (done && !disabled && !target__none(&opts->target)) { >>>> - auxtrace_snapshot_enabled = 0; >>>> + auxtrace_snapshot_disable(); >>>> perf_evlist__disable(rec->evlist); >>>> disabled = true; >>>> } >>>> } >>>> - auxtrace_snapshot_enabled = 0; >>>> + auxtrace_snapshot_disable(); >>>> if (forks && workload_exec_errno) { >>>> char msg[STRERR_BUFSIZE]; >>>> @@ -1358,9 +1397,9 @@ out_symbol_exit: >>>> static void snapshot_sig_handler(int sig __maybe_unused) >>>> { >>>> - if (!auxtrace_snapshot_enabled) >>>> + if (!auxtrace_snapshot_is_enabled()) >>>> return; >>>> - auxtrace_snapshot_enabled = 0; >>>> + auxtrace_snapshot_disable(); >>>> auxtrace_snapshot_err = auxtrace_record__snapshot_start(record.itr); >>>> auxtrace_record__snapshot_started = 1; >>>> } >>>> -- >>>> 1.8.3.4 > > >