Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752948AbbGPGCU (ORCPT ); Thu, 16 Jul 2015 02:02:20 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:33854 "EHLO mail-ob0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751300AbbGPGCR (ORCPT ); Thu, 16 Jul 2015 02:02:17 -0400 MIME-Version: 1.0 Reply-To: eranian@gmail.com In-Reply-To: <20150715123546.GK2859@worktop.programming.kicks-ass.net> References: <20150703131336.GI19282@twins.programming.kicks-ass.net> <20150703190420.GS3644@twins.programming.kicks-ass.net> <20150715123546.GK2859@worktop.programming.kicks-ass.net> Date: Thu, 16 Jul 2015 08:02:03 +0200 Message-ID: Subject: Re: perf: fuzzer triggered warning in intel_pmu_drain_pebs_nhm() From: Stephane Eranian To: Peter Zijlstra Cc: Vince Weaver , LKML , Ingo Molnar , Arnaldo Carvalho de Melo , kan.liang@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: 5619 Lines: 127 On Wed, Jul 15, 2015 at 2:35 PM, Peter Zijlstra wrote: > On Wed, Jul 15, 2015 at 08:42:50AM +0200, Stephane Eranian wrote: >> On Fri, Jul 3, 2015 at 9:49 PM, Vince Weaver wrote: >> > On Fri, 3 Jul 2015, Peter Zijlstra wrote: >> > >> >> That said, its far too warm and I might just not be making sense. >> > >> > you need to come visit Maine! Although I am not sure the cooler weather >> > necessarily improves my kernel debugging skills. >> > >> > I managed to lock the machine (again this is with the patch applied). >> > >> I can reproduce the problem on my HSW running the fuzzer. >> >> I can see why this could be happening if you are mixing PEBS and non PEBS events >> in the bottom 4 counters. I suspect: >> for (bit = 0; bit < x86_pmu.max_pebs_events; bit++) { >> if ((counts[bit] == 0) && (error[bit] == 0)) >> continue; >> >> This test is not correct when you have non-PEBS events mixed with PEBS >> events and >> they overflow at the same time. They will have counts[i] != 0 but >> error[i] == 0, and thus >> you fall thru the loop and hit the assert. Or it is something along those lines. >> > > The only way I can make this work is if ->status only has !PEBS events > set, because if it has both set we'll take that slow path which masks > out the !PEBS bits. > > After masking there are 3 options: > > - there is one bit set, and its @bit, we increment counts[bit]. > - there are multiple bits set, we increment error[] for each set bit, > we do not increment counts[]. > - there are no bits set, we do nothing. > > The intent was to never increment counts[] for !PEBS events. > > Now if we start out with only a single !PEBS event set, we'll pass the > test and increment counts[] for a !PEBS and hit the warn. > > The below patch modifies the code such that it can deal with that > particular issue. Can you try? > Been running it for a couple of hours, so far so good. I will let it run all night. Testing it on HSW and NHM, my SNB is broken at the moment. Don't know if the fuzzer has already reproduced the test case. Thanks. > --- > arch/x86/kernel/cpu/perf_event_intel_ds.c | 29 +++++++++++++---------------- > 1 file changed, 13 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c > index 71fc40238843..68d0ced1d229 100644 > --- a/arch/x86/kernel/cpu/perf_event_intel_ds.c > +++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c > @@ -1142,6 +1142,7 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs) > > for (at = base; at < top; at += x86_pmu.pebs_record_size) { > struct pebs_record_nhm *p = at; > + u64 pebs_status; > > /* PEBS v3 has accurate status bits */ > if (x86_pmu.intel_cap.pebs_format >= 3) { > @@ -1152,12 +1153,14 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs) > continue; > } > > - bit = find_first_bit((unsigned long *)&p->status, > + pebs_status = p->status & cpuc->pebs_enabled; > + pebs_status &= (1ULL << x86_pmu.max_pebs_events) - 1; > + > + bit = find_first_bit((unsigned long *)&pebs_status, > x86_pmu.max_pebs_events); > if (bit >= x86_pmu.max_pebs_events) > continue; > - if (!test_bit(bit, cpuc->active_mask)) > - continue; > + > /* > * The PEBS hardware does not deal well with the situation > * when events happen near to each other and multiple bits > @@ -1172,27 +1175,21 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs) > * one, and it's not possible to reconstruct all events > * that caused the PEBS record. It's called collision. > * If collision happened, the record will be dropped. > - * > */ > - if (p->status != (1 << bit)) { > - u64 pebs_status; > - > - /* slow path */ > - pebs_status = p->status & cpuc->pebs_enabled; > - pebs_status &= (1ULL << MAX_PEBS_EVENTS) - 1; > - if (pebs_status != (1 << bit)) { > - for_each_set_bit(i, (unsigned long *)&pebs_status, > - MAX_PEBS_EVENTS) > - error[i]++; > - continue; > - } > + if (p->status != (1ULL << bit)) { > + for_each_set_bit(i, (unsigned long *)&pebs_status, > + x86_pmu.max_pebs_events) > + error[i]++; > + continue; > } > + > counts[bit]++; > } > > for (bit = 0; bit < x86_pmu.max_pebs_events; bit++) { > if ((counts[bit] == 0) && (error[bit] == 0)) > continue; > + > event = cpuc->events[bit]; > WARN_ON_ONCE(!event); > WARN_ON_ONCE(!event->attr.precise_ip); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/