Re: [PATCH 7/7] printk: provide a filtering macro for printk
On Wed, 2 Sep 2009, Tim Bird wrote: Marc Andre Tanner wrote: On Tue, Sep 01, 2009 at 07:32:25PM -0400, H Hartley Sweeten wrote: On Tuesday, September 01, 2009 4:24 PM, Tim Bird wrote: Some places in the kernel break the message into pieces, like so: printk(KERN_ERR, Error: first part ); ... printk( more info for error.\n); Technically, shouldn't the second part of the message actually be: printk(KERN_CONT more info for error.\n); Maybe some mechanism could be created to handle the continued message if they have the KERN_CONT? Yes it's true that KERN_CONT isn't handled correctly, but I don't see a way to change that. These parts would not be handled consistently under certain conditions. It would be confusing to see only part of the message, but I don't know how often this construct is used. $ grep -R KERN_CONT linux-2.6 | wc -l 373 Maybe another mechanism is needed to ensure that continuation printk lines have the same log level as their start strings. I currently don't see a way to achieve this with the CPP. If it's that few, then maybe it's OK to actually change the code for those printk statements. (Heck, these locations were all changed in the last 2 years anyway.) I'm just brainstorming here, but how about changing them from: printk(KERN_INFO foo); printk(KERN_CONT bar\n); to: printk(KERN_INFO foo); printk_cont(KERN_INFO bar\n); This way the continuation line has the log level, and can be conditionally compiled based on the VERBOSITY level. A little magic would be needed to strip the first 3 chars of the fmt string in printk_cont(). You could strip the first 3 chars by adding 3 to the pointer. Which will fail horribly if the KERN_* is forgotten and the format string is very short. And the KERN_* still consumes memory. Or make the KERN_* an explicit first parameter: printk_cont(KERN_INFO, bar\n); Is strcmp(p, KERN_INFO) optimized away for constant strings? I think this makes the printk messages a bit more consistent anyway, and still marks lines that are continuation lines. With kind regards, Geert Uytterhoeven Software Architect Techsoft Centre Technology and Software Centre Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: geert.uytterhoe...@sonycom.com Internet: http://www.sony-europe.com/ A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis · BIC GEBABEBB · IBAN BE41293037680010 -- To unsubscribe from this list: send the line unsubscribe linux-embedded in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] printk: provide a filtering macro for printk
On Wed, Sep 02, 2009 at 12:35:42AM +0100, Jamie Lokier wrote:
Marc Andre Tanner wrote:
+ * The check with sizeof(void*) should make sure that we don't operate on
+ * pointers, which the compiler wouldn't be able to optimize out, but only
+ * on string constants.
Take a look at __builtin_constant_p in the GCC manual.
You'll probably find that wrapping the whole of the rest of the
expression (without the sizeof) in __builtin_constant_p is a good
way to know when to depend on the result of the expression.
Thanks, so if I understood it correctly this should be used like this:
#define PRINTK_FILTER(fmt) (
\
(((const char *)(fmt))[0] != '' CONFIG_PRINTK_VERBOSITY = 4) ||
\
(((const char *)(fmt))[0] == ''
\
((const char *)(fmt))[1] = *__stringify(CONFIG_PRINTK_VERBOSITY))
\
)
#define printk(fmt, ...) ({
\
if (__builtin_constant_p(PRINTK_FILTER(fmt)) PRINTK_FILTER(fmt))
\
printk((fmt), ##__VA_ARGS__);
\
})
The sizeof check wouldn't be necessary. Is this correct?
Marc
--
Marc Andre Tanner http://www.brain-dump.org/ GPG key: CF7D56C0
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] printk: provide a filtering macro for printk
On Tue, Sep 01, 2009 at 07:32:25PM -0400, H Hartley Sweeten wrote: On Tuesday, September 01, 2009 4:24 PM, Tim Bird wrote: Some places in the kernel break the message into pieces, like so: printk(KERN_ERR, Error: first part ); ... printk( more info for error.\n); Technically, shouldn't the second part of the message actually be: printk(KERN_CONT more info for error.\n); Maybe some mechanism could be created to handle the continued message if they have the KERN_CONT? Yes it's true that KERN_CONT isn't handled correctly, but I don't see a way to change that. These parts would not be handled consistently under certain conditions. It would be confusing to see only part of the message, but I don't know how often this construct is used. $ grep -R KERN_CONT linux-2.6 | wc -l 373 Maybe another mechanism is needed to ensure that continuation printk lines have the same log level as their start strings. I currently don't see a way to achieve this with the CPP. But, overall, very slick! It's nice to see a solution that doesn't require changing all printks statements in the kernel. Yes that's what I thought too. Thanks to the comments so far the next version of the patch will contain even less changes to the rest of the kernel. I haven't looked over this patch series yet but does it work with the pr_level macros (pr_info, pr_err, etc.)? It should work, yes. Regards, Marc -- Marc Andre Tanner http://www.brain-dump.org/ GPG key: CF7D56C0 -- To unsubscribe from this list: send the line unsubscribe linux-embedded in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] printk: provide a filtering macro for printk
Mike Frysinger wrote:
it depends completely on how the macro is intended to be used. if you
want to maintain the this macro has a return value, then you have to
use ({...}). if you want the macro to return a void, then you have to
use do{...}while(0).
Actually no. The difference is do {...} while(0) is a _statement_ and
cannot be used in an expression. Whereas a void value can be used in
expressions like A?B:C, (A,B) and returned from a function, it just
has type void.
-- Jamie
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] printk: provide a filtering macro for printk
Marc Andre Tanner wrote: + * The check with sizeof(void*) should make sure that we don't operate on + * pointers, which the compiler wouldn't be able to optimize out, but only + * on string constants. Take a look at __builtin_constant_p in the GCC manual. You'll probably find that wrapping the whole of the rest of the expression (without the sizeof) in __builtin_constant_p is a good way to know when to depend on the result of the expression. -- Jamie -- To unsubscribe from this list: send the line unsubscribe linux-embedded in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 7/7] printk: provide a filtering macro for printk
On Tuesday, September 01, 2009 4:24 PM, Tim Bird wrote:
Marc Andre Tanner wrote:
The macro filters out printk messages based on a configurable verbosity
level (CONFIG_PRINTK_VERBOSITY).
Signed-off-by: Marc Andre Tanner m...@brain-dump.org
---
include/linux/kernel.h | 24
1 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index c2b3047..1f5d01f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -242,6 +242,30 @@ asmlinkage int printk(const char * fmt, ...)
asmlinkage int printk_unfiltered(const char *fmt, ...)
__attribute__ ((format (printf, 1, 2))) __cold;
+#if defined(CONFIG_PRINTK_VERBOSITY) CONFIG_PRINTK_VERBOSITY 0
+/*
+ * The idea here is to wrap the actual printk function with a macro which
+ * will filter out all messages above a certain verbosity level. Because
+ * the if condition evaluates to a constant expression the compiler will be
+ * able to eliminate it and the resulting kernel image will be smaller.
+ *
+ * The check with sizeof(void*) should make sure that we don't operate on
+ * pointers, which the compiler wouldn't be able to optimize out, but only
+ * on string constants.
+ */
+
+#include linux/stringify.h
+
+#define printk(fmt, ...) ({
\
+if (sizeof(fmt) == sizeof(void *) ||
\
+(((const char *)(fmt))[0] != '' CONFIG_PRINTK_VERBOSITY = 4)
|| \
+(((const char *)(fmt))[0] == ''
\
+ ((const char *)(fmt))[1] =
*__stringify(CONFIG_PRINTK_VERBOSITY))) \
+printk((fmt), ##__VA_ARGS__);
\
+})
+
+#endif /* CONFIG_PRINTK_VERBOSITY */
+
extern struct ratelimit_state printk_ratelimit_state;
extern int printk_ratelimit(void);
extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
Some places in the kernel break the message into pieces, like so:
printk(KERN_ERR, Error: first part );
...
printk( more info for error.\n);
Technically, shouldn't the second part of the message actually be:
printk(KERN_CONT more info for error.\n);
Maybe some mechanism could be created to handle the continued message
if they have the KERN_CONT?
These parts would not be handled consistently under certain
conditions.
It would be confusing to see only part of the message,
but I don't know how often this construct is used. Maybe
another mechanism is needed to ensure that continuation
printk lines have the same log level as their start strings.
But, overall, very slick! It's nice to see a solution that doesn't
require changing all printks statements in the kernel.
I haven't looked over this patch series yet but does it work with the
pr_level macros (pr_info, pr_err, etc.)?
Regards,
Hartley
