LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* A number of perf stat improvements
@ 2015-03-08 23:55 Andi Kleen
  2015-03-08 23:55 ` [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode Andi Kleen
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Andi Kleen @ 2015-03-08 23:55 UTC (permalink / raw)
  To: acme; +Cc: jolsa, linux-kernel

Here are a number of perf stat improvements I had lying around
in various branches. Two are bug fixes, and two others are 
(minor) new features.

-Andi


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode
  2015-03-08 23:55 A number of perf stat improvements Andi Kleen
@ 2015-03-08 23:55 ` Andi Kleen
  2015-03-09  7:49   ` Jiri Olsa
  2015-03-10  7:15   ` Namhyung Kim
  2015-03-08 23:55 ` [PATCH 2/4] perf, tools: Fix metrics calculation with event qualifiers Andi Kleen
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 16+ messages in thread
From: Andi Kleen @ 2015-03-08 23:55 UTC (permalink / raw)
  To: acme; +Cc: jolsa, linux-kernel, Andi Kleen

From: Andi Kleen <ak@linux.intel.com>

The information how much a counter ran in perf stat can be quite
interesting for other tools to judge how trustworthy a measurement is.

Currently it is only output in non CSV mode.

This patches make perf stat always output the running time and the
enabled/running ratio in CSV mode.

This adds two new fields at the end for each line. I assume that existing
tools ignore new fields at the end, so it's on by default.

Only CSV mode is affected, no difference otherwise.

v2: Add extra print_running function
v3: Avoid printing nan
Reviewed-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/builtin-stat.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d28949d..5d0c6ea 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -769,6 +769,16 @@ static int run_perf_stat(int argc, const char **argv)
 	return ret;
 }
 
+static void print_running(u64 run, u64 ena)
+{
+	if (csv_output)
+		fprintf(output, "%s%" PRIu64 "%s%.2f",
+					csv_sep,
+					run,
+					csv_sep,
+					ena ? 100.0 * run / ena : 100.0);
+}
+
 static void print_noise_pct(double total, double avg)
 {
 	double pct = rel_stddev_stats(total, avg);
@@ -1252,6 +1262,7 @@ static void print_aggr(char *prefix)
 					fprintf(output, "%s%s",
 						csv_sep, counter->cgrp->name);
 
+				print_running(run, ena);
 				fputc('\n', output);
 				continue;
 			}
@@ -1268,6 +1279,8 @@ static void print_aggr(char *prefix)
 				if (run != ena)
 					fprintf(output, "  (%.2f%%)",
 						100.0 * run / ena);
+			} else {
+				print_running(run, ena);
 			}
 			fputc('\n', output);
 		}
@@ -1284,6 +1297,10 @@ static void print_counter_aggr(struct perf_evsel *counter, char *prefix)
 	double avg = avg_stats(&ps->res_stats[0]);
 	int scaled = counter->counts->scaled;
 	double uval;
+	double avg_enabled, avg_running;
+
+	avg_enabled = avg_stats(&ps->res_stats[1]);
+	avg_running = avg_stats(&ps->res_stats[2]);
 
 	if (prefix)
 		fprintf(output, "%s", prefix);
@@ -1303,6 +1320,7 @@ static void print_counter_aggr(struct perf_evsel *counter, char *prefix)
 		if (counter->cgrp)
 			fprintf(output, "%s%s", csv_sep, counter->cgrp->name);
 
+		print_running(avg_running, avg_enabled);
 		fputc('\n', output);
 		return;
 	}
@@ -1316,19 +1334,9 @@ static void print_counter_aggr(struct perf_evsel *counter, char *prefix)
 
 	print_noise(counter, avg);
 
-	if (csv_output) {
-		fputc('\n', output);
-		return;
-	}
-
-	if (scaled) {
-		double avg_enabled, avg_running;
-
-		avg_enabled = avg_stats(&ps->res_stats[1]);
-		avg_running = avg_stats(&ps->res_stats[2]);
-
+	print_running(avg_running, avg_enabled);
+	if (!csv_output)
 		fprintf(output, " [%5.2f%%]", 100 * avg_running / avg_enabled);
-	}
 	fprintf(output, "\n");
 }
 
@@ -1370,6 +1378,7 @@ static void print_counter(struct perf_evsel *counter, char *prefix)
 				fprintf(output, "%s%s",
 					csv_sep, counter->cgrp->name);
 
+			print_running(run, ena);
 			fputc('\n', output);
 			continue;
 		}
@@ -1387,7 +1396,10 @@ static void print_counter(struct perf_evsel *counter, char *prefix)
 			if (run != ena)
 				fprintf(output, "  (%.2f%%)",
 					100.0 * run / ena);
+		} else {
+			print_running(run, ena);
 		}
+
 		fputc('\n', output);
 	}
 }
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/4] perf, tools: Fix metrics calculation with event qualifiers
  2015-03-08 23:55 A number of perf stat improvements Andi Kleen
  2015-03-08 23:55 ` [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode Andi Kleen
@ 2015-03-08 23:55 ` Andi Kleen
  2015-03-09  7:55   ` Jiri Olsa
  2015-03-08 23:55 ` [PATCH 3/4] perf, tools, stat: Fix IPC and other formulas with -A Andi Kleen
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Andi Kleen @ 2015-03-08 23:55 UTC (permalink / raw)
  To: acme; +Cc: jolsa, linux-kernel, Andi Kleen

From: Andi Kleen <ak@linux.intel.com>

Currently in perf IPC and other metrics cannot be directly shown
separately for both user and kernel in a single run. The problem was
that the metrics matching code did not check event qualifiers.

With this patch the following case works correctly.

% perf stat -e cycles:k,cycles:u,instructions:k,instructions:u true

 Performance counter stats for 'true':

           531,718      cycles:k
           203,895      cycles:u
           338,151      instructions:k            #    0.64  insns per cycle
           105,961      instructions:u            #    0.52  insns per cycle

       0.002989739 seconds time elapsed

Previously it would misreport the ratios because they were matching
the wrong value.

The patch is fairly big, but quite mechanic as it just
adds context indexes everywhere.

Reported-by: William Cohen <wcohen@redhat.com>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/builtin-stat.c | 129 +++++++++++++++++++++++++++-------------------
 1 file changed, 77 insertions(+), 52 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 5d0c6ea..b5ba033 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -247,21 +247,35 @@ out_free:
 	return -1;
 }
 
+#define NUM_CTX 3
+
+enum { CTX_USER, CTX_KERNEL, CTX_ALL };
+
 static struct stats runtime_nsecs_stats[MAX_NR_CPUS];
-static struct stats runtime_cycles_stats[MAX_NR_CPUS];
-static struct stats runtime_stalled_cycles_front_stats[MAX_NR_CPUS];
-static struct stats runtime_stalled_cycles_back_stats[MAX_NR_CPUS];
-static struct stats runtime_branches_stats[MAX_NR_CPUS];
-static struct stats runtime_cacherefs_stats[MAX_NR_CPUS];
-static struct stats runtime_l1_dcache_stats[MAX_NR_CPUS];
-static struct stats runtime_l1_icache_stats[MAX_NR_CPUS];
-static struct stats runtime_ll_cache_stats[MAX_NR_CPUS];
-static struct stats runtime_itlb_cache_stats[MAX_NR_CPUS];
-static struct stats runtime_dtlb_cache_stats[MAX_NR_CPUS];
-static struct stats runtime_cycles_in_tx_stats[MAX_NR_CPUS];
+static struct stats runtime_cycles_stats[NUM_CTX][MAX_NR_CPUS];
+static struct stats runtime_stalled_cycles_front_stats[NUM_CTX][MAX_NR_CPUS];
+static struct stats runtime_stalled_cycles_back_stats[NUM_CTX][MAX_NR_CPUS];
+static struct stats runtime_branches_stats[NUM_CTX][MAX_NR_CPUS];
+static struct stats runtime_cacherefs_stats[NUM_CTX][MAX_NR_CPUS];
+static struct stats runtime_l1_dcache_stats[NUM_CTX][MAX_NR_CPUS];
+static struct stats runtime_l1_icache_stats[NUM_CTX][MAX_NR_CPUS];
+static struct stats runtime_ll_cache_stats[NUM_CTX][MAX_NR_CPUS];
+static struct stats runtime_itlb_cache_stats[NUM_CTX][MAX_NR_CPUS];
+static struct stats runtime_dtlb_cache_stats[NUM_CTX][MAX_NR_CPUS];
+static struct stats runtime_cycles_in_tx_stats[NUM_CTX][MAX_NR_CPUS];
 static struct stats walltime_nsecs_stats;
-static struct stats runtime_transaction_stats[MAX_NR_CPUS];
-static struct stats runtime_elision_stats[MAX_NR_CPUS];
+static struct stats runtime_transaction_stats[NUM_CTX][MAX_NR_CPUS];
+static struct stats runtime_elision_stats[NUM_CTX][MAX_NR_CPUS];
+
+static int evsel_context(struct perf_evsel *evsel)
+{
+	if (evsel->attr.exclude_kernel)
+		return CTX_USER;
+	if (evsel->attr.exclude_user)
+		return CTX_KERNEL;
+	/* Handle hypervisor too? */
+	return CTX_ALL;
+}
 
 static void perf_stat__reset_stats(struct perf_evlist *evlist)
 {
@@ -355,37 +369,39 @@ static struct perf_evsel *nth_evsel(int n)
  */
 static void update_shadow_stats(struct perf_evsel *counter, u64 *count)
 {
+	int ctx = evsel_context(counter);
+
 	if (perf_evsel__match(counter, SOFTWARE, SW_TASK_CLOCK))
 		update_stats(&runtime_nsecs_stats[0], count[0]);
 	else if (perf_evsel__match(counter, HARDWARE, HW_CPU_CYCLES))
-		update_stats(&runtime_cycles_stats[0], count[0]);
+		update_stats(&runtime_cycles_stats[ctx][0], count[0]);
 	else if (transaction_run &&
 		 perf_evsel__cmp(counter, nth_evsel(T_CYCLES_IN_TX)))
-		update_stats(&runtime_cycles_in_tx_stats[0], count[0]);
+		update_stats(&runtime_cycles_in_tx_stats[ctx][0], count[0]);
 	else if (transaction_run &&
 		 perf_evsel__cmp(counter, nth_evsel(T_TRANSACTION_START)))
-		update_stats(&runtime_transaction_stats[0], count[0]);
+		update_stats(&runtime_transaction_stats[ctx][0], count[0]);
 	else if (transaction_run &&
 		 perf_evsel__cmp(counter, nth_evsel(T_ELISION_START)))
-		update_stats(&runtime_elision_stats[0], count[0]);
+		update_stats(&runtime_elision_stats[ctx][0], count[0]);
 	else if (perf_evsel__match(counter, HARDWARE, HW_STALLED_CYCLES_FRONTEND))
-		update_stats(&runtime_stalled_cycles_front_stats[0], count[0]);
+		update_stats(&runtime_stalled_cycles_front_stats[ctx][0], count[0]);
 	else if (perf_evsel__match(counter, HARDWARE, HW_STALLED_CYCLES_BACKEND))
-		update_stats(&runtime_stalled_cycles_back_stats[0], count[0]);
+		update_stats(&runtime_stalled_cycles_back_stats[ctx][0], count[0]);
 	else if (perf_evsel__match(counter, HARDWARE, HW_BRANCH_INSTRUCTIONS))
-		update_stats(&runtime_branches_stats[0], count[0]);
+		update_stats(&runtime_branches_stats[ctx][0], count[0]);
 	else if (perf_evsel__match(counter, HARDWARE, HW_CACHE_REFERENCES))
-		update_stats(&runtime_cacherefs_stats[0], count[0]);
+		update_stats(&runtime_cacherefs_stats[ctx][0], count[0]);
 	else if (perf_evsel__match(counter, HW_CACHE, HW_CACHE_L1D))
-		update_stats(&runtime_l1_dcache_stats[0], count[0]);
+		update_stats(&runtime_l1_dcache_stats[ctx][0], count[0]);
 	else if (perf_evsel__match(counter, HW_CACHE, HW_CACHE_L1I))
-		update_stats(&runtime_l1_icache_stats[0], count[0]);
+		update_stats(&runtime_l1_icache_stats[ctx][0], count[0]);
 	else if (perf_evsel__match(counter, HW_CACHE, HW_CACHE_LL))
-		update_stats(&runtime_ll_cache_stats[0], count[0]);
+		update_stats(&runtime_ll_cache_stats[ctx][0], count[0]);
 	else if (perf_evsel__match(counter, HW_CACHE, HW_CACHE_DTLB))
-		update_stats(&runtime_dtlb_cache_stats[0], count[0]);
+		update_stats(&runtime_dtlb_cache_stats[ctx][0], count[0]);
 	else if (perf_evsel__match(counter, HW_CACHE, HW_CACHE_ITLB))
-		update_stats(&runtime_itlb_cache_stats[0], count[0]);
+		update_stats(&runtime_itlb_cache_stats[ctx][0], count[0]);
 }
 
 static void zero_per_pkg(struct perf_evsel *counter)
@@ -902,8 +918,9 @@ static void print_stalled_cycles_frontend(int cpu,
 {
 	double total, ratio = 0.0;
 	const char *color;
+	int ctx = evsel_context(evsel);
 
-	total = avg_stats(&runtime_cycles_stats[cpu]);
+	total = avg_stats(&runtime_cycles_stats[ctx][cpu]);
 
 	if (total)
 		ratio = avg / total * 100.0;
@@ -921,8 +938,9 @@ static void print_stalled_cycles_backend(int cpu,
 {
 	double total, ratio = 0.0;
 	const char *color;
+	int ctx = evsel_context(evsel);
 
-	total = avg_stats(&runtime_cycles_stats[cpu]);
+	total = avg_stats(&runtime_cycles_stats[ctx][cpu]);
 
 	if (total)
 		ratio = avg / total * 100.0;
@@ -940,8 +958,9 @@ static void print_branch_misses(int cpu,
 {
 	double total, ratio = 0.0;
 	const char *color;
+	int ctx = evsel_context(evsel);
 
-	total = avg_stats(&runtime_branches_stats[cpu]);
+	total = avg_stats(&runtime_branches_stats[ctx][cpu]);
 
 	if (total)
 		ratio = avg / total * 100.0;
@@ -959,8 +978,9 @@ static void print_l1_dcache_misses(int cpu,
 {
 	double total, ratio = 0.0;
 	const char *color;
+	int ctx = evsel_context(evsel);
 
-	total = avg_stats(&runtime_l1_dcache_stats[cpu]);
+	total = avg_stats(&runtime_l1_dcache_stats[ctx][cpu]);
 
 	if (total)
 		ratio = avg / total * 100.0;
@@ -978,8 +998,9 @@ static void print_l1_icache_misses(int cpu,
 {
 	double total, ratio = 0.0;
 	const char *color;
+	int ctx = evsel_context(evsel);
 
-	total = avg_stats(&runtime_l1_icache_stats[cpu]);
+	total = avg_stats(&runtime_l1_icache_stats[ctx][cpu]);
 
 	if (total)
 		ratio = avg / total * 100.0;
@@ -997,8 +1018,9 @@ static void print_dtlb_cache_misses(int cpu,
 {
 	double total, ratio = 0.0;
 	const char *color;
+	int ctx = evsel_context(evsel);
 
-	total = avg_stats(&runtime_dtlb_cache_stats[cpu]);
+	total = avg_stats(&runtime_dtlb_cache_stats[ctx][cpu]);
 
 	if (total)
 		ratio = avg / total * 100.0;
@@ -1016,8 +1038,9 @@ static void print_itlb_cache_misses(int cpu,
 {
 	double total, ratio = 0.0;
 	const char *color;
+	int ctx = evsel_context(evsel);
 
-	total = avg_stats(&runtime_itlb_cache_stats[cpu]);
+	total = avg_stats(&runtime_itlb_cache_stats[ctx][cpu]);
 
 	if (total)
 		ratio = avg / total * 100.0;
@@ -1035,8 +1058,9 @@ static void print_ll_cache_misses(int cpu,
 {
 	double total, ratio = 0.0;
 	const char *color;
+	int ctx = evsel_context(evsel);
 
-	total = avg_stats(&runtime_ll_cache_stats[cpu]);
+	total = avg_stats(&runtime_ll_cache_stats[ctx][cpu]);
 
 	if (total)
 		ratio = avg / total * 100.0;
@@ -1054,6 +1078,7 @@ static void abs_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 	double sc =  evsel->scale;
 	const char *fmt;
 	int cpu = cpu_map__id_to_cpu(id);
+	int ctx = evsel_context(evsel);
 
 	if (csv_output) {
 		fmt = sc != 1.0 ?  "%.2f%s" : "%.0f%s";
@@ -1085,13 +1110,13 @@ static void abs_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 		return;
 
 	if (perf_evsel__match(evsel, HARDWARE, HW_INSTRUCTIONS)) {
-		total = avg_stats(&runtime_cycles_stats[cpu]);
+		total = avg_stats(&runtime_cycles_stats[ctx][cpu]);
 		if (total) {
 			ratio = avg / total;
 			fprintf(output, " #   %5.2f  insns per cycle        ", ratio);
 		}
-		total = avg_stats(&runtime_stalled_cycles_front_stats[cpu]);
-		total = max(total, avg_stats(&runtime_stalled_cycles_back_stats[cpu]));
+		total = avg_stats(&runtime_stalled_cycles_front_stats[ctx][cpu]);
+		total = max(total, avg_stats(&runtime_stalled_cycles_back_stats[ctx][cpu]));
 
 		if (total && avg) {
 			ratio = total / avg;
@@ -1102,46 +1127,46 @@ static void abs_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 		}
 
 	} else if (perf_evsel__match(evsel, HARDWARE, HW_BRANCH_MISSES) &&
-			runtime_branches_stats[cpu].n != 0) {
+			runtime_branches_stats[ctx][cpu].n != 0) {
 		print_branch_misses(cpu, evsel, avg);
 	} else if (
 		evsel->attr.type == PERF_TYPE_HW_CACHE &&
 		evsel->attr.config ==  ( PERF_COUNT_HW_CACHE_L1D |
 					((PERF_COUNT_HW_CACHE_OP_READ) << 8) |
 					((PERF_COUNT_HW_CACHE_RESULT_MISS) << 16)) &&
-			runtime_l1_dcache_stats[cpu].n != 0) {
+			runtime_l1_dcache_stats[ctx][cpu].n != 0) {
 		print_l1_dcache_misses(cpu, evsel, avg);
 	} else if (
 		evsel->attr.type == PERF_TYPE_HW_CACHE &&
 		evsel->attr.config ==  ( PERF_COUNT_HW_CACHE_L1I |
 					((PERF_COUNT_HW_CACHE_OP_READ) << 8) |
 					((PERF_COUNT_HW_CACHE_RESULT_MISS) << 16)) &&
-			runtime_l1_icache_stats[cpu].n != 0) {
+			runtime_l1_icache_stats[ctx][cpu].n != 0) {
 		print_l1_icache_misses(cpu, evsel, avg);
 	} else if (
 		evsel->attr.type == PERF_TYPE_HW_CACHE &&
 		evsel->attr.config ==  ( PERF_COUNT_HW_CACHE_DTLB |
 					((PERF_COUNT_HW_CACHE_OP_READ) << 8) |
 					((PERF_COUNT_HW_CACHE_RESULT_MISS) << 16)) &&
-			runtime_dtlb_cache_stats[cpu].n != 0) {
+			runtime_dtlb_cache_stats[ctx][cpu].n != 0) {
 		print_dtlb_cache_misses(cpu, evsel, avg);
 	} else if (
 		evsel->attr.type == PERF_TYPE_HW_CACHE &&
 		evsel->attr.config ==  ( PERF_COUNT_HW_CACHE_ITLB |
 					((PERF_COUNT_HW_CACHE_OP_READ) << 8) |
 					((PERF_COUNT_HW_CACHE_RESULT_MISS) << 16)) &&
-			runtime_itlb_cache_stats[cpu].n != 0) {
+			runtime_itlb_cache_stats[ctx][cpu].n != 0) {
 		print_itlb_cache_misses(cpu, evsel, avg);
 	} else if (
 		evsel->attr.type == PERF_TYPE_HW_CACHE &&
 		evsel->attr.config ==  ( PERF_COUNT_HW_CACHE_LL |
 					((PERF_COUNT_HW_CACHE_OP_READ) << 8) |
 					((PERF_COUNT_HW_CACHE_RESULT_MISS) << 16)) &&
-			runtime_ll_cache_stats[cpu].n != 0) {
+			runtime_ll_cache_stats[ctx][cpu].n != 0) {
 		print_ll_cache_misses(cpu, evsel, avg);
 	} else if (perf_evsel__match(evsel, HARDWARE, HW_CACHE_MISSES) &&
-			runtime_cacherefs_stats[cpu].n != 0) {
-		total = avg_stats(&runtime_cacherefs_stats[cpu]);
+			runtime_cacherefs_stats[ctx][cpu].n != 0) {
+		total = avg_stats(&runtime_cacherefs_stats[ctx][cpu]);
 
 		if (total)
 			ratio = avg * 100 / total;
@@ -1161,15 +1186,15 @@ static void abs_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 		}
 	} else if (transaction_run &&
 		   perf_evsel__cmp(evsel, nth_evsel(T_CYCLES_IN_TX))) {
-		total = avg_stats(&runtime_cycles_stats[cpu]);
+		total = avg_stats(&runtime_cycles_stats[ctx][cpu]);
 		if (total)
 			fprintf(output,
 				" #   %5.2f%% transactional cycles   ",
 				100.0 * (avg / total));
 	} else if (transaction_run &&
 		   perf_evsel__cmp(evsel, nth_evsel(T_CYCLES_IN_TX_CP))) {
-		total = avg_stats(&runtime_cycles_stats[cpu]);
-		total2 = avg_stats(&runtime_cycles_in_tx_stats[cpu]);
+		total = avg_stats(&runtime_cycles_stats[ctx][cpu]);
+		total2 = avg_stats(&runtime_cycles_in_tx_stats[ctx][cpu]);
 		if (total2 < avg)
 			total2 = avg;
 		if (total)
@@ -1179,8 +1204,8 @@ static void abs_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 	} else if (transaction_run &&
 		   perf_evsel__cmp(evsel, nth_evsel(T_TRANSACTION_START)) &&
 		   avg > 0 &&
-		   runtime_cycles_in_tx_stats[cpu].n != 0) {
-		total = avg_stats(&runtime_cycles_in_tx_stats[cpu]);
+		   runtime_cycles_in_tx_stats[ctx][cpu].n != 0) {
+		total = avg_stats(&runtime_cycles_in_tx_stats[ctx][cpu]);
 
 		if (total)
 			ratio = total / avg;
@@ -1189,8 +1214,8 @@ static void abs_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 	} else if (transaction_run &&
 		   perf_evsel__cmp(evsel, nth_evsel(T_ELISION_START)) &&
 		   avg > 0 &&
-		   runtime_cycles_in_tx_stats[cpu].n != 0) {
-		total = avg_stats(&runtime_cycles_in_tx_stats[cpu]);
+		   runtime_cycles_in_tx_stats[ctx][cpu].n != 0) {
+		total = avg_stats(&runtime_cycles_in_tx_stats[ctx][cpu]);
 
 		if (total)
 			ratio = total / avg;
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/4] perf, tools, stat: Fix IPC and other formulas with -A
  2015-03-08 23:55 A number of perf stat improvements Andi Kleen
  2015-03-08 23:55 ` [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode Andi Kleen
  2015-03-08 23:55 ` [PATCH 2/4] perf, tools: Fix metrics calculation with event qualifiers Andi Kleen
@ 2015-03-08 23:55 ` Andi Kleen
  2015-03-09  8:11   ` Jiri Olsa
  2015-03-08 23:55 ` [PATCH 4/4] perf, tools, stat: Always correctly indent ratio column Andi Kleen
  2015-03-11 13:35 ` A number of perf stat improvements Arnaldo Carvalho de Melo
  4 siblings, 1 reply; 16+ messages in thread
From: Andi Kleen @ 2015-03-08 23:55 UTC (permalink / raw)
  To: acme; +Cc: jolsa, linux-kernel, Andi Kleen

From: Andi Kleen <ak@linux.intel.com>

perf stat didn't compute the IPC and other formulas for individual
CPUs with -A. Fix this for the easy -A case. As before,
--per-core and --per-socket do not handle it, they
simply print nothing.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/builtin-stat.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index b5ba033..05c7a00 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -367,41 +367,42 @@ static struct perf_evsel *nth_evsel(int n)
  * more semantic information such as miss/hit ratios,
  * instruction rates, etc:
  */
-static void update_shadow_stats(struct perf_evsel *counter, u64 *count)
+static void update_shadow_stats(struct perf_evsel *counter, u64 *count,
+				int cpu)
 {
 	int ctx = evsel_context(counter);
 
 	if (perf_evsel__match(counter, SOFTWARE, SW_TASK_CLOCK))
-		update_stats(&runtime_nsecs_stats[0], count[0]);
+		update_stats(&runtime_nsecs_stats[cpu], count[0]);
 	else if (perf_evsel__match(counter, HARDWARE, HW_CPU_CYCLES))
-		update_stats(&runtime_cycles_stats[ctx][0], count[0]);
+		update_stats(&runtime_cycles_stats[ctx][cpu], count[0]);
 	else if (transaction_run &&
 		 perf_evsel__cmp(counter, nth_evsel(T_CYCLES_IN_TX)))
-		update_stats(&runtime_cycles_in_tx_stats[ctx][0], count[0]);
+		update_stats(&runtime_cycles_in_tx_stats[ctx][cpu], count[0]);
 	else if (transaction_run &&
 		 perf_evsel__cmp(counter, nth_evsel(T_TRANSACTION_START)))
-		update_stats(&runtime_transaction_stats[ctx][0], count[0]);
+		update_stats(&runtime_transaction_stats[ctx][cpu], count[0]);
 	else if (transaction_run &&
 		 perf_evsel__cmp(counter, nth_evsel(T_ELISION_START)))
-		update_stats(&runtime_elision_stats[ctx][0], count[0]);
+		update_stats(&runtime_elision_stats[ctx][cpu], count[0]);
 	else if (perf_evsel__match(counter, HARDWARE, HW_STALLED_CYCLES_FRONTEND))
-		update_stats(&runtime_stalled_cycles_front_stats[ctx][0], count[0]);
+		update_stats(&runtime_stalled_cycles_front_stats[ctx][cpu], count[0]);
 	else if (perf_evsel__match(counter, HARDWARE, HW_STALLED_CYCLES_BACKEND))
-		update_stats(&runtime_stalled_cycles_back_stats[ctx][0], count[0]);
+		update_stats(&runtime_stalled_cycles_back_stats[ctx][cpu], count[0]);
 	else if (perf_evsel__match(counter, HARDWARE, HW_BRANCH_INSTRUCTIONS))
-		update_stats(&runtime_branches_stats[ctx][0], count[0]);
+		update_stats(&runtime_branches_stats[ctx][cpu], count[0]);
 	else if (perf_evsel__match(counter, HARDWARE, HW_CACHE_REFERENCES))
-		update_stats(&runtime_cacherefs_stats[ctx][0], count[0]);
+		update_stats(&runtime_cacherefs_stats[ctx][cpu], count[0]);
 	else if (perf_evsel__match(counter, HW_CACHE, HW_CACHE_L1D))
-		update_stats(&runtime_l1_dcache_stats[ctx][0], count[0]);
+		update_stats(&runtime_l1_dcache_stats[ctx][cpu], count[0]);
 	else if (perf_evsel__match(counter, HW_CACHE, HW_CACHE_L1I))
-		update_stats(&runtime_l1_icache_stats[ctx][0], count[0]);
+		update_stats(&runtime_l1_icache_stats[ctx][cpu], count[0]);
 	else if (perf_evsel__match(counter, HW_CACHE, HW_CACHE_LL))
-		update_stats(&runtime_ll_cache_stats[ctx][0], count[0]);
+		update_stats(&runtime_ll_cache_stats[ctx][cpu], count[0]);
 	else if (perf_evsel__match(counter, HW_CACHE, HW_CACHE_DTLB))
-		update_stats(&runtime_dtlb_cache_stats[ctx][0], count[0]);
+		update_stats(&runtime_dtlb_cache_stats[ctx][cpu], count[0]);
 	else if (perf_evsel__match(counter, HW_CACHE, HW_CACHE_ITLB))
-		update_stats(&runtime_itlb_cache_stats[ctx][0], count[0]);
+		update_stats(&runtime_itlb_cache_stats[ctx][cpu], count[0]);
 }
 
 static void zero_per_pkg(struct perf_evsel *counter)
@@ -463,7 +464,8 @@ static int read_cb(struct perf_evsel *evsel, int cpu, int thread __maybe_unused,
 			perf_evsel__compute_deltas(evsel, cpu, count);
 		perf_counts_values__scale(count, scale, NULL);
 		evsel->counts->cpu[cpu] = *count;
-		update_shadow_stats(evsel, count->values);
+		if (aggr_mode == AGGR_NONE)
+			update_shadow_stats(evsel, count->values, cpu);
 		break;
 	case AGGR_GLOBAL:
 		aggr->val += count->val;
@@ -511,7 +513,7 @@ static int read_counter_aggr(struct perf_evsel *counter)
 	/*
 	 * Save the full runtime - to allow normalization during printout:
 	 */
-	update_shadow_stats(counter, count);
+	update_shadow_stats(counter, count, 0);
 
 	return 0;
 }
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/4] perf, tools, stat: Always correctly indent ratio column
  2015-03-08 23:55 A number of perf stat improvements Andi Kleen
                   ` (2 preceding siblings ...)
  2015-03-08 23:55 ` [PATCH 3/4] perf, tools, stat: Fix IPC and other formulas with -A Andi Kleen
@ 2015-03-08 23:55 ` Andi Kleen
  2015-03-09  8:24   ` Jiri Olsa
  2015-03-11 13:35 ` A number of perf stat improvements Arnaldo Carvalho de Melo
  4 siblings, 1 reply; 16+ messages in thread
From: Andi Kleen @ 2015-03-08 23:55 UTC (permalink / raw)
  To: acme; +Cc: jolsa, linux-kernel, Andi Kleen

From: Andi Kleen <ak@linux.intel.com>

When cycles or instructions do not print anything, as in being,
--per-socket or --per-core modi, the ratio column was not
correctly indented for them. This lead to some ratios
not lining up with the others. Always indent correctly
when nothing is printed.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/builtin-stat.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 05c7a00..0b5162d 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1116,7 +1116,8 @@ static void abs_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 		if (total) {
 			ratio = avg / total;
 			fprintf(output, " #   %5.2f  insns per cycle        ", ratio);
-		}
+		} else
+			fprintf(output, "                                   ");
 		total = avg_stats(&runtime_stalled_cycles_front_stats[ctx][cpu]);
 		total = max(total, avg_stats(&runtime_stalled_cycles_back_stats[ctx][cpu]));
 
@@ -1185,7 +1186,8 @@ static void abs_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 		if (total) {
 			ratio = avg / total;
 			fprintf(output, " # %8.3f GHz                    ", ratio);
-		}
+		} else
+			fprintf(output, "                                   ");
 	} else if (transaction_run &&
 		   perf_evsel__cmp(evsel, nth_evsel(T_CYCLES_IN_TX))) {
 		total = avg_stats(&runtime_cycles_stats[ctx][cpu]);
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode
  2015-03-08 23:55 ` [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode Andi Kleen
@ 2015-03-09  7:49   ` Jiri Olsa
  2015-03-09 14:04     ` Andi Kleen
  2015-03-10  7:15   ` Namhyung Kim
  1 sibling, 1 reply; 16+ messages in thread
From: Jiri Olsa @ 2015-03-09  7:49 UTC (permalink / raw)
  To: Andi Kleen; +Cc: acme, linux-kernel, Andi Kleen

On Sun, Mar 08, 2015 at 04:55:21PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> The information how much a counter ran in perf stat can be quite
> interesting for other tools to judge how trustworthy a measurement is.
> 
> Currently it is only output in non CSV mode.
> 
> This patches make perf stat always output the running time and the
> enabled/running ratio in CSV mode.
> 
> This adds two new fields at the end for each line. I assume that existing
> tools ignore new fields at the end, so it's on by default.
> 
> Only CSV mode is affected, no difference otherwise.

hum, so people using CSV mode will now see extra 2 numbers
at the end of the line:

  $ ./perf.old stat -x, -e cycles ls > /dev/null
  2679161,,cycles
  $ ./perf stat -x, -e cycles ls > /dev/null
  2574169,,cycles,1295544,100.00

we could be breaking existing scripts..

you recently added 'running-time' option for record,
could you please add same option for stat?

plus 2 nits below

thanks,
jirka


> 
> v2: Add extra print_running function
> v3: Avoid printing nan
> Reviewed-by: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/builtin-stat.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index d28949d..5d0c6ea 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -769,6 +769,16 @@ static int run_perf_stat(int argc, const char **argv)
>  	return ret;
>  }
>  
> +static void print_running(u64 run, u64 ena)
> +{
> +	if (csv_output)
> +		fprintf(output, "%s%" PRIu64 "%s%.2f",
> +					csv_sep,
> +					run,
> +					csv_sep,
> +					ena ? 100.0 * run / ena : 100.0);

nit - need { } for multiplne lines 'if' leg

> +}
> +
>  static void print_noise_pct(double total, double avg)
>  {
>  	double pct = rel_stddev_stats(total, avg);
> @@ -1252,6 +1262,7 @@ static void print_aggr(char *prefix)
>  					fprintf(output, "%s%s",
>  						csv_sep, counter->cgrp->name);
>  
> +				print_running(run, ena);
>  				fputc('\n', output);
>  				continue;
>  			}
> @@ -1268,6 +1279,8 @@ static void print_aggr(char *prefix)
>  				if (run != ena)
>  					fprintf(output, "  (%.2f%%)",
>  						100.0 * run / ena);
> +			} else {
> +				print_running(run, ena);
>  			}

we already have 'csv_output' check in print_running

>  			fputc('\n', output);
>  		}
> @@ -1284,6 +1297,10 @@ static void print_counter_aggr(struct perf_evsel *counter, char *prefix)
>  	double avg = avg_stats(&ps->res_stats[0]);
>  	int scaled = counter->counts->scaled;
>  	double uval;
> +	double avg_enabled, avg_running;
> +
> +	avg_enabled = avg_stats(&ps->res_stats[1]);
> +	avg_running = avg_stats(&ps->res_stats[2]);
>  
>  	if (prefix)
>  		fprintf(output, "%s", prefix);
> @@ -1303,6 +1320,7 @@ static void print_counter_aggr(struct perf_evsel *counter, char *prefix)
>  		if (counter->cgrp)
>  			fprintf(output, "%s%s", csv_sep, counter->cgrp->name);
>  
> +		print_running(avg_running, avg_enabled);
>  		fputc('\n', output);
>  		return;
>  	}
> @@ -1316,19 +1334,9 @@ static void print_counter_aggr(struct perf_evsel *counter, char *prefix)
>  
>  	print_noise(counter, avg);
>  
> -	if (csv_output) {
> -		fputc('\n', output);
> -		return;
> -	}
> -
> -	if (scaled) {
> -		double avg_enabled, avg_running;
> -
> -		avg_enabled = avg_stats(&ps->res_stats[1]);
> -		avg_running = avg_stats(&ps->res_stats[2]);
> -
> +	print_running(avg_running, avg_enabled);
> +	if (!csv_output)
>  		fprintf(output, " [%5.2f%%]", 100 * avg_running / avg_enabled);
> -	}
>  	fprintf(output, "\n");
>  }
>  
> @@ -1370,6 +1378,7 @@ static void print_counter(struct perf_evsel *counter, char *prefix)
>  				fprintf(output, "%s%s",
>  					csv_sep, counter->cgrp->name);
>  
> +			print_running(run, ena);
>  			fputc('\n', output);
>  			continue;
>  		}
> @@ -1387,7 +1396,10 @@ static void print_counter(struct perf_evsel *counter, char *prefix)
>  			if (run != ena)
>  				fprintf(output, "  (%.2f%%)",
>  					100.0 * run / ena);
> +		} else {
> +			print_running(run, ena);
>  		}

same here

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/4] perf, tools: Fix metrics calculation with event qualifiers
  2015-03-08 23:55 ` [PATCH 2/4] perf, tools: Fix metrics calculation with event qualifiers Andi Kleen
@ 2015-03-09  7:55   ` Jiri Olsa
  0 siblings, 0 replies; 16+ messages in thread
From: Jiri Olsa @ 2015-03-09  7:55 UTC (permalink / raw)
  To: Andi Kleen; +Cc: acme, linux-kernel, Andi Kleen

On Sun, Mar 08, 2015 at 04:55:22PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Currently in perf IPC and other metrics cannot be directly shown
> separately for both user and kernel in a single run. The problem was
> that the metrics matching code did not check event qualifiers.
> 
> With this patch the following case works correctly.
> 
> % perf stat -e cycles:k,cycles:u,instructions:k,instructions:u true
> 
>  Performance counter stats for 'true':
> 
>            531,718      cycles:k
>            203,895      cycles:u
>            338,151      instructions:k            #    0.64  insns per cycle
>            105,961      instructions:u            #    0.52  insns per cycle
> 
>        0.002989739 seconds time elapsed
> 
> Previously it would misreport the ratios because they were matching
> the wrong value.
> 
> The patch is fairly big, but quite mechanic as it just
> adds context indexes everywhere.
> 
> Reported-by: William Cohen <wcohen@redhat.com>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

I like the patch, but your new version doesn't address Ingo's
comments on first version:
  http://marc.info/?l=linux-kernel&m=139748629929175&w=2

I can make follow up patch, if you are not going to work on that,
because I think this is really needed.. let me know

jirka

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/4] perf, tools, stat: Fix IPC and other formulas with -A
  2015-03-08 23:55 ` [PATCH 3/4] perf, tools, stat: Fix IPC and other formulas with -A Andi Kleen
@ 2015-03-09  8:11   ` Jiri Olsa
  0 siblings, 0 replies; 16+ messages in thread
From: Jiri Olsa @ 2015-03-09  8:11 UTC (permalink / raw)
  To: Andi Kleen; +Cc: acme, linux-kernel, Andi Kleen

On Sun, Mar 08, 2015 at 04:55:23PM -0700, Andi Kleen wrote:

SNIP

> -		update_stats(&runtime_dtlb_cache_stats[ctx][0], count[0]);
> +		update_stats(&runtime_dtlb_cache_stats[ctx][cpu], count[0]);
>  	else if (perf_evsel__match(counter, HW_CACHE, HW_CACHE_ITLB))
> -		update_stats(&runtime_itlb_cache_stats[ctx][0], count[0]);
> +		update_stats(&runtime_itlb_cache_stats[ctx][cpu], count[0]);
>  }
>  
>  static void zero_per_pkg(struct perf_evsel *counter)
> @@ -463,7 +464,8 @@ static int read_cb(struct perf_evsel *evsel, int cpu, int thread __maybe_unused,
>  			perf_evsel__compute_deltas(evsel, cpu, count);
>  		perf_counts_values__scale(count, scale, NULL);
>  		evsel->counts->cpu[cpu] = *count;
> -		update_shadow_stats(evsel, count->values);
> +		if (aggr_mode == AGGR_NONE)
> +			update_shadow_stats(evsel, count->values, cpu);

this will also prevent the 'socket' and 'core' aggregation
not to print bogus data for socket/core cpu 0, we'll need
to aggregate shadow stats first to support that.. anyway:

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/4] perf, tools, stat: Always correctly indent ratio column
  2015-03-08 23:55 ` [PATCH 4/4] perf, tools, stat: Always correctly indent ratio column Andi Kleen
@ 2015-03-09  8:24   ` Jiri Olsa
  0 siblings, 0 replies; 16+ messages in thread
From: Jiri Olsa @ 2015-03-09  8:24 UTC (permalink / raw)
  To: Andi Kleen; +Cc: acme, linux-kernel, Andi Kleen

On Sun, Mar 08, 2015 at 04:55:24PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> When cycles or instructions do not print anything, as in being,
> --per-socket or --per-core modi, the ratio column was not
> correctly indented for them. This lead to some ratios
> not lining up with the others. Always indent correctly
> when nothing is printed.

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode
  2015-03-09  7:49   ` Jiri Olsa
@ 2015-03-09 14:04     ` Andi Kleen
  0 siblings, 0 replies; 16+ messages in thread
From: Andi Kleen @ 2015-03-09 14:04 UTC (permalink / raw)
  To: Jiri Olsa; +Cc: Andi Kleen, acme, linux-kernel

> hum, so people using CSV mode will now see extra 2 numbers
> at the end of the line:
> 
>   $ ./perf.old stat -x, -e cycles ls > /dev/null
>   2679161,,cycles
>   $ ./perf stat -x, -e cycles ls > /dev/null
>   2574169,,cycles,1295544,100.00
> 
> we could be breaking existing scripts..

I doubt it. Typical CSV parsers ignore additional data at the end.

Just don't insert stuff in the middle (like it was done recently).
That breaks a lot of things.

> you recently added 'running-time' option for record,
> could you please add same option for stat?

Could be done, but I'm not sure it's worth it here.

-Andi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode
  2015-03-08 23:55 ` [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode Andi Kleen
  2015-03-09  7:49   ` Jiri Olsa
@ 2015-03-10  7:15   ` Namhyung Kim
  2015-03-10 15:37     ` Andi Kleen
  1 sibling, 1 reply; 16+ messages in thread
From: Namhyung Kim @ 2015-03-10  7:15 UTC (permalink / raw)
  To: Andi Kleen; +Cc: acme, jolsa, linux-kernel, Andi Kleen

Hi Andi,

On Sun, Mar 08, 2015 at 04:55:21PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> The information how much a counter ran in perf stat can be quite
> interesting for other tools to judge how trustworthy a measurement is.
> 
> Currently it is only output in non CSV mode.
> 
> This patches make perf stat always output the running time and the
> enabled/running ratio in CSV mode.
> 
> This adds two new fields at the end for each line. I assume that existing
> tools ignore new fields at the end, so it's on by default.
> 
> Only CSV mode is affected, no difference otherwise.
> 
> v2: Add extra print_running function
> v3: Avoid printing nan
> Reviewed-by: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/builtin-stat.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index d28949d..5d0c6ea 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -769,6 +769,16 @@ static int run_perf_stat(int argc, const char **argv)
>  	return ret;
>  }
>  
> +static void print_running(u64 run, u64 ena)
> +{
> +	if (csv_output)
> +		fprintf(output, "%s%" PRIu64 "%s%.2f",
> +					csv_sep,
> +					run,
> +					csv_sep,
> +					ena ? 100.0 * run / ena : 100.0);
> +}

Why not handle both cases here?

static void print_running(u64 run, u64 ena)
{
	if (csv_output)
		fprintf(output, ...);
	else if (run != ena)
		fprintf(output, ...);
}

Thanks,
Namhyung


> +
>  static void print_noise_pct(double total, double avg)
>  {
>  	double pct = rel_stddev_stats(total, avg);
> @@ -1252,6 +1262,7 @@ static void print_aggr(char *prefix)
>  					fprintf(output, "%s%s",
>  						csv_sep, counter->cgrp->name);
>  
> +				print_running(run, ena);
>  				fputc('\n', output);
>  				continue;
>  			}
> @@ -1268,6 +1279,8 @@ static void print_aggr(char *prefix)
>  				if (run != ena)
>  					fprintf(output, "  (%.2f%%)",
>  						100.0 * run / ena);
> +			} else {
> +				print_running(run, ena);
>  			}
>  			fputc('\n', output);
>  		}
> @@ -1284,6 +1297,10 @@ static void print_counter_aggr(struct perf_evsel *counter, char *prefix)
>  	double avg = avg_stats(&ps->res_stats[0]);
>  	int scaled = counter->counts->scaled;
>  	double uval;
> +	double avg_enabled, avg_running;
> +
> +	avg_enabled = avg_stats(&ps->res_stats[1]);
> +	avg_running = avg_stats(&ps->res_stats[2]);
>  
>  	if (prefix)
>  		fprintf(output, "%s", prefix);
> @@ -1303,6 +1320,7 @@ static void print_counter_aggr(struct perf_evsel *counter, char *prefix)
>  		if (counter->cgrp)
>  			fprintf(output, "%s%s", csv_sep, counter->cgrp->name);
>  
> +		print_running(avg_running, avg_enabled);
>  		fputc('\n', output);
>  		return;
>  	}
> @@ -1316,19 +1334,9 @@ static void print_counter_aggr(struct perf_evsel *counter, char *prefix)
>  
>  	print_noise(counter, avg);
>  
> -	if (csv_output) {
> -		fputc('\n', output);
> -		return;
> -	}
> -
> -	if (scaled) {
> -		double avg_enabled, avg_running;
> -
> -		avg_enabled = avg_stats(&ps->res_stats[1]);
> -		avg_running = avg_stats(&ps->res_stats[2]);
> -
> +	print_running(avg_running, avg_enabled);
> +	if (!csv_output)
>  		fprintf(output, " [%5.2f%%]", 100 * avg_running / avg_enabled);
> -	}
>  	fprintf(output, "\n");
>  }
>  
> @@ -1370,6 +1378,7 @@ static void print_counter(struct perf_evsel *counter, char *prefix)
>  				fprintf(output, "%s%s",
>  					csv_sep, counter->cgrp->name);
>  
> +			print_running(run, ena);
>  			fputc('\n', output);
>  			continue;
>  		}
> @@ -1387,7 +1396,10 @@ static void print_counter(struct perf_evsel *counter, char *prefix)
>  			if (run != ena)
>  				fprintf(output, "  (%.2f%%)",
>  					100.0 * run / ena);
> +		} else {
> +			print_running(run, ena);
>  		}
> +
>  		fputc('\n', output);
>  	}
>  }
> -- 
> 1.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode
  2015-03-10  7:15   ` Namhyung Kim
@ 2015-03-10 15:37     ` Andi Kleen
  2015-03-11  0:52       ` Namhyung Kim
  0 siblings, 1 reply; 16+ messages in thread
From: Andi Kleen @ 2015-03-10 15:37 UTC (permalink / raw)
  To: Namhyung Kim; +Cc: Andi Kleen, acme, jolsa, linux-kernel

> Why not handle both cases here?
> 
> static void print_running(u64 run, u64 ena)
> {
> 	if (csv_output)
> 		fprintf(output, ...);
> 	else if (run != ena)
> 		fprintf(output, ...);
> }

print_running has 6 callers. run != ena is only needed
for two of them. So I don't think it makes sense to do.

-Andi

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode
  2015-03-10 15:37     ` Andi Kleen
@ 2015-03-11  0:52       ` Namhyung Kim
  0 siblings, 0 replies; 16+ messages in thread
From: Namhyung Kim @ 2015-03-11  0:52 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Andi Kleen, acme, jolsa, linux-kernel

On Tue, Mar 10, 2015 at 08:37:04AM -0700, Andi Kleen wrote:
> > Why not handle both cases here?
> > 
> > static void print_running(u64 run, u64 ena)
> > {
> > 	if (csv_output)
> > 		fprintf(output, ...);
> > 	else if (run != ena)
> > 		fprintf(output, ...);
> > }
> 
> print_running has 6 callers. run != ena is only needed
> for two of them. So I don't think it makes sense to do.

Those 6 are from print_aggr(), print_counter_aggr() and
print_counter().  They all have two branch - one is for no-scaling or
not-supported counter (I guess run or ena being 0 goes to this case),
another is scaling case - so IMHO print_counter_aggr() should check
the run and the ena in this case too.

The former can call print_running() to print empty column to CSV and
discard normal (scaling) output.  The latter also can call
print_running() to print for both output.

So by using print_running(), we can enforce same check to all cases
and reduce code duplication also IMHO.

Thanks,
Namhyung

> 
> -Andi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: A number of perf stat improvements
  2015-03-08 23:55 A number of perf stat improvements Andi Kleen
                   ` (3 preceding siblings ...)
  2015-03-08 23:55 ` [PATCH 4/4] perf, tools, stat: Always correctly indent ratio column Andi Kleen
@ 2015-03-11 13:35 ` Arnaldo Carvalho de Melo
  2015-03-11 15:29   ` Andi Kleen
  4 siblings, 1 reply; 16+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-03-11 13:35 UTC (permalink / raw)
  To: Andi Kleen; +Cc: jolsa, linux-kernel

Em Sun, Mar 08, 2015 at 04:55:20PM -0700, Andi Kleen escreveu:
> Here are a number of perf stat improvements I had lying around
> in various branches. Two are bug fixes, and two others are 
> (minor) new features.

So none of these patches are applying since I skipped the first due to
Jiri's comments, that were not addressed.

- Arnaldo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: A number of perf stat improvements
  2015-03-11 13:35 ` A number of perf stat improvements Arnaldo Carvalho de Melo
@ 2015-03-11 15:29   ` Andi Kleen
  2015-03-11 19:13     ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 16+ messages in thread
From: Andi Kleen @ 2015-03-11 15:29 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: Andi Kleen, jolsa, linux-kernel

On Wed, Mar 11, 2015 at 10:35:19AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Mar 08, 2015 at 04:55:20PM -0700, Andi Kleen escreveu:
> > Here are a number of perf stat improvements I had lying around
> > in various branches. Two are bug fixes, and two others are 
> > (minor) new features.
> 
> So none of these patches are applying since I skipped the first due to
> Jiri's comments, that were not addressed.

There was nothing to address. Anyways I resent rebased without that
patch. If you don't want to fix bugs I'm not gonna bother with it
either.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: A number of perf stat improvements
  2015-03-11 15:29   ` Andi Kleen
@ 2015-03-11 19:13     ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 16+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-03-11 19:13 UTC (permalink / raw)
  To: Andi Kleen; +Cc: jolsa, linux-kernel

Em Wed, Mar 11, 2015 at 04:29:49PM +0100, Andi Kleen escreveu:
> On Wed, Mar 11, 2015 at 10:35:19AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sun, Mar 08, 2015 at 04:55:20PM -0700, Andi Kleen escreveu:
> > > Here are a number of perf stat improvements I had lying around
> > > in various branches. Two are bug fixes, and two others are 
> > > (minor) new features.
> > 
> > So none of these patches are applying since I skipped the first due to
> > Jiri's comments, that were not addressed.
> 
> There was nothing to address. Anyways I resent rebased without that
> patch. If you don't want to fix bugs I'm not gonna bother with it
> either.

Thank you.

- Arnaldo

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2015-03-11 19:13 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-08 23:55 A number of perf stat improvements Andi Kleen
2015-03-08 23:55 ` [PATCH 1/4] perf, tools: Output running time and run/enabled ratio in CSV mode Andi Kleen
2015-03-09  7:49   ` Jiri Olsa
2015-03-09 14:04     ` Andi Kleen
2015-03-10  7:15   ` Namhyung Kim
2015-03-10 15:37     ` Andi Kleen
2015-03-11  0:52       ` Namhyung Kim
2015-03-08 23:55 ` [PATCH 2/4] perf, tools: Fix metrics calculation with event qualifiers Andi Kleen
2015-03-09  7:55   ` Jiri Olsa
2015-03-08 23:55 ` [PATCH 3/4] perf, tools, stat: Fix IPC and other formulas with -A Andi Kleen
2015-03-09  8:11   ` Jiri Olsa
2015-03-08 23:55 ` [PATCH 4/4] perf, tools, stat: Always correctly indent ratio column Andi Kleen
2015-03-09  8:24   ` Jiri Olsa
2015-03-11 13:35 ` A number of perf stat improvements Arnaldo Carvalho de Melo
2015-03-11 15:29   ` Andi Kleen
2015-03-11 19:13     ` Arnaldo Carvalho de Melo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).