public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>,
	stephane eranian <eranian@googlemail.com>,
	Corey J Ashford <cjashfor@us.ibm.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: [PATCH 4/2] perf_counter: Fix swcounter context invariance
Date: Thu, 13 Aug 2009 09:51:55 +0200	[thread overview]
Message-ID: <1250149915.10001.66.camel@twins> (raw)
In-Reply-To: <20090812153529.716542680@chello.nl>

Not really related to this topic, but it needs posting anyway.

---

Subject: perf_counter: Fix swcounter context invariance
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Fri Aug 07 13:29:13 CEST 2009

perf_swcounter_is_counting() uses a lock, which means we cannot use
swcounters from NMI or when holding that particular lock, this is
unintended.

The below removes the lock, this opens up race window, but not worse
than the swcounters already experience due to RCU traversal of the
context in perf_swcounter_ctx_event().

Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/perf_counter.c |   44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

Index: linux-2.6/kernel/perf_counter.c
===================================================================
--- linux-2.6.orig/kernel/perf_counter.c
+++ linux-2.6/kernel/perf_counter.c
@@ -3519,40 +3519,32 @@ static void perf_swcounter_add(struct pe
 
 static int perf_swcounter_is_counting(struct perf_counter *counter)
 {
-	struct perf_counter_context *ctx;
-	unsigned long flags;
-	int count;
-
+	/*
+	 * The counter is active, we're good!
+	 */
 	if (counter->state == PERF_COUNTER_STATE_ACTIVE)
 		return 1;
 
+	/*
+	 * The counter is off/error, not counting.
+	 */
 	if (counter->state != PERF_COUNTER_STATE_INACTIVE)
 		return 0;
 
 	/*
-	 * If the counter is inactive, it could be just because
-	 * its task is scheduled out, or because it's in a group
-	 * which could not go on the PMU.  We want to count in
-	 * the first case but not the second.  If the context is
-	 * currently active then an inactive software counter must
-	 * be the second case.  If it's not currently active then
-	 * we need to know whether the counter was active when the
-	 * context was last active, which we can determine by
-	 * comparing counter->tstamp_stopped with ctx->time.
-	 *
-	 * We are within an RCU read-side critical section,
-	 * which protects the existence of *ctx.
+	 * The counter is inactive, if the context is active
+	 * we're part of a group that didn't make it on the 'pmu',
+	 * not counting.
 	 */
-	ctx = counter->ctx;
-	spin_lock_irqsave(&ctx->lock, flags);
-	count = 1;
-	/* Re-check state now we have the lock */
-	if (counter->state < PERF_COUNTER_STATE_INACTIVE ||
-	    counter->ctx->is_active ||
-	    counter->tstamp_stopped < ctx->time)
-		count = 0;
-	spin_unlock_irqrestore(&ctx->lock, flags);
-	return count;
+	if (counter->ctx->is_active)
+		return 0;
+
+	/*
+	 * We're inactive and the context is too, this means the
+	 * task is scheduled out, we're counting events that happen
+	 * to us, like migration events.
+	 */
+	return 1;
 }
 
 static int perf_swcounter_match(struct perf_counter *counter,


  parent reply	other threads:[~2009-08-13  7:52 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-08-12 15:35 [PATCH 0/2] perf_counter: fix the group mess Peter Zijlstra
2009-08-12 15:35 ` [PATCH 1/2] perf: rework the whole read vs group stuff Peter Zijlstra
2009-08-12 15:35 ` [PATCH 2/2] perf_counter: Fix an ipi-deadlock Peter Zijlstra
2009-08-12 15:54 ` [PATCH 0/2] perf_counter: fix the group mess stephane eranian
2009-08-12 16:01   ` Peter Zijlstra
2009-08-13  7:51 ` [PATCH 3/2] perf tools: Fixup read ABI breakage Peter Zijlstra
2009-08-13  7:51 ` Peter Zijlstra [this message]
2009-08-13  8:05   ` [PATCH 4/2] perf_counter: Fix swcounter context invariance Frederic Weisbecker
2009-08-13  8:22     ` Peter Zijlstra
2009-08-13  8:29       ` Frederic Weisbecker
2009-08-13 10:21   ` [tip:perfcounters/urgent] " tip-bot for Peter Zijlstra

Reply instructions:

You may reply publicly 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=1250149915.10001.66.camel@twins \
    --to=peterz@infradead.org \
    --cc=cjashfor@us.ibm.com \
    --cc=eranian@googlemail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=paulus@samba.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox