From: Namhyung Kim <namhyung@kernel.org>
To: Jiri Olsa <jolsa@redhat.com>
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
Date: Sun, 19 Jul 2015 12:21:28 +0900 [thread overview]
Message-ID: <20150719032128.GA24219@danjae.kornet> (raw)
In-Reply-To: <20150718124547.GA3759@krava.brq.redhat.com>
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) {
next prev parent reply other threads:[~2015-07-19 3:23 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-17 11:30 [PATCH RFC V5 0/4] per event callgrap and " kan.liang
2015-07-17 11:30 ` [PATCH RFC V5 1/4] perf,tools: introduce callgraph_set for callgraph option kan.liang
2015-07-17 11:30 ` [PATCH RFC V5 2/4] perf,tool: per-event time support kan.liang
2015-07-18 12:45 ` Jiri Olsa
2015-07-19 3:21 ` Namhyung Kim [this message]
2015-07-19 19:13 ` Jiri Olsa
2015-07-20 15:04 ` Liang, Kan
2015-07-20 17:40 ` Jiri Olsa
2015-07-21 3:39 ` Liang, Kan
2015-07-21 14:45 ` Namhyung Kim
2015-07-21 14:56 ` Jiri Olsa
2015-07-21 14:59 ` Jiri Olsa
2015-07-17 11:30 ` [PATCH RFC V5 3/4] perf,tool: per-event callgrap support kan.liang
2015-07-17 11:30 ` [PATCH RFC V5 4/4] perf,tests: Add tests to callgrap and time parse kan.liang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150719032128.GA24219@danjae.kornet \
--to=namhyung@kernel.org \
--cc=acme@kernel.org \
--cc=ak@linux.intel.com \
--cc=jolsa@kernel.org \
--cc=jolsa@redhat.com \
--cc=kan.liang@intel.com \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®