With CONFIG_JUMP_LABEL enabled:
# mount -t cgroup -o perf_event xxx /mnt
# mkdir /mnt/test
# ./perf stat -B -a -e cycles:u -G test -- sleep 1
lockdep warning will be triggered immediately, and the machine
will become unresponsive.
I guess this is the culprit?
In the "perf cgroup support" patch:
@@ -419,6 +781,17 @@ list_add_event(struct perf_event *event, struct perf_event_con
list_add_tail(&event->group_entry, list);
}
+ if (is_cgroup_event(event)) {
+ ctx->nr_cgroups++;
+ /*
+ * one more event:
+ * - that has cgroup constraint on event->cpu
+ * - that may need work on context switch
+ */
+ atomic_inc(&per_cpu(perf_cgroup_events, event->cpu));
+ jump_label_inc(&perf_sched_events);
+ }
list_add_event() is called with spin lock ctl->lock held, while
jump_label_inc() will acquire jump_label_mutex..
On Thu, 2011-03-03 at 16:31 +0800, Li Zefan wrote:
> With CONFIG_JUMP_LABEL enabled:
>
> # mount -t cgroup -o perf_event xxx /mnt
> # mkdir /mnt/test
> # ./perf stat -B -a -e cycles:u -G test -- sleep 1
>
> lockdep warning will be triggered immediately, and the machine
> will become unresponsive.
>
> I guess this is the culprit?
>
> In the "perf cgroup support" patch:
>
> @@ -419,6 +781,17 @@ list_add_event(struct perf_event *event, struct perf_event_con
> list_add_tail(&event->group_entry, list);
> }
>
> + if (is_cgroup_event(event)) {
> + ctx->nr_cgroups++;
> + /*
> + * one more event:
> + * - that has cgroup constraint on event->cpu
> + * - that may need work on context switch
> + */
> + atomic_inc(&per_cpu(perf_cgroup_events, event->cpu));
> + jump_label_inc(&perf_sched_events);
> + }
>
> list_add_event() is called with spin lock ctl->lock held, while
> jump_label_inc() will acquire jump_label_mutex..
I think something like the below ought to cure that.
---
kernel/perf_event.c | 20 +++++++++++---------
1 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 64a018e..6bc69a4 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -817,16 +817,8 @@ list_add_event(struct perf_event *event, struct perf_event_context *ctx)
list_add_tail(&event->group_entry, list);
}
- if (is_cgroup_event(event)) {
+ if (is_cgroup_event(event))
ctx->nr_cgroups++;
- /*
- * one more event:
- * - that has cgroup constraint on event->cpu
- * - that may need work on context switch
- */
- atomic_inc(&per_cpu(perf_cgroup_events, event->cpu));
- jump_label_inc(&perf_sched_events);
- }
list_add_rcu(&event->event_entry, &ctx->event_list);
if (!ctx->nr_events)
@@ -6553,6 +6545,16 @@ SYSCALL_DEFINE5(perf_event_open,
goto err_context;
}
+ if (is_cgroup_event(event)) {
+ /*
+ * one more event:
+ * - that has cgroup constraint on event->cpu
+ * - that may need work on context switch
+ */
+ atomic_inc(&per_cpu(perf_cgroup_events, event->cpu));
+ jump_label_inc(&perf_sched_events);
+ }
+
if (move_group) {
struct perf_event_context *gctx = group_leader->ctx;
On Thu, 2011-03-03 at 11:06 +0100, Peter Zijlstra wrote:
>
> > list_add_event() is called with spin lock ctl->lock held, while
> > jump_label_inc() will acquire jump_label_mutex..
>
The below seems to actually work, I totally overlooked the dec case
earlier.
---
Subject: perf: Fix cgroup vs jump_label problem
From: Peter Zijlstra <[email protected]>
Date: Thu Mar 03 11:31:20 CET 2011
Li Zefan reported that the jump label code sleeps and we're calling it
under a spinlock, *fail* ;-)
Reported-by: Li Zefan <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
---
kernel/perf_event.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
Index: linux-2.6/kernel/perf_event.c
===================================================================
--- linux-2.6.orig/kernel/perf_event.c
+++ linux-2.6/kernel/perf_event.c
@@ -820,16 +820,8 @@ list_add_event(struct perf_event *event,
list_add_tail(&event->group_entry, list);
}
- if (is_cgroup_event(event)) {
+ if (is_cgroup_event(event))
ctx->nr_cgroups++;
- /*
- * one more event:
- * - that has cgroup constraint on event->cpu
- * - that may need work on context switch
- */
- atomic_inc(&per_cpu(perf_cgroup_events, event->cpu));
- jump_label_inc(&perf_sched_events);
- }
list_add_rcu(&event->event_entry, &ctx->event_list);
if (!ctx->nr_events)
@@ -957,11 +949,8 @@ list_del_event(struct perf_event *event,
event->attach_state &= ~PERF_ATTACH_CONTEXT;
- if (is_cgroup_event(event)) {
+ if (is_cgroup_event(event))
ctx->nr_cgroups--;
- atomic_dec(&per_cpu(perf_cgroup_events, event->cpu));
- jump_label_dec(&perf_sched_events);
- }
ctx->nr_events--;
if (event->attr.inherit_stat)
@@ -2903,6 +2892,10 @@ static void free_event(struct perf_event
atomic_dec(&nr_task_events);
if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
put_callchain_buffers();
+ if (is_cgroup_event(event)) {
+ atomic_dec(&per_cpu(perf_cgroup_events, event->cpu));
+ jump_label_dec(&perf_sched_events);
+ }
}
if (event->buffer) {
@@ -6478,6 +6471,13 @@ SYSCALL_DEFINE5(perf_event_open,
err = perf_cgroup_connect(pid, event, &attr, group_leader);
if (err)
goto err_alloc;
+ /*
+ * one more event:
+ * - that has cgroup constraint on event->cpu
+ * - that may need work on context switch
+ */
+ atomic_inc(&per_cpu(perf_cgroup_events, event->cpu));
+ jump_label_inc(&perf_sched_events);
}
/*
Peter Zijlstra wrote:
> On Thu, 2011-03-03 at 11:06 +0100, Peter Zijlstra wrote:
>>
>>> list_add_event() is called with spin lock ctl->lock held, while
>>> jump_label_inc() will acquire jump_label_mutex..
>>
>
> The below seems to actually work, I totally overlooked the dec case
> earlier.
>
Yes, it works. Thanks!