Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752926AbbGSDXr (ORCPT ); Sat, 18 Jul 2015 23:23:47 -0400 Received: from mail-pd0-f175.google.com ([209.85.192.175]:33282 "EHLO mail-pd0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752560AbbGSDXq (ORCPT ); Sat, 18 Jul 2015 23:23:46 -0400 Date: Sun, 19 Jul 2015 12:21:28 +0900 From: Namhyung Kim To: Jiri Olsa Cc: kan.liang@intel.com, acme@kernel.org, jolsa@kernel.org, ak@linux.intel.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH RFC V5 2/4] perf,tool: per-event time support Message-ID: <20150719032128.GA24219@danjae.kornet> References: <1437132655-15883-1-git-send-email-kan.liang@intel.com> <1437132655-15883-3-git-send-email-kan.liang@intel.com> <20150718124547.GA3759@krava.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150718124547.GA3759@krava.brq.redhat.com> User-Agent: Mutt/1.5.23+89 (0255b37be491) (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9559 Lines: 246 Hi Jiri, On Sat, Jul 18, 2015 at 02:45:47PM +0200, Jiri Olsa wrote: > On Fri, Jul 17, 2015 at 07:30:53AM -0400, kan.liang@intel.com wrote: > > SNIP > > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > > index a71eeb2..c9981df 100644 > > --- a/tools/perf/util/parse-events.c > > +++ b/tools/perf/util/parse-events.c > > @@ -25,6 +25,9 @@ > > #ifdef PARSER_DEBUG > > extern int parse_events_debug; > > #endif > > + > > +bool time_term_detected = false; > > + > > int parse_events_parse(void *data, void *scanner); > > > > static struct perf_pmu_event_symbol *perf_pmu_events_list; > > @@ -598,6 +601,14 @@ do { \ > > * attr->branch_sample_type = term->val.num; > > */ > > break; > > + case PARSE_EVENTS__TERM_TYPE_TIME: > > + CHECK_TYPE_VAL(NUM); > > + if (term->val.num > 1) > > + return -EINVAL; > > + time_term_detected = true; > > + if (term->val.num == 1) > > + attr->sample_type |= PERF_SAMPLE_TIME; > > + break; > > case PARSE_EVENTS__TERM_TYPE_NAME: > > CHECK_TYPE_VAL(STR); > > break; > > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h > > index 131f29b..1083478 100644 > > --- a/tools/perf/util/parse-events.h > > +++ b/tools/perf/util/parse-events.h > > @@ -22,6 +22,8 @@ struct tracepoint_path { > > struct tracepoint_path *next; > > }; > > > > +extern bool time_term_detected; > > so I wasnt happy about this time_term_detected global variable, > and I tried to make it without and ended up with somewhat siplified > patch.. not tested very deeply, just the basics: > > > [jolsa@krava perf]$ ./perf record -e 'cpu/cpu-cycles/,cpu/instructions/' kill > ... > [jolsa@krava perf]$ ./perf evlist -v > cpu/cpu-cycles/: type: 4, size: 112, config: 0x3c, { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|ID|PERIOD, read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1 > cpu/instructions/: type: 4, size: 112, config: 0xc0, { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|ID|PERIOD, read_format: ID, disabled: 1, inherit: 1, freq: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1 > > > > [jolsa@krava perf]$ ./perf record -e 'cpu/cpu-cycles/,cpu/instructions,time/' kill > ... > [jolsa@krava perf]$ ./perf evlist -v > cpu/cpu-cycles/: type: 4, size: 112, config: 0x3c, { sample_period, sample_freq }: 4000, sample_type: IP|TID|PERIOD|IDENTIFIER, read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1 > cpu/instructions,time/: type: 4, size: 112, config: 0xc0, { sample_period, sample_freq }: 4000, sample_type: IP|TID|TIME|PERIOD|IDENTIFIER, read_format: ID, disabled: 1, inherit: 1, freq: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1 What about this case? $ perf record -e 'cpu/cpu-cycles/,cpu/instructions,time=0/' kill > > > --- > diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt > index 5b47b2c88223..df479077384d 100644 > --- a/tools/perf/Documentation/perf-record.txt > +++ b/tools/perf/Documentation/perf-record.txt > @@ -49,7 +49,9 @@ OPTIONS > These params can be used to set event defaults. > Here is a list of the params. > - 'period': Set event sampling period > - > + - 'time': Disable/enable time stamping. Acceptable values are 1 for > + enabling time stamping. 0 for disabling time stamping. > + The default is 1. > Note: If user explicitly sets options which conflict with the params, > the value set by the params will be overridden. > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index 83c08037e7e2..8e3a17845c37 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -712,7 +712,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts) > perf_evsel__set_sample_bit(evsel, TIME); > > if (opts->raw_samples && !evsel->no_aux_samples) { > - perf_evsel__set_sample_bit(evsel, TIME); > + if (opts->sample_time) > + perf_evsel__set_sample_bit(evsel, TIME); > perf_evsel__set_sample_bit(evsel, RAW); > perf_evsel__set_sample_bit(evsel, CPU); > } > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index a71eeb279ed2..95100478200a 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -598,6 +598,13 @@ do { \ > * attr->branch_sample_type = term->val.num; > */ > break; > + case PARSE_EVENTS__TERM_TYPE_TIME: > + CHECK_TYPE_VAL(NUM); > + if (term->val.num > 1) > + return -EINVAL; > + if (term->val.num == 1) > + attr->sample_type |= PERF_SAMPLE_TIME; > + break; > case PARSE_EVENTS__TERM_TYPE_NAME: > CHECK_TYPE_VAL(STR); > break; > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h > index 131f29b2f132..0d8cae31b506 100644 > --- a/tools/perf/util/parse-events.h > +++ b/tools/perf/util/parse-events.h > @@ -62,6 +62,7 @@ enum { > PARSE_EVENTS__TERM_TYPE_NAME, > PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD, > PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE, > + PARSE_EVENTS__TERM_TYPE_TIME, > }; > > struct parse_events_term { > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l > index 13cef3c65565..f5427505ae77 100644 > --- a/tools/perf/util/parse-events.l > +++ b/tools/perf/util/parse-events.l > @@ -183,6 +183,7 @@ config2 { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_CONFIG2); } > name { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NAME); } > period { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD); } > branch_type { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE); } > +time { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_TIME); } > , { return ','; } > "/" { BEGIN(INITIAL); return '/'; } > {name_minus} { return str(yyscanner, PE_NAME); } > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c > index 7bcb8c315615..b615cdf211d6 100644 > --- a/tools/perf/util/pmu.c > +++ b/tools/perf/util/pmu.c > @@ -607,7 +607,7 @@ static char *formats_error_string(struct list_head *formats) > { > struct perf_pmu_format *format; > char *err, *str; > - static const char *static_terms = "config,config1,config2,name,period,branch_type\n"; > + static const char *static_terms = "config,config1,config2,name,period,branch_type,time\n"; > unsigned i = 0; > > if (!asprintf(&str, "valid terms:")) > diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c > index 1f7becbe5e18..6b42c1339fde 100644 > --- a/tools/perf/util/record.c > +++ b/tools/perf/util/record.c > @@ -95,6 +95,18 @@ static bool perf_can_comm_exec(void) > return perf_probe_api(perf_probe_comm_exec); > } > > +static bool perf_evlist__has_time(struct perf_evlist *evlist) > +{ > + struct perf_evsel *evsel; > + > + evlist__for_each(evlist, evsel) { > + if (evsel->attr.sample_type & PERF_SAMPLE_TIME) > + return true; > + } > + > + return false; > +} > + > void perf_evlist__config(struct perf_evlist *evlist, struct record_opts *opts) > { > struct perf_evsel *evsel; > @@ -111,6 +123,14 @@ void perf_evlist__config(struct perf_evlist *evlist, struct record_opts *opts) > if (evlist->cpus->map[0] < 0) > opts->no_inherit = true; > > + /* > + * If time (-T) is not set and we have events with TIME sample_type > + * set (tracepoints or events with time term), disable timestamp for > + * the rest of the events. > + */ > + if (!opts->sample_time_set && perf_evlist__has_time(evlist)) > + opts->sample_time = false; I think it'd be better if the -T/--timestamp option gives the default TIME sample_type value but it can be overridden by per-event setting. I mean: (per-event 'time' setting is meaningless here) $ perf record -e 'cpu/cpu-cycles/,cpu/instructions,time/' kill $ perf evlist -v cpu/cpu-cycles/: ..., sample_type: IP|TID|TIME|PERIOD|IDENTIFIER, ... cpu/instructions,time/: ..., sample_type: IP|TID|TIME|PERIOD|IDENTIFIER, ... (adding -T option, same as the default) $ perf record -T -e 'cpu/cpu-cycles/,cpu/instructions,time/' kill $ perf evlist -v cpu/cpu-cycles/: ..., sample_type: IP|TID|TIME|PERIOD|IDENTIFIER, ... cpu/instructions,time/: ..., sample_type: IP|TID|TIME|PERIOD|IDENTIFIER, ... $ perf record -T -e 'cpu/cpu-cycles/,cpu/instructions,time=0/' kill $ perf evlist -v cpu/cpu-cycles/: ..., sample_type: IP|TID|TIME|PERIOD|IDENTIFIER, ... cpu/instructions,time/: ..., sample_type: IP|TID|PERIOD|IDENTIFIER, ... (adding --no-timestamp option) $ perf record --no-timestamp -e 'cpu/cpu-cycles/,cpu/instructions/' kill $ perf evlist -v cpu/cpu-cycles/: ..., sample_type: IP|TID|PERIOD|IDENTIFIER, ... cpu/instructions,time/: ..., sample_type: IP|TID|PERIOD|IDENTIFIER, ... $ perf record --no-timestamp -e 'cpu/cpu-cycles/,cpu/instructions,time=1/' kill $ perf evlist -v cpu/cpu-cycles/: ..., sample_type: IP|TID|PERIOD|IDENTIFIER, ... cpu/instructions,time/: ..., sample_type: IP|TID|TIME|PERIOD|IDENTIFIER, ... Thanks, Namhyung > + > use_comm_exec = perf_can_comm_exec(); > > evlist__for_each(evlist, evsel) { -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/