Re: [PATCH 01/13] RFC ONLY - kdb: core for kgdb back end

[Ingo Molnar]

* Jason Wessel <jason.wessel@xxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/