Re: [PATCH 2/8] dynamic debug - core infrastructure

Jason Baron
2008-06-16T19:16:44Z

On Fri, Jun 13, 2008 at 03:30:18PM -0700, Joe Perches wrote:
> On Fri, 2008-06-13 at 15:00 -0400, Jason Baron wrote:
> > This is the core patch that implements a new dynamic debug
> > infrastructure.
>
> Some general and specific comments.
>
> > +int dynamic_printk_enabled[NR_CPUS];
>
> I don't understand why you use NR_CPUS.
>
> Also, I think the major use cases are 1 or 2 modules enabled,
> no modules enabled, or all modules enabled, so the hashing of
> module names is particularly not useful.
>
> Any pr_debug or dev_dbg is in a slow path.
>
> I think a list and a direct comparison to KBUILD_MODNAME
> would be simpler and as fast as necessary.
>

NR_CPUS is used to set up a per-cpu int variable where bit 'n' is set if any
enabled module's name hashes into bucket 'n'. Having a single linked list of
the enabled modules would work, but i like just checking a bit in the common
case. Like you pointed out, most modules are likely disabled which means in the
common case, i'm just checking a bit, whereas in a linked list I'd be walking
a small number of entries.

However, we can better optimize the case of no modules enabled or all modules
enabled, by simply checking globals that indicate this state. I think that
would be improvment. So dynamic_dbg_enabled() looks something like:

if (alloff)
return 0;
elseif (allon)
return 1;
elseif (if hash bucket of KBUILD_MODNAME is disabled)
return 0;
else
lookup KBUILD_MODNAME in hash table
if (level or flag comparisons pass and enabled)
return 1;
else
return 0;

> > + To set the level or flag value for type 'level' or 'flag':
> > +
> > + $echo "set level=<#> <module_name>" > dynamic_printk/modules
> > +
>
> I think that set level=<#> should also work for "all"
>

good idea.

> Perhaps a simpler interface would be to use
> enable <module> <level>
> where <level> not specified is 0.
>

hmmm, there are also 'flags' that can be set for certain modules. Right now
flags and level are set the same way, via 'set level=". However, i want flags
to be able to be set via their name - "set FLAG_FOOBAR=1" or
"set FLAG_FOOBAR=0". I also like having this independent of whether the module
is enabled or disabled.

> > +int unregister_debug_module(char *mod_name)
> > +{
> > + struct debug_name *element;
> > + struct debug_name *parent = NULL;
> > + int ret = 0;
> > + int i;
> > +
> > + down(&debug_list_mutex);
> > + element = find_debug_module(mod_name);
> > + if (!element) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
>
> []
>
> > +out:
> > + up(&debug_list_mutex);
> > + return 0;
> > +}
>
> Because the return values aren't used,
> the functions might as well be declared void
>

ok

> You should probably add a sprinkling of "const"
> to the argument lists.
>

ok

> +extern void dynamic_printk(char *, char *, ...);
>
> + dynamic_printk(KBUILD_MODNAME, \
> + KERN_DEBUG KBUILD_MODNAME ": " fmt,\
>
> Instead of prefixing every fmt with KBUILD_MODNAME ": ",
> why not change this to something like:
>
> extern void dynamic_printk(const char *level, const char *module,
> const char *fmt, ...);
>
> and just do the printk in 2 parts?
>
> if (module_enabled) {
> printk("%s%s: ", level, module);
> vprintk(fmt, args);
> }
>
> If not that, why should dynamic_printk prefix a KBUILD_MODNAME
> at all? Why not just check if the module is enabled, then
> output what the code specifies?
>
> trivial: The dynamic versions of the dev_dbg and pr_debug macros
> don't return the number of chars output, but always return 0.
>
>

good idea - i think pr_debug and dev_dbg should simply be implemented on top
of dynamic_dbg_enabled(). I'll add that in the next spin.

thanks,

-Jason