On Tue, Mar 29, 2016 at 03:33:23AM +0200, Stephane Eranian wrote:
> This patch fixes a bug introduced by:
>
> commit 3cbaa59069677920186dcf502632ca1df4329f80
> Author: Peter Zijlstra <pet...@infradead.org>
> Date: Wed Feb 24 18:45:47 2016 +0100
>
> perf: Fix ctx time tracking by introducing EVENT_TIME
Normal quoting style is:
3cbaa5906967 ("perf: Fix ctx time tracking by introducing EVENT_TIME")
I have the following git alias to help with that:
one = show -s --pretty='format:%h (\"%s\")'
> The problem is that in case that the kernel enters ctx_sched_out() with the
> following state:
> ctx->is_active=0x7 event_type=0x1
> Call Trace:
> [<ffffffff813ddd41>] dump_stack+0x63/0x82
> [<ffffffff81182bdc>] ctx_sched_out+0x2bc/0x2d0
> [<ffffffff81183896>] perf_mux_hrtimer_handler+0xf6/0x2c0
> [<ffffffff811837a0>] ? __perf_install_in_context+0x130/0x130
> [<ffffffff810f5818>] __hrtimer_run_queues+0xf8/0x2f0
> [<ffffffff810f6097>] hrtimer_interrupt+0xb7/0x1d0
> [<ffffffff810509a8>] local_apic_timer_interrupt+0x38/0x60
> [<ffffffff8175ca9d>] smp_apic_timer_interrupt+0x3d/0x50
> [<ffffffff8175ac7c>] apic_timer_interrupt+0x8c/0xa0
>
> In that case, the test:
> if (is_active & EVENT_TIME)
>
> will be false and the time will not be updated. Time must always be updated on
> sched out. This patch fixes the problem.
Humm, no. It breaks things like ctx_sched_out(.event_type = EVENT_ALL),
which will set ctx->is_active = 0, and then not update time.
> +++ b/kernel/events/core.c
> @@ -2447,7 +2447,7 @@ static void ctx_sched_out(struct perf_event_context
> *ctx,
>
> is_active ^= ctx->is_active; /* changed bits */
>
> - if (is_active & EVENT_TIME) {
> + if (ctx->is_active & EVENT_TIME) {
> /* update (and stop) ctx time */
> update_context_time(ctx);
> update_cgrp_time_from_cpuctx(cpuctx);
I think you want something like this.
---
kernel/events/core.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 614614821f00..10ee22b8d2a8 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2402,14 +2402,24 @@ static void ctx_sched_out(struct perf_event_context
*ctx,
cpuctx->task_ctx = NULL;
}
- is_active ^= ctx->is_active; /* changed bits */
-
+ /*
+ * Always update time if it was set; not only when it changes.
+ * Otherwise we can 'forget' to update time for any but the last
+ * context we sched out. For example:
+ *
+ * ctx_sched_out(.event_type = EVENT_FLEXIBLE)
+ * ctx_sched_out(.event_type = EVENT_PINNED)
+ *
+ * would only update time for the pinned events.
+ */
if (is_active & EVENT_TIME) {
/* update (and stop) ctx time */
update_context_time(ctx);
update_cgrp_time_from_cpuctx(cpuctx);
}
+ is_active ^= ctx->is_active; /* changed bits */
+
if (!ctx->nr_active || !(is_active & EVENT_ALL))
return;
