* [PATCH 0/4] perf record: Make per-cpu mmaps the default.
@ 2013-11-18 9:55 Adrian Hunter
2013-11-18 9:55 ` [PATCH 1/4] " Adrian Hunter
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Adrian Hunter @ 2013-11-18 9:55 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, David Ahern,
Frederic Weisbecker, Jiri Olsa, Mike Galbraith, Namhyung Kim,
Paul Mackerras, Stephane Eranian
Hi
Here are patches to use per-cpu mmaps
by default as requested by Ingo and Peter.
Adrian Hunter (4):
perf record: Make per-cpu mmaps the default.
perf tools: Allow '--inherit' as the negation of '--no-inherit'
perf tools: Add option macro OPT_BOOLEAN_SET
perf record: Default -t option to no inheritance
tools/perf/Documentation/perf-record.txt | 12 +++++++-----
tools/perf/builtin-record.c | 13 +++++++++----
tools/perf/perf.h | 1 +
tools/perf/tests/attr/test-record-no-inherit | 2 +-
tools/perf/util/evlist.c | 6 ++++--
tools/perf/util/evsel.c | 5 +++--
tools/perf/util/parse-options.c | 21 +++++++++++++++++++++
tools/perf/util/parse-options.h | 8 ++++++++
tools/perf/util/target.c | 11 ++++++++++-
tools/perf/util/target.h | 4 +++-
10 files changed, 67 insertions(+), 16 deletions(-)
Regards
Adrian Hunter
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] perf record: Make per-cpu mmaps the default.
2013-11-18 9:55 [PATCH 0/4] perf record: Make per-cpu mmaps the default Adrian Hunter
@ 2013-11-18 9:55 ` Adrian Hunter
2013-11-18 9:55 ` [PATCH 2/4] perf tools: Allow '--inherit' as the negation of '--no-inherit' Adrian Hunter
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Adrian Hunter @ 2013-11-18 9:55 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, David Ahern,
Frederic Weisbecker, Jiri Olsa, Mike Galbraith, Namhyung Kim,
Paul Mackerras, Stephane Eranian
This affects the -p, -t and -u options that
previously defaulted to per-thread mmaps.
A side-effect is that inheritance is
automatically enabled by default. A later
patch will disable it by default for the -t
option.
Consequently add an option to select
per-thread mmaps to support the old
behaviour.
Note that per-thread can be used with a
workload-only (i.e. none of -p, -t, -u,
-a or -C is selected) to get a per-thread
mmap with no inheritance.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Requested-by: Ingo Molnar <mingo@kernel.org>
---
tools/perf/Documentation/perf-record.txt | 10 +++++-----
tools/perf/builtin-record.c | 5 +++--
tools/perf/tests/attr/test-record-no-inherit | 2 +-
tools/perf/util/evlist.c | 6 ++++--
tools/perf/util/evsel.c | 5 +++--
tools/perf/util/target.c | 11 ++++++++++-
tools/perf/util/target.h | 4 +++-
7 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 43b42c4..6ac867e 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -201,11 +201,11 @@ abort events and some memory events in precise mode on modern Intel CPUs.
--transaction::
Record transaction flags for transaction related events.
---force-per-cpu::
-Force the use of per-cpu mmaps. By default, when tasks are specified (i.e. -p,
--t or -u options) per-thread mmaps are created. This option overrides that and
-forces per-cpu mmaps. A side-effect of that is that inheritance is
-automatically enabled. Add the -i option also to disable inheritance.
+--per-thread::
+Use per-thread mmaps. By default per-cpu mmaps are created. This option
+overrides that and uses per-thread mmaps. A side-effect of that is that
+inheritance is automatically disabled. --per-thread is ignored with a warning
+if combined with -a or -C options.
SEE ALSO
--------
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 7c8020a..f5b18b8 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -800,6 +800,7 @@ static struct perf_record record = {
.freq = 4000,
.target = {
.uses_mmap = true,
+ .default_per_cpu = true,
},
},
};
@@ -888,8 +889,8 @@ const struct option record_options[] = {
"sample by weight (on special events only)"),
OPT_BOOLEAN(0, "transaction", &record.opts.sample_transaction,
"sample transaction flags (special events only)"),
- OPT_BOOLEAN(0, "force-per-cpu", &record.opts.target.force_per_cpu,
- "force the use of per-cpu mmaps"),
+ OPT_BOOLEAN(0, "per-thread", &record.opts.target.per_thread,
+ "use per-thread mmaps"),
OPT_END()
};
diff --git a/tools/perf/tests/attr/test-record-no-inherit b/tools/perf/tests/attr/test-record-no-inherit
index 9079a25..44edcb2 100644
--- a/tools/perf/tests/attr/test-record-no-inherit
+++ b/tools/perf/tests/attr/test-record-no-inherit
@@ -3,5 +3,5 @@ command = record
args = -i kill >/dev/null 2>&1
[event:base-record]
-sample_type=259
+sample_type=263
inherit=0
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index bbc746a..76fa764 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -819,8 +819,10 @@ int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)
if (evlist->threads == NULL)
return -1;
- if (target->force_per_cpu)
- evlist->cpus = cpu_map__new(target->cpu_list);
+ if (target->default_per_cpu)
+ evlist->cpus = target->per_thread ?
+ cpu_map__dummy_new() :
+ cpu_map__new(target->cpu_list);
else if (target__has_task(target))
evlist->cpus = cpu_map__dummy_new();
else if (!target__has_cpu(target) && !target->uses_mmap)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 46dd4c2..77e38ff 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -572,6 +572,7 @@ void perf_evsel__config(struct perf_evsel *evsel,
struct perf_evsel *leader = evsel->leader;
struct perf_event_attr *attr = &evsel->attr;
int track = !evsel->idx; /* only the first counter needs these */
+ bool per_cpu = opts->target.default_per_cpu && !opts->target.per_thread;
attr->sample_id_all = perf_missing_features.sample_id_all ? 0 : 1;
attr->inherit = !opts->no_inherit;
@@ -645,7 +646,7 @@ void perf_evsel__config(struct perf_evsel *evsel,
}
}
- if (target__has_cpu(&opts->target) || opts->target.force_per_cpu)
+ if (target__has_cpu(&opts->target))
perf_evsel__set_sample_bit(evsel, CPU);
if (opts->period)
@@ -653,7 +654,7 @@ void perf_evsel__config(struct perf_evsel *evsel,
if (!perf_missing_features.sample_id_all &&
(opts->sample_time || !opts->no_inherit ||
- target__has_cpu(&opts->target) || opts->target.force_per_cpu))
+ target__has_cpu(&opts->target) || per_cpu))
perf_evsel__set_sample_bit(evsel, TIME);
if (opts->raw_samples) {
diff --git a/tools/perf/util/target.c b/tools/perf/util/target.c
index 3c778a0..e74c596 100644
--- a/tools/perf/util/target.c
+++ b/tools/perf/util/target.c
@@ -55,6 +55,13 @@ enum target_errno target__validate(struct target *target)
ret = TARGET_ERRNO__UID_OVERRIDE_SYSTEM;
}
+ /* THREAD and SYSTEM/CPU are mutually exclusive */
+ if (target->per_thread && (target->system_wide || target->cpu_list)) {
+ target->per_thread = false;
+ if (ret == TARGET_ERRNO__SUCCESS)
+ ret = TARGET_ERRNO__SYSTEM_OVERRIDE_THREAD;
+ }
+
return ret;
}
@@ -100,6 +107,7 @@ static const char *target__error_str[] = {
"UID switch overriding CPU",
"PID/TID switch overriding SYSTEM",
"UID switch overriding SYSTEM",
+ "SYSTEM/CPU switch overriding PER-THREAD",
"Invalid User: %s",
"Problems obtaining information for user %s",
};
@@ -131,7 +139,8 @@ int target__strerror(struct target *target, int errnum,
msg = target__error_str[idx];
switch (errnum) {
- case TARGET_ERRNO__PID_OVERRIDE_CPU ... TARGET_ERRNO__UID_OVERRIDE_SYSTEM:
+ case TARGET_ERRNO__PID_OVERRIDE_CPU ...
+ TARGET_ERRNO__SYSTEM_OVERRIDE_THREAD:
snprintf(buf, buflen, "%s", msg);
break;
diff --git a/tools/perf/util/target.h b/tools/perf/util/target.h
index 2d0c506..31dd2e9 100644
--- a/tools/perf/util/target.h
+++ b/tools/perf/util/target.h
@@ -12,7 +12,8 @@ struct target {
uid_t uid;
bool system_wide;
bool uses_mmap;
- bool force_per_cpu;
+ bool default_per_cpu;
+ bool per_thread;
};
enum target_errno {
@@ -33,6 +34,7 @@ enum target_errno {
TARGET_ERRNO__UID_OVERRIDE_CPU,
TARGET_ERRNO__PID_OVERRIDE_SYSTEM,
TARGET_ERRNO__UID_OVERRIDE_SYSTEM,
+ TARGET_ERRNO__SYSTEM_OVERRIDE_THREAD,
/* for target__parse_uid() */
TARGET_ERRNO__INVALID_UID,
--
1.7.11.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/4] perf tools: Allow '--inherit' as the negation of '--no-inherit'
2013-11-18 9:55 [PATCH 0/4] perf record: Make per-cpu mmaps the default Adrian Hunter
2013-11-18 9:55 ` [PATCH 1/4] " Adrian Hunter
@ 2013-11-18 9:55 ` Adrian Hunter
2013-11-18 21:15 ` David Ahern
2013-11-30 12:50 ` [tip:perf/core] " tip-bot for Adrian Hunter
2013-11-18 9:55 ` [PATCH 3/4] perf tools: Add option macro OPT_BOOLEAN_SET Adrian Hunter
2013-11-18 9:55 ` [PATCH 4/4] perf record: Default -t option to no inheritance Adrian Hunter
3 siblings, 2 replies; 12+ messages in thread
From: Adrian Hunter @ 2013-11-18 9:55 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, David Ahern,
Frederic Weisbecker, Jiri Olsa, Mike Galbraith, Namhyung Kim,
Paul Mackerras, Stephane Eranian
Long options can be negated by prefixing them
with 'no-'. However options that already start
with 'no-', such as '--no-inherit' result in ugly
double 'no's. Avoid that by accepting that the
removal of 'no-' also negates the long option.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
---
tools/perf/util/parse-options.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index 31f404a..b6b39ff 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -224,6 +224,24 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
return 0;
}
if (!rest) {
+ if (!prefixcmp(options->long_name, "no-")) {
+ /*
+ * The long name itself starts with "no-", so
+ * accept the option without "no-" so that users
+ * do not have to enter "no-no-" to get the
+ * negation.
+ */
+ rest = skip_prefix(arg, options->long_name + 3);
+ if (rest) {
+ flags |= OPT_UNSET;
+ goto match;
+ }
+ /* Abbreviated case */
+ if (!prefixcmp(options->long_name + 3, arg)) {
+ flags |= OPT_UNSET;
+ goto is_abbreviated;
+ }
+ }
/* abbreviated? */
if (!strncmp(options->long_name, arg, arg_end - arg)) {
is_abbreviated:
@@ -259,6 +277,7 @@ is_abbreviated:
if (!rest)
continue;
}
+match:
if (*rest) {
if (*rest != '=')
continue;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/4] perf tools: Add option macro OPT_BOOLEAN_SET
2013-11-18 9:55 [PATCH 0/4] perf record: Make per-cpu mmaps the default Adrian Hunter
2013-11-18 9:55 ` [PATCH 1/4] " Adrian Hunter
2013-11-18 9:55 ` [PATCH 2/4] perf tools: Allow '--inherit' as the negation of '--no-inherit' Adrian Hunter
@ 2013-11-18 9:55 ` Adrian Hunter
2013-11-30 12:50 ` [tip:perf/core] " tip-bot for Adrian Hunter
2013-11-18 9:55 ` [PATCH 4/4] perf record: Default -t option to no inheritance Adrian Hunter
3 siblings, 1 reply; 12+ messages in thread
From: Adrian Hunter @ 2013-11-18 9:55 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, David Ahern,
Frederic Weisbecker, Jiri Olsa, Mike Galbraith, Namhyung Kim,
Paul Mackerras, Stephane Eranian
OPT_BOOLEAN_SET records whether a boolean
option was set by the user. That information
can be used to change the default value
for the option after the options have been
parsed.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
tools/perf/util/parse-options.c | 2 ++
tools/perf/util/parse-options.h | 8 ++++++++
2 files changed, 10 insertions(+)
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index b6b39ff..d22e3f8 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -78,6 +78,8 @@ static int get_value(struct parse_opt_ctx_t *p,
case OPTION_BOOLEAN:
*(bool *)opt->value = unset ? false : true;
+ if (opt->set)
+ *(bool *)opt->set = true;
return 0;
case OPTION_INCR:
diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
index b0241e2..cbf0149 100644
--- a/tools/perf/util/parse-options.h
+++ b/tools/perf/util/parse-options.h
@@ -82,6 +82,9 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
* OPTION_{BIT,SET_UINT,SET_PTR} store the {mask,integer,pointer} to put in
* the value when met.
* CALLBACKS can use it like they want.
+ *
+ * `set`::
+ * whether an option was set by the user
*/
struct option {
enum parse_opt_type type;
@@ -94,6 +97,7 @@ struct option {
int flags;
parse_opt_cb *callback;
intptr_t defval;
+ bool *set;
};
#define check_vtype(v, type) ( BUILD_BUG_ON_ZERO(!__builtin_types_compatible_p(typeof(v), type)) + v )
@@ -103,6 +107,10 @@ struct option {
#define OPT_GROUP(h) { .type = OPTION_GROUP, .help = (h) }
#define OPT_BIT(s, l, v, h, b) { .type = OPTION_BIT, .short_name = (s), .long_name = (l), .value = check_vtype(v, int *), .help = (h), .defval = (b) }
#define OPT_BOOLEAN(s, l, v, h) { .type = OPTION_BOOLEAN, .short_name = (s), .long_name = (l), .value = check_vtype(v, bool *), .help = (h) }
+#define OPT_BOOLEAN_SET(s, l, v, os, h) \
+ { .type = OPTION_BOOLEAN, .short_name = (s), .long_name = (l), \
+ .value = check_vtype(v, bool *), .help = (h), \
+ .set = check_vtype(os, bool *)}
#define OPT_INCR(s, l, v, h) { .type = OPTION_INCR, .short_name = (s), .long_name = (l), .value = check_vtype(v, int *), .help = (h) }
#define OPT_SET_UINT(s, l, v, h, i) { .type = OPTION_SET_UINT, .short_name = (s), .long_name = (l), .value = check_vtype(v, unsigned int *), .help = (h), .defval = (i) }
#define OPT_SET_PTR(s, l, v, h, p) { .type = OPTION_SET_PTR, .short_name = (s), .long_name = (l), .value = (v), .help = (h), .defval = (p) }
--
1.7.11.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/4] perf record: Default -t option to no inheritance
2013-11-18 9:55 [PATCH 0/4] perf record: Make per-cpu mmaps the default Adrian Hunter
` (2 preceding siblings ...)
2013-11-18 9:55 ` [PATCH 3/4] perf tools: Add option macro OPT_BOOLEAN_SET Adrian Hunter
@ 2013-11-18 9:55 ` Adrian Hunter
2013-11-30 12:51 ` [tip:perf/core] " tip-bot for Adrian Hunter
3 siblings, 1 reply; 12+ messages in thread
From: Adrian Hunter @ 2013-11-18 9:55 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, David Ahern,
Frederic Weisbecker, Jiri Olsa, Mike Galbraith, Namhyung Kim,
Paul Mackerras, Stephane Eranian
The change to per-cpu mmaps causes the -p, -t
and -u options now to have inheritance enabled
by default. Change that back to no inheritance
but for the -t option only.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Requested-by: Peter Zijlstra <peterz@infradead.org>
---
tools/perf/Documentation/perf-record.txt | 2 ++
tools/perf/builtin-record.c | 8 ++++++--
tools/perf/perf.h | 1 +
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 6ac867e..c407897 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -57,6 +57,8 @@ OPTIONS
-t::
--tid=::
Record events on existing thread ID (comma separated list).
+ This option also disables inheritance by default. Enable it by adding
+ --inherit.
-u::
--uid=::
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f5b18b8..65615a8 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -843,8 +843,9 @@ const struct option record_options[] = {
OPT_U64('c', "count", &record.opts.user_interval, "event period to sample"),
OPT_STRING('o', "output", &record.file.path, "file",
"output file name"),
- OPT_BOOLEAN('i', "no-inherit", &record.opts.no_inherit,
- "child tasks do not inherit counters"),
+ OPT_BOOLEAN_SET('i', "no-inherit", &record.opts.no_inherit,
+ &record.opts.no_inherit_set,
+ "child tasks do not inherit counters"),
OPT_UINTEGER('F', "freq", &record.opts.user_freq, "profile at this frequency"),
OPT_CALLBACK('m', "mmap-pages", &record.opts.mmap_pages, "pages",
"number of mmap data pages",
@@ -939,6 +940,9 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
goto out_symbol_exit;
}
+ if (rec->opts.target.tid && !rec->opts.no_inherit_set)
+ rec->opts.no_inherit = true;
+
err = target__validate(&rec->opts.target);
if (err) {
target__strerror(&rec->opts.target, err, errbuf, BUFSIZ);
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index b079304..b23fed5 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -254,6 +254,7 @@ struct perf_record_opts {
bool inherit_stat;
bool no_delay;
bool no_inherit;
+ bool no_inherit_set;
bool no_samples;
bool raw_samples;
bool sample_address;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] perf tools: Allow '--inherit' as the negation of '--no-inherit'
2013-11-18 9:55 ` [PATCH 2/4] perf tools: Allow '--inherit' as the negation of '--no-inherit' Adrian Hunter
@ 2013-11-18 21:15 ` David Ahern
2013-11-19 8:12 ` Adrian Hunter
2013-11-30 12:50 ` [tip:perf/core] " tip-bot for Adrian Hunter
1 sibling, 1 reply; 12+ messages in thread
From: David Ahern @ 2013-11-18 21:15 UTC (permalink / raw)
To: Adrian Hunter, Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Frederic Weisbecker,
Jiri Olsa, Mike Galbraith, Namhyung Kim, Paul Mackerras,
Stephane Eranian
On 11/18/13, 2:55 AM, Adrian Hunter wrote:
> Long options can be negated by prefixing them
> with 'no-'. However options that already start
> with 'no-', such as '--no-inherit' result in ugly
> double 'no's. Avoid that by accepting that the
> removal of 'no-' also negates the long option.
>
Why not cleanup the options for the commands and move all of the no-xxxx
to just xxxx? Anyone using no-xxxx would still just work by the existing
code.
David
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] perf tools: Allow '--inherit' as the negation of '--no-inherit'
2013-11-18 21:15 ` David Ahern
@ 2013-11-19 8:12 ` Adrian Hunter
2013-11-19 16:20 ` David Ahern
0 siblings, 1 reply; 12+ messages in thread
From: Adrian Hunter @ 2013-11-19 8:12 UTC (permalink / raw)
To: David Ahern
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
linux-kernel, Frederic Weisbecker, Jiri Olsa, Mike Galbraith,
Namhyung Kim, Paul Mackerras, Stephane Eranian
On 18/11/13 23:15, David Ahern wrote:
> On 11/18/13, 2:55 AM, Adrian Hunter wrote:
>> Long options can be negated by prefixing them
>> with 'no-'. However options that already start
>> with 'no-', such as '--no-inherit' result in ugly
>> double 'no's. Avoid that by accepting that the
>> removal of 'no-' also negates the long option.
>>
>
> Why not cleanup the options for the commands and move all of the no-xxxx to
> just xxxx? Anyone using no-xxxx would still just work by the existing code.
Interesting idea but the short and long options are a combination, and short
options don't work the same way e.g. -i and --no-inherit go together.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] perf tools: Allow '--inherit' as the negation of '--no-inherit'
2013-11-19 8:12 ` Adrian Hunter
@ 2013-11-19 16:20 ` David Ahern
2013-11-20 7:16 ` Adrian Hunter
0 siblings, 1 reply; 12+ messages in thread
From: David Ahern @ 2013-11-19 16:20 UTC (permalink / raw)
To: Adrian Hunter
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
linux-kernel, Frederic Weisbecker, Jiri Olsa, Mike Galbraith,
Namhyung Kim, Paul Mackerras, Stephane Eranian
On 11/19/13, 1:12 AM, Adrian Hunter wrote:
> On 18/11/13 23:15, David Ahern wrote:
>> Why not cleanup the options for the commands and move all of the no-xxxx to
>> just xxxx? Anyone using no-xxxx would still just work by the existing code.
>
> Interesting idea but the short and long options are a combination, and short
> options don't work the same way e.g. -i and --no-inherit go together.
>
If inherit is on by default and -i / --inherit is the option then -i is
a no-op and --no-inherit disables it. That's the same effect has having
inherit on by default and an option to disable it called --no-inherit.
David
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] perf tools: Allow '--inherit' as the negation of '--no-inherit'
2013-11-19 16:20 ` David Ahern
@ 2013-11-20 7:16 ` Adrian Hunter
0 siblings, 0 replies; 12+ messages in thread
From: Adrian Hunter @ 2013-11-20 7:16 UTC (permalink / raw)
To: David Ahern
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
linux-kernel, Frederic Weisbecker, Jiri Olsa, Mike Galbraith,
Namhyung Kim, Paul Mackerras, Stephane Eranian
On 19/11/13 18:20, David Ahern wrote:
> On 11/19/13, 1:12 AM, Adrian Hunter wrote:
>> On 18/11/13 23:15, David Ahern wrote:
>>> Why not cleanup the options for the commands and move all of the no-xxxx to
>>> just xxxx? Anyone using no-xxxx would still just work by the existing code.
>>
>> Interesting idea but the short and long options are a combination, and short
>> options don't work the same way e.g. -i and --no-inherit go together.
>>
>
> If inherit is on by default and -i / --inherit is the option then -i is a
> no-op and --no-inherit disables it. That's the same effect has having
> inherit on by default and an option to disable it called --no-inherit.
Well, if you are going to change the meaning of -i and deny the users a
short option to disable inheritance, then it is no longer a cleanup - it
is a functional change.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tip:perf/core] perf tools: Allow '--inherit' as the negation of '--no-inherit'
2013-11-18 9:55 ` [PATCH 2/4] perf tools: Allow '--inherit' as the negation of '--no-inherit' Adrian Hunter
2013-11-18 21:15 ` David Ahern
@ 2013-11-30 12:50 ` tip-bot for Adrian Hunter
1 sibling, 0 replies; 12+ messages in thread
From: tip-bot for Adrian Hunter @ 2013-11-30 12:50 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, eranian, mingo, mingo, a.p.zijlstra, efault, peterz, jolsa,
fweisbec, dsahern, tglx, hpa, paulus, linux-kernel, namhyung,
adrian.hunter
Commit-ID: 4bc437964ef540462bd15af4a713da62961809aa
Gitweb: http://git.kernel.org/tip/4bc437964ef540462bd15af4a713da62961809aa
Author: Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Mon, 18 Nov 2013 11:55:55 +0200
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 27 Nov 2013 14:58:36 -0300
perf tools: Allow '--inherit' as the negation of '--no-inherit'
Long options can be negated by prefixing them with 'no-'. However
options that already start with 'no-', such as '--no-inherit' result in
ugly double 'no's.
Avoid that by accepting that the removal of 'no-' also negates the long
option.
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1384768557-23331-3-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/parse-options.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index 31f404a..b6b39ff 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -224,6 +224,24 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
return 0;
}
if (!rest) {
+ if (!prefixcmp(options->long_name, "no-")) {
+ /*
+ * The long name itself starts with "no-", so
+ * accept the option without "no-" so that users
+ * do not have to enter "no-no-" to get the
+ * negation.
+ */
+ rest = skip_prefix(arg, options->long_name + 3);
+ if (rest) {
+ flags |= OPT_UNSET;
+ goto match;
+ }
+ /* Abbreviated case */
+ if (!prefixcmp(options->long_name + 3, arg)) {
+ flags |= OPT_UNSET;
+ goto is_abbreviated;
+ }
+ }
/* abbreviated? */
if (!strncmp(options->long_name, arg, arg_end - arg)) {
is_abbreviated:
@@ -259,6 +277,7 @@ is_abbreviated:
if (!rest)
continue;
}
+match:
if (*rest) {
if (*rest != '=')
continue;
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [tip:perf/core] perf tools: Add option macro OPT_BOOLEAN_SET
2013-11-18 9:55 ` [PATCH 3/4] perf tools: Add option macro OPT_BOOLEAN_SET Adrian Hunter
@ 2013-11-30 12:50 ` tip-bot for Adrian Hunter
0 siblings, 0 replies; 12+ messages in thread
From: tip-bot for Adrian Hunter @ 2013-11-30 12:50 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, linux-kernel, eranian, paulus, mingo, hpa, mingo,
a.p.zijlstra, efault, namhyung, jolsa, fweisbec, adrian.hunter,
dsahern, tglx
Commit-ID: 167faf32b07fc47637048fbcbdfcf4a89481686d
Gitweb: http://git.kernel.org/tip/167faf32b07fc47637048fbcbdfcf4a89481686d
Author: Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Mon, 18 Nov 2013 11:55:56 +0200
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 27 Nov 2013 14:58:36 -0300
perf tools: Add option macro OPT_BOOLEAN_SET
OPT_BOOLEAN_SET records whether a boolean option was set by the user.
That information can be used to change the default value for the option
after the options have been parsed.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1384768557-23331-4-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/parse-options.c | 2 ++
tools/perf/util/parse-options.h | 8 ++++++++
2 files changed, 10 insertions(+)
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index b6b39ff..d22e3f8 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -78,6 +78,8 @@ static int get_value(struct parse_opt_ctx_t *p,
case OPTION_BOOLEAN:
*(bool *)opt->value = unset ? false : true;
+ if (opt->set)
+ *(bool *)opt->set = true;
return 0;
case OPTION_INCR:
diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
index b0241e2..cbf0149 100644
--- a/tools/perf/util/parse-options.h
+++ b/tools/perf/util/parse-options.h
@@ -82,6 +82,9 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
* OPTION_{BIT,SET_UINT,SET_PTR} store the {mask,integer,pointer} to put in
* the value when met.
* CALLBACKS can use it like they want.
+ *
+ * `set`::
+ * whether an option was set by the user
*/
struct option {
enum parse_opt_type type;
@@ -94,6 +97,7 @@ struct option {
int flags;
parse_opt_cb *callback;
intptr_t defval;
+ bool *set;
};
#define check_vtype(v, type) ( BUILD_BUG_ON_ZERO(!__builtin_types_compatible_p(typeof(v), type)) + v )
@@ -103,6 +107,10 @@ struct option {
#define OPT_GROUP(h) { .type = OPTION_GROUP, .help = (h) }
#define OPT_BIT(s, l, v, h, b) { .type = OPTION_BIT, .short_name = (s), .long_name = (l), .value = check_vtype(v, int *), .help = (h), .defval = (b) }
#define OPT_BOOLEAN(s, l, v, h) { .type = OPTION_BOOLEAN, .short_name = (s), .long_name = (l), .value = check_vtype(v, bool *), .help = (h) }
+#define OPT_BOOLEAN_SET(s, l, v, os, h) \
+ { .type = OPTION_BOOLEAN, .short_name = (s), .long_name = (l), \
+ .value = check_vtype(v, bool *), .help = (h), \
+ .set = check_vtype(os, bool *)}
#define OPT_INCR(s, l, v, h) { .type = OPTION_INCR, .short_name = (s), .long_name = (l), .value = check_vtype(v, int *), .help = (h) }
#define OPT_SET_UINT(s, l, v, h, i) { .type = OPTION_SET_UINT, .short_name = (s), .long_name = (l), .value = check_vtype(v, unsigned int *), .help = (h), .defval = (i) }
#define OPT_SET_PTR(s, l, v, h, p) { .type = OPTION_SET_PTR, .short_name = (s), .long_name = (l), .value = (v), .help = (h), .defval = (p) }
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [tip:perf/core] perf record: Default -t option to no inheritance
2013-11-18 9:55 ` [PATCH 4/4] perf record: Default -t option to no inheritance Adrian Hunter
@ 2013-11-30 12:51 ` tip-bot for Adrian Hunter
0 siblings, 0 replies; 12+ messages in thread
From: tip-bot for Adrian Hunter @ 2013-11-30 12:51 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, eranian, mingo, mingo, a.p.zijlstra, efault, peterz, jolsa,
fweisbec, dsahern, tglx, hpa, paulus, linux-kernel, namhyung,
adrian.hunter
Commit-ID: 69e7e5b02bc6a9e5cf4a54911b27ca133cc1f99f
Gitweb: http://git.kernel.org/tip/69e7e5b02bc6a9e5cf4a54911b27ca133cc1f99f
Author: Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Mon, 18 Nov 2013 11:55:57 +0200
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 27 Nov 2013 14:58:36 -0300
perf record: Default -t option to no inheritance
The change to per-cpu mmaps causes the -p, -t and -u options now to have
inheritance enabled by default. Change that back to no inheritance but
for the -t option only.
Requested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1384768557-23331-5-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/Documentation/perf-record.txt | 2 ++
tools/perf/builtin-record.c | 8 ++++++--
tools/perf/perf.h | 1 +
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 6ac867e..c407897 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -57,6 +57,8 @@ OPTIONS
-t::
--tid=::
Record events on existing thread ID (comma separated list).
+ This option also disables inheritance by default. Enable it by adding
+ --inherit.
-u::
--uid=::
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f5b18b8..65615a8 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -843,8 +843,9 @@ const struct option record_options[] = {
OPT_U64('c', "count", &record.opts.user_interval, "event period to sample"),
OPT_STRING('o', "output", &record.file.path, "file",
"output file name"),
- OPT_BOOLEAN('i', "no-inherit", &record.opts.no_inherit,
- "child tasks do not inherit counters"),
+ OPT_BOOLEAN_SET('i', "no-inherit", &record.opts.no_inherit,
+ &record.opts.no_inherit_set,
+ "child tasks do not inherit counters"),
OPT_UINTEGER('F', "freq", &record.opts.user_freq, "profile at this frequency"),
OPT_CALLBACK('m', "mmap-pages", &record.opts.mmap_pages, "pages",
"number of mmap data pages",
@@ -939,6 +940,9 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
goto out_symbol_exit;
}
+ if (rec->opts.target.tid && !rec->opts.no_inherit_set)
+ rec->opts.no_inherit = true;
+
err = target__validate(&rec->opts.target);
if (err) {
target__strerror(&rec->opts.target, err, errbuf, BUFSIZ);
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index b079304..b23fed5 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -254,6 +254,7 @@ struct perf_record_opts {
bool inherit_stat;
bool no_delay;
bool no_inherit;
+ bool no_inherit_set;
bool no_samples;
bool raw_samples;
bool sample_address;
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-11-30 12:56 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-18 9:55 [PATCH 0/4] perf record: Make per-cpu mmaps the default Adrian Hunter
2013-11-18 9:55 ` [PATCH 1/4] " Adrian Hunter
2013-11-18 9:55 ` [PATCH 2/4] perf tools: Allow '--inherit' as the negation of '--no-inherit' Adrian Hunter
2013-11-18 21:15 ` David Ahern
2013-11-19 8:12 ` Adrian Hunter
2013-11-19 16:20 ` David Ahern
2013-11-20 7:16 ` Adrian Hunter
2013-11-30 12:50 ` [tip:perf/core] " tip-bot for Adrian Hunter
2013-11-18 9:55 ` [PATCH 3/4] perf tools: Add option macro OPT_BOOLEAN_SET Adrian Hunter
2013-11-30 12:50 ` [tip:perf/core] " tip-bot for Adrian Hunter
2013-11-18 9:55 ` [PATCH 4/4] perf record: Default -t option to no inheritance Adrian Hunter
2013-11-30 12:51 ` [tip:perf/core] " tip-bot for Adrian Hunter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).