[RFC PATCHSET 00/13] perf tools: Fix cpu/thread map handling v2

Hi,

This is a second iteration of my previous series [1] but group event handling
part already got mainlined separately.

The current behaviour of perf tools dealing with PID/TID, UID and CPU has
some implications and I think there're a few bugs - For example,
'perf record sleep 1' will create multiple events as many as number of online
cpus on the system. I don't think it's intended. This indeed makes perf test
fails on validation of PERF_RECORD_* event and perf_sample fields on my 6-core
(12-thread) system like this:

namhyung@sejong:perf$ ./perf test -v 7
7: Validate PERF_RECORD_* events & perf_sample fields:
--- start ---
perf_evlist__mmap: Operation not permitted
---- end ----
Validate PERF_RECORD_* events & perf_sample fields: FAILED!

It's because perf_evlist__create_maps() created 12 cpu maps when no target PID,
TID, UID and CPU list is given (it's same as 'perf record sleep 1'), and then
perf_evlist__mmap() tried to mmap 256 pages for each cpu map so it hit a mlock
limit for a process. After this patch set was applied, the problem was gone.

During the cleanup I found some combinations of PID/TID, UID and CPU are not
allowed and have some implications. They need to be fixed and warned to user
explicitly IMHO, if needed. I think we have following implicit rules:

* If -p option is given, -t option would be ignored.
* If -p or -t option is given, -u, -C and/or -a options would be ignored.
* If -u option is given (w/o -p or -t), -C and/or -a options would be ignored.

A subtle case is when -C option is given without -a option. I think it should
be treated as a system-wide mode as if -a option is given. Also if *NO* option
is given (like above examples) it should be treated as a task mode, not a
system-wide mode.

To make libperf more generic library, perf_target code use its own error code
and perf_target__strerror() as Arnaldo suggested. Although I tried to address
all of concerns he raised, I'm not 100% sure this is in the shape he wanted
to see finally. Comments on this area would be appreciated especially :).

Once it's settled down, perf_evlist__create_maps() and its related functions
can be converted to use perf_target_errno incrementally IMHO.

This series is based on latest tip/perf/core: 3dbe927b1edd ("Merge tag
'v3.4-rc4' into perf/core").

* Changes from v1
- Drop group handling patches since it's mainlined independently.
- Rename 'struct perf_maps_ops' to 'struct perf_target' as Arnaldo suggested.
- Introduce perf_target_strerror() for better error handling as Arnaldo
suggested.
- Add perf_target__parse_uid() function to replace parse_target_uid().
- Not break python/twatch.py any more :).

Any comments are welcome, thanks.
Namhyung

[1] https://lkml.org/lkml/2012/2/13/57 - sorry, it wasn't threaded properly :(


Namhyung Kim (13):
perf tools: Introduce struct perf_target
perf stat: Convert to struct perf_target
perf top: Convert to struct perf_target
perf tools: Introduce perf_target__validate() helper
perf tools: Make perf_evlist__create_maps() take struct perf_target
perf tools: Check more combinations of PID/TID, UID and CPU switches
perf evlist: Fix creation of cpu map
perf target: Split out perf_target handling code
perf target: Introduce perf_target_errno
perf target: Introduce perf_target__parse_uid()
perf tools: Introduce perf_target__strerror()
perf target: Consolidate target task/cpu checking
perf stat: Use perf_evlist__create_maps

tools/perf/Makefile | 2 +
tools/perf/builtin-record.c | 48 ++++++++-------
tools/perf/builtin-stat.c | 58 ++++++++----------
tools/perf/builtin-test.c | 6 +-
tools/perf/builtin-top.c | 46 +++++++-------
tools/perf/perf.h | 8 +--
tools/perf/util/debug.c | 1 +
tools/perf/util/evlist.c | 16 ++---
tools/perf/util/evlist.h | 4 +-
tools/perf/util/evsel.c | 9 +--
tools/perf/util/target.c | 140 +++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/target.h | 58 ++++++++++++++++++
tools/perf/util/top.c | 19 +++---
tools/perf/util/top.h | 6 +-
tools/perf/util/usage.c | 38 ------------
tools/perf/util/util.h | 3 -
16 files changed, 311 insertions(+), 151 deletions(-)
create mode 100644 tools/perf/util/target.c
create mode 100644 tools/perf/util/target.h

--
1.7.10

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

There were some combinations of these switches that are not so
appropriate IMHO. Since there are implicit priorities between
them and they worked well anyway, but it ends 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 perf_target__validate().

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/usage.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/tools/perf/util/usage.c b/tools/perf/util/usage.c
index 0a1a885a5914..228f0a558872 100644
--- a/tools/perf/util/usage.c
+++ b/tools/perf/util/usage.c
@@ -132,4 +132,18 @@ void perf_target__validate(struct perf_target *target)
sleep(1);
target->uid_str = NULL;
}
+
+ /* UID and CPU are mutually exclusive */
+ if (target->uid_str && target->cpu_list) {
+ ui__warning("UID switch overriding CPU\n");
+ sleep(1);
+ target->cpu_list = NULL;
+ }
+
+ /* PID/UID and SYSTEM are mutually exclusive */
+ if ((target->tid || target->uid_str) && target->system_wide) {
+ ui__warning("PID/TID/UID switch overriding CPU\n");
+ sleep(1);
+ target->system_wide = false;
+ }
}
--
1.7.10

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

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
perf_target__validate(), so we can make the conditional bit simpler.

This also fixes perf test 7 (Validate) failure on my 6 core machine:

$ cat /sys/devices/system/cpu/online
0-11
$ ./perf test -v 7
7: Validate PERF_RECORD_* events & perf_sample fields:
--- start ---
perf_evlist__mmap: Operation not permitted
---- end ----
Validate PERF_RECORD_* events & perf_sample fields: FAILED!

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/evlist.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index a43e2c56d1c6..556d98af1a0b 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -608,8 +608,9 @@ int perf_evlist__create_maps(struct perf_evlist *evlist,
if (evlist->threads == NULL)
return -1;

- if (target->uid != UINT_MAX ||
- (target->cpu_list == NULL && target->tid))
+ if (target->uid != UINT_MAX || target->tid)
+ evlist->cpus = cpu_map__dummy_new();
+ else if (!target->system_wide && target->cpu_list == NULL)
evlist->cpus = cpu_map__dummy_new();
else
evlist->cpus = cpu_map__new(target->cpu_list);
--
1.7.10

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

The perf_target__validate 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 <[email protected]>
---
tools/perf/builtin-record.c | 9 +++------
tools/perf/builtin-stat.c | 3 +--
tools/perf/builtin-top.c | 15 +++------------
tools/perf/util/usage.c | 29 +++++++++++++++++++++--------
tools/perf/util/util.h | 4 +++-
5 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 4dcf27057bd2..3596ccab6d3b 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -884,16 +884,13 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
goto out_symbol_exit;
}

- rec->opts.target.uid = parse_target_uid(rec->opts.target.uid_str,
- rec->opts.target.tid,
- rec->opts.target.pid);
+ perf_target__validate(&rec->opts.target);
+
+ rec->opts.target.uid = parse_target_uid(rec->opts.target.uid_str);
if (rec->opts.target.uid_str != NULL &&
rec->opts.target.uid == UINT_MAX - 1)
goto out_free_fd;

- if (rec->opts.target.pid)
- rec->opts.target.tid = rec->opts.target.pid;
-
if (perf_evlist__create_maps(evsel_list, rec->opts.target.pid,
rec->opts.target.tid, rec->opts.target.uid,
rec->opts.target.cpu_list) < 0)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 1ca767d906ef..bb7723221c0d 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1203,8 +1203,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
if (add_default_attributes())
goto out;

- if (target.pid)
- target.tid = target.pid;
+ perf_target__validate(&target);

evsel_list->threads = thread_map__new_str(target.pid,
target.tid, UINT_MAX);
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 2c1c207627b4..4f47952eddbd 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1252,21 +1252,12 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)

setup_browser(false);

- top.target.uid = parse_target_uid(top.target.uid_str, top.target.tid,
- top.target.pid);
+ perf_target__validate(&top.target);
+
+ top.target.uid = parse_target_uid(top.target.uid_str);
if (top.target.uid_str != NULL && top.target.uid == UINT_MAX - 1)
goto out_delete_evlist;

- /* CPU and PID are mutually exclusive */
- if (top.target.tid && top.target.cpu_list) {
- printf("WARNING: PID switch overriding CPU\n");
- sleep(1);
- top.target.cpu_list = NULL;
- }
-
- if (top.target.pid)
- top.target.tid = top.target.pid;
-
if (perf_evlist__create_maps(top.evlist, top.target.pid,
top.target.tid, top.target.uid,
top.target.cpu_list) < 0)
diff --git a/tools/perf/util/usage.c b/tools/perf/util/usage.c
index 52bb07c6442a..0a1a885a5914 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, const char *tid, const char *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, const char *tid, const char *pid)
if (str == NULL)
return UINT_MAX;

- /* UID and PID are mutually exclusive */
- if (tid || pid) {
- 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,23 @@ uid_t parse_target_uid(const char *str, const char *tid, const char *pid)

return result->pw_uid;
}
+
+void perf_target__validate(struct perf_target *target)
+{
+ if (target->pid)
+ target->tid = target->pid;
+
+ /* CPU and PID are mutually exclusive */
+ if (target->tid && target->cpu_list) {
+ ui__warning("WARNING: PID switch overriding CPU\n");
+ sleep(1);
+ target->cpu_list = NULL;
+ }
+
+ /* UID and PID are mutually exclusive */
+ if (target->tid && target->uid_str) {
+ ui__warning("PID/TID switch overriding UID\n");
+ sleep(1);
+ target->uid_str = NULL;
+ }
+}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 0f99f394d8e0..3f05d6264dab 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -246,10 +246,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_target;

void event_attr_init(struct perf_event_attr *attr);

-uid_t parse_target_uid(const char *str, const char *tid, const char *pid);
+uid_t parse_target_uid(const char *str);
+void perf_target__validate(struct perf_target *target);

#define _STR(x) #x
#define STR(x) _STR(x)
--
1.7.10

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

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

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/builtin-stat.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d9ff24637eeb..e720ba7b801e 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -175,7 +175,9 @@ static struct perf_event_attr very_very_detailed_attrs[] = {

static struct perf_evlist *evsel_list;

-static struct perf_target target;
+static struct perf_target target = {
+ .uid = UINT_MAX,
+};

static int run_idx = 0;
static int run_count = 1;
@@ -1205,20 +1207,12 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)

perf_target__validate(&target);

- evsel_list->threads = thread_map__new_str(target.pid,
- target.tid, UINT_MAX);
- if (evsel_list->threads == NULL) {
- pr_err("Problems finding threads of monitor\n");
- usage_with_options(stat_usage, options);
- }
-
- if (target.system_wide)
- evsel_list->cpus = cpu_map__new(target.cpu_list);
- else
- evsel_list->cpus = cpu_map__dummy_new();
+ if (perf_evlist__create_maps(evsel_list, &target) < 0) {
+ if (!perf_target__no_task(&target))
+ pr_err("Problems finding threads of monitor\n");
+ if (!perf_target__no_cpu(&target))
+ perror("failed to parse CPUs map");

- if (evsel_list->cpus == NULL) {
- perror("failed to parse CPUs map");
usage_with_options(stat_usage, options);
return -1;
}
--
1.7.10

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

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 <[email protected]>
---
tools/perf/builtin-record.c | 4 +---
tools/perf/builtin-stat.c | 12 ++++++------
tools/perf/builtin-top.c | 2 +-
tools/perf/util/evlist.c | 10 ++++------
tools/perf/util/evsel.c | 8 ++++----
tools/perf/util/target.h | 15 +++++++++++++++
6 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 8caa84034a51..9c0fa105e063 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -843,9 +843,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.target.pid && !rec->opts.target.tid &&
- !rec->opts.target.system_wide && !rec->opts.target.cpu_list &&
- !rec->opts.target.uid_str)
+ if (!argc && perf_target__none(&rec->opts.target))
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 bb7723221c0d..d9ff24637eeb 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -290,10 +290,10 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,

attr->inherit = !no_inherit;

- if (target.system_wide)
+ if (!perf_target__no_cpu(&target))
return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
group, group_fd);
- if (!target.pid && !target.tid && (!group || evsel == first)) {
+ if (perf_target__no_task(&target) && (!group || evsel == first)) {
attr->disabled = 1;
attr->enable_on_exec = 1;
}
@@ -443,7 +443,7 @@ static int run_perf_stat(int argc __used, const char **argv)
exit(-1);
}

- if (!target.tid && !target.pid && !target.system_wide)
+ if (perf_target__none(&target))
evsel_list->threads->map[0] = child_pid;

/*
@@ -965,7 +965,7 @@ static void print_stat(int argc, const char **argv)
if (!csv_output) {
fprintf(output, "\n");
fprintf(output, " Performance counter stats for ");
- if (!target.pid && !target.tid) {
+ if (perf_target__no_task(&target)) {
fprintf(output, "\'%s", argv[0]);
for (i = 1; i < argc; i++)
fprintf(output, " %s", argv);
@@ -1187,13 +1187,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 && !target.tid)
+ if (!argc && perf_target__no_task(&target))
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) && !target.system_wide) {
+ if ((no_aggr || nr_cgroups) && perf_target__no_cpu(&target)) {
fprintf(stderr, "both cgroup and no-aggregation "
"modes only available in system-wide mode\n");

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 6f584179ecc1..948eff6be59a 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1016,7 +1016,7 @@ static int __cmd_top(struct perf_top *top)
if (ret)
goto out_delete;

- if (top->target.tid || top->target.uid != UINT_MAX)
+ if (!perf_target__no_task(&top->target))
perf_event__synthesize_thread_map(&top->tool, top->evlist->threads,
perf_event__process,
&top->session->host_machine);
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 183b199b0d09..1201daf71719 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -609,12 +609,10 @@ int perf_evlist__create_maps(struct perf_evlist *evlist,
if (evlist->threads == NULL)
return -1;

- if (target->uid != UINT_MAX || target->tid)
- evlist->cpus = cpu_map__dummy_new();
- else if (!target->system_wide && target->cpu_list == NULL)
- evlist->cpus = cpu_map__dummy_new();
- else
+ if (!perf_target__no_cpu(target))
evlist->cpus = cpu_map__new(target->cpu_list);
+ else
+ evlist->cpus = cpu_map__dummy_new();

if (evlist->cpus == NULL)
goto out_delete_threads;
@@ -831,7 +829,7 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist,
exit(-1);
}

- if (!opts->target.system_wide && !opts->target.tid && !opts->target.pid)
+ if (perf_target__none(&opts->target))
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 bb785a098ced..21eaab240396 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -114,8 +114,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
attr->sample_type |= PERF_SAMPLE_PERIOD;

if (!opts->sample_id_all_missing &&
- (opts->sample_time || opts->target.system_wide ||
- !opts->no_inherit || opts->target.cpu_list))
+ (opts->sample_time || !opts->no_inherit ||
+ !perf_target__no_cpu(&opts->target)))
attr->sample_type |= PERF_SAMPLE_TIME;

if (opts->raw_samples) {
@@ -136,8 +136,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
attr->mmap = track;
attr->comm = track;

- if (!opts->target.pid && !opts->target.tid &&
- !opts->target.system_wide && (!opts->group || evsel == first)) {
+ if (perf_target__none(&opts->target) &&
+ (!opts->group || evsel == first)) {
attr->disabled = 1;
attr->enable_on_exec = 1;
}
diff --git a/tools/perf/util/target.h b/tools/perf/util/target.h
index fff28c18f401..c9d7ee587fe2 100644
--- a/tools/perf/util/target.h
+++ b/tools/perf/util/target.h
@@ -40,4 +40,19 @@ enum perf_target_errno perf_target__parse_uid(struct perf_target *target);
int perf_target__strerror(struct perf_target *target, int errnum, char *buf,
size_t buflen);

+static inline bool perf_target__no_task(struct perf_target *target)
+{
+ return !target->pid && !target->tid && !target->uid_str;
+}
+
+static inline bool perf_target__no_cpu(struct perf_target *target)
+{
+ return !target->system_wide && !target->cpu_list;
+}
+
+static inline bool perf_target__none(struct perf_target *target)
+{
+ return perf_target__no_task(target) && perf_target__no_cpu(target);
+}
+
#endif /* _PERF_TARGET_H */
--
1.7.10

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

The perf_target__strerror() sets @buf to a string that
describes the (perf_target-specific) error condition
that is passed via @errnum.

This is similar to strerror_r() and does same thing if
@errnum has a standard errno value.

Suggested-by: Arnaldo Carvalho de Melo <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/builtin-record.c | 18 ++++++++++++++--
tools/perf/builtin-top.c | 19 ++++++++++++++---
tools/perf/util/debug.c | 1 +
tools/perf/util/target.c | 49 +++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/target.h | 3 +++
5 files changed, 85 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index e5d77452eef6..8caa84034a51 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -831,6 +831,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
struct perf_evsel *pos;
struct perf_evlist *evsel_list;
struct perf_record *rec = &record;
+ char errbuf[BUFSIZ];

perf_header__set_cmdline(argc, argv);

@@ -884,11 +885,24 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
goto out_symbol_exit;
}

- perf_target__validate(&rec->opts.target);
+ err = perf_target__validate(&rec->opts.target);
+ if (err != PERF_TARGET__SUCCESS) {
+ perf_target__strerror(&rec->opts.target, err, errbuf, BUFSIZ);
+ ui__warning("%s", errbuf);
+ }
+
+ err = perf_target__parse_uid(&rec->opts.target);
+ if (err != PERF_TARGET__SUCCESS) {
+ int saved_errno = errno;

- if (perf_target__parse_uid(&rec->opts.target) != PERF_TARGET__SUCCESS)
+ perf_target__strerror(&rec->opts.target, err, errbuf, BUFSIZ);
+ ui__warning("%s", errbuf);
+
+ err = -saved_errno;
goto out_free_fd;
+ }

+ err = -ENOMEM;
if (perf_evlist__create_maps(evsel_list, &rec->opts.target) < 0)
usage_with_options(record_usage, record_options);

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 80fccc7bd1e0..6f584179ecc1 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1150,7 +1150,8 @@ static const char * const top_usage[] = {
int cmd_top(int argc, const char **argv, const char *prefix __used)
{
struct perf_evsel *pos;
- int status = -ENOMEM;
+ int status;
+ char errbuf[BUFSIZ];
struct perf_top top = {
.count_filter = 5,
.delay_secs = 2,
@@ -1252,10 +1253,22 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)

setup_browser(false);

- perf_target__validate(&top.target);
+ status = perf_target__validate(&top.target);
+ if (status != PERF_TARGET__SUCCESS) {
+ perf_target__strerror(&top.target, status, errbuf, BUFSIZ);
+ ui__warning("%s", errbuf);
+ }
+
+ status = perf_target__parse_uid(&top.target);
+ if (status != PERF_TARGET__SUCCESS) {
+ int saved_errno = errno;

- if (perf_target__parse_uid(&top.target) != PERF_TARGET__SUCCESS)
+ perf_target__strerror(&top.target, status, errbuf, BUFSIZ);
+ ui__warning("%s", errbuf);
+
+ status = -saved_errno;
goto out_delete_evlist;
+ }

if (perf_evlist__create_maps(top.evlist, &top.target) < 0)
usage_with_options(top_usage, options);
diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
index 26817daa2961..efb1fce259a4 100644
--- a/tools/perf/util/debug.c
+++ b/tools/perf/util/debug.c
@@ -11,6 +11,7 @@
#include "event.h"
#include "debug.h"
#include "util.h"
+#include "target.h"

int verbose;
bool dump_trace = false, quiet = false;
diff --git a/tools/perf/util/target.c b/tools/perf/util/target.c
index 140adfdfd520..196a24390bcf 100644
--- a/tools/perf/util/target.c
+++ b/tools/perf/util/target.c
@@ -89,3 +89,52 @@ enum perf_target_errno perf_target__parse_uid(struct perf_target *target)
target->uid = result->pw_uid;
return PERF_TARGET__SUCCESS;
}
+
+/*
+ * This must have a same ordering as the enum perf_target_errno.
+ */
+static const char *perf_target__error_str[] = {
+ "Success",
+ "PID/TID switch overriding CPU",
+ "PID/TID switch overriding UID",
+ "UID switch overriding CPU",
+ "PID/TID switch overriding SYSTEM",
+ "UID switch overriding SYSTEM",
+ "Invalid User: %s",
+ "Problems obtaining information for user %s",
+};
+
+int perf_target__strerror(struct perf_target *target, int errnum,
+ char *buf, size_t buflen)
+{
+ const char *msg;
+
+ if (errnum < __PERF_TARGET__ERRNO_START) {
+ strerror_r(errnum, buf, buflen);
+ return 0;
+ }
+
+ if (errnum >= __PERF_TARGET__ERRNO_END)
+ return -1;
+
+ errnum -= __PERF_TARGET__ERRNO_START;
+ msg = perf_target__error_str[errnum];
+
+ switch (errnum) {
+ case PERF_TARGET__SUCCESS:
+ case PERF_TARGET__PID_OVERRIDE_CPU ... PERF_TARGET__UID_OVERRIDE_SYSTEM:
+ snprintf(buf, buflen, "%s", msg);
+ break;
+
+ case PERF_TARGET__INVALID_UID:
+ case PERF_TARGET__USER_NOT_FOUND:
+ snprintf(buf, buflen, msg, target->uid_str);
+ break;
+
+ default:
+ /* cannot reach here */
+ break;
+ }
+
+ return 0;
+}
diff --git a/tools/perf/util/target.h b/tools/perf/util/target.h
index ec35fc043260..fff28c18f401 100644
--- a/tools/perf/util/target.h
+++ b/tools/perf/util/target.h
@@ -37,4 +37,7 @@ enum perf_target_errno {
enum perf_target_errno perf_target__validate(struct perf_target *target);
enum perf_target_errno perf_target__parse_uid(struct perf_target *target);

+int perf_target__strerror(struct perf_target *target, int errnum, char *buf,
+ size_t buflen);
+
#endif /* _PERF_TARGET_H */
--
1.7.10

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

The perf_target_errno enumerations are used to indicate
specific error cases on perf target operations. It'd
help libperf being a more generic library.

Suggested-by: Arnaldo Carvalho de Melo <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/target.c | 33 ++++++++++++++++++++++-----------
tools/perf/util/target.h | 20 +++++++++++++++++++-
2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/target.c b/tools/perf/util/target.c
index 3fadf85dd7e3..04cc374b7c32 100644
--- a/tools/perf/util/target.c
+++ b/tools/perf/util/target.c
@@ -10,36 +10,47 @@
#include "debug.h"


-void perf_target__validate(struct perf_target *target)
+enum perf_target_errno perf_target__validate(struct perf_target *target)
{
+ enum perf_target_errno ret = PERF_TARGET__SUCCESS;
+
if (target->pid)
target->tid = target->pid;

/* CPU and PID are mutually exclusive */
if (target->tid && target->cpu_list) {
- ui__warning("WARNING: PID switch overriding CPU\n");
- sleep(1);
target->cpu_list = NULL;
+ if (ret == PERF_TARGET__SUCCESS)
+ ret = PERF_TARGET__PID_OVERRIDE_CPU;
}

/* UID and PID are mutually exclusive */
if (target->tid && target->uid_str) {
- ui__warning("PID/TID switch overriding UID\n");
- sleep(1);
target->uid_str = NULL;
+ if (ret == PERF_TARGET__SUCCESS)
+ ret = PERF_TARGET__PID_OVERRIDE_UID;
}

/* UID and CPU are mutually exclusive */
if (target->uid_str && target->cpu_list) {
- ui__warning("UID switch overriding CPU\n");
- sleep(1);
target->cpu_list = NULL;
+ if (ret == PERF_TARGET__SUCCESS)
+ ret = PERF_TARGET__UID_OVERRIDE_CPU;
}

- /* PID/UID and SYSTEM are mutually exclusive */
- if ((target->tid || target->uid_str) && target->system_wide) {
- ui__warning("PID/TID/UID switch overriding CPU\n");
- sleep(1);
+ /* PID and SYSTEM are mutually exclusive */
+ if (target->tid && target->system_wide) {
target->system_wide = false;
+ if (ret == PERF_TARGET__SUCCESS)
+ ret = PERF_TARGET__PID_OVERRIDE_SYSTEM;
}
+
+ /* UID and SYSTEM are mutually exclusive */
+ if (target->uid_str && target->system_wide) {
+ target->system_wide = false;
+ if (ret == PERF_TARGET__SUCCESS)
+ ret = PERF_TARGET__UID_OVERRIDE_SYSTEM;
+ }
+
+ return ret;
}
diff --git a/tools/perf/util/target.h b/tools/perf/util/target.h
index 1348065ada5e..c3914c8a9890 100644
--- a/tools/perf/util/target.h
+++ b/tools/perf/util/target.h
@@ -12,6 +12,24 @@ struct perf_target {
bool system_wide;
};

-void perf_target__validate(struct perf_target *target);
+enum perf_target_errno {
+ /*
+ * XXX: Just choose an arbitrary big number standard errno can't have
+ */
+ __PERF_TARGET__ERRNO_START = 0x10000,
+
+ PERF_TARGET__SUCCESS = __PERF_TARGET__ERRNO_START,
+
+ /* for perf_target__validate() */
+ PERF_TARGET__PID_OVERRIDE_CPU,
+ PERF_TARGET__PID_OVERRIDE_UID,
+ PERF_TARGET__UID_OVERRIDE_CPU,
+ PERF_TARGET__PID_OVERRIDE_SYSTEM,
+ PERF_TARGET__UID_OVERRIDE_SYSTEM,
+
+ __PERF_TARGET__ERRNO_END
+};
+
+enum perf_target_errno perf_target__validate(struct perf_target *target);

#endif /* _PERF_TARGET_H */
--
1.7.10

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

Add and use the modern perf_target__parse_uid() and get rid of
the old parse_target_uid().

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/builtin-record.c | 4 +---
tools/perf/builtin-top.c | 3 +--
tools/perf/util/target.c | 35 +++++++++++++++++++++++++++++++++++
tools/perf/util/target.h | 5 +++++
tools/perf/util/usage.c | 31 -------------------------------
tools/perf/util/util.h | 3 ---
6 files changed, 42 insertions(+), 39 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index d16590942cec..e5d77452eef6 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -886,9 +886,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)

perf_target__validate(&rec->opts.target);

- rec->opts.target.uid = parse_target_uid(rec->opts.target.uid_str);
- if (rec->opts.target.uid_str != NULL &&
- rec->opts.target.uid == UINT_MAX - 1)
+ if (perf_target__parse_uid(&rec->opts.target) != PERF_TARGET__SUCCESS)
goto out_free_fd;

if (perf_evlist__create_maps(evsel_list, &rec->opts.target) < 0)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 2a0ec09b9b77..80fccc7bd1e0 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1254,8 +1254,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)

perf_target__validate(&top.target);

- top.target.uid = parse_target_uid(top.target.uid_str);
- if (top.target.uid_str != NULL && top.target.uid == UINT_MAX - 1)
+ if (perf_target__parse_uid(&top.target) != PERF_TARGET__SUCCESS)
goto out_delete_evlist;

if (perf_evlist__create_maps(top.evlist, &top.target) < 0)
diff --git a/tools/perf/util/target.c b/tools/perf/util/target.c
index 04cc374b7c32..140adfdfd520 100644
--- a/tools/perf/util/target.c
+++ b/tools/perf/util/target.c
@@ -9,6 +9,8 @@
#include "target.h"
#include "debug.h"

+#include <pwd.h>
+

enum perf_target_errno perf_target__validate(struct perf_target *target)
{
@@ -54,3 +56,36 @@ enum perf_target_errno perf_target__validate(struct perf_target *target)

return ret;
}
+
+enum perf_target_errno perf_target__parse_uid(struct perf_target *target)
+{
+ struct passwd pwd, *result;
+ char buf[1024];
+ const char *str = target->uid_str;
+
+ target->uid = UINT_MAX;
+ if (str == NULL)
+ return PERF_TARGET__SUCCESS;
+
+ /* Try user name first */
+ getpwnam_r(str, &pwd, buf, sizeof(buf), &result);
+
+ if (result == NULL) {
+ /*
+ * The user name not found. Maybe it's a UID number.
+ */
+ char *endptr;
+ int uid = strtol(str, &endptr, 10);
+
+ if (*endptr != '\0')
+ return PERF_TARGET__INVALID_UID;
+
+ getpwuid_r(uid, &pwd, buf, sizeof(buf), &result);
+
+ if (result == NULL)
+ return PERF_TARGET__USER_NOT_FOUND;
+ }
+
+ target->uid = result->pw_uid;
+ return PERF_TARGET__SUCCESS;
+}
diff --git a/tools/perf/util/target.h b/tools/perf/util/target.h
index c3914c8a9890..ec35fc043260 100644
--- a/tools/perf/util/target.h
+++ b/tools/perf/util/target.h
@@ -27,9 +27,14 @@ enum perf_target_errno {
PERF_TARGET__PID_OVERRIDE_SYSTEM,
PERF_TARGET__UID_OVERRIDE_SYSTEM,

+ /* for perf_target__parse_uid() */
+ PERF_TARGET__INVALID_UID,
+ PERF_TARGET__USER_NOT_FOUND,
+
__PERF_TARGET__ERRNO_END
};

enum perf_target_errno perf_target__validate(struct perf_target *target);
+enum perf_target_errno perf_target__parse_uid(struct perf_target *target);

#endif /* _PERF_TARGET_H */
diff --git a/tools/perf/util/usage.c b/tools/perf/util/usage.c
index e851abc22ccc..4007aca8e0ca 100644
--- a/tools/perf/util/usage.c
+++ b/tools/perf/util/usage.c
@@ -82,34 +82,3 @@ void warning(const char *warn, ...)
warn_routine(warn, params);
va_end(params);
}
-
-uid_t parse_target_uid(const char *str)
-{
- struct passwd pwd, *result;
- char buf[1024];
-
- if (str == NULL)
- return UINT_MAX;
-
- getpwnam_r(str, &pwd, buf, sizeof(buf), &result);
-
- if (result == NULL) {
- char *endptr;
- int uid = strtol(str, &endptr, 10);
-
- if (*endptr != '\0') {
- ui__error("Invalid user %s\n", str);
- return UINT_MAX - 1;
- }
-
- getpwuid_r(uid, &pwd, buf, sizeof(buf), &result);
-
- if (result == NULL) {
- ui__error("Problems obtaining information for user %s\n",
- str);
- return UINT_MAX - 1;
- }
- }
-
- return result->pw_uid;
-}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 52be74c359d3..27a11a78ad39 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -74,7 +74,6 @@
#include <netinet/tcp.h>
#include <arpa/inet.h>
#include <netdb.h>
-#include <pwd.h>
#include <inttypes.h>
#include "../../../include/linux/magic.h"
#include "types.h"
@@ -249,8 +248,6 @@ struct perf_event_attr;

void event_attr_init(struct perf_event_attr *attr);

-uid_t parse_target_uid(const char *str);
-
#define _STR(x) #x
#define STR(x) _STR(x)

--
1.7.10

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

Use struct perf_target as it is introduced by previous patch.
This is a preparation of further changes.

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/builtin-stat.c | 47 +++++++++++++++++++++------------------------
1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index dde9e17c018b..1ca767d906ef 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -175,22 +175,19 @@ static struct perf_event_attr very_very_detailed_attrs[] = {

static struct perf_evlist *evsel_list;

-static bool system_wide = false;
-static int run_idx = 0;
+static struct perf_target target;

+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 const char *target_pid;
-static const char *target_tid;
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;
@@ -293,10 +290,10 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,

attr->inherit = !no_inherit;

- if (system_wide)
+ if (target.system_wide)
return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
group, group_fd);
- if (!target_pid && !target_tid && (!group || evsel == first)) {
+ if (!target.pid && !target.tid && (!group || evsel == first)) {
attr->disabled = 1;
attr->enable_on_exec = 1;
}
@@ -446,7 +443,7 @@ static int run_perf_stat(int argc __used, const char **argv)
exit(-1);
}

- if (!target_tid && !target_pid && !system_wide)
+ if (!target.tid && !target.pid && !target.system_wide)
evsel_list->threads->map[0] = child_pid;

/*
@@ -476,7 +473,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 " : "");
+ target.system_wide ? "system-wide " : "");
} else {
error("open_counter returned with %d (%s). "
"/bin/dmesg may provide additional information.\n",
@@ -968,14 +965,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 && !target_tid) {
+ if (!target.pid && !target.tid) {
fprintf(output, "\'%s", argv[0]);
for (i = 1; i < argc; i++)
fprintf(output, " %s", argv);
- } else if (target_pid)
- fprintf(output, "process id \'%s", target_pid);
+ } else if (target.pid)
+ fprintf(output, "process id \'%s", target.pid);
else
- fprintf(output, "thread id \'%s", target_tid);
+ fprintf(output, "thread id \'%s", target.tid);

fprintf(output, "\'");
if (run_count > 1)
@@ -1049,11 +1046,11 @@ static const struct option options[] = {
"event filter", parse_filter),
OPT_BOOLEAN('i', "no-inherit", &no_inherit,
"child tasks do not inherit counters"),
- OPT_STRING('p', "pid", &target_pid, "pid",
+ OPT_STRING('p', "pid", &target.pid, "pid",
"stat events on existing process id"),
- OPT_STRING('t', "tid", &target_tid, "tid",
+ OPT_STRING('t', "tid", &target.tid, "tid",
"stat events on existing thread id"),
- OPT_BOOLEAN('a', "all-cpus", &system_wide,
+ OPT_BOOLEAN('a', "all-cpus", &target.system_wide,
"system-wide collection from all CPUs"),
OPT_BOOLEAN('g', "group", &group,
"put the counters into a counter group"),
@@ -1072,7 +1069,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", &target.cpu_list, "cpu",
"list of cpus to monitor in system-wide"),
OPT_BOOLEAN('A', "no-aggr", &no_aggr,
"disable CPU count aggregation"),
@@ -1190,13 +1187,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 && !target_tid)
+ if (!argc && !target.pid && !target.tid)
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) && !target.system_wide) {
fprintf(stderr, "both cgroup and no-aggregation "
"modes only available in system-wide mode\n");

@@ -1206,18 +1203,18 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
if (add_default_attributes())
goto out;

- if (target_pid)
- target_tid = target_pid;
+ if (target.pid)
+ target.tid = target.pid;

- evsel_list->threads = thread_map__new_str(target_pid,
- target_tid, UINT_MAX);
+ evsel_list->threads = thread_map__new_str(target.pid,
+ 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 (target.system_wide)
+ evsel_list->cpus = cpu_map__new(target.cpu_list);
else
evsel_list->cpus = cpu_map__dummy_new();

--
1.7.10

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

For further work on perf_target, it'd be better off splitting
the code into a separate file.

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/Makefile | 2 ++
tools/perf/perf.h | 9 +--------
tools/perf/util/evlist.c | 1 +
tools/perf/util/evsel.c | 1 +
tools/perf/util/target.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/target.h | 17 +++++++++++++++++
tools/perf/util/usage.c | 34 ----------------------------------
tools/perf/util/util.h | 2 --
8 files changed, 67 insertions(+), 44 deletions(-)
create mode 100644 tools/perf/util/target.c
create mode 100644 tools/perf/util/target.h

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index e98e14c88532..4122a668952e 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -300,6 +300,7 @@ LIB_H += util/cpumap.h
LIB_H += util/top.h
LIB_H += $(ARCH_INCLUDE)
LIB_H += util/cgroup.h
+LIB_H += util/target.h

LIB_OBJS += $(OUTPUT)util/abspath.o
LIB_OBJS += $(OUTPUT)util/alias.o
@@ -361,6 +362,7 @@ LIB_OBJS += $(OUTPUT)util/util.o
LIB_OBJS += $(OUTPUT)util/xyarray.o
LIB_OBJS += $(OUTPUT)util/cpumap.o
LIB_OBJS += $(OUTPUT)util/cgroup.o
+LIB_OBJS += $(OUTPUT)util/target.o

BUILTIN_OBJS += $(OUTPUT)builtin-annotate.o

diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 7e226c0e0e31..14f1034f14f9 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -207,14 +207,7 @@ extern const char perf_version_string[];

void pthread__unblock_sigwinch(void);

-struct perf_target {
- const char *pid;
- const char *tid;
- const char *cpu_list;
- const char *uid_str;
- uid_t uid;
- bool system_wide;
-};
+#include "util/target.h"

struct perf_record_opts {
struct perf_target target;
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 556d98af1a0b..183b199b0d09 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -11,6 +11,7 @@
#include <poll.h>
#include "cpumap.h"
#include "thread_map.h"
+#include "target.h"
#include "evlist.h"
#include "evsel.h"
#include <unistd.h>
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index d90598edcf1d..bb785a098ced 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -14,6 +14,7 @@
#include "util.h"
#include "cpumap.h"
#include "thread_map.h"
+#include "target.h"

#define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
#define GROUP_FD(group_fd, cpu) (*(int *)xyarray__entry(group_fd, cpu, 0))
diff --git a/tools/perf/util/target.c b/tools/perf/util/target.c
new file mode 100644
index 000000000000..3fadf85dd7e3
--- /dev/null
+++ b/tools/perf/util/target.c
@@ -0,0 +1,45 @@
+/*
+ * Helper functions for handling target threads/cpus
+ *
+ * Copyright (C) 2012, LG Electronics, Namhyung Kim <[email protected]>
+ *
+ * Released under the GPL v2.
+ */
+
+#include "target.h"
+#include "debug.h"
+
+
+void perf_target__validate(struct perf_target *target)
+{
+ if (target->pid)
+ target->tid = target->pid;
+
+ /* CPU and PID are mutually exclusive */
+ if (target->tid && target->cpu_list) {
+ ui__warning("WARNING: PID switch overriding CPU\n");
+ sleep(1);
+ target->cpu_list = NULL;
+ }
+
+ /* UID and PID are mutually exclusive */
+ if (target->tid && target->uid_str) {
+ ui__warning("PID/TID switch overriding UID\n");
+ sleep(1);
+ target->uid_str = NULL;
+ }
+
+ /* UID and CPU are mutually exclusive */
+ if (target->uid_str && target->cpu_list) {
+ ui__warning("UID switch overriding CPU\n");
+ sleep(1);
+ target->cpu_list = NULL;
+ }
+
+ /* PID/UID and SYSTEM are mutually exclusive */
+ if ((target->tid || target->uid_str) && target->system_wide) {
+ ui__warning("PID/TID/UID switch overriding CPU\n");
+ sleep(1);
+ target->system_wide = false;
+ }
+}
diff --git a/tools/perf/util/target.h b/tools/perf/util/target.h
new file mode 100644
index 000000000000..1348065ada5e
--- /dev/null
+++ b/tools/perf/util/target.h
@@ -0,0 +1,17 @@
+#ifndef _PERF_TARGET_H
+#define _PERF_TARGET_H
+
+#include "util.h"
+
+struct perf_target {
+ const char *pid;
+ const char *tid;
+ const char *cpu_list;
+ const char *uid_str;
+ uid_t uid;
+ bool system_wide;
+};
+
+void perf_target__validate(struct perf_target *target);
+
+#endif /* _PERF_TARGET_H */
diff --git a/tools/perf/util/usage.c b/tools/perf/util/usage.c
index 228f0a558872..e851abc22ccc 100644
--- a/tools/perf/util/usage.c
+++ b/tools/perf/util/usage.c
@@ -113,37 +113,3 @@ uid_t parse_target_uid(const char *str)

return result->pw_uid;
}
-
-void perf_target__validate(struct perf_target *target)
-{
- if (target->pid)
- target->tid = target->pid;
-
- /* CPU and PID are mutually exclusive */
- if (target->tid && target->cpu_list) {
- ui__warning("WARNING: PID switch overriding CPU\n");
- sleep(1);
- target->cpu_list = NULL;
- }
-
- /* UID and PID are mutually exclusive */
- if (target->tid && target->uid_str) {
- ui__warning("PID/TID switch overriding UID\n");
- sleep(1);
- target->uid_str = NULL;
- }
-
- /* UID and CPU are mutually exclusive */
- if (target->uid_str && target->cpu_list) {
- ui__warning("UID switch overriding CPU\n");
- sleep(1);
- target->cpu_list = NULL;
- }
-
- /* PID/UID and SYSTEM are mutually exclusive */
- if ((target->tid || target->uid_str) && target->system_wide) {
- ui__warning("PID/TID/UID switch overriding CPU\n");
- sleep(1);
- target->system_wide = false;
- }
-}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 3f05d6264dab..52be74c359d3 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -246,12 +246,10 @@ unsigned long convert_unit(unsigned long value, char *unit);
int readn(int fd, void *buf, size_t size);

struct perf_event_attr;
-struct perf_target;

void event_attr_init(struct perf_event_attr *attr);

uid_t parse_target_uid(const char *str);
-void perf_target__validate(struct perf_target *target);

#define _STR(x) #x
#define STR(x) _STR(x)
--
1.7.10

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

Use struct perf_target as it is introduced by previous patch.
This is a preparation of further changes.

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/builtin-top.c | 33 +++++++++++++++++----------------
tools/perf/util/top.c | 19 ++++++++++---------
tools/perf/util/top.h | 6 +-----
3 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 8ef59f8262bb..2c1c207627b4 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -588,7 +588,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->target.uid_str;

perf_evlist__tui_browse_hists(top->evlist, help,
perf_top__sort_new_samples,
@@ -1016,7 +1016,7 @@ static int __cmd_top(struct perf_top *top)
if (ret)
goto out_delete;

- if (top->target_tid || top->uid != UINT_MAX)
+ if (top->target.tid || top->target.uid != UINT_MAX)
perf_event__synthesize_thread_map(&top->tool, top->evlist->threads,
perf_event__process,
&top->session->host_machine);
@@ -1154,7 +1154,6 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
struct perf_top top = {
.count_filter = 5,
.delay_secs = 2,
- .uid = UINT_MAX,
.freq = 1000, /* 1 KHz */
.mmap_pages = 128,
.sym_pcnt_filter = 5,
@@ -1166,13 +1165,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_STRING('p', "pid", &top.target_pid, "pid",
+ OPT_STRING('p', "pid", &top.target.pid, "pid",
"profile events on existing process id"),
- OPT_STRING('t', "tid", &top.target_tid, "tid",
+ OPT_STRING('t', "tid", &top.target.tid, "tid",
"profile events on existing thread id"),
- OPT_BOOLEAN('a', "all-cpus", &top.system_wide,
+ OPT_BOOLEAN('a', "all-cpus", &top.target.system_wide,
"system-wide collection from all CPUs"),
- OPT_STRING('C', "cpu", &top.cpu_list, "cpu",
+ OPT_STRING('C', "cpu", &top.target.cpu_list, "cpu",
"list of cpus to monitor"),
OPT_STRING('k', "vmlinux", &symbol_conf.vmlinux_name,
"file", "vmlinux pathname"),
@@ -1227,7 +1226,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.target.uid_str, "user", "user to profile"),
OPT_END()
};

@@ -1253,22 +1252,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.target.uid = parse_target_uid(top.target.uid_str, top.target.tid,
+ top.target.pid);
+ if (top.target.uid_str != NULL && top.target.uid == UINT_MAX - 1)
goto out_delete_evlist;

/* CPU and PID are mutually exclusive */
- if (top.target_tid && top.cpu_list) {
+ if (top.target.tid && top.target.cpu_list) {
printf("WARNING: PID switch overriding CPU\n");
sleep(1);
- top.cpu_list = NULL;
+ top.target.cpu_list = NULL;
}

- if (top.target_pid)
- top.target_tid = top.target_pid;
+ if (top.target.pid)
+ top.target.tid = top.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.target.pid,
+ top.target.tid, top.target.uid,
+ top.target.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 09fe579ccafb..abe0e8e95068 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)
+ if (top->target.pid)
ret += SNPRINTF(bf + ret, size - ret, " (target_pid: %s",
- top->target_pid);
- else if (top->target_tid)
+ top->target.pid);
+ else if (top->target.tid)
ret += SNPRINTF(bf + ret, size - ret, " (target_tid: %s",
- top->target_tid);
- else if (top->uid_str != NULL)
+ top->target.tid);
+ else if (top->target.uid_str != NULL)
ret += SNPRINTF(bf + ret, size - ret, " (uid: %s",
- top->uid_str);
+ top->target.uid_str);
else
ret += SNPRINTF(bf + ret, size - ret, " (all");

- if (top->cpu_list)
+ if (top->target.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->target.cpu_list);
else {
- if (top->target_tid)
+ if (top->target.tid)
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 ce61cb2d1acf..33347ca89ee4 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_target target;
/*
* Symbols will be added here in perf_event__process_sample and will
* get out after decayed.
@@ -23,10 +24,7 @@ struct perf_top {
u64 guest_us_samples, guest_kernel_samples;
int print_entries, count_filter, delay_secs;
int freq;
- const char *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;
@@ -37,7 +35,6 @@ struct perf_top {
bool sample_id_all_missing;
bool exclude_guest_missing;
bool dump_symtab;
- const char *cpu_list;
struct hist_entry *sym_filter_entry;
struct perf_evsel *sym_evsel;
struct perf_session *session;
@@ -47,7 +44,6 @@ struct perf_top {
int realtime_prio;
int sym_pcnt_filter;
const char *sym_filter;
- const char *uid_str;
};

size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size);
--
1.7.10

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

The perf_target 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 better factoring it out.

Thanks to Arnaldo for suggesting the better name.

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/builtin-record.c | 41 ++++++++++++++++++++++-------------------
tools/perf/builtin-test.c | 5 +++--
tools/perf/perf.h | 15 ++++++++++-----
tools/perf/util/evlist.c | 2 +-
tools/perf/util/evsel.c | 10 +++++-----
5 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 10b1f1f25ed7..4dcf27057bd2 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;
@@ -218,7 +217,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->target.cpu_list) {
die("No such device - did you specify"
" an out-of-range profile CPU?\n");
} else if (err == EINVAL) {
@@ -578,7 +577,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->target.system_wide)
perf_event__synthesize_thread_map(tool, evsel_list->threads,
process_synthesized_event,
machine);
@@ -765,9 +764,9 @@ const struct option record_options[] = {
parse_events_option),
OPT_CALLBACK(0, "filter", &record.evlist, "filter",
"event filter", parse_filter),
- OPT_STRING('p', "pid", &record.opts.target_pid, "pid",
+ OPT_STRING('p', "pid", &record.opts.target.pid, "pid",
"record events on existing process id"),
- OPT_STRING('t', "tid", &record.opts.target_tid, "tid",
+ OPT_STRING('t', "tid", &record.opts.target.tid, "tid",
"record events on existing thread id"),
OPT_INTEGER('r', "realtime", &record.realtime_prio,
"collect data with this RT SCHED_FIFO priority"),
@@ -775,11 +774,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.target.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.target.cpu_list, "cpu",
"list of cpus to monitor"),
OPT_BOOLEAN('f', "force", &record.force,
"overwrite existing data file (deprecated)"),
@@ -813,7 +812,8 @@ 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.target.uid_str, "user",
+ "user to profile"),

OPT_CALLBACK_NOOPT('b', "branch-any", &record.opts.branch_stack,
"branch any", "sample any taken branches",
@@ -842,8 +842,9 @@ 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 && !rec->opts.target_tid &&
- !rec->opts.system_wide && !rec->opts.cpu_list && !rec->uid_str)
+ if (!argc && !rec->opts.target.pid && !rec->opts.target.tid &&
+ !rec->opts.target.system_wide && !rec->opts.target.cpu_list &&
+ !rec->opts.target.uid_str)
usage_with_options(record_usage, record_options);

if (rec->force && rec->append_file) {
@@ -856,7 +857,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.target.system_wide) {
fprintf(stderr, "cgroup monitoring only available in"
" system-wide mode\n");
usage_with_options(record_usage, record_options);
@@ -883,17 +884,19 @@ 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.target.uid = parse_target_uid(rec->opts.target.uid_str,
+ rec->opts.target.tid,
+ rec->opts.target.pid);
+ if (rec->opts.target.uid_str != NULL &&
+ rec->opts.target.uid == UINT_MAX - 1)
goto out_free_fd;

- if (rec->opts.target_pid)
- rec->opts.target_tid = rec->opts.target_pid;
+ if (rec->opts.target.pid)
+ rec->opts.target.tid = rec->opts.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.target.pid,
+ rec->opts.target.tid, rec->opts.target.uid,
+ rec->opts.target.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 1c5b9801ac61..e9b256920da0 100644
--- a/tools/perf/builtin-test.c
+++ b/tools/perf/builtin-test.c
@@ -1207,8 +1207,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.target.pid,
+ opts.target.tid, UINT_MAX,
+ opts.target.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 89e3355ab173..7e226c0e0e31 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -207,10 +207,17 @@ extern const char perf_version_string[];

void pthread__unblock_sigwinch(void);

-struct perf_record_opts {
- const char *target_pid;
- const char *target_tid;
+struct perf_target {
+ const char *pid;
+ const char *tid;
+ const char *cpu_list;
+ const char *uid_str;
uid_t uid;
+ bool system_wide;
+};
+
+struct perf_record_opts {
+ struct perf_target target;
bool call_graph;
bool group;
bool inherit_stat;
@@ -223,7 +230,6 @@ struct perf_record_opts {
bool sample_time;
bool sample_id_all_missing;
bool exclude_guest_missing;
- bool system_wide;
bool period;
unsigned int freq;
unsigned int mmap_pages;
@@ -231,7 +237,6 @@ struct perf_record_opts {
int branch_stack;
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 1986d8051bd1..7080901a2717 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -827,7 +827,7 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist,
exit(-1);
}

- if (!opts->system_wide && !opts->target_tid && !opts->target_pid)
+ if (!opts->target.system_wide && !opts->target.tid && !opts->target.pid)
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 8c13dbcb84b9..d90598edcf1d 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -106,15 +106,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->target.system_wide)
attr->sample_type |= PERF_SAMPLE_CPU;

if (opts->period)
attr->sample_type |= PERF_SAMPLE_PERIOD;

if (!opts->sample_id_all_missing &&
- (opts->sample_time || opts->system_wide ||
- !opts->no_inherit || opts->cpu_list))
+ (opts->sample_time || opts->target.system_wide ||
+ !opts->no_inherit || opts->target.cpu_list))
attr->sample_type |= PERF_SAMPLE_TIME;

if (opts->raw_samples) {
@@ -135,8 +135,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
attr->mmap = track;
attr->comm = track;

- if (!opts->target_pid && !opts->target_tid && !opts->system_wide &&
- (!opts->group || evsel == first)) {
+ if (!opts->target.pid && !opts->target.tid &&
+ !opts->target.system_wide && (!opts->group || evsel == first)) {
attr->disabled = 1;
attr->enable_on_exec = 1;
}
--
1.7.10

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

On 4/25/12 11:15 PM, Namhyung Kim wrote:
> 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
> perf_target__validate(), so we can make the conditional bit simpler.
>
> This also fixes perf test 7 (Validate) failure on my 6 core machine:
>
> $ cat /sys/devices/system/cpu/online
> 0-11
> $ ./perf test -v 7
> 7: Validate PERF_RECORD_* events& perf_sample fields:
> --- start ---
> perf_evlist__mmap: Operation not permitted
> ---- end ----
> Validate PERF_RECORD_* events& perf_sample fields: FAILED!

Works fine for me with latest tip:
$ cat /sys/devices/system/cpu/online
0-15

$ /tmp/perf/perf test -v 7
7: Validate PERF_RECORD_* events & perf_sample fields:
--- start ---
64740167922229 0 PERF_RECORD_SAMPLE
64740167926354 0 PERF_RECORD_SAMPLE
64740167928389 0 PERF_RECORD_SAMPLE
64740167930832 0 PERF_RECORD_SAMPLE
64740168404145 0 PERF_RECORD_COMM: sleep:16523
64740168424672 0 PERF_RECORD_MMAP 16523/16523: [0x400000(0x6000) @ 0]:
/bin/sleep
64740168441676 0 PERF_RECORD_MMAP 16523/16523: [0x7f83de5bb000(0x224000)
@ 0]: /lib64/ld-2.14.90.so
64740168458460 0 PERF_RECORD_MMAP 16523/16523: [0x7fff009ff000(0x1000) @
0x7fff009ff000]: [vdso]
64740168586358 0 PERF_RECORD_MMAP 16523/16523: [0x7f83de203000(0x3b8000)
@ 0]: /lib64/libc-2.14.90.so
64741168625653 0 PERF_RECORD_EXIT(16523:16523):(16523:16523)
---- end ----
Validate PERF_RECORD_* events & perf_sample fields: Ok

Is the failure a by-product of the other patches in this set?

David

>
> Signed-off-by: Namhyung Kim<[email protected]>
> ---
> tools/perf/util/evlist.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index a43e2c56d1c6..556d98af1a0b 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -608,8 +608,9 @@ int perf_evlist__create_maps(struct perf_evlist *evlist,
> if (evlist->threads == NULL)
> return -1;
>
> - if (target->uid != UINT_MAX ||
> - (target->cpu_list == NULL&& target->tid))
> + if (target->uid != UINT_MAX || target->tid)
> + evlist->cpus = cpu_map__dummy_new();
> + else if (!target->system_wide&& target->cpu_list == NULL)
> evlist->cpus = cpu_map__dummy_new();
> else
> evlist->cpus = cpu_map__new(target->cpu_list);

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

Hi,

2012-04-26 (Thu), 09:05 -0600, David Ahern wrote:
> On 4/25/12 11:15 PM, Namhyung Kim wrote:
> > 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
> > perf_target__validate(), so we can make the conditional bit simpler.
> >
> > This also fixes perf test 7 (Validate) failure on my 6 core machine:
> >
> > $ cat /sys/devices/system/cpu/online
> > 0-11
> > $ ./perf test -v 7
> > 7: Validate PERF_RECORD_* events& perf_sample fields:
> > --- start ---
> > perf_evlist__mmap: Operation not permitted
> > ---- end ----
> > Validate PERF_RECORD_* events& perf_sample fields: FAILED!
>
> Works fine for me with latest tip:
> $ cat /sys/devices/system/cpu/online
> 0-15
>
> $ /tmp/perf/perf test -v 7
> 7: Validate PERF_RECORD_* events & perf_sample fields:
> --- start ---
> 64740167922229 0 PERF_RECORD_SAMPLE
> 64740167926354 0 PERF_RECORD_SAMPLE
> 64740167928389 0 PERF_RECORD_SAMPLE
> 64740167930832 0 PERF_RECORD_SAMPLE
> 64740168404145 0 PERF_RECORD_COMM: sleep:16523
> 64740168424672 0 PERF_RECORD_MMAP 16523/16523: [0x400000(0x6000) @ 0]:
> /bin/sleep
> 64740168441676 0 PERF_RECORD_MMAP 16523/16523: [0x7f83de5bb000(0x224000)
> @ 0]: /lib64/ld-2.14.90.so
> 64740168458460 0 PERF_RECORD_MMAP 16523/16523: [0x7fff009ff000(0x1000) @
> 0x7fff009ff000]: [vdso]
> 64740168586358 0 PERF_RECORD_MMAP 16523/16523: [0x7f83de203000(0x3b8000)
> @ 0]: /lib64/libc-2.14.90.so
> 64741168625653 0 PERF_RECORD_EXIT(16523:16523):(16523:16523)
> ---- end ----
> Validate PERF_RECORD_* events & perf_sample fields: Ok
>
> Is the failure a by-product of the other patches in this set?
>

Hmm.. No, I can reproduce it without any of this series. And now I think
that it is not related to the number of cpus. On my 4 core (no
hyperthreading) machine at home, the result was same.

BTW, did you change sysctl settings?

$ cat /sys/devices/system/cpu/online
0-3
$ grep . /proc/sys/kernel/perf_event_*
/proc/sys/kernel/perf_event_max_sample_rate:100000
/proc/sys/kernel/perf_event_mlock_kb:516
/proc/sys/kernel/perf_event_paranoid:1
$ ./perf test 7
7: Validate PERF_RECORD_* events & perf_sample fields: FAILED!
$ ./perf --version
perf version 3.4.rc1


Thanks.

--
Regards,
Namhyung Kim


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

On 4/26/12 3:12 PM, Namhyung Kim wrote:
> Hmm.. No, I can reproduce it without any of this series. And now I think
> that it is not related to the number of cpus. On my 4 core (no
> hyperthreading) machine at home, the result was same.
>
> BTW, did you change sysctl settings?
>
> $ cat /sys/devices/system/cpu/online
> 0-3
> $ grep . /proc/sys/kernel/perf_event_*
> /proc/sys/kernel/perf_event_max_sample_rate:100000
> /proc/sys/kernel/perf_event_mlock_kb:516
> /proc/sys/kernel/perf_event_paranoid:1

$ grep . /proc/sys/kernel/perf_event_*
/proc/sys/kernel/perf_event_max_sample_rate:100000
/proc/sys/kernel/perf_event_mlock_kb:516
/proc/sys/kernel/perf_event_paranoid:-1

That last one is the key. I have it set to not paranoid and usually run
perf a non-root user.

> $ ./perf test 7
> 7: Validate PERF_RECORD_* events& perf_sample fields: FAILED!
> $ ./perf --version
> perf version 3.4.rc1

running 3.4-rc4 kernel + perf from tip/perf/core branch.

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

Hi,

On Thu, 26 Apr 2012 15:22:55 -0600, David Ahern wrote:
> On 4/26/12 3:12 PM, Namhyung Kim wrote:
>> Hmm.. No, I can reproduce it without any of this series. And now I think
>> that it is not related to the number of cpus. On my 4 core (no
>> hyperthreading) machine at home, the result was same.
>>
>> BTW, did you change sysctl settings?
>>
>> $ cat /sys/devices/system/cpu/online
>> 0-3
>> $ grep . /proc/sys/kernel/perf_event_*
>> /proc/sys/kernel/perf_event_max_sample_rate:100000
>> /proc/sys/kernel/perf_event_mlock_kb:516
>> /proc/sys/kernel/perf_event_paranoid:1
>
> $ grep . /proc/sys/kernel/perf_event_*
> /proc/sys/kernel/perf_event_max_sample_rate:100000
> /proc/sys/kernel/perf_event_mlock_kb:516
> /proc/sys/kernel/perf_event_paranoid:-1
>
> That last one is the key. I have it set to not paranoid and usually
> run perf a non-root user.
>

That's exactly what I want to see :). On perf_mmap() we have:

if ((locked > lock_limit) && perf_paranoid_tracepoint_raw() &&
!capable(CAP_IPC_LOCK)) {
ret = -EPERM;
goto unlock;
}

So as long as you set perf_event_paranoid to -1 or run perf test as
root, you cannot see the failure.

Thanks,
Namhyung


>> $ ./perf test 7
>> 7: Validate PERF_RECORD_* events& perf_sample fields: FAILED!
>> $ ./perf --version
>> perf version 3.4.rc1
>
> running 3.4-rc4 kernel + perf from tip/perf/core branch.
>
> David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Em Fri, Apr 27, 2012 at 09:16:18AM +0900, Namhyung Kim escreveu:
> Hi,
>
> On Thu, 26 Apr 2012 15:22:55 -0600, David Ahern wrote:
> > On 4/26/12 3:12 PM, Namhyung Kim wrote:
> >> Hmm.. No, I can reproduce it without any of this series. And now I think
> >> that it is not related to the number of cpus. On my 4 core (no
> >> hyperthreading) machine at home, the result was same.
> >>
> >> BTW, did you change sysctl settings?
> >>
> >> $ cat /sys/devices/system/cpu/online
> >> 0-3
> >> $ grep . /proc/sys/kernel/perf_event_*
> >> /proc/sys/kernel/perf_event_max_sample_rate:100000
> >> /proc/sys/kernel/perf_event_mlock_kb:516
> >> /proc/sys/kernel/perf_event_paranoid:1
> >
> > $ grep . /proc/sys/kernel/perf_event_*
> > /proc/sys/kernel/perf_event_max_sample_rate:100000
> > /proc/sys/kernel/perf_event_mlock_kb:516
> > /proc/sys/kernel/perf_event_paranoid:-1
> >
> > That last one is the key. I have it set to not paranoid and usually
> > run perf a non-root user.
> >
>
> That's exactly what I want to see :). On perf_mmap() we have:
>
> if ((locked > lock_limit) && perf_paranoid_tracepoint_raw() &&
> !capable(CAP_IPC_LOCK)) {
> ret = -EPERM;
> goto unlock;
> }
>
> So as long as you set perf_event_paranoid to -1 or run perf test as
> root, you cannot see the failure.

Ok, after this discussion, David, can I have your acked-by or tested-by?

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

On 4/27/12 9:08 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 27, 2012 at 09:16:18AM +0900, Namhyung Kim escreveu:
>> Hi,
>>
>> On Thu, 26 Apr 2012 15:22:55 -0600, David Ahern wrote:
>>> On 4/26/12 3:12 PM, Namhyung Kim wrote:
>>>> Hmm.. No, I can reproduce it without any of this series. And now I think
>>>> that it is not related to the number of cpus. On my 4 core (no
>>>> hyperthreading) machine at home, the result was same.
>>>>
>>>> BTW, did you change sysctl settings?
>>>>
>>>> $ cat /sys/devices/system/cpu/online
>>>> 0-3
>>>> $ grep . /proc/sys/kernel/perf_event_*
>>>> /proc/sys/kernel/perf_event_max_sample_rate:100000
>>>> /proc/sys/kernel/perf_event_mlock_kb:516
>>>> /proc/sys/kernel/perf_event_paranoid:1
>>>
>>> $ grep . /proc/sys/kernel/perf_event_*
>>> /proc/sys/kernel/perf_event_max_sample_rate:100000
>>> /proc/sys/kernel/perf_event_mlock_kb:516
>>> /proc/sys/kernel/perf_event_paranoid:-1
>>>
>>> That last one is the key. I have it set to not paranoid and usually
>>> run perf a non-root user.
>>>
>>
>> That's exactly what I want to see :). On perf_mmap() we have:
>>
>> if ((locked> lock_limit)&& perf_paranoid_tracepoint_raw()&&
>> !capable(CAP_IPC_LOCK)) {
>> ret = -EPERM;
>> goto unlock;
>> }
>>
>> So as long as you set perf_event_paranoid to -1 or run perf test as
>> root, you cannot see the failure.
>
> Ok, after this discussion, David, can I have your acked-by or tested-by?
>
> - Arnaldo

I had reviewed the whole series yesterday, so you can add my

Reviewed-by: David Ahern <[email protected]>

to the whole series if you want. I did not pull down the set and apply
to test it, and this particular patch requires prior ones so I have not
tested it.

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

Em Fri, Apr 27, 2012 at 09:32:09AM -0600, David Ahern escreveu:
> On 4/27/12 9:08 AM, Arnaldo Carvalho de Melo wrote:
> >Ok, after this discussion, David, can I have your acked-by or tested-by?

> I had reviewed the whole series yesterday, so you can add my

> Reviewed-by: David Ahern <[email protected]>

> to the whole series if you want. I did not pull down the set and
> apply to test it, and this particular patch requires prior ones so I
> have not tested it.

Fair enough, I'll add the Reviewed-by tag.

I'm running 'perf test' after each patch. Doing it as root, but we need
to automate this to do it as root, user, and user with each of the
paranoid levels...

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

On 4/27/12 10:00 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 27, 2012 at 09:32:09AM -0600, David Ahern escreveu:
>> On 4/27/12 9:08 AM, Arnaldo Carvalho de Melo wrote:
>>> Ok, after this discussion, David, can I have your acked-by or tested-by?
>
>> I had reviewed the whole series yesterday, so you can add my
>
>> Reviewed-by: David Ahern<[email protected]>
>
>> to the whole series if you want. I did not pull down the set and
>> apply to test it, and this particular patch requires prior ones so I
>> have not tested it.
>
> Fair enough, I'll add the Reviewed-by tag.
>
> I'm running 'perf test' after each patch. Doing it as root, but we need
> to automate this to do it as root, user, and user with each of the
> paranoid levels...
>
> - Arnaldo

Right - and that may include changing paranoid settings.

Also, the patchset is listed as RFC if that matters from running through
all the formalities. One of the reasons I was not motivated to pull down
the patches and apply/build/test.

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

Em Fri, Apr 27, 2012 at 10:05:06AM -0600, David Ahern escreveu:
> On 4/27/12 10:00 AM, Arnaldo Carvalho de Melo wrote:
> >Em Fri, Apr 27, 2012 at 09:32:09AM -0600, David Ahern escreveu:
> >>On 4/27/12 9:08 AM, Arnaldo Carvalho de Melo wrote:
> >>>Ok, after this discussion, David, can I have your acked-by or tested-by?
> >
> >>I had reviewed the whole series yesterday, so you can add my
> >
> >>Reviewed-by: David Ahern<[email protected]>
> >
> >>to the whole series if you want. I did not pull down the set and
> >>apply to test it, and this particular patch requires prior ones so I
> >>have not tested it.
> >
> >Fair enough, I'll add the Reviewed-by tag.
> >
> >I'm running 'perf test' after each patch. Doing it as root, but we need
> >to automate this to do it as root, user, and user with each of the
> >paranoid levels...
> >
> >- Arnaldo
>
> Right - and that may include changing paranoid settings.
>
> Also, the patchset is listed as RFC if that matters from running
> through all the formalities. One of the reasons I was not motivated
> to pull down the patches and apply/build/test.

I'm ok with it, so I happily disregarded the RFC on it 8-)

And now that you reviewed it all too, even better.

Thanks,

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

Em Thu, Apr 26, 2012 at 02:15:22PM +0900, Namhyung Kim escreveu:
> For further work on perf_target, it'd be better off splitting
> the code into a separate file.
>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/perf/Makefile | 2 ++
> tools/perf/perf.h | 9 +--------
> tools/perf/util/evlist.c | 1 +
> tools/perf/util/evsel.c | 1 +
> tools/perf/util/target.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> tools/perf/util/target.h | 17 +++++++++++++++++
> tools/perf/util/usage.c | 34 ----------------------------------
> tools/perf/util/util.h | 2 --
> 8 files changed, 67 insertions(+), 44 deletions(-)
> create mode 100644 tools/perf/util/target.c
> create mode 100644 tools/perf/util/target.h

Trying to fix this now...

[acme@sandy linux]$ make -C tools/perf/ O=/home/git/build/perf install
make: Entering directory `/home/git/linux/tools/perf'
CC /home/git/build/perf/builtin-bench.o
CC /home/git/build/perf/bench/sched-messaging.o
CC /home/git/build/perf/bench/sched-pipe.o
CC /home/git/build/perf/scripts/perl/Perf-Trace-Util/Context.o
In file included from
scripts/perl/Perf-Trace-Util/../../../util/target.h:4,
from scripts/perl/Perf-Trace-Util/../../../perf.h:210,
from Context.xs:25:
scripts/perl/Perf-Trace-Util/../../../util/util.h:44:1: error:
"HAS_BOOL" redefined
In file included from /usr/lib64/perl5/CORE/perl.h:2424,
from Context.xs:23:
/usr/lib64/perl5/CORE/handy.h:110:1: error: this is the location of the
previous definition
In file included from
scripts/perl/Perf-Trace-Util/../../../util/target.h:4,
from scripts/perl/Perf-Trace-Util/../../../perf.h:210,
from Context.xs:25:
scripts/perl/Perf-Trace-Util/../../../util/util.h:133: error:
conflicting types for ‘Perl_die_nocontext’
/usr/lib64/perl5/CORE/proto.h:331: note: previous declaration of
‘Perl_die_nocontext’ was here
make: *** [/home/git/build/perf/scripts/perl/Perf-Trace-Util/Context.o]
Error 1
make: Leaving directory `/home/git/linux/tools/perf'
[acme@sandy linux]$
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Em Thu, Apr 26, 2012 at 02:15:23PM +0900, Namhyung Kim escreveu:
> The perf_target_errno enumerations are used to indicate
> specific error cases on perf target operations. It'd
> help libperf being a more generic library.
>
> Suggested-by: Arnaldo Carvalho de Melo <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> + /* UID and SYSTEM are mutually exclusive */
> + if (target->uid_str && target->system_wide) {
> + target->system_wide = false;
> + if (ret == PERF_TARGET__SUCCESS)
> + ret = PERF_TARGET__UID_OVERRIDE_SYSTEM;
> + }
> +
> + return ret;
> }
> diff --git a/tools/perf/util/target.h b/tools/perf/util/target.h
> index 1348065ada5e..c3914c8a9890 100644
> --- a/tools/perf/util/target.h
> +++ b/tools/perf/util/target.h
> @@ -12,6 +12,24 @@ struct perf_target {
> bool system_wide;
> };
>
> -void perf_target__validate(struct perf_target *target);
> +enum perf_target_errno {
> + /*
> + * XXX: Just choose an arbitrary big number standard errno can't have

Here I think its better for us to use _negative_ big numbers, because
according to:

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

<quote>
Issue 6

The following new requirements on POSIX implementations derive from
alignment with the Single UNIX Specification:

The majority of the error conditions previously marked as extensions are
now mandatory, except for the STREAMS-related error conditions.

Values for errno are now required to be distinct positive values rather
than non-zero values. This change is for alignment with the ISO/IEC
9899:1999 standard.
</quote>

So system errno range is all positive, since our error enumeration is a
superset of the system one, using negative values won't ever clash.

Also it would be better to have it as PERF_ERRNO__PID_OVERRIDE_CPU, etc.

Agreed?

Anybody else with reasons not to use this ernno range scheme?

Ingo?

- Arnaldo

> + */
> + __PERF_TARGET__ERRNO_START = 0x10000,
> +
> + PERF_TARGET__SUCCESS = __PERF_TARGET__ERRNO_START,
> +
> + /* for perf_target__validate() */
> + PERF_TARGET__PID_OVERRIDE_CPU,
> + PERF_TARGET__PID_OVERRIDE_UID,
> + PERF_TARGET__UID_OVERRIDE_CPU,
> + PERF_TARGET__PID_OVERRIDE_SYSTEM,
> + PERF_TARGET__UID_OVERRIDE_SYSTEM,
> +
> + __PERF_TARGET__ERRNO_END
> +};
> +
> +enum perf_target_errno perf_target__validate(struct perf_target *target);
>
> #endif /* _PERF_TARGET_H */
> --
> 1.7.10
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Em Fri, Apr 27, 2012 at 09:16:18AM +0900, Namhyung Kim escreveu:
> On Thu, 26 Apr 2012 15:22:55 -0600, David Ahern wrote:
> > On 4/26/12 3:12 PM, Namhyung Kim wrote:
> >> Hmm.. No, I can reproduce it without any of this series. And now I think
> >> that it is not related to the number of cpus. On my 4 core (no
> >> hyperthreading) machine at home, the result was same.
> >>
> >> BTW, did you change sysctl settings?
> >>
> >> $ cat /sys/devices/system/cpu/online
> >> 0-3
> >> $ grep . /proc/sys/kernel/perf_event_*
> >> /proc/sys/kernel/perf_event_max_sample_rate:100000
> >> /proc/sys/kernel/perf_event_mlock_kb:516
> >> /proc/sys/kernel/perf_event_paranoid:1
> >
> > $ grep . /proc/sys/kernel/perf_event_*
> > /proc/sys/kernel/perf_event_max_sample_rate:100000
> > /proc/sys/kernel/perf_event_mlock_kb:516
> > /proc/sys/kernel/perf_event_paranoid:-1
> >
> > That last one is the key. I have it set to not paranoid and usually
> > run perf a non-root user.
> >
>
> That's exactly what I want to see :). On perf_mmap() we have:
>
> if ((locked > lock_limit) && perf_paranoid_tracepoint_raw() &&
> !capable(CAP_IPC_LOCK)) {
> ret = -EPERM;
> goto unlock;
> }
>
> So as long as you set perf_event_paranoid to -1 or run perf test as
> root, you cannot see the failure.

Ok, as root try 'perf top', here I get, with this patch:

[root@sandy ~]# perf top --stdio
Warning:
The sys_perf_event_open() syscall returned with 22 (Invalid argument).
/bin/dmesg may provide additional information.
No CONFIG_PERF_EVENTS=y kernel support configured?
[root@sandy ~]#

Skipping this one, will look again later.

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

2012-05-02 (수), 15:30 -0300, Arnaldo Carvalho de Melo:
> Em Thu, Apr 26, 2012 at 02:15:22PM +0900, Namhyung Kim escreveu:
> > For further work on perf_target, it'd be better off splitting
> > the code into a separate file.
> >
> > Signed-off-by: Namhyung Kim <[email protected]>
> > ---
> > tools/perf/Makefile | 2 ++
> > tools/perf/perf.h | 9 +--------
> > tools/perf/util/evlist.c | 1 +
> > tools/perf/util/evsel.c | 1 +
> > tools/perf/util/target.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > tools/perf/util/target.h | 17 +++++++++++++++++
> > tools/perf/util/usage.c | 34 ----------------------------------
> > tools/perf/util/util.h | 2 --
> > 8 files changed, 67 insertions(+), 44 deletions(-)
> > create mode 100644 tools/perf/util/target.c
> > create mode 100644 tools/perf/util/target.h
>
> Trying to fix this now...
>

Oops, sorry. I'll investigate it tomorrow.

Thanks,
Namhyung


> [acme@sandy linux]$ make -C tools/perf/ O=/home/git/build/perf install
> make: Entering directory `/home/git/linux/tools/perf'
> CC /home/git/build/perf/builtin-bench.o
> CC /home/git/build/perf/bench/sched-messaging.o
> CC /home/git/build/perf/bench/sched-pipe.o
> CC /home/git/build/perf/scripts/perl/Perf-Trace-Util/Context.o
> In file included from
> scripts/perl/Perf-Trace-Util/../../../util/target.h:4,
> from scripts/perl/Perf-Trace-Util/../../../perf.h:210,
> from Context.xs:25:
> scripts/perl/Perf-Trace-Util/../../../util/util.h:44:1: error:
> "HAS_BOOL" redefined
> In file included from /usr/lib64/perl5/CORE/perl.h:2424,
> from Context.xs:23:
> /usr/lib64/perl5/CORE/handy.h:110:1: error: this is the location of the
> previous definition
> In file included from
> scripts/perl/Perf-Trace-Util/../../../util/target.h:4,
> from scripts/perl/Perf-Trace-Util/../../../perf.h:210,
> from Context.xs:25:
> scripts/perl/Perf-Trace-Util/../../../util/util.h:133: error:
> conflicting types for ‘Perl_die_nocontext’
> /usr/lib64/perl5/CORE/proto.h:331: note: previous declaration of
> ‘Perl_die_nocontext’ was here
> make: *** [/home/git/build/perf/scripts/perl/Perf-Trace-Util/Context.o]
> Error 1
> make: Leaving directory `/home/git/linux/tools/perf'
> [acme@sandy linux]$


--
Regards,
Namhyung Kim


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

Hi,

2012-05-02 (수), 15:59 -0300, Arnaldo Carvalho de Melo:
> Em Thu, Apr 26, 2012 at 02:15:23PM +0900, Namhyung Kim escreveu:
> > The perf_target_errno enumerations are used to indicate
> > specific error cases on perf target operations. It'd
> > help libperf being a more generic library.
> >
> > Suggested-by: Arnaldo Carvalho de Melo <[email protected]>
> > Signed-off-by: Namhyung Kim <[email protected]>
> > + /* UID and SYSTEM are mutually exclusive */
> > + if (target->uid_str && target->system_wide) {
> > + target->system_wide = false;
> > + if (ret == PERF_TARGET__SUCCESS)
> > + ret = PERF_TARGET__UID_OVERRIDE_SYSTEM;
> > + }
> > +
> > + return ret;
> > }
> > diff --git a/tools/perf/util/target.h b/tools/perf/util/target.h
> > index 1348065ada5e..c3914c8a9890 100644
> > --- a/tools/perf/util/target.h
> > +++ b/tools/perf/util/target.h
> > @@ -12,6 +12,24 @@ struct perf_target {
> > bool system_wide;
> > };
> >
> > -void perf_target__validate(struct perf_target *target);
> > +enum perf_target_errno {
> > + /*
> > + * XXX: Just choose an arbitrary big number standard errno can't have
>
> Here I think its better for us to use _negative_ big numbers, because
> according to:
>
> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html
>
> <quote>
> Issue 6
>
> The following new requirements on POSIX implementations derive from
> alignment with the Single UNIX Specification:
>
> The majority of the error conditions previously marked as extensions are
> now mandatory, except for the STREAMS-related error conditions.
>
> Values for errno are now required to be distinct positive values rather
> than non-zero values. This change is for alignment with the ISO/IEC
> 9899:1999 standard.
> </quote>
>
> So system errno range is all positive, since our error enumeration is a
> superset of the system one, using negative values won't ever clash.
>
> Also it would be better to have it as PERF_ERRNO__PID_OVERRIDE_CPU, etc.
>
> Agreed?
>

Agreed and thanks for the info. Will change the name of error constants
too.

Thanks,
Namhyung


> Anybody else with reasons not to use this ernno range scheme?
>
> Ingo?
>
> - Arnaldo
>
> > + */
> > + __PERF_TARGET__ERRNO_START = 0x10000,
> > +
> > + PERF_TARGET__SUCCESS = __PERF_TARGET__ERRNO_START,
> > +
> > + /* for perf_target__validate() */
> > + PERF_TARGET__PID_OVERRIDE_CPU,
> > + PERF_TARGET__PID_OVERRIDE_UID,
> > + PERF_TARGET__UID_OVERRIDE_CPU,
> > + PERF_TARGET__PID_OVERRIDE_SYSTEM,
> > + PERF_TARGET__UID_OVERRIDE_SYSTEM,
> > +
> > + __PERF_TARGET__ERRNO_END
> > +};
> > +
> > +enum perf_target_errno perf_target__validate(struct perf_target *target);
> >
> > #endif /* _PERF_TARGET_H */
> > --
> > 1.7.10


--
Regards,
Namhyung Kim


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

Em Thu, May 03, 2012 at 11:39:47PM +0900, Namhyung Kim escreveu:
> 2012-05-02 (수), 15:30 -0300, Arnaldo Carvalho de Melo:
> > Em Thu, Apr 26, 2012 at 02:15:22PM +0900, Namhyung Kim escreveu:
> > > For further work on perf_target, it'd be better off splitting
> > > the code into a separate file.
> > >
> > > Signed-off-by: Namhyung Kim <[email protected]>
> > > ---
> > > tools/perf/Makefile | 2 ++
> > > tools/perf/perf.h | 9 +--------
> > > tools/perf/util/evlist.c | 1 +
> > > tools/perf/util/evsel.c | 1 +
> > > tools/perf/util/target.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > > tools/perf/util/target.h | 17 +++++++++++++++++
> > > tools/perf/util/usage.c | 34 ----------------------------------
> > > tools/perf/util/util.h | 2 --
> > > 8 files changed, 67 insertions(+), 44 deletions(-)
> > > create mode 100644 tools/perf/util/target.c
> > > create mode 100644 tools/perf/util/target.h
> >
> > Trying to fix this now...
> >
>
> Oops, sorry. I'll investigate it tomorrow.
>

I fixed it, its on my latest perf/core pull req to Ingo.

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

2012-05-03 (목), 12:27 -0300, Arnaldo Carvalho de Melo:
> Em Thu, May 03, 2012 at 11:39:47PM +0900, Namhyung Kim escreveu:
> > 2012-05-02 (수), 15:30 -0300, Arnaldo Carvalho de Melo:
> > > Em Thu, Apr 26, 2012 at 02:15:22PM +0900, Namhyung Kim escreveu:
> > > > For further work on perf_target, it'd be better off splitting
> > > > the code into a separate file.
> > > >
> > > > Signed-off-by: Namhyung Kim <[email protected]>
> > > > ---
> > > > tools/perf/Makefile | 2 ++
> > > > tools/perf/perf.h | 9 +--------
> > > > tools/perf/util/evlist.c | 1 +
> > > > tools/perf/util/evsel.c | 1 +
> > > > tools/perf/util/target.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > > > tools/perf/util/target.h | 17 +++++++++++++++++
> > > > tools/perf/util/usage.c | 34 ----------------------------------
> > > > tools/perf/util/util.h | 2 --
> > > > 8 files changed, 67 insertions(+), 44 deletions(-)
> > > > create mode 100644 tools/perf/util/target.c
> > > > create mode 100644 tools/perf/util/target.h
> > >
> > > Trying to fix this now...
> > >
> >
> > Oops, sorry. I'll investigate it tomorrow.
> >
>
> I fixed it, its on my latest perf/core pull req to Ingo.
>

Cool :). Thanks a lot, Arnaldo.

Namhyung


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

On 5/2/12 12:59 PM, Arnaldo Carvalho de Melo wrote:
> Also it would be better to have it as PERF_ERRNO__PID_OVERRIDE_CPU, etc.

I thought you wanted subsystem based errno's (PERF_TARGET__XXXXX) versus
one big set (PERF_ERRNO__XXXXX). Did you change your mind?

David

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