Re: [RFC PATCH] printk: Convert pr_<level> macros to functions

[Andrew Morton]

On Wed, 03 Mar 2010 07:20:18 -0800
Joe Perches <joe@xxxxxxxxxxx> wrote:

> printk is defined asmlinkage (extern "C")
> For this RFC patch, pr_<level> calls are as well.
> Is this declaration style really necessary?
> Maybe moving printed_len to file scope is racy somehow?

Yes, printed_len will get corrupted when multiple CPU's are running
printk/vprintk simultaneously.  That'll need to be fixed.

> Save 3 bytes per pr_<level> use after printk overhead
> 
> Does not store the KERN_<level> string as constant string
> when using pr_<level> calls
> 
> printk.o increases ~200 bytes
> defconfig decreases ~700 bytes
> 
> Minor printk neatening, moving comments above function declaration
> Renamed vprint to __vprintk, added vprintk wrapper
> Moved automatic printed_len to file static
> Moved EXPORT_SYMBOL after functions
> 

Looks good at the first peek.  Is it possible to reduce the amount of
code movement in the patch, make it a bit easier to follow?  Maybe do
it as two patches, with the move-stuff-around patch being [1/2]?

>
> ...
>
> +asmlinkage int pr_emerg(const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +
> +	va_start(args, fmt);
> +	r = pr_printk('0', fmt, args);
> +	r = __vprintk(fmt, args);
> +	va_end(args);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL(pr_emerg);
> +
> +asmlinkage int pr_alert(const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +
> +	va_start(args, fmt);
> +	r = pr_printk('1', fmt, args);
> +	va_end(args);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL(pr_alert);
> +
> +asmlinkage int pr_crit(const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +
> +	va_start(args, fmt);
> +	r = pr_printk('2', fmt, args);
> +	va_end(args);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL(pr_crit);
> +
> +asmlinkage int pr_err(const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +
> +	va_start(args, fmt);
> +	r = pr_printk('3', fmt, args);
> +	va_end(args);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL(pr_err);
> +
> +asmlinkage int pr_warning(const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +
> +	va_start(args, fmt);
> +	r = pr_printk('4', fmt, args);
> +	va_end(args);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL(pr_warning);
> +
> +asmlinkage int pr_notice(const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +
> +	va_start(args, fmt);
> +	r = pr_printk('5', fmt, args);
> +	va_end(args);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL(pr_notice);
> +
> +asmlinkage int pr_info(const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +
> +	va_start(args, fmt);
> +	r = pr_printk('6', fmt, args);
> +	va_end(args);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL(pr_info);
> +
> +asmlinkage int pr_cont(const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +
> +	va_start(args, fmt);
> +	r = pr_printk('c', fmt, args);
> +	va_end(args);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL(pr_cont);

I think it would be justifiable to cook up a freaky macro and expand it
eight times to avoid this duplication.  Ugly, but better than lots of
duplication.

Or perhaps we can do it via a helper function which takes the
additional argument?

asmlinkage int pr_everything(char levelchar, const char *fmt, ...)

?

--
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/