Skip to contentSkip to conversation

lock issue in perf cgroup with jump label enabled

BUG Li Zefan 2 participants 4 messages Mar 3, 2011 – Mar 4, 2011

lore
Share & tools
lore

Li Zefan

author
#0 lore raw

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..

#1 lore raw
Quoted 28 lines · Li Zefan

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:

(quoted diff) +10 -0
@@ -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);
+       }
Quoted 2 lines

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(-)
kernel/perf_event.c +11 -9
index 64a018e..6bc69a4 100644
@@ -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;
#2 lore raw
Quoted 4 lines · Peter Zijlstra
Quoted 3 lines

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(-)
kernel/perf_event.c +13 -13
Index: 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);
 	}
 
 	/*
author
#3 lore raw
Quoted 9 lines · Peter Zijlstra
Quoted 4 lines · Peter Zijlstra
Quoted 3 lines

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!