From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756342Ab0GWPuI (ORCPT ); Fri, 23 Jul 2010 11:50:08 -0400 Received: from mail.windriver.com ([147.11.1.11]:62203 "EHLO mail.windriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756737Ab0GWPuG (ORCPT ); Fri, 23 Jul 2010 11:50:06 -0400 Message-ID: <4C49BA00.5030200@windriver.com> Date: Fri, 23 Jul 2010 10:49:20 -0500 From: Jason Wessel User-Agent: Thunderbird 2.0.0.24 (X11/20100411) MIME-Version: 1.0 To: Frederic Weisbecker CC: "Deng, Dongdong" , will.deacon@arm.com, lethal@linux-sh.org, mahesh@linux.vnet.ibm.com, prasad@linux.vnet.ibm.com, benh@kernel.crashing.org, paulus@samba.org, mingo@elte.hu, kgdb-bugreport@lists.sourceforge.net, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH] hw-breakpoints, kgdb, x86: add a flag topassDIE_DEBUG notification References: <1279851361-32604-1-git-send-email-dongdong.deng@windriver.com> <20100723130430.GA5255@nowhere> <4C4996FA.2010301@windriver.com> <20100723140745.GB5255@nowhere> In-Reply-To: <20100723140745.GB5255@nowhere> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-OriginalArrivalTime: 23 Jul 2010 15:49:21.0808 (UTC) FILETIME=[9E2B6100:01CB2A7E] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/23/2010 09:07 AM, Frederic Weisbecker wrote: > On Fri, Jul 23, 2010 at 08:19:54AM -0500, Jason Wessel wrote: > >> On 07/23/2010 08:04 AM, Frederic Weisbecker wrote: >> >> The patch may or may not be the right way to solve the problem. It is >> worth noting that early breakpoints are handled separately with a direct >> writes to the debug registers so this API does not apply. >> > > > > But you still need to handle them on the debug exception, right? > > > Yes, but at that point kgdb is first in line for the notifier so it works out of the box. > > >> This patch effectively causes the events to get passed to the normal >> handlers. >> >> The source of the original problem (which was merged in 2.6.35) is >> commit: 018cbffe6819f6f8db20a0a3acd9bab9bfd667e4 - Merge commit >> 'v2.6.33' into perf/core >> >> Specifically this line right here: >> @@@ -502,6 -486,8 +486,6 @@@ static int __kprobes hw_breakpoint_hand >> rcu_read_lock(); >> >> bp = per_cpu(bp_per_reg[i], cpu); >> - if (bp) >> - rc = NOTIFY_DONE; >> >> Because the NOTIFY_DONE is never set, a default value of NOTIFY_STOP is >> passed at the end and kgdb never gets to see the break point even that >> was never intended for the perf handler in the first place. >> >> It is not as easy of course to just revert this patch because it changed >> other logic. >> >> Jason. >> > > > > Right. > > Actually NOTIFY_DONE is returned when there is more work to do: handling > another exception than breakpoint, or sending a signal. Otherwise yeah, > we return NOTIFY_STOP as we assume there is more work to do. > > For this specific case the hw_breakpoint handler simply consumed a breakpoint which was not intended for it. > So the following alternatives appear to me: > > - Moving the breakpoint exception handling into the > struct perf_event:overflow_handler. In fact I can't find the breakpoint > handling in kgdb.c > > It is in the generic die notification handler for kgdb (looking at 2.6.35-rc6) arch/x86/kernel/kgdb.c 516 static int __kgdb_notify(struct die_args *args, unsigned long cmd) ... 551 case DIE_DEBUG: 552 if (atomic_read(&kgdb_cpu_doing_single_step) != -1) { 553 if (user_mode(regs)) 554 return single_step_cont(regs, args); 555 break; 556 } else if (test_thread_flag(TIF_SINGLESTEP)) 557 /* This means a user thread is single stepping 558 * a system call which should be ignored 559 */ 560 return NOTIFY_DONE; 561 /* fall through */ > - Have a higher priority in kgdb notifier (which means decreasing the one > of hw_breakpoint.c) > kgdb had always been last in line in arch/x86/kernel/kgdb.c: 608 static struct notifier_block kgdb_notifier = { 609 .notifier_call = kgdb_notify, 610 611 /* 612 * Lowest-prio notifier priority, we want to be notified last: 613 */ 614 .priority = -INT_MAX, 615 }; > - Always returning NOTIFY_DONE from the breakpoint path. > > Without some further investigation, I am not sure what this will do. We don't want to make things worse of course. Because kgdb uses the request hw_breakpoint api to request slot reservation having an attribute to say don't do anything to this HW breakpoint is certainly one way to fix it. > Is this a regression BTW? > > Absolutely this is a regression. No change was made in kgdb related to this and the kgdb HW breakpoint regression tests (which come with the kernel) stopped working and bisect to the commit I mentioned. Jason.