[PATCH 1/2] perf, x86: Add INTEL_FLAGS_UEVENT_CONSTRAINT

[PATCH 1/2] perf, x86: Add INTEL_FLAGS_UEVENT_CONSTRAINT

[Andi Kleen]

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

Add a FLAGS_UEVENT_CONSTRAINT macro that allows us to
match on event+umask, and in additional all flags.

This is needed to ensure the INV and CMASK fields
are zero for specific events, as this can cause undefined
behavior.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
index fc5eb39..4e6cdb0 100644
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -253,6 +253,10 @@ struct cpu_hw_events {
 #define INTEL_UEVENT_CONSTRAINT(c, n)	\
 	EVENT_CONSTRAINT(c, n, INTEL_ARCH_EVENT_MASK)
 
+/* Like UEVENT_CONSTRAINT, but match flags too */
+#define INTEL_FLAGS_UEVENT_CONSTRAINT(c, n)	\
+	EVENT_CONSTRAINT(c, n, INTEL_ARCH_EVENT_MASK|X86_ALL_EVENT_FLAGS)
+
 #define INTEL_PLD_CONSTRAINT(c, n)	\
 	__EVENT_CONSTRAINT(c, n, INTEL_ARCH_EVENT_MASK|X86_ALL_EVENT_FLAGS, \
 			   HWEIGHT(n), 0, PERF_X86_EVENT_PEBS_LDLAT)
-- 
1.9.3

[PATCH 2/2] perf, x86: Use INTEL_FLAGS_UEVENT_CONSTRAINT for PRECDIST

[Andi Kleen]

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

The earlier commit 86a04461a made near all PEBS on
Sandy/IvyBridge/Haswell to reject non zero flags.

However this wasn't done for the INST_RETIRED.PREC_DIST event
because no suitable macro existed. Now that we have
INTEL_FLAGS_UEVENT_CONSTRAINT enforce zero flags for
INST_RETIRED.PREC_DIST too.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event_intel_ds.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c
index 9dc4199..e578c66 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_ds.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c
@@ -606,7 +606,7 @@ struct event_constraint intel_westmere_pebs_event_constraints[] = {
 };
 
 struct event_constraint intel_snb_pebs_event_constraints[] = {
-	INTEL_UEVENT_CONSTRAINT(0x01c0, 0x2), /* INST_RETIRED.PRECDIST */
+	INTEL_FLAGS_UEVENT_CONSTRAINT(0x01c0, 0x2), /* INST_RETIRED.PRECDIST */
 	INTEL_PLD_CONSTRAINT(0x01cd, 0x8),    /* MEM_TRANS_RETIRED.LAT_ABOVE_THR */
 	INTEL_PST_CONSTRAINT(0x02cd, 0x8),    /* MEM_TRANS_RETIRED.PRECISE_STORES */
 	/* UOPS_RETIRED.ALL, inv=1, cmask=16 (cycles:p). */
@@ -617,7 +617,7 @@ struct event_constraint intel_snb_pebs_event_constraints[] = {
 };
 
 struct event_constraint intel_ivb_pebs_event_constraints[] = {
-        INTEL_UEVENT_CONSTRAINT(0x01c0, 0x2), /* INST_RETIRED.PRECDIST */
+        INTEL_FLAGS_UEVENT_CONSTRAINT(0x01c0, 0x2), /* INST_RETIRED.PRECDIST */
         INTEL_PLD_CONSTRAINT(0x01cd, 0x8),    /* MEM_TRANS_RETIRED.LAT_ABOVE_THR */
 	INTEL_PST_CONSTRAINT(0x02cd, 0x8),    /* MEM_TRANS_RETIRED.PRECISE_STORES */
 	/* UOPS_RETIRED.ALL, inv=1, cmask=16 (cycles:p). */
@@ -628,7 +628,7 @@ struct event_constraint intel_ivb_pebs_event_constraints[] = {
 };
 
 struct event_constraint intel_hsw_pebs_event_constraints[] = {
-	INTEL_UEVENT_CONSTRAINT(0x01c0, 0x2), /* INST_RETIRED.PRECDIST */
+	INTEL_FLAGS_UEVENT_CONSTRAINT(0x01c0, 0x2), /* INST_RETIRED.PRECDIST */
 	INTEL_PLD_CONSTRAINT(0x01cd, 0xf),    /* MEM_TRANS_RETIRED.* */
 	/* UOPS_RETIRED.ALL, inv=1, cmask=16 (cycles:p). */
 	INTEL_FLAGS_EVENT_CONSTRAINT(0x108001c2, 0xf),
-- 
1.9.3

Re: [PATCH 2/2] perf, x86: Use INTEL_FLAGS_UEVENT_CONSTRAINT for PRECDIST

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

On Tue, Sep 09, 2014 at 05:49:08PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> The earlier commit 86a04461a made near all PEBS on
> Sandy/IvyBridge/Haswell to reject non zero flags.

What's magic about nehalem and westmere?

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] perf, x86: Use INTEL_FLAGS_UEVENT_CONSTRAINT for PRECDIST

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On Wed, Sep 10, 2014 at 09:59:26AM +0200, Peter Zijlstra wrote:
> On Tue, Sep 09, 2014 at 05:49:08PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > The earlier commit 86a04461a made near all PEBS on
> > Sandy/IvyBridge/Haswell to reject non zero flags.
> 
> What's magic about nehalem and westmere?

And core2 and whatever else we support PEBS for, for that matter.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] perf, x86: Use INTEL_FLAGS_UEVENT_CONSTRAINT for PRECDIST

[Stephane Eranian]

On Wed, Sep 10, 2014 at 9:59 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Sep 10, 2014 at 09:59:26AM +0200, Peter Zijlstra wrote:
>> On Tue, Sep 09, 2014 at 05:49:08PM -0700, Andi Kleen wrote:
>> > From: Andi Kleen <ak@linux.intel.com>
>> >
>> > The earlier commit 86a04461a made near all PEBS on
>> > Sandy/IvyBridge/Haswell to reject non zero flags.
>>
>> What's magic about nehalem and westmere?
>
> And core2 and whatever else we support PEBS for, for that matter.

nhm, wsm, core2 do not have precise distribution (PREC_DIST) umask to
inst_Retired.

But as I said before, PEBS events do not support any filters. cycles
-> uops_retired:any_inv=1:cmask=1
being an exception.

Re: [PATCH 2/2] perf, x86: Use INTEL_FLAGS_UEVENT_CONSTRAINT for PRECDIST

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On Wed, Sep 10, 2014 at 10:37:14AM +0200, Stephane Eranian wrote:
> On Wed, Sep 10, 2014 at 9:59 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Wed, Sep 10, 2014 at 09:59:26AM +0200, Peter Zijlstra wrote:
> >> On Tue, Sep 09, 2014 at 05:49:08PM -0700, Andi Kleen wrote:
> >> > From: Andi Kleen <ak@linux.intel.com>
> >> >
> >> > The earlier commit 86a04461a made near all PEBS on
> >> > Sandy/IvyBridge/Haswell to reject non zero flags.
> >>
> >> What's magic about nehalem and westmere?
> >
> > And core2 and whatever else we support PEBS for, for that matter.
> 
> nhm, wsm, core2 do not have precise distribution (PREC_DIST) umask to
> inst_Retired.

That was a comment to 86a04461a, I should have spotted it at the time.
There is no sane reason to only change PEBS for a subset of chips.

That needs to be fixed before I'll take anything else from Andi.


[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] perf, x86: Use INTEL_FLAGS_UEVENT_CONSTRAINT for PRECDIST

[Andi Kleen]

On Wed, Sep 10, 2014 at 09:59:26AM +0200, Peter Zijlstra wrote:
> On Tue, Sep 09, 2014 at 05:49:08PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > The earlier commit 86a04461a made near all PEBS on
> > Sandy/IvyBridge/Haswell to reject non zero flags.
> 
> What's magic about nehalem and westmere?

I wasn't able to confirm their behavior explicitly, so I felt
it best to leave them alone.

But in principle adding the _FLAGS changes there too would
make sense too.

-Andi

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

Re: [PATCH 2/2] perf, x86: Use INTEL_FLAGS_UEVENT_CONSTRAINT for PRECDIST

[Peter Zijlstra]

On Wed, Sep 10, 2014 at 07:02:58AM -0700, Andi Kleen wrote:
> On Wed, Sep 10, 2014 at 09:59:26AM +0200, Peter Zijlstra wrote:
> > On Tue, Sep 09, 2014 at 05:49:08PM -0700, Andi Kleen wrote:
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > The earlier commit 86a04461a made near all PEBS on
> > > Sandy/IvyBridge/Haswell to reject non zero flags.
> > 
> > What's magic about nehalem and westmere?
> 
> I wasn't able to confirm their behavior explicitly, so I felt
> it best to leave them alone.
> 
> But in principle adding the _FLAGS changes there too would
> make sense too.

Yeah please do that patch ASAP, having PEBS behave differently across
uarchs is wrong.

Re: [PATCH 2/2] perf, x86: Use INTEL_FLAGS_UEVENT_CONSTRAINT for PRECDIST

[Thomas Gleixner]

On Wed, 10 Sep 2014, Andi Kleen wrote:
> On Wed, Sep 10, 2014 at 09:59:26AM +0200, Peter Zijlstra wrote:
> > On Tue, Sep 09, 2014 at 05:49:08PM -0700, Andi Kleen wrote:
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > The earlier commit 86a04461a made near all PEBS on
> > > Sandy/IvyBridge/Haswell to reject non zero flags.
> > 
> > What's magic about nehalem and westmere?
> 
> I wasn't able to confirm their behavior explicitly, so I felt
> it best to leave them alone.
> 
> But in principle adding the _FLAGS changes there too would
> make sense too.

Can you please once in a while come up with a precise answer to a
question instead of wrapping your internal knowledge into "in
principle", "would" and other magic keywords?

Thanks

	tglx

Re: [PATCH 2/2] perf, x86: Use INTEL_FLAGS_UEVENT_CONSTRAINT for PRECDIST

[Peter Zijlstra]

On Wed, Sep 10, 2014 at 04:22:32PM +0200, Peter Zijlstra wrote:
> On Wed, Sep 10, 2014 at 07:02:58AM -0700, Andi Kleen wrote:
> > On Wed, Sep 10, 2014 at 09:59:26AM +0200, Peter Zijlstra wrote:
> > > On Tue, Sep 09, 2014 at 05:49:08PM -0700, Andi Kleen wrote:
> > > > From: Andi Kleen <ak@linux.intel.com>
> > > > 
> > > > The earlier commit 86a04461a made near all PEBS on
> > > > Sandy/IvyBridge/Haswell to reject non zero flags.
> > > 
> > > What's magic about nehalem and westmere?
> > 
> > I wasn't able to confirm their behavior explicitly, so I felt
> > it best to leave them alone.
> > 
> > But in principle adding the _FLAGS changes there too would
> > make sense too.
> 
> Yeah please do that patch ASAP, having PEBS behave differently across
> uarchs is wrong.

Ping!

Re: [PATCH 2/2] perf, x86: Use INTEL_FLAGS_UEVENT_CONSTRAINT for PRECDIST

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Sep 10, 2014 at 04:22:32PM +0200, Peter Zijlstra wrote:
> > On Wed, Sep 10, 2014 at 07:02:58AM -0700, Andi Kleen wrote:
> > > On Wed, Sep 10, 2014 at 09:59:26AM +0200, Peter Zijlstra wrote:
> > > > On Tue, Sep 09, 2014 at 05:49:08PM -0700, Andi Kleen wrote:
> > > > > From: Andi Kleen <ak@linux.intel.com>
> > > > > 
> > > > > The earlier commit 86a04461a made near all PEBS on
> > > > > Sandy/IvyBridge/Haswell to reject non zero flags.
> > > > 
> > > > What's magic about nehalem and westmere?
> > > 
> > > I wasn't able to confirm their behavior explicitly, so I felt
> > > it best to leave them alone.
> > > 
> > > But in principle adding the _FLAGS changes there too would
> > > make sense too.
> > 
> > Yeah please do that patch ASAP, having PEBS behave differently across
> > uarchs is wrong.
> 
> Ping!

Also, I have to NAK this patch for trivial style reasons: the 
patch adds gobs of misaligned, misplaced spaces instead of using 
proper tabs - and apparently that's not the first time Andi did 
that to the constraints tables.

This is annoying and ineffective and Andi Kleen continues to 
introduce trivial flaws into the kernel despite repeated 
complaints and requests for him to use available tools to check 
patch quality such as scripts/checkpatch.pl.

As a result I'm dropping most patches from Andi in the perf/core 
pipeline and I will delay them to at least until v3.19 when I 
might have more time to waste on reviewing trivially flawed 
patches.

I only took this single patch:

  perf/x86/intel/uncore: Fix minor race in box set up

which fixes a bug, and I've dropped all other patches from Andi.

Next time I see trivial flaws in Andi's patches I'll drop and 
delay them for two cycles, not one, to give them ample time to be 
improved.

Thanks,

	Ingo