Re: [tip:perf/urgent] perf/x86/intel: Fix INTEL_FLAGS_EVENT_CONSTRAINT* masking

[Stephane Eranian]

On Sat, May 18, 2019 at 2:16 PM Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>
> * tip-bot for Stephane Eranian <tipbot@xxxxxxxxx> wrote:
>
> > Commit-ID:  6b89d4c1ae8596a8c9240f169ef108704de373f2
> > Gitweb:     https://git.kernel.org/tip/6b89d4c1ae8596a8c9240f169ef108704de373f2
> > Author:     Stephane Eranian <eranian@xxxxxxxxxx>
> > AuthorDate: Thu, 9 May 2019 14:45:56 -0700
> > Committer:  Ingo Molnar <mingo@xxxxxxxxxx>
> > CommitDate: Fri, 10 May 2019 08:04:17 +0200
> >
> > perf/x86/intel: Fix INTEL_FLAGS_EVENT_CONSTRAINT* masking
> >
> > On Intel Westmere, a cmdline as follows:
> >
> >   $ perf record -e cpu/event=0xc4,umask=0x2,name=br_inst_retired.near_call/p ....
> >
> > was failing. Yet the event+ umask support PEBS.
> >
> > It turns out this is due to a bug in the the PEBS event constraint table for
> > westmere. All forms of BR_INST_RETIRED.* support PEBS. Therefore the constraint
> > mask should ignore the umask. The name of the macro INTEL_FLAGS_EVENT_CONSTRAINT()
> > hint that this is the case but it was not. That macros was checking both the
> > event code and event umask. Therefore, it was only matching on 0x00c4.
> > There are code+umask macros, they all have *UEVENT*.
> >
> > This bug fixes the issue by checking only the event code in the mask.
> > Both single and range version are modified.
> >
> > Signed-off-by: Stephane Eranian <eranian@xxxxxxxxxx>
> > Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> > Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Vince Weaver <vincent.weaver@xxxxxxxxx>
> > Cc: kan.liang@xxxxxxxxx
> > Link: http://lkml.kernel.org/r/20190509214556.123493-1-eranian@xxxxxxxxxx
> > Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
> > ---
> >  arch/x86/events/perf_event.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> > index 07fc84bb85c1..a6ac2f4f76fc 100644
> > --- a/arch/x86/events/perf_event.h
> > +++ b/arch/x86/events/perf_event.h
> > @@ -394,10 +394,10 @@ struct cpu_hw_events {
> >
> >  /* Event constraint, but match on all event flags too. */
> >  #define INTEL_FLAGS_EVENT_CONSTRAINT(c, n) \
> > -     EVENT_CONSTRAINT(c, n, INTEL_ARCH_EVENT_MASK|X86_ALL_EVENT_FLAGS)
> > +     EVENT_CONSTRAINT(c, n, ARCH_PERFMON_EVENTSEL_EVENT|X86_ALL_EVENT_FLAGS)
> >
> >  #define INTEL_FLAGS_EVENT_CONSTRAINT_RANGE(c, e, n)                  \
> > -     EVENT_CONSTRAINT_RANGE(c, e, n, INTEL_ARCH_EVENT_MASK|X86_ALL_EVENT_FLAGS)
> > +     EVENT_CONSTRAINT_RANGE(c, e, n, ARCH_PERFMON_EVENTSEL_EVENT|X86_ALL_EVENT_FLAGS)
>
> This commit broke one of my testboxes - and unfortunately I noticed this
> too late and the commit is now upstream.
>
> The breakage is that 'perf top' stops working altogether, it errors out
> in the event creation:
>
>  $ perf top --stdio
>  Error:
>  The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (cycles).
>
> I bisected it back to this commit:
>
>  6b89d4c1ae8596a8c9240f169ef108704de373f2 is the first bad commit
>  commit 6b89d4c1ae8596a8c9240f169ef108704de373f2
>  Author: Stephane Eranian <eranian@xxxxxxxxxx>
>  Date:   Thu May 9 14:45:56 2019 -0700
>
>     perf/x86/intel: Fix INTEL_FLAGS_EVENT_CONSTRAINT* masking
>
> The system is IvyBridge model 62, running a defconfig-ish kernel, and
> with perf_event_paranoid set to -1:
>
>  [    3.756600] Performance Events: PEBS fmt1+, IvyBridge events, 16-deep LBR, full-width counters, Intel PMU driver.
>
>  processor      : 39
>  vendor_id      : GenuineIntel
>  cpu family     : 6
>  model          : 62
>  model name     : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
>  stepping       : 4
>  microcode      : 0x428
>
> If I revert the commit 'perf top' starts working again.
>
I have some ivybridge systems, let me debug this. This is likely
related to cycles:ppp stuff given what perf top does.
I think my patch is right, but there may be assumptions or bugs
elsewhere exposed by the fix.

>
> Thanks,
>
>         Ingo