Prev: [PATCH 11/11] viafb: fix accel_flags check_var bug
Next: [PATCH] fix warning: us_to_pm_timer_ticks defined but not used
From: Jason Wessel on 23 Jul 2010 12:00
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [PATCH 11/11] viafb: fix accel_flags check_var bug
Next: [PATCH] fix warning: us_to_pm_timer_ticks defined but not used