Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754726AbdDDSXp (ORCPT ); Tue, 4 Apr 2017 14:23:45 -0400 Received: from mail-io0-f171.google.com ([209.85.223.171]:33173 "EHLO mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754239AbdDDSXo (ORCPT ); Tue, 4 Apr 2017 14:23:44 -0400 MIME-Version: 1.0 In-Reply-To: <1491328366-4089-1-git-send-email-kan.liang@intel.com> References: <1491328366-4089-1-git-send-email-kan.liang@intel.com> From: Stephane Eranian Date: Tue, 4 Apr 2017 11:23:42 -0700 Message-ID: Subject: Re: [PATCH] perf/x86: fix spurious NMI with PEBS Load Latency event To: "Liang, Kan" Cc: "mingo@redhat.com" , Peter Zijlstra , LKML , "ak@linux.intel.com" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2461 Lines: 58 On Tue, Apr 4, 2017 at 10:52 AM, wrote: > From: Kan Liang > > 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 > --- > 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 >