Messages in this thread | | | Date | Fri, 23 Jul 2010 10:49:20 -0500 | From | Jason Wessel <> | Subject | Re: [RFC PATCH] hw-breakpoints, kgdb, x86: add a flag topassDIE_DEBUG notification |
| |
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.
| |