From: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
To: Namhyung Kim <namhyung.kim@lge.com>
Cc: Namhyung Kim <namhyung@gmail.com>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Paul Mackerras <paulus@samba.org>, Ingo Molnar <mingo@elte.hu>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/11] perf tools: Introduce struct perf_maps_opts
Date: Mon, 13 Feb 2012 16:32:32 -0200 [thread overview]
Message-ID: <20120213183232.GD15955@infradead.org> (raw)
In-Reply-To: <1329118064-9412-2-git-send-email-namhyung.kim@lge.com>
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
next prev parent reply other threads:[~2012-02-13 18:32 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1329118064-9412-1-git-send-email-namhyung.kim@lge.com>
2012-02-13 7:27 ` [PATCH 01/11] perf tools: Introduce struct perf_maps_opts Namhyung Kim
2012-02-13 7:44 ` [RFC PATCHSET] perf: Fix cpu/thread map and group event handling Namhyung Kim
2012-02-13 18:32 ` Arnaldo Carvalho de Melo [this message]
2012-02-13 18:50 ` [PATCH 01/11] perf tools: Introduce struct perf_maps_opts David Ahern
2012-02-13 19:05 ` Arnaldo Carvalho de Melo
2012-02-13 19:19 ` David Ahern
2012-02-13 20:12 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 02/11] perf stat: Convert to perf_maps_opts Namhyung Kim
2012-02-13 18:33 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 03/11] perf top: " Namhyung Kim
2012-02-13 7:27 ` [PATCH 04/11] perf tools: Introduce check_target_maps() helper Namhyung Kim
2012-02-13 18:36 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 05/11] perf tools: Make perf_evlist__create_maps() take struct perf_maps_opts Namhyung Kim
2012-02-13 18:36 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 06/11] perf tools: Check more combinations of PID/TID, UID and CPU switches Namhyung Kim
2012-02-13 7:27 ` [PATCH 07/11] perf tools: Fix creation of cpu map Namhyung Kim
2012-02-13 7:27 ` [PATCH 08/11] perf tools: Consolidate target task/cpu checking Namhyung Kim
2012-02-13 18:39 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 09/11] perf stat: Use perf_evlist__create_maps Namhyung Kim
2012-02-13 18:40 ` Arnaldo Carvalho de Melo
2012-02-13 7:27 ` [PATCH 10/11] perf stat: Fix event grouping on forked task Namhyung Kim
2012-02-13 18:41 ` Arnaldo Carvalho de Melo
2012-02-14 1:20 ` [PATCH] " Namhyung Kim
2012-02-13 7:27 ` [PATCH 11/11] perf record: " Namhyung Kim
2012-02-13 18:42 ` Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120213183232.GD15955@infradead.org \
--to=acme@ghostprotocols.net \
--cc=a.p.zijlstra@chello.nl \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=namhyung.kim@lge.com \
--cc=namhyung@gmail.com \
--cc=paulus@samba.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.