* [PATCH 1/5] perf tool: Fix attributes for '{}' defined event groups
2012-11-12 17:33 [PATCH 0/5] perf tool: Fix enable/disable events logic for record command Jiri Olsa
@ 2012-11-12 17:34 ` Jiri Olsa
2012-12-08 14:56 ` [tip:perf/core] perf tools: " tip-bot for Jiri Olsa
2012-11-12 17:34 ` [PATCH 2/5] perf tool: Fix 'disabled' attribute config for record command Jiri Olsa
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2012-11-12 17:34 UTC (permalink / raw)
To: linux-kernel
Cc: Jiri Olsa, Corey Ashford, Frederic Weisbecker, Ingo Molnar,
Paul Mackerras, Peter Zijlstra, Arnaldo Carvalho de Melo
Fixing events attributes for groups defined via '{}'.
Currently 'enable_on_exec' attribute in record command and both
'disabled ' and 'enable_on_exec' attributes in stat command are
set based on the 'group' option. This eliminates proper setup
for '{}' defined groups as they don't set 'group' option.
Making above attributes values based on the 'evsel->leader' as
this is common to both group definition.
Moving perf_evlist__set_leader call within builtin-record ahead
perf_evlist__config_attrs call, because the latter needs possible
group leader links in place.
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-record.c | 8 ++++++--
tools/perf/builtin-stat.c | 11 ++++-------
tools/perf/tests/attr/test-record-group1 | 4 +---
tools/perf/tests/attr/test-stat-group1 | 6 ++----
tools/perf/tests/open-syscall-tp-fields.c | 2 +-
tools/perf/util/evlist.c | 6 ++----
tools/perf/util/evsel.c | 8 +++-----
tools/perf/util/evsel.h | 3 +--
8 files changed, 20 insertions(+), 28 deletions(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 5783c32..3717027 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -230,11 +230,15 @@ static int perf_record__open(struct perf_record *rec)
struct perf_record_opts *opts = &rec->opts;
int rc = 0;
- perf_evlist__config_attrs(evlist, opts);
-
+ /*
+ * Set the evsel leader links before we configure attributes,
+ * since some might depend on this info.
+ */
if (opts->group)
perf_evlist__set_leader(evlist);
+ perf_evlist__config_attrs(evlist, opts);
+
list_for_each_entry(pos, &evlist->entries, node) {
struct perf_event_attr *attr = &pos->attr;
/*
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 6888960..557081e 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -129,8 +129,7 @@ static struct stats runtime_itlb_cache_stats[MAX_NR_CPUS];
static struct stats runtime_dtlb_cache_stats[MAX_NR_CPUS];
static struct stats walltime_nsecs_stats;
-static int create_perf_stat_counter(struct perf_evsel *evsel,
- struct perf_evsel *first)
+static int create_perf_stat_counter(struct perf_evsel *evsel)
{
struct perf_event_attr *attr = &evsel->attr;
bool exclude_guest_missing = false;
@@ -153,7 +152,7 @@ retry:
return 0;
}
- if (!perf_target__has_task(&target) && (!group || evsel == first)) {
+ if (!perf_target__has_task(&target) && (!evsel->leader)) {
attr->disabled = 1;
attr->enable_on_exec = 1;
}
@@ -272,7 +271,7 @@ static int read_counter(struct perf_evsel *counter)
static int __run_perf_stat(int argc __maybe_unused, const char **argv)
{
unsigned long long t0, t1;
- struct perf_evsel *counter, *first;
+ struct perf_evsel *counter;
int status = 0;
int child_ready_pipe[2], go_pipe[2];
const bool forks = (argc > 0);
@@ -332,10 +331,8 @@ static int __run_perf_stat(int argc __maybe_unused, const char **argv)
if (group)
perf_evlist__set_leader(evsel_list);
- first = perf_evlist__first(evsel_list);
-
list_for_each_entry(counter, &evsel_list->entries, node) {
- if (create_perf_stat_counter(counter, first) < 0) {
+ if (create_perf_stat_counter(counter) < 0) {
/*
* PPC returns ENXIO for HW counters until 2.6.37
* (behavior changed with commit b0a873e).
diff --git a/tools/perf/tests/attr/test-record-group1 b/tools/perf/tests/attr/test-record-group1
index 39bf860..013572f 100644
--- a/tools/perf/tests/attr/test-record-group1
+++ b/tools/perf/tests/attr/test-record-group1
@@ -15,6 +15,4 @@ config=1
sample_type=327
mmap=0
comm=0
-# TODO this is disabled for --group option, enabled otherwise
-# check why..
-enable_on_exec=1
+enable_on_exec=0
diff --git a/tools/perf/tests/attr/test-stat-group1 b/tools/perf/tests/attr/test-stat-group1
index 5ae2718..2a1f86e 100644
--- a/tools/perf/tests/attr/test-stat-group1
+++ b/tools/perf/tests/attr/test-stat-group1
@@ -11,7 +11,5 @@ group_fd=-1
fd=2
group_fd=1
config=1
-# TODO both disabled and enable_on_exec are disabled for --group option,
-# enabled otherwise, check why..
-disabled=1
-enable_on_exec=1
+disabled=0
+enable_on_exec=0
diff --git a/tools/perf/tests/open-syscall-tp-fields.c b/tools/perf/tests/open-syscall-tp-fields.c
index b05b6a6..1c52fdc 100644
--- a/tools/perf/tests/open-syscall-tp-fields.c
+++ b/tools/perf/tests/open-syscall-tp-fields.c
@@ -41,7 +41,7 @@ int test__syscall_open_tp_fields(void)
goto out_delete_evlist;
}
- perf_evsel__config(evsel, &opts, evsel);
+ perf_evsel__config(evsel, &opts);
evlist->threads->map[0] = getpid();
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index a41dc4a..04acae0 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -52,15 +52,13 @@ struct perf_evlist *perf_evlist__new(struct cpu_map *cpus,
void perf_evlist__config_attrs(struct perf_evlist *evlist,
struct perf_record_opts *opts)
{
- struct perf_evsel *evsel, *first;
+ struct perf_evsel *evsel;
if (evlist->cpus->map[0] < 0)
opts->no_inherit = true;
- first = perf_evlist__first(evlist);
-
list_for_each_entry(evsel, &evlist->entries, node) {
- perf_evsel__config(evsel, opts, first);
+ perf_evsel__config(evsel, opts);
if (evlist->nr_entries > 1)
evsel->attr.sample_type |= PERF_SAMPLE_ID;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 618d411..6d4a5f6 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -404,8 +404,8 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
return evsel->name ?: "unknown";
}
-void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
- struct perf_evsel *first)
+void perf_evsel__config(struct perf_evsel *evsel,
+ struct perf_record_opts *opts)
{
struct perf_event_attr *attr = &evsel->attr;
int track = !evsel->idx; /* only the first counter needs these */
@@ -486,10 +486,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
attr->mmap = track;
attr->comm = track;
- if (perf_target__none(&opts->target) &&
- (!opts->group || evsel == first)) {
+ if (perf_target__none(&opts->target) && (!evsel->leader))
attr->enable_on_exec = 1;
- }
}
int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 6f94d6d..32d7ec7 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -92,8 +92,7 @@ void perf_evsel__exit(struct perf_evsel *evsel);
void perf_evsel__delete(struct perf_evsel *evsel);
void perf_evsel__config(struct perf_evsel *evsel,
- struct perf_record_opts *opts,
- struct perf_evsel *first);
+ struct perf_record_opts *opts);
bool perf_evsel__is_cache_op_valid(u8 type, u8 op);
--
1.7.11.7
^ permalink raw reply [flat|nested] 14+ messages in thread* [tip:perf/core] perf tools: Fix attributes for '{}' defined event groups
2012-11-12 17:34 ` [PATCH 1/5] perf tool: Fix attributes for '{}' defined event groups Jiri Olsa
@ 2012-12-08 14:56 ` tip-bot for Jiri Olsa
0 siblings, 0 replies; 14+ messages in thread
From: tip-bot for Jiri Olsa @ 2012-12-08 14:56 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, linux-kernel, paulus, hpa, mingo, a.p.zijlstra, namhyung,
jolsa, fweisbec, tglx, cjashfor, mingo
Commit-ID: cac21425578abddc4e9f529845832a57ba27ce0f
Gitweb: http://git.kernel.org/tip/cac21425578abddc4e9f529845832a57ba27ce0f
Author: Jiri Olsa <jolsa@redhat.com>
AuthorDate: Mon, 12 Nov 2012 18:34:00 +0100
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 14 Nov 2012 16:51:50 -0300
perf tools: Fix attributes for '{}' defined event groups
Fixing events attributes for groups defined via '{}'.
Currently 'enable_on_exec' attribute in record command and both
'disabled ' and 'enable_on_exec' attributes in stat command are set
based on the 'group' option. This eliminates proper setup for '{}'
defined groups as they don't set 'group' option.
Making above attributes values based on the 'evsel->leader' as this is
common to both group definition.
Moving perf_evlist__set_leader call within builtin-record ahead
perf_evlist__config_attrs call, because the latter needs possible group
leader links in place.
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1352741644-16809-2-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-record.c | 8 ++++++--
tools/perf/builtin-stat.c | 11 ++++-------
tools/perf/tests/attr/test-record-group1 | 4 +---
tools/perf/tests/attr/test-stat-group1 | 6 ++----
tools/perf/tests/open-syscall-tp-fields.c | 2 +-
tools/perf/util/evlist.c | 6 ++----
tools/perf/util/evsel.c | 8 +++-----
tools/perf/util/evsel.h | 3 +--
8 files changed, 20 insertions(+), 28 deletions(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 5783c32..3717027 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -230,11 +230,15 @@ static int perf_record__open(struct perf_record *rec)
struct perf_record_opts *opts = &rec->opts;
int rc = 0;
- perf_evlist__config_attrs(evlist, opts);
-
+ /*
+ * Set the evsel leader links before we configure attributes,
+ * since some might depend on this info.
+ */
if (opts->group)
perf_evlist__set_leader(evlist);
+ perf_evlist__config_attrs(evlist, opts);
+
list_for_each_entry(pos, &evlist->entries, node) {
struct perf_event_attr *attr = &pos->attr;
/*
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 6888960..557081e 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -129,8 +129,7 @@ static struct stats runtime_itlb_cache_stats[MAX_NR_CPUS];
static struct stats runtime_dtlb_cache_stats[MAX_NR_CPUS];
static struct stats walltime_nsecs_stats;
-static int create_perf_stat_counter(struct perf_evsel *evsel,
- struct perf_evsel *first)
+static int create_perf_stat_counter(struct perf_evsel *evsel)
{
struct perf_event_attr *attr = &evsel->attr;
bool exclude_guest_missing = false;
@@ -153,7 +152,7 @@ retry:
return 0;
}
- if (!perf_target__has_task(&target) && (!group || evsel == first)) {
+ if (!perf_target__has_task(&target) && (!evsel->leader)) {
attr->disabled = 1;
attr->enable_on_exec = 1;
}
@@ -272,7 +271,7 @@ static int read_counter(struct perf_evsel *counter)
static int __run_perf_stat(int argc __maybe_unused, const char **argv)
{
unsigned long long t0, t1;
- struct perf_evsel *counter, *first;
+ struct perf_evsel *counter;
int status = 0;
int child_ready_pipe[2], go_pipe[2];
const bool forks = (argc > 0);
@@ -332,10 +331,8 @@ static int __run_perf_stat(int argc __maybe_unused, const char **argv)
if (group)
perf_evlist__set_leader(evsel_list);
- first = perf_evlist__first(evsel_list);
-
list_for_each_entry(counter, &evsel_list->entries, node) {
- if (create_perf_stat_counter(counter, first) < 0) {
+ if (create_perf_stat_counter(counter) < 0) {
/*
* PPC returns ENXIO for HW counters until 2.6.37
* (behavior changed with commit b0a873e).
diff --git a/tools/perf/tests/attr/test-record-group1 b/tools/perf/tests/attr/test-record-group1
index 39bf860..013572f 100644
--- a/tools/perf/tests/attr/test-record-group1
+++ b/tools/perf/tests/attr/test-record-group1
@@ -15,6 +15,4 @@ config=1
sample_type=327
mmap=0
comm=0
-# TODO this is disabled for --group option, enabled otherwise
-# check why..
-enable_on_exec=1
+enable_on_exec=0
diff --git a/tools/perf/tests/attr/test-stat-group1 b/tools/perf/tests/attr/test-stat-group1
index 5ae2718..2a1f86e 100644
--- a/tools/perf/tests/attr/test-stat-group1
+++ b/tools/perf/tests/attr/test-stat-group1
@@ -11,7 +11,5 @@ group_fd=-1
fd=2
group_fd=1
config=1
-# TODO both disabled and enable_on_exec are disabled for --group option,
-# enabled otherwise, check why..
-disabled=1
-enable_on_exec=1
+disabled=0
+enable_on_exec=0
diff --git a/tools/perf/tests/open-syscall-tp-fields.c b/tools/perf/tests/open-syscall-tp-fields.c
index b05b6a6..1c52fdc 100644
--- a/tools/perf/tests/open-syscall-tp-fields.c
+++ b/tools/perf/tests/open-syscall-tp-fields.c
@@ -41,7 +41,7 @@ int test__syscall_open_tp_fields(void)
goto out_delete_evlist;
}
- perf_evsel__config(evsel, &opts, evsel);
+ perf_evsel__config(evsel, &opts);
evlist->threads->map[0] = getpid();
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index a41dc4a..04acae0 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -52,15 +52,13 @@ struct perf_evlist *perf_evlist__new(struct cpu_map *cpus,
void perf_evlist__config_attrs(struct perf_evlist *evlist,
struct perf_record_opts *opts)
{
- struct perf_evsel *evsel, *first;
+ struct perf_evsel *evsel;
if (evlist->cpus->map[0] < 0)
opts->no_inherit = true;
- first = perf_evlist__first(evlist);
-
list_for_each_entry(evsel, &evlist->entries, node) {
- perf_evsel__config(evsel, opts, first);
+ perf_evsel__config(evsel, opts);
if (evlist->nr_entries > 1)
evsel->attr.sample_type |= PERF_SAMPLE_ID;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 618d411..6d4a5f6 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -404,8 +404,8 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
return evsel->name ?: "unknown";
}
-void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
- struct perf_evsel *first)
+void perf_evsel__config(struct perf_evsel *evsel,
+ struct perf_record_opts *opts)
{
struct perf_event_attr *attr = &evsel->attr;
int track = !evsel->idx; /* only the first counter needs these */
@@ -486,10 +486,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts,
attr->mmap = track;
attr->comm = track;
- if (perf_target__none(&opts->target) &&
- (!opts->group || evsel == first)) {
+ if (perf_target__none(&opts->target) && (!evsel->leader))
attr->enable_on_exec = 1;
- }
}
int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 6f94d6d..32d7ec7 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -92,8 +92,7 @@ void perf_evsel__exit(struct perf_evsel *evsel);
void perf_evsel__delete(struct perf_evsel *evsel);
void perf_evsel__config(struct perf_evsel *evsel,
- struct perf_record_opts *opts,
- struct perf_evsel *first);
+ struct perf_record_opts *opts);
bool perf_evsel__is_cache_op_valid(u8 type, u8 op);
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/5] perf tool: Fix 'disabled' attribute config for record command
2012-11-12 17:33 [PATCH 0/5] perf tool: Fix enable/disable events logic for record command Jiri Olsa
2012-11-12 17:34 ` [PATCH 1/5] perf tool: Fix attributes for '{}' defined event groups Jiri Olsa
@ 2012-11-12 17:34 ` Jiri Olsa
2012-12-08 14:57 ` [tip:perf/core] perf tools: " tip-bot for Jiri Olsa
2012-11-12 17:34 ` [PATCH 3/5] perf tool: Ensure single disable call per event in record comand Jiri Olsa
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2012-11-12 17:34 UTC (permalink / raw)
To: linux-kernel
Cc: Jiri Olsa, Corey Ashford, Frederic Weisbecker, Ingo Molnar,
Paul Mackerras, Peter Zijlstra, Arnaldo Carvalho de Melo
Currently the record command sets all events initially as disabled.
There's non conditional perf_evlist__enable call, that enables
all events before we exec tracee program. That actually screws
whole enable_on_exec logic, because the event is enabled before
the traced program got executed.
What we actually want is:
1) For any type of traced program:
- all independent events and group leaders are disabled
- all group members are enabled
Group members are ruled by group leaders. They need to
be enabled, because the group scheduling relies on that.
2) For traced programs executed by perf:
- all independent events and group leaders have
enable_on_exec set
- we don't specifically enable or disable any event during
the record command
Independent events and group leaders are initially disabled
and get enabled by exec. Group members are ruled by group
leaders as stated in 1).
3) For traced programs attached by perf (pid/tid):
- we specifically enable or disable all events during
the record command
When attaching events to already running traced we
enable/disable events specifically, as there's no
initial traced exec call.
Fixing appropriate perf_event_attr test case to cover this change.
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-record.c | 15 ++++++++++--
tools/perf/tests/attr/test-record-group | 1 +
tools/perf/tests/attr/test-record-group1 | 1 +
tools/perf/util/evsel.c | 42 +++++++++++++++++++++++++++++++-
4 files changed, 56 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 3717027..268b356 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -701,7 +701,13 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
}
}
- perf_evlist__enable(evsel_list);
+ /*
+ * When perf is starting the traced process, all the events
+ * (apart from group members) have enable_on_exec=1 set,
+ * so don't spoil it by prematurely enabling them.
+ */
+ if (!perf_target__none(&opts->target))
+ perf_evlist__enable(evsel_list);
/*
* Let the child rip
@@ -724,7 +730,12 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
waking++;
}
- if (done)
+ /*
+ * When perf is starting the traced process, at the end events
+ * die with the process and we wait for that. Thus no need to
+ * disable events in this case.
+ */
+ if (done && !perf_target__none(&opts->target))
perf_evlist__disable(evsel_list);
}
diff --git a/tools/perf/tests/attr/test-record-group b/tools/perf/tests/attr/test-record-group
index b945f77..a6599e9 100644
--- a/tools/perf/tests/attr/test-record-group
+++ b/tools/perf/tests/attr/test-record-group
@@ -15,3 +15,4 @@ sample_type=327
mmap=0
comm=0
enable_on_exec=0
+disabled=0
diff --git a/tools/perf/tests/attr/test-record-group1 b/tools/perf/tests/attr/test-record-group1
index 013572f..5a8359d 100644
--- a/tools/perf/tests/attr/test-record-group1
+++ b/tools/perf/tests/attr/test-record-group1
@@ -16,3 +16,4 @@ sample_type=327
mmap=0
comm=0
enable_on_exec=0
+disabled=0
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 6d4a5f6..fc4faaa 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -404,13 +404,40 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
return evsel->name ?: "unknown";
}
+/*
+ * The enable_on_exec/disabled value strategy:
+ *
+ * 1) For any type of traced program:
+ * - all independent events and group leaders are disabled
+ * - all group members are enabled
+ *
+ * Group members are ruled by group leaders. They need to
+ * be enabled, because the group scheduling relies on that.
+ *
+ * 2) For traced programs executed by perf:
+ * - all independent events and group leaders have
+ * enable_on_exec set
+ * - we don't specifically enable or disable any event during
+ * the record command
+ *
+ * Independent events and group leaders are initially disabled
+ * and get enabled by exec. Group members are ruled by group
+ * leaders as stated in 1).
+ *
+ * 3) For traced programs attached by perf (pid/tid):
+ * - we specifically enable or disable all events during
+ * the record command
+ *
+ * When attaching events to already running traced we
+ * enable/disable events specifically, as there's no
+ * initial traced exec call.
+ */
void perf_evsel__config(struct perf_evsel *evsel,
struct perf_record_opts *opts)
{
struct perf_event_attr *attr = &evsel->attr;
int track = !evsel->idx; /* only the first counter needs these */
- attr->disabled = 1;
attr->sample_id_all = opts->sample_id_all_missing ? 0 : 1;
attr->inherit = !opts->no_inherit;
attr->read_format = PERF_FORMAT_TOTAL_TIME_ENABLED |
@@ -486,6 +513,19 @@ void perf_evsel__config(struct perf_evsel *evsel,
attr->mmap = track;
attr->comm = track;
+ /*
+ * XXX see the function comment above
+ *
+ * Disabling only independent events or group leaders,
+ * keeping group members enabled.
+ */
+ if (!evsel->leader)
+ attr->disabled = 1;
+
+ /*
+ * Setting enable_on_exec for independent events and
+ * group leaders for traced executed by perf.
+ */
if (perf_target__none(&opts->target) && (!evsel->leader))
attr->enable_on_exec = 1;
}
--
1.7.11.7
^ permalink raw reply [flat|nested] 14+ messages in thread* [tip:perf/core] perf tools: Fix 'disabled' attribute config for record command
2012-11-12 17:34 ` [PATCH 2/5] perf tool: Fix 'disabled' attribute config for record command Jiri Olsa
@ 2012-12-08 14:57 ` tip-bot for Jiri Olsa
0 siblings, 0 replies; 14+ messages in thread
From: tip-bot for Jiri Olsa @ 2012-12-08 14:57 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, linux-kernel, paulus, hpa, mingo, a.p.zijlstra, namhyung,
jolsa, fweisbec, tglx, cjashfor, mingo
Commit-ID: 774cb499ca9ab0e5950a149d1fe102b125da1cee
Gitweb: http://git.kernel.org/tip/774cb499ca9ab0e5950a149d1fe102b125da1cee
Author: Jiri Olsa <jolsa@redhat.com>
AuthorDate: Mon, 12 Nov 2012 18:34:01 +0100
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 14 Nov 2012 16:52:03 -0300
perf tools: Fix 'disabled' attribute config for record command
Currently the record command sets all events initially as disabled.
There's non conditional perf_evlist__enable call, that enables all
events before we exec tracee program. That actually screws whole
enable_on_exec logic, because the event is enabled before the traced
program got executed.
What we actually want is:
1) For any type of traced program:
- all independent events and group leaders are disabled
- all group members are enabled
Group members are ruled by group leaders. They need to
be enabled, because the group scheduling relies on that.
2) For traced programs executed by perf:
- all independent events and group leaders have
enable_on_exec set
- we don't specifically enable or disable any event during
the record command
Independent events and group leaders are initially disabled
and get enabled by exec. Group members are ruled by group
leaders as stated in 1).
3) For traced programs attached by perf (pid/tid):
- we specifically enable or disable all events during
the record command
When attaching events to already running traced we
enable/disable events specifically, as there's no
initial traced exec call.
Fixing appropriate perf_event_attr test case to cover this change.
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1352741644-16809-3-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-record.c | 15 ++++++++++--
tools/perf/tests/attr/test-record-group | 1 +
tools/perf/tests/attr/test-record-group1 | 1 +
tools/perf/util/evsel.c | 42 +++++++++++++++++++++++++++++++-
4 files changed, 56 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 3717027..268b356 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -701,7 +701,13 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
}
}
- perf_evlist__enable(evsel_list);
+ /*
+ * When perf is starting the traced process, all the events
+ * (apart from group members) have enable_on_exec=1 set,
+ * so don't spoil it by prematurely enabling them.
+ */
+ if (!perf_target__none(&opts->target))
+ perf_evlist__enable(evsel_list);
/*
* Let the child rip
@@ -724,7 +730,12 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
waking++;
}
- if (done)
+ /*
+ * When perf is starting the traced process, at the end events
+ * die with the process and we wait for that. Thus no need to
+ * disable events in this case.
+ */
+ if (done && !perf_target__none(&opts->target))
perf_evlist__disable(evsel_list);
}
diff --git a/tools/perf/tests/attr/test-record-group b/tools/perf/tests/attr/test-record-group
index b945f77..a6599e9 100644
--- a/tools/perf/tests/attr/test-record-group
+++ b/tools/perf/tests/attr/test-record-group
@@ -15,3 +15,4 @@ sample_type=327
mmap=0
comm=0
enable_on_exec=0
+disabled=0
diff --git a/tools/perf/tests/attr/test-record-group1 b/tools/perf/tests/attr/test-record-group1
index 013572f..5a8359d 100644
--- a/tools/perf/tests/attr/test-record-group1
+++ b/tools/perf/tests/attr/test-record-group1
@@ -16,3 +16,4 @@ sample_type=327
mmap=0
comm=0
enable_on_exec=0
+disabled=0
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 6d4a5f6..fc4faaa 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -404,13 +404,40 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
return evsel->name ?: "unknown";
}
+/*
+ * The enable_on_exec/disabled value strategy:
+ *
+ * 1) For any type of traced program:
+ * - all independent events and group leaders are disabled
+ * - all group members are enabled
+ *
+ * Group members are ruled by group leaders. They need to
+ * be enabled, because the group scheduling relies on that.
+ *
+ * 2) For traced programs executed by perf:
+ * - all independent events and group leaders have
+ * enable_on_exec set
+ * - we don't specifically enable or disable any event during
+ * the record command
+ *
+ * Independent events and group leaders are initially disabled
+ * and get enabled by exec. Group members are ruled by group
+ * leaders as stated in 1).
+ *
+ * 3) For traced programs attached by perf (pid/tid):
+ * - we specifically enable or disable all events during
+ * the record command
+ *
+ * When attaching events to already running traced we
+ * enable/disable events specifically, as there's no
+ * initial traced exec call.
+ */
void perf_evsel__config(struct perf_evsel *evsel,
struct perf_record_opts *opts)
{
struct perf_event_attr *attr = &evsel->attr;
int track = !evsel->idx; /* only the first counter needs these */
- attr->disabled = 1;
attr->sample_id_all = opts->sample_id_all_missing ? 0 : 1;
attr->inherit = !opts->no_inherit;
attr->read_format = PERF_FORMAT_TOTAL_TIME_ENABLED |
@@ -486,6 +513,19 @@ void perf_evsel__config(struct perf_evsel *evsel,
attr->mmap = track;
attr->comm = track;
+ /*
+ * XXX see the function comment above
+ *
+ * Disabling only independent events or group leaders,
+ * keeping group members enabled.
+ */
+ if (!evsel->leader)
+ attr->disabled = 1;
+
+ /*
+ * Setting enable_on_exec for independent events and
+ * group leaders for traced executed by perf.
+ */
if (perf_target__none(&opts->target) && (!evsel->leader))
attr->enable_on_exec = 1;
}
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/5] perf tool: Ensure single disable call per event in record comand
2012-11-12 17:33 [PATCH 0/5] perf tool: Fix enable/disable events logic for record command Jiri Olsa
2012-11-12 17:34 ` [PATCH 1/5] perf tool: Fix attributes for '{}' defined event groups Jiri Olsa
2012-11-12 17:34 ` [PATCH 2/5] perf tool: Fix 'disabled' attribute config for record command Jiri Olsa
@ 2012-11-12 17:34 ` Jiri Olsa
2012-12-08 14:58 ` [tip:perf/core] perf tools: " tip-bot for Jiri Olsa
2012-11-12 17:34 ` [PATCH 4/5] perf tool: Omit group members from perf_evlist__disable/enable Jiri Olsa
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2012-11-12 17:34 UTC (permalink / raw)
To: linux-kernel
Cc: Jiri Olsa, Corey Ashford, Frederic Weisbecker, Ingo Molnar,
Paul Mackerras, Peter Zijlstra, Arnaldo Carvalho de Melo
It's possible we issue the event disable ioctl multiple times
until we read the final portion of the mmap buffer.
Ensuring just single disable ioctl call for event, because
there's no need to do that more than once.
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-record.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 268b356..f3151d3 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -502,6 +502,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
struct perf_evlist *evsel_list = rec->evlist;
const char *output_name = rec->output_name;
struct perf_session *session;
+ bool disabled = false;
rec->progname = argv[0];
@@ -735,8 +736,10 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
* die with the process and we wait for that. Thus no need to
* disable events in this case.
*/
- if (done && !perf_target__none(&opts->target))
+ if (done && !disabled && !perf_target__none(&opts->target)) {
perf_evlist__disable(evsel_list);
+ disabled = true;
+ }
}
if (quiet || signr == SIGUSR1)
--
1.7.11.7
^ permalink raw reply [flat|nested] 14+ messages in thread* [tip:perf/core] perf tools: Ensure single disable call per event in record comand
2012-11-12 17:34 ` [PATCH 3/5] perf tool: Ensure single disable call per event in record comand Jiri Olsa
@ 2012-12-08 14:58 ` tip-bot for Jiri Olsa
0 siblings, 0 replies; 14+ messages in thread
From: tip-bot for Jiri Olsa @ 2012-12-08 14:58 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, linux-kernel, paulus, hpa, mingo, a.p.zijlstra, namhyung,
jolsa, fweisbec, tglx, cjashfor, mingo
Commit-ID: 2711926a416733b853977a0e014c713955ad0d8a
Gitweb: http://git.kernel.org/tip/2711926a416733b853977a0e014c713955ad0d8a
Author: Jiri Olsa <jolsa@redhat.com>
AuthorDate: Mon, 12 Nov 2012 18:34:02 +0100
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 14 Nov 2012 16:52:09 -0300
perf tools: Ensure single disable call per event in record comand
It's possible we issue the event disable ioctl multiple times until we
read the final portion of the mmap buffer.
Ensuring just single disable ioctl call for event, because there's no
need to do that more than once.
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1352741644-16809-4-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-record.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 268b356..f3151d3 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -502,6 +502,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
struct perf_evlist *evsel_list = rec->evlist;
const char *output_name = rec->output_name;
struct perf_session *session;
+ bool disabled = false;
rec->progname = argv[0];
@@ -735,8 +736,10 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
* die with the process and we wait for that. Thus no need to
* disable events in this case.
*/
- if (done && !perf_target__none(&opts->target))
+ if (done && !disabled && !perf_target__none(&opts->target)) {
perf_evlist__disable(evsel_list);
+ disabled = true;
+ }
}
if (quiet || signr == SIGUSR1)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/5] perf tool: Omit group members from perf_evlist__disable/enable
2012-11-12 17:33 [PATCH 0/5] perf tool: Fix enable/disable events logic for record command Jiri Olsa
` (2 preceding siblings ...)
2012-11-12 17:34 ` [PATCH 3/5] perf tool: Ensure single disable call per event in record comand Jiri Olsa
@ 2012-11-12 17:34 ` Jiri Olsa
2012-12-08 14:59 ` [tip:perf/core] perf tools: " tip-bot for Jiri Olsa
2012-11-12 17:34 ` [PATCH 5/5] perf tool: Add basic event modifier sanity check Jiri Olsa
2012-11-13 2:26 ` [PATCH 0/5] perf tool: Fix enable/disable events logic for record command Namhyung Kim
5 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2012-11-12 17:34 UTC (permalink / raw)
To: linux-kernel
Cc: Jiri Olsa, Corey Ashford, Frederic Weisbecker, Ingo Molnar,
Paul Mackerras, Peter Zijlstra, Arnaldo Carvalho de Melo
There's no need to disable/enable ordinary group member events,
because they are initialy enabled and get scheduled by the leader.
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/evlist.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 04acae0..e9d2d5d 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -222,6 +222,8 @@ void perf_evlist__disable(struct perf_evlist *evlist)
for (cpu = 0; cpu < evlist->cpus->nr; cpu++) {
list_for_each_entry(pos, &evlist->entries, node) {
+ if (pos->leader)
+ continue;
for (thread = 0; thread < evlist->threads->nr; thread++)
ioctl(FD(pos, cpu, thread),
PERF_EVENT_IOC_DISABLE, 0);
@@ -236,6 +238,8 @@ void perf_evlist__enable(struct perf_evlist *evlist)
for (cpu = 0; cpu < cpu_map__nr(evlist->cpus); cpu++) {
list_for_each_entry(pos, &evlist->entries, node) {
+ if (pos->leader)
+ continue;
for (thread = 0; thread < evlist->threads->nr; thread++)
ioctl(FD(pos, cpu, thread),
PERF_EVENT_IOC_ENABLE, 0);
--
1.7.11.7
^ permalink raw reply [flat|nested] 14+ messages in thread* [tip:perf/core] perf tools: Omit group members from perf_evlist__disable/enable
2012-11-12 17:34 ` [PATCH 4/5] perf tool: Omit group members from perf_evlist__disable/enable Jiri Olsa
@ 2012-12-08 14:59 ` tip-bot for Jiri Olsa
0 siblings, 0 replies; 14+ messages in thread
From: tip-bot for Jiri Olsa @ 2012-12-08 14:59 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, linux-kernel, paulus, hpa, mingo, a.p.zijlstra, namhyung,
jolsa, fweisbec, tglx, cjashfor, mingo
Commit-ID: 3fe4430dd66837d8fcdb63c167e908655fc842e3
Gitweb: http://git.kernel.org/tip/3fe4430dd66837d8fcdb63c167e908655fc842e3
Author: Jiri Olsa <jolsa@redhat.com>
AuthorDate: Mon, 12 Nov 2012 18:34:03 +0100
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 14 Nov 2012 16:52:15 -0300
perf tools: Omit group members from perf_evlist__disable/enable
There's no need to disable/enable ordinary group member events,
because they are initialy enabled and get scheduled by the leader.
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1352741644-16809-5-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/evlist.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 04acae0..e9d2d5d 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -222,6 +222,8 @@ void perf_evlist__disable(struct perf_evlist *evlist)
for (cpu = 0; cpu < evlist->cpus->nr; cpu++) {
list_for_each_entry(pos, &evlist->entries, node) {
+ if (pos->leader)
+ continue;
for (thread = 0; thread < evlist->threads->nr; thread++)
ioctl(FD(pos, cpu, thread),
PERF_EVENT_IOC_DISABLE, 0);
@@ -236,6 +238,8 @@ void perf_evlist__enable(struct perf_evlist *evlist)
for (cpu = 0; cpu < cpu_map__nr(evlist->cpus); cpu++) {
list_for_each_entry(pos, &evlist->entries, node) {
+ if (pos->leader)
+ continue;
for (thread = 0; thread < evlist->threads->nr; thread++)
ioctl(FD(pos, cpu, thread),
PERF_EVENT_IOC_ENABLE, 0);
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 5/5] perf tool: Add basic event modifier sanity check
2012-11-12 17:33 [PATCH 0/5] perf tool: Fix enable/disable events logic for record command Jiri Olsa
` (3 preceding siblings ...)
2012-11-12 17:34 ` [PATCH 4/5] perf tool: Omit group members from perf_evlist__disable/enable Jiri Olsa
@ 2012-11-12 17:34 ` Jiri Olsa
2012-11-13 2:22 ` Namhyung Kim
2012-11-13 2:26 ` [PATCH 0/5] perf tool: Fix enable/disable events logic for record command Namhyung Kim
5 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2012-11-12 17:34 UTC (permalink / raw)
To: linux-kernel
Cc: Jiri Olsa, Corey Ashford, Frederic Weisbecker, Ingo Molnar,
Paul Mackerras, Peter Zijlstra, Arnaldo Carvalho de Melo
Updating event parser to allow any non zero string containing
[ukhpGH] characters for event modifier.
The modifier sanity is checked later in parse-event object logic.
The check validates modifier to contain only one instance of any
modifier (apart from 'p') present.
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/parse-events.c | 20 ++++++++++++++++++++
tools/perf/util/parse-events.l | 2 +-
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c0b785b..b321d43 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -722,6 +722,23 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
return 0;
}
+/*
+ * Basic modifier sanity check to validate it contains only one
+ * instance of any modifier (apart from 'p') present.
+ */
+static int check_modifier(char *str)
+{
+ char *p = str;
+
+ while (*p) {
+ if (*p != 'p' && strchr(p + 1, *p))
+ return -1;
+ p++;
+ }
+
+ return 0;
+}
+
int parse_events__modifier_event(struct list_head *list, char *str, bool add)
{
struct perf_evsel *evsel;
@@ -730,6 +747,9 @@ int parse_events__modifier_event(struct list_head *list, char *str, bool add)
if (str == NULL)
return 0;
+ if (check_modifier(str))
+ return -EINVAL;
+
if (!add && get_event_modifier(&mod, str, NULL))
return -EINVAL;
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 66959fa..e9d1134 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -82,7 +82,7 @@ num_hex 0x[a-fA-F0-9]+
num_raw_hex [a-fA-F0-9]+
name [a-zA-Z_*?][a-zA-Z0-9_*?]*
name_minus [a-zA-Z_*?][a-zA-Z0-9\-_*?]*
-modifier_event [ukhpGH]{1,8}
+modifier_event [ukhpGH]+
modifier_bp [rwx]{1,3}
%%
--
1.7.11.7
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 5/5] perf tool: Add basic event modifier sanity check
2012-11-12 17:34 ` [PATCH 5/5] perf tool: Add basic event modifier sanity check Jiri Olsa
@ 2012-11-13 2:22 ` Namhyung Kim
2012-11-13 14:32 ` [PATCHv2 " Jiri Olsa
0 siblings, 1 reply; 14+ messages in thread
From: Namhyung Kim @ 2012-11-13 2:22 UTC (permalink / raw)
To: Jiri Olsa
Cc: linux-kernel, Corey Ashford, Frederic Weisbecker, Ingo Molnar,
Paul Mackerras, Peter Zijlstra, Arnaldo Carvalho de Melo
Hi Jiri,
On Mon, 12 Nov 2012 18:34:04 +0100, Jiri Olsa wrote:
> Updating event parser to allow any non zero string containing
> [ukhpGH] characters for event modifier.
>
> The modifier sanity is checked later in parse-event object logic.
> The check validates modifier to contain only one instance of any
> modifier (apart from 'p') present.
[snip]
> +/*
> + * Basic modifier sanity check to validate it contains only one
> + * instance of any modifier (apart from 'p') present.
> + */
> +static int check_modifier(char *str)
> +{
> + char *p = str;
> +
> + while (*p) {
> + if (*p != 'p' && strchr(p + 1, *p))
> + return -1;
> + p++;
> + }
How about adding a length check too? i.e. something like:
if ((p - str) > strlen("ukhGHppp"))
return -1;
Thanks,
Namhyung
> +
> + return 0;
> +}
> +
> int parse_events__modifier_event(struct list_head *list, char *str, bool add)
> {
> struct perf_evsel *evsel;
> @@ -730,6 +747,9 @@ int parse_events__modifier_event(struct list_head *list, char *str, bool add)
> if (str == NULL)
> return 0;
>
> + if (check_modifier(str))
> + return -EINVAL;
> +
> if (!add && get_event_modifier(&mod, str, NULL))
> return -EINVAL;
>
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 66959fa..e9d1134 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -82,7 +82,7 @@ num_hex 0x[a-fA-F0-9]+
> num_raw_hex [a-fA-F0-9]+
> name [a-zA-Z_*?][a-zA-Z0-9_*?]*
> name_minus [a-zA-Z_*?][a-zA-Z0-9\-_*?]*
> -modifier_event [ukhpGH]{1,8}
> +modifier_event [ukhpGH]+
> modifier_bp [rwx]{1,3}
>
> %%
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCHv2 5/5] perf tool: Add basic event modifier sanity check
2012-11-13 2:22 ` Namhyung Kim
@ 2012-11-13 14:32 ` Jiri Olsa
2012-12-08 15:00 ` [tip:perf/core] perf tools: " tip-bot for Jiri Olsa
0 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2012-11-13 14:32 UTC (permalink / raw)
To: Namhyung Kim
Cc: linux-kernel, Corey Ashford, Frederic Weisbecker, Ingo Molnar,
Paul Mackerras, Peter Zijlstra, Arnaldo Carvalho de Melo
On Tue, Nov 13, 2012 at 11:22:24AM +0900, Namhyung Kim wrote:
> Hi Jiri,
>
> On Mon, 12 Nov 2012 18:34:04 +0100, Jiri Olsa wrote:
> > Updating event parser to allow any non zero string containing
> > [ukhpGH] characters for event modifier.
> >
> > The modifier sanity is checked later in parse-event object logic.
> > The check validates modifier to contain only one instance of any
> > modifier (apart from 'p') present.
> [snip]
> > +/*
> > + * Basic modifier sanity check to validate it contains only one
> > + * instance of any modifier (apart from 'p') present.
> > + */
> > +static int check_modifier(char *str)
> > +{
> > + char *p = str;
> > +
> > + while (*p) {
> > + if (*p != 'p' && strchr(p + 1, *p))
> > + return -1;
> > + p++;
> > + }
>
> How about adding a length check too? i.e. something like:
>
> if ((p - str) > strlen("ukhGHppp"))
> return -1;
ok, v2 attached
thanks,
jirka
---
Updating event parser to allow any non zero string containing
[ukhpGH] characters for event modifier.
The modifier sanity is checked later in parse-event object logic.
The check validates modifier to contain only one instance of any
modifier (apart from 'p') present.
v2:
- added length check suggested Namhyung Kim
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/parse-events.c | 24 ++++++++++++++++++++++++
tools/perf/util/parse-events.l | 2 +-
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c0b785b..020323a 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -722,6 +722,27 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
return 0;
}
+/*
+ * Basic modifier sanity check to validate it contains only one
+ * instance of any modifier (apart from 'p') present.
+ */
+static int check_modifier(char *str)
+{
+ char *p = str;
+
+ /* The sizeof includes 0 byte as well. */
+ if (strlen(str) > (sizeof("ukhGHppp") - 1))
+ return -1;
+
+ while (*p) {
+ if (*p != 'p' && strchr(p + 1, *p))
+ return -1;
+ p++;
+ }
+
+ return 0;
+}
+
int parse_events__modifier_event(struct list_head *list, char *str, bool add)
{
struct perf_evsel *evsel;
@@ -730,6 +751,9 @@ int parse_events__modifier_event(struct list_head *list, char *str, bool add)
if (str == NULL)
return 0;
+ if (check_modifier(str))
+ return -EINVAL;
+
if (!add && get_event_modifier(&mod, str, NULL))
return -EINVAL;
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 66959fa..e9d1134 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -82,7 +82,7 @@ num_hex 0x[a-fA-F0-9]+
num_raw_hex [a-fA-F0-9]+
name [a-zA-Z_*?][a-zA-Z0-9_*?]*
name_minus [a-zA-Z_*?][a-zA-Z0-9\-_*?]*
-modifier_event [ukhpGH]{1,8}
+modifier_event [ukhpGH]+
modifier_bp [rwx]{1,3}
%%
--
1.7.11.7
^ permalink raw reply [flat|nested] 14+ messages in thread* [tip:perf/core] perf tools: Add basic event modifier sanity check
2012-11-13 14:32 ` [PATCHv2 " Jiri Olsa
@ 2012-12-08 15:00 ` tip-bot for Jiri Olsa
0 siblings, 0 replies; 14+ messages in thread
From: tip-bot for Jiri Olsa @ 2012-12-08 15:00 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, linux-kernel, paulus, hpa, mingo, a.p.zijlstra, namhyung,
jolsa, fweisbec, tglx, cjashfor, mingo
Commit-ID: 534123f458f196bcc269f97d20ccf125925c2e7b
Gitweb: http://git.kernel.org/tip/534123f458f196bcc269f97d20ccf125925c2e7b
Author: Jiri Olsa <jolsa@redhat.com>
AuthorDate: Tue, 13 Nov 2012 15:32:58 +0100
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 14 Nov 2012 16:52:24 -0300
perf tools: Add basic event modifier sanity check
Updating event parser to allow any non zero string containing [ukhpGH]
characters for event modifier.
The modifier sanity is checked later in parse-event object logic. The
check validates modifier to contain only one instance of any modifier
(apart from 'p') present.
v2:
- added length check suggested Namhyung Kim
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20121113143258.GA2481@krava.brq.redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/parse-events.c | 24 ++++++++++++++++++++++++
tools/perf/util/parse-events.l | 2 +-
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c0b785b..020323a 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -722,6 +722,27 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
return 0;
}
+/*
+ * Basic modifier sanity check to validate it contains only one
+ * instance of any modifier (apart from 'p') present.
+ */
+static int check_modifier(char *str)
+{
+ char *p = str;
+
+ /* The sizeof includes 0 byte as well. */
+ if (strlen(str) > (sizeof("ukhGHppp") - 1))
+ return -1;
+
+ while (*p) {
+ if (*p != 'p' && strchr(p + 1, *p))
+ return -1;
+ p++;
+ }
+
+ return 0;
+}
+
int parse_events__modifier_event(struct list_head *list, char *str, bool add)
{
struct perf_evsel *evsel;
@@ -730,6 +751,9 @@ int parse_events__modifier_event(struct list_head *list, char *str, bool add)
if (str == NULL)
return 0;
+ if (check_modifier(str))
+ return -EINVAL;
+
if (!add && get_event_modifier(&mod, str, NULL))
return -EINVAL;
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 66959fa..e9d1134 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -82,7 +82,7 @@ num_hex 0x[a-fA-F0-9]+
num_raw_hex [a-fA-F0-9]+
name [a-zA-Z_*?][a-zA-Z0-9_*?]*
name_minus [a-zA-Z_*?][a-zA-Z0-9\-_*?]*
-modifier_event [ukhpGH]{1,8}
+modifier_event [ukhpGH]+
modifier_bp [rwx]{1,3}
%%
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/5] perf tool: Fix enable/disable events logic for record command
2012-11-12 17:33 [PATCH 0/5] perf tool: Fix enable/disable events logic for record command Jiri Olsa
` (4 preceding siblings ...)
2012-11-12 17:34 ` [PATCH 5/5] perf tool: Add basic event modifier sanity check Jiri Olsa
@ 2012-11-13 2:26 ` Namhyung Kim
5 siblings, 0 replies; 14+ messages in thread
From: Namhyung Kim @ 2012-11-13 2:26 UTC (permalink / raw)
To: Jiri Olsa
Cc: linux-kernel, Corey Ashford, Frederic Weisbecker, Ingo Molnar,
Paul Mackerras, Peter Zijlstra, Arnaldo Carvalho de Melo
On Mon, 12 Nov 2012 18:33:59 +0100, Jiri Olsa wrote:
> hi,
> this patchset fixes the event enable/disable logic for record command
> (patches 1 - 4) plus small fix for event parsing (patch 5).
>
> Attached patches:
> 1/5 perf tool: Fix attributes for '{}' defined event groups
> 2/5 perf tool: Fix 'disabled' attribute config for record command
> 3/5 perf tool: Ensure single disable call per event in record comand
> 4/5 perf tool: Omit group members from perf_evlist__disable/enable
> 5/5 perf tool: Add basic event modifier sanity check
>
> Also available in:
> git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/linux.git
> perf/enable5
>
> thanks,
> jirka
>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
I commented on the patch 5/5, anyway looks good to me:
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 14+ messages in thread