Prev: [PATCH 1/2] xfs: fix shrinker build
Next: 2.6.35-rc6 to 2.6.32.16: JuJu firewire issues
From: Frederic Weisbecker on 23 Jul 2010 10:10
On Fri, Jul 23, 2010 at 08:19:54AM -0500, Jason Wessel wrote:
> On 07/23/2010 08:04 AM, Frederic Weisbecker wrote:
> > On Fri, Jul 23, 2010 at 10:16:01AM +0800, Dongdong Deng wrote:
> >
> >> The hw_breakpoint subsystem consumes all the hardware
> >> breakpoint exceptions since it hooks the notify_die
> >> handlers first, this means that kgdb doesn't get the
> >> opportunity to handle hw breakpoint exceptions generated
> >> by kgdb itself.
> >>
> >> This patch adds an extend flag to perf_event_attr for
> >> hw_breakpoint_handler() to decide to pass or stop the
> >> DIE_DEBUG notification.
> >>
> >> As KGDB set that flag, hw_breakpoint_handler() will pass
> >> the DIE_DEBUG notification, thus kgdb have the chance
> >> to take DIE_DEBUG notification.
> >>
> >> Signed-off-by: Dongdong Deng <dongdong.deng(a)windriver.com>
> >> Reviewed-by: Bruce Ashfield <bruce.ashfield(a)windriver.com>
> >> ---
> >> arch/x86/kernel/hw_breakpoint.c | 14 ++++++++++++++
> >> arch/x86/kernel/kgdb.c | 2 ++
> >> include/linux/perf_event.h | 9 +++++++++
> >> 3 files changed, 25 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> >> index a8f1b80..b38f786 100644
> >> --- a/arch/x86/kernel/hw_breakpoint.c
> >> +++ b/arch/x86/kernel/hw_breakpoint.c
> >> @@ -406,6 +406,8 @@ EXPORT_SYMBOL_GPL(hw_breakpoint_restore);
> >> * ii) When there are more bits than trap<n> set in DR6 register (such
> >> * as BD, BS or BT) indicating that more than one debug condition is
> >> * met and requires some more action in do_debug().
> >> + * iii) The source of hw breakpoint event want to handle the event
> >> + * by itself, currently just KGDB have this notion.
> >> *
> >> * NOTIFY_STOP returned for all other cases
> >> *
> >> @@ -464,6 +466,18 @@ static int __kprobes hw_breakpoint_handler(struct die_args *args)
> >> break;
> >> }
> >>
> >> + if (bp->attr.flag == SKIP_HWBP_EVENT_PERF_FLAG) {
> >> + /*
> >> + * when attr.flag is set to SKIP_HWBP_EVENT_PERF_FLAG
> >> + * it indicates currently hw breakpoint event
> >> + * source want to handle this event by itself.
> >> + * thus return NOTIFY_DONE here.
> >> + */
> >> + rc = NOTIFY_DONE;
> >> + rcu_read_unlock();
> >> + break;
> >> + }
> >> +
> >>
> >
> >
> >
> > No. We really shouldn't make a user ABI change (adding attr.flag) just
> > to solve an in-kernel-only problem.
> >
> > And moreover we probably don't need flags at all. Why not just turning kgdb handler
> > into a higher priority?
> >
> > I don't even remember why kgdb has its own handler instead of using the
> > struct perf_event:overflow_handler. May be that's because of the early breakpoints.
> >
> >
>
>
> 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?



> 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.

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

- Have a higher priority in kgdb notifier (which means decreasing the one
of hw_breakpoint.c)

- Always returning NOTIFY_DONE from the breakpoint path.


Is this a regression BTW?

--
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 1/2] xfs: fix shrinker build
Next: 2.6.35-rc6 to 2.6.32.16: JuJu firewire issues