After recent discussions on LKML and a general dissatisfaction at the
current printk() kernel-message logging interface, I've decided to
write down some of the ideas for a better system.
Requirements
============
* Backwards compatibility with printk(), syslog(), etc. There is no
way the whole kernel can be converted to a new interface in one go.
printk() is used all over the kernel, in many different ways,
including calls from assembly, multi-line prints spread over several
calls, etc.
* Extensibility. Features like timestamping or file/line recording
[1] should be both selectable at compile-time and (if compiled in) at
run-time.
API
===
#define kprint(fmt, ...)
The main part of the kprint interface should be the kprint() function.
The string must be a single line of information (ie. it must not
contain any newlines). Calling kprint("Hello world") is equivalent to
calling printk("Hello world\n"). kprint() may be implemented as a
macro, and must not be called from assembly.
To support the different log-levels, there exists one kprint_*()
function for each log-level, for example kprint_info(). The string
must be a single line of information. Calling kprint_emerg("Oops.") is
equivalent to calling printk(KERN_EMERG "Oops.\n"). These functions
may also be implemented as macros, and must also not be called from
assembly. The different log-levels (and their functions) are:
enum kprint_loglevel {
KPRINT_EMERG, /* kprint_emerg() */
KPRINT_ALERT, /* kprint_alert() */
KPRINT_CRIT, /* kprint_crit() */
KPRINT_ERROR, /* kprint_error() and/or kprint_err() */
KPRINT_WARNING, /* kprint_warning() and/or kprint_warn() */
KPRINT_NOTICE, /* kprint_notice() */
KPRINT_INFO, /* kprint_info() */
KPRINT_DEBUG, /* kprint_debug() */
};
In order to print several related lines as one chunk, the emitter
should first allocate an object of the type struct kprint_buffer. This
buffer is initialized with the function kprint_buffer_init() which
takes as arguments a pointer to an object of the type struct
kprint_buffer followed by the log-level number. The emitter may then
make as many or as few calls to kprint_buffer() that is desired.
kprint_buffer() is a function that takes a literal string followed by
a variable number of arguments, similarly to the kprint() function. A
final call to kprint_buffer_flush() appends the messages to the kernel
message log in a single, atomic call. After it has been flushed, the
buffer is not usable again (unless it is re-initialized). If for any
reason the buffer should be de-initialized without writing it to the
log, a call to kprint_buffer_abort() must be made.
Example: {
struct kprint_buffer buf;
kprint_buffer_init(&buf, KPRINT_DEBUG);
kprint_buffer(&buf, "Stack trace:");
while(unwind_stack()) {
kprint_buffer(&buf, "%p %s", address, symbol);
}
kprint_buffer_flush(&buf);
}
It may happen that certain parts of the kernel might wish to emit
messages to the log (and console, if any) in an early part of the boot
procedure, for example before the main memory allocation routines have
been set up properly. For this purpose, a function kprint_early()
exists. This "early" is a minimal way for the kernel to log its
functions, and may as such not include all the features of the full
kprint system. When the kernel is beyond the critical "early" point,
the messages (if any) in the "early" log may be moved into the main
logging store and kprint_early() must not be used again.
kprint_early() is a function and may be called from assembly.
To allow non-early calls from assembly, a function kprint_asm()
exists. This function takes a log-level number followed by a string
literal followed by a variable number of arguments.
The then legacy printk() function must be replaced by a
backwards-compatible but different interface. In short, printk should
parse messages, remove (and convert) initial log-level tokens, remove
any newlines (splitting the string into several lines), and call the
apropriate kprint()-system functions.
Internals
=========
The kprint() and its log-level variants are implemented as macros in
order to be able to transparently pass extra information into the main
kprint() machinery. As an example, it might happen that an extra
feature is added that prepends the current file and line (the __FILE__
and __LINE__ macros) to the message, but in such a way that it can be
discarded at run-time, or recorded along with the messages.
Additionally, this would allow the compiler to completely optimize out
calls that are below a certain log-level severity level [2][3].
With such a modular interface, message attributes (for example the
current time) and arguments can be recorded separately from the actual
format string, instead of written formatted to a ring buffer as a
sequence of characters. Parameters would be formatted to their own
strings (regardless of the original type) and saved in an array.
Example: {
struct kprint_message {
const char *format;
unsigned int argc;
char **argv;
#ifdef KPRINT_TIMESTAMP
unsigned long long timestamp;
#endif
#ifndef KPRINT_LOCATION
const char *file;
unsigned int line;
const char *function;
#endif
};
}
This can be a great help, for example in (user-space) localisation of
kernel messages [4], since the "static" message (ie. format string)
can be translated seperately and the arguments re-attached at
run-time, possibly in a different order. A new kernel-/user-space
interface would be needed to retrieve the messages in this format.
The syslog() and /proc/kmsg interfaces can retain backwards
compatibility by formatting messages as they are requested from
user-space.
Considerations
==============
This scheme is obviously much more complex than the printk() is today.
But at the same time, it is also much more powerful, extensible, and
clearly/cleanly defined.
One of the difficult things with kernel messages is that the interface
should be fail-proof. We must be able to tell the user that our memory
allocator just blew. In the current printk() interface, a ring buffer
of fixed size is used. With a fixed, static buffer, we are certain to
never run into memory-related problems, even if the rest of the system
is unstable or unusable.
I think an easy solution would be to check for EMERG messages and
treat these with special care, like using a static reservoir.
The message entry and a message's arguments are kept separately. Most
likely, there will be a huge number of tiny allocations. I am not sure
how well this is handled in the kernel. Or we could try to merge the
allocation of the struct kprint_message and its arguments into a
single allocation by looking at the arguments before they're
formatted. After all, the arguments cannot exist without the message
or vice versa. Alternatively, a statically-sized ring buffer of struct
kprint_message objects could be used, and then only arguments would
need to be allocated dynamically. Either way, I think it should be
possible to come up with a fairly memory-efficient system even for
this method of storing the kernel log.
Also undealt with are dev_printk(), sdev_printk(), and
ata_dev_printk(), but our backwards compatible printk() should handle
this. Alternatively, the macros could be changed, though as I know
close to nothing about them, I'll leave this entire field to somebody
else.
References:
[1] http://lkml.org/lkml/2007/9/21/267 (Joe Perches <[email protected]>)
[2] http://lkml.org/lkml/2007/9/20/352 (Rob Landley <[email protected]>)
[3] http://lkml.org/lkml/2007/9/21/151 (Dick Streefland
<[email protected]>)
[4] http://lkml.org/lkml/2007/6/13/146 (holzheu <[email protected])
Vegard
On Sat, 2007-09-22 at 21:27 +0200, Vegard Nossum wrote:
> #define kprint(fmt, ...)
Good ideas. Perhaps a prefix of klog or kp_ instead?
Given the number of 80 column zealots, character naming length matters.
> Additionally, this would allow the compiler to completely optimize out
> calls that are below a certain log-level severity level [2][3].
Severity doesn't really equate to desire to log.
I'd prefer arbitrary control of log levels.
cheers, Joe
On Sep 22, 2007, at 20:13:03, Joe Perches wrote:
> On Sat, 2007-09-22 at 21:27 +0200, Vegard Nossum wrote:
>> Additionally, this would allow the compiler to completely optimize
>> out calls that are below a certain log-level severity level [2][3].
>
> Severity doesn't really equate to desire to log. I'd prefer
> arbitrary control of log levels.
Umm, actually... Severity is *exactly* "desirability of logging". If
you find any place that's otherwise then please provide patches to
fix it (or at least notify the maintainer with a useful bug report
and reasoning.
Cheers,
Kyle Moffett
On Sat, 2007-09-22 at 20:40 -0400, Kyle Moffett wrote:
> Severity is *exactly* "desirability of logging".
Disagree.
What's info to someone is an alert to someone else.
The problem is the valuation of the reasoning.
It's all opinion.
cheers, Joe
On 9/22/07, Vegard Nossum <[email protected]> wrote:
> After recent discussions on LKML and a general dissatisfaction at the
> current printk() kernel-message logging interface, I've decided to
> write down some of the ideas for a better system.
Nice. I would suggest having some kind of standard way to show the
information on the screen/dmesg. I mean, instead of having plain lines
being written to the log, having something very short like:
SSL: Message sent.
Being:
SS - Subsystem ("EA" for early code, "MM" for memory managment, "PU"
for processor stuff, "HD" for hard disks, "PP" for parallel port, "NT"
for networking, "VI" for video stuff, "FB" for framebuffers, "SN" for
sound stuff, "KE" for keyboard, "MO" for mouse, ... I think you get
the idea, just generic things).
L - Log level (0 for emerg, ..., 7 for debug)
And maybe some other character for other information. This would be
great to read pretty easily dmesg and for grepping, like:
$ dmesg | grep ^FB
for getting just information about framebuffers, or
$ dmesg | grep ^..[0123]
to get all the problems of the whole kernel/system.
So, for example, userspace scripts will be able to separate into
different log files the kernel stuff:
#!/bin/sh
dmesg | grep ^..[0123] > klog.errors
dmesg | grep ^NT > klog.networking
dmesg | grep ^HD > klog.harddisks
dmesg | grep ^FB > klog.framebuffers
Maybe its weird at first, but I think it will speed up the reading of
plain dmesg's outputs for everyone at the cost of 3-5 more characters
at every line in dmesg.
Also, it may help to make printk()'s messages to be more uniform,
instead of having every .c file having differents ways to express
similar things.
Getting more complex, lets add another character:
SSLR: Message sent.
being R the reason of the message (D for information about a
probed-and-detected hardware [like a PCI card], R for a new
succesfully registered device [like a framebuffer], S for new settings
in a device [like taking up a ethernet link, or that messages about
IRQs], C for copyright/about/info lines, ...).
Now we have at some dmesg (for example):
eth0: Broadcom 4400 10/100BaseT Ethernet 00:1f:a2:0c:4a:72
ieee80211_crypt: registered algorithm 'TKIP'
ipw3945: Intel(R) PRO/Wireless 3945 Network Connection 1.2.2d
ipw3945: Copyright(c) 2003-2006 Intel Corporation
ACPI: PCI Interrupt 0000:0c:00.0[A] -> GSI 17 (level, low) -> IRQ 17
PCI: Setting latency timer of device 0000:0c:00.0 to 64
ipw3945: Detected Intel PRO/Wireless 3945ABG Network Connection
ipw3945: Detected geography ABG (13 802.11bg channels, 23 802.11a channels)
usb 6-1: new low speed USB device using uhci_hcd and address 2
usb 6-1: configuration #1 chosen from 1 choice
usbcore: registered new interface driver hiddev
and we could have:
NT6R eth0: Broadcom 4400 10/100BaseT Ethernet 00:1f:a2:0c:4a:72
NT6R ieee80211_crypt: algorithm 'TKIP'
NT6C ipw3945: Intel(R) PRO/Wireless 3945 Network Connection driver 1.2.2d
NT7C ipw3945: Copyright(c) 2003-2006 Intel Corporation
XX6S ACPI: PCI Interrupt 0000:0c:00.0[A] -> GSI 17 (level, low) -> IRQ 17
XX6S PCI: Latency timer of device 0000:0c:00.0 to 64
NT6R ipw3945: Intel PRO/Wireless 3945ABG Network Connection
NT6D ipw3945: geography ABG (13 802.11bg channels, 23 802.11a channels)
US6R usb 6-1: low speed USB device using uhci_hcd and address 2
US6S usb 6-1: configuration #1 chosen from 1 choice
US6R usbcore: new interface driver hiddev
which I think its much more clear... Wanna know about registered
networking-related devices? Then grep using "^NT.R":
NT6R eth0: Broadcom 4400 10/100BaseT Ethernet 00:1f:a2:0c:4a:72
NT6R ieee80211_crypt: algorithm 'TKIP'
NT6R ipw3945: Intel PRO/Wireless 3945ABG Network Connection
...
Nothing more, thanks for reading :)
--
Miguel Ojeda
http://maxextreme.googlepages.com/index.htm
On Sep 22, 2007, at 20:47:21, Joe Perches wrote:
> On Sat, 2007-09-22 at 20:40 -0400, Kyle Moffett wrote:
>> Severity is *exactly* "desirability of logging".
>
> Disagree.
>
> What's info to someone is an alert to someone else. The problem is
> the valuation of the reasoning.
>
> It's all opinion.
For starters, I think "its all opinion" is bogus; the value of each
log level is pretty standardly defined for the kernel:
EMERG: The system is completely failing right NOW. Immediate admin
action is definitely required
ALERT: The system is about to fail or halfway-failing. Immediate
admin action may be required.
ERR: Something went badly wrong. We are able to continue without
endangering the overall system.
NOTICE: Something unusual happened but it's either not a device/code
failure or it's an expected failure.
INFO: Something useful happened, like a device probe or a major
system-wide event
DEBUG: Something happened that only a developer or somebody looking
for bugs would care about.
All of those are basically defined as: "This is how much the admin
probably wants to see this message". If you find a message which
doesn't seem to match the log level it's logged at, please submit a
patch.
Even assuming its not a bogus argument, what you actually want is
*NOT* "Show me INFO and DEBUG, but not EMERG, ALERT, ERR, or NOTICE",
you actually want multiple metrics by which to measure things. We
already have multiple ways of filtering log messages:
1) Log-level (greater than $LEVEL).
2) Userspace textual post-processing filters and classifiers (EG:
logcheck)
3) CONFIG_${SUBSYS}_DEBUG and CONFIG_${DRIVER}_DEBUG
Perhaps what you want is something like the syslog "facility" parameter?
The only real reason to add more kconfig options for logging is to
decrease the size of the kernel binary; most anything else can be
better done in userspace in something like logcheck. If you're going
to add new kconfig options then you aren't going to configure them
generically since they're inherently subsystem/driver specific. In
that case you might add a per-driver "CONFIG_${DRIVER}_NOLOG" for the
drivers you care about or a "CONFIG_MAX_LOG_LEVEL" to globally limit
by severity but any other qualifiers inherently don't fit in a single
scalar
Cheers,
Kyle Moffett
On 9/23/07, Joe Perches <[email protected]> wrote:
> On Sat, 2007-09-22 at 21:27 +0200, Vegard Nossum wrote:
> > #define kprint(fmt, ...)
>
> Good ideas. Perhaps a prefix of klog or kp_ instead?
> Given the number of 80 column zealots, character naming length matters.
I don't know. Compare the following two lines:
printk(KERN_INFO "Message.\n");
kprint_info("Message.");
By dropping the lengthy macro (it's not like it's going to change
while we're running anyway, so why not make it a part of the function
name?) and the final newline, we actually end up with a net decrease
in line length.
I thought it would be nice to have something that looks familiar,
since that would ease an eventual transition. klog is a valid
alternative, but isn't kp a bit cryptic?
Vegard
On Sep 23 2007 10:39, Vegard Nossum wrote:
>On 9/23/07, Joe Perches <[email protected]> wrote:
>> On Sat, 2007-09-22 at 21:27 +0200, Vegard Nossum wrote:
>> > #define kprint(fmt, ...)
>>
>> Good ideas. Perhaps a prefix of klog or kp_ instead?
>> Given the number of 80 column zealots, character naming length matters.
>
>I don't know. Compare the following two lines:
>
>printk(KERN_INFO "Message.\n");
>kprint_info("Message.");
>
>By dropping the lengthy macro (it's not like it's going to change
>while we're running anyway, so why not make it a part of the function
>name?) and the final newline, we actually end up with a net decrease
>in line length.
>
>I thought it would be nice to have something that looks familiar,
>since that would ease an eventual transition. klog is a valid
>alternative, but isn't kp a bit cryptic?
I'd rather fix up code to reduce its indent rather than
trying microoptimizations at the function name level!
On 9/23/07, Jan Engelhardt <[email protected]> wrote:
>
> On Sep 23 2007 10:39, Vegard Nossum wrote:
> >On 9/23/07, Joe Perches <[email protected]> wrote:
> >> On Sat, 2007-09-22 at 21:27 +0200, Vegard Nossum wrote:
> >> > #define kprint(fmt, ...)
> >>
> >> Good ideas. Perhaps a prefix of klog or kp_ instead?
> >> Given the number of 80 column zealots, character naming length matters.
> >
> >I don't know. Compare the following two lines:
> >
> >printk(KERN_INFO "Message.\n");
> >kprint_info("Message.");
> >
> >By dropping the lengthy macro (it's not like it's going to change
> >while we're running anyway, so why not make it a part of the function
> >name?) and the final newline, we actually end up with a net decrease
> >in line length.
> >
> >I thought it would be nice to have something that looks familiar,
> >since that would ease an eventual transition. klog is a valid
> >alternative, but isn't kp a bit cryptic?
>
> I'd rather fix up code to reduce its indent rather than
> trying microoptimizations at the function name level!
Well, that's a different discussion, really. Is fixing the body of
[v]printk() enough? I don't think it's possible if you want to do
everything that I described in my original post.
It's not even an optimization from my point of view; I think Joe
Perches was arguing that code should be kept short. I was arguing that
it's already shorter than with the current situation.
Vegard
On Sep 23 2007 11:20, Vegard Nossum wrote:
>> >printk(KERN_INFO "Message.\n");
>> >kprint_info("Message.");
>>
>> I'd rather fix up code to reduce its indent rather than
>> trying microoptimizations at the function name level!
>
>Well, that's a different discussion, really. Is fixing the body of
>[v]printk() enough?
I mean the bodies of functions calling printk. (where possible)
Like this:
diff --git a/fs/quota_v2.c b/fs/quota_v2.c
index c519a58..49e04e7 100644
--- a/fs/quota_v2.c
+++ b/fs/quota_v2.c
@@ -86,12 +86,12 @@ static int v2_write_file_info(struct super_block *sb, int type)
dinfo.dqi_free_entry = cpu_to_le32(info->u.v2_i.dqi_free_entry);
size = sb->s_op->quota_write(sb, type, (char *)&dinfo,
sizeof(struct v2_disk_dqinfo), V2_DQINFOOFF);
- if (size != sizeof(struct v2_disk_dqinfo)) {
- printk(KERN_WARNING "Can't write info structure on device %s.\n",
- sb->s_id);
- return -1;
- }
- return 0;
+ if (size == sizeof(struct v2_disk_dqinfo))
+ return 0;
+
+ printk(KERN_WARNING "Can't write info structure on device %s.\n",
+ sb->s_id);
+ return -1;
}
static void disk2memdqb(struct mem_dqblk *m, struct v2_disk_dqblk *d)
I gained 8 columns _without_ fiddling with the names.
On Sun, 2007-09-23 at 10:45 +0200, Jan Engelhardt wrote:
> I'd rather fix up code to reduce its indent rather than
> trying microoptimizations at the function name level!
I think that's a different discussion.
You could fix some of them whenever you like.
$ egrep -r -l --include=*.c "^[[:cntrl:]]{6,}printk" *
cheers, Joe
On Sun, 2007-09-23 at 10:39 +0200, Vegard Nossum wrote:
> On 9/23/07, Joe Perches <[email protected]> wrote:
> > Given the number of 80 column zealots, character naming length
> > matters.
> I don't know. Compare the following two lines:
> printk(KERN_INFO "Message.\n");
> kprint_info("Message.");
The problem isn't printk(KERN_<level> to kprint_<level>(.
The problems are the no level printk(foo)s to kprint_<level>(foo)
and the pr_<level>() to kprint_<level>() translations.
> By dropping the lengthy macro (it's not like it's going to change
> while we're running anyway, so why not make it a part of the function
> name?) and the final newline, we actually end up with a net decrease
> in line length.
Which I do appreciate and think good.
> I thought it would be nice to have something that looks familiar,
> since that would ease an eventual transition. klog is a valid
> alternative, but isn't kp a bit cryptic?
Probably no more than pr_<level> is today.
cheers, Joe
On Saturday 22 September 2007 2:27:29 pm Vegard Nossum wrote:
> After recent discussions on LKML and a general dissatisfaction at the
> current printk() kernel-message logging interface, I've decided to
> write down some of the ideas for a better system.
>
>
> Requirements
> ============
>
> * Backwards compatibility with printk(), syslog(), etc.
I.E. what we have now works just fine for what it does.
> * Extensibility. Features like timestamping or file/line recording
> [1] should be both selectable at compile-time and (if compiled in) at
> run-time.
That doesn't require changing the API. Allowing the compiler to eliminate
messages below a threshold requires changing the API.
>
> API
> ===
>
> #define kprint(fmt, ...)
>
> The main part of the kprint interface should be the kprint() function.
And then you propose not having a single kprint() function...
> To support the different log-levels, there exists one kprint_*()
> function for each log-level, for example kprint_info().
Why is this better than feeding the level in as an argument to the macro?
> In order to print several related lines as one chunk, the emitter
> should first allocate an object of the type struct kprint_buffer.
You know, I'm pretty happy with a first pass that doesn't address this issue
at all. Why bundle three unrelated problems into a single all-or-nothing
pass?
Rob
--
"One of my most productive days was throwing away 1000 lines of code."
- Ken Thompson.
Hi Vegard,
On Sat, 2007-09-22 at 21:27 +0200, Vegard Nossum wrote:
> After recent discussions on LKML and a general dissatisfaction at the
> current printk() kernel-message logging interface, I've decided to
> write down some of the ideas for a better system.
Good luck :-)
[snip]
> Example: {
> struct kprint_buffer buf;
>
> kprint_buffer_init(&buf, KPRINT_DEBUG);
> kprint_buffer(&buf, "Stack trace:");
>
> while(unwind_stack()) {
> kprint_buffer(&buf, "%p %s", address, symbol);
> }
>
> kprint_buffer_flush(&buf);
> }
>
Together with the idea of not allowing multiple lines in the kprint_xxx
functions, that would go with our approach having message numbers to
identify a message. Multiple lines are combined explicitly to one
message. I think it is a good idea to be able to identify, which lines
of a message belong together.
[snip]
> With such a modular interface, message attributes (for example the
> current time) and arguments can be recorded separately from the actual
> format string, instead of written formatted to a ring buffer as a
> sequence of characters. Parameters would be formatted to their own
> strings (regardless of the original type) and saved in an array.
>
> Example: {
> struct kprint_message {
> const char *format;
>
> unsigned int argc;
> char **argv;
>
> #ifdef KPRINT_TIMESTAMP
> unsigned long long timestamp;
> #endif
>
> #ifndef KPRINT_LOCATION
> const char *file;
> unsigned int line;
>
> const char *function;
> #endif
> };
> }
[snip]
> The message entry and a message's arguments are kept separately. Most
> likely, there will be a huge number of tiny allocations. I am not sure
> how well this is handled in the kernel. Or we could try to merge the
> allocation of the struct kprint_message and its arguments into a
> single allocation by looking at the arguments before they're
> formatted. After all, the arguments cannot exist without the message
> or vice versa. Alternatively, a statically-sized ring buffer of struct
> kprint_message objects could be used, and then only arguments would
> need to be allocated dynamically. Either way, I think it should be
> possible to come up with a fairly memory-efficient system even for
> this method of storing the kernel log.
Would be nice to have some code here. How do you want to implement that?
You have to allocate / preallocate memory for the argv array. Something
like:
kprint_err(const char* fmt, ...)
{
va_list ap;
struct kprint_message *msg;
msg = &message_arry[current];
va_start(ap, fmt);
msg->argv = kmalloc(sizeof(long) * argc, GFP_KERNEL);
...
for (i = 0; i < argc, i++) {
msg->argv[i] = va_arg(ap, long);
}
If you do it like that, you can't support "%s", since then you would
store only the pointer and not the whole string. I think, that we can't
live without %s.
Michael
On Mon, 2007-09-24 at 11:22 +0200, Michael Holzheu wrote:
> Together with the idea of not allowing multiple lines in the kprint_xxx
> functions, that would go with our approach having message numbers to
> identify a message.
How does this equate/give message numbers?
> If you do it like that, you can't support "%s", since then you would
> store only the pointer and not the whole string. I think, that we can't
> live without %s.
long long would not work either.
Perhaps it's better to scan the format string or indirect the
printk formatting logic.
Another possibility:
An added pass between gcc preprocessor and compiler could compact
or compress the format string without modifying the conversion
specifications so __attribute__ ((format (printf)) would still work.
printk could expand the compacted/compressed format.
On 9/24/07, Joe Perches <[email protected]> wrote:
> On Mon, 2007-09-24 at 11:22 +0200, Michael Holzheu wrote:
> > Together with the idea of not allowing multiple lines in the kprint_xxx
> > functions, that would go with our approach having message numbers to
> > identify a message.
>
> How does this equate/give message numbers?
Storing the format-string separately allows us to hash THAT instead of
the formatted (ie. console output) message. Since this will never
change from message to message, it can be looked up in a table or
whatever and allow user-space to do translations without for example
regular expressions.
I'm pretty sure this *would* be possible to do with existing printk(),
but I'm betting it also wouldn't look very nice (i.e. hackery).
> > If you do it like that, you can't support "%s", since then you would
> > store only the pointer and not the whole string. I think, that we can't
> > live without %s.
>
> long long would not work either.
>
> Perhaps it's better to scan the format string or indirect the
> printk formatting logic.
The idea was to not use snprintf() for the stored message, but have a
printf-like function that instead writes the output of each format
specifier to its own string (ie. each argument produces a new char*
object). Also needed would be an additional function that reads the
format string but uses the stored arguments.
This turned out to be slightly harder than I assumed at first, though
it's of course not impossible. The downside of this approach is of
course the need to maintain normal vsnprintf() and new functions side
by side, with little possibility of reusing the format parsing of
vsnprintf().
I will follow up with some code to demonstrate as soon as I can.
Vegard
On Mon, 2007-09-24 at 18:43 +0200, Vegard Nossum wrote:
> Storing the format-string separately allows us to hash THAT instead of
> the formatted (ie. console output) message. Since this will never
> change from message to message, it can be looked up in a table or
> whatever and allow user-space to do translations without for example
> regular expressions.
That hash will change with each linux version given the
inevitable spelling fixes, message reformatting and such.
> I will follow up with some code to demonstrate as soon as I can.
Looking forward to it.
On Saturday, September 22, 2007 12:27 pm Vegard Nossum wrote:
> enum kprint_loglevel {
> KPRINT_EMERG, /* kprint_emerg() */
> KPRINT_ALERT, /* kprint_alert() */
> KPRINT_CRIT, /* kprint_crit() */
> KPRINT_ERROR, /* kprint_error() and/or kprint_err() */
> KPRINT_WARNING, /* kprint_warning() and/or kprint_warn() */
> KPRINT_NOTICE, /* kprint_notice() */
> KPRINT_INFO, /* kprint_info() */
> KPRINT_DEBUG, /* kprint_debug() */
> };
I wonder if all these levels are still needed (though I really like the
error/err & warning/warn aliases, those always get me :).
It seems like fewer levels would make things easier on both kernel
developers and administrators; looking at current counts may help
figure out which ones could be combined (warning, very naive grep -r
data):
KERN_EMERG: 371
KERN_ALERT: 236
KERN_CRIT: 602
KERN_ERR: 11961
KERN_WARNING: 6463
KERN_NOTICE: 1142
KERN_INFO: 8491
KERN_DEBUG: 6125
So KERN_ERR is the most common by a pretty large margin, though it seems
to me that KERN_NOTICE, KERN_INFO and KERN_DEBUG are mostly redundant
and probably make up a majority of the "SIMD FPU exception support was
enabled" (as if I care) type messages. Likewise, ERR, ALERT, CRIT and
EMERG serve very similar purposes (i.e. something unrecoverable
occurred), maybe they could be condensed into one or two levels rather
than four? So that would drop us to three levels:
KERN_ERR /* something really bad happened, machine is dead or near so */
KERN_WARNING /* you really ought to know about this */
KERN_INFO /* no one but the kernel developer likely cares about this */
But maybe I'm just living in a dream world where then number of printks
the kernel spits out suddenly drops by 99% and only actually important
messages make it to my log...
Jesse
On 9/24/07, Joe Perches <[email protected]> wrote:
> On Mon, 2007-09-24 at 18:43 +0200, Vegard Nossum wrote:
> > Storing the format-string separately allows us to hash THAT instead of
> > the formatted (ie. console output) message. Since this will never
> > change from message to message, it can be looked up in a table or
> > whatever and allow user-space to do translations without for example
> > regular expressions.
>
> That hash will change with each linux version given the
> inevitable spelling fixes, message reformatting and such.
But we can keep the old ones too. That shouldn't be much of a problem.
I mean, it probably wouldn't rely on a hash alone. The format string
itself can be compared with the translation database.
> > I will follow up with some code to demonstrate as soon as I can.
>
> Looking forward to it.
Okay, so I have one huge file that does more or less what I want. The
main problem with this, as I see it, is that it largely duplicates
vsnprintf() from lib/vsprintf.c (minus the functions I coped verbatim
from the same file). This is bad because the job of maintaining that
is now doubled (or worse). Hopefully it won't change much in the
future either.
For now, the main() function is most important. This demonstrates that
what I want is in fact possible. printf() is now split in two --
args_printf_prepare() and args_snprintf(). The former takes your
arguments as you would pass them to any *printff() function, but only
converts your arguments (and stores them in a struct args). The latter
takes the format string and the stored struct args and puts the whole
thing in a buffer (just like snprintf()).
I know Gmail isn't too friendly with code formatting, but this is just
an example program anyway, so it shouldn't really matter. Also, a
disclaimer: Almost completely untested.
Vegard
#define _GNU_SOURCE
#include <ctype.h>
#include <stdarg.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
/* This was shamelessly copied form include/asm-generic/div64.h */
# define do_div(n,base) ({ \
uint32_t __base = (base); \
uint32_t __rem; \
__rem = ((uint64_t)(n)) % __base; \
(n) = ((uint64_t)(n)) / __base; \
__rem; \
})
/* This was shamelessly copied from lib/vsprintf.c */
static char* put_dec_trunc(char *buf, unsigned q)
{
unsigned d3, d2, d1, d0;
d1 = (q>>4) & 0xf;
d2 = (q>>8) & 0xf;
d3 = (q>>12);
d0 = 6*(d3 + d2 + d1) + (q & 0xf);
q = (d0 * 0xcd) >> 11;
d0 = d0 - 10*q;
*buf++ = d0 + '0'; /* least significant digit */
d1 = q + 9*d3 + 5*d2 + d1;
if (d1 != 0) {
q = (d1 * 0xcd) >> 11;
d1 = d1 - 10*q;
*buf++ = d1 + '0'; /* next digit */
d2 = q + 2*d2;
if ((d2 != 0) || (d3 != 0)) {
q = (d2 * 0xd) >> 7;
d2 = d2 - 10*q;
*buf++ = d2 + '0'; /* next digit */
d3 = q + 4*d3;
if (d3 != 0) {
q = (d3 * 0xcd) >> 11;
d3 = d3 - 10*q;
*buf++ = d3 + '0'; /* next digit */
if (q != 0)
*buf++ = q + '0'; /* most sign. digit */
}
}
}
return buf;
}
static char* put_dec_full(char *buf, unsigned q)
{
unsigned d3, d2, d1, d0;
d1 = (q>>4) & 0xf;
d2 = (q>>8) & 0xf;
d3 = (q>>12);
d0 = 6*(d3 + d2 + d1) + (q & 0xf);
q = (d0 * 0xcd) >> 11;
d0 = d0 - 10*q;
*buf++ = d0 + '0';
d1 = q + 9*d3 + 5*d2 + d1;
q = (d1 * 0xcd) >> 11;
d1 = d1 - 10*q;
*buf++ = d1 + '0';
d2 = q + 2*d2;
q = (d2 * 0xd) >> 7;
d2 = d2 - 10*q;
*buf++ = d2 + '0';
d3 = q + 4*d3;
q = (d3 * 0xcd) >> 11; /* - shorter code */
d3 = d3 - 10*q;
*buf++ = d3 + '0';
*buf++ = q + '0';
return buf;
}
static char* put_dec(char *buf, unsigned long long num)
{
while (1) {
unsigned rem;
if (num < 100000)
return put_dec_trunc(buf, num);
rem = do_div(num, 100000);
buf = put_dec_full(buf, rem);
}
}
#define ZEROPAD 1 /* pad with zero */
#define SIGN 2 /* unsigned/signed long */
#define PLUS 4 /* show plus */
#define SPACE 8 /* space if plus */
#define LEFT 16 /* left justified */
#define SPECIAL 32 /* 0x */
#define LARGE 64 /* use 'ABCDEF' instead of 'abcdef' */
static int skip_atoi(const char **s)
{
int i=0;
while (isdigit(**s))
i = i*10 + *((*s)++) - '0';
return i;
}
/* End of shameless copy. */
/**
* Simple string buffer implementation
*/
struct buffer {
unsigned int size;
unsigned int pos;
char *data;
};
void buffer_init(struct buffer *buf)
{
buf->size = 0;
buf->pos = 0;
buf->data = NULL;
}
void buffer_deinit(struct buffer *buf)
{
free(buf->data);
buf->size = 0;
buf->pos = 0;
buf->data = NULL;
}
void buffer_write(struct buffer *buf, char c)
{
if(buf->pos == buf->size) {
buf->size = buf->size ? 2 * buf->size : 8;
buf->data = realloc(buf->data, buf->size);
}
buf->data[buf->pos++] = c;
}
/* The equivalent of number() in lib/vsprintf.c */
void buffer_write_number(struct buffer *buf, unsigned long long num, int base,
int size, int precision, int type)
{
char sign, tmp[66];
const char *digits;
static const char small_digits[] = "0123456789abcdefx";
static const char large_digits[] = "0123456789ABCDEFX";
int need_pfx = ((type & SPECIAL) && base != 10);
int i;
digits = (type & LARGE) ? large_digits : small_digits;
if(type & LEFT)
type &= ~ZEROPAD;
if(base < 2 || base > 36)
return;
sign = 0;
if(type & SIGN) {
if((signed long long) num < 0) {
sign = '-';
num = -(signed long long) num;
--size;
} else if(type & PLUS) {
sign = '+';
--size;
} else if(type & SPACE) {
sign = ' ';
--size;
}
}
if(need_pfx) {
--size;
if(base == 16)
size--;
}
i = 0;
if(num == 0)
tmp[i++] = '0';
else if(base != 10) {
int mask = base - 1;
int shift = 3;
if(base == 16)
shift = 4;
do {
tmp[i++] = digits[((unsigned char) num) & mask];
num >>= shift;
} while(num);
} else {
i = put_dec(tmp, num) - tmp;
}
if(i > precision)
precision = i;
size -= precision;
if(!(type & (ZEROPAD + LEFT))) {
while(--size >= 0)
buffer_write(buf, ' ');
}
if(sign)
buffer_write(buf, sign);
if(need_pfx) {
buffer_write(buf, '0');
if(base == 16)
buffer_write(buf, digits[16]);
}
if(!(type & LEFT)) {
char c = (type & ZEROPAD) ? '0' : ' ';
while(--size >= 0)
buffer_write(buf, c);
}
while(i <= --precision)
buffer_write(buf, '0');
while(--i >= 0)
buffer_write(buf, tmp[i]);
while(--size >= 0)
buffer_write(buf, ' ');
}
char *buffer_dup(struct buffer *buf)
{
return strndup(buf->data, buf->pos);
}
/**
* Expanding args-array
*/
struct args {
unsigned int size;
unsigned int argc;
char **argv;
};
void args_push(struct args *args, char *arg)
{
if(args->argc == args->size) {
args->size = args->size ? 2 * args->size : 8;
args->argv = realloc(args->argv, args->size);
}
args->argv[args->argc++] = arg;
}
void args_vprintf_prepare(struct args *args, const char *fmt, va_list ap)
{
const char *p;
for(p = fmt; *p; ++p) {
int flags;
int field_width;
int precision;
int qualifier;
int base;
int i;
char c;
const char *s;
int len;
unsigned long long num;
struct buffer str;
/* skip to the next format */
if(*p != '%')
continue;
buffer_init(&str);
/* read flags */
flags = 0;
repeat:
++p;
switch(*p) {
case '-': flags |= LEFT; goto repeat;
case '+': flags |= PLUS; goto repeat;
case ' ': flags |= SPACE; goto repeat;
case '#': flags |= SPECIAL; goto repeat;
case '0': flags |= ZEROPAD; goto repeat;
}
/* read field width */
field_width = -1;
if(isdigit(*p))
field_width = skip_atoi(&p);
else if(*p == '*') {
++p;
field_width = va_arg(ap, int);
if(field_width < 0) {
field_width = -field_width;
flags |= LEFT;
}
}
/* read precision */
precision = -1;
if(*p == '.') {
++p;
if(isdigit(*p))
precision = skip_atoi(&p);
else if(*p == '*') {
++p;
precision = va_arg(ap, int);
}
if(precision < 0)
precision = 0;
}
/* read conversion qualifier */
qualifier = -1;
if(*p == 'h' || *p == 'l' || *p == 'L'
|| *p == 'Z' || *p == 'z' || *p == 't')
{
qualifier = *p;
++p;
if(qualifier == 'l' && *p == 'l') {
qualifier = 'L';
++p;
}
}
/* default base */
base = 10;
/* XXX: only %c and %s supported yet */
switch(*p) {
case 'c':
if(!(flags & LEFT)) {
while(--field_width > 0)
buffer_write(&str, ' ');
}
c = (unsigned char) va_arg(ap, int);
buffer_write(&str, c);
while(--field_width > 0)
buffer_write(&str, ' ');
args_push(args, buffer_dup(&str));
buffer_deinit(&str);
continue;
case 's':
/* XXX: see vnprintf() in lib/vsprintf.c */
s = va_arg(ap, char *);
len = strnlen(s, precision);
if(!(flags & LEFT)) {
while(len < field_width--)
buffer_write(&str, ' ');
}
for(i = 0; i < len; ++i, ++s)
buffer_write(&str, *s);
while(len < field_width--)
buffer_write(&str, ' ');
args_push(args, buffer_dup(&str));
buffer_deinit(&str);
continue;
case 'p':
if(field_width == -1) {
field_width = 2 * sizeof(void *);
flags |= ZEROPAD;
}
buffer_write_number(&str,
(unsigned long) va_arg(ap, void *),
16, field_width, precision, flags);
args_push(args, buffer_dup(&str));
buffer_deinit(&str);
continue;
case 'n':
/* Nothing happens. */
buffer_deinit(&str);
continue;
case '%':
/* Nothing happens. */
buffer_deinit(&str);
continue;
case 'o':
base = 8;
break;
case 'X':
flags |= LARGE;
case 'x':
base = 16;
break;
case 'd':
case 'i':
flags |= SIGN;
case 'u':
break;
default:
/* In this case, we need to pop an empty argument in
* the real snprintf(). */
args_push(args, buffer_dup(&str));
buffer_deinit(&str);
continue;
}
if(qualifier == 'L')
num = va_arg(ap, long long);
else if(qualifier == 'l') {
num = va_arg(ap, unsigned long);
if(flags & SIGN)
num = (signed long) num;
} else if(qualifier == 'Z' || qualifier == 'z') {
num = va_arg(ap, size_t);
} else if(qualifier == 't') {
#define ptrdiff_t typeof((void *) 0 - (void *) 0)
num = va_arg(ap, ptrdiff_t);
} else if(qualifier == 'h') {
num = (unsigned short) va_arg(ap, int);
if(flags & SIGN)
num = (signed short) num;
} else {
num = va_arg(ap, unsigned int);
if(flags & SIGN)
num = (signed int) num;
}
buffer_write_number(&str, num, base, field_width,
precision, flags);
args_push(args, buffer_dup(&str));
buffer_deinit(&str);
}
}
void args_printf_prepare(struct args *args, const char *fmt, ...)
{
va_list ap;
va_start(ap, fmt);
args_vprintf_prepare(args, fmt, ap);
va_end(ap);
}
/* Here we just parse the format string without really caring about the field
* parameters; they've been formatted already by args_printf_prepare(). */
int args_snprintf(struct args *args,
char *buf, size_t size, const char *fmt)
{
const char *p;
unsigned int argp = 0;
char *str;
char *end;
if((int) size < 0)
return 0;
str = buf;
end = buf + size;
/* Make sure end is always >= buf */
if(end < buf) {
end = ((void *) -1);
size = end - buf;
}
for(p = fmt; *p; ++p) {
char *arg;
if(*p != '%') {
if(str < end)
*str = *p;
++str;
continue;
}
/* skip flags */
repeat:
++p;
switch(*p) {
case '-': goto repeat;
case '+': goto repeat;
case ' ': goto repeat;
case '#': goto repeat;
case '0': goto repeat;
}
/* skip field width */
if(*p == '*')
++p;
else while(isdigit(*p))
++p;
/* skip precision */
if(*p == '.') {
++p;
if(*p == '*')
++p;
else while(isdigit(*p))
++p;
}
/* skip the conversion qualifier */
if(*p == 'h' || *p == 'l' || *p == 'L'
|| *p == 'Z' || *p == 'z' || *p == 't')
{
if(*p == 'l') {
++p;
if(*p == 'l')
++p;
} else
++p;
}
/* skip the conversion specifier */
switch(*p) {
case 'n':
/* XXX: (not implemented) */
continue;
case '%':
if(str < end)
*str = '%';
++str;
continue;
}
for(arg = args->argv[argp]; *arg; ++arg) {
if(str < end)
*str = *arg;
++str;
}
++argp;
}
if(size > 0) {
if(str < end)
*str = '\0';
else
end[-1] = '\0';
}
return str - buf;
}
int
main(int argc, char *argv[])
{
const char format[] = "Hello %s (%d times)!";
struct args args;
args.size = 0;
args.argc = 0;
args.argv = NULL;
args_printf_prepare(&args, format, "world", 5);
/* We can do whatever we want with the arguments now, like dumping
* them. */
{
unsigned int i;
printf("Argument dump:\n");
for(i = 0; i < args.argc; i++)
printf("arg #%d: %s\n", 1 + i, args.argv[i]);
printf("\n");
}
/* Use the format string and our "prepared" (ie. pre-formatted)
* argument list to actually format the string. */
{
static char out[512];
printf("Reformatting:\n");
args_snprintf(&args, out, sizeof(out), format);
printf("%s\n", out);
printf("\n");
}
return 0;
}
On Monday 24 September 2007 10:19:16 am Joe Perches wrote:
> On Mon, 2007-09-24 at 11:22 +0200, Michael Holzheu wrote:
> > Together with the idea of not allowing multiple lines in the kprint_xxx
> > functions, that would go with our approach having message numbers to
> > identify a message.
>
> How does this equate/give message numbers?
I actively want to avoid giving message numbers. My interest is in
selectively removing messages from the kernel to shrink the binary size (and
NOT make it up in either a larger userspace utility to translate them, or
else magic proprietary numbers you can only diagnose if you pay my support
staff).
> An added pass between gcc preprocessor and compiler could compact
> or compress the format string without modifying the conversion
> specifications so __attribute__ ((format (printf)) would still work.
This does not address my problem. Spitting out a proprietary hash code
instead of a human readable message is not a solution for my use case.
Rob
--
"One of my most productive days was throwing away 1000 lines of code."
- Ken Thompson.
On Mon, 2007-09-24 at 18:51 -0500, Rob Landley wrote:
> > An added pass between gcc preprocessor and compiler could compact
> > or compress the format string without modifying the conversion
> > specifications so __attribute__ ((format (printf)) would still work.
> This does not address my problem. Spitting out a proprietary hash code
> instead of a human readable message is not a solution for my use case.
What is your problem Rob?
I think message numbers are difficult to produce from format strings.
I think kernel version/file/func/line is enough to identify messages
for normal use but not too useful for embedded.
I think losslessly compressing, not hashing, the format string
would be useful. The format string would be expanded by printk.
The kernel size would be smaller and more easily fit in minimal RAM.
Losslessly compressing the format strings probably won't reduce
flash image size.
cheers, Joe
On Monday 24 September 2007 3:37:55 pm Vegard Nossum wrote:
> On 9/24/07, Joe Perches <[email protected]> wrote:
> > On Mon, 2007-09-24 at 18:43 +0200, Vegard Nossum wrote:
> > > Storing the format-string separately allows us to hash THAT instead of
> > > the formatted (ie. console output) message. Since this will never
> > > change from message to message, it can be looked up in a table or
> > > whatever and allow user-space to do translations without for example
> > > regular expressions.
> >
> > That hash will change with each linux version given the
> > inevitable spelling fixes, message reformatting and such.
>
> But we can keep the old ones too. That shouldn't be much of a problem.
> I mean, it probably wouldn't rely on a hash alone. The format string
> itself can be compared with the translation database.
I point out that the thread started with a comment about how to _reduce_
bloat.
Just sayin'.
Rob
--
"One of my most productive days was throwing away 1000 lines of code."
- Ken Thompson.
On Monday 24 September 2007 7:10:32 pm Joe Perches wrote:
> On Mon, 2007-09-24 at 18:51 -0500, Rob Landley wrote:
> > > An added pass between gcc preprocessor and compiler could compact
> > > or compress the format string without modifying the conversion
> > > specifications so __attribute__ ((format (printf)) would still work.
> >
> > This does not address my problem. Spitting out a proprietary hash code
> > instead of a human readable message is not a solution for my use case.
>
> What is your problem Rob?
The single largest space savings in the existing -tiny patches comes from
removing printk() calls and strings. I would like to be able to selectively
do this based on severity level, which is information most existing printk()
calls already have. I proposed a minimal change to how printk() works,
allowing the compiler to remove unused code that wouldn't be below the
displayed level of printk() anyway in the deployed product so wouldn't
actually lose any output.
The kernel image is usually already compressed in flash and decompressed to
dram during boot. (Not always, sometimes it's run directly out of flash, but
there's often a speed penalty for doing this, you have to set it up
specially, and dram is cheaper than flash anyway.) This means recompressing
it doesn't help save flash.
If you want to save dram, have printk and associated strings be a function in
a module that's demand loaded and unloaded again after each call. Then you
can foist compression off on userspace, and we're already used to modules
having to match a given kernel version exactly. Why come up with new
infrastructure?
Rob
--
"One of my most productive days was throwing away 1000 lines of code."
- Ken Thompson.
> I don't know. Compare the following two lines:
>
> printk(KERN_INFO "Message.\n");
> kprint_info("Message.");
>
> By dropping the lengthy macro (it's not like it's going to change
> while we're running anyway, so why not make it a part of the function
> name?) and the final newline, we actually end up with a net decrease
> in line length.
Agreed. In fact, you may want to write a header that implements the
kprint_ functions in terms of printk for out-of-core driver writers to
incorporate into their code bases, so they can upgrade their API while
maintaining backward compatibility. (If it were me, I'd also give it
a very permissive license, like outright public domain, to encourage use.)
> I thought it would be nice to have something that looks familiar,
> since that would ease an eventual transition. klog is a valid
> alternative, but isn't kp a bit cryptic?
Well, in context:
kp_info("Message.");
Even the "kp_" prefix is actually pretty unnecessary. It's "info"
and a human-readable string that make it recognizable as a log message.
Another reason to keep it short is just that it's going to get typed a LOT.
Anyway, just MHO.
On 9/23/07, Miguel Ojeda <[email protected]> wrote:
> Nice. I would suggest having some kind of standard way to show the
> information on the screen/dmesg. I mean, instead of having plain lines
> being written to the log, having something very short like:
Thanks for the idea. Is this something you want to (manually) insert
in each printk, or would it be passed like a parameter? For the
subsystem part, it might be possible to #define a subsystem name at
the beginning of each file and have printing functions automatically
use this. But otherwise, I think the usefulness of this is perhaps a
little low compared to the effort needed (ie. for this to be useful,
each and every printk of the kernel would need to be changed). Also
notice, even in your examples, that most subsystems/drivers already
prefix their messages to identify the source. Perhaps a better effort
spent would be to go through the ones that are missing this "source"
prefix and fix them?
Vegard
On Tue, 2007-09-25 at 00:58 -0400, [email protected] wrote:
> Even the "kp_" prefix is actually pretty unnecessary. It's "info"
> and a human-readable string that make it recognizable as a log message.
While I agree a prefix isn't necessary, info, warn, err
are already frequently #define'd and used.
kp_<level> isn't currently in use.
$ egrep -r -l --include=*.[ch] "^[[:space:]]*#[[:space:]]*define[[:space:]]+(info|err|warn)\b" * | wc -l
29
>> Even the "kp_" prefix is actually pretty unnecessary. It's "info"
>> and a human-readable string that make it recognizable as a log message.
> While I agree a prefix isn't necessary, info, warn, err
> are already frequently #define'd and used.
>
> kp_<level> isn't currently in use.
>
> $ egrep -r -l --include=*.[ch] "^[[:space:]]*#[[:space:]]*define[[:space:]]+(info|err|warn)\b" * | wc -l
> 29
Sorry for being unclear. I wasn't seriously recommending no prefix,
due to name collisions (exactly your point), but rather saying that no
prefix is necessary for human understanding.
Something to avoid the ambiguity is still useful. I was just saying
that it can be pretty much anything withouyt confusing the casual
reader.
We're in violent agreement, I just didn't say it very well the first
time.
On 9/25/07, Joe Perches <[email protected]> wrote:
> On Tue, 2007-09-25 at 00:58 -0400, [email protected] wrote:
> > Even the "kp_" prefix is actually pretty unnecessary. It's "info"
> > and a human-readable string that make it recognizable as a log message.
>
> While I agree a prefix isn't necessary, info, warn, err
> are already frequently #define'd and used.
>
> kp_<level> isn't currently in use.
>
> $ egrep -r -l --include=*.[ch] "^[[:space:]]*#[[:space:]]*define[[:space:]]+(info|err|warn)\b" * | wc -l
> 29
Yes, this is a very good point, they're already used. If they hadn't
been, everything would have been perfect. Actually, I'd have preferred
info/warn/err over kprint_<level> if it wasn't for the fact that
they're used (and in slightly different ways too).
As I wrote initially, one of the absolute requirements of a new API is
to retain full backwards compatibility with printk(). Which means that
using simply err()/info()/warn() is out of the question *for now*.
That is not to say we can't change this at a later time.
I think it would be good to have a base layer containing the functions
kprint_<level>(), just to have something that 1) has a meaningful
name, and 2) doesn't disturb anybody else's names. err/info/warn or
kp_err/info/warn() (in order to have shorter names) can then be
implemented in terms of this.
I suppose that another goal of a new API would be to unify the
somewhat-a-mess of API that is now, i.e. many alternatives that do the
same thing is also not good. But this can be changed with patches (to
convert to new API) later.
If you look closer at the current definitions of erro/warn/info, it
turns out that most of them also do this to automatically prefix all
messages with the driver name. This makes me realize that there really
is a need for a way to automatically prefix messages or store a
per-message "subsystem" field. I propose the following solution:
The kprint.h header file looks like this:
/* This is a back-up string to be used if the source file doesn't
define this as a macro. */
const char *SUBSYSTEM = "";
/* Call this macro whatever you want, it's just an example anyway. */
#define info(msg, ...) printf("%s: " msg, SUBSYSTEM, ## __VA_ARGS__)
Then you can have a C file that overrides SUBSYSTEM by defining it as a macro:
#include <linux/kprint.h>
#define SUBSYSTEM "usb"
info("test");
--> results in printf("%s: " "test", "usb");
Or, a C file that doesn't:
#include <linux/kprint.h>
info("test");
--> results in printf("%s: " "test", SYBSYSTEM);
--> output is ": test"
Though, instead of actually incorporating this SUBSYSTEM name into the
string, I suggest passing it off as an argument into the real kprint()
machinery, to be stored along (but seperately) with timestamp, etc.
Hm, that's a digression. But thanks for the idea :)
Vegard
On Mon, 2007-09-24 at 08:19 -0700, Joe Perches wrote:
> On Mon, 2007-09-24 at 11:22 +0200, Michael Holzheu wrote:
> > Together with the idea of not allowing multiple lines in the kprint_xxx
> > functions, that would go with our approach having message numbers to
> > identify a message.
>
> How does this equate/give message numbers?
It does not give us message numbers. It allows us to tag printks, which
have more than one line, with exactly one number.
Michael
On 9/25/07, Vegard Nossum <[email protected]> wrote:
> On 9/23/07, Miguel Ojeda <[email protected]> wrote:
> > Nice. I would suggest having some kind of standard way to show the
> > information on the screen/dmesg. I mean, instead of having plain lines
> > being written to the log, having something very short like:
>
> Thanks for the idea. Is this something you want to (manually) insert
> in each printk, or would it be passed like a parameter?
No parameters if possible: As you said, one or two #define's in the
beggining of the file can tell printk what to print as subsystem and
driver/...; and the reason (printk_xxx) could tell the loglevel. For
example: "detected", "registered" or "copyright" messages will have
always some low level priority; and "errors" or "alerts" high
priorities. This can help whenever selecting what loglevel priorities
to log, as all drivers will log most common messages in the same
loglevel.
So, with your change to printk_xxx, we can add real meaning to the
messages, and forget about selecting a priority for most common
messages. And that does not break your idea of writing printk_yyy,
being yyy all the priorities, as we will be able to use that functions
for uncommon kind of messages.
To clarify what I'm saying, here is an example:
#define PRINTK_SUBSYSTEM "video"
#define PRINTK_DRIVER "some123ag"
...
printk_copyright("Pretty Systems Inc. (C) <[email protected]>");
...
printk_detected("Some Pro 123ag VGA");
...
printk_registered("Framebuffer /dev/fb1");
...
if (really_weird_stuff_happened)
printk_emerg("Video card appears to be disconnected!");
As you see, each message will write to the correct priority. Even,
things like "printk_copyright" could have a macro like:
#define PRINTK_COPYRIGHT(who, year, email) printk_copyright(who " (C)
" year " <" email ">");
But I think that its going too far.
> For the subsystem part, it might be possible to #define a subsystem name at
> the beginning of each file and have printing functions automatically
> use this. But otherwise, I think the usefulness of this is perhaps a
> little low compared to the effort needed (ie. for this to be useful,
> each and every printk of the kernel would need to be changed). Also
Right... But in the long time, it can help a lot. For now, every
maintainer should add the #define line in each file and write the
reason while changing the function name (that is something we will
have to do anyway if you change printk).
> notice, even in your examples, that most subsystems/drivers already
> prefix their messages to identify the source. Perhaps a better effort
> spent would be to go through the ones that are missing this "source"
> prefix and fix them?
I would say 70% do it. Also, they just write the name of the
subsystem/driver, a ':' and then a non-standard string. I mean, most
of the messages are about a new detected device, an error encountered,
a registered/mounted device/link, the copyright, the authors, ... Some
of them say "registered XXX", others "XXX registered", other just say
"new XXX" or split the message in two lines, etc.
My point is: Most of the messages are inside five or six categories,
so if people is ready to change/improve printk() API, let's finish all
the duplication, as we usually do with code.
Also, I think it could help a lot in the future if we categorize the
messages by subsystem/driver and level/reason: then we could write
more useful tools for debugging, logging... We could "grep" it, or
select what we want to log, etc.
I'm sure this idea is maybe overwhelming at first, but I think that in
the future will help a lot. Linux is growing everyday more and more,
and if printk() is about to change, as soon as we introduce new
features, easier will be.
>
> Vegard
>
Thanks for your interest in the idea.
--
Miguel Ojeda
http://maxextreme.googlepages.com/index.htm