Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932689AbcDTHqS (ORCPT ); Wed, 20 Apr 2016 03:46:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55186 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932391AbcDTHqR (ORCPT ); Wed, 20 Apr 2016 03:46:17 -0400 Date: Wed, 20 Apr 2016 09:46:10 +0200 From: Jiri Olsa To: Wang Nan Cc: acme@kernel.org, linux-kernel@vger.kernel.org, pi3orama@163.com, Adrian Hunter , He Kuang , Jiri Olsa , Masami Hiramatsu , Namhyung Kim , Zefan Li Subject: Re: [PATCH v5 1/6] perf tools: Derive trigger class from auxtrace_snapshot Message-ID: <20160420074610.GB25541@krava.redhat.com> References: <1460991332-185772-1-git-send-email-wangnan0@huawei.com> <1460991332-185772-2-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1460991332-185772-2-git-send-email-wangnan0@huawei.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 20 Apr 2016 07:46:16 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2614 Lines: 86 On Mon, Apr 18, 2016 at 02:55:27PM +0000, Wang Nan wrote: SNIP > -static volatile int auxtrace_snapshot_err; > static volatile int auxtrace_record__snapshot_started; > +static DEFINE_TRIGGER(auxtrace_snapshot); > > static void sig_handler(int sig) > { > @@ -282,11 +247,12 @@ static void record__read_auxtrace_snapshot(struct record *rec) > { > pr_debug("Recording AUX area tracing snapshot\n"); > if (record__auxtrace_read_snapshot_all(rec) < 0) { > - auxtrace_snapshot_err = -1; > + auxtrace_snapshot_error(); > } else { > - auxtrace_snapshot_err = auxtrace_record__snapshot_finish(rec->itr); > - if (!auxtrace_snapshot_err) > - auxtrace_snapshot_enable(); > + if (auxtrace_record__snapshot_finish(rec->itr)) > + auxtrace_snapshot_error(); > + else > + auxtrace_snapshot_ready(); > } > } > > @@ -815,21 +781,21 @@ static int __cmd_record(struct record *rec, int argc, const char **argv) > perf_evlist__enable(rec->evlist); > } > > - auxtrace_snapshot_enable(); > + auxtrace_snapshot_ready(); > for (;;) { > unsigned long long hits = rec->samples; > > if (record__mmap_read_all(rec) < 0) { > - auxtrace_snapshot_disable(); > + auxtrace_snapshot_error(); > err = -1; > goto out_child; > } > > if (auxtrace_record__snapshot_started) { > auxtrace_record__snapshot_started = 0; I'm ok with the chenge, however what I wanted to understand is why we need auxtrace_record__snapshot_started ;-) could we get rid of it like in the patch below? thanks, jirka --- diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index ad37f6937410..e220bd5eea0e 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -128,7 +128,6 @@ static volatile int done; static volatile int signr = -1; static volatile int child_finished; -static volatile int auxtrace_record__snapshot_started; static DEFINE_TRIGGER(auxtrace_snapshot); static void sig_handler(int sig) @@ -791,8 +790,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv) goto out_child; } - if (auxtrace_record__snapshot_started) { - auxtrace_record__snapshot_started = 0; + if (auxtrace_snapshot_is_toggled()) { if (!auxtrace_snapshot_is_error()) record__read_auxtrace_snapshot(rec); if (auxtrace_snapshot_is_error()) { @@ -1416,7 +1414,6 @@ static void snapshot_sig_handler(int sig __maybe_unused) if (!auxtrace_snapshot_is_ready()) return; auxtrace_snapshot_toggle(); - auxtrace_record__snapshot_started = 1; if (auxtrace_record__snapshot_start(record.itr)) auxtrace_snapshot_error(); }