perf_event: fix slow and broken cgroup context switch code

PATCH In review Stephane Eranian 2 participants 9 messages Aug 25, 2011 – Aug 29, 2011 perf events

Share & tools

Stephane Eranian

author

2011-08-25 13:58 UTC #0 lore raw

PATCH 1/1 perf_event: fix slow and broken cgroup context switch code

The current cgroup context switch code was incorrect leading to bogus counts. Furthermore, as soon as there was an active cgroup event on a CPU, the context switch cost on that CPU would increase by a significant amount as demonstrated by a simple ping/pong example:

$ ./pong 
Both processes pinned to CPU1, running for 10s
10684.51 ctxsw/s

Now start a cgroup perf stat:

$ perf stat -e cycles,cycles -A -a -G test  -C 1 -- sleep 100

$ ./pong 
Both processes pinned to CPU1, running for 10s
6674.61 ctxsw/s

That's a 37% penalty.

Note that pong is not even in the monitored cgroup.

The results shown by perf stat are bogus:

$ perf stat -e cycles,cycles -A -a -G test  -C 1 -- sleep 100

Performance counter stats for 'sleep 100':

CPU1 <not counted> cycles test
CPU1 16,984,189,138 cycles # 0.000 GHz

The second 'cycles' event should report a count @ CPU clock (here 2.4GHz) as it is counting across all cgroups.

The patch below fixes the bogus accounting and bypasses any cgroup switches in case the outgoing and incoming tasks are in the same cgroup.

With this patch the same test now yields:

$ ./pong
Both processes pinned to CPU1, running for 10s
10775.30 ctxsw/s

Start perf stat with cgroup:

$ perf stat -e cycles,cycles -A -a -G test  -C 1 -- sleep 10

Run pong outside the cgroup:

$ /pong 
Both processes pinned to CPU1, running for 10s
10687.80 ctxsw/s

The penalty is now less than 2%.

And the results for perf stat are correct:

$ perf stat -e cycles,cycles -A -a -G test  -C 1 -- sleep 10

Performance counter stats for 'sleep 10':

CPU1 <not counted> cycles test # 0.000 GHz
CPU1 23,933,981,448 cycles # 0.000 GHz

Now perf stat reports the correct counts for
for the non cgroup event.

If we run pong inside the cgroup, then we also get the correct counts:

$ perf stat -e cycles,cycles -A -a -G test  -C 1 -- sleep 10

Performance counter stats for 'sleep 10':

CPU1 22,297,726,205 cycles test # 0.000 GHz
CPU1 23,933,981,448 cycles # 0.000 GHz

      10.001457237 seconds time elapsed

Signed-off-by Stephane Eranian <[email protected]>

---

include/linux/perf_event.h +15 -9

index 245bafd..c816075 100644
@@ -944,8 +944,10 @@ extern void perf_pmu_unregister(struct pmu *pmu);
 
 extern int perf_num_counters(void);
 extern const char *perf_pmu_name(void);
-extern void __perf_event_task_sched_in(struct task_struct *task);
-extern void __perf_event_task_sched_out(struct task_struct *task, struct task_struct *next);
+extern void __perf_event_task_sched_in(struct task_struct *prev,
+				       struct task_struct *task);
+extern void __perf_event_task_sched_out(struct task_struct *prev,
+					struct task_struct *next);
 extern int perf_event_init_task(struct task_struct *child);
 extern void perf_event_exit_task(struct task_struct *child);
 extern void perf_event_free_task(struct task_struct *task);

@@ -1059,17 +1061,20 @@ perf_sw_event(u32 event_id, u64 nr, struct pt_regs *regs, u64 addr)
 
 extern struct jump_label_key perf_sched_events;
 
-static inline void perf_event_task_sched_in(struct task_struct *task)
+static inline void perf_event_task_sched_in(struct task_struct *prev,
+					    struct task_struct *task)
 {
 	if (static_branch(&perf_sched_events))
-		__perf_event_task_sched_in(task);
+		__perf_event_task_sched_in(prev, task);
 }
 
-static inline void perf_event_task_sched_out(struct task_struct *task, struct task_struct *next)
+static inline void perf_event_task_sched_out(struct task_struct *prev,
+					     struct task_struct *next)
 {
 	perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, NULL, 0);
 
-	__perf_event_task_sched_out(task, next);
+	if (static_branch(&perf_sched_events))
+		__perf_event_task_sched_out(prev, next);
 }
 
 extern void perf_event_mmap(struct vm_area_struct *vma);

@@ -1139,10 +1144,11 @@ extern void perf_event_disable(struct perf_event *event);
 extern void perf_event_task_tick(void);
 #else
 static inline void
-perf_event_task_sched_in(struct task_struct *task)			{ }
+perf_event_task_sched_in(struct task_struct *prev,
+			 struct task_struct *task)			{ }
 static inline void
-perf_event_task_sched_out(struct task_struct *task,
-			    struct task_struct *next)			{ }
+perf_event_task_sched_out(struct task_struct *prev,
+			  struct task_struct *next)			{ }
 static inline int perf_event_init_task(struct task_struct *child)	{ return 0; }
 static inline void perf_event_exit_task(struct task_struct *child)	{ }
 static inline void perf_event_free_task(struct task_struct *task)	{ }

kernel/events/core.c +53 ~~-10~~

106 lines not on this page · raw message

kernel/sched.c +1 -1

index 6baade0..f42fa78 100644
@@ -3206,7 +3206,7 @@ static void finish_task_switch(struct rq *rq, struct task_struct *prev)
 #ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
 	local_irq_disable();
 #endif /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */
-	perf_event_task_sched_in(current);
+	perf_event_task_sched_in(prev, current);
 #ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
 	local_irq_enable();
 #endif /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */

Peter Zijlstra

maintainer

2011-08-25 14:20 UTC #1 lore raw

include/linux/perf_event.h perf_sw_event(u32 event_id, u64 nr, struct pt_regs *regs, u64 addr) · line 1078

Quoted 10 lines · Stephane Eranian

+static inline void perf_event_task_sched_out(struct task_struct *prev,

include/linux/perf_event.h +3 -1

+                                            struct task_struct *next)
 {
        perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, NULL, 0);
 
-       __perf_event_task_sched_out(task, next);
+       if (static_branch(&perf_sched_events))
+               __perf_event_task_sched_out(prev, next);
 }

Right, so the reason we removed the static branch from there is

lkml.kernel.org/r/[email protected]

now I think the series 075e0b0085 to 64ce312618e should have cured that problem, and adding the static_branch() is now safe again. But there's no mention of any of this in the Changelog.

Also, the adding back of the static_branch() is mostly unrelated to the rest of the patch, which I shall now stare at :-)

Stephane Eranian

author

2011-08-25 14:36 UTC #2 lore raw

Quoted 12 lines · Peter Zijlstra

Quoted 10 lines · Stephane Eranian
+static inline void perf_event_task_sched_out(struct task_struct *prev,
+ struct task_struct *next) {
perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, NULL, 0);
- __perf_event_task_sched_out(task, next);
+ if (static_branch(&perf_sched_events))
+ __perf_event_task_sched_out(prev, next); }

Quoted 8 lines

Right, so the reason we removed the static branch from there is
lkml.kernel.org/r/[email protected]
now I think the series 075e0b0085 to 64ce312618e should have cured that problem, and adding the static_branch() is now safe again. But there's no mention of any of this in the Changelog.

I realized I did not talk about the static_branch() change after I had clicked on
Send. But to me, this looks natural to have the static branch in the ctxsw out routine. This has to be symmetrical with ctxsw in . The static branch is about avoiding perf ctxsw when there is no need for it, i.e., no per-thread nor per-cgroup
events.

Peter Zijlstra

maintainer

2011-08-25 14:42 UTC #3 lore raw

Quoted 21 lines · Stephane Eranian

Quoted 12 lines · Peter Zijlstra
include/linux/perf_event.h perf_sw_event(u32 event_id, u64 nr, struct pt_regs *regs, u64 addr) · line 1078
Quoted 10 lines · Stephane Eranian
+static inline void perf_event_task_sched_out(struct task_struct *prev,
include/linux/perf_event.h +3 -1
+                                            struct task_struct *next)
 {
        perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, NULL, 0);
 
-       __perf_event_task_sched_out(task, next);
+       if (static_branch(&perf_sched_events))
+               __perf_event_task_sched_out(prev, next);
 }
Quoted 8 lines
Right, so the reason we removed the static branch from there is
lkml.kernel.org/r/[email protected]
now I think the series 075e0b0085 to 64ce312618e should have cured that problem, and adding the static_branch() is now safe again. But there's no mention of any of this in the Changelog.

Quoted 7 lines

I realized I did not talk about the static_branch() change after I had clicked on
Send. But to me, this looks natural to have the static branch in the ctxsw out routine. This has to be symmetrical with ctxsw in . The static branch is about avoiding perf ctxsw when there is no need for it, i.e., no per-thread nor per-cgroup
events.

Yeah, that argument is what got us into trouble initially :) But I think its ok now, we'll see if stuff explodes or not..

Stephane Eranian

author

2011-08-25 15:03 UTC #4 lore raw

Quoted 30 lines · Peter Zijlstra

Quoted 21 lines · Stephane Eranian
Quoted 12 lines · Peter Zijlstra
Quoted 10 lines · Stephane Eranian
+static inline void perf_event_task_sched_out(struct task_struct *prev,
+ struct task_struct *next) {
perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, NULL, 0);
- __perf_event_task_sched_out(task, next);
+ if (static_branch(&perf_sched_events))
+ __perf_event_task_sched_out(prev, next); }
Quoted 8 lines
Right, so the reason we removed the static branch from there is
lkml.kernel.org/r/[email protected]
now I think the series 075e0b0085 to 64ce312618e should have cured that problem, and adding the static_branch() is now safe again. But there's no mention of any of this in the Changelog.
Quoted 7 lines
I realized I did not talk about the static_branch() change after I had clicked on
Send. But to me, this looks natural to have the static branch in the ctxsw out routine. This has to be symmetrical with ctxsw in . The static branch is about avoiding perf ctxsw when there is no need for it, i.e., no per-thread nor per-cgroup
events.

Quoted 3 lines

Yeah, that argument is what got us into trouble initially :) But I think its ok now, we'll see if stuff explodes or not..

I just ran that crash test you pointed out and so far so good. Don't know if the crash was systematic, though.

Peter Zijlstra

maintainer

2011-08-25 17:34 UTC #5 lore raw

Quoted 2 lines · Stephane Eranian

I just ran that crash test you pointed out and so far so good. Don't know if the crash was systematic, though.

Yeah, it was quite reliable.. but 64ce312618 should really have sorted the problem.

Peter Zijlstra

maintainer

2011-08-25 14:22 UTC #6 lore raw

In reply to Stephane Eranian

Quoted 5 lines · Stephane Eranian

include/linux/perf_event.h +3 -0

+static inline void perf_event_task_sched_in(struct task_struct *prev,
+                                           struct task_struct *task)
 {
        if (static_branch(&perf_sched_events))
+               __perf_event_task_sched_in(prev, task);

include/linux/perf_event.h perf_sw_event(u32 event_id, u64 nr, struct pt_regs *regs, u64 addr) · line 1078

Quoted 10 lines

include/linux/perf_event.h +4 -0

 }
 
+static inline void perf_event_task_sched_out(struct task_struct *prev,
+                                            struct task_struct *next)
 {
        perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, NULL, 0);
 
+       if (static_branch(&perf_sched_events))
+               __perf_event_task_sched_out(prev, next);
 }

BTW, Ingo, is there any particular reason we have these two calls and not a single perf_event_task_switch() call?

Peter Zijlstra

maintainer

2011-08-25 14:26 UTC #7 lore raw

In reply to Stephane Eranian

Quoted 39 lines · Stephane Eranian

kernel/events/core.c +35 -0

+static inline void perf_cgroup_sched_out(struct task_struct *task,
+                                        struct task_struct *next)
 {
+       struct perf_cgroup *cgrp1;
+       struct perf_cgroup *cgrp2 = NULL;
+
+       /*
+        * we come here when we know perf_cgroup_events > 0
+        */
+       cgrp1 = perf_cgroup_from_task(task);
+
+       /*
+        * next is NULL when called from perf_event_enable_on_exec()
+        * that will systematically cause a cgroup_switch()
+        */
+       if (next)
+               cgrp2 = perf_cgroup_from_task(next);
+
+       /*
+        * only schedule out current cgroup events if we know
+        * that we are switching to a different cgroup. Otherwise,
+        * do no touch the cgroup events.
+        */
+       if (cgrp1 != cgrp2)
+               perf_cgroup_switch(task, PERF_CGROUP_SWOUT);
 }
 
+static inline void perf_cgroup_sched_in(struct task_struct *prev,
+                                       struct task_struct *task)
 {
+       struct perf_cgroup *cgrp1;
+       struct perf_cgroup *cgrp2 = NULL;
+
+       /*
+        * we come here when we know perf_cgroup_events > 0
+        */
+       cgrp1 = perf_cgroup_from_task(task);
+
+       /* prev can never be NULL */

kernel/events/core.c void perf_cgroup_switch(struct task_struct *task, int mode) · line 451

Quoted 10 lines

kernel/events/core.c +9 -0

+       cgrp2 = perf_cgroup_from_task(prev);
+
+       /*
+        * only need to schedule in cgroup events if we are changing
+        * cgroup during ctxsw. Cgroup events were not scheduled
+        * out of ctxsw out if that was not the case.
+        */
+       if (cgrp1 != cgrp2)
+               perf_cgroup_switch(task, PERF_CGROUP_SWIN);
 }

OK, looks sane enough, queued the patch, thanks!

tip-bot for Stephane Eranian

author

2011-08-29 14:57 UTC #8 lore raw

[tip:perf/urgent] perf events: Fix slow and broken cgroup context switch code

In reply to Stephane Eranian

Commit-ID:  a8d757ef076f0f95f13a918808824058de25b3eb
Gitweb:     http://git.kernel.org/tip/a8d757ef076f0f95f13a918808824058de25b3eb
Author:     Stephane Eranian <[email protected]>
AuthorDate: Thu, 25 Aug 2011 15:58:03 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 29 Aug 2011 12:28:33 +0200

perf events: Fix slow and broken cgroup context switch code

 $ ./pong
 Both processes pinned to CPU1, running for 10s
 10684.51 ctxsw/s

Now start a cgroup perf stat:

 $ perf stat -e cycles,cycles -A -a -G test  -C 1 -- sleep 100

$ ./pong
 Both processes pinned to CPU1, running for 10s
 6674.61 ctxsw/s

That's a 37% penalty.

Note that pong is not even in the monitored cgroup.

The results shown by perf stat are bogus:

 $ perf stat -e cycles,cycles -A -a -G test  -C 1 -- sleep 100

Performance counter stats for 'sleep 100':

CPU1 <not counted> cycles test
CPU1 16,984,189,138 cycles # 0.000 GHz

The second 'cycles' event should report a count @ CPU clock (here 2.4GHz) as it is counting across all cgroups.

The patch below fixes the bogus accounting and bypasses any cgroup switches in case the outgoing and incoming tasks are in the same cgroup.

With this patch the same test now yields:

 $ ./pong
 Both processes pinned to CPU1, running for 10s
 10775.30 ctxsw/s

Start perf stat with cgroup:

 $ perf stat -e cycles,cycles -A -a -G test  -C 1 -- sleep 10

Run pong outside the cgroup:

 $ /pong
 Both processes pinned to CPU1, running for 10s
 10687.80 ctxsw/s

The penalty is now less than 2%.

And the results for perf stat are correct:

$ perf stat -e cycles,cycles -A -a -G test  -C 1 -- sleep 10

Performance counter stats for 'sleep 10':

CPU1 <not counted> cycles test # 0.000 GHz
CPU1 23,933,981,448 cycles # 0.000 GHz

Now perf stat reports the correct counts for
for the non cgroup event.

If we run pong inside the cgroup, then we also get the correct counts:

$ perf stat -e cycles,cycles -A -a -G test  -C 1 -- sleep 10

Performance counter stats for 'sleep 10':

CPU1 22,297,726,205 cycles test # 0.000 GHz
CPU1 23,933,981,448 cycles # 0.000 GHz

      10.001457237 seconds time elapsed

Signed-off-by Stephane Eranian <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/20110825135803.GA4697@quad open here

Signed-off-by Ingo Molnar <[email protected]>

---
 include/linux/perf_event.h |   24 ++++++++++------
 kernel/events/core.c       |   63 +++++++++++++++++++++++++++++++++++++-------
 kernel/sched.c             |    2 +-
 3 files changed, 69 insertions(+), 20 deletions(-)

include/linux/perf_event.h +15 -9

index 245bafd..c816075 100644
@@ -944,8 +944,10 @@ extern void perf_pmu_unregister(struct pmu *pmu);
 
 extern int perf_num_counters(void);
 extern const char *perf_pmu_name(void);
-extern void __perf_event_task_sched_in(struct task_struct *task);
-extern void __perf_event_task_sched_out(struct task_struct *task, struct task_struct *next);
+extern void __perf_event_task_sched_in(struct task_struct *prev,
+				       struct task_struct *task);
+extern void __perf_event_task_sched_out(struct task_struct *prev,
+					struct task_struct *next);
 extern int perf_event_init_task(struct task_struct *child);
 extern void perf_event_exit_task(struct task_struct *child);
 extern void perf_event_free_task(struct task_struct *task);

@@ -1059,17 +1061,20 @@ perf_sw_event(u32 event_id, u64 nr, struct pt_regs *regs, u64 addr)
 
 extern struct jump_label_key perf_sched_events;
 
-static inline void perf_event_task_sched_in(struct task_struct *task)
+static inline void perf_event_task_sched_in(struct task_struct *prev,
+					    struct task_struct *task)
 {
 	if (static_branch(&perf_sched_events))
-		__perf_event_task_sched_in(task);
+		__perf_event_task_sched_in(prev, task);
 }
 
-static inline void perf_event_task_sched_out(struct task_struct *task, struct task_struct *next)
+static inline void perf_event_task_sched_out(struct task_struct *prev,
+					     struct task_struct *next)
 {
 	perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, NULL, 0);
 
-	__perf_event_task_sched_out(task, next);
+	if (static_branch(&perf_sched_events))
+		__perf_event_task_sched_out(prev, next);
 }
 
 extern void perf_event_mmap(struct vm_area_struct *vma);

@@ -1139,10 +1144,11 @@ extern void perf_event_disable(struct perf_event *event);
 extern void perf_event_task_tick(void);
 #else
 static inline void
-perf_event_task_sched_in(struct task_struct *task)			{ }
+perf_event_task_sched_in(struct task_struct *prev,
+			 struct task_struct *task)			{ }
 static inline void
-perf_event_task_sched_out(struct task_struct *task,
-			    struct task_struct *next)			{ }
+perf_event_task_sched_out(struct task_struct *prev,
+			  struct task_struct *next)			{ }
 static inline int perf_event_init_task(struct task_struct *child)	{ return 0; }
 static inline void perf_event_exit_task(struct task_struct *child)	{ }
 static inline void perf_event_free_task(struct task_struct *task)	{ }

kernel/events/core.c +53 ~~-10~~

106 lines not on this page · raw message

kernel/sched.c +1 -1

index ccacdbd..0408cdc 100644
@@ -3065,7 +3065,7 @@ static void finish_task_switch(struct rq *rq, struct task_struct *prev)
 #ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
 	local_irq_disable();
 #endif /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */
-	perf_event_task_sched_in(current);
+	perf_event_task_sched_in(prev, current);
 #ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
 	local_irq_enable();
 #endif /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */