Message-Id: <20080527143959.ccdad922.akpm@linux-foundation.org>
Date:	Tue, 27 May 2008 14:39:59 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Jason Baron <jbaron@...hat.com>
Cc:	joe@...ches.com, greg@...ah.com, nick@...k-andrew.net,
	randy.dunlap@...cle.com, linux-kernel@...r.kernel.org
Subject: Re: [patch 0/8] debug printk infrastructure

On Thu, 22 May 2008 17:13:31 -0400
Jason Baron <jbaron@...hat.com> wrote:

> hi,

Hi.

Three or four thousand people received your patches, but only one chose
to get in and review them.  Why I have so much more spare time than
everyone else is, I guess, a topic which we should examine separately. 
Sigh.

> So i've attempted to make this infrastrucutre general enough to be used by the
> kernel as a whole. The basic interfaces i'm using are the basic, 'pr_debug()',
> and 'dev_dbg()' calls.

How are you using them?  What is the interface?  What do the patches
do?  Please pretend that I have totally forgotten everything which went
before and my mind is now a virginal blank.  That's actually not far
from reality.

> And then i've added:
> 
> -register_dev_dbg_handler(parent, type, max, init)
> This registers a handler that can support level, or flag printks. 'type' is
> either 'TYPE_LEVEL' or 'TYPE_FLAG'. The 'parent' field can be used to support
> a hierarchical debugging between modules, for example where we want to share
> the same levels across modules. 'init' is the initial setting for the flag
> or level. 'max' is currently unused and probably should be discarded.

uh, I think I vaguely understand what you're getting at here.  Not
knowing what type `parent' has makes that harder that it needs to be.

What is "hierarchical debugging between modules" and why would I want one?

> -dev_dbg_level(value, dev, format, ...)
> This call is used by code in the core driver as its interface into 'printk'.
> 'value' is the level or flag value for the particular printk. So modules can
> do: #define dprintk dev_dbg_level(value, dev, format, ...)

ok, I guess.  But I'm a bit lost on the earlier stuff, so this is a bit
eye-glazing.
 
> -dev_dbg_enabled(value)
> This call is used to allow the infrastructure to be more flexible and handle
> blocks of printks or even blocks of other debugging code.

How would one do that?

Perhaps some suitably-explained code examples would help here.  In the
changelog text.

> -'pr_debug' and 'dev_dbg' can continue to be used as before (no API change),
> but this infrastructure automatically detects where these calls are and displays
> the modules that have them in a debugfs file: debug/dynamic_printk/modules
> 
> The debugfs file, then lists all the modules that can be 'enabled' for dynamic
> printk calls. Currently, the file has a 4 column format (excerpt from my system:
> 
> <module_name> <enabled/disabled> <current level/flag setting> <# of call sites>
> 
> 8250 0 0 2
> acpi_cpufreq 0 0 1
> aio 0 0 24
> backlight 0 0 3

ok..

I'm not a big fan of the relative pathnames in debugfs.  Such as
"debug/dynamic_printk/modules".  Given that people will hopefully write
userspace tools which access these files I believe that it is valuable
for the kernel developers to provide them with a well-known,
predictable (ie: fixed) absolute pathname.  Or at least a standardised
well-known way of locating that file at runtime.

Maybe such a mechanism already exists for debugfs - I haven't been
keeping up.  But if I were writing a userspace script which needed to
access debug/dynamic_printk/modules I'd be wondering "how the heck do I
reliably locate that file on everyone's machines?".

> I've converted a few subsysmts to this infrastructure to provide some examples
> of what it can do: module.c bonding aio and cpufreq (including sub-drivers).
> 
> I realize the code is a bit rough around the edges, but the basic idea is here, 
> and i'm looking for feedback on the approach.
> 
> todo:
> 
> *free memory used in text sections to detect call sites
> *provide potential level settings in the debugfs file
> *convert more modules/subsystems
> *tidy up lib/dynamic_printk.c

I hate to sound nitpicky here, but the lack of suitably detailed
description here really does make the patches harder to understand and
harder to review.  Ideally, I'd like to fully understand what you have
set out to achieve before getting in there and looking at how you have
sought to achieve it.


I believe this is an important effort - please persist.  I _think_ what
you have here looks good and promising, but it's a little hard to get
the old head around it all.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists