* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.