Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933777AbdDFJLO (ORCPT ); Thu, 6 Apr 2017 05:11:14 -0400 Received: from merlin.infradead.org ([205.233.59.134]:35872 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932903AbdDFJKs (ORCPT ); Thu, 6 Apr 2017 05:10:48 -0400 Date: Thu, 6 Apr 2017 11:10:39 +0200 From: Peter Zijlstra To: kan.liang@intel.com Cc: mingo@redhat.com, linux-kernel@vger.kernel.org, eranian@google.com, ak@linux.intel.com Subject: Re: [PATCH V2] perf/x86: fix spurious NMI with PEBS Load Latency event Message-ID: <20170406091039.wbdgvnlzffddthha@hirez.programming.kicks-ass.net> References: <1491333246-3965-1-git-send-email-kan.liang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1491333246-3965-1-git-send-email-kan.liang@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4392 Lines: 118 On Tue, Apr 04, 2017 at 03:14:06PM -0400, kan.liang@intel.com 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. > > Fixes: 8077eca079a2 ("perf/x86/pebs: Add workaround for broken OVFL > status on HSW+") > Signed-off-by: Kan Liang That is atrocious unreadable garbage; in a large part due to the random line breaks. Any half sane editor can reflow text; use that. --- Subject: perf/x86: Fix spurious NMI with PEBS Load Latency event From: Kan Liang Date: Tue, 4 Apr 2017 15:14:06 -0400 Spurious NMIs will be observed with the following command: while :; do perf record -bae "cpu/umask=0x01,event=0xcd,ldlat=0x80/pp" -e "cpu/umask=0x03,event=0x0/" -e "cpu/umask=0x02,event=0x0/" -e cycles,branches,cache-misses -e cache-references -- sleep 10 done The issue was introduced by commit: 8077eca079a2 ("perf/x86/pebs: Add workaround for broken OVFL status on HSW+") That commit clears the status bits for the counters used for PEBS events, by masking the whole 64 bits pebs_enabled. However, only the low 32 bits of both status and pebs_enabled are reserved for PEBS-able counters. For status bits 32-34 are fixed counter overflow bits. For pebe_enabled bits 32-34 are for PEBS Load Latency. In the test case, the PEBS Load Latency event and fixed counter event could overflow 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. Fixes: 8077eca079a2 ("perf/x86/pebs: Add workaround for broken OVFL status on HSW+") Cc: mingo@redhat.com Cc: ak@linux.intel.com Cc: eranian@google.com Signed-off-by: Kan Liang Signed-off-by: Peter Zijlstra (Intel) Link: http://lkml.kernel.org/r/1491333246-3965-1-git-send-email-kan.liang@intel.com --- arch/x86/events/intel/core.c | 2 +- arch/x86/events/intel/ds.c | 2 +- arch/x86/events/perf_event.h | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) --- 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 p * counters from the GLOBAL_STATUS mask and we always process PEBS * events via drain_pebs(). */ - status &= ~cpuc->pebs_enabled; + status &= ~(cpuc->pebs_enabled & PEBS_COUNTER_MASK); /* * PEBS overflow sets bit 62 in the global status register --- a/arch/x86/events/intel/ds.c +++ b/arch/x86/events/intel/ds.c @@ -1222,7 +1222,7 @@ get_next_pebs_record_by_bit(void *base, /* clear non-PEBS bit and re-check */ pebs_status = p->status & cpuc->pebs_enabled; - pebs_status &= (1ULL << MAX_PEBS_EVENTS) - 1; + pebs_status &= PEBS_COUNTER_MASK; if (pebs_status == (1 << bit)) return at; } --- a/arch/x86/events/perf_event.h +++ b/arch/x86/events/perf_event.h @@ -79,6 +79,7 @@ struct amd_nb { /* The maximal number of PEBS events: */ #define MAX_PEBS_EVENTS 8 +#define PEBS_COUNTER_MASK ((1ULL << MAX_PEBS_EVENTS) - 1) /* * Flags PEBS can handle without an PMI.