> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
