All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC V5 0/4] per event callgrap and time support
@ 2015-07-17 11:30 kan.liang
  2015-07-17 11:30 ` [PATCH RFC V5 1/4] perf,tools: introduce callgraph_set for callgraph option kan.liang
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: kan.liang @ 2015-07-17 11:30 UTC (permalink / raw)
  To: acme, jolsa; +Cc: namhyung, ak, linux-kernel, Kan Liang

From: Kan Liang <kan.liang@intel.com>

This patchkit adds the ability to turn off callgraphs and time stamps
per event. This in term can reduce sampling overhead and the size of
the perf.data.

Changes since V1:
  - Break up V1 patches into three patches(parse option changes,
    partial time support and partial callgrap support).
  - Use strings 'fp,dwarf,lbr,no' to identify callchains
  - Add test case in parse-events.c

Changes since V2:
  - Rebase on 60cd37eb10

Changes since V3:
  - Replace OPT_CALLBACK_SET by current existing callback mechanism.
  - Using perf_evsel__set_sample_bit if possible
  - Change the expression "partial" to "per event"
  - Using global variable to indicate if 'time' is set per event.
    If 'time' is not set, enable it by default for perf record.

Changes since V4:
  - Fix issue of setting callgraph_set

Kan Liang (4):
  perf,tools: introduce callgraph_set for callgraph option
  perf,tool: per-event time support
  perf,tool: per-event callgrap support
  perf,tests: Add tests to callgrap and time parse

 tools/perf/Documentation/perf-record.txt |  8 ++++-
 tools/perf/builtin-record.c              | 16 ++++++++--
 tools/perf/builtin-trace.c               |  1 +
 tools/perf/perf.h                        |  1 +
 tools/perf/tests/parse-events.c          | 28 +++++++++++++++++
 tools/perf/util/evsel.c                  | 54 ++++++++++++++++++++++++++++++--
 tools/perf/util/parse-events.c           | 29 +++++++++++++++++
 tools/perf/util/parse-events.h           |  5 +++
 tools/perf/util/parse-events.l           |  3 ++
 tools/perf/util/pmu.c                    |  3 +-
 10 files changed, 140 insertions(+), 8 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH RFC V5 1/4] perf,tools: introduce callgraph_set for callgraph option
  2015-07-17 11:30 [PATCH RFC V5 0/4] per event callgrap and time support kan.liang
@ 2015-07-17 11:30 ` kan.liang
  2015-07-17 11:30 ` [PATCH RFC V5 2/4] perf,tool: per-event time support kan.liang
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: kan.liang @ 2015-07-17 11:30 UTC (permalink / raw)
  To: acme, jolsa; +Cc: namhyung, ak, linux-kernel, Kan Liang

From: Kan Liang <kan.liang@intel.com>

Introduce callgraph_set to indicate whether the callgraph option was set
by user.

Signed-off-by: Kan Liang <kan.liang@intel.com>
---
 tools/perf/builtin-record.c | 9 +++++++--
 tools/perf/perf.h           | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 283fe96..eae6510 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -762,12 +762,14 @@ static void callchain_debug(void)
 			 callchain_param.dump_size);
 }
 
-int record_parse_callchain_opt(const struct option *opt __maybe_unused,
+int record_parse_callchain_opt(const struct option *opt,
 			       const char *arg,
 			       int unset)
 {
 	int ret;
+	struct record_opts *record = (struct record_opts *)opt->value;
 
+	record->callgraph_set = true;
 	callchain_param.enabled = !unset;
 
 	/* --no-call-graph */
@@ -784,10 +786,13 @@ int record_parse_callchain_opt(const struct option *opt __maybe_unused,
 	return ret;
 }
 
-int record_callchain_opt(const struct option *opt __maybe_unused,
+int record_callchain_opt(const struct option *opt,
 			 const char *arg __maybe_unused,
 			 int unset __maybe_unused)
 {
+	struct record_opts *record = (struct record_opts *)opt->value;
+
+	record->callgraph_set = true;
 	callchain_param.enabled = true;
 
 	if (callchain_param.record_mode == CALLCHAIN_NONE)
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 937b16a..9ba02e0 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -52,6 +52,7 @@ struct record_opts {
 	bool	     sample_weight;
 	bool	     sample_time;
 	bool	     sample_time_set;
+	bool	     callgraph_set;
 	bool	     period;
 	bool	     sample_intr_regs;
 	bool	     running_time;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH RFC V5 2/4] perf,tool: per-event time support
  2015-07-17 11:30 [PATCH RFC V5 0/4] per event callgrap and time support 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 ` kan.liang
  2015-07-18 12:45   ` 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
  3 siblings, 1 reply; 14+ messages in thread
From: kan.liang @ 2015-07-17 11:30 UTC (permalink / raw)
  To: acme, jolsa; +Cc: namhyung, ak, linux-kernel, Kan Liang

From: Kan Liang <kan.liang@intel.com>

This patchkit adds the ability to turn off time stamps per event.
One usable case of partial time is to work with per-event callgraph to
enable "PEBS threshold > 1" (https://lkml.org/lkml/2015/5/10/196), which
can significantly reduce the sampling overhead.
The event samples with time stamps off will not be ordered.

Signed-off-by: Kan Liang <kan.liang@intel.com>
---
 tools/perf/Documentation/perf-record.txt |  4 +++-
 tools/perf/builtin-record.c              |  7 ++++++-
 tools/perf/builtin-trace.c               |  1 +
 tools/perf/util/evsel.c                  | 30 ++++++++++++++++++++++++++++--
 tools/perf/util/parse-events.c           | 11 +++++++++++
 tools/perf/util/parse-events.h           |  3 +++
 tools/perf/util/parse-events.l           |  1 +
 tools/perf/util/pmu.c                    |  2 +-
 8 files changed, 54 insertions(+), 5 deletions(-)

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/builtin-record.c b/tools/perf/builtin-record.c
index eae6510..a06d8c2 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -956,7 +956,6 @@ const char * const *record_usage = __record_usage;
  */
 static struct record record = {
 	.opts = {
-		.sample_time	     = true,
 		.mmap_pages	     = UINT_MAX,
 		.user_freq	     = UINT_MAX,
 		.user_interval	     = ULLONG_MAX,
@@ -1139,6 +1138,12 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 		goto out_symbol_exit;
 	}
 
+	/* If no one set time, let time = true as default */
+	if (!rec->opts.sample_time_set && !time_term_detected) {
+		rec->opts.sample_time = true;
+		rec->opts.sample_time_set = true;
+	}
+
 	if (rec->opts.target.tid && !rec->opts.no_inherit_set)
 		rec->opts.no_inherit = true;
 
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 0ebf55b..469d316 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -2899,6 +2899,7 @@ int cmd_trace(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (trace.trace_pgfaults) {
 		trace.opts.sample_address = true;
 		trace.opts.sample_time = true;
+		trace.opts.sample_time_set = true;
 	}
 
 	if (trace.evlist->nr_entries > 0)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 83c0803..34f9cfd 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) {
+		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/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;
+
 extern struct tracepoint_path *tracepoint_id_to_path(u64 config);
 extern struct tracepoint_path *tracepoint_name_to_path(const char *name);
 extern bool have_tracepoints(struct list_head *evlist);
@@ -62,6 +64,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:"))
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH RFC V5 3/4] perf,tool: per-event callgrap support
  2015-07-17 11:30 [PATCH RFC V5 0/4] per event callgrap and time support 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-17 11:30 ` kan.liang
  2015-07-17 11:30 ` [PATCH RFC V5 4/4] perf,tests: Add tests to callgrap and time parse kan.liang
  3 siblings, 0 replies; 14+ messages in thread
From: kan.liang @ 2015-07-17 11:30 UTC (permalink / raw)
  To: acme, jolsa; +Cc: namhyung, ak, linux-kernel, Kan Liang

From: Kan Liang <kan.liang@intel.com>

When multiple events are sampled it may not be needed to collect
callgraphs for all of them. The sample sites are usually nearby, and
it's enough to collect the callgraphs on a reference event (such as
precise cycles or precise instructions).
This patchkit adds the ability to turn off callgraphs and time stamp
per event. This in term can reduce sampling overhead and the size of the
perf.data. Furthermore, it makes collecting back traces and timestamps
possible when PEBS threshold > 1, which significantly reducing the
sampling overhead especially for frequently occurring events
(https://lkml.org/lkml/2015/5/10/196). For example, A slower event with
a larger period collects back traces/timestamps. Other more events run
fast with multi-pebs. The time stamps from the slower events can be used
to order the faster events. Their backtraces can give the user enough
hint to find the right spot.

Here are some examples and test results.

1. Comparing the elapsed time and perf.data size from "kernbench -M -H".

 The test command for FULL callgrap and time support.
   "perf record -e
   '{cpu/cpu-cycles,period=100000/,cpu/instructions,period=20000/p}'
   --call-graph fp --time"

 The test command for PARTIAL callgrap and time support.
   "perf record -e
   '{cpu/cpu-cycles,callgraph=fp,time,period=100000/,
     cpu/instructions,callgraph=no,time=0,period=20000/p}'"

 The elapsed time for FULL is 24.3 Sec, while for PARTIAL is 16.9 Sec.
 The perf.data size for FULL is 22.1 Gb, while for PARTIAL is 12.4 Gb.

2. Comparing the perf.data size and callgraph results.

 The test command for FULL callgrap and time support.
   "perf record -e
   '{cpu/cpu-cycles,period=100000/pp,cpu/instructions,period=20000/p}'
   --call-graph fp -- ./tchain_edit"

 The test command for PARTIAL callgrap and time support.
   "perf record -e
   '{cpu/cpu-cycles,callgraph=fp,time,period=100000/pp,
     cpu/instructions,callgraph=no,time=0,period=20000/p}'
   -- ./tchain_edit"

 The perf.data size for FULL is 43.2 MB, while for PARTIAL is 21.1 MB.
 The callgraph is roughly the same.

 The callgraph from FULL
 # Samples: 87K of event
 'cpu/cpu-cycles,callgraph=fp,time,period=100000/pp'
 # Event count (approx.): 8760000000
 #
 # Children      Self  Command      Shared Object       Symbol
 # ........  ........  ...........  ..................
..........................................
 #
    99.98%     0.00%  tchain_edit  libc-2.15.so        [.]
__libc_start_main
            |
            ---__libc_start_main

    99.97%     0.00%  tchain_edit  tchain_edit         [.] main
            |
            ---main
               __libc_start_main

    99.97%     0.00%  tchain_edit  tchain_edit         [.] f1
            |
            ---f1
               main
               __libc_start_main

    99.85%    87.01%  tchain_edit  tchain_edit         [.] f3
            |
            ---f3
               |
               |--99.74%-- f2
               |          f1
               |          main
               |          __libc_start_main
                --0.26%-- [...]
    99.71%     0.12%  tchain_edit  tchain_edit         [.] f2
            |
            ---f2
               f1
               main
               __libc_start_main

 The callgraph from PARTIAL
 # Samples: 417K of event
 'cpu/instructions,callgraph=no,time=0,period=20000/p'
 # Event count (approx.): 8346980000
 #
 # Children      Self  Command      Shared Object     Symbol
 # ........  ........  ...........  ................
..........................................
 #
    98.82%     0.00%  tchain_edit  libc-2.15.so      [.]
__libc_start_main
            |
            ---__libc_start_main

    98.82%     0.00%  tchain_edit  tchain_edit       [.] main
            |
            ---main
               __libc_start_main

    98.82%     0.00%  tchain_edit  tchain_edit       [.] f1
            |
            ---f1
               main
               __libc_start_main

    98.82%    98.28%  tchain_edit  tchain_edit       [.] f3
            |
            ---f3
               |
               |--0.53%-- f2
               |          f1
               |          main
               |          __libc_start_main
               |
               |--0.01%-- f1
               |          main
               |          __libc_start_main
                --99.46%-- [...]
    97.63%     0.03%  tchain_edit  tchain_edit       [.] f2
            |
            ---f2
               f1
               main
               __libc_start_main

     7.13%     0.03%  tchain_edit  [kernel.vmlinux]  [k] do_nmi
            |
            ---do_nmi
               end_repeat_nmi
               f3
               f2
               f1
               main
               __libc_start_main

Signed-off-by: Kan Liang <kan.liang@intel.com>
---
 tools/perf/Documentation/perf-record.txt |  4 ++++
 tools/perf/util/evsel.c                  | 24 +++++++++++++++++++++++-
 tools/perf/util/parse-events.c           | 18 ++++++++++++++++++
 tools/perf/util/parse-events.h           |  2 ++
 tools/perf/util/parse-events.l           |  2 ++
 tools/perf/util/pmu.c                    |  3 ++-
 6 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index df47907..f478dc2 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -52,6 +52,10 @@ OPTIONS
 	  - 'time': Disable/enable time stamping. Acceptable values are 1 for
 		    enabling time stamping. 0 for disabling time stamping.
 		    The default is 1.
+	  - 'callgraph': Disable/enable callgraph. Acceptable str are "fp" for
+			 FP mode, "dwarf" for DWARF mode, "lbr" for LBR mode and
+			 "no" for disable callgraph.
+	  - 'stack_size': user stack size for dwarf mode
 	  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 34f9cfd..c253ca7 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -620,6 +620,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 	int track = evsel->tracking;
 	bool per_cpu = opts->target.default_per_cpu && !opts->target.per_thread;
 	bool sample_time = opts->sample_time;
+	bool callgraph = callchain_param.enabled;
 
 	attr->sample_id_all = perf_missing_features.sample_id_all ? 0 : 1;
 	attr->inherit	    = !opts->no_inherit;
@@ -636,6 +637,23 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 			sample_time = false;
 	}
 
+	if (!opts->callgraph_set) {
+		if (attr->sample_type & PERF_SAMPLE_CALLCHAIN) {
+			callgraph = true;
+			if (attr->sample_type & PERF_SAMPLE_STACK_USER) {
+				callchain_param.record_mode = CALLCHAIN_DWARF;
+				if (attr->sample_stack_user)
+					callchain_param.dump_size = attr->sample_stack_user;
+				else
+					callchain_param.dump_size = 8192;
+			} else if (attr->sample_type & PERF_SAMPLE_BRANCH_STACK)
+				callchain_param.record_mode = CALLCHAIN_LBR;
+			else
+				callchain_param.record_mode = CALLCHAIN_FP;
+		} else
+			callgraph = false;
+	}
+
 	/*
 	 * Event parsing doesn't check the availability
 	 * Clear the bit which event parsing may be set.
@@ -648,6 +666,10 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 	if (attr->sample_type & PERF_SAMPLE_TIME)
 		perf_evsel__reset_sample_bit(evsel, TIME);
 
+	attr->sample_type &= ~(PERF_SAMPLE_CALLCHAIN |
+			       PERF_SAMPLE_STACK_USER |
+			       PERF_SAMPLE_BRANCH_STACK);
+
 	perf_evsel__set_sample_bit(evsel, IP);
 	perf_evsel__set_sample_bit(evsel, TID);
 
@@ -713,7 +735,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 	if (perf_evsel__is_function_event(evsel))
 		evsel->attr.exclude_callchain_user = 1;
 
-	if (callchain_param.enabled && !evsel->no_aux_samples)
+	if (callgraph && !evsel->no_aux_samples)
 		perf_evsel__config_callgraph(evsel, opts);
 
 	if (opts->sample_intr_regs) {
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c9981df..1a8ed26 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -19,6 +19,7 @@
 #include "thread_map.h"
 #include "cpumap.h"
 #include "asm/bug.h"
+#include "callchain.h"
 
 #define MAX_NAME_LEN 100
 
@@ -609,6 +610,23 @@ do {									   \
 		if (term->val.num == 1)
 			attr->sample_type |= PERF_SAMPLE_TIME;
 		 break;
+	case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
+		CHECK_TYPE_VAL(STR);
+		if (!strcmp(term->val.str, "fp"))
+			attr->sample_type |= PERF_SAMPLE_CALLCHAIN;
+		else if (!strcmp(term->val.str, "dwarf"))
+			attr->sample_type |= PERF_SAMPLE_CALLCHAIN |
+					     PERF_SAMPLE_STACK_USER;
+		else if (!strcmp(term->val.str, "lbr"))
+			attr->sample_type |= PERF_SAMPLE_CALLCHAIN |
+					     PERF_SAMPLE_BRANCH_STACK;
+		else if (strcmp(term->val.str, "no"))
+			return -EINVAL;
+		break;
+	case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
+		CHECK_TYPE_VAL(NUM);
+		attr->sample_stack_user = term->val.num;
+		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 1083478..47136e5 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -65,6 +65,8 @@ enum {
 	PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD,
 	PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE,
 	PARSE_EVENTS__TERM_TYPE_TIME,
+	PARSE_EVENTS__TERM_TYPE_CALLGRAPH,
+	PARSE_EVENTS__TERM_TYPE_STACKSIZE,
 };
 
 struct parse_events_term {
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index f542750..16af73b 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -184,6 +184,8 @@ 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); }
+callgraph		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_CALLGRAPH); }
+stack_size		{ 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 b615cdf..586b9fd 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -607,7 +607,8 @@ 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,time\n";
+	static const char *static_terms = "config,config1,config2,name,period,"
+					  "branch_type,time,callgraph,stack_size\n";
 	unsigned i = 0;
 
 	if (!asprintf(&str, "valid terms:"))
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH RFC V5 4/4] perf,tests: Add tests to callgrap and time parse
  2015-07-17 11:30 [PATCH RFC V5 0/4] per event callgrap and time support kan.liang
                   ` (2 preceding siblings ...)
  2015-07-17 11:30 ` [PATCH RFC V5 3/4] perf,tool: per-event callgrap support kan.liang
@ 2015-07-17 11:30 ` kan.liang
  3 siblings, 0 replies; 14+ messages in thread
From: kan.liang @ 2015-07-17 11:30 UTC (permalink / raw)
  To: acme, jolsa; +Cc: namhyung, ak, linux-kernel, Kan Liang

From: Kan Liang <kan.liang@intel.com>

Add tests in tests/parse-events.c to check callgrap and time option

Signed-off-by: Kan Liang <kan.liang@intel.com>
---
 tools/perf/tests/parse-events.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index d76963f..d6f9447 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -471,6 +471,29 @@ static int test__checkevent_pmu_name(struct perf_evlist *evlist)
 	return 0;
 }
 
+static int test__checkevent_pmu_partial_time_callgraph(struct perf_evlist *evlist)
+{
+	struct perf_evsel *evsel = perf_evlist__first(evlist);
+
+	/* cpu/config=1,callgraph=fp,time,period=100000/ */
+	TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->nr_entries);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config",  1 == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong period",  100000 == evsel->attr.sample_period);
+	TEST_ASSERT_VAL("wrong callgraph",  PERF_SAMPLE_CALLCHAIN & evsel->attr.sample_type);
+	TEST_ASSERT_VAL("wrong time",  PERF_SAMPLE_TIME & evsel->attr.sample_type);
+
+	/* cpu/config=2,callgraph=no,time=0,period=2000/ */
+	evsel = perf_evsel__next(evsel);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config",  2 == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong period",  2000 == evsel->attr.sample_period);
+	TEST_ASSERT_VAL("wrong callgraph",  !(PERF_SAMPLE_CALLCHAIN & evsel->attr.sample_type));
+	TEST_ASSERT_VAL("wrong time",  !(PERF_SAMPLE_TIME & evsel->attr.sample_type));
+
+	return 0;
+}
+
 static int test__checkevent_pmu_events(struct perf_evlist *evlist)
 {
 	struct perf_evsel *evsel = perf_evlist__first(evlist);
@@ -1547,6 +1570,11 @@ static struct evlist_test test__events_pmu[] = {
 		.check = test__checkevent_pmu_name,
 		.id    = 1,
 	},
+	{
+		.name  = "cpu/config=1,callgraph=fp,time,period=100000/,cpu/config=2,callgraph=no,time=0,period=2000/",
+		.check = test__checkevent_pmu_partial_time_callgraph,
+		.id    = 2,
+	},
 };
 
 struct terms_test {
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH RFC V5 2/4] perf,tool: per-event time support
  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
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2015-07-18 12:45 UTC (permalink / raw)
  To: kan.liang; +Cc: acme, jolsa, namhyung, ak, linux-kernel

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


thoughts?

jirka


---
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;
+
 	use_comm_exec = perf_can_comm_exec();
 
 	evlist__for_each(evlist, evsel) {

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH RFC V5 2/4] perf,tool: per-event time support
  2015-07-18 12:45   ` Jiri Olsa
@ 2015-07-19  3:21     ` Namhyung Kim
  2015-07-19 19:13       ` Jiri Olsa
  0 siblings, 1 reply; 14+ messages in thread
From: Namhyung Kim @ 2015-07-19  3:21 UTC (permalink / raw)
  To: Jiri Olsa; +Cc: kan.liang, acme, jolsa, ak, linux-kernel

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) {

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH RFC V5 2/4] perf,tool: per-event time support
  2015-07-19  3:21     ` Namhyung Kim
@ 2015-07-19 19:13       ` Jiri Olsa
  2015-07-20 15:04         ` Liang, Kan
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2015-07-19 19:13 UTC (permalink / raw)
  To: Namhyung Kim; +Cc: kan.liang, acme, jolsa, ak, linux-kernel

On Sun, Jul 19, 2015 at 12:21:28PM +0900, Namhyung Kim wrote:

SNIP

> > 
> > [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 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, ...

agreed

thanks,
jirka

^ permalink raw reply	[flat|nested] 14+ messages in thread
* RE: [PATCH RFC V5 2/4] perf,tool: per-event time support
  2015-07-19 19:13       ` Jiri Olsa
@ 2015-07-20 15:04         ` Liang, Kan
  2015-07-20 17:40           ` Jiri Olsa
  0 siblings, 1 reply; 14+ messages in thread
From: Liang, Kan @ 2015-07-20 15:04 UTC (permalink / raw)
  To: Jiri Olsa, Namhyung Kim; +Cc: acme, jolsa, ak, linux-kernel

> > > [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


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH RFC V5 2/4] perf,tool: per-event time support
  2015-07-20 15:04         ` Liang, Kan
@ 2015-07-20 17:40           ` Jiri Olsa
  2015-07-21  3:39             ` Liang, Kan
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2015-07-20 17:40 UTC (permalink / raw)
  To: Liang, Kan; +Cc: Namhyung Kim, acme, jolsa, ak, linux-kernel

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 ;-)

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

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);
 }
 
 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;

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* RE: [PATCH RFC V5 2/4] perf,tool: per-event time support
  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:59               ` Jiri Olsa
  0 siblings, 2 replies; 14+ messages in thread
From: Liang, Kan @ 2015-07-21  3:39 UTC (permalink / raw)
  To: Jiri Olsa; +Cc: Namhyung Kim, acme, jolsa, ak, linux-kernel

> 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;

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH RFC V5 2/4] perf,tool: per-event time support
  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
  1 sibling, 1 reply; 14+ messages in thread
From: Namhyung Kim @ 2015-07-21 14:45 UTC (permalink / raw)
  To: Liang, Kan; +Cc: Jiri Olsa, acme, jolsa, ak, linux-kernel

Hi,

On Tue, Jul 21, 2015 at 03:39:17AM +0000, Liang, Kan wrote:
> > 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?

Hmm.. I agree that changing current behavior is not good.  But I think
it makes more sense to prefer per-event settings over global settings
in general.

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH RFC V5 2/4] perf,tool: per-event time support
  2015-07-21 14:45               ` Namhyung Kim
@ 2015-07-21 14:56                 ` Jiri Olsa
  0 siblings, 0 replies; 14+ messages in thread
From: Jiri Olsa @ 2015-07-21 14:56 UTC (permalink / raw)
  To: Namhyung Kim; +Cc: Liang, Kan, acme, jolsa, ak, linux-kernel

On Tue, Jul 21, 2015 at 11:45:01PM +0900, Namhyung Kim wrote:

SNIP

> > > 
> > > [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?
> 
> Hmm.. I agree that changing current behavior is not good.  But I think
> it makes more sense to prefer per-event settings over global settings
> in general.

hum right.. but it's just period ATM that will be affected..
I'd risk it, since I dont think maybe people actualy used it ;-)

together with time term, which wasn't possible to change before
anyway, so there should be no problem

jirka

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH RFC V5 2/4] perf,tool: per-event time support
  2015-07-21  3:39             ` Liang, Kan
  2015-07-21 14:45               ` Namhyung Kim
@ 2015-07-21 14:59               ` Jiri Olsa
  1 sibling, 0 replies; 14+ messages in thread
From: Jiri Olsa @ 2015-07-21 14:59 UTC (permalink / raw)
  To: Liang, Kan; +Cc: Namhyung Kim, acme, jolsa, ak, linux-kernel

On Tue, Jul 21, 2015 at 03:39:17AM +0000, Liang, Kan wrote:

SNIP

> > +		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.

well, it's at the end to ensure we do the event specific setting
after everything else is done

if you set this together with group leader sampling for non leader
event, I guess you know what you're doing.. right?

jirka

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-07-21 14:59 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-17 11:30 [PATCH RFC V5 0/4] per event callgrap and time support 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
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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.