* Jason Wessel <jason.wes...@windriver.com> wrote:
> This is an RFC patch. This work is by no means in its final form,
> nor is it in a form that would be suitible for upstream merging.
> This is an early prototype of a kdb frontend talking to a kgdb
> backend. It is meant to foster some discussion around the
> usefulness of merging kdb and kgdb together, as well as experiment
> with changes to kgdb's core to improve robustness and
> functionality.
>
> This patch contains the kdb core and some instrumentation into the
> core kernel which kdb requires in order to gather information for
> some of its reporting functions.
Just a first quick 30-seconds impression from skimming through the
patch:
- The cleanups are an absolute must before doing any in-depth
review. We only want to waste valuable review bandwidth on code
that at least _looks_ nice and tidy.
- Many functions are way too large, with many indentation levels -
they need a split-up.
- Most of the code patterns dont match core kernel standards and
practices, so it's not reviewable in detail. It needs a
thorough clean-up not just on the surface, but on the algorithmic
level as well.
bits like:
> + // HACK HACK HACK
> + printk(KERN_CRIT "DOH NEED TO IMPLEMENT THIS!");
need fixed.
Locking needs reviewed and fixed:
> +/* Locking is awkward. The debug code is called from all contexts, including
> + * non maskable interrupts. A normal spinlock is not safe in NMI context.
> Try
> + * to get the debug allocator lock, if it cannot be obtained after a second
> + * then give up. If the lock could not be previously obtained on this cpu
> then
> + * only try once.
> + *
> + * sparse has no annotation for "this function _sometimes_ acquires a lock",
> so
> + * fudge the acquire/release notation.
> + */
Plus, if _any_ debugger front-end is considered for merging, it
_must_ work with Kernel Mode Setting properly, out of X. No ifs
and when.
Also, high-level file organization: i'd suggest to move it all under
the kernel/debug/ hierarchy, and move kernel/kgdb.c to
kernel/debug/backend/core.c or so [possibly split up a bit, it's
getting quite large] and the KDB bits under kernel/debug/frontend/.
We dont want multiple back-ends nor multiple front-ends. We want one
good back-end and one good (built-in) front-end.
I supported and helped a debugging backend and i dont consider a
front-end completely impossible either. But it will have to meet a
_lot_ of stringent standards because a good kernel debugging
front-end's cross section to the system is even larger than a
backend's. It's a tough job to get this done.
Ingo
_______________________________________________
kdb mailing list
kdb@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/kdb
