Re: [PATCH] perf/x86: fix spurious NMI with PEBS Load Latency event

[Stephane Eranian]

On Tue, Apr 4, 2017 at 10:52 AM,  <kan.liang@xxxxxxxxx> wrote:
> From: Kan Liang <kan.liang@xxxxxxxxx>
>
> Spurious NMIs will be observed when applying the following command.
>    while true ; do sudo perf record -b -a -e
>    "cpu/umask=0x01,event=0xcd,ldlat=0x80/pp,cpu/umask=0x03,event=0x0/,
>     cpu/umask=0x02,event=0x0/,cycles,branches,cache-misses,
>     cache-references" -- sleep 10 ; done
>
> The issue was introduced by
> commit 8077eca079a2 ("perf/x86/pebs: Add workaround for broken OVFL status
> on HSW+")
>
> The previous patch clear the status's bits for the counters used for
> PEBS events, by masking the whole 64 bits pebs_enabled.
> However, only the lower 32 bits of both status and pebs_enabled are
> reserved for PEBS-able counters.
> For status, the first three bits of upper 32 bits are fixed counter
> overflow bit.
> For pebs_enabled, the first three bits of upper 32 bits are for PEBS
> Load Latency event.
> In the test case, the PEBS Load Latency event and fixed counter event
> could be overflowed at the same time. The fixed counter overflow bit will
> be cleared by mistake. Once it is cleared, the fixed counter overflow
> never be processed, which finally trigger spurious NMI.
>
> Correct the PEBS enabled mask by ignoring the non-PEBS bits.
>
Yes, the patch is correct, bits 32-35 are used for load latency, bit 63 for
precise stores. So yes, the clearing must only look at the bottom 4 bits.

I would suggest using a macros for (1ULL << MAX_PEBS_EVENTS) - 1)
as it is used in at least 2 other places. That would make this cleaner and
more explicit: PEBS_COUNTER_MASK, for instance.

> Fixes: 8077eca079a2 ("perf/x86/pebs: Add workaround for broken OVFL
> status on HSW+")
> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxx>
> ---
>  arch/x86/events/intel/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 319da60..5b69787 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -2151,7 +2151,7 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
>          * counters from the GLOBAL_STATUS mask and we always process PEBS
>          * events via drain_pebs().
>          */
> -       status &= ~cpuc->pebs_enabled;
> +       status &= ~(cpuc->pebs_enabled & ((1ULL << MAX_PEBS_EVENTS) - 1));
>
>         /*
>          * PEBS overflow sets bit 62 in the global status register
> --
> 2.4.3
>