All of lore.kernel.org
 help / color / mirror / Atom feed
* kgdb in git-x86#mm review
@ 2008-02-11  1:53 Andi Kleen
  2008-02-11 15:32 ` Frank Ch. Eigler
  2008-02-11 16:03 ` kgdb in git-x86#mm review Mark Lord
  0 siblings, 2 replies; 52+ messages in thread
From: Andi Kleen @ 2008-02-11  1:53 UTC (permalink / raw)
  To: linux-kernel


I sent Jan & Jason earlier a quick review of the kgdb code currently in git-x86#mm.

The kgdb code in git-x86#mm is right now is totally broken of course because the CFI
annotations in assembler code are gone now, but they are needed for gdb use. 
And the bogus fault handling code is still in there, but Jason apprently fixed that one.

Here are the other commments I wrote just in case someone is interested.
Overall I would say there is quite a lot of stuff to clean up still.

<snip>

ok, doing a review on the code in git-x86#mm. I have not read
everything in detail, just a quick skip over.

My personal test for clean kernel debugger integration is if the
debugger could be made a loadable (but not unloadable) module with
minor/no effort. How far away is your code from that?

The 32bit in_exception_stack code looks weird. Are you sure you cannot
just use the pt_regs passed to the die notifier?

It might be safer to explicitely disable interrupts early in the notifier.

You should probably use simple_strtoul() instead of inventing an
own hex parser in kgdb.c. And sprintf instead of an own hex writer.
In general more use sprintf would probably shorten a lot of the parser
code.

The num_online_cpu()s check in getthread() looks weird. What is that
supposed to do?

kgdb_softlock_notify: I think the right way to handle this is just
calling touch_softlockup_watchdog() (or perhaps better touch_nmi_watchdog)
Actually it should be only needed once when you exit the debugger
for soft lockup, but regularly for nmi watchdog.

On x86 it is ok, but on other architectures i'm suspect the SMP
synchronization with atomics might benefit from more memory barriers.

gdb_cmd_reboot: always calling emergency_sync looks dangerous.
In fact i suspect if you hold the other CPUs the changes are good
it will lock up. You should at least offer a option to not call that
(or better don't do it by default). Even machine_start is likely
lock up actually if the other CPUs are truly halted (as they should
be) because it will try to halt them. So if anything I suspect you
need to exit the debugger first before executing this.

The logic to halt all the other CPUs in kgdb_handle_exception et.al.
looks a little fragile. It would be probably best to use the same
as kcrash uses factored out into a common function.
One obvious problem is that it is racy against cpu hotplug, but there
are likely others too. With a better implementation you likely
wouldn't need the mdelay(2) hack further down.

You should use raw spinlocks everywhere in the debugger -- i don't
think you want lockdep etc. anywhere.

The unregister hook stuff looks racy against NMIs etc.

-Andi




^ permalink raw reply	[flat|nested] 52+ messages in thread

end of thread, other threads:[~2008-02-15 20:36 UTC | newest]

Thread overview: 52+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-02-11  1:53 kgdb in git-x86#mm review Andi Kleen
2008-02-11 15:32 ` Frank Ch. Eigler
2008-02-11 16:11   ` Andi Kleen
2008-02-11 16:21   ` [git pull] kgdb-light -v8, (was: Re: kgdb in git-x86#mm review) Ingo Molnar
2008-02-11 16:41     ` [git pull] kgdb-light -v8, Jan Kiszka
2008-02-11 16:54       ` Ingo Molnar
2008-02-11 17:10     ` [git pull] kgdb-light -v8, (was: Re: kgdb in git-x86#mm review) Andi Kleen
2008-02-11 23:03       ` [git pull] kgdb-light -v9 Ingo Molnar
2008-02-12 10:03         ` Andi Kleen
2008-02-12  9:35           ` Sam Ravnborg
2008-02-12 10:26           ` Roland McGrath
2008-02-12 10:34             ` Ingo Molnar
2008-02-12 11:27           ` [git pull] kgdb-light -v10 Ingo Molnar
2008-02-12 12:19             ` Andi Kleen
2008-02-12 12:38               ` Ingo Molnar
2008-02-12 13:30                 ` Jason Wessel
2008-02-12 14:39                   ` Andi Kleen
2008-02-12 14:35                     ` Jason Wessel
2008-02-12 15:36                       ` Andi Kleen
2008-02-12 16:21                         ` Jason Wessel
2008-02-12 17:10                           ` Andi Kleen
2008-02-12 16:48                             ` Jason Wessel
2008-02-12 13:50                 ` Andi Kleen
2008-02-12 15:16                   ` Ingo Molnar
2008-02-12 15:28                     ` Andi Kleen
2008-02-12 15:28                   ` Ingo Molnar
2008-02-12 16:11                     ` Andi Kleen
2008-02-12 16:24                       ` Ingo Molnar
2008-02-12 17:01                         ` Andi Kleen
2008-02-12 16:25                       ` Linus Torvalds
2008-02-12 16:42                         ` Ingo Molnar
2008-02-12 17:07                         ` Andi Kleen
2008-02-15 12:35                           ` [RFC][PATCH] modular kgdb-light (was: Re: [git pull] kgdb-light -v10) Jan Kiszka
2008-02-15 13:32                             ` Andi Kleen
2008-02-15 20:24                               ` [RFC][PATCH] modular kgdb-light Jason Wessel
2008-02-15 20:36                         ` [git pull] kgdb-light -v10 Jason Wessel
2008-02-12 16:46                     ` Linus Torvalds
2008-02-12 17:01                       ` Ingo Molnar
2008-02-12 17:10                         ` Ingo Molnar
2008-02-12 18:20                       ` Andi Kleen
2008-02-12 18:11                         ` Linus Torvalds
2008-02-12 19:22                           ` Andi Kleen
2008-02-12 19:01                             ` Linus Torvalds
2008-02-12 18:20                         ` Andrew Morton
2008-02-12 19:16                           ` Andi Kleen
2008-02-12 21:01                             ` Ingo Molnar
2008-02-12 19:34                           ` Frank Ch. Eigler
2008-02-12 20:16                             ` Andi Kleen
2008-02-12 13:18             ` Domenico Andreoli
2008-02-12 13:59               ` Jason Wessel
2008-02-12 15:45                 ` Domenico Andreoli
2008-02-11 16:03 ` kgdb in git-x86#mm review Mark Lord

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.