[PATCH 00/15] perf_event patches
Peter Zijlstra
There's still one issue open with that and that is that we're reading
->total_time_{enabled,running} without serialization.
Please have a thorough look at 7-14.
It also removes all the system_state == STATE_RUNNING muck from
everywhere and it still boots, which leaves me wondering what changed to
not make things go boom.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
tip-bot for Peter Zijlstra
Gitweb: http://git.kernel.org/tip/0cff784ae41cc125368ae77f1c01328ae2fdc6b3
Author: Peter Zijlstra <a.p.zi...@chello.nl>
AuthorDate: Fri, 20 Nov 2009 22:19:44 +0100
Committer: Ingo Molnar <mi...@elte.hu>
CommitDate: Sat, 21 Nov 2009 14:11:35 +0100
perf: Optimize some swcounter attr.sample_period==1 paths
Avoid the rather expensive perf_swevent_set_period() if we know
we have to sample every single event anyway.
Signed-off-by: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Paul Mackerras <pau...@samba.org>
LKML-Reference: <200911202125...@chello.nl>
Signed-off-by: Ingo Molnar <mi...@elte.hu>
---
kernel/perf_event.c | 17 +++++++++++------
1 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 1dfb6cc..8e55b44 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -3759,16 +3759,16 @@ again:
return nr;
}
-static void perf_swevent_overflow(struct perf_event *event,
+static void perf_swevent_overflow(struct perf_event *event, u64 overflow,
int nmi, struct perf_sample_data *data,
struct pt_regs *regs)
{
struct hw_perf_event *hwc = &event->hw;
int throttle = 0;
- u64 overflow;
data->period = event->hw.last_period;
- overflow = perf_swevent_set_period(event);
+ if (!overflow)
+ overflow = perf_swevent_set_period(event);
if (hwc->interrupts == MAX_INTERRUPTS)
return;
@@ -3801,14 +3801,19 @@ static void perf_swevent_add(struct perf_event *event, u64 nr,
atomic64_add(nr, &event->count);
+ if (!regs)
+ return;
+
if (!hwc->sample_period)
return;
- if (!regs)
+ if (nr == 1 && hwc->sample_period == 1 && !event->attr.freq)
+ return perf_swevent_overflow(event, 1, nmi, data, regs);
+
+ if (atomic64_add_negative(nr, &hwc->period_left))
return;
- if (!atomic64_add_negative(nr, &hwc->period_left))
- perf_swevent_overflow(event, nmi, data, regs);
+ perf_swevent_overflow(event, 0, nmi, data, regs);
}
static int perf_swevent_is_counting(struct perf_event *event)
tip-bot for Peter Zijlstra
Gitweb: http://git.kernel.org/tip/81520183878a8813c71c9372de28bb70913ba549
Author: Peter Zijlstra <a.p.zi...@chello.nl>
AuthorDate: Fri, 20 Nov 2009 22:19:45 +0100
perf: Optimize perf_swevent_ctx_event()
Remove a rcu_read_{,un}lock() pair and a few conditionals.
We can remove the rcu_read_lock() by increasing the scope of one
in the calling function.
We can do away with the system_state check if the machine still
boots after this patch (seems to be the case).
We can do away with the list_empty() check because the bare
list_for_each_entry_rcu() reduces to that now that we've removed
everything else.
Signed-off-by: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Paul Mackerras <pau...@samba.org>
LKML-Reference: <200911202125...@chello.nl>
Signed-off-by: Ingo Molnar <mi...@elte.hu>
---
kernel/perf_event.c | 7 +------
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 8e55b44..cda17ac 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -3886,15 +3886,10 @@ static void perf_swevent_ctx_event(struct perf_event_context *ctx,
{
struct perf_event *event;
- if (system_state != SYSTEM_RUNNING || list_empty(&ctx->event_list))
- return;
-
- rcu_read_lock();
list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
if (perf_swevent_match(event, type, event_id, data, regs))
perf_swevent_add(event, nr, nmi, data, regs);
}
- rcu_read_unlock();
}
static int *perf_swevent_recursion_context(struct perf_cpu_context *cpuctx)
@@ -3926,9 +3921,9 @@ static void do_perf_sw_event(enum perf_type_id type, u32 event_id,
(*recursion)++;
barrier();
+ rcu_read_lock();
perf_swevent_ctx_event(&cpuctx->ctx, type, event_id,
nr, nmi, data, regs);
- rcu_read_lock();
/*
* doesn't really matter which of the child contexts the
* events ends up in.
tip-bot for Peter Zijlstra
Gitweb: http://git.kernel.org/tip/f6d9dd237da400effb265f3554c64413f8a3e7b4
Author: Peter Zijlstra <a.p.zi...@chello.nl>
AuthorDate: Fri, 20 Nov 2009 22:19:48 +0100
Committer: Ingo Molnar <mi...@elte.hu>
CommitDate: Sat, 21 Nov 2009 14:11:37 +0100
perf: Optimize perf_event_mmap_ctx()
Remove a rcu_read_{,un}lock() pair and a few conditionals.
We can remove the rcu_read_lock() by increasing the scope of one
in the calling function.
We can do away with the system_state check if the machine still
boots after this patch (seems to be the case).
We can do away with the list_empty() check because the bare
list_for_each_entry_rcu() reduces to that now that we've removed
everything else.
Signed-off-by: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Paul Mackerras <pau...@samba.org>
LKML-Reference: <200911202125...@chello.nl>
Signed-off-by: Ingo Molnar <mi...@elte.hu>
---
kernel/perf_event.c | 7 +------
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 4deefaa..68fbf4f 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -3493,15 +3493,10 @@ static void perf_event_mmap_ctx(struct perf_event_context *ctx,
{
struct perf_event *event;
- if (system_state != SYSTEM_RUNNING || list_empty(&ctx->event_list))
- return;
-
- rcu_read_lock();
list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
if (perf_event_mmap_match(event, mmap_event))
perf_event_mmap_output(event, mmap_event);
}
- rcu_read_unlock();
}
static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
@@ -3557,11 +3552,11 @@ got_name:
mmap_event->event_id.header.size = sizeof(mmap_event->event_id) + size;
+ rcu_read_lock();
cpuctx = &get_cpu_var(perf_cpu_context);
perf_event_mmap_ctx(&cpuctx->ctx, mmap_event);
put_cpu_var(perf_cpu_context);
tip-bot for Peter Zijlstra
Gitweb: http://git.kernel.org/tip/d6ff86cfb50a72df820e7e839836d55d245306fb
Author: Peter Zijlstra <a.p.zi...@chello.nl>
AuthorDate: Fri, 20 Nov 2009 22:19:46 +0100
Committer: Ingo Molnar <mi...@elte.hu>
CommitDate: Sat, 21 Nov 2009 14:11:36 +0100
perf: Optimize perf_event_task_ctx()
Remove a rcu_read_{,un}lock() pair and a few conditionals.
We can remove the rcu_read_lock() by increasing the scope of one
in the calling function.
We can do away with the system_state check if the machine still
boots after this patch (seems to be the case).
We can do away with the list_empty() check because the bare
list_for_each_entry_rcu() reduces to that now that we've removed
everything else.
Signed-off-by: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Paul Mackerras <pau...@samba.org>
LKML-Reference: <200911202125...@chello.nl>
Signed-off-by: Ingo Molnar <mi...@elte.hu>
---
kernel/perf_event.c | 7 +------
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index cda17ac..2afb305 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -3267,15 +3267,10 @@ static void perf_event_task_ctx(struct perf_event_context *ctx,
{
struct perf_event *event;
- if (system_state != SYSTEM_RUNNING || list_empty(&ctx->event_list))
- return;
-
- rcu_read_lock();
list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
if (perf_event_task_match(event))
perf_event_task_output(event, task_event);
}
- rcu_read_unlock();
}
static void perf_event_task_event(struct perf_task_event *task_event)
@@ -3283,11 +3278,11 @@ static void perf_event_task_event(struct perf_task_event *task_event)
struct perf_cpu_context *cpuctx;
struct perf_event_context *ctx = task_event->task_ctx;
+ rcu_read_lock();
cpuctx = &get_cpu_var(perf_cpu_context);
perf_event_task_ctx(&cpuctx->ctx, task_event);
put_cpu_var(perf_cpu_context);
- rcu_read_lock();
if (!ctx)
ctx = rcu_dereference(task_event->task->perf_event_ctxp);
if (ctx)
tip-bot for Peter Zijlstra
Gitweb: http://git.kernel.org/tip/02ffdbc866c8b1c8644601e9aa6155700eed4c91
Author: Peter Zijlstra <a.p.zi...@chello.nl>
AuthorDate: Fri, 20 Nov 2009 22:19:50 +0100
Committer: Ingo Molnar <mi...@elte.hu>
CommitDate: Sat, 21 Nov 2009 14:11:38 +0100
perf: Optimize perf_event_task_sched_out
Remove an update_context_time() call from the
perf_event_task_sched_out() path and into the branch its needed.
The call was both superfluous, because __perf_event_sched_out()
already does it, and wrong, because it was done without holding
ctx->lock.
Place it in perf_event_sync_stat(), which is the only place it
is needed and which does already hold ctx->lock.
Signed-off-by: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Paul Mackerras <pau...@samba.org>
LKML-Reference: <200911202125...@chello.nl>
Signed-off-by: Ingo Molnar <mi...@elte.hu>
---
kernel/perf_event.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 9a18ff2..65f4dab 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -1120,6 +1120,8 @@ static void perf_event_sync_stat(struct perf_event_context *ctx,
if (!ctx->nr_stat)
return;
+ update_context_time(ctx);
+
event = list_first_entry(&ctx->event_list,
struct perf_event, event_entry);
@@ -1163,8 +1165,6 @@ void perf_event_task_sched_out(struct task_struct *task,
if (likely(!ctx || !cpuctx->task_ctx))
return;
- update_context_time(ctx);
-
rcu_read_lock();
parent = rcu_dereference(ctx->parent_ctx);
next_ctx = next->perf_event_ctxp;
tip-bot for Peter Zijlstra
Gitweb: http://git.kernel.org/tip/abf4868b8548cae18d4fe8bbfb4e207443be01be
Author: Peter Zijlstra <a.p.zi...@chello.nl>
AuthorDate: Fri, 20 Nov 2009 22:19:49 +0100
Committer: Ingo Molnar <mi...@elte.hu>
CommitDate: Sat, 21 Nov 2009 14:11:37 +0100
perf: Fix PERF_FORMAT_GROUP scale info
As Corey reported, the total_enabled and total_running times
could occasionally be 0, even though there were events counted.
It turns out this is because we record the times before reading
the counter while the latter updates the times.
This patch corrects that.
While looking at this code I found that there is a lot of
locking iffyness around, the following patches correct most of
that.
Reported-by: Corey Ashford <cjas...@linux.vnet.ibm.com>
Signed-off-by: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Paul Mackerras <pau...@samba.org>
LKML-Reference: <200911202125...@chello.nl>
Signed-off-by: Ingo Molnar <mi...@elte.hu>
---
kernel/perf_event.c | 52 ++++++++++++++++++++------------------------------
1 files changed, 21 insertions(+), 31 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 68fbf4f..9a18ff2 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -1784,30 +1784,15 @@ u64 perf_event_read_value(struct perf_event *event)
}
EXPORT_SYMBOL_GPL(perf_event_read_value);
-static int perf_event_read_entry(struct perf_event *event,
- u64 read_format, char __user *buf)
-{
- int n = 0, count = 0;
- u64 values[2];
-
- values[n++] = perf_event_read_value(event);
- if (read_format & PERF_FORMAT_ID)
- values[n++] = primary_event_id(event);
-
- count = n * sizeof(u64);
-
- if (copy_to_user(buf, values, count))
- return -EFAULT;
-
- return count;
-}
-
static int perf_event_read_group(struct perf_event *event,
u64 read_format, char __user *buf)
{
struct perf_event *leader = event->group_leader, *sub;
- int n = 0, size = 0, err = -EFAULT;
- u64 values[3];
+ int n = 0, size = 0, ret = 0;
+ u64 values[5];
+ u64 count;
+
+ count = perf_event_read_value(leader);
values[n++] = 1 + leader->nr_siblings;
if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED) {
@@ -1818,28 +1803,33 @@ static int perf_event_read_group(struct perf_event *event,
values[n++] = leader->total_time_running +
atomic64_read(&leader->child_total_time_running);
}
+ values[n++] = count;
+ if (read_format & PERF_FORMAT_ID)
+ values[n++] = primary_event_id(leader);
size = n * sizeof(u64);
if (copy_to_user(buf, values, size))
return -EFAULT;
- err = perf_event_read_entry(leader, read_format, buf + size);
- if (err < 0)
- return err;
-
- size += err;
+ ret += size;
list_for_each_entry(sub, &leader->sibling_list, group_entry) {
- err = perf_event_read_entry(sub, read_format,
- buf + size);
- if (err < 0)
- return err;
+ n = 0;
- size += err;
+ values[n++] = perf_event_read_value(sub);
+ if (read_format & PERF_FORMAT_ID)
+ values[n++] = primary_event_id(sub);
+
+ size = n * sizeof(u64);
+
+ if (copy_to_user(buf + size, values, size))
+ return -EFAULT;
+
+ ret += size;
}
- return size;
+ return ret;
}
static int perf_event_read_one(struct perf_event *event,
tip-bot for Peter Zijlstra
Gitweb: http://git.kernel.org/tip/6f10581aeaa5543a3b7a8c7a87a064375ec357f8
Author: Peter Zijlstra <a.p.zi...@chello.nl>
AuthorDate: Fri, 20 Nov 2009 22:19:56 +0100
Committer: Ingo Molnar <mi...@elte.hu>
CommitDate: Sat, 21 Nov 2009 14:11:40 +0100
perf: Fix locking for PERF_FORMAT_GROUP
We should hold event->child_mutex when iterating the inherited
counters, we should hold ctx->mutex when iterating siblings.
Signed-off-by: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Paul Mackerras <pau...@samba.org>
LKML-Reference: <200911202125...@chello.nl>
Signed-off-by: Ingo Molnar <mi...@elte.hu>
---
kernel/perf_event.c | 20 +++++++++++++-------
1 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 80f40da..3ede098 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -1782,6 +1782,7 @@ u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
*enabled = 0;
*running = 0;
+ mutex_lock(&event->child_mutex);
total += perf_event_read(event);
*enabled += event->total_time_enabled +
atomic64_read(&event->child_total_time_enabled);
@@ -1793,6 +1794,7 @@ u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
*enabled += child->total_time_enabled;
*running += child->total_time_running;
}
+ mutex_unlock(&event->child_mutex);
return total;
}
@@ -1802,10 +1804,12 @@ static int perf_event_read_group(struct perf_event *event,
u64 read_format, char __user *buf)
{
struct perf_event *leader = event->group_leader, *sub;
- int n = 0, size = 0, ret = 0;
+ int n = 0, size = 0, ret = -EFAULT;
+ struct perf_event_context *ctx = leader->ctx;
u64 values[5];
u64 count, enabled, running;
+ mutex_lock(&ctx->mutex);
count = perf_event_read_value(leader, &enabled, &running);
values[n++] = 1 + leader->nr_siblings;
@@ -1820,9 +1824,9 @@ static int perf_event_read_group(struct perf_event *event,
size = n * sizeof(u64);
if (copy_to_user(buf, values, size))
- return -EFAULT;
+ goto unlock;
- ret += size;
+ ret = size;
list_for_each_entry(sub, &leader->sibling_list, group_entry) {
n = 0;
@@ -1833,11 +1837,15 @@ static int perf_event_read_group(struct perf_event *event,
size = n * sizeof(u64);
- if (copy_to_user(buf + size, values, size))
- return -EFAULT;
+ if (copy_to_user(buf + size, values, size)) {
+ ret = -EFAULT;
+ goto unlock;
+ }
ret += size;
}
+unlock:
+ mutex_unlock(&ctx->mutex);
return ret;
}
@@ -1884,12 +1892,10 @@ perf_read_hw(struct perf_event *event, char __user *buf, size_t count)
return -ENOSPC;
WARN_ON_ONCE(event->ctx->parent_ctx);
- mutex_lock(&event->child_mutex);
if (read_format & PERF_FORMAT_GROUP)
ret = perf_event_read_group(event, read_format, buf);
else
ret = perf_event_read_one(event, read_format, buf);
- mutex_unlock(&event->child_mutex);
return ret;
tip-bot for Peter Zijlstra
Gitweb: http://git.kernel.org/tip/59ed446f792cc07d37b1536b9c4664d14e25e425
Author: Peter Zijlstra <a.p.zi...@chello.nl>
AuthorDate: Fri, 20 Nov 2009 22:19:55 +0100
perf: Fix event scaling for inherited counters
Properly account the full hierarchy of counters for both the
count (we already did so) and the scale times (new).
Signed-off-by: Peter Zijlstra <a.p.zi...@chello.nl>
Cc: Paul Mackerras <pau...@samba.org>
LKML-Reference: <200911202125...@chello.nl>
Signed-off-by: Ingo Molnar <mi...@elte.hu>
---
include/linux/perf_event.h | 3 +-
kernel/perf_event.c | 48 +++++++++++++++++++++++--------------------
2 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index a430ac3..36fe89f 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -782,7 +782,8 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr,
int cpu,
pid_t pid,
perf_callback_t callback);
-extern u64 perf_event_read_value(struct perf_event *event);
+extern u64 perf_event_read_value(struct perf_event *event,
+ u64 *enabled, u64 *running);
struct perf_sample_data {
u64 type;
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index fdfae88..80f40da 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -1774,14 +1774,25 @@ static int perf_event_read_size(struct perf_event *event)
return size;
}
-u64 perf_event_read_value(struct perf_event *event)
+u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
{
struct perf_event *child;
u64 total = 0;
+ *enabled = 0;
+ *running = 0;
+
total += perf_event_read(event);
- list_for_each_entry(child, &event->child_list, child_list)
+ *enabled += event->total_time_enabled +
+ atomic64_read(&event->child_total_time_enabled);
+ *running += event->total_time_running +
+ atomic64_read(&event->child_total_time_running);
+
+ list_for_each_entry(child, &event->child_list, child_list) {
total += perf_event_read(child);
+ *enabled += child->total_time_enabled;
+ *running += child->total_time_running;
+ }
return total;
}
@@ -1793,19 +1804,15 @@ static int perf_event_read_group(struct perf_event *event,
struct perf_event *leader = event->group_leader, *sub;
int n = 0, size = 0, ret = 0;
u64 values[5];
- u64 count;
+ u64 count, enabled, running;
- count = perf_event_read_value(leader);
+ count = perf_event_read_value(leader, &enabled, &running);
values[n++] = 1 + leader->nr_siblings;
- if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED) {
- values[n++] = leader->total_time_enabled +
- atomic64_read(&leader->child_total_time_enabled);
- }
- if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING) {
- values[n++] = leader->total_time_running +
- atomic64_read(&leader->child_total_time_running);
- }
+ if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
+ values[n++] = enabled;
+ if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
+ values[n++] = running;
values[n++] = count;
if (read_format & PERF_FORMAT_ID)
values[n++] = primary_event_id(leader);
@@ -1820,7 +1827,7 @@ static int perf_event_read_group(struct perf_event *event,
list_for_each_entry(sub, &leader->sibling_list, group_entry) {
n = 0;
- values[n++] = perf_event_read_value(sub);
+ values[n++] = perf_event_read_value(sub, &enabled, &running);
if (read_format & PERF_FORMAT_ID)
values[n++] = primary_event_id(sub);
@@ -1838,18 +1845,15 @@ static int perf_event_read_group(struct perf_event *event,
static int perf_event_read_one(struct perf_event *event,
u64 read_format, char __user *buf)
{
+ u64 enabled, running;
u64 values[4];
int n = 0;
- values[n++] = perf_event_read_value(event);
- if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED) {
- values[n++] = event->total_time_enabled +
- atomic64_read(&event->child_total_time_enabled);
- }
- if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING) {
- values[n++] = event->total_time_running +
- atomic64_read(&event->child_total_time_running);
- }
+ values[n++] = perf_event_read_value(event, &enabled, &running);
+ if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
+ values[n++] = enabled;
+ if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
+ values[n++] = running;
if (read_format & PERF_FORMAT_ID)
values[n++] = primary_event_id(event);
Paul Mackerras
> We can do away with the system_state check if the machine still boots
> after this patch (seems to be the case).
I have a recollection (possible faulty) that the problem we can get
into if we don't have this check is that if we take a bad page fault
in the kernel (e.g. NULL dereference) early in boot before the perf
cpu context has been initialized, we then get another NULL dereference
because the pointers in ctx->event_list are NULL, and recurse to
death.
So that check was possibly more about debugging than correctness.
Possibly also the x86 do_page_fault() is different enough from the
powerpc one that the problem can't occur on x86.
Paul.
Paul Mackerras
> Some perf patches, most resulting from chasing Corey's scale issue.
>
> There's still one issue open with that and that is that we're reading
> ->total_time_{enabled,running} without serialization.
>
> Please have a thorough look at 7-14.
>
> It also removes all the system_state == STATE_RUNNING muck from
> everywhere and it still boots, which leaves me wondering what changed to
> not make things go boom.
Nice series! At a quickish look it all looks fine to me. I'd give
you acked-bys, but Ingo has already committed them all.
Paul.
Peter Zijlstra
> Peter Zijlstra writes:
>
> > We can do away with the system_state check if the machine still boots
> > after this patch (seems to be the case).
>
> I have a recollection (possible faulty) that the problem we can get
> into if we don't have this check is that if we take a bad page fault
> in the kernel (e.g. NULL dereference) early in boot before the perf
> cpu context has been initialized, we then get another NULL dereference
> because the pointers in ctx->event_list are NULL, and recurse to
> death.
>
> So that check was possibly more about debugging than correctness.
> Possibly also the x86 do_page_fault() is different enough from the
> powerpc one that the problem can't occur on x86.
Right, I remembered there was _something_ we added them for, but
couldn't for the live of me remember what.
Hmm, maybe we can initialize all the recursion variables to 1, that
should avoid us ever entering into the swcounter code until we reset
them.
Peter Zijlstra
> On Mon, 2009-11-23 at 16:50 +1100, Paul Mackerras wrote:
> > Peter Zijlstra writes:
> >
> > > We can do away with the system_state check if the machine still boots
> > > after this patch (seems to be the case).
> >
> > I have a recollection (possible faulty) that the problem we can get
> > into if we don't have this check is that if we take a bad page fault
> > in the kernel (e.g. NULL dereference) early in boot before the perf
> > cpu context has been initialized, we then get another NULL dereference
> > because the pointers in ctx->event_list are NULL, and recurse to
> > death.
> >
> > So that check was possibly more about debugging than correctness.
> > Possibly also the x86 do_page_fault() is different enough from the
> > powerpc one that the problem can't occur on x86.
>
> Right, I remembered there was _something_ we added them for, but
> couldn't for the live of me remember what.
>
> Hmm, maybe we can initialize all the recursion variables to 1, that
> should avoid us ever entering into the swcounter code until we reset
> them.
I think the below patch fixed that..
---
commit f29ac756a40d0f1bb07d682ea521e7b666ff06d5
Author: Peter Zijlstra <a.p.zi...@chello.nl>
Date: Fri Jun 19 18:27:26 2009 +0200
perf_counter: Optimize perf_swcounter_event()
Similar to tracepoints, use an enable variable to reduce
overhead when unused.
Only look for a counter of a particular event type when we know
there is at least one in the system.
Signed-off-by: Peter Zijlstra <a.p.zi...@chello.nl>
LKML-Reference: <new-submission>
Cc: Mike Galbraith <efa...@gmx.de>
Cc: Paul Mackerras <pau...@samba.org>
Cc: Arnaldo Carvalho de Melo <ac...@redhat.com>
Signed-off-by: Ingo Molnar <mi...@elte.hu>
diff --git a/include/linux/perf_counter.h b/include/linux/perf_counter.h
index 89698d8..e7213e4 100644
--- a/include/linux/perf_counter.h
+++ b/include/linux/perf_counter.h
@@ -669,7 +669,16 @@ static inline int is_software_counter(struct perf_counter *counter)
(counter->attr.type != PERF_TYPE_HW_CACHE);
}
-extern void perf_swcounter_event(u32, u64, int, struct pt_regs *, u64);
+extern atomic_t perf_swcounter_enabled[PERF_COUNT_SW_MAX];
+
+extern void __perf_swcounter_event(u32, u64, int, struct pt_regs *, u64);
+
+static inline void
+perf_swcounter_event(u32 event, u64 nr, int nmi, struct pt_regs *regs, u64 addr)
+{
+ if (atomic_read(&perf_swcounter_enabled[event]))
+ __perf_swcounter_event(event, nr, nmi, regs, addr);
+}
extern void __perf_counter_mmap(struct vm_area_struct *vma);
diff --git a/kernel/perf_counter.c b/kernel/perf_counter.c
index 1a933a2..7515c76 100644
--- a/kernel/perf_counter.c
+++ b/kernel/perf_counter.c
@@ -3317,8 +3317,8 @@ out:
put_cpu_var(perf_cpu_context);
}
-void
-perf_swcounter_event(u32 event, u64 nr, int nmi, struct pt_regs *regs, u64 addr)
+void __perf_swcounter_event(u32 event, u64 nr, int nmi,
+ struct pt_regs *regs, u64 addr)
{
struct perf_sample_data data = {
.regs = regs,
@@ -3509,9 +3509,19 @@ static const struct pmu *tp_perf_counter_init(struct perf_counter *counter)
}
#endif
+atomic_t perf_swcounter_enabled[PERF_COUNT_SW_MAX];
+
+static void sw_perf_counter_destroy(struct perf_counter *counter)
+{
+ u64 event = counter->attr.config;
+
+ atomic_dec(&perf_swcounter_enabled[event]);
+}
+
static const struct pmu *sw_perf_counter_init(struct perf_counter *counter)
{
const struct pmu *pmu = NULL;
+ u64 event = counter->attr.config;
/*
* Software counters (currently) can't in general distinguish
@@ -3520,7 +3530,7 @@ static const struct pmu *sw_perf_counter_init(struct perf_counter *counter)
* to be kernel events, and page faults are never hypervisor
* events.
*/
- switch (counter->attr.config) {
+ switch (event) {
case PERF_COUNT_SW_CPU_CLOCK:
pmu = &perf_ops_cpu_clock;
@@ -3541,6 +3551,8 @@ static const struct pmu *sw_perf_counter_init(struct perf_counter *counter)
case PERF_COUNT_SW_PAGE_FAULTS_MAJ:
case PERF_COUNT_SW_CONTEXT_SWITCHES:
case PERF_COUNT_SW_CPU_MIGRATIONS:
+ atomic_inc(&perf_swcounter_enabled[event]);
+ counter->destroy = sw_perf_counter_destroy;
pmu = &perf_ops_generic;
break;