Messages in this thread Patch in this message | | | Date | Tue, 29 Mar 2016 09:26:44 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH] perf/core: Fix time tracking bug with multiplexing |
| |
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 <peterz@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;
| |