All of lore.kernel.org
 help / color / mirror / Atom feed
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ingo Molnar <mingo@kernel.org>
Cc: linux-kernel@vger.kernel.org,
	Stephane Eranian <eranian@google.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Andi Kleen <ak@linux.intel.com>, David Ahern <dsahern@gmail.com>,
	Jiri Olsa <jolsa@redhat.com>, Kan Liang <kan.liang@intel.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Arnaldo Carvalho de Melo <acme@redhat.com>
Subject: [PATCH 1/5] perf stat: Fix per-pkg event reporting bug
Date: Thu, 17 Sep 2015 15:54:28 -0300	[thread overview]
Message-ID: <1442516072-27033-2-git-send-email-acme@kernel.org> (raw)
In-Reply-To: <1442516072-27033-1-git-send-email-acme@kernel.org>

From: Stephane Eranian <eranian@google.com>

Per-pkg events need to be captured once per processor socket. The code
in check_per_pkg() ensures only one value per processor package is used.
However there is a problem with this function in case the first CPU of
the package does not measure anything for the per-pkg event, but other
CPUs do.

Consider the following:

  $ create cgroup FOO; echo $$ >FOO/tasks; taskset -c 1 noploop &
  $ perf stat -a -I 1000 -e intel_cqm/llc_occupancy/ -G FOO sleep 100
    1.00000 <not counted> Bytes intel_cqm/llc_occupancy/  FOO

The reason for this is that CPU0 in the cgroup has nothing running on it.
Yet check_per_plg() will mark socket0 as processed and no other event
value will be considered for the socket.

This patch fixes the problem by having check_per_pkg() only consider
events which actually ran.

Signed-off-by: Stephane Eranian <eranian@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1441286620-10117-1-git-send-email-eranian@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/stat.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
index 415c359de465..2d065d065b67 100644
--- a/tools/perf/util/stat.c
+++ b/tools/perf/util/stat.c
@@ -196,7 +196,8 @@ static void zero_per_pkg(struct perf_evsel *counter)
 		memset(counter->per_pkg_mask, 0, MAX_NR_CPUS);
 }
 
-static int check_per_pkg(struct perf_evsel *counter, int cpu, bool *skip)
+static int check_per_pkg(struct perf_evsel *counter,
+			 struct perf_counts_values *vals, int cpu, bool *skip)
 {
 	unsigned long *mask = counter->per_pkg_mask;
 	struct cpu_map *cpus = perf_evsel__cpus(counter);
@@ -218,6 +219,17 @@ static int check_per_pkg(struct perf_evsel *counter, int cpu, bool *skip)
 		counter->per_pkg_mask = mask;
 	}
 
+	/*
+	 * we do not consider an event that has not run as a good
+	 * instance to mark a package as used (skip=1). Otherwise
+	 * we may run into a situation where the first CPU in a package
+	 * is not running anything, yet the second is, and this function
+	 * would mark the package as used after the first CPU and would
+	 * not read the values from the second CPU.
+	 */
+	if (!(vals->run && vals->ena))
+		return 0;
+
 	s = cpu_map__get_socket(cpus, cpu);
 	if (s < 0)
 		return -1;
@@ -235,7 +247,7 @@ process_counter_values(struct perf_stat_config *config, struct perf_evsel *evsel
 	static struct perf_counts_values zero;
 	bool skip = false;
 
-	if (check_per_pkg(evsel, cpu, &skip)) {
+	if (check_per_pkg(evsel, count, cpu, &skip)) {
 		pr_err("failed to read per-pkg counter\n");
 		return -1;
 	}
-- 
2.1.0


  reply	other threads:[~2015-09-17 18:54 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-09-17 18:54 [GIT PULL 0/5] perf/urgent fixes Arnaldo Carvalho de Melo
2015-09-17 18:54 ` Arnaldo Carvalho de Melo [this message]
2015-09-17 18:54 ` [PATCH 2/5] Revert "perf symbols: Fix mismatched declarations for elf_getphdrnum" Arnaldo Carvalho de Melo
2015-09-17 18:54 ` [PATCH 3/5] tools build: Add test for presence of numa_num_possible_cpus() in libnuma Arnaldo Carvalho de Melo
2015-09-17 18:54 ` [PATCH 4/5] tools build: Add test for presence of __get_cpuid() gcc builtin Arnaldo Carvalho de Melo
2015-09-17 18:54 ` [PATCH 5/5] perf tools: Bool functions shouldn't return -1 Arnaldo Carvalho de Melo
2015-09-18  5:46 ` [GIT PULL 0/5] perf/urgent fixes Ingo Molnar

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=1442516072-27033-2-git-send-email-acme@kernel.org \
    --to=acme@kernel.org \
    --cc=acme@redhat.com \
    --cc=adrian.hunter@intel.com \
    --cc=ak@linux.intel.com \
    --cc=dsahern@gmail.com \
    --cc=eranian@google.com \
    --cc=jolsa@redhat.com \
    --cc=kan.liang@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.