[PATCH 0/4] Attempt to cleanup the HSW offcore bits

[PATCH 0/4] Attempt to cleanup the HSW offcore bits

[Peter Zijlstra]

So Don asked about offcore and because I forgot I looked at the code and found
the terrible mess Andi created with the HSW/BDW bits.

This series attempts to clean some of that up but seeing how it was all magic
numbers and no reasons provided for the differences with existing uarchs this
might just break things horribly.

That said, if this does break things, fixes will have to explain things, so
that's good.

[PATCH 1/4] perf,x86: De-obfuscate HSW offcore bits

[Peter Zijlstra]

[-- Attachment #1: peterz-hsw-offcore-fix-1.patch --]
[-- Type: text/plain, Size: 3471 bytes --]

Andi introduced the HSW cache events array, but used magic constants
against convention as set by all the other uarchs. Try and deobfuscate
these a bit.

This patch should not change the actual values generated; however
weird they seems.

In that patch Andi also said there were differences between the
SNB/IVB and HSW/BDW offcore tables but failed to specify which and
why.

Fixes: 86a349a28b24 ("perf/x86/intel: Add Broadwell core support")
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: http://lkml.kernel.org/n/tip-@git.kernel.org
---
 arch/x86/kernel/cpu/perf_event_intel.c |   48 +++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 8 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -247,6 +247,7 @@ static u64 intel_pmu_event_map(int hw_ev
 #define SNB_BUS_LOCKS		(1ULL << 10)
 #define SNB_STRM_ST		(1ULL << 11)
 #define SNB_OTHER		(1ULL << 15)
+
 #define SNB_RESP_ANY		(1ULL << 16)
 #define SNB_NO_SUPP		(1ULL << 17)
 #define SNB_LLC_HITM		(1ULL << 18)
@@ -255,6 +256,7 @@ static u64 intel_pmu_event_map(int hw_ev
 #define SNB_LLC_HITF		(1ULL << 21)
 #define SNB_LOCAL		(1ULL << 22)
 #define SNB_REMOTE		(0xffULL << 23)
+
 #define SNB_SNP_NONE		(1ULL << 31)
 #define SNB_SNP_NOT_NEEDED	(1ULL << 32)
 #define SNB_SNP_MISS		(1ULL << 33)
@@ -519,6 +521,40 @@ static __initconst const u64 hsw_hw_cach
  },
 };
 
+/* HSW Request type */
+#define HSW_DMND_DATA_RD	(1ULL << 0)
+#define HSW_DMND_RFO		(1ULL << 1)
+#define HSW_DMND_IFETCH		(1ULL << 2)
+
+#define HSW_PF_DATA_RD		(1ULL << 4)
+#define HSW_PF_RFO		(1ULL << 5)
+#define HSW_PF_IFETCH		(1ULL << 6)
+
+#define HSW_OTHER		(1ULL << 15)
+
+/* HSW Supplier info := SNB Supplier info */
+/* HSW Snoop Info := SNB Snoop Info */
+
+/*
+ * SNB_LLC_* is specified as 'reserved' in the SDM
+ * but Andi added it anyhow, suggesting HSW/BDW have it just fine
+ *
+ * XXX NHM/WSM/SNB/IVB don't have the PF/IFETCH bits set
+ */
+#define HSW_DMND_READ		(HSW_DMND_DATA_RD|HSW_DMND_IFETCH| \
+				 HSW_PF_DATA_RD|HSW_PF_IFETCH| \
+				 SNB_LLC_DATA_RD|SNB_LLC_IFETCH)
+
+/* XXX NHM/WSM/SNB/IVB dont have the PF bits set */
+#define HSW_DMND_WRITE		(HSW_DMND_RFO|HSW_PF_RFO|SNB_LLC_RFO)
+
+#define HSW_DMND_PREFETCH	(0) /* XXX broken ? */
+
+#define HSW_DRAM_ANY		(SNB_NO_SUPP|SNB_SNP_ANY|(0x78ULL << 23)) /* WTF */
+
+#define HSW_L3_ACCESS		(0) /* XXX no supplier! */
+#define NSW_L3_MISS		(HSW_DRAM_ANY|SNB_NON_DRAM)
+
 static __initconst const u64 hsw_hw_cache_extra_regs
 				[PERF_COUNT_HW_CACHE_MAX]
 				[PERF_COUNT_HW_CACHE_OP_MAX]
@@ -526,16 +562,12 @@ static __initconst const u64 hsw_hw_cach
 {
  [ C(LL  ) ] = {
 	[ C(OP_READ) ] = {
-		/* OFFCORE_RESPONSE:ALL_DATA_RD|ALL_CODE_RD */
-		[ C(RESULT_ACCESS) ] = 0x2d5,
-		/* OFFCORE_RESPONSE:ALL_DATA_RD|ALL_CODE_RD|SUPPLIER_NONE|
-                   L3_MISS|ANY_SNOOP */
-		[ C(RESULT_MISS)   ] = 0x3fbc0202d5ull,
+		[ C(RESULT_ACCESS) ] = HSW_DMND_READ|HSW_L3_ACCESS,
+		[ C(RESULT_MISS)   ] = HSW_DMND_READ|HSW_L3_MISS,
 	},
 	[ C(OP_WRITE) ] = {
-		[ C(RESULT_ACCESS) ] = 0x122, 	/* OFFCORE_RESPONSE:ALL_RFO */
-		/* OFFCORE_RESPONSE:ALL_RFO|SUPPLIER_NONE|L3_MISS|ANY_SNOOP */
-		[ C(RESULT_MISS)   ] = 0x3fbc020122ull,
+		[ C(RESULT_ACCESS) ] = HSW_DMND_WRITE|HSW_L3_ACCESS,
+		[ C(RESULT_MISS)   ] = HSW_DMND_WRITE|HSW_L3_MISS,
 	},
 	[ C(OP_PREFETCH) ] = {
 		[ C(RESULT_ACCESS) ] = 0x0,

[PATCH 2/4] perf,x86: HSW offcore prefetch events

[Peter Zijlstra]

[-- Attachment #1: peterz-hsw-offcore-fix-2.patch --]
[-- Type: text/plain, Size: 1711 bytes --]

This changes the HSW events to be more inline with the other uarchs
and removes the prefetch request bits from the read/write demands and
into the prefetch demand.

Cc: Andi Kleen <ak@linux.intel.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: http://lkml.kernel.org/n/tip-@git.kernel.org
---
 arch/x86/kernel/cpu/perf_event_intel.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -539,16 +539,12 @@ static __initconst const u64 hsw_hw_cach
  * SNB_LLC_* is specified as 'reserved' in the SDM
  * but Andi added it anyhow, suggesting HSW/BDW have it just fine
  *
- * XXX NHM/WSM/SNB/IVB don't have the PF/IFETCH bits set
+ * XXX NHM/WSM/SNB/IVB don't have the IFETCH bits set
  */
 #define HSW_DMND_READ		(HSW_DMND_DATA_RD|HSW_DMND_IFETCH| \
-				 HSW_PF_DATA_RD|HSW_PF_IFETCH| \
 				 SNB_LLC_DATA_RD|SNB_LLC_IFETCH)
-
-/* XXX NHM/WSM/SNB/IVB dont have the PF bits set */
-#define HSW_DMND_WRITE		(HSW_DMND_RFO|HSW_PF_RFO|SNB_LLC_RFO)
-
-#define HSW_DMND_PREFETCH	(0) /* XXX broken ? */
+#define HSW_DMND_WRITE		(HSW_DMND_RFO|SNB_LLC_RFO)
+#define HSW_DMND_PREFETCH	(HSW_PF_DATA_RD|HSW_PF_RFO|HSW_PF_IFETCH)
 
 #define HSW_DRAM_ANY		(SNB_NO_SUPP|SNB_SNP_ANY|(0x78ULL << 23)) /* WTF */
 
@@ -570,8 +566,8 @@ static __initconst const u64 hsw_hw_cach
 		[ C(RESULT_MISS)   ] = HSW_DMND_WRITE|HSW_L3_MISS,
 	},
 	[ C(OP_PREFETCH) ] = {
-		[ C(RESULT_ACCESS) ] = 0x0,
-		[ C(RESULT_MISS)   ] = 0x0,
+		[ C(RESULT_ACCESS) ] = HSW_DMND_PREFETCH|HSW_L3_ACCESS,
+		[ C(RESULT_MISS)   ] = HSW_DMND_PREFETCH|HSW_L3_MISS,
 	},
  },
 };

[PATCH 3/4] perf,x86: Attempt to sanitize the HSW supplier info

[Peter Zijlstra]

[-- Attachment #1: peterz-hsw-offcore-fix-3.patch --]
[-- Type: text/plain, Size: 2172 bytes --]

The SDM states the HSW/BDW supplier and snoop info are identical to
SNB/IVB, make it so.

Furthermore, it states you have to minimally program a request and
supplier type, but the current code does not set a supplier for the
ACCESS events.

This significantly alters the actual events and does away with some of
the weirdness found by decoding the magic numbers. If this breaks
things someone needs to go explain things and augment the SDM.

Cc: Andi Kleen <ak@linux.intel.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: http://lkml.kernel.org/n/tip-@git.kernel.org
---
 arch/x86/kernel/cpu/perf_event_intel.c |   17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -544,11 +544,6 @@ static __initconst const u64 hsw_hw_cach
 #define HSW_DMND_WRITE		(HSW_DMND_RFO|SNB_LLC_RFO)
 #define HSW_DMND_PREFETCH	(HSW_PF_DATA_RD|HSW_PF_RFO|HSW_PF_IFETCH)
 
-#define HSW_DRAM_ANY		(SNB_NO_SUPP|SNB_SNP_ANY|(0x78ULL << 23)) /* WTF */
-
-#define HSW_L3_ACCESS		(0) /* XXX no supplier! */
-#define NSW_L3_MISS		(HSW_DRAM_ANY|SNB_NON_DRAM)
-
 static __initconst const u64 hsw_hw_cache_extra_regs
 				[PERF_COUNT_HW_CACHE_MAX]
 				[PERF_COUNT_HW_CACHE_OP_MAX]
@@ -556,16 +551,16 @@ static __initconst const u64 hsw_hw_cach
 {
  [ C(LL  ) ] = {
 	[ C(OP_READ) ] = {
-		[ C(RESULT_ACCESS) ] = HSW_DMND_READ|HSW_L3_ACCESS,
-		[ C(RESULT_MISS)   ] = HSW_DMND_READ|HSW_L3_MISS,
+		[ C(RESULT_ACCESS) ] = HSW_DMND_READ|SNB_L3_ACCESS,
+		[ C(RESULT_MISS)   ] = HSW_DMND_READ|SNB_L3_MISS,
 	},
 	[ C(OP_WRITE) ] = {
-		[ C(RESULT_ACCESS) ] = HSW_DMND_WRITE|HSW_L3_ACCESS,
-		[ C(RESULT_MISS)   ] = HSW_DMND_WRITE|HSW_L3_MISS,
+		[ C(RESULT_ACCESS) ] = HSW_DMND_WRITE|SNB_L3_ACCESS,
+		[ C(RESULT_MISS)   ] = HSW_DMND_WRITE|SNB_L3_MISS,
 	},
 	[ C(OP_PREFETCH) ] = {
-		[ C(RESULT_ACCESS) ] = HSW_DMND_PREFETCH|HSW_L3_ACCESS,
-		[ C(RESULT_MISS)   ] = HSW_DMND_PREFETCH|HSW_L3_MISS,
+		[ C(RESULT_ACCESS) ] = HSW_DMND_PREFETCH|SNB_L3_ACCESS,
+		[ C(RESULT_MISS)   ] = HSW_DMND_PREFETCH|SNB_L3_MISS,
 	},
  },
 };

[PATCH 4/4] perf,x86: Introduce HSW cache numa events

[Peter Zijlstra]

[-- Attachment #1: peterz-hsw-offcore-node.patch --]
[-- Type: text/plain, Size: 1164 bytes --]

Seeing how HSW-EP is now available and the SDM states the supplier and
snoop info is identical to SNB/IVB, provide the cache numa events.

Cc: Andi Kleen <ak@linux.intel.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: http://lkml.kernel.org/n/tip-@git.kernel.org
---
 arch/x86/kernel/cpu/perf_event_intel.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -563,6 +563,20 @@ static __initconst const u64 hsw_hw_cach
 		[ C(RESULT_MISS)   ] = HSW_DMND_PREFETCH|SNB_L3_MISS,
 	},
  },
+ [ C(NODE) ] = {
+	[ C(OP_READ) ] = {
+		[ C(RESULT_ACCESS) ] = HSW_DMND_READ|SNB_DRAM_ANY,
+		[ C(RESULT_MISS)   ] = HSW_DMND_READ|SNB_DRAM_REMOTE,
+	},
+	[ C(OP_WRITE) ] = {
+		[ C(RESULT_ACCESS) ] = HSW_DMND_WRITE|SNB_DRAM_ANY,
+		[ C(RESULT_MISS)   ] = HSW_DMND_WRITE|SNB_DRAM_REMOTE,
+	},
+	[ C(OP_PREFETCH) ] = {
+		[ C(RESULT_ACCESS) ] = HSW_DMND_PREFETCH|SNB_DRAM_ANY,
+		[ C(RESULT_MISS)   ] = HSW_DMND_PREFETCH|SNB_DRAM_REMOTE,
+	},
+ },
 };
 
 static __initconst const u64 westmere_hw_cache_event_ids

Re: [PATCH 1/4] perf,x86: De-obfuscate HSW offcore bits

[Borislav Petkov]

On Thu, Oct 23, 2014 at 12:51:20PM +0200, Peter Zijlstra wrote:
> Andi introduced the HSW cache events array, but used magic constants
> against convention as set by all the other uarchs. Try and deobfuscate
> these a bit.
> 
> This patch should not change the actual values generated; however
> weird they seems.
> 
> In that patch Andi also said there were differences between the
> SNB/IVB and HSW/BDW offcore tables but failed to specify which and
> why.
> 
> Fixes: 86a349a28b24 ("perf/x86/intel: Add Broadwell core support")
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Stephane Eranian <eranian@google.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: http://lkml.kernel.org/n/tip-@git.kernel.org

This link looks funny.

> ---
>  arch/x86/kernel/cpu/perf_event_intel.c |   48 +++++++++++++++++++++++++++------
>  1 file changed, 40 insertions(+), 8 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -247,6 +247,7 @@ static u64 intel_pmu_event_map(int hw_ev
>  #define SNB_BUS_LOCKS		(1ULL << 10)
>  #define SNB_STRM_ST		(1ULL << 11)
>  #define SNB_OTHER		(1ULL << 15)
> +
>  #define SNB_RESP_ANY		(1ULL << 16)
>  #define SNB_NO_SUPP		(1ULL << 17)
>  #define SNB_LLC_HITM		(1ULL << 18)
> @@ -255,6 +256,7 @@ static u64 intel_pmu_event_map(int hw_ev
>  #define SNB_LLC_HITF		(1ULL << 21)
>  #define SNB_LOCAL		(1ULL << 22)
>  #define SNB_REMOTE		(0xffULL << 23)
> +
>  #define SNB_SNP_NONE		(1ULL << 31)
>  #define SNB_SNP_NOT_NEEDED	(1ULL << 32)
>  #define SNB_SNP_MISS		(1ULL << 33)
> @@ -519,6 +521,40 @@ static __initconst const u64 hsw_hw_cach
>   },
>  };
>  
> +/* HSW Request type */
> +#define HSW_DMND_DATA_RD	(1ULL << 0)
> +#define HSW_DMND_RFO		(1ULL << 1)
> +#define HSW_DMND_IFETCH		(1ULL << 2)
> +
> +#define HSW_PF_DATA_RD		(1ULL << 4)
> +#define HSW_PF_RFO		(1ULL << 5)
> +#define HSW_PF_IFETCH		(1ULL << 6)
> +
> +#define HSW_OTHER		(1ULL << 15)
> +
> +/* HSW Supplier info := SNB Supplier info */
> +/* HSW Snoop Info := SNB Snoop Info */
> +
> +/*
> + * SNB_LLC_* is specified as 'reserved' in the SDM
> + * but Andi added it anyhow, suggesting HSW/BDW have it just fine
> + *
> + * XXX NHM/WSM/SNB/IVB don't have the PF/IFETCH bits set
> + */
> +#define HSW_DMND_READ		(HSW_DMND_DATA_RD|HSW_DMND_IFETCH| \
> +				 HSW_PF_DATA_RD|HSW_PF_IFETCH| \
> +				 SNB_LLC_DATA_RD|SNB_LLC_IFETCH)
> +
> +/* XXX NHM/WSM/SNB/IVB dont have the PF bits set */
> +#define HSW_DMND_WRITE		(HSW_DMND_RFO|HSW_PF_RFO|SNB_LLC_RFO)
> +
> +#define HSW_DMND_PREFETCH	(0) /* XXX broken ? */
> +
> +#define HSW_DRAM_ANY		(SNB_NO_SUPP|SNB_SNP_ANY|(0x78ULL << 23)) /* WTF */
> +
> +#define HSW_L3_ACCESS		(0) /* XXX no supplier! */
> +#define NSW_L3_MISS		(HSW_DRAM_ANY|SNB_NON_DRAM)

You probably mean HSW and not NSW here...

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 1/4] perf,x86: De-obfuscate HSW offcore bits

[Peter Zijlstra]

On Thu, Oct 23, 2014 at 02:16:19PM +0200, Borislav Petkov wrote:
> On Thu, Oct 23, 2014 at 12:51:20PM +0200, Peter Zijlstra wrote:
> > Andi introduced the HSW cache events array, but used magic constants
> > against convention as set by all the other uarchs. Try and deobfuscate
> > these a bit.
> > 
> > This patch should not change the actual values generated; however
> > weird they seems.
> > 
> > In that patch Andi also said there were differences between the
> > SNB/IVB and HSW/BDW offcore tables but failed to specify which and
> > why.
> > 
> > Fixes: 86a349a28b24 ("perf/x86/intel: Add Broadwell core support")
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Cc: Stephane Eranian <eranian@google.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Link: http://lkml.kernel.org/n/tip-@git.kernel.org
> 
> This link looks funny.

Yes it does, I seem to have not isntalled rndpwd or whatever that script
is using. Also I usually grep -v "^Link:" .mbox files before sending,
guess what I forgot :-)

Re: [PATCH 0/4] Attempt to cleanup the HSW offcore bits

[Andi Kleen]

On Thu, Oct 23, 2014 at 12:51:19PM +0200, Peter Zijlstra wrote:
> So Don asked about offcore and because I forgot I looked at the code and found
> the terrible mess Andi created with the HSW/BDW bits.
> 
> This series attempts to clean some of that up but seeing how it was all magic
> numbers 

All the bits are documented. The actual definitions are available
in the JSON offcore definitions at https://download.01.org/perfmon/

> and no reasons provided for the differences with existing uarchs this
> might just break things horribly.
> That said, if this does break things, fixes will have to explain things, so
> that's good.

I trust you won't submit or merge untested patches.

-Andi

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

Re: [PATCH 0/4] Attempt to cleanup the HSW offcore bits

[Peter Zijlstra]

On Mon, Oct 27, 2014 at 01:23:40PM +0100, Andi Kleen wrote:
> On Thu, Oct 23, 2014 at 12:51:19PM +0200, Peter Zijlstra wrote:
> > So Don asked about offcore and because I forgot I looked at the code and found
> > the terrible mess Andi created with the HSW/BDW bits.
> > 
> > This series attempts to clean some of that up but seeing how it was all magic
> > numbers 
> 
> All the bits are documented. The actual definitions are available
> in the JSON offcore definitions at https://download.01.org/perfmon/

Yeah, no. That's not how we write code. Also, there's no actual JSON
offcore file for HSW only some TSV file, and I've no mind to go decode
and reverse engineer that stuff.

Also, semi unreadable files on a weird web location is not
documentation, the SDM is and the SDM does not explain why you didn't
put the PF request bits in the OP_PREFETCH events, nor why you added the
IFETCH bits to the OP_READ where all the other uarchs didn't.

Nor does it explain why you set bits 7,9 in OP_READ even though they're
listed as 'reserved'.

It also doesn't explain why you don't program a Supplier Info for
RESULT_ACCESS.

Nor why you set 0x78 in bits 23:30, which even for SNB are still listed
as reserved, even though its official offcore events have that set to
0xFF for all remote events.

And lacking SDM you should have explained all that in your Changelog,
but that's also entirely devoid of any usable information.

> > and no reasons provided for the differences with existing uarchs this
> > might just break things horribly.
> > That said, if this does break things, fixes will have to explain things, so
> > that's good.
> 
> I trust you won't submit or merge untested patches.

You trust wrong.. I should never have merged your patches, but seeing
they're in it needed cleaning up.

I can revert your patches if you'd rather have that?

Re: [PATCH 0/4] Attempt to cleanup the HSW offcore bits

[Andi Kleen]

On Mon, Oct 27, 2014 at 02:07:12PM +0100, Peter Zijlstra wrote:
> On Mon, Oct 27, 2014 at 01:23:40PM +0100, Andi Kleen wrote:
> > On Thu, Oct 23, 2014 at 12:51:19PM +0200, Peter Zijlstra wrote:
> > > So Don asked about offcore and because I forgot I looked at the code and found
> > > the terrible mess Andi created with the HSW/BDW bits.
> > > 
> > > This series attempts to clean some of that up but seeing how it was all magic
> > > numbers 
> > 
> > All the bits are documented. The actual definitions are available
> > in the JSON offcore definitions at https://download.01.org/perfmon/
> 
> Yeah, no. That's not how we write code. Also, there's no actual JSON
> offcore file for HSW only some TSV file, and I've no mind to go decode

https://download.01.org/perfmon/HSW/Haswell_matrix_V14.json
https://download.01.org/perfmon/HSW/Haswell_matrix_bit_definitions_V14.json

-Andi

Re: [PATCH 0/4] Attempt to cleanup the HSW offcore bits

[Peter Zijlstra]

On Mon, Oct 27, 2014 at 07:28:18AM -0700, Andi Kleen wrote:
> On Mon, Oct 27, 2014 at 02:07:12PM +0100, Peter Zijlstra wrote:
> > On Mon, Oct 27, 2014 at 01:23:40PM +0100, Andi Kleen wrote:
> > > On Thu, Oct 23, 2014 at 12:51:19PM +0200, Peter Zijlstra wrote:
> > > > So Don asked about offcore and because I forgot I looked at the code and found
> > > > the terrible mess Andi created with the HSW/BDW bits.
> > > > 
> > > > This series attempts to clean some of that up but seeing how it was all magic
> > > > numbers 
> > > 
> > > All the bits are documented. The actual definitions are available
> > > in the JSON offcore definitions at https://download.01.org/perfmon/
> > 
> > Yeah, no. That's not how we write code. Also, there's no actual JSON
> > offcore file for HSW only some TSV file, and I've no mind to go decode
> 
> https://download.01.org/perfmon/HSW/Haswell_matrix_V14.json
> https://download.01.org/perfmon/HSW/Haswell_matrix_bit_definitions_V14.json

So you expected me to somehow magically know it was not the file with
offcore in its name then?

Re: [PATCH 0/4] Attempt to cleanup the HSW offcore bits

[Thomas Gleixner]

Andi,

On Mon, 27 Oct 2014, Andi Kleen wrote:
> On Mon, Oct 27, 2014 at 02:07:12PM +0100, Peter Zijlstra wrote:
> > On Mon, Oct 27, 2014 at 01:23:40PM +0100, Andi Kleen wrote:
> > > On Thu, Oct 23, 2014 at 12:51:19PM +0200, Peter Zijlstra wrote:
> > > > So Don asked about offcore and because I forgot I looked at the code and found
> > > > the terrible mess Andi created with the HSW/BDW bits.
> > > > 
> > > > This series attempts to clean some of that up but seeing how it was all magic
> > > > numbers 
> > > 
> > > All the bits are documented. The actual definitions are available
> > > in the JSON offcore definitions at https://download.01.org/perfmon/
> > 
> > Yeah, no. That's not how we write code. Also, there's no actual JSON
> > offcore file for HSW only some TSV file, and I've no mind to go decode
> 
> https://download.01.org/perfmon/HSW/Haswell_matrix_V14.json
> https://download.01.org/perfmon/HSW/Haswell_matrix_bit_definitions_V14.json

Of course you did not answer any of the other legitimate questions
Peter brought up.

Care to answer them w/o pointing to magic json files which lack ANY
useful information about the magic bits they provide? And why they are
not consistent with the SDM?

Either you come forth with reasonable explanations or I'm going to rip
out the mess you created even before Peter can persuade himself to do
so. 

Thanks,

	tglx

Re: [PATCH 0/4] Attempt to cleanup the HSW offcore bits

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> Andi,
> 
> On Mon, 27 Oct 2014, Andi Kleen wrote:
> > On Mon, Oct 27, 2014 at 02:07:12PM +0100, Peter Zijlstra wrote:
> > > On Mon, Oct 27, 2014 at 01:23:40PM +0100, Andi Kleen wrote:
> > > > On Thu, Oct 23, 2014 at 12:51:19PM +0200, Peter Zijlstra wrote:
> > > > > So Don asked about offcore and because I forgot I looked at the code and found
> > > > > the terrible mess Andi created with the HSW/BDW bits.
> > > > > 
> > > > > This series attempts to clean some of that up but seeing how it was all magic
> > > > > numbers 
> > > > 
> > > > All the bits are documented. The actual definitions are available
> > > > in the JSON offcore definitions at https://download.01.org/perfmon/
> > > 
> > > Yeah, no. That's not how we write code. Also, there's no actual JSON
> > > offcore file for HSW only some TSV file, and I've no mind to go decode
> > 
> > https://download.01.org/perfmon/HSW/Haswell_matrix_V14.json
> > https://download.01.org/perfmon/HSW/Haswell_matrix_bit_definitions_V14.json
> 
> Of course you did not answer any of the other legitimate 
> questions Peter brought up.
> 
> Care to answer them w/o pointing to magic json files which lack 
> ANY useful information about the magic bits they provide? And 
> why they are not consistent with the SDM?
> 
> Either you come forth with reasonable explanations or I'm going 
> to rip out the mess you created even before Peter can persuade 
> himself to do so.

So I waited a week in the hope that all this can be resolved 
quickly, but the problem is that we are already at -rc3 and we 
are running out of time and I'm not seeing constructive behavior 
from Andi.

So I've done a straightforward revert of the messy Broadwell 
client driver changes, to bring us back to the v3.17 baseline:

  1776b10627e4 ("perf/x86/intel: Revert incomplete and undocumented Broadwell client support")

Andi, please rework the series on top of that and also 
proactively integrate the cleanups, requests and observations 
from Thomas Gleixner and Peter Zijlstra. Only submit a new series 
once all feedback given to you in the past has been addressed to 
the fullest!

If the patches are fixed quickly enough then we might be able to 
merge support in v3.19. If not then it has to wait until v3.20 or 
later kernels.

Thanks,

	Ingo