Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1758191AbcC2VXi (ORCPT <rfc822;w@1wt.eu>);
	Tue, 29 Mar 2016 17:23:38 -0400
Received: from mail-wm0-f42.google.com ([74.125.82.42]:34452 "EHLO
	mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753988AbcC2VXh (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 29 Mar 2016 17:23:37 -0400
MIME-Version: 1.0
In-Reply-To: <20160329072644.GB3408@twins.programming.kicks-ass.net>
References: <1459215203-12885-1-git-send-email-eranian@google.com>
	<20160329072644.GB3408@twins.programming.kicks-ass.net>
Date: Tue, 29 Mar 2016 14:23:35 -0700
Message-ID: <CABPqkBSG0J-d2_2Ek6Myj2A-td=saujvx_+p0NwhrD_YXxmP4w@mail.gmail.com>
Subject: Re: [PATCH] perf/core: Fix time tracking bug with multiplexing
From: Stephane Eranian <eranian@google.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        "mingo@elte.hu" <mingo@elte.hu>,
        "ak@linux.intel.com" <ak@linux.intel.com>,
        "Liang, Kan" <kan.liang@intel.com>, Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Vince Weaver <vincent.weaver@maine.edu>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3455
Lines: 93

On Tue, Mar 29, 2016 at 12:26 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> 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.
>
Works for me. Thanks.
Tested-by: Stephane Eranian <eranian@google.com>

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