Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753518AbbGUDkg (ORCPT ); Mon, 20 Jul 2015 23:40:36 -0400 Received: from mga11.intel.com ([192.55.52.93]:13434 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752498AbbGUDke convert rfc822-to-8bit (ORCPT ); Mon, 20 Jul 2015 23:40:34 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,512,1432623600"; d="scan'208";a="766397387" From: "Liang, Kan" To: Jiri Olsa CC: Namhyung Kim , "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//+sggCAAQszcA== Date: Tue, 21 Jul 2015 03:39:17 +0000 Message-ID: <37D7C6CF3E00A74B8858931C1DB2F0770188F94B@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> <37D7C6CF3E00A74B8858931C1DB2F0770188F5E9@SHSMSX103.ccr.corp.intel.com> <20150720174046.GA27331@krava.brq.redhat.com> In-Reply-To: <20150720174046.GA27331@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: 11083 Lines: 364 > On Mon, Jul 20, 2015 at 03:04:20PM +0000, Liang, Kan wrote: > > SNIP > > > 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)) > > I think we should rather have some generic way to mart event specific > settings otherwise this function prototype will grow wild ;-) > I agree. > how about posponing static terms configuration after global config was set.. > like in attached change > > > works for period now: > > [jolsa@krava perf]$ ./perf record -e 'cpu/instructions,period=20000/' -c > 1000 sleep 1 [ perf record: Woken up 1 times to write data ] /proc/kcore > requires CAP_SYS_RAWIO capability to access. > [ perf record: Captured and wrote 0.015 MB perf.data (35 samples) ] > [jolsa@krava perf]$ ./perf evlist -vv > cpu/instructions,period=20000/: type: 4, size: 112, config: 0xc0, > { sample_period, sample_freq }: 20000 > So you are going to change current behavior? The current behavior is "global opts setting" > "per_event settring" > default You are going to change it to "per_event settring" > "global opts setting" > Default. Right? Personally, I like the current behavior, since I don't see any problem with it. But either is fine with me. > we'd just add new term for time the same way > > also need to check if that will help for the backtrace per-event change > > jirka > > > --- > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index > 83c08037e7e2..f635d6ba83b0 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -207,6 +207,7 @@ void perf_evsel__init(struct perf_evsel *evsel, > evsel->unit = ""; > evsel->scale = 1.0; > INIT_LIST_HEAD(&evsel->node); > + INIT_LIST_HEAD(&evsel->config_terms); > perf_evsel__object.init(evsel); > evsel->sample_size = __perf_evsel__sample_size(attr- > >sample_type); > perf_evsel__calc_id_pos(evsel); > @@ -585,6 +586,21 @@ perf_evsel__config_callgraph(struct perf_evsel > *evsel, > } > } > > +static void apply_config_terms(struct perf_event_attr *attr, > + struct list_head *config_terms) { > + struct perf_evsel_config_term *term; > + > + list_for_each_entry(term, config_terms, list) { > + switch (term->type) { > + case PERF_EVSEL__CONFIG_TERM_PERIOD: > + attr->sample_period = term->val.period; > + default: > + break; > + } > + } > +} > + > /* > * The enable_on_exec/disabled value strategy: > * > @@ -773,6 +789,8 @@ void perf_evsel__config(struct perf_evsel *evsel, > struct record_opts *opts) > attr->use_clockid = 1; > attr->clockid = opts->clockid; > } > + > + apply_config_terms(attr, &evsel->config_terms); > } > Other options/event modifier may also change the sample_period. E.g. sample read. So I think we may move it to the beginning of perf_evsel__config. > static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int > nthreads) @@ -896,6 +914,16 @@ static void perf_evsel__free_id(struct > perf_evsel *evsel) > zfree(&evsel->id); > } > > +static void perf_evsel__free_config_terms(struct perf_evsel *evsel) { > + struct perf_evsel_config_term *term, *h; > + > + list_for_each_entry_safe(term, h, &evsel->config_terms, list) { > + list_del(&term->list); > + free(term); > + } > +} > + > void perf_evsel__close_fd(struct perf_evsel *evsel, int ncpus, int > nthreads) { > int cpu, thread; > @@ -915,6 +943,7 @@ void perf_evsel__exit(struct perf_evsel *evsel) > assert(list_empty(&evsel->node)); > perf_evsel__free_fd(evsel); > perf_evsel__free_id(evsel); > + perf_evsel__free_config_terms(evsel); > close_cgroup(evsel->cgrp); > cpu_map__put(evsel->cpus); > thread_map__put(evsel->threads); > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h index > fe9f3279632b..de2ba4eb858c 100644 > --- a/tools/perf/util/evsel.h > +++ b/tools/perf/util/evsel.h > @@ -31,6 +31,18 @@ struct perf_sample_id { > > struct cgroup_sel; > > +enum { > + PERF_EVSEL__CONFIG_TERM_PERIOD, > +}; > + > +struct perf_evsel_config_term { > + struct list_head list; > + int type; > + union { > + u64 period; > + } val; > +}; > + > /** struct perf_evsel - event selector > * > * @name - Can be set to retain the original event name passed by the > user, @@ -86,6 +98,7 @@ struct perf_evsel { > unsigned long *per_pkg_mask; > struct perf_evsel *leader; > char *group_name; > + struct list_head config_terms; > }; > > union u64_swap { > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index a71eeb279ed2..d83ac773ab4f 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -276,7 +276,8 @@ const char *event_type(int type) static struct > perf_evsel * __add_event(struct list_head *list, int *idx, > struct perf_event_attr *attr, > - char *name, struct cpu_map *cpus) > + char *name, struct cpu_map *cpus, > + struct list_head *config_terms) > { > struct perf_evsel *evsel; > > @@ -291,14 +292,19 @@ __add_event(struct list_head *list, int *idx, > > if (name) > evsel->name = strdup(name); > + > + if (config_terms) > + list_splice(config_terms, &evsel->config_terms); > + > list_add_tail(&evsel->node, list); > return evsel; > } > > static int add_event(struct list_head *list, int *idx, > - struct perf_event_attr *attr, char *name) > + struct perf_event_attr *attr, char *name, > + struct list_head *config_terms) > { > - return __add_event(list, idx, attr, name, NULL) ? 0 : -ENOMEM; > + return __add_event(list, idx, attr, name, NULL, config_terms) ? 0 : > +-ENOMEM; > } > > static int parse_aliases(char *str, const char > *names[][PERF_EVSEL__MAX_ALIASES], int size) @@ -377,7 +383,7 @@ > int parse_events_add_cache(struct list_head *list, int *idx, > memset(&attr, 0, sizeof(attr)); > attr.config = cache_type | (cache_op << 8) | (cache_result << 16); > attr.type = PERF_TYPE_HW_CACHE; > - return add_event(list, idx, &attr, name); > + return add_event(list, idx, &attr, name, NULL); > } > > static int add_tracepoint(struct list_head *list, int *idx, @@ -539,7 +545,7 > @@ int parse_events_add_breakpoint(struct list_head *list, int *idx, > attr.type = PERF_TYPE_BREAKPOINT; > attr.sample_period = 1; > > - return add_event(list, idx, &attr, NULL); > + return add_event(list, idx, &attr, NULL, NULL); > } > > static int check_type_val(struct parse_events_term *term, @@ -561,7 > +567,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, > + struct list_head *config_terms) > { > #define CHECK_TYPE_VAL(type) > \ > do { \ > @@ -569,6 +576,20 @@ do { > \ > return -EINVAL; > \ > } while (0) > > +#define ADD_EVSEL_CONFIG(__type, __name, __val) > \ > +do { \ > + struct perf_evsel_config_term *__t; \ > + \ > + __t = zalloc(sizeof(*__t)); \ > + if (!__t) \ > + return -ENOMEM; \ > + \ > + INIT_LIST_HEAD(&__t->list); \ > + __t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \ > + __t->val.__name = __val; \ > + list_add_tail(&__t->list, config_terms); \ > +} while (0) > + > switch (term->type_term) { > case PARSE_EVENTS__TERM_TYPE_USER: > /* > @@ -590,7 +611,7 @@ do { > \ > break; > case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD: > CHECK_TYPE_VAL(NUM); > - attr->sample_period = term->val.num; > + ADD_EVSEL_CONFIG(PERIOD, period, term->val.num); > break; > case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE: > /* > @@ -606,17 +627,19 @@ do { > \ > } > > return 0; > +#undef ADD_EVSEL_CONFIG > #undef CHECK_TYPE_VAL > } > > static int config_attr(struct perf_event_attr *attr, > struct list_head *head, > - struct parse_events_error *err) > + struct parse_events_error *err, > + struct list_head *config_terms) > { > struct parse_events_term *term; > > list_for_each_entry(term, head, list) > - if (config_term(attr, term, err)) > + if (config_term(attr, term, err, config_terms)) > return -EINVAL; > > return 0; > @@ -628,16 +651,17 @@ int parse_events_add_numeric(struct > parse_events_evlist *data, > struct list_head *head_config) > { > struct perf_event_attr attr; > + LIST_HEAD(config_terms); > > memset(&attr, 0, sizeof(attr)); > attr.type = type; > attr.config = config; > > if (head_config && > - config_attr(&attr, head_config, data->error)) > + config_attr(&attr, head_config, data->error, &config_terms)) > return -EINVAL; > > - return add_event(list, &data->idx, &attr, NULL); > + return add_event(list, &data->idx, &attr, NULL, &config_terms); > } > > static int parse_events__is_name_term(struct parse_events_term *term) > @@ -664,6 +688,7 @@ int parse_events_add_pmu(struct > parse_events_evlist *data, > struct perf_pmu_info info; > struct perf_pmu *pmu; > struct perf_evsel *evsel; > + LIST_HEAD(config_terms); > > pmu = perf_pmu__find(name); > if (!pmu) > @@ -678,7 +703,7 @@ int parse_events_add_pmu(struct > parse_events_evlist *data, > > if (!head_config) { > attr.type = pmu->type; > - evsel = __add_event(list, &data->idx, &attr, NULL, pmu- > >cpus); > + evsel = __add_event(list, &data->idx, &attr, NULL, pmu- > >cpus, NULL); > return evsel ? 0 : -ENOMEM; > } > > @@ -689,14 +714,14 @@ 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, &config_terms)) > return -EINVAL; > > if (perf_pmu__config(pmu, &attr, head_config, data->error)) > return -EINVAL; > > evsel = __add_event(list, &data->idx, &attr, > - pmu_event_name(head_config), pmu->cpus); > + pmu_event_name(head_config), pmu->cpus, > &config_terms); > if (evsel) { > evsel->unit = info.unit; > evsel->scale = info.scale; -- 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/