linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).