Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932952AbbGTPEj (ORCPT ); Mon, 20 Jul 2015 11:04:39 -0400 Received: from mga03.intel.com ([134.134.136.65]:37996 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932552AbbGTPEi convert rfc822-to-8bit (ORCPT ); Mon, 20 Jul 2015 11:04:38 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,508,1432623600"; d="scan'208";a="527177740" From: "Liang, Kan" To: Jiri Olsa , Namhyung Kim CC: "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 Thread-Topic: [PATCH RFC V5 2/4] perf,tool: per-event time support Thread-Index: AQHQwMDEZCKCd/XInUO2fSdQ+Z3G953gp7iAgAD0qgCAAQn1AIABy/NA Date: Mon, 20 Jul 2015 15:04:20 +0000 Message-ID: <37D7C6CF3E00A74B8858931C1DB2F0770188F5E9@SHSMSX103.ccr.corp.intel.com> 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> <20150719032128.GA24219@danjae.kornet> <20150719191322.GA27893@krava.brq.redhat.com> In-Reply-To: <20150719191322.GA27893@krava.brq.redhat.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10244 Lines: 285 > > > [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 > > right.. hum, we need somehow separate/pospone the users term > application to the final perf_event_attr.. spent some time on it today, but > could not find any nice solution so far.. will try tomorrow ;-) > > SNIP > > > > + * 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'm not sure about it. Because, for period value, the user set specific value by option can override the per-event setting. I think we should make them consistent. $ perf record -e 'cpu/instructions,period=20000/' -c 1000 sleep 1 $ perf evlist -v cpu/instructions,period=20000/: type: 4, size: 112, config: 0xc0, { sample_period, sample_freq }: 1000 How about adding a per-evsel user_time_set which indicate if the time item is set? $ perf record -e 'cpu/cpu-cycles/,cpu/instructions,time=0/' $ perf evlist -v cpu/cpu-cycles/:... sample_type: IP|TID|TIME|PERIOD|IDENTIFIER cpu/instructions,time=0/:... sample_type: IP|TID|PERIOD|IDENTIFIER $ perf record -e 'cpu/cpu-cycles/,cpu/instructions,time/' $ perf evlist -v cpu/cpu-cycles/:... sample_type: IP|TID|TIME|PERIOD|IDENTIFIER cpu/instructions,time=0/:... sample_type: IP|TID|TIME|PERIOD|IDENTIFIER $ perf record -T -e 'cpu/cpu-cycles/,cpu/instructions,time=0/' sleep 1 $ perf evlist -v cpu/cpu-cycles/:... sample_type: IP|TID|TIME|PERIOD|IDENTIFIER cpu/instructions,time=0/:... sample_type: IP|TID|TIME|PERIOD|IDENTIFIER $perf record --no-timestamp -e 'cpu/cpu-cycles/,cpu/instructions,time=0/' sleep 1 $ perf evlist -v cpu/cpu-cycles/:... sample_type: IP|TID|PERIOD|IDENTIFIER cpu/instructions,time=0/:... sample_type: IP|TID|PERIOD|IDENTIFIER diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt index 5b47b2c..df47907 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 83c0803..cfa09f1 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -619,10 +619,35 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts) struct perf_event_attr *attr = &evsel->attr; int track = evsel->tracking; bool per_cpu = opts->target.default_per_cpu && !opts->target.per_thread; + bool sample_time = opts->sample_time; attr->sample_id_all = perf_missing_features.sample_id_all ? 0 : 1; attr->inherit = !opts->no_inherit; + /* + * If user doesn't explicitly set time option, + * let event attribute decide. + */ + + if (!opts->sample_time_set && evsel->user_time_set) { + if (attr->sample_type & PERF_SAMPLE_TIME) + sample_time = true; + else + sample_time = false; + } + + /* + * Event parsing doesn't check the availability + * Clear the bit which event parsing may be set. + * Let following code check and reset if available + * + * Also, the sample size may be caculated mistakenly, + * because event parsing may set the PERF_SAMPLE_TIME. + * Remove the size which add in perf_evsel__init + */ + if (attr->sample_type & PERF_SAMPLE_TIME) + perf_evsel__reset_sample_bit(evsel, TIME); + perf_evsel__set_sample_bit(evsel, IP); perf_evsel__set_sample_bit(evsel, TID); @@ -705,14 +730,15 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts) /* * When the user explicitely disabled time don't force it here. */ - if (opts->sample_time && + if (sample_time && (!perf_missing_features.sample_id_all && (!opts->no_inherit || target__has_cpu(&opts->target) || per_cpu || opts->sample_time_set))) perf_evsel__set_sample_bit(evsel, TIME); if (opts->raw_samples && !evsel->no_aux_samples) { - perf_evsel__set_sample_bit(evsel, TIME); + if (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/evsel.h b/tools/perf/util/evsel.h index fe9f327..b654b90 100644 --- a/tools/perf/util/evsel.h +++ b/tools/perf/util/evsel.h @@ -79,6 +79,7 @@ struct perf_evsel { bool system_wide; bool tracking; bool per_pkg; + bool user_time_set; /* parse modifier helper */ int exclude_GH; int nr_members; diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index a71eeb2..6f2e10e 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -561,7 +561,8 @@ static int check_type_val(struct parse_events_term *term, static int config_term(struct perf_event_attr *attr, struct parse_events_term *term, - struct parse_events_error *err) + struct parse_events_error *err, + bool *time_set) { #define CHECK_TYPE_VAL(type) \ do { \ @@ -598,6 +599,15 @@ do { \ * attr->branch_sample_type = term->val.num; */ break; + case PARSE_EVENTS__TERM_TYPE_TIME: + if (time_set) + *time_set = true; + 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; @@ -611,12 +621,13 @@ do { \ static int config_attr(struct perf_event_attr *attr, struct list_head *head, - struct parse_events_error *err) + struct parse_events_error *err, + bool *time_set) { struct parse_events_term *term; list_for_each_entry(term, head, list) - if (config_term(attr, term, err)) + if (config_term(attr, term, err, time_set)) return -EINVAL; return 0; @@ -634,7 +645,7 @@ int parse_events_add_numeric(struct parse_events_evlist *data, attr.config = config; if (head_config && - config_attr(&attr, head_config, data->error)) + config_attr(&attr, head_config, data->error, NULL)) return -EINVAL; return add_event(list, &data->idx, &attr, NULL); @@ -664,6 +675,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data, struct perf_pmu_info info; struct perf_pmu *pmu; struct perf_evsel *evsel; + bool time_set = false; pmu = perf_pmu__find(name); if (!pmu) @@ -689,7 +701,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data, * Configure hardcoded terms first, no need to check * return value when called with fail == 0 ;) */ - if (config_attr(&attr, head_config, data->error)) + if (config_attr(&attr, head_config, data->error, &time_set)) return -EINVAL; if (perf_pmu__config(pmu, &attr, head_config, data->error)) @@ -702,6 +714,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data, evsel->scale = info.scale; evsel->per_pkg = info.per_pkg; evsel->snapshot = info.snapshot; + evsel->user_time_set = time_set; } return evsel ? 0 : -ENOMEM; diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h index 131f29b..0d8cae3 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 13cef3c..f542750 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 7bcb8c3..b615cdf 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:")) Thanks, Kan -- 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/