lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 20 Jul 2015 15:04:20 +0000
From:	"Liang, Kan" <kan.liang@...el.com>
To:	Jiri Olsa <jolsa@...hat.com>, Namhyung Kim <namhyung@...nel.org>
CC:	"acme@...nel.org" <acme@...nel.org>,
	"jolsa@...nel.org" <jolsa@...nel.org>,
	"ak@...ux.intel.com" <ak@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH RFC V5 2/4] perf,tool: per-event time support

> > > [jolsa@...va perf]$ ./perf record   -e 'cpu/cpu-
> cycles/,cpu/instructions,time/' kill
> > > ...
> > > [jolsa@...va perf]$ ./perf evlist -v
> > > cpu/cpu-cycles/: type: 4, size: 112, config: 0x3c, { sample_period,
> > > sample_freq }: 4000, sample_type: IP|TID|PERIOD|IDENTIFIER,
> > > read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1,
> > > enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1,
> > > mmap2: 1, comm_exec: 1
> > > cpu/instructions,time/: type: 4, size: 112, config: 0xc0, {
> > > sample_period, sample_freq }: 4000, sample_type:
> > > IP|TID|TIME|PERIOD|IDENTIFIER, read_format: ID, disabled: 1,
> > > inherit: 1, freq: 1, enable_on_exec: 1, sample_id_all: 1,
> > > exclude_guest: 1
> >
> > What about this case?
> >
> >   $ perf record -e 'cpu/cpu-cycles/,cpu/instructions,time=0/' kill
> 
> right.. hum, we need somehow separate/pospone the users term
> application to the final perf_event_attr.. spent some time on it today, but
> could not find any nice solution so far.. will try tomorrow ;-)
> 
> SNIP
> 
> > > +	 * If time (-T) is not set and we have events with TIME
> sample_type
> > > +	 * set (tracepoints or events with time term), disable timestamp
> for
> > > +	 * the rest of the events.
> > > +	 */
> > > +	if (!opts->sample_time_set && perf_evlist__has_time(evlist))
> > > +		opts->sample_time = false;
> >
> > I think it'd be better if the -T/--timestamp option gives the default
> > TIME sample_type value but it can be overridden by per-event setting.
> >

I'm not sure about it. Because, for period value, the user set specific value
by option can override the per-event setting.
I think we should make them consistent. 

$ perf record -e 'cpu/instructions,period=20000/' -c 1000 sleep 1
$ perf evlist -v
cpu/instructions,period=20000/: type: 4, size: 112, config: 0xc0,
{ sample_period, sample_freq }: 1000


How about adding a per-evsel user_time_set which indicate if the time
item is set?

$ perf record -e 'cpu/cpu-cycles/,cpu/instructions,time=0/'
$ perf evlist -v
cpu/cpu-cycles/:... sample_type: IP|TID|TIME|PERIOD|IDENTIFIER
cpu/instructions,time=0/:... sample_type: IP|TID|PERIOD|IDENTIFIER

$ perf record -e 'cpu/cpu-cycles/,cpu/instructions,time/'
$ perf evlist -v
cpu/cpu-cycles/:... sample_type: IP|TID|TIME|PERIOD|IDENTIFIER
cpu/instructions,time=0/:... sample_type: IP|TID|TIME|PERIOD|IDENTIFIER

$ perf record -T -e 'cpu/cpu-cycles/,cpu/instructions,time=0/' sleep 1
$ perf evlist -v
cpu/cpu-cycles/:... sample_type: IP|TID|TIME|PERIOD|IDENTIFIER
cpu/instructions,time=0/:... sample_type: IP|TID|TIME|PERIOD|IDENTIFIER

$perf record --no-timestamp -e 'cpu/cpu-cycles/,cpu/instructions,time=0/' sleep 1
$ perf evlist -v
cpu/cpu-cycles/:... sample_type: IP|TID|PERIOD|IDENTIFIER
cpu/instructions,time=0/:... sample_type: IP|TID|PERIOD|IDENTIFIER




diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 5b47b2c..df47907 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -49,7 +49,9 @@ OPTIONS
 	  These params can be used to set event defaults.
 	  Here is a list of the params.
 	  - 'period': Set event sampling period
-
+	  - 'time': Disable/enable time stamping. Acceptable values are 1 for
+		    enabling time stamping. 0 for disabling time stamping.
+		    The default is 1.
 	  Note: If user explicitly sets options which conflict with the params,
 	  the value set by the params will be overridden.
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 83c0803..cfa09f1 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -619,10 +619,35 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 	struct perf_event_attr *attr = &evsel->attr;
 	int track = evsel->tracking;
 	bool per_cpu = opts->target.default_per_cpu && !opts->target.per_thread;
+	bool sample_time = opts->sample_time;
 
 	attr->sample_id_all = perf_missing_features.sample_id_all ? 0 : 1;
 	attr->inherit	    = !opts->no_inherit;
 
+	/*
+	 * If user doesn't explicitly set time option,
+	 * let event attribute decide.
+	 */
+
+	if (!opts->sample_time_set && evsel->user_time_set) {
+		if (attr->sample_type & PERF_SAMPLE_TIME)
+			sample_time = true;
+		else
+			sample_time = false;
+	}
+
+	/*
+	 * Event parsing doesn't check the availability
+	 * Clear the bit which event parsing may be set.
+	 * Let following code check and reset if available
+	 *
+	 * Also, the sample size may be caculated mistakenly,
+	 * because event parsing may set the PERF_SAMPLE_TIME.
+	 * Remove the size which add in perf_evsel__init
+	 */
+	if (attr->sample_type & PERF_SAMPLE_TIME)
+		perf_evsel__reset_sample_bit(evsel, TIME);
+
 	perf_evsel__set_sample_bit(evsel, IP);
 	perf_evsel__set_sample_bit(evsel, TID);
 
@@ -705,14 +730,15 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 	/*
 	 * When the user explicitely disabled time don't force it here.
 	 */
-	if (opts->sample_time &&
+	if (sample_time &&
 	    (!perf_missing_features.sample_id_all &&
 	    (!opts->no_inherit || target__has_cpu(&opts->target) || per_cpu ||
 	     opts->sample_time_set)))
 		perf_evsel__set_sample_bit(evsel, TIME);
 
 	if (opts->raw_samples && !evsel->no_aux_samples) {
-		perf_evsel__set_sample_bit(evsel, TIME);
+		if (sample_time)
+			perf_evsel__set_sample_bit(evsel, TIME);
 		perf_evsel__set_sample_bit(evsel, RAW);
 		perf_evsel__set_sample_bit(evsel, CPU);
 	}
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index fe9f327..b654b90 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -79,6 +79,7 @@ struct perf_evsel {
 	bool			system_wide;
 	bool			tracking;
 	bool			per_pkg;
+	bool			user_time_set;
 	/* parse modifier helper */
 	int			exclude_GH;
 	int			nr_members;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index a71eeb2..6f2e10e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -561,7 +561,8 @@ static int check_type_val(struct parse_events_term *term,
 
 static int config_term(struct perf_event_attr *attr,
 		       struct parse_events_term *term,
-		       struct parse_events_error *err)
+		       struct parse_events_error *err,
+		       bool *time_set)
 {
 #define CHECK_TYPE_VAL(type)						   \
 do {									   \
@@ -598,6 +599,15 @@ do {									   \
 		 * attr->branch_sample_type = term->val.num;
 		 */
 		break;
+	case PARSE_EVENTS__TERM_TYPE_TIME:
+		if (time_set)
+			*time_set = true;
+		CHECK_TYPE_VAL(NUM);
+		if (term->val.num > 1)
+			return -EINVAL;
+		if (term->val.num == 1)
+			attr->sample_type |= PERF_SAMPLE_TIME;
+		 break;
 	case PARSE_EVENTS__TERM_TYPE_NAME:
 		CHECK_TYPE_VAL(STR);
 		break;
@@ -611,12 +621,13 @@ do {									   \
 
 static int config_attr(struct perf_event_attr *attr,
 		       struct list_head *head,
-		       struct parse_events_error *err)
+		       struct parse_events_error *err,
+		       bool *time_set)
 {
 	struct parse_events_term *term;
 
 	list_for_each_entry(term, head, list)
-		if (config_term(attr, term, err))
+		if (config_term(attr, term, err, time_set))
 			return -EINVAL;
 
 	return 0;
@@ -634,7 +645,7 @@ int parse_events_add_numeric(struct parse_events_evlist *data,
 	attr.config = config;
 
 	if (head_config &&
-	    config_attr(&attr, head_config, data->error))
+	    config_attr(&attr, head_config, data->error, NULL))
 		return -EINVAL;
 
 	return add_event(list, &data->idx, &attr, NULL);
@@ -664,6 +675,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
 	struct perf_pmu_info info;
 	struct perf_pmu *pmu;
 	struct perf_evsel *evsel;
+	bool time_set = false;
 
 	pmu = perf_pmu__find(name);
 	if (!pmu)
@@ -689,7 +701,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
 	 * Configure hardcoded terms first, no need to check
 	 * return value when called with fail == 0 ;)
 	 */
-	if (config_attr(&attr, head_config, data->error))
+	if (config_attr(&attr, head_config, data->error, &time_set))
 		return -EINVAL;
 
 	if (perf_pmu__config(pmu, &attr, head_config, data->error))
@@ -702,6 +714,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
 		evsel->scale = info.scale;
 		evsel->per_pkg = info.per_pkg;
 		evsel->snapshot = info.snapshot;
+		evsel->user_time_set = time_set;
 	}
 
 	return evsel ? 0 : -ENOMEM;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 131f29b..0d8cae3 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -62,6 +62,7 @@ enum {
 	PARSE_EVENTS__TERM_TYPE_NAME,
 	PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD,
 	PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE,
+	PARSE_EVENTS__TERM_TYPE_TIME,
 };
 
 struct parse_events_term {
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 13cef3c..f542750 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -183,6 +183,7 @@ config2			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_CONFIG2); }
 name			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NAME); }
 period			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD); }
 branch_type		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE); }
+time			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_TIME); }
 ,			{ return ','; }
 "/"			{ BEGIN(INITIAL); return '/'; }
 {name_minus}		{ return str(yyscanner, PE_NAME); }
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 7bcb8c3..b615cdf 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -607,7 +607,7 @@ static char *formats_error_string(struct list_head *formats)
 {
 	struct perf_pmu_format *format;
 	char *err, *str;
-	static const char *static_terms = "config,config1,config2,name,period,branch_type\n";
+	static const char *static_terms = "config,config1,config2,name,period,branch_type,time\n";
 	unsigned i = 0;
 
 	if (!asprintf(&str, "valid terms:"))

Thanks,
Kan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists