LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Stephane Eranian <eranian@google.com>
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
Date: Tue, 29 Mar 2016 09:26:44 +0200
Message-ID: <20160329072644.GB3408@twins.programming.kicks-ass.net> (raw)
In-Reply-To: <1459215203-12885-1-git-send-email-eranian@google.com>

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;
 

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-29  1:33 Stephane Eranian
2016-03-29  7:26 ` Peter Zijlstra [this message]
2016-03-29 15:43   ` Arnaldo Carvalho de Melo
2016-03-29 21:23   ` Stephane Eranian
2016-03-31  9:18   ` [tip:perf/core] " tip-bot for Peter Zijlstra

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160329072644.GB3408@twins.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=acme@redhat.com \
    --cc=ak@linux.intel.com \
    --cc=eranian@google.com \
    --cc=jolsa@redhat.com \
    --cc=kan.liang@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=namhyung@kernel.org \
    --cc=vincent.weaver@maine.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git