Re: [RFC PATCH] hw-breakpoints, kgdb, x86: add a flag topassDIE_DEBUG notification

[Jason Wessel <jason.wessel@windriver.com>]

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.