Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756055AbcC2H1C (ORCPT ); Tue, 29 Mar 2016 03:27:02 -0400 Received: from casper.infradead.org ([85.118.1.10]:40552 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751569AbcC2H07 (ORCPT ); Tue, 29 Mar 2016 03:26:59 -0400 Date: Tue, 29 Mar 2016 09:26:44 +0200 From: Peter Zijlstra To: Stephane Eranian Cc: linux-kernel@vger.kernel.org, acme@redhat.com, mingo@elte.hu, ak@linux.intel.com, kan.liang@intel.com, jolsa@redhat.com, namhyung@kernel.org, vincent.weaver@maine.edu Subject: Re: [PATCH] perf/core: Fix time tracking bug with multiplexing Message-ID: <20160329072644.GB3408@twins.programming.kicks-ass.net> References: <1459215203-12885-1-git-send-email-eranian@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1459215203-12885-1-git-send-email-eranian@google.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2932 Lines: 88 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 > 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: > [] dump_stack+0x63/0x82 > [] ctx_sched_out+0x2bc/0x2d0 > [] perf_mux_hrtimer_handler+0xf6/0x2c0 > [] ? __perf_install_in_context+0x130/0x130 > [] __hrtimer_run_queues+0xf8/0x2f0 > [] hrtimer_interrupt+0xb7/0x1d0 > [] local_apic_timer_interrupt+0x38/0x60 > [] smp_apic_timer_interrupt+0x3d/0x50 > [] 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;