Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757235AbcCXJ55 (ORCPT ); Thu, 24 Mar 2016 05:57:57 -0400 Received: from foss.arm.com ([217.140.101.70]:50363 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755189AbcCXJ5s (ORCPT ); Thu, 24 Mar 2016 05:57:48 -0400 Date: Thu, 24 Mar 2016 09:58:02 +0000 From: Will Deacon To: Peter Zijlstra Cc: Wang Nan , mingo@redhat.com, linux-kernel@vger.kernel.org, He Kuang , Alexei Starovoitov , Arnaldo Carvalho de Melo , Brendan Gregg , Jiri Olsa , Masami Hiramatsu , Namhyung Kim , Zefan Li , pi3orama@163.com Subject: Re: [PATCH 2/5] perf core: Set event's default overflow_handler Message-ID: <20160324095802.GA9323@arm.com> References: <1457949585-191064-1-git-send-email-wangnan0@huawei.com> <1457949585-191064-3-git-send-email-wangnan0@huawei.com> <20160323175021.GK6356@twins.programming.kicks-ass.net> <20160323181348.GA2149@arm.com> <20160323192938.GM6356@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160323192938.GM6356@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2913 Lines: 69 On Wed, Mar 23, 2016 at 08:29:38PM +0100, Peter Zijlstra wrote: > On Wed, Mar 23, 2016 at 06:13:49PM +0000, Will Deacon wrote: > > On Wed, Mar 23, 2016 at 06:50:21PM +0100, Peter Zijlstra wrote: > > > On Mon, Mar 14, 2016 at 09:59:42AM +0000, Wang Nan wrote: > > > > +++ b/arch/arm/kernel/hw_breakpoint.c > > > > @@ -631,7 +631,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp) > > > > info->address &= ~alignment_mask; > > > > info->ctrl.len <<= offset; > > > > > > > > - if (!bp->overflow_handler) { > > > > + if (is_default_overflow_handler(bp)) { > > > > /* > > > > * Mismatch breakpoints are required for single-stepping > > > > * breakpoints. > > > > @@ -754,7 +754,7 @@ static void watchpoint_handler(unsigned long addr, unsigned int fsr, > > > > * mismatch breakpoint so we can single-step over the > > > > * watchpoint trigger. > > > > */ > > > > - if (!wp->overflow_handler) > > > > + if (is_default_overflow_handler(wp)) > > > > enable_single_step(wp, instruction_pointer(regs)); > > > > > > > > unlock: > > > > diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c > > > > index b45c95d..4ef5373 100644 > > > > --- a/arch/arm64/kernel/hw_breakpoint.c > > > > +++ b/arch/arm64/kernel/hw_breakpoint.c > > > > @@ -616,7 +616,7 @@ static int breakpoint_handler(unsigned long unused, unsigned int esr, > > > > perf_bp_event(bp, regs); > > > > > > > > /* Do we need to handle the stepping? */ > > > > - if (!bp->overflow_handler) > > > > + if (is_default_overflow_handler(bp)) > > > > step = 1; > > > > unlock: > > > > rcu_read_unlock(); > > > > @@ -712,7 +712,7 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr, > > > > perf_bp_event(wp, regs); > > > > > > > > /* Do we need to handle the stepping? */ > > > > - if (!wp->overflow_handler) > > > > + if (is_default_overflow_handler(wp)) > > > > step = 1; > > > > > > > > unlock: > > > > > > Will, why does it matter what the overflow handler is for this stuff? > > > > Because ptrace registers an overflow handler for raising a SIGTRAP and > > ptrace users (e.g. GDB) expect to handle the single-stepping themselves. > > Perf, on the other hand, will livelock if the kernel doesn't do the > > stepping. > > Would it, perhaps, make sense to invert this test and check for > ->overflow_handler == ptrace_hbptriggered instead? That way nobody gets > surprise live-locks, endlessly triggering the same trap. Not sure... I can imagine kgdb, for example, wanting to handle the stepping itself. You also need to play clever tricks if you want to step through LL/SC atomics, which the code here doesn't even try to handle (because it involves disassembling the instructions and applying a bunch of heuristics), so I imagine most debuggers wanting to take care of the step themselves. > But yes, this kinda blows. Yup. Will