[PATCH V2] perf/x86/intel/uncore: Fix client IMC events return huge result

[PATCH V2] perf/x86/intel/uncore: Fix client IMC events return huge result

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

The client IMC bandwidth events return very huge result.
  perf stat -e uncore_imc/data_reads/ -e uncore_imc/data_writes/ -I
  10000 -a
  10.000117222 34,788.76 MiB uncore_imc/data_reads/
  10.000117222 8.26 MiB uncore_imc/data_writes/
  20.000374584 34,842.89 MiB uncore_imc/data_reads/
  20.000374584 10.45 MiB uncore_imc/data_writes/
  30.000633299 37,965.29 MiB uncore_imc/data_reads/
  30.000633299 323.62 MiB uncore_imc/data_writes/
  40.000891548 41,012.88 MiB uncore_imc/data_reads/
  40.000891548 6.98 MiB uncore_imc/data_writes/
  50.001142480 1,125,899,906,621,494.75 MiB uncore_imc/data_reads/
  50.001142480 6.97 MiB uncore_imc/data_writes/

The client IMC events are freerunning counters. They still use the
old event encoding format (0x1 for data_read and 0x2 for data write).
The counter bit width is calculated by common code, which assume that
the standard encoding format is used for the freerunning counters.
Error bit width information is calculated.

The event->attr.config, which directly from user space, should not be
used by the functions of freerunning counters.
For client IMC events, the attr.config needs to be converted to the
standard encoding format. The modified event config will be stored in
event->hw.config.
For other freerunning counters, the attr.config has the correct format.
Just save it in event->hw.config.
Using event->hw.config to replace event->attr.config for the functions
of freerunning counters.

Fixes: commit 9aae1780e7e8 ("perf/x86/intel/uncore: Clean up client IMC uncore")
Reported-by: Jin Yao <yao.jin@linux.intel.com>
Tested-by: Jin Yao <yao.jin@linux.intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Cc: stable@kernel.org
---
V2:
- Fix the "Fixes" tag.
- Add Cc: stable@kernel.org

 arch/x86/events/intel/uncore.c     |  1 +
 arch/x86/events/intel/uncore.h     | 12 ++++++------
 arch/x86/events/intel/uncore_snb.c |  4 +++-
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 27a461414b30..2690135bf83f 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -740,6 +740,7 @@ static int uncore_pmu_event_init(struct perf_event *event)
 		/* fixed counters have event field hardcoded to zero */
 		hwc->config = 0ULL;
 	} else if (is_freerunning_event(event)) {
+		hwc->config = event->attr.config;
 		if (!check_valid_freerunning_event(box, event))
 			return -EINVAL;
 		event->hw.idx = UNCORE_PMC_IDX_FREERUNNING;
diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
index e17ab885b1e9..cc6dd4f78158 100644
--- a/arch/x86/events/intel/uncore.h
+++ b/arch/x86/events/intel/uncore.h
@@ -285,8 +285,8 @@ static inline
 unsigned int uncore_freerunning_counter(struct intel_uncore_box *box,
 					struct perf_event *event)
 {
-	unsigned int type = uncore_freerunning_type(event->attr.config);
-	unsigned int idx = uncore_freerunning_idx(event->attr.config);
+	unsigned int type = uncore_freerunning_type(event->hw.config);
+	unsigned int idx = uncore_freerunning_idx(event->hw.config);
 	struct intel_uncore_pmu *pmu = box->pmu;
 
 	return pmu->type->freerunning[type].counter_base +
@@ -360,7 +360,7 @@ static inline
 unsigned int uncore_freerunning_bits(struct intel_uncore_box *box,
 				     struct perf_event *event)
 {
-	unsigned int type = uncore_freerunning_type(event->attr.config);
+	unsigned int type = uncore_freerunning_type(event->hw.config);
 
 	return box->pmu->type->freerunning[type].bits;
 }
@@ -368,7 +368,7 @@ unsigned int uncore_freerunning_bits(struct intel_uncore_box *box,
 static inline int uncore_num_freerunning(struct intel_uncore_box *box,
 					 struct perf_event *event)
 {
-	unsigned int type = uncore_freerunning_type(event->attr.config);
+	unsigned int type = uncore_freerunning_type(event->hw.config);
 
 	return box->pmu->type->freerunning[type].num_counters;
 }
@@ -382,8 +382,8 @@ static inline int uncore_num_freerunning_types(struct intel_uncore_box *box,
 static inline bool check_valid_freerunning_event(struct intel_uncore_box *box,
 						 struct perf_event *event)
 {
-	unsigned int type = uncore_freerunning_type(event->attr.config);
-	unsigned int idx = uncore_freerunning_idx(event->attr.config);
+	unsigned int type = uncore_freerunning_type(event->hw.config);
+	unsigned int idx = uncore_freerunning_idx(event->hw.config);
 
 	return (type < uncore_num_freerunning_types(box, event)) &&
 	       (idx < uncore_num_freerunning(box, event));
diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
index 8527c3e1038b..48d7121f71c7 100644
--- a/arch/x86/events/intel/uncore_snb.c
+++ b/arch/x86/events/intel/uncore_snb.c
@@ -425,9 +425,11 @@ static int snb_uncore_imc_event_init(struct perf_event *event)
 
 	/* must be done before validate_group */
 	event->hw.event_base = base;
-	event->hw.config = cfg;
 	event->hw.idx = idx;
 
+	/* Convert to standard encoding format for free running counters */
+	event->hw.config = ((cfg - 1) << 8) | 0x10ff;
+
 	/* no group validation needed, we have free running counters */
 
 	return 0;
-- 
2.14.3

Re: [PATCH V2] perf/x86/intel/uncore: Fix client IMC events return huge result

[Peter Zijlstra]

On Fri, Nov 16, 2018 at 07:20:55AM -0800, kan.liang@linux.intel.com wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> The client IMC bandwidth events return very huge result.
>   perf stat -e uncore_imc/data_reads/ -e uncore_imc/data_writes/ -I
>   10000 -a
>   10.000117222 34,788.76 MiB uncore_imc/data_reads/
>   10.000117222 8.26 MiB uncore_imc/data_writes/
>   20.000374584 34,842.89 MiB uncore_imc/data_reads/
>   20.000374584 10.45 MiB uncore_imc/data_writes/
>   30.000633299 37,965.29 MiB uncore_imc/data_reads/
>   30.000633299 323.62 MiB uncore_imc/data_writes/
>   40.000891548 41,012.88 MiB uncore_imc/data_reads/
>   40.000891548 6.98 MiB uncore_imc/data_writes/
>   50.001142480 1,125,899,906,621,494.75 MiB uncore_imc/data_reads/
>   50.001142480 6.97 MiB uncore_imc/data_writes/
> 
> The client IMC events are freerunning counters. They still use the
> old event encoding format (0x1 for data_read and 0x2 for data write).
> The counter bit width is calculated by common code, which assume that
> the standard encoding format is used for the freerunning counters.
> Error bit width information is calculated.

So far so good; some client IMC events have a different format and need
converting.

> The event->attr.config, which directly from user space, should not be
> used by the functions of freerunning counters.
> For client IMC events, the attr.config needs to be converted to the
> standard encoding format. The modified event config will be stored in
> event->hw.config.
> For other freerunning counters, the attr.config has the correct format.
> Just save it in event->hw.config.
> Using event->hw.config to replace event->attr.config for the functions
> of freerunning counters.

This above section seems unclear/confusing at best. The first sentence
is actively wrong; it lost the 'client IMC' specification.

Please restructure.