Messages in this thread | | | Subject | Re: [PATCH 2/8] dynamic debug - core infrastructure | From | Joe Perches <> | Date | Fri, 13 Jun 2008 15:30:18 -0700 |
| |
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.
> + 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"
Perhaps a simpler interface would be to use enable <module> <level> where <level> not specified is 0.
> +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
You should probably add a sprinkling of "const" to the argument lists.
+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.
| |