Messages in this thread | | | Date | Sat, 9 May 2009 06:09:42 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 01/13] RFC ONLY - kdb: core for kgdb back end |
| |
* Jason Wessel <jason.wessel@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
| |