[PATCH 01/11] perf tools: Introduce struct perf_maps_opts

[PATCH 01/11] perf tools: Introduce struct perf_maps_opts

[Namhyung Kim]

The perf_maps_opts struct will be used for taking care of cpu/thread
maps based on user's input. Since it is used on various subcommands
it'd be better factoring it out.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/builtin-record.c |   46 +++++++++++++++++++++++-------------------
 tools/perf/builtin-test.c   |   11 ++++++---
 tools/perf/perf.h           |   11 +++++++--
 tools/perf/util/evlist.c    |    3 +-
 tools/perf/util/evsel.c     |    9 ++++---
 5 files changed, 47 insertions(+), 33 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f8d9a545dd6e..7b50316136bf 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -44,7 +44,6 @@ struct perf_record {
 	struct perf_evlist	*evlist;
 	struct perf_session	*session;
 	const char		*progname;
-	const char		*uid_str;
 	int			output;
 	unsigned int		page_size;
 	int			realtime_prio;
@@ -215,7 +214,7 @@ try_again:
 			if (err == EPERM || err == EACCES) {
 				ui__error_paranoid();
 				exit(EXIT_FAILURE);
-			} else if (err ==  ENODEV && opts->cpu_list) {
+			} else if (err ==  ENODEV && opts->maps.cpu_list) {
 				die("No such device - did you specify"
 					" an out-of-range profile CPU?\n");
 			} else if (err == EINVAL && opts->sample_id_all_avail) {
@@ -564,7 +563,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 		perf_session__process_machines(session, tool,
 					       perf_event__synthesize_guest_os);
 
-	if (!opts->system_wide)
+	if (!opts->maps.system_wide)
 		perf_event__synthesize_thread_map(tool, evsel_list->threads,
 						  process_synthesized_event,
 						  machine);
@@ -645,8 +644,10 @@ static const char * const record_usage[] = {
  */
 static struct perf_record record = {
 	.opts = {
-		.target_pid	     = -1,
-		.target_tid	     = -1,
+		.maps = {
+			.target_pid	     = -1,
+			.target_tid	     = -1,
+		},
 		.mmap_pages	     = UINT_MAX,
 		.user_freq	     = UINT_MAX,
 		.user_interval	     = ULLONG_MAX,
@@ -670,9 +671,9 @@ const struct option record_options[] = {
 		     parse_events_option),
 	OPT_CALLBACK(0, "filter", &record.evlist, "filter",
 		     "event filter", parse_filter),
-	OPT_INTEGER('p', "pid", &record.opts.target_pid,
+	OPT_INTEGER('p', "pid", &record.opts.maps.target_pid,
 		    "record events on existing process id"),
-	OPT_INTEGER('t', "tid", &record.opts.target_tid,
+	OPT_INTEGER('t', "tid", &record.opts.maps.target_tid,
 		    "record events on existing thread id"),
 	OPT_INTEGER('r', "realtime", &record.realtime_prio,
 		    "collect data with this RT SCHED_FIFO priority"),
@@ -680,11 +681,11 @@ const struct option record_options[] = {
 		    "collect data without buffering"),
 	OPT_BOOLEAN('R', "raw-samples", &record.opts.raw_samples,
 		    "collect raw sample records from all opened counters"),
-	OPT_BOOLEAN('a', "all-cpus", &record.opts.system_wide,
+	OPT_BOOLEAN('a', "all-cpus", &record.opts.maps.system_wide,
 			    "system-wide collection from all CPUs"),
 	OPT_BOOLEAN('A', "append", &record.append_file,
 			    "append to the output file to do incremental profiling"),
-	OPT_STRING('C', "cpu", &record.opts.cpu_list, "cpu",
+	OPT_STRING('C', "cpu", &record.opts.maps.cpu_list, "cpu",
 		    "list of cpus to monitor"),
 	OPT_BOOLEAN('f', "force", &record.force,
 			"overwrite existing data file (deprecated)"),
@@ -718,7 +719,7 @@ const struct option record_options[] = {
 	OPT_CALLBACK('G', "cgroup", &record.evlist, "name",
 		     "monitor event in cgroup name only",
 		     parse_cgroups),
-	OPT_STRING('u', "uid", &record.uid_str, "user", "user to profile"),
+	OPT_STRING('u', "uid", &record.opts.maps.uid_str, "user", "user to profile"),
 	OPT_END()
 };
 
@@ -739,8 +740,10 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 
 	argc = parse_options(argc, argv, record_options, record_usage,
 			    PARSE_OPT_STOP_AT_NON_OPTION);
-	if (!argc && rec->opts.target_pid == -1 && rec->opts.target_tid == -1 &&
-		!rec->opts.system_wide && !rec->opts.cpu_list && !rec->uid_str)
+	if (!argc && rec->opts.maps.target_pid == -1 &&
+	    rec->opts.maps.target_tid == -1 &&
+	    !rec->opts.maps.system_wide && !rec->opts.maps.cpu_list &&
+	    !rec->opts.maps.uid_str)
 		usage_with_options(record_usage, record_options);
 
 	if (rec->force && rec->append_file) {
@@ -753,7 +756,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 		rec->write_mode = WRITE_FORCE;
 	}
 
-	if (nr_cgroups && !rec->opts.system_wide) {
+	if (nr_cgroups && !rec->opts.maps.system_wide) {
 		fprintf(stderr, "cgroup monitoring only available in"
 			" system-wide mode\n");
 		usage_with_options(record_usage, record_options);
@@ -780,17 +783,18 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 		goto out_symbol_exit;
 	}
 
-	rec->opts.uid = parse_target_uid(rec->uid_str, rec->opts.target_tid,
-					 rec->opts.target_pid);
-	if (rec->uid_str != NULL && rec->opts.uid == UINT_MAX - 1)
+	rec->opts.maps.uid = parse_target_uid(rec->opts.maps.uid_str,
+					      rec->opts.maps.target_tid,
+					      rec->opts.maps.target_pid);
+	if (rec->opts.maps.uid_str && rec->opts.maps.uid == UINT_MAX - 1)
 		goto out_free_fd;
 
-	if (rec->opts.target_pid != -1)
-		rec->opts.target_tid = rec->opts.target_pid;
+	if (rec->opts.maps.target_pid != -1)
+		rec->opts.maps.target_tid = rec->opts.maps.target_pid;
 
-	if (perf_evlist__create_maps(evsel_list, rec->opts.target_pid,
-				     rec->opts.target_tid, rec->opts.uid,
-				     rec->opts.cpu_list) < 0)
+	if (perf_evlist__create_maps(evsel_list, rec->opts.maps.target_pid,
+				     rec->opts.maps.target_tid, rec->opts.maps.uid,
+				     rec->opts.maps.cpu_list) < 0)
 		usage_with_options(record_usage, record_options);
 
 	list_for_each_entry(pos, &evsel_list->entries, node) {
diff --git a/tools/perf/builtin-test.c b/tools/perf/builtin-test.c
index 70c4eb2bdf72..da3c1c3f8524 100644
--- a/tools/perf/builtin-test.c
+++ b/tools/perf/builtin-test.c
@@ -1010,8 +1010,10 @@ realloc:
 static int test__PERF_RECORD(void)
 {
 	struct perf_record_opts opts = {
-		.target_pid = -1,
-		.target_tid = -1,
+		.maps = {
+			.target_pid = -1,
+			.target_tid = -1,
+		},
 		.no_delay   = true,
 		.freq	    = 10,
 		.mmap_pages = 256,
@@ -1055,8 +1057,9 @@ static int test__PERF_RECORD(void)
 	 * perf_evlist__prepare_workload we'll fill in the only thread
 	 * we're monitoring, the one forked there.
 	 */
-	err = perf_evlist__create_maps(evlist, opts.target_pid,
-				       opts.target_tid, UINT_MAX, opts.cpu_list);
+	err = perf_evlist__create_maps(evlist, opts.maps.target_pid,
+				       opts.maps.target_tid, UINT_MAX,
+				       opts.maps.cpu_list);
 	if (err < 0) {
 		pr_debug("Not enough memory to create thread/cpu maps\n");
 		goto out_delete_evlist;
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 92af1688bae4..972cd99fb509 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -185,10 +185,17 @@ extern const char perf_version_string[];
 
 void pthread__unblock_sigwinch(void);
 
-struct perf_record_opts {
+struct perf_maps_opts {
 	pid_t	     target_pid;
 	pid_t	     target_tid;
 	uid_t	     uid;
+	bool	     system_wide;
+	const char   *cpu_list;
+	const char   *uid_str;
+};
+
+struct perf_record_opts {
+	struct perf_maps_opts maps;
 	bool	     call_graph;
 	bool	     group;
 	bool	     inherit_stat;
@@ -200,14 +207,12 @@ struct perf_record_opts {
 	bool	     sample_address;
 	bool	     sample_time;
 	bool	     sample_id_all_avail;
-	bool	     system_wide;
 	bool	     period;
 	unsigned int freq;
 	unsigned int mmap_pages;
 	unsigned int user_freq;
 	u64	     default_interval;
 	u64	     user_interval;
-	const char   *cpu_list;
 };
 
 #endif
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index a57a8cfc5d90..d7c3a2d5771c 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -820,7 +820,8 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist,
 		exit(-1);
 	}
 
-	if (!opts->system_wide && opts->target_tid == -1 && opts->target_pid == -1)
+	if (!opts->maps.system_wide && opts->maps.target_tid == -1 &&
+	    opts->maps.target_pid == -1)
 		evlist->threads->map[0] = evlist->workload.pid;
 
 	close(child_ready_pipe[1]);
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 9a11f9edac12..6a4e97d5f6d5 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -105,15 +105,15 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts)
 	if (opts->call_graph)
 		attr->sample_type	|= PERF_SAMPLE_CALLCHAIN;
 
-	if (opts->system_wide)
+	if (opts->maps.system_wide)
 		attr->sample_type	|= PERF_SAMPLE_CPU;
 
 	if (opts->period)
 		attr->sample_type	|= PERF_SAMPLE_PERIOD;
 
 	if (opts->sample_id_all_avail &&
-	    (opts->sample_time || opts->system_wide ||
-	     !opts->no_inherit || opts->cpu_list))
+	    (opts->sample_time || opts->maps.system_wide ||
+	     !opts->no_inherit || opts->maps.cpu_list))
 		attr->sample_type	|= PERF_SAMPLE_TIME;
 
 	if (opts->raw_samples) {
@@ -130,7 +130,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts)
 	attr->mmap = track;
 	attr->comm = track;
 
-	if (opts->target_pid == -1 && opts->target_tid == -1 && !opts->system_wide) {
+	if (opts->maps.target_pid == -1 && opts->maps.target_tid == -1 &&
+	    !opts->maps.system_wide) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}
-- 
1.7.9

[RFC PATCHSET] perf: Fix cpu/thread map and group event handling

[Namhyung Kim]

Resend as it was not sent to LKML due to my cccmd problem.

=====================
Hello,

When I was playing with event group, I found a bug that didn't count
such events properly. After looking at the code, it seemed there were
few more bugs, so I decided to refactor the code during fixing them.

The first one I found was that 'perf stat --group' doesn't count any
event but the first. It was a bug on setting attr->disabled and
attr->enable_on_exec to a member of a group event. And it was related
to the code which decides the target task/cpu that those events are
attached to. They did same things, but were separated to each file.

To clean it up, I've made a new struct and used it for record, stat,
top and test. During the cleanup I found some combinations of PID/TID,
UID and CPU weren't allowed and had implicit behaviors. For example,
'perf record -- sleep 1' will create multiple events as much as number
of online cpus on the system. I don't think it's intended. So they
should be fixed and warned to user explicitly, if needed.

I think we have following prorities and implications:

 * If -p option is given, -t option should be ignored.
 * If -p or -t option is given, -u, -C and/or -a options should be ignored.
 * If -u option is given, -C and/or -a option should be ignored.

And if only -C option is given, it should be treated as a system-wide
mode. Also if *NO* option is given (i.e. fork a child for command line
argument), it should be treated as a task mode, not a system-wide mode.

These patches are based on latest tip/perf/core: f8d98f109521 ("x86:
Fix to decode grouped AVX with VEX pp bits").

Any comments are welcome.
Thanks.

Namhyung Kim (11):
  perf tools: Introduce struct perf_maps_opts
  perf stat: Convert to perf_maps_opts
  perf top: Convert to perf_maps_opts
  perf tools: Introduce check_target_maps() helper
  perf tools: Make perf_evlist__create_maps() take struct perf_maps_opts
  perf tools: Check more combinations of PID/TID, UID and CPU switches
  perf tools: Fix creation of cpu map
  perf tools: Consolidate target task/cpu checking
  perf stat: Use perf_evlist__create_maps
  perf stat: Fix event grouping on forked task
  perf record: Fix event grouping on forked task

 tools/perf/builtin-record.c |   42 ++++++++++++++----------------
 tools/perf/builtin-stat.c   |   56 ++++++++++++++++------------------------
 tools/perf/builtin-test.c   |   12 +++++---
 tools/perf/builtin-top.c    |   41 ++++++++++++-----------------
 tools/perf/perf.h           |   11 ++++++--
 tools/perf/util/evlist.c    |   20 ++++++++------
 tools/perf/util/evlist.h    |    7 +++--
 tools/perf/util/evsel.c     |   11 ++++---
 tools/perf/util/evsel.h     |    3 +-
 tools/perf/util/top.c       |   15 ++++++-----
 tools/perf/util/top.h       |    4 +--
 tools/perf/util/usage.c     |   59 +++++++++++++++++++++++++++++++++++++------
 tools/perf/util/util.h      |    7 ++++-
 13 files changed, 163 insertions(+), 125 deletions(-)

-- 
1.7.9

Re: [PATCH 01/11] perf tools: Introduce struct perf_maps_opts

[Arnaldo Carvalho de Melo]

Em Mon, Feb 13, 2012 at 04:27:33PM +0900, Namhyung Kim escreveu:
> The perf_maps_opts struct will be used for taking care of cpu/thread
> maps based on user's input. Since it is used on various subcommands
> it'd be better factoring it out.

I think 'struct perf_target' is a better name than 'struct
perf_maps_opts'.

Then you can remove the 'target_' prefix from pid and tid:

struct perf_target {
	pid_t	     pid;
	pid_t	     tid;
	uid_t	     uid;
	const char   *cpu_list;
	const char   *uid_str;
	bool	     system_wide;
};

Also bear in mind that this patch will clash with David Ahern's patch
for supporting pid and tid lists, I'll try to integrate it today and
then you can work on top of it in my perf/core branch, ok?

Now to the other patches...

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
> ---
>  tools/perf/builtin-record.c |   46 +++++++++++++++++++++++-------------------
>  tools/perf/builtin-test.c   |   11 ++++++---
>  tools/perf/perf.h           |   11 +++++++--
>  tools/perf/util/evlist.c    |    3 +-
>  tools/perf/util/evsel.c     |    9 ++++---
>  5 files changed, 47 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index f8d9a545dd6e..7b50316136bf 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -44,7 +44,6 @@ struct perf_record {
>  	struct perf_evlist	*evlist;
>  	struct perf_session	*session;
>  	const char		*progname;
> -	const char		*uid_str;
>  	int			output;
>  	unsigned int		page_size;
>  	int			realtime_prio;
> @@ -215,7 +214,7 @@ try_again:
>  			if (err == EPERM || err == EACCES) {
>  				ui__error_paranoid();
>  				exit(EXIT_FAILURE);
> -			} else if (err ==  ENODEV && opts->cpu_list) {
> +			} else if (err ==  ENODEV && opts->maps.cpu_list) {
>  				die("No such device - did you specify"
>  					" an out-of-range profile CPU?\n");
>  			} else if (err == EINVAL && opts->sample_id_all_avail) {
> @@ -564,7 +563,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>  		perf_session__process_machines(session, tool,
>  					       perf_event__synthesize_guest_os);
>  
> -	if (!opts->system_wide)
> +	if (!opts->maps.system_wide)
>  		perf_event__synthesize_thread_map(tool, evsel_list->threads,
>  						  process_synthesized_event,
>  						  machine);
> @@ -645,8 +644,10 @@ static const char * const record_usage[] = {
>   */
>  static struct perf_record record = {
>  	.opts = {
> -		.target_pid	     = -1,
> -		.target_tid	     = -1,
> +		.maps = {
> +			.target_pid	     = -1,
> +			.target_tid	     = -1,
> +		},
>  		.mmap_pages	     = UINT_MAX,
>  		.user_freq	     = UINT_MAX,
>  		.user_interval	     = ULLONG_MAX,
> @@ -670,9 +671,9 @@ const struct option record_options[] = {
>  		     parse_events_option),
>  	OPT_CALLBACK(0, "filter", &record.evlist, "filter",
>  		     "event filter", parse_filter),
> -	OPT_INTEGER('p', "pid", &record.opts.target_pid,
> +	OPT_INTEGER('p', "pid", &record.opts.maps.target_pid,
>  		    "record events on existing process id"),
> -	OPT_INTEGER('t', "tid", &record.opts.target_tid,
> +	OPT_INTEGER('t', "tid", &record.opts.maps.target_tid,
>  		    "record events on existing thread id"),
>  	OPT_INTEGER('r', "realtime", &record.realtime_prio,
>  		    "collect data with this RT SCHED_FIFO priority"),
> @@ -680,11 +681,11 @@ const struct option record_options[] = {
>  		    "collect data without buffering"),
>  	OPT_BOOLEAN('R', "raw-samples", &record.opts.raw_samples,
>  		    "collect raw sample records from all opened counters"),
> -	OPT_BOOLEAN('a', "all-cpus", &record.opts.system_wide,
> +	OPT_BOOLEAN('a', "all-cpus", &record.opts.maps.system_wide,
>  			    "system-wide collection from all CPUs"),
>  	OPT_BOOLEAN('A', "append", &record.append_file,
>  			    "append to the output file to do incremental profiling"),
> -	OPT_STRING('C', "cpu", &record.opts.cpu_list, "cpu",
> +	OPT_STRING('C', "cpu", &record.opts.maps.cpu_list, "cpu",
>  		    "list of cpus to monitor"),
>  	OPT_BOOLEAN('f', "force", &record.force,
>  			"overwrite existing data file (deprecated)"),
> @@ -718,7 +719,7 @@ const struct option record_options[] = {
>  	OPT_CALLBACK('G', "cgroup", &record.evlist, "name",
>  		     "monitor event in cgroup name only",
>  		     parse_cgroups),
> -	OPT_STRING('u', "uid", &record.uid_str, "user", "user to profile"),
> +	OPT_STRING('u', "uid", &record.opts.maps.uid_str, "user", "user to profile"),
>  	OPT_END()
>  };
>  
> @@ -739,8 +740,10 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
>  
>  	argc = parse_options(argc, argv, record_options, record_usage,
>  			    PARSE_OPT_STOP_AT_NON_OPTION);
> -	if (!argc && rec->opts.target_pid == -1 && rec->opts.target_tid == -1 &&
> -		!rec->opts.system_wide && !rec->opts.cpu_list && !rec->uid_str)
> +	if (!argc && rec->opts.maps.target_pid == -1 &&
> +	    rec->opts.maps.target_tid == -1 &&
> +	    !rec->opts.maps.system_wide && !rec->opts.maps.cpu_list &&
> +	    !rec->opts.maps.uid_str)
>  		usage_with_options(record_usage, record_options);
>  
>  	if (rec->force && rec->append_file) {
> @@ -753,7 +756,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
>  		rec->write_mode = WRITE_FORCE;
>  	}
>  
> -	if (nr_cgroups && !rec->opts.system_wide) {
> +	if (nr_cgroups && !rec->opts.maps.system_wide) {
>  		fprintf(stderr, "cgroup monitoring only available in"
>  			" system-wide mode\n");
>  		usage_with_options(record_usage, record_options);
> @@ -780,17 +783,18 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
>  		goto out_symbol_exit;
>  	}
>  
> -	rec->opts.uid = parse_target_uid(rec->uid_str, rec->opts.target_tid,
> -					 rec->opts.target_pid);
> -	if (rec->uid_str != NULL && rec->opts.uid == UINT_MAX - 1)
> +	rec->opts.maps.uid = parse_target_uid(rec->opts.maps.uid_str,
> +					      rec->opts.maps.target_tid,
> +					      rec->opts.maps.target_pid);
> +	if (rec->opts.maps.uid_str && rec->opts.maps.uid == UINT_MAX - 1)
>  		goto out_free_fd;
>  
> -	if (rec->opts.target_pid != -1)
> -		rec->opts.target_tid = rec->opts.target_pid;
> +	if (rec->opts.maps.target_pid != -1)
> +		rec->opts.maps.target_tid = rec->opts.maps.target_pid;
>  
> -	if (perf_evlist__create_maps(evsel_list, rec->opts.target_pid,
> -				     rec->opts.target_tid, rec->opts.uid,
> -				     rec->opts.cpu_list) < 0)
> +	if (perf_evlist__create_maps(evsel_list, rec->opts.maps.target_pid,
> +				     rec->opts.maps.target_tid, rec->opts.maps.uid,
> +				     rec->opts.maps.cpu_list) < 0)
>  		usage_with_options(record_usage, record_options);
>  
>  	list_for_each_entry(pos, &evsel_list->entries, node) {
> diff --git a/tools/perf/builtin-test.c b/tools/perf/builtin-test.c
> index 70c4eb2bdf72..da3c1c3f8524 100644
> --- a/tools/perf/builtin-test.c
> +++ b/tools/perf/builtin-test.c
> @@ -1010,8 +1010,10 @@ realloc:
>  static int test__PERF_RECORD(void)
>  {
>  	struct perf_record_opts opts = {
> -		.target_pid = -1,
> -		.target_tid = -1,
> +		.maps = {
> +			.target_pid = -1,
> +			.target_tid = -1,
> +		},
>  		.no_delay   = true,
>  		.freq	    = 10,
>  		.mmap_pages = 256,
> @@ -1055,8 +1057,9 @@ static int test__PERF_RECORD(void)
>  	 * perf_evlist__prepare_workload we'll fill in the only thread
>  	 * we're monitoring, the one forked there.
>  	 */
> -	err = perf_evlist__create_maps(evlist, opts.target_pid,
> -				       opts.target_tid, UINT_MAX, opts.cpu_list);
> +	err = perf_evlist__create_maps(evlist, opts.maps.target_pid,
> +				       opts.maps.target_tid, UINT_MAX,
> +				       opts.maps.cpu_list);
>  	if (err < 0) {
>  		pr_debug("Not enough memory to create thread/cpu maps\n");
>  		goto out_delete_evlist;
> diff --git a/tools/perf/perf.h b/tools/perf/perf.h
> index 92af1688bae4..972cd99fb509 100644
> --- a/tools/perf/perf.h
> +++ b/tools/perf/perf.h
> @@ -185,10 +185,17 @@ extern const char perf_version_string[];
>  
>  void pthread__unblock_sigwinch(void);
>  
> -struct perf_record_opts {
> +struct perf_maps_opts {
>  	pid_t	     target_pid;
>  	pid_t	     target_tid;
>  	uid_t	     uid;
> +	bool	     system_wide;
> +	const char   *cpu_list;
> +	const char   *uid_str;
> +};
> +
> +struct perf_record_opts {
> +	struct perf_maps_opts maps;
>  	bool	     call_graph;
>  	bool	     group;
>  	bool	     inherit_stat;
> @@ -200,14 +207,12 @@ struct perf_record_opts {
>  	bool	     sample_address;
>  	bool	     sample_time;
>  	bool	     sample_id_all_avail;
> -	bool	     system_wide;
>  	bool	     period;
>  	unsigned int freq;
>  	unsigned int mmap_pages;
>  	unsigned int user_freq;
>  	u64	     default_interval;
>  	u64	     user_interval;
> -	const char   *cpu_list;
>  };
>  
>  #endif
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index a57a8cfc5d90..d7c3a2d5771c 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -820,7 +820,8 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist,
>  		exit(-1);
>  	}
>  
> -	if (!opts->system_wide && opts->target_tid == -1 && opts->target_pid == -1)
> +	if (!opts->maps.system_wide && opts->maps.target_tid == -1 &&
> +	    opts->maps.target_pid == -1)
>  		evlist->threads->map[0] = evlist->workload.pid;
>  
>  	close(child_ready_pipe[1]);
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 9a11f9edac12..6a4e97d5f6d5 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -105,15 +105,15 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts)
>  	if (opts->call_graph)
>  		attr->sample_type	|= PERF_SAMPLE_CALLCHAIN;
>  
> -	if (opts->system_wide)
> +	if (opts->maps.system_wide)
>  		attr->sample_type	|= PERF_SAMPLE_CPU;
>  
>  	if (opts->period)
>  		attr->sample_type	|= PERF_SAMPLE_PERIOD;
>  
>  	if (opts->sample_id_all_avail &&
> -	    (opts->sample_time || opts->system_wide ||
> -	     !opts->no_inherit || opts->cpu_list))
> +	    (opts->sample_time || opts->maps.system_wide ||
> +	     !opts->no_inherit || opts->maps.cpu_list))
>  		attr->sample_type	|= PERF_SAMPLE_TIME;
>  
>  	if (opts->raw_samples) {
> @@ -130,7 +130,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts)
>  	attr->mmap = track;
>  	attr->comm = track;
>  
> -	if (opts->target_pid == -1 && opts->target_tid == -1 && !opts->system_wide) {
> +	if (opts->maps.target_pid == -1 && opts->maps.target_tid == -1 &&
> +	    !opts->maps.system_wide) {
>  		attr->disabled = 1;
>  		attr->enable_on_exec = 1;
>  	}
> -- 
> 1.7.9

Re: [PATCH 01/11] perf tools: Introduce struct perf_maps_opts

[David Ahern]

On 02/13/2012 11:32 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Feb 13, 2012 at 04:27:33PM +0900, Namhyung Kim escreveu:
>> The perf_maps_opts struct will be used for taking care of cpu/thread
>> maps based on user's input. Since it is used on various subcommands
>> it'd be better factoring it out.
> 
> I think 'struct perf_target' is a better name than 'struct
> perf_maps_opts'.
> 
> Then you can remove the 'target_' prefix from pid and tid:
> 
> struct perf_target {
> 	pid_t	     pid;
> 	pid_t	     tid;
> 	uid_t	     uid;
> 	const char   *cpu_list;
> 	const char   *uid_str;
> 	bool	     system_wide;
> };
> 
> Also bear in mind that this patch will clash with David Ahern's patch
> for supporting pid and tid lists, I'll try to integrate it today and
> then you can work on top of it in my perf/core branch, ok?
> 
> Now to the other patches...
> 
> - Arnaldo
>  

The cleanup might make my multiple tid/pid patch easier: e.g.,

struct perf_target{
	...
	char errmsg[128];
};

Then if the tid/pid string parsing fails in perf_evlist__create_maps and
friends the errmsg can be put into the buffer for the callers to get a
more useful message to the user as to what happened.

Today's perf if you give it an invalid pid, scandir fails and the
command spits out the usage statement. Which is completely confusing --
ie., not clear that the command failed b/c the pid does not exist.

David

Re: [PATCH 01/11] perf tools: Introduce struct perf_maps_opts

[Arnaldo Carvalho de Melo]

Em Mon, Feb 13, 2012 at 11:50:29AM -0700, David Ahern escreveu:
> The cleanup might make my multiple tid/pid patch easier: e.g.,
> 
> struct perf_target{
> 	...
> 	char errmsg[128];
> };
> 
> Then if the tid/pid string parsing fails in perf_evlist__create_maps and
> friends the errmsg can be put into the buffer for the callers to get a
> more useful message to the user as to what happened.
> 
> Today's perf if you give it an invalid pid, scandir fails and the
> command spits out the usage statement. Which is completely confusing --
> ie., not clear that the command failed b/c the pid does not exist.

Humm, ok, but then I think we should have an enum + a strerror(3)
equivalent, i.e.:

	enum perf_target_error perf_evlist__create_maps(...);

	int perf_target__strerror(struct perf_target *target, int errnum,
				  char *buf, size_t buflen);

Please see 'man strerror_r", and make it work like the POSIX compliant
variant.

Ok, so it may be better to first process Kim's patches and then you
rework yours?

- Arnaldo

Re: [PATCH 01/11] perf tools: Introduce struct perf_maps_opts

[David Ahern]

On 02/13/2012 12:05 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Feb 13, 2012 at 11:50:29AM -0700, David Ahern escreveu:
>> The cleanup might make my multiple tid/pid patch easier: e.g.,
>>
>> struct perf_target{
>> 	...
>> 	char errmsg[128];
>> };
>>
>> Then if the tid/pid string parsing fails in perf_evlist__create_maps and
>> friends the errmsg can be put into the buffer for the callers to get a
>> more useful message to the user as to what happened.
>>
>> Today's perf if you give it an invalid pid, scandir fails and the
>> command spits out the usage statement. Which is completely confusing --
>> ie., not clear that the command failed b/c the pid does not exist.
> 
> Humm, ok, but then I think we should have an enum + a strerror(3)
> equivalent, i.e.:
> 
> 	enum perf_target_error perf_evlist__create_maps(...);
> 
> 	int perf_target__strerror(struct perf_target *target, int errnum,
> 				  char *buf, size_t buflen);

ok, so you are proposing an internal generation of enum error codes and
correlating them to strings rather than adding a buffer into
perf_target. If that's the case perhaps we need a libperf-wide design:

enum perf_error
perf__strerror(enum perf_error)

which effectively taps an array similar to _sys_errlist_internal based
on enum index.

> 
> Please see 'man strerror_r", and make it work like the POSIX compliant
> variant.

No globals are in use, so I would expect the _r to be redundant. I have
glibc source; scanning __strerror_r implementation ....

> 
> Ok, so it may be better to first process Kim's patches and then you
> rework yours?

The current patch is ready to go; I just don't like the error handling
and lack of a useful message. That said, it is no worse than what
happens today.

David

> 
> - Arnaldo

Re: [PATCH 01/11] perf tools: Introduce struct perf_maps_opts

[Arnaldo Carvalho de Melo]

Em Mon, Feb 13, 2012 at 12:19:37PM -0700, David Ahern escreveu:
> 
> 
> On 02/13/2012 12:05 PM, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Feb 13, 2012 at 11:50:29AM -0700, David Ahern escreveu:
> >> Today's perf if you give it an invalid pid, scandir fails and the
> >> command spits out the usage statement. Which is completely confusing --
> >> ie., not clear that the command failed b/c the pid does not exist.

> > Humm, ok, but then I think we should have an enum + a strerror(3)
> > equivalent, i.e.:

> > 	enum perf_target_error perf_evlist__create_maps(...);

> > 	int perf_target__strerror(struct perf_target *target, int errnum,
> > 				  char *buf, size_t buflen);

> ok, so you are proposing an internal generation of enum error codes and
> correlating them to strings rather than adding a buffer into
> perf_target. If that's the case perhaps we need a libperf-wide design:

> enum perf_error perf__strerror(enum perf_error)
> 
> which effectively taps an array similar to _sys_errlist_internal based
> on enum index.

I think a per class mechanism is better. I.e. some errors are too
specific.

I couldn't find any standard way to know the max errno value used :-\ If
we had that we could reuse strerror_r and use a different range for per
class specific errnos, i.e.:

 int perf_target__strerror(struct perf_target *target, int errnum,
                           char *buf, size_t buflen)
 {
	if (errnum < MAX_ERRNO)
		return strerror_r(errnum, buf, buflen);

	errnum -= MAX_ERRNO;

	if (errnum >= PERF_TARGET__MAX_ERRNO)
		return -1;

	snprintf(buf, buflen, "%s", perf_target__error_str[errnum]);
	return 0;
 }

 
> > Please see 'man strerror_r", and make it work like the POSIX compliant
> > variant.
> 
> No globals are in use, so I would expect the _r to be redundant. I have
> glibc source; scanning __strerror_r implementation ....
> 
> > 
> > Ok, so it may be better to first process Kim's patches and then you
> > rework yours?
> 
> The current patch is ready to go; I just don't like the error handling
> and lack of a useful message. That said, it is no worse than what
> happens today.

Yeah, we can go with what you have and then add the
perf_target__strerror on top, I'll read it now.

- Arnaldo

[PATCH 02/11] perf stat: Convert to perf_maps_opts

[Namhyung Kim]

Use struct perf_maps_opts as it is introduces by previous patch.
This is a preparation of further changes.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/builtin-stat.c |   50 +++++++++++++++++++++++---------------------
 1 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d14b37ad7638..14a30c2b9fe0 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -175,22 +175,17 @@ static struct perf_event_attr very_very_detailed_attrs[] = {
 
 struct perf_evlist		*evsel_list;
 
-static bool			system_wide			=  false;
 static int			run_idx				=  0;
-
 static int			run_count			=  1;
 static bool			no_inherit			= false;
 static bool			scale				=  true;
 static bool			no_aggr				= false;
-static pid_t			target_pid			= -1;
-static pid_t			target_tid			= -1;
 static pid_t			child_pid			= -1;
 static bool			null_run			=  false;
 static int			detailed_run			=  0;
 static bool			sync_run			=  false;
 static bool			big_num				=  true;
 static int			big_num_opt			=  -1;
-static const char		*cpu_list;
 static const char		*csv_sep			= NULL;
 static bool			csv_output			= false;
 static bool			group				= false;
@@ -198,6 +193,11 @@ static const char		*output_name			= NULL;
 static FILE			*output				= NULL;
 static int			output_fd;
 
+static struct perf_maps_opts maps = {
+	.target_pid		= -1,
+	.target_tid		= -1,
+};
+
 static volatile int done = 0;
 
 struct stats
@@ -293,10 +293,10 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
 
 	attr->inherit = !no_inherit;
 
-	if (system_wide)
+	if (maps.system_wide)
 		return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
 						group, group_fd);
-	if (target_pid == -1 && target_tid == -1) {
+	if (maps.target_pid == -1 && maps.target_tid == -1) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}
@@ -446,7 +446,8 @@ static int run_perf_stat(int argc __used, const char **argv)
 			exit(-1);
 		}
 
-		if (target_tid == -1 && target_pid == -1 && !system_wide)
+		if (maps.target_tid == -1 && maps.target_pid == -1 &&
+		    !maps.system_wide)
 			evsel_list->threads->map[0] = child_pid;
 
 		/*
@@ -476,7 +477,7 @@ static int run_perf_stat(int argc __used, const char **argv)
 				error("You may not have permission to collect %sstats.\n"
 				      "\t Consider tweaking"
 				      " /proc/sys/kernel/perf_event_paranoid or running as root.",
-				      system_wide ? "system-wide " : "");
+				      maps.system_wide ? "system-wide " : "");
 			} else {
 				error("open_counter returned with %d (%s). "
 				      "/bin/dmesg may provide additional information.\n",
@@ -968,14 +969,14 @@ static void print_stat(int argc, const char **argv)
 	if (!csv_output) {
 		fprintf(output, "\n");
 		fprintf(output, " Performance counter stats for ");
-		if(target_pid == -1 && target_tid == -1) {
+		if(maps.target_pid == -1 && maps.target_tid == -1) {
 			fprintf(output, "\'%s", argv[0]);
 			for (i = 1; i < argc; i++)
 				fprintf(output, " %s", argv[i]);
-		} else if (target_pid != -1)
-			fprintf(output, "process id \'%d", target_pid);
+		} else if (maps.target_pid != -1)
+			fprintf(output, "process id \'%d", maps.target_pid);
 		else
-			fprintf(output, "thread id \'%d", target_tid);
+			fprintf(output, "thread id \'%d", maps.target_tid);
 
 		fprintf(output, "\'");
 		if (run_count > 1)
@@ -1049,11 +1050,11 @@ static const struct option options[] = {
 		     "event filter", parse_filter),
 	OPT_BOOLEAN('i', "no-inherit", &no_inherit,
 		    "child tasks do not inherit counters"),
-	OPT_INTEGER('p', "pid", &target_pid,
+	OPT_INTEGER('p', "pid", &maps.target_pid,
 		    "stat events on existing process id"),
-	OPT_INTEGER('t', "tid", &target_tid,
+	OPT_INTEGER('t', "tid", &maps.target_tid,
 		    "stat events on existing thread id"),
-	OPT_BOOLEAN('a', "all-cpus", &system_wide,
+	OPT_BOOLEAN('a', "all-cpus", &maps.system_wide,
 		    "system-wide collection from all CPUs"),
 	OPT_BOOLEAN('g', "group", &group,
 		    "put the counters into a counter group"),
@@ -1072,7 +1073,7 @@ static const struct option options[] = {
 	OPT_CALLBACK_NOOPT('B', "big-num", NULL, NULL, 
 			   "print large numbers with thousands\' separators",
 			   stat__set_big_num),
-	OPT_STRING('C', "cpu", &cpu_list, "cpu",
+	OPT_STRING('C', "cpu", &maps.cpu_list, "cpu",
 		    "list of cpus to monitor in system-wide"),
 	OPT_BOOLEAN('A', "no-aggr", &no_aggr,
 		    "disable CPU count aggregation"),
@@ -1190,13 +1191,13 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
 	} else if (big_num_opt == 0) /* User passed --no-big-num */
 		big_num = false;
 
-	if (!argc && target_pid == -1 && target_tid == -1)
+	if (!argc && maps.target_pid == -1 && maps.target_tid == -1)
 		usage_with_options(stat_usage, options);
 	if (run_count <= 0)
 		usage_with_options(stat_usage, options);
 
 	/* no_aggr, cgroup are for system-wide only */
-	if ((no_aggr || nr_cgroups) && !system_wide) {
+	if ((no_aggr || nr_cgroups) && !maps.system_wide) {
 		fprintf(stderr, "both cgroup and no-aggregation "
 			"modes only available in system-wide mode\n");
 
@@ -1206,17 +1207,18 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
 	if (add_default_attributes())
 		goto out;
 
-	if (target_pid != -1)
-		target_tid = target_pid;
+	if (maps.target_pid != -1)
+		maps.target_tid = maps.target_pid;
 
-	evsel_list->threads = thread_map__new(target_pid, target_tid, UINT_MAX);
+	evsel_list->threads = thread_map__new(maps.target_pid, maps.target_tid,
+					      UINT_MAX);
 	if (evsel_list->threads == NULL) {
 		pr_err("Problems finding threads of monitor\n");
 		usage_with_options(stat_usage, options);
 	}
 
-	if (system_wide)
-		evsel_list->cpus = cpu_map__new(cpu_list);
+	if (maps.system_wide)
+		evsel_list->cpus = cpu_map__new(maps.cpu_list);
 	else
 		evsel_list->cpus = cpu_map__dummy_new();
 
-- 
1.7.9

Re: [PATCH 02/11] perf stat: Convert to perf_maps_opts

[Arnaldo Carvalho de Melo]

Em Mon, Feb 13, 2012 at 04:27:34PM +0900, Namhyung Kim escreveu:
> Use struct perf_maps_opts as it is introduces by previous patch.
> This is a preparation of further changes.

This one ok, as the other ones that are just makes the tools use 'struct
perf_target'

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
> ---
>  tools/perf/builtin-stat.c |   50 +++++++++++++++++++++++---------------------
>  1 files changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index d14b37ad7638..14a30c2b9fe0 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -175,22 +175,17 @@ static struct perf_event_attr very_very_detailed_attrs[] = {
>  
>  struct perf_evlist		*evsel_list;
>  
> -static bool			system_wide			=  false;
>  static int			run_idx				=  0;
> -
>  static int			run_count			=  1;
>  static bool			no_inherit			= false;
>  static bool			scale				=  true;
>  static bool			no_aggr				= false;
> -static pid_t			target_pid			= -1;
> -static pid_t			target_tid			= -1;
>  static pid_t			child_pid			= -1;
>  static bool			null_run			=  false;
>  static int			detailed_run			=  0;
>  static bool			sync_run			=  false;
>  static bool			big_num				=  true;
>  static int			big_num_opt			=  -1;
> -static const char		*cpu_list;
>  static const char		*csv_sep			= NULL;
>  static bool			csv_output			= false;
>  static bool			group				= false;
> @@ -198,6 +193,11 @@ static const char		*output_name			= NULL;
>  static FILE			*output				= NULL;
>  static int			output_fd;
>  
> +static struct perf_maps_opts maps = {
> +	.target_pid		= -1,
> +	.target_tid		= -1,
> +};
> +
>  static volatile int done = 0;
>  
>  struct stats
> @@ -293,10 +293,10 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
>  
>  	attr->inherit = !no_inherit;
>  
> -	if (system_wide)
> +	if (maps.system_wide)
>  		return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
>  						group, group_fd);
> -	if (target_pid == -1 && target_tid == -1) {
> +	if (maps.target_pid == -1 && maps.target_tid == -1) {
>  		attr->disabled = 1;
>  		attr->enable_on_exec = 1;
>  	}
> @@ -446,7 +446,8 @@ static int run_perf_stat(int argc __used, const char **argv)
>  			exit(-1);
>  		}
>  
> -		if (target_tid == -1 && target_pid == -1 && !system_wide)
> +		if (maps.target_tid == -1 && maps.target_pid == -1 &&
> +		    !maps.system_wide)
>  			evsel_list->threads->map[0] = child_pid;
>  
>  		/*
> @@ -476,7 +477,7 @@ static int run_perf_stat(int argc __used, const char **argv)
>  				error("You may not have permission to collect %sstats.\n"
>  				      "\t Consider tweaking"
>  				      " /proc/sys/kernel/perf_event_paranoid or running as root.",
> -				      system_wide ? "system-wide " : "");
> +				      maps.system_wide ? "system-wide " : "");
>  			} else {
>  				error("open_counter returned with %d (%s). "
>  				      "/bin/dmesg may provide additional information.\n",
> @@ -968,14 +969,14 @@ static void print_stat(int argc, const char **argv)
>  	if (!csv_output) {
>  		fprintf(output, "\n");
>  		fprintf(output, " Performance counter stats for ");
> -		if(target_pid == -1 && target_tid == -1) {
> +		if(maps.target_pid == -1 && maps.target_tid == -1) {
>  			fprintf(output, "\'%s", argv[0]);
>  			for (i = 1; i < argc; i++)
>  				fprintf(output, " %s", argv[i]);
> -		} else if (target_pid != -1)
> -			fprintf(output, "process id \'%d", target_pid);
> +		} else if (maps.target_pid != -1)
> +			fprintf(output, "process id \'%d", maps.target_pid);
>  		else
> -			fprintf(output, "thread id \'%d", target_tid);
> +			fprintf(output, "thread id \'%d", maps.target_tid);
>  
>  		fprintf(output, "\'");
>  		if (run_count > 1)
> @@ -1049,11 +1050,11 @@ static const struct option options[] = {
>  		     "event filter", parse_filter),
>  	OPT_BOOLEAN('i', "no-inherit", &no_inherit,
>  		    "child tasks do not inherit counters"),
> -	OPT_INTEGER('p', "pid", &target_pid,
> +	OPT_INTEGER('p', "pid", &maps.target_pid,
>  		    "stat events on existing process id"),
> -	OPT_INTEGER('t', "tid", &target_tid,
> +	OPT_INTEGER('t', "tid", &maps.target_tid,
>  		    "stat events on existing thread id"),
> -	OPT_BOOLEAN('a', "all-cpus", &system_wide,
> +	OPT_BOOLEAN('a', "all-cpus", &maps.system_wide,
>  		    "system-wide collection from all CPUs"),
>  	OPT_BOOLEAN('g', "group", &group,
>  		    "put the counters into a counter group"),
> @@ -1072,7 +1073,7 @@ static const struct option options[] = {
>  	OPT_CALLBACK_NOOPT('B', "big-num", NULL, NULL, 
>  			   "print large numbers with thousands\' separators",
>  			   stat__set_big_num),
> -	OPT_STRING('C', "cpu", &cpu_list, "cpu",
> +	OPT_STRING('C', "cpu", &maps.cpu_list, "cpu",
>  		    "list of cpus to monitor in system-wide"),
>  	OPT_BOOLEAN('A', "no-aggr", &no_aggr,
>  		    "disable CPU count aggregation"),
> @@ -1190,13 +1191,13 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
>  	} else if (big_num_opt == 0) /* User passed --no-big-num */
>  		big_num = false;
>  
> -	if (!argc && target_pid == -1 && target_tid == -1)
> +	if (!argc && maps.target_pid == -1 && maps.target_tid == -1)
>  		usage_with_options(stat_usage, options);
>  	if (run_count <= 0)
>  		usage_with_options(stat_usage, options);
>  
>  	/* no_aggr, cgroup are for system-wide only */
> -	if ((no_aggr || nr_cgroups) && !system_wide) {
> +	if ((no_aggr || nr_cgroups) && !maps.system_wide) {
>  		fprintf(stderr, "both cgroup and no-aggregation "
>  			"modes only available in system-wide mode\n");
>  
> @@ -1206,17 +1207,18 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
>  	if (add_default_attributes())
>  		goto out;
>  
> -	if (target_pid != -1)
> -		target_tid = target_pid;
> +	if (maps.target_pid != -1)
> +		maps.target_tid = maps.target_pid;
>  
> -	evsel_list->threads = thread_map__new(target_pid, target_tid, UINT_MAX);
> +	evsel_list->threads = thread_map__new(maps.target_pid, maps.target_tid,
> +					      UINT_MAX);
>  	if (evsel_list->threads == NULL) {
>  		pr_err("Problems finding threads of monitor\n");
>  		usage_with_options(stat_usage, options);
>  	}
>  
> -	if (system_wide)
> -		evsel_list->cpus = cpu_map__new(cpu_list);
> +	if (maps.system_wide)
> +		evsel_list->cpus = cpu_map__new(maps.cpu_list);
>  	else
>  		evsel_list->cpus = cpu_map__dummy_new();
>  
> -- 
> 1.7.9

[PATCH 03/11] perf top: Convert to perf_maps_opts

[Namhyung Kim]

Use struct perf_maps_opts as it is introduces by previous patch.
This is a preparation of further changes.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/builtin-top.c |   40 ++++++++++++++++++++++------------------
 tools/perf/util/top.c    |   15 ++++++++-------
 tools/perf/util/top.h    |    4 +---
 3 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index d869b214ada2..36917458ea53 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -555,7 +555,7 @@ static void *display_thread_tui(void *arg)
 	 * via --uid.
 	 */
 	list_for_each_entry(pos, &top->evlist->entries, node)
-		pos->hists.uid_filter_str = top->uid_str;
+		pos->hists.uid_filter_str = top->maps.uid_str;
 
 	perf_evlist__tui_browse_hists(top->evlist, help,
 				      perf_top__sort_new_samples,
@@ -965,7 +965,7 @@ static int __cmd_top(struct perf_top *top)
 	if (ret)
 		goto out_delete;
 
-	if (top->target_tid != -1 || top->uid != UINT_MAX)
+	if (top->maps.target_tid != -1 || top->maps.uid != UINT_MAX)
 		perf_event__synthesize_thread_map(&top->tool, top->evlist->threads,
 						  perf_event__process,
 						  &top->session->host_machine);
@@ -1101,11 +1101,13 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
 	struct perf_evsel *pos;
 	int status = -ENOMEM;
 	struct perf_top top = {
+		.maps = {
+			.target_pid  = -1,
+			.target_tid  = -1,
+			.uid	     = UINT_MAX,
+		},
 		.count_filter	     = 5,
 		.delay_secs	     = 2,
-		.target_pid	     = -1,
-		.target_tid	     = -1,
-		.uid		     = UINT_MAX,
 		.freq		     = 1000, /* 1 KHz */
 		.sample_id_all_avail = true,
 		.mmap_pages	     = 128,
@@ -1118,13 +1120,13 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
 		     parse_events_option),
 	OPT_INTEGER('c', "count", &top.default_interval,
 		    "event period to sample"),
-	OPT_INTEGER('p', "pid", &top.target_pid,
+	OPT_INTEGER('p', "pid", &top.maps.target_pid,
 		    "profile events on existing process id"),
-	OPT_INTEGER('t', "tid", &top.target_tid,
+	OPT_INTEGER('t', "tid", &top.maps.target_tid,
 		    "profile events on existing thread id"),
-	OPT_BOOLEAN('a', "all-cpus", &top.system_wide,
+	OPT_BOOLEAN('a', "all-cpus", &top.maps.system_wide,
 			    "system-wide collection from all CPUs"),
-	OPT_STRING('C', "cpu", &top.cpu_list, "cpu",
+	OPT_STRING('C', "cpu", &top.maps.cpu_list, "cpu",
 		    "list of cpus to monitor"),
 	OPT_STRING('k', "vmlinux", &symbol_conf.vmlinux_name,
 		   "file", "vmlinux pathname"),
@@ -1179,7 +1181,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
 		    "Display raw encoding of assembly instructions (default)"),
 	OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
 		   "Specify disassembler style (e.g. -M intel for intel syntax)"),
-	OPT_STRING('u', "uid", &top.uid_str, "user", "user to profile"),
+	OPT_STRING('u', "uid", &top.maps.uid_str, "user", "user to profile"),
 	OPT_END()
 	};
 
@@ -1205,22 +1207,24 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
 
 	setup_browser(false);
 
-	top.uid = parse_target_uid(top.uid_str, top.target_tid, top.target_pid);
-	if (top.uid_str != NULL && top.uid == UINT_MAX - 1)
+	top.maps.uid = parse_target_uid(top.maps.uid_str, top.maps.target_tid,
+					top.maps.target_pid);
+	if (top.maps.uid_str != NULL && top.maps.uid == UINT_MAX - 1)
 		goto out_delete_evlist;
 
 	/* CPU and PID are mutually exclusive */
-	if (top.target_tid > 0 && top.cpu_list) {
+	if (top.maps.target_tid > 0 && top.maps.cpu_list) {
 		printf("WARNING: PID switch overriding CPU\n");
 		sleep(1);
-		top.cpu_list = NULL;
+		top.maps.cpu_list = NULL;
 	}
 
-	if (top.target_pid != -1)
-		top.target_tid = top.target_pid;
+	if (top.maps.target_pid != -1)
+		top.maps.target_tid = top.maps.target_pid;
 
-	if (perf_evlist__create_maps(top.evlist, top.target_pid,
-				     top.target_tid, top.uid, top.cpu_list) < 0)
+	if (perf_evlist__create_maps(top.evlist, top.maps.target_pid,
+				     top.maps.target_tid, top.maps.uid,
+				     top.maps.cpu_list) < 0)
 		usage_with_options(top_usage, options);
 
 	if (!top.evlist->nr_entries &&
diff --git a/tools/perf/util/top.c b/tools/perf/util/top.c
index e4370ca27193..be50e655ecab 100644
--- a/tools/perf/util/top.c
+++ b/tools/perf/util/top.c
@@ -69,23 +69,24 @@ size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
 
 	ret += SNPRINTF(bf + ret, size - ret, "], ");
 
-	if (top->target_pid != -1)
+	if (top->maps.target_pid != -1)
 		ret += SNPRINTF(bf + ret, size - ret, " (target_pid: %d",
-				top->target_pid);
-	else if (top->target_tid != -1)
+				top->maps.target_pid);
+	else if (top->maps.target_tid != -1)
 		ret += SNPRINTF(bf + ret, size - ret, " (target_tid: %d",
-				top->target_tid);
+				top->maps.target_tid);
 	else if (top->uid_str != NULL)
 		ret += SNPRINTF(bf + ret, size - ret, " (uid: %s",
 				top->uid_str);
 	else
 		ret += SNPRINTF(bf + ret, size - ret, " (all");
 
-	if (top->cpu_list)
+	if (top->maps.cpu_list)
 		ret += SNPRINTF(bf + ret, size - ret, ", CPU%s: %s)",
-				top->evlist->cpus->nr > 1 ? "s" : "", top->cpu_list);
+				top->evlist->cpus->nr > 1 ? "s" : "",
+				top->maps.cpu_list);
 	else {
-		if (top->target_tid != -1)
+		if (top->maps.target_tid != -1)
 			ret += SNPRINTF(bf + ret, size - ret, ")");
 		else
 			ret += SNPRINTF(bf + ret, size - ret, ", %d CPU%s)",
diff --git a/tools/perf/util/top.h b/tools/perf/util/top.h
index def3e53e0fe0..77a767efa9c3 100644
--- a/tools/perf/util/top.h
+++ b/tools/perf/util/top.h
@@ -13,6 +13,7 @@ struct perf_session;
 struct perf_top {
 	struct perf_tool   tool;
 	struct perf_evlist *evlist;
+	struct perf_maps_opts maps;
 	/*
 	 * Symbols will be added here in perf_event__process_sample and will
 	 * get out after decayed.
@@ -23,10 +24,8 @@ struct perf_top {
 	u64		   guest_us_samples, guest_kernel_samples;
 	int		   print_entries, count_filter, delay_secs;
 	int		   freq;
-	pid_t		   target_pid, target_tid;
 	uid_t		   uid;
 	bool		   hide_kernel_symbols, hide_user_symbols, zero;
-	bool		   system_wide;
 	bool		   use_tui, use_stdio;
 	bool		   sort_has_symbols;
 	bool		   dont_use_callchains;
@@ -36,7 +35,6 @@ struct perf_top {
 	bool		   group;
 	bool		   sample_id_all_avail;
 	bool		   dump_symtab;
-	const char	   *cpu_list;
 	struct hist_entry  *sym_filter_entry;
 	struct perf_evsel  *sym_evsel;
 	struct perf_session *session;
-- 
1.7.9

[PATCH 04/11] perf tools: Introduce check_target_maps() helper

[Namhyung Kim]

The check_target_maps function is used to check given PID/TID/UID/CPU
target options and warn if some combination is impossible. Also this
can make some arguments of parse_target_uid() function useless as it
is checked before the call via our new helper.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/builtin-record.c |    9 +++------
 tools/perf/builtin-stat.c   |    3 +--
 tools/perf/builtin-top.c    |   15 +++------------
 tools/perf/util/usage.c     |   30 ++++++++++++++++++++++--------
 tools/perf/util/util.h      |    4 +++-
 5 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 7b50316136bf..9e209e3279e4 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -783,15 +783,12 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 		goto out_symbol_exit;
 	}
 
-	rec->opts.maps.uid = parse_target_uid(rec->opts.maps.uid_str,
-					      rec->opts.maps.target_tid,
-					      rec->opts.maps.target_pid);
+	check_target_maps(&rec->opts.maps);
+
+	rec->opts.maps.uid = parse_target_uid(rec->opts.maps.uid_str);
 	if (rec->opts.maps.uid_str && rec->opts.maps.uid == UINT_MAX - 1)
 		goto out_free_fd;
 
-	if (rec->opts.maps.target_pid != -1)
-		rec->opts.maps.target_tid = rec->opts.maps.target_pid;
-
 	if (perf_evlist__create_maps(evsel_list, rec->opts.maps.target_pid,
 				     rec->opts.maps.target_tid, rec->opts.maps.uid,
 				     rec->opts.maps.cpu_list) < 0)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 14a30c2b9fe0..f1f4e8f60a3e 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1207,8 +1207,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
 	if (add_default_attributes())
 		goto out;
 
-	if (maps.target_pid != -1)
-		maps.target_tid = maps.target_pid;
+	check_target_maps(&maps);
 
 	evsel_list->threads = thread_map__new(maps.target_pid, maps.target_tid,
 					      UINT_MAX);
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 36917458ea53..9e9a8de78899 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1207,21 +1207,12 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
 
 	setup_browser(false);
 
-	top.maps.uid = parse_target_uid(top.maps.uid_str, top.maps.target_tid,
-					top.maps.target_pid);
+	check_target_maps(&top.maps);
+
+	top.maps.uid = parse_target_uid(top.maps.uid_str);
 	if (top.maps.uid_str != NULL && top.maps.uid == UINT_MAX - 1)
 		goto out_delete_evlist;
 
-	/* CPU and PID are mutually exclusive */
-	if (top.maps.target_tid > 0 && top.maps.cpu_list) {
-		printf("WARNING: PID switch overriding CPU\n");
-		sleep(1);
-		top.maps.cpu_list = NULL;
-	}
-
-	if (top.maps.target_pid != -1)
-		top.maps.target_tid = top.maps.target_pid;
-
 	if (perf_evlist__create_maps(top.evlist, top.maps.target_pid,
 				     top.maps.target_tid, top.maps.uid,
 				     top.maps.cpu_list) < 0)
diff --git a/tools/perf/util/usage.c b/tools/perf/util/usage.c
index d0c013934f30..d235013600e5 100644
--- a/tools/perf/util/usage.c
+++ b/tools/perf/util/usage.c
@@ -83,7 +83,7 @@ void warning(const char *warn, ...)
 	va_end(params);
 }
 
-uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
+uid_t parse_target_uid(const char *str)
 {
 	struct passwd pwd, *result;
 	char buf[1024];
@@ -91,13 +91,6 @@ uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
 	if (str == NULL)
 		return UINT_MAX;
 
-	/* CPU and PID are mutually exclusive */
-	if (tid > 0 || pid > 0) {
-		ui__warning("PID/TID switch overriding UID\n");
-		sleep(1);
-		return UINT_MAX;
-	}
-
 	getpwnam_r(str, &pwd, buf, sizeof(buf), &result);
 
 	if (result == NULL) {
@@ -120,3 +113,24 @@ uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
 
 	return result->pw_uid;
 }
+
+void check_target_maps(struct perf_maps_opts *maps)
+{
+	if (maps->target_pid != -1)
+		maps->target_tid = maps->target_pid;
+
+	/* CPU and PID are mutually exclusive */
+	if (maps->target_tid > 0 && maps->cpu_list) {
+		ui__warning("PID/TID switch overriding CPU\n");
+		sleep(1);
+		maps->cpu_list = NULL;
+	}
+
+	/* UID and PID are mutually exclusive */
+	if (maps->target_tid > 0 && maps->uid_str) {
+		ui__warning("PID/TID switch overriding UID\n");
+		sleep(1);
+		maps->uid_str = NULL;
+	}
+
+}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 232d17ef3e60..2573985f8408 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -242,10 +242,12 @@ unsigned long convert_unit(unsigned long value, char *unit);
 int readn(int fd, void *buf, size_t size);
 
 struct perf_event_attr;
+struct perf_maps_opts;
 
 void event_attr_init(struct perf_event_attr *attr);
 
-uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid);
+uid_t parse_target_uid(const char *str);
+void check_target_maps(struct perf_maps_opts *maps);
 
 #define _STR(x) #x
 #define STR(x) _STR(x)
-- 
1.7.9

Re: [PATCH 04/11] perf tools: Introduce check_target_maps() helper

[Arnaldo Carvalho de Melo]

Em Mon, Feb 13, 2012 at 04:27:36PM +0900, Namhyung Kim escreveu:
> The check_target_maps function is used to check given PID/TID/UID/CPU
> target options and warn if some combination is impossible. Also this
> can make some arguments of parse_target_uid() function useless as it
> is checked before the call via our new helper.

Here I think the naming should be:

  perf_target__validate(&rec->opts.target);

instead of check_target_maps(...)

I.e. prefix with the struct name followed by two _, that is the practice
in tools/perf/, at least in code I wrote :-)

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
> ---
>  tools/perf/builtin-record.c |    9 +++------
>  tools/perf/builtin-stat.c   |    3 +--
>  tools/perf/builtin-top.c    |   15 +++------------
>  tools/perf/util/usage.c     |   30 ++++++++++++++++++++++--------
>  tools/perf/util/util.h      |    4 +++-
>  5 files changed, 32 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 7b50316136bf..9e209e3279e4 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -783,15 +783,12 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
>  		goto out_symbol_exit;
>  	}
>  
> -	rec->opts.maps.uid = parse_target_uid(rec->opts.maps.uid_str,
> -					      rec->opts.maps.target_tid,
> -					      rec->opts.maps.target_pid);
> +	check_target_maps(&rec->opts.maps);
> +
> +	rec->opts.maps.uid = parse_target_uid(rec->opts.maps.uid_str);
>  	if (rec->opts.maps.uid_str && rec->opts.maps.uid == UINT_MAX - 1)
>  		goto out_free_fd;
>  
> -	if (rec->opts.maps.target_pid != -1)
> -		rec->opts.maps.target_tid = rec->opts.maps.target_pid;
> -
>  	if (perf_evlist__create_maps(evsel_list, rec->opts.maps.target_pid,
>  				     rec->opts.maps.target_tid, rec->opts.maps.uid,
>  				     rec->opts.maps.cpu_list) < 0)
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 14a30c2b9fe0..f1f4e8f60a3e 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1207,8 +1207,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
>  	if (add_default_attributes())
>  		goto out;
>  
> -	if (maps.target_pid != -1)
> -		maps.target_tid = maps.target_pid;
> +	check_target_maps(&maps);
>  
>  	evsel_list->threads = thread_map__new(maps.target_pid, maps.target_tid,
>  					      UINT_MAX);
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 36917458ea53..9e9a8de78899 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1207,21 +1207,12 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
>  
>  	setup_browser(false);
>  
> -	top.maps.uid = parse_target_uid(top.maps.uid_str, top.maps.target_tid,
> -					top.maps.target_pid);
> +	check_target_maps(&top.maps);
> +
> +	top.maps.uid = parse_target_uid(top.maps.uid_str);
>  	if (top.maps.uid_str != NULL && top.maps.uid == UINT_MAX - 1)
>  		goto out_delete_evlist;
>  
> -	/* CPU and PID are mutually exclusive */
> -	if (top.maps.target_tid > 0 && top.maps.cpu_list) {
> -		printf("WARNING: PID switch overriding CPU\n");
> -		sleep(1);
> -		top.maps.cpu_list = NULL;
> -	}
> -
> -	if (top.maps.target_pid != -1)
> -		top.maps.target_tid = top.maps.target_pid;
> -
>  	if (perf_evlist__create_maps(top.evlist, top.maps.target_pid,
>  				     top.maps.target_tid, top.maps.uid,
>  				     top.maps.cpu_list) < 0)
> diff --git a/tools/perf/util/usage.c b/tools/perf/util/usage.c
> index d0c013934f30..d235013600e5 100644
> --- a/tools/perf/util/usage.c
> +++ b/tools/perf/util/usage.c
> @@ -83,7 +83,7 @@ void warning(const char *warn, ...)
>  	va_end(params);
>  }
>  
> -uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
> +uid_t parse_target_uid(const char *str)
>  {
>  	struct passwd pwd, *result;
>  	char buf[1024];
> @@ -91,13 +91,6 @@ uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
>  	if (str == NULL)
>  		return UINT_MAX;
>  
> -	/* CPU and PID are mutually exclusive */
> -	if (tid > 0 || pid > 0) {
> -		ui__warning("PID/TID switch overriding UID\n");
> -		sleep(1);
> -		return UINT_MAX;
> -	}
> -
>  	getpwnam_r(str, &pwd, buf, sizeof(buf), &result);
>  
>  	if (result == NULL) {
> @@ -120,3 +113,24 @@ uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
>  
>  	return result->pw_uid;
>  }
> +
> +void check_target_maps(struct perf_maps_opts *maps)
> +{
> +	if (maps->target_pid != -1)
> +		maps->target_tid = maps->target_pid;
> +
> +	/* CPU and PID are mutually exclusive */
> +	if (maps->target_tid > 0 && maps->cpu_list) {
> +		ui__warning("PID/TID switch overriding CPU\n");
> +		sleep(1);
> +		maps->cpu_list = NULL;
> +	}
> +
> +	/* UID and PID are mutually exclusive */
> +	if (maps->target_tid > 0 && maps->uid_str) {
> +		ui__warning("PID/TID switch overriding UID\n");
> +		sleep(1);
> +		maps->uid_str = NULL;
> +	}
> +
> +}
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 232d17ef3e60..2573985f8408 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -242,10 +242,12 @@ unsigned long convert_unit(unsigned long value, char *unit);
>  int readn(int fd, void *buf, size_t size);
>  
>  struct perf_event_attr;
> +struct perf_maps_opts;
>  
>  void event_attr_init(struct perf_event_attr *attr);
>  
> -uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid);
> +uid_t parse_target_uid(const char *str);
> +void check_target_maps(struct perf_maps_opts *maps);
>  
>  #define _STR(x) #x
>  #define STR(x) _STR(x)
> -- 
> 1.7.9

[PATCH 05/11] perf tools: Make perf_evlist__create_maps() take struct perf_maps_opts

[Namhyung Kim]

Now we have all information that needed to create cpu/thread maps
in struct perf_maps_opts, it'd be better using it as an argument.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/builtin-record.c |    4 +---
 tools/perf/builtin-test.c   |    5 ++---
 tools/perf/builtin-top.c    |    4 +---
 tools/perf/util/evlist.c    |   12 +++++++-----
 tools/perf/util/evlist.h    |    4 ++--
 5 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 9e209e3279e4..a8abee03a62d 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -789,9 +789,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 	if (rec->opts.maps.uid_str && rec->opts.maps.uid == UINT_MAX - 1)
 		goto out_free_fd;
 
-	if (perf_evlist__create_maps(evsel_list, rec->opts.maps.target_pid,
-				     rec->opts.maps.target_tid, rec->opts.maps.uid,
-				     rec->opts.maps.cpu_list) < 0)
+	if (perf_evlist__create_maps(evsel_list, &rec->opts.maps) < 0)
 		usage_with_options(record_usage, record_options);
 
 	list_for_each_entry(pos, &evsel_list->entries, node) {
diff --git a/tools/perf/builtin-test.c b/tools/perf/builtin-test.c
index da3c1c3f8524..91a6eba000f2 100644
--- a/tools/perf/builtin-test.c
+++ b/tools/perf/builtin-test.c
@@ -1013,6 +1013,7 @@ static int test__PERF_RECORD(void)
 		.maps = {
 			.target_pid = -1,
 			.target_tid = -1,
+			.uid	    = UINT_MAX,
 		},
 		.no_delay   = true,
 		.freq	    = 10,
@@ -1057,9 +1058,7 @@ static int test__PERF_RECORD(void)
 	 * perf_evlist__prepare_workload we'll fill in the only thread
 	 * we're monitoring, the one forked there.
 	 */
-	err = perf_evlist__create_maps(evlist, opts.maps.target_pid,
-				       opts.maps.target_tid, UINT_MAX,
-				       opts.maps.cpu_list);
+	err = perf_evlist__create_maps(evlist, &opts.maps);
 	if (err < 0) {
 		pr_debug("Not enough memory to create thread/cpu maps\n");
 		goto out_delete_evlist;
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 9e9a8de78899..5bba7ad92fc6 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1213,9 +1213,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
 	if (top.maps.uid_str != NULL && top.maps.uid == UINT_MAX - 1)
 		goto out_delete_evlist;
 
-	if (perf_evlist__create_maps(top.evlist, top.maps.target_pid,
-				     top.maps.target_tid, top.maps.uid,
-				     top.maps.cpu_list) < 0)
+	if (perf_evlist__create_maps(top.evlist, &top.maps) < 0)
 		usage_with_options(top_usage, options);
 
 	if (!top.evlist->nr_entries &&
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index d7c3a2d5771c..3d763c95fd62 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -593,18 +593,20 @@ int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages,
 	return perf_evlist__mmap_per_cpu(evlist, prot, mask);
 }
 
-int perf_evlist__create_maps(struct perf_evlist *evlist, pid_t target_pid,
-			     pid_t target_tid, uid_t uid, const char *cpu_list)
+int perf_evlist__create_maps(struct perf_evlist *evlist,
+			     struct perf_maps_opts *maps)
 {
-	evlist->threads = thread_map__new(target_pid, target_tid, uid);
+	evlist->threads = thread_map__new(maps->target_pid, maps->target_tid,
+					  maps->uid);
 
 	if (evlist->threads == NULL)
 		return -1;
 
-	if (uid != UINT_MAX || (cpu_list == NULL && target_tid != -1))
+	if (maps->uid != UINT_MAX ||
+	    (maps->cpu_list == NULL && maps->target_tid != -1))
 		evlist->cpus = cpu_map__dummy_new();
 	else
-		evlist->cpus = cpu_map__new(cpu_list);
+		evlist->cpus = cpu_map__new(maps->cpu_list);
 
 	if (evlist->cpus == NULL)
 		goto out_delete_threads;
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 1b4282be8fe7..7e99ac513893 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -106,8 +106,8 @@ static inline void perf_evlist__set_maps(struct perf_evlist *evlist,
 	evlist->threads	= threads;
 }
 
-int perf_evlist__create_maps(struct perf_evlist *evlist, pid_t target_pid,
-			     pid_t tid, uid_t uid, const char *cpu_list);
+int perf_evlist__create_maps(struct perf_evlist *evlist,
+			     struct perf_maps_opts *maps);
 void perf_evlist__delete_maps(struct perf_evlist *evlist);
 int perf_evlist__set_filters(struct perf_evlist *evlist);
 
-- 
1.7.9

Re: [PATCH 05/11] perf tools: Make perf_evlist__create_maps() take struct perf_maps_opts

[Arnaldo Carvalho de Melo]

Em Mon, Feb 13, 2012 at 04:27:37PM +0900, Namhyung Kim escreveu:
> Now we have all information that needed to create cpu/thread maps
> in struct perf_maps_opts, it'd be better using it as an argument.

Ok
 

[PATCH 06/11] perf tools: Check more combinations of PID/TID, UID and CPU switches

[Namhyung Kim]

There were some combinations of these switches that are not so
appropriate IMHO. Since there are implicit priorities between
them, they worked well anyway, but end up opening useless
duplicated events. For example, 'perf stat -t <pid> -a' will
open multiple events for the thread instead of one.

Add explicit checks and warn user in check_target_maps().

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/util/usage.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/usage.c b/tools/perf/util/usage.c
index d235013600e5..08423c037a7e 100644
--- a/tools/perf/util/usage.c
+++ b/tools/perf/util/usage.c
@@ -133,4 +133,17 @@ void check_target_maps(struct perf_maps_opts *maps)
 		maps->uid_str = NULL;
 	}
 
+	/* UID and CPU are mutually exclusive */
+	if (maps->uid_str && maps->cpu_list) {
+		ui__warning("UID switch overriding CPU\n");
+		sleep(1);
+		maps->cpu_list = NULL;
+	}
+
+	/* PID/UID and SYSTEM are mutually exclusive */
+	if ((maps->target_tid > 0 || maps->uid_str) && maps->system_wide) {
+		ui__warning("PID/TID/UID switch overriding CPU\n");
+		sleep(1);
+		maps->system_wide = false;
+	}
 }
-- 
1.7.9

[PATCH 07/11] perf tools: Fix creation of cpu map

[Namhyung Kim]

Currently, 'perf record -- sleep 1' creates a cpu map for all online
cpus since it turns out calling cpu_map__new(NULL). Fix it. Also it
is guaranteed that cpu_list is NULL if PID/TID is given by calling
check_target_maps(), so we can make the conditional bit simpler.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/util/evlist.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 3d763c95fd62..b444f311897c 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -602,8 +602,9 @@ int perf_evlist__create_maps(struct perf_evlist *evlist,
 	if (evlist->threads == NULL)
 		return -1;
 
-	if (maps->uid != UINT_MAX ||
-	    (maps->cpu_list == NULL && maps->target_tid != -1))
+	if (maps->uid != UINT_MAX || maps->target_tid != -1)
+		evlist->cpus = cpu_map__dummy_new();
+	else if (!maps->system_wide && maps->cpu_list == NULL)
 		evlist->cpus = cpu_map__dummy_new();
 	else
 		evlist->cpus = cpu_map__new(maps->cpu_list);
-- 
1.7.9

[PATCH 08/11] perf tools: Consolidate target task/cpu checking

[Namhyung Kim]

There are places that check whether target task/cpu is given or not
and some of them didn't check newly introduced uid or cpu list. Add
and use three of helper functions to treat them properly.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/builtin-record.c |    5 +----
 tools/perf/builtin-stat.c   |   11 +++++------
 tools/perf/util/evlist.c    |    7 +++----
 tools/perf/util/evsel.c     |    3 +--
 tools/perf/util/usage.c     |   16 ++++++++++++++++
 tools/perf/util/util.h      |    3 +++
 6 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index a8abee03a62d..fbf71edd31ba 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -740,10 +740,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 
 	argc = parse_options(argc, argv, record_options, record_usage,
 			    PARSE_OPT_STOP_AT_NON_OPTION);
-	if (!argc && rec->opts.maps.target_pid == -1 &&
-	    rec->opts.maps.target_tid == -1 &&
-	    !rec->opts.maps.system_wide && !rec->opts.maps.cpu_list &&
-	    !rec->opts.maps.uid_str)
+	if (!argc && no_target_maps(&rec->opts.maps))
 		usage_with_options(record_usage, record_options);
 
 	if (rec->force && rec->append_file) {
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index f1f4e8f60a3e..d080254a2d0c 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -296,7 +296,7 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
 	if (maps.system_wide)
 		return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
 						group, group_fd);
-	if (maps.target_pid == -1 && maps.target_tid == -1) {
+	if (no_target_task(&maps)) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}
@@ -446,8 +446,7 @@ static int run_perf_stat(int argc __used, const char **argv)
 			exit(-1);
 		}
 
-		if (maps.target_tid == -1 && maps.target_pid == -1 &&
-		    !maps.system_wide)
+		if (no_target_maps(&maps))
 			evsel_list->threads->map[0] = child_pid;
 
 		/*
@@ -969,7 +968,7 @@ static void print_stat(int argc, const char **argv)
 	if (!csv_output) {
 		fprintf(output, "\n");
 		fprintf(output, " Performance counter stats for ");
-		if(maps.target_pid == -1 && maps.target_tid == -1) {
+		if (no_target_task(&maps)) {
 			fprintf(output, "\'%s", argv[0]);
 			for (i = 1; i < argc; i++)
 				fprintf(output, " %s", argv[i]);
@@ -1191,13 +1190,13 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
 	} else if (big_num_opt == 0) /* User passed --no-big-num */
 		big_num = false;
 
-	if (!argc && maps.target_pid == -1 && maps.target_tid == -1)
+	if (!argc && no_target_task(&maps))
 		usage_with_options(stat_usage, options);
 	if (run_count <= 0)
 		usage_with_options(stat_usage, options);
 
 	/* no_aggr, cgroup are for system-wide only */
-	if ((no_aggr || nr_cgroups) && !maps.system_wide) {
+	if ((no_aggr || nr_cgroups) && !no_target_cpu(&maps)) {
 		fprintf(stderr, "both cgroup and no-aggregation "
 			"modes only available in system-wide mode\n");
 
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index b444f311897c..aa0418034d82 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -602,9 +602,9 @@ int perf_evlist__create_maps(struct perf_evlist *evlist,
 	if (evlist->threads == NULL)
 		return -1;
 
-	if (maps->uid != UINT_MAX || maps->target_tid != -1)
+	if (!no_target_task(maps))
 		evlist->cpus = cpu_map__dummy_new();
-	else if (!maps->system_wide && maps->cpu_list == NULL)
+	else if (no_target_cpu(maps))
 		evlist->cpus = cpu_map__dummy_new();
 	else
 		evlist->cpus = cpu_map__new(maps->cpu_list);
@@ -823,8 +823,7 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist,
 		exit(-1);
 	}
 
-	if (!opts->maps.system_wide && opts->maps.target_tid == -1 &&
-	    opts->maps.target_pid == -1)
+	if (no_target_maps(&opts->maps))
 		evlist->threads->map[0] = evlist->workload.pid;
 
 	close(child_ready_pipe[1]);
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 6a4e97d5f6d5..e7ddf27f240b 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -130,8 +130,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts)
 	attr->mmap = track;
 	attr->comm = track;
 
-	if (opts->maps.target_pid == -1 && opts->maps.target_tid == -1 &&
-	    !opts->maps.system_wide) {
+	if (no_target_maps(&opts->maps)) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}
diff --git a/tools/perf/util/usage.c b/tools/perf/util/usage.c
index 08423c037a7e..d7e77fde966d 100644
--- a/tools/perf/util/usage.c
+++ b/tools/perf/util/usage.c
@@ -147,3 +147,19 @@ void check_target_maps(struct perf_maps_opts *maps)
 		maps->system_wide = false;
 	}
 }
+
+bool no_target_task(struct perf_maps_opts *maps)
+{
+	return maps->target_pid == -1 && maps->target_tid == -1 &&
+		maps->uid_str == NULL;
+}
+
+bool no_target_cpu(struct perf_maps_opts *maps)
+{
+	return !maps->system_wide && maps->cpu_list == NULL;
+}
+
+bool no_target_maps(struct perf_maps_opts *maps)
+{
+	return no_target_task(maps) && no_target_cpu(maps);
+}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 2573985f8408..e1343eea14eb 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -248,6 +248,9 @@ void event_attr_init(struct perf_event_attr *attr);
 
 uid_t parse_target_uid(const char *str);
 void check_target_maps(struct perf_maps_opts *maps);
+bool no_target_task(struct perf_maps_opts *maps);
+bool no_target_cpu(struct perf_maps_opts *maps);
+bool no_target_maps(struct perf_maps_opts *maps);
 
 #define _STR(x) #x
 #define STR(x) _STR(x)
-- 
1.7.9

Re: [PATCH 08/11] perf tools: Consolidate target task/cpu checking

[Arnaldo Carvalho de Melo]

Em Mon, Feb 13, 2012 at 04:27:40PM +0900, Namhyung Kim escreveu:
> There are places that check whether target task/cpu is given or not
> and some of them didn't check newly introduced uid or cpu list. Add
> and use three of helper functions to treat them properly.

Following the changes I suggested these:

 bool no_target_task(struct perf_maps_opts *maps);
 bool no_target_cpu(struct perf_maps_opts *maps);
 bool no_target_maps(struct perf_maps_opts *maps);

become

 bool perf_target__no_task(struct perf_target *target);
 bool perf_target__no_cpu(struct perf_target *target);
 bool perf_target__empty(struct perf_target *target);

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
> ---
>  tools/perf/builtin-record.c |    5 +----
>  tools/perf/builtin-stat.c   |   11 +++++------
>  tools/perf/util/evlist.c    |    7 +++----
>  tools/perf/util/evsel.c     |    3 +--
>  tools/perf/util/usage.c     |   16 ++++++++++++++++
>  tools/perf/util/util.h      |    3 +++
>  6 files changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index a8abee03a62d..fbf71edd31ba 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -740,10 +740,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
>  
>  	argc = parse_options(argc, argv, record_options, record_usage,
>  			    PARSE_OPT_STOP_AT_NON_OPTION);
> -	if (!argc && rec->opts.maps.target_pid == -1 &&
> -	    rec->opts.maps.target_tid == -1 &&
> -	    !rec->opts.maps.system_wide && !rec->opts.maps.cpu_list &&
> -	    !rec->opts.maps.uid_str)
> +	if (!argc && no_target_maps(&rec->opts.maps))
>  		usage_with_options(record_usage, record_options);
>  
>  	if (rec->force && rec->append_file) {
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index f1f4e8f60a3e..d080254a2d0c 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -296,7 +296,7 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
>  	if (maps.system_wide)
>  		return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
>  						group, group_fd);
> -	if (maps.target_pid == -1 && maps.target_tid == -1) {
> +	if (no_target_task(&maps)) {
>  		attr->disabled = 1;
>  		attr->enable_on_exec = 1;
>  	}
> @@ -446,8 +446,7 @@ static int run_perf_stat(int argc __used, const char **argv)
>  			exit(-1);
>  		}
>  
> -		if (maps.target_tid == -1 && maps.target_pid == -1 &&
> -		    !maps.system_wide)
> +		if (no_target_maps(&maps))
>  			evsel_list->threads->map[0] = child_pid;
>  
>  		/*
> @@ -969,7 +968,7 @@ static void print_stat(int argc, const char **argv)
>  	if (!csv_output) {
>  		fprintf(output, "\n");
>  		fprintf(output, " Performance counter stats for ");
> -		if(maps.target_pid == -1 && maps.target_tid == -1) {
> +		if (no_target_task(&maps)) {
>  			fprintf(output, "\'%s", argv[0]);
>  			for (i = 1; i < argc; i++)
>  				fprintf(output, " %s", argv[i]);
> @@ -1191,13 +1190,13 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
>  	} else if (big_num_opt == 0) /* User passed --no-big-num */
>  		big_num = false;
>  
> -	if (!argc && maps.target_pid == -1 && maps.target_tid == -1)
> +	if (!argc && no_target_task(&maps))
>  		usage_with_options(stat_usage, options);
>  	if (run_count <= 0)
>  		usage_with_options(stat_usage, options);
>  
>  	/* no_aggr, cgroup are for system-wide only */
> -	if ((no_aggr || nr_cgroups) && !maps.system_wide) {
> +	if ((no_aggr || nr_cgroups) && !no_target_cpu(&maps)) {
>  		fprintf(stderr, "both cgroup and no-aggregation "
>  			"modes only available in system-wide mode\n");
>  
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index b444f311897c..aa0418034d82 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -602,9 +602,9 @@ int perf_evlist__create_maps(struct perf_evlist *evlist,
>  	if (evlist->threads == NULL)
>  		return -1;
>  
> -	if (maps->uid != UINT_MAX || maps->target_tid != -1)
> +	if (!no_target_task(maps))
>  		evlist->cpus = cpu_map__dummy_new();
> -	else if (!maps->system_wide && maps->cpu_list == NULL)
> +	else if (no_target_cpu(maps))
>  		evlist->cpus = cpu_map__dummy_new();
>  	else
>  		evlist->cpus = cpu_map__new(maps->cpu_list);
> @@ -823,8 +823,7 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist,
>  		exit(-1);
>  	}
>  
> -	if (!opts->maps.system_wide && opts->maps.target_tid == -1 &&
> -	    opts->maps.target_pid == -1)
> +	if (no_target_maps(&opts->maps))
>  		evlist->threads->map[0] = evlist->workload.pid;
>  
>  	close(child_ready_pipe[1]);
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 6a4e97d5f6d5..e7ddf27f240b 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -130,8 +130,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts)
>  	attr->mmap = track;
>  	attr->comm = track;
>  
> -	if (opts->maps.target_pid == -1 && opts->maps.target_tid == -1 &&
> -	    !opts->maps.system_wide) {
> +	if (no_target_maps(&opts->maps)) {
>  		attr->disabled = 1;
>  		attr->enable_on_exec = 1;
>  	}
> diff --git a/tools/perf/util/usage.c b/tools/perf/util/usage.c
> index 08423c037a7e..d7e77fde966d 100644
> --- a/tools/perf/util/usage.c
> +++ b/tools/perf/util/usage.c
> @@ -147,3 +147,19 @@ void check_target_maps(struct perf_maps_opts *maps)
>  		maps->system_wide = false;
>  	}
>  }
> +
> +bool no_target_task(struct perf_maps_opts *maps)
> +{
> +	return maps->target_pid == -1 && maps->target_tid == -1 &&
> +		maps->uid_str == NULL;
> +}
> +
> +bool no_target_cpu(struct perf_maps_opts *maps)
> +{
> +	return !maps->system_wide && maps->cpu_list == NULL;
> +}
> +
> +bool no_target_maps(struct perf_maps_opts *maps)
> +{
> +	return no_target_task(maps) && no_target_cpu(maps);
> +}
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 2573985f8408..e1343eea14eb 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -248,6 +248,9 @@ void event_attr_init(struct perf_event_attr *attr);
>  
>  uid_t parse_target_uid(const char *str);
>  void check_target_maps(struct perf_maps_opts *maps);
> +bool no_target_task(struct perf_maps_opts *maps);
> +bool no_target_cpu(struct perf_maps_opts *maps);
> +bool no_target_maps(struct perf_maps_opts *maps);
>  
>  #define _STR(x) #x
>  #define STR(x) _STR(x)
> -- 
> 1.7.9

[PATCH 09/11] perf stat: Use perf_evlist__create_maps

[Namhyung Kim]

Use same function with perf record and top to share the code
checks combinations of different switches.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/builtin-stat.c |   16 ++--------------
 1 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d080254a2d0c..be2667236bea 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -196,6 +196,7 @@ static int			output_fd;
 static struct perf_maps_opts maps = {
 	.target_pid		= -1,
 	.target_tid		= -1,
+	.uid			= UINT_MAX,
 };
 
 static volatile int done = 0;
@@ -1208,20 +1209,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
 
 	check_target_maps(&maps);
 
-	evsel_list->threads = thread_map__new(maps.target_pid, maps.target_tid,
-					      UINT_MAX);
-	if (evsel_list->threads == NULL) {
-		pr_err("Problems finding threads of monitor\n");
-		usage_with_options(stat_usage, options);
-	}
-
-	if (maps.system_wide)
-		evsel_list->cpus = cpu_map__new(maps.cpu_list);
-	else
-		evsel_list->cpus = cpu_map__dummy_new();
-
-	if (evsel_list->cpus == NULL) {
-		perror("failed to parse CPUs map");
+	if (perf_evlist__create_maps(evsel_list, &maps) < 0) {
 		usage_with_options(stat_usage, options);
 		return -1;
 	}
-- 
1.7.9

Re: [PATCH 09/11] perf stat: Use perf_evlist__create_maps

[Arnaldo Carvalho de Melo]

Em Mon, Feb 13, 2012 at 04:27:41PM +0900, Namhyung Kim escreveu:
> Use same function with perf record and top to share the code
> checks combinations of different switches.

Haven't fully checked, but if equivalent, nice cleanup!

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
> ---
>  tools/perf/builtin-stat.c |   16 ++--------------
>  1 files changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index d080254a2d0c..be2667236bea 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -196,6 +196,7 @@ static int			output_fd;
>  static struct perf_maps_opts maps = {
>  	.target_pid		= -1,
>  	.target_tid		= -1,
> +	.uid			= UINT_MAX,
>  };
>  
>  static volatile int done = 0;
> @@ -1208,20 +1209,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
>  
>  	check_target_maps(&maps);
>  
> -	evsel_list->threads = thread_map__new(maps.target_pid, maps.target_tid,
> -					      UINT_MAX);
> -	if (evsel_list->threads == NULL) {
> -		pr_err("Problems finding threads of monitor\n");
> -		usage_with_options(stat_usage, options);
> -	}
> -
> -	if (maps.system_wide)
> -		evsel_list->cpus = cpu_map__new(maps.cpu_list);
> -	else
> -		evsel_list->cpus = cpu_map__dummy_new();
> -
> -	if (evsel_list->cpus == NULL) {
> -		perror("failed to parse CPUs map");
> +	if (perf_evlist__create_maps(evsel_list, &maps) < 0) {
>  		usage_with_options(stat_usage, options);
>  		return -1;
>  	}
> -- 
> 1.7.9

[PATCH 10/11] perf stat: Fix event grouping on forked task

[Namhyung Kim]

When event group is enabled for forked task (i.e. no target task was
specified) all events were disabled and marked ->enable_on_exec.
However they are not counted at all since only group leader will be
enabled on exec actually. So the result looked like below:

 $ perf stat --group sleep 1

 Performance counter stats for 'sleep 1':

          0.530891 task-clock                #    0.001 CPUs utilized
     <not counted> context-switches
     <not counted> CPU-migrations
     <not counted> page-faults
     <not counted> cycles
   <not supported> stalled-cycles-frontend
   <not supported> stalled-cycles-backend
     <not counted> instructions
     <not counted> branches
     <not counted> branch-misses

       1.001140177 seconds time elapsed

Fix it by disabling group leader only.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/builtin-stat.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index be2667236bea..2a592e52eaee 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -297,7 +297,7 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
 	if (maps.system_wide)
 		return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
 						group, group_fd);
-	if (no_target_task(&maps)) {
+	if (no_target_task(&maps) && (!group || evsel == first)) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}
-- 
1.7.9

Re: [PATCH 10/11] perf stat: Fix event grouping on forked task

[Arnaldo Carvalho de Melo]

Em Mon, Feb 13, 2012 at 04:27:42PM +0900, Namhyung Kim escreveu:
> When event group is enabled for forked task (i.e. no target task was
> specified) all events were disabled and marked ->enable_on_exec.
> However they are not counted at all since only group leader will be
> enabled on exec actually. So the result looked like below:
> 
>  $ perf stat --group sleep 1

This one is a bugfix and would be better if we have it isolated from
this patchset, not needing the cleanups, so that we can send it to
perf/urgent and stable@
 
>  Performance counter stats for 'sleep 1':
> 
>           0.530891 task-clock                #    0.001 CPUs utilized
>      <not counted> context-switches
>      <not counted> CPU-migrations
>      <not counted> page-faults
>      <not counted> cycles
>    <not supported> stalled-cycles-frontend
>    <not supported> stalled-cycles-backend
>      <not counted> instructions
>      <not counted> branches
>      <not counted> branch-misses
> 
>        1.001140177 seconds time elapsed
> 
> Fix it by disabling group leader only.
> 
> Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
> ---
>  tools/perf/builtin-stat.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index be2667236bea..2a592e52eaee 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -297,7 +297,7 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
>  	if (maps.system_wide)
>  		return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
>  						group, group_fd);
> -	if (no_target_task(&maps)) {
> +	if (no_target_task(&maps) && (!group || evsel == first)) {
>  		attr->disabled = 1;
>  		attr->enable_on_exec = 1;
>  	}
> -- 
> 1.7.9

[PATCH] perf stat: Fix event grouping on forked task

[Namhyung Kim]

When event group is enabled for forked task (i.e. no target task was
specified) all events were disabled and marked ->enable_on_exec.
However they are not counted at all since only group leader will be
enabled on exec actually. So the result looked like below:

 $ ./perf stat --group -- sleep 1

 Performance counter stats for 'sleep 1':

          0.554926 task-clock                #    0.001 CPUs utilized
     <not counted> context-switches
     <not counted> CPU-migrations
     <not counted> page-faults
     <not counted> cycles
   <not supported> stalled-cycles-frontend
   <not supported> stalled-cycles-backend
     <not counted> instructions
     <not counted> branches
     <not counted> branch-misses

       1.001228093 seconds time elapsed

Fix it by disabling group leader only.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/builtin-stat.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d14b37ad7638..b2fa61e539c0 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -296,7 +296,8 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
 	if (system_wide)
 		return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
 						group, group_fd);
-	if (target_pid == -1 && target_tid == -1) {
+	if (target_pid == -1 && target_tid == -1 &&
+	    (!group || evsel == first)) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}
-- 
1.7.9

[PATCH 11/11] perf record: Fix event grouping on forked task

[Namhyung Kim]

When event group is enabled for forked task (i.e. no target task/cpu
was specified) all events were disabled and marked ->enable_on_exec.
However they wouldn't be counted at all since only group leader will
be enabled on exec actually.

In contrast to perf stat, perf record doesn't have a real problem
as it enables all the event before proceeding. But it needs to be
fixed anyway IMHO.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 tools/perf/builtin-record.c |    2 +-
 tools/perf/builtin-test.c   |    2 +-
 tools/perf/util/evlist.c    |    5 +++--
 tools/perf/util/evlist.h    |    3 ++-
 tools/perf/util/evsel.c     |    5 +++--
 tools/perf/util/evsel.h     |    3 ++-
 6 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index fbf71edd31ba..a65b0a5a81ea 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -183,7 +183,7 @@ static void perf_record__open(struct perf_record *rec)
 
 	first = list_entry(evlist->entries.next, struct perf_evsel, node);
 
-	perf_evlist__config_attrs(evlist, opts);
+	perf_evlist__config_attrs(evlist, opts, first);
 
 	list_for_each_entry(pos, &evlist->entries, node) {
 		struct perf_event_attr *attr = &pos->attr;
diff --git a/tools/perf/builtin-test.c b/tools/perf/builtin-test.c
index 91a6eba000f2..b72b12aedd29 100644
--- a/tools/perf/builtin-test.c
+++ b/tools/perf/builtin-test.c
@@ -1083,7 +1083,7 @@ static int test__PERF_RECORD(void)
 	evsel->attr.sample_type |= PERF_SAMPLE_CPU;
 	evsel->attr.sample_type |= PERF_SAMPLE_TID;
 	evsel->attr.sample_type |= PERF_SAMPLE_TIME;
-	perf_evlist__config_attrs(evlist, &opts);
+	perf_evlist__config_attrs(evlist, &opts, evsel);
 
 	err = sched__get_first_possible_cpu(evlist->workload.pid, &cpu_mask,
 					    &cpu_mask_size);
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index aa0418034d82..7ab81906c1aa 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -49,7 +49,8 @@ struct perf_evlist *perf_evlist__new(struct cpu_map *cpus,
 }
 
 void perf_evlist__config_attrs(struct perf_evlist *evlist,
-			       struct perf_record_opts *opts)
+			       struct perf_record_opts *opts,
+			       struct perf_evsel *first)
 {
 	struct perf_evsel *evsel;
 
@@ -57,7 +58,7 @@ void perf_evlist__config_attrs(struct perf_evlist *evlist,
 		opts->no_inherit = true;
 
 	list_for_each_entry(evsel, &evlist->entries, node) {
-		perf_evsel__config(evsel, opts);
+		perf_evsel__config(evsel, opts, first);
 
 		if (evlist->nr_entries > 1)
 			evsel->attr.sample_type |= PERF_SAMPLE_ID;
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 7e99ac513893..7ebb6419e8d0 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -81,7 +81,8 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *self, int idx);
 int perf_evlist__open(struct perf_evlist *evlist, bool group);
 
 void perf_evlist__config_attrs(struct perf_evlist *evlist,
-			       struct perf_record_opts *opts);
+			       struct perf_record_opts *opts,
+			       struct perf_evsel *first);
 
 int perf_evlist__prepare_workload(struct perf_evlist *evlist,
 				  struct perf_record_opts *opts,
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index e7ddf27f240b..cb5c18b66d06 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -63,7 +63,8 @@ struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr, int idx)
 	return evsel;
 }
 
-void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts)
+void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
+			struct perf_evsel *first)
 {
 	struct perf_event_attr *attr = &evsel->attr;
 	int track = !evsel->idx; /* only the first counter needs these */
@@ -130,7 +131,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts)
 	attr->mmap = track;
 	attr->comm = track;
 
-	if (no_target_maps(&opts->maps)) {
+	if (no_target_maps(&opts->maps) && (!opts->group || evsel == first)) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 326b8e4d5035..3158ca3d69a1 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -80,7 +80,8 @@ void perf_evsel__exit(struct perf_evsel *evsel);
 void perf_evsel__delete(struct perf_evsel *evsel);
 
 void perf_evsel__config(struct perf_evsel *evsel,
-			struct perf_record_opts *opts);
+			struct perf_record_opts *opts,
+			struct perf_evsel *first);
 
 int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads);
 int perf_evsel__alloc_id(struct perf_evsel *evsel, int ncpus, int nthreads);
-- 
1.7.9

Re: [PATCH 11/11] perf record: Fix event grouping on forked task

[Arnaldo Carvalho de Melo]

Em Mon, Feb 13, 2012 at 04:27:43PM +0900, Namhyung Kim escreveu:
> When event group is enabled for forked task (i.e. no target task/cpu
> was specified) all events were disabled and marked ->enable_on_exec.
> However they wouldn't be counted at all since only group leader will
> be enabled on exec actually.
> 
> In contrast to perf stat, perf record doesn't have a real problem
> as it enables all the event before proceeding. But it needs to be
> fixed anyway IMHO.
> 
> Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
> ---
>  tools/perf/builtin-record.c |    2 +-
>  tools/perf/builtin-test.c   |    2 +-
>  tools/perf/util/evlist.c    |    5 +++--
>  tools/perf/util/evlist.h    |    3 ++-
>  tools/perf/util/evsel.c     |    5 +++--
>  tools/perf/util/evsel.h     |    3 ++-
>  6 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index fbf71edd31ba..a65b0a5a81ea 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -183,7 +183,7 @@ static void perf_record__open(struct perf_record *rec)
>  
>  	first = list_entry(evlist->entries.next, struct perf_evsel, node);
>  
> -	perf_evlist__config_attrs(evlist, opts);
> +	perf_evlist__config_attrs(evlist, opts, first);

No need to pass the first entry in the evlist, since we can find it at
perf_evlist__config_attrs using:

  list_entry(evlist->entries.next, struct perf_evsel, node);

That way you reduce the patch size by not touching all the callers of
perf_evlist__config_attrs.

- Arnaldo
  
>  	list_for_each_entry(pos, &evlist->entries, node) {
>  		struct perf_event_attr *attr = &pos->attr;
> diff --git a/tools/perf/builtin-test.c b/tools/perf/builtin-test.c
> index 91a6eba000f2..b72b12aedd29 100644
> --- a/tools/perf/builtin-test.c
> +++ b/tools/perf/builtin-test.c
> @@ -1083,7 +1083,7 @@ static int test__PERF_RECORD(void)
>  	evsel->attr.sample_type |= PERF_SAMPLE_CPU;
>  	evsel->attr.sample_type |= PERF_SAMPLE_TID;
>  	evsel->attr.sample_type |= PERF_SAMPLE_TIME;
> -	perf_evlist__config_attrs(evlist, &opts);
> +	perf_evlist__config_attrs(evlist, &opts, evsel);
>  
>  	err = sched__get_first_possible_cpu(evlist->workload.pid, &cpu_mask,
>  					    &cpu_mask_size);
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index aa0418034d82..7ab81906c1aa 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -49,7 +49,8 @@ struct perf_evlist *perf_evlist__new(struct cpu_map *cpus,
>  }
>  
>  void perf_evlist__config_attrs(struct perf_evlist *evlist,
> -			       struct perf_record_opts *opts)
> +			       struct perf_record_opts *opts,
> +			       struct perf_evsel *first)
>  {
>  	struct perf_evsel *evsel;
>  
> @@ -57,7 +58,7 @@ void perf_evlist__config_attrs(struct perf_evlist *evlist,
>  		opts->no_inherit = true;
>  
>  	list_for_each_entry(evsel, &evlist->entries, node) {
> -		perf_evsel__config(evsel, opts);
> +		perf_evsel__config(evsel, opts, first);
>  
>  		if (evlist->nr_entries > 1)
>  			evsel->attr.sample_type |= PERF_SAMPLE_ID;
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 7e99ac513893..7ebb6419e8d0 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -81,7 +81,8 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *self, int idx);
>  int perf_evlist__open(struct perf_evlist *evlist, bool group);
>  
>  void perf_evlist__config_attrs(struct perf_evlist *evlist,
> -			       struct perf_record_opts *opts);
> +			       struct perf_record_opts *opts,
> +			       struct perf_evsel *first);
>  
>  int perf_evlist__prepare_workload(struct perf_evlist *evlist,
>  				  struct perf_record_opts *opts,
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index e7ddf27f240b..cb5c18b66d06 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -63,7 +63,8 @@ struct perf_evsel *perf_evsel__new(struct perf_event_attr *attr, int idx)
>  	return evsel;
>  }
>  
> -void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts)
> +void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
> +			struct perf_evsel *first)
>  {
>  	struct perf_event_attr *attr = &evsel->attr;
>  	int track = !evsel->idx; /* only the first counter needs these */
> @@ -130,7 +131,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts)
>  	attr->mmap = track;
>  	attr->comm = track;
>  
> -	if (no_target_maps(&opts->maps)) {
> +	if (no_target_maps(&opts->maps) && (!opts->group || evsel == first)) {
>  		attr->disabled = 1;
>  		attr->enable_on_exec = 1;
>  	}
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 326b8e4d5035..3158ca3d69a1 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -80,7 +80,8 @@ void perf_evsel__exit(struct perf_evsel *evsel);
>  void perf_evsel__delete(struct perf_evsel *evsel);
>  
>  void perf_evsel__config(struct perf_evsel *evsel,
> -			struct perf_record_opts *opts);
> +			struct perf_record_opts *opts,
> +			struct perf_evsel *first);
>  
>  int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads);
>  int perf_evsel__alloc_id(struct perf_evsel *evsel, int ncpus, int nthreads);
> -- 
> 1.7.9