2007-09-27 21:18:18

by Vegard Nossum

[permalink] [raw]
Subject: [RFC] New kernel-message logging API (take 2)

Hello,

A big thanks to everybody who read and replied to my first e-mail; I
have tried my best to incorporate your feedback and suggestions. I
also added some CCs who recently participated in logging-related
discussions.


Changes (since Sept. 22):

* Extensibility -> Allowing the compiler to eliminate messages below
a certain threshold requires changing the API.
* Add some special-purpose logging functions
(printk_detected(), _registered(), _settings(), and _copyright())
* Fine-grained log-level control. "Everything above" or "everything
below" can be emulated by turning the specific log-levels on or off.
* Define an extra header containing the (optional) secondary
interface (err()/warn()/info())
* Remove kprint_*() aliases.
* kprint_<level>() is better than kprint(<level), ) because in the
case of the default log-level, the argument can be omitted.
* Memory allocated for entries and arguments is done in a ring-buffer
with variable-sized chunks. Arguments are chained with a singly-
linked list.
* Use SUBSYSTEM and KBUILD_MODNAME
* Rename kprint buffers to kprint blocks


Vegard



1. 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 eliminating messages below a given
threshold or recording the location (i.e. source file/line) of a
message [1] should be both selectable at compile-time and (if compiled
in) at run-time.

2. API

2.1. linux/kprint.h

This header defines the primary (i.e. lowest-level) interface to
kprint that is made available to the rest of the kernel.

2.1.1. kprint()

#define kprint(fmt, ...)

This function is the equivalent of the old printk(), except that it
does not take a log-level parameter, but emits messages using the
default log-level. The string must be a single line of information
(i.e. it must not contain any newlines). kprint() is implemented as a
macro, and must not be called from assembly.

2.1.2. Log-levels

To support the different log-levels, there exists one kprint_*()
function for each log-level, for example kprint_info(). This contrasts
with the printk() interface, but allows the log-level argument to be
passed as an argument (instead of prepending it to the message string)
and omitted if the default log-level should be used.

The string must be a single line of information. Calling
kprint_emerg("Oops.") is equivalent to calling printk(KERN_EMERG
"Oops.\n"). These functions are implemented as macros, and must 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_err() */
KPRINT_WARNING, /* kprint_warn() */
KPRINT_NOTICE, /* kprint_notice() */
KPRINT_INFO, /* kprint_info() */
KPRINT_DEBUG, /* kprint_debug() */
};

The individual log-levels can be enabled/disabled in the kernel
configuration and subsequently removed from the kernel (by the
compiler) entirely. It is not an option to filter out messages that
are simply above a certain log-level, although it could be more
convenient; controlling each log-level is the more general approach
and can be used to emulate the threshold filter. [8]

2.1.3. Classification tags

It turns out that many messages share a similar purpose. It would be
useful to classify these by a different scheme than severity [6].
Therefore, an additional four special-purpose macros are defined:

* printk_detected() A "hardware detected" message
* printk_registered() A "device driver (un-)registered" message
* printk_setting() A "hardware setting change" message
* printk_copyright() A copyright message, such as module authorship
information

Each message will be assigned the appropriate log-level for the
message class in question.

2.1.4. Blocks

In order to print several related lines as one chunk, the emitter
should first allocate an object of the type struct kprint_block. This
struct is initialized with the function kprint_block_init() which
takes as arguments a pointer to an object of the type struct
kprint_block followed by the log-level number. The emitter may then
make as many or as few calls to kprint_block() that is desired. A
final call to kprint_block_flush() appends the messages to the kernel
message log in a single, atomic operation. After it has been flushed,
the struct is not usable again (unless it is re-initialized). If for
any reason the struct should be de-initialized without writing it to
the log, a call to kprint_block_abort() must be made.

Example: {
struct kprint_block out;
kprint_block_init(&out, KPRINT_DEBUG);
kprint_block(&out, "Stack trace:");

while(unwind_stack()) {
kprint_block(&out, "%p %s", address, symbol);
}
kprint_block_flush(&out);
}

2.1.5. Subsystem/driver tags

Many parts of the kernel already prefix their log messages with a
subsystem and/or driver tag to identify the source of a particular
message. With the kprint interface, these tags are redundant. Instead,
the macros SUBSYSTEM and KBUILD_MODNAME are used and recorded along
with each log message. Therefore, each source file should define the
macro SUBSYSTEM before any of the kprint functions are used. If this
macro is not defined, the recorded subsystem will be an empty string.
[6][7]

2.1.6. Early- and assembly functions

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 and a variable number of arguments.

2.1.7 Backwards compatibility

The legacy printk() function is replaced by a backwards-compatible
interface but different implementation. 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
appropriate kprint()-system functions.

Because printk() itself remains a function with the same
specification, all printing/logging mechanisms based on it, such as
dev_printk(), sdev_printk(), and ata_dev_printk() will remain
functional. Alternatively, the macros could be changed to use the new
kprint API.


2.2. linux/kprint-light.h

The kprint "light" interface provides a simpler interface that
covers the most frequent usage patterns. It should be noted that this
interface is preferred over the primary interface as it encourages the
use of a smaller set of log-levels [5].

The interface consists primarily of the three macros, err(), warn(),
and info() that emit messages at the corresponding log-level.

This header is optional and must not be included by other header
files. The names defined by this header are already used by different
parts of the kernel, but in different ways. With an optional header,
new code (or code converted to the kprint API) can explicitly request
these definitions while leaving old locations unchanged.


3. Internals

Most of the kprint entry-points (especially kprint() and its
log-level variations) are implemented as macros in order to be able to
transparently pass extra information into the main kprint machinery.
This makes the interface abstract, because we can change the behaviour
(through the configuration and by adding extensions) without changing
the calling code. As an example, prepending the current file and line
(the __FILE__ and __LINE__ macros) to the message can be done in such
a way that it can be discarded at run-time, or recorded along with
(but separate from) the messages. This allows 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 also be recorded separately from the
actual format string, instead of written already formatted to a ring
buffer of characters. Parameters would be formatted to their own
strings (regardless of the original type) and saved in a list.

Example: {
struct kprint_message {
const char *format;
struct kprint_arg *args;

#ifdef KPRINT_TIMESTAMP
unsigned long long timestamp;
#endif

#ifndef KPRINT_LOCATION
const char *file;
unsigned int line;

const char *function;
#endif
};

struct kprint_arg {
struct kprint_arg *next;

/* The string formatted argument, regardless
* of original type. */
char *arg;
};
}

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 separately 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,
though the syslog() and /proc/kmsg interfaces will retain backwards
compatibility by formatting messages as they are requested from
user-space.

This does not mean that user-space is obliged provide any form of
lookup-/translation tables in order to read the kernel log; the kernel
simply hands over the format string used to format a particular
message in addition to the full log message, and this format string
may be used to easily look up translations (if they exist).

4. 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.

The kernel can still used a fixed-size ring-buffer for storing the
kernel log. The ring-buffer would need to allow variable-sized
elements, instead of simply fixed-size chars. 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.

It should be possible to optimize out multi-line (block) entries
based on log-level filtering even though the log-level is only given
in the first call (the initializer). It may take the shape of an
if-block that spans several macros. This is not very elegant or robust
if the macros are used incorrectly, however. Aborting a message can
also be hard this way (since the abort would usually appear inside an
if-statement that tests for some abnormal condition, thus appear in a
different block, and thoroughly mess up the bracket order).

Example: {
#define kprint_block_init(block, loglevel) \
if(loglevel > CONFIG_KPRINT_LOGLEVEL_MAX) { \
kprint_real_block_init(block, loglevel);

#define kprint_block(block, fmt, ...) \
kprint_real_block(block, fmt, ## __VA_ARGS__);

#define kprint_block_flush(block) \
kprint_real_block_flush(block); \
}

/* Thus, this C code: */
kprint_block_init(&block, KPRINT_INFO);
kprint_block(&block, "Hello world");
kprint_block_flush(&block);

/* Would pre-process into this: */
if(6 < 4) {
kprint_real_block_init(&block, 6);
kprint_real_block(&block, "Hello world");
kprint_block_flush(&block);
}
}


References

[1] http://lkml.org/lkml/2007/9/21/267 (Joe Perches)
[2] http://lkml.org/lkml/2007/9/20/352 (Rob Landley)
[3] http://lkml.org/lkml/2007/9/21/151 (Dick Streefland)
[4] http://lkml.org/lkml/2007/6/13/146 (Michael Holzheu)
[5] http://lkml.org/lkml/2007/9/24/320 (Jesse Barnes)
[6] http://lkml.org/lkml/2007/9/22/162 (Miguel Ojeda)
[7] http://lkml.org/lkml/2007/9/25/62 (Vegard Nossum)
[8] http://lkml.org/lkml/2007/9/22/157 (Joe Perches)


2007-09-28 01:38:35

by George Spelvin

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

> Example: {
> struct kprint_block out;
> kprint_block_init(&out, KPRINT_DEBUG);
> kprint_block(&out, "Stack trace:");
>
> while(unwind_stack()) {
> kprint_block(&out, "%p %s", address, symbol);
> }
> kprint_block_flush(&out);
> }

Assuming that kprint_block_flush() is a combination of
kprint_block_printit() and kprint_block_abort(), you
coulld make a macro wrapper for this to preclude leaks:

#define KPRINT_BLOCK(block, level, code) \
do { \
struct kprint_block block; \
kprint_block_init(&block, KPRINT_##level); \
do { \
code ; \
kprint_block_printit(&block); \
while (0); \
kprint_block_abort(&block); \
} while(0)

The inner do { } while(0) region is so you can abort with "break".

(Or you can split it into KPRINT_BEGIN() and KPRINT_END() macros,
if that works out to be cleaner.)

2007-09-28 07:31:33

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

On Thu, 27 Sep 2007, Vegard Nossum wrote:
> It should be possible to optimize out multi-line (block) entries
> based on log-level filtering even though the log-level is only given
> in the first call (the initializer). It may take the shape of an
> if-block that spans several macros. This is not very elegant or robust
> if the macros are used incorrectly, however. Aborting a message can
> also be hard this way (since the abort would usually appear inside an
> if-statement that tests for some abnormal condition, thus appear in a
> different block, and thoroughly mess up the bracket order).
>
> Example: {
> #define kprint_block_init(block, loglevel) \
> if(loglevel > CONFIG_KPRINT_LOGLEVEL_MAX) { \
> kprint_real_block_init(block, loglevel);
>
> #define kprint_block(block, fmt, ...) \
> kprint_real_block(block, fmt, ## __VA_ARGS__);
>
> #define kprint_block_flush(block) \
> kprint_real_block_flush(block); \
> }
>
> /* Thus, this C code: */
> kprint_block_init(&block, KPRINT_INFO);
> kprint_block(&block, "Hello world");
> kprint_block_flush(&block);
>
> /* Would pre-process into this: */
> if(6 < 4) {
> kprint_real_block_init(&block, 6);
> kprint_real_block(&block, "Hello world");
> kprint_block_flush(&block);
> }
> }

If-blocks spanning macros are really dangerous!

E.g. an Ethernet driver may want to do:

kprint_block(&block, "MAC ");
for (i = 0; i < 6; i++) {
card->mac[i] = obtain_mac_byte_from_hw(i);
kprint_block(&block, "%02x", card->mac[i]);
}

This looks (and should be) innocent, but the actual MAC addres retrieval
would never be executed if loglevel <= CONFIG_KPRINT_LOGLEVEL_MAX.

Can't you store the loglevel in the kprint_block and check it in all
successive kprint_*() macros? If gcc knows it's constant, it can optimize
the non-wanted code away. As other fields in struct kprint_block cannot be
constant (they store internal state), you have to split it like:

struct kprint_block {
int loglevel;
struct real_kprint_block real; /* internal state */
}

and pass &block.real() instead of &block to all successive internal functions.
I haven't tried this, so let's hope gcc is actually smart enough...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2007-09-28 07:44:39

by Kyle Moffett

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

On Sep 28, 2007, at 03:31:11, Geert Uytterhoeven wrote:
> Can't you store the loglevel in the kprint_block and check it in
> all successive kprint_*() macros? If gcc knows it's constant, it
> can optimize the non-wanted code away. As other fields in struct
> kprint_block cannot be constant (they store internal state), you
> have to split it like:
>
> struct kprint_block {
> int loglevel;
> struct real_kprint_block real; /* internal state */
> }
>
> and pass &block.real() instead of &block to all successive internal
> functions. I haven't tried this, so let's hope gcc is actually
> smart enough...

Well actually, I believe you could just do:

struct kprint_block {
const int loglevel;
[...];
};

Then cast away the constness to actually set it initially:
*((int *)&block.loglevel) = LOGLEVEL;

Cheers,
Kyle Moffett

2007-09-28 08:23:07

by Dick Streefland

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

"Vegard Nossum" <[email protected]> wrote:
| It should be possible to optimize out multi-line (block) entries
| based on log-level filtering even though the log-level is only given
| in the first call (the initializer). It may take the shape of an
| if-block that spans several macros. This is not very elegant or robust
| if the macros are used incorrectly, however. Aborting a message can
| also be hard this way (since the abort would usually appear inside an
| if-statement that tests for some abnormal condition, thus appear in a
| different block, and thoroughly mess up the bracket order).
|
| Example: {
| #define kprint_block_init(block, loglevel) \
| if(loglevel > CONFIG_KPRINT_LOGLEVEL_MAX) { \
| kprint_real_block_init(block, loglevel);
|
| #define kprint_block(block, fmt, ...) \
| kprint_real_block(block, fmt, ## __VA_ARGS__);
|
| #define kprint_block_flush(block) \
| kprint_real_block_flush(block); \
| }

As you point out yourself, this is not very elegant or robust. In fact,
it is very dangerous. Why not simply pass the loglevel to each macro?

--
Dick Streefland //// Altium BV
[email protected] (@ @) http://www.altium.com
--------------------------------oOO--(_)--OOo---------------------------

2007-09-28 09:45:48

by Jan Engelhardt

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)


On Sep 27 2007 23:18, Vegard Nossum wrote:
> * kprint_<level>() is better than kprint(<level), ) because in the
> case of the default log-level, the argument can be omitted.
> * Memory allocated for entries and arguments is done in a ring-buffer
> with variable-sized chunks. Arguments are chained with a singly-
> linked list.
> * Use SUBSYSTEM and KBUILD_MODNAME
> * Rename kprint buffers to kprint blocks

This is overall, quite a lot. I suggest one-thing-at-a-time,
starting with kprint_<level>() that is compiled out if desired
and no fancy block or newline stuff.

_Then_, will see how it flies. All of this smells like a bit of
overdesigning, aka http://en.wikipedia.org/wiki/YAGNI

2007-09-28 11:46:17

by Vegard Nossum

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

> If-blocks spanning macros are really dangerous!
>
> E.g. an Ethernet driver may want to do:
>
> kprint_block(&block, "MAC ");
> for (i = 0; i < 6; i++) {
> card->mac[i] = obtain_mac_byte_from_hw(i);
> kprint_block(&block, "%02x", card->mac[i]);
> }
>
> This looks (and should be) innocent, but the actual MAC addres retrieval
> would never be executed if loglevel <= CONFIG_KPRINT_LOGLEVEL_MAX.

Yup. Okay, so it's definitely NOT an option.

> Can't you store the loglevel in the kprint_block and check it in all
> successive kprint_*() macros? If gcc knows it's constant, it can optimize
> the non-wanted code away. As other fields in struct kprint_block cannot be
> constant (they store internal state), you have to split it like:
>
> struct kprint_block {
> int loglevel;
> struct real_kprint_block real; /* internal state */
> }
>
> and pass &block.real() instead of &block to all successive internal functions.
> I haven't tried this, so let's hope gcc is actually smart enough...

It isn't, apparently. Or not with my test, anyway. Either way, it's
probably better not to make those assumptions about or rely too much
on the smartness of the compiler (we don't have *any* guarantees).

The best solution for now is probably to pass the log-level into each
line, as Dick Streefland suggested, though it would lead to a hairier
syntax, or just skip the whole interface for now, as Jan Engelhardt
suggested. Thanks.

Vegard

2007-09-28 11:49:53

by Vegard Nossum

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

On 9/28/07, Kyle Moffett <[email protected]> wrote:
> On Sep 28, 2007, at 03:31:11, Geert Uytterhoeven wrote:
> > Can't you store the loglevel in the kprint_block and check it in
> > all successive kprint_*() macros? If gcc knows it's constant, it
> > can optimize the non-wanted code away. As other fields in struct
> > kprint_block cannot be constant (they store internal state), you
> > have to split it like:
> >
> > struct kprint_block {
> > int loglevel;
> > struct real_kprint_block real; /* internal state */
> > }
> >
> > and pass &block.real() instead of &block to all successive internal
> > functions. I haven't tried this, so let's hope gcc is actually
> > smart enough...
>
> Well actually, I believe you could just do:
>
> struct kprint_block {
> const int loglevel;
> [...];
> };
>
> Then cast away the constness to actually set it initially:
> *((int *)&block.loglevel) = LOGLEVEL;

This doesn't seem to work either (i.e. it is not optimized out). I
tried initializing the struct statically too, to no avail. But thanks
for the tip.

Vegard

2007-09-28 11:59:28

by Vegard Nossum

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

On 9/28/07, Jan Engelhardt <[email protected]> wrote:
>
> On Sep 27 2007 23:18, Vegard Nossum wrote:
> > * kprint_<level>() is better than kprint(<level), ) because in the
> > case of the default log-level, the argument can be omitted.
> > * Memory allocated for entries and arguments is done in a ring-buffer
> > with variable-sized chunks. Arguments are chained with a singly-
> > linked list.
> > * Use SUBSYSTEM and KBUILD_MODNAME
> > * Rename kprint buffers to kprint blocks
>
> This is overall, quite a lot. I suggest one-thing-at-a-time,
> starting with kprint_<level>() that is compiled out if desired
> and no fancy block or newline stuff.
>
> _Then_, will see how it flies. All of this smells like a bit of
> overdesigning, aka http://en.wikipedia.org/wiki/YAGNI

Well, that's why I'm writing it down before actually coding it.
Designing is also trying to make things fit together without actually
having the physical parts at hand. So I'm trying to make an interface
that CAN support multi-line blocks in the future, since it's obviously
a desired (and currently missing) feature.

But I agree; It *is* hard to see how multi-line blocks can be
implemented without actually spelling it out in code. I've tried to do
it, and failed. Until a brilliant solution comes up, I'll skip it.

Vegard

2007-09-28 12:11:19

by Vegard Nossum

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

On 9/27/07, Vegard Nossum <[email protected]> wrote:
> * Use SUBSYSTEM and KBUILD_MODNAME

snip.

> 2.1.5. Subsystem/driver tags
>
> Many parts of the kernel already prefix their log messages with a
> subsystem and/or driver tag to identify the source of a particular
> message. With the kprint interface, these tags are redundant. Instead,
> the macros SUBSYSTEM and KBUILD_MODNAME are used and recorded along
> with each log message. Therefore, each source file should define the
> macro SUBSYSTEM before any of the kprint functions are used. If this
> macro is not defined, the recorded subsystem will be an empty string.
> [6][7]

This changes to KPRINT_SUBSYSTEM and KPRINT_DRIVER. The KPRINT_ prefix
is to clearly say that this is something related to logging. The
reason we can't use KBUILD_MODNAME is that this is defined on the
command line. The declaration inside the header would thus be horribly
wrong. We can, however, use KBUILD_MODNAME as a default value for
KPRINT_DRIVER, like:
static const char *KPRINT_DRIVER = KBUILD_MODNAME;
which would pre-process to something like:
static const char *KPRINT_DRIVER = "bcm43xx";

This value can still be overridden using #define KPRINT_DRIVER "new
name". In this case, it is possible that the original KPRINT_DRIVER
symbol can cause an "unused variable"-warning. I guess this is fixable
with the gcc "unused" variable attribute.

Vegard

2007-09-28 13:30:53

by Miguel Ojeda

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

On 9/28/07, Vegard Nossum <[email protected]> wrote:
> On 9/27/07, Vegard Nossum <[email protected]> wrote:
> > * Use SUBSYSTEM and KBUILD_MODNAME
>
> snip.
>
> > 2.1.5. Subsystem/driver tags
> >
> > Many parts of the kernel already prefix their log messages with a
> > subsystem and/or driver tag to identify the source of a particular
> > message. With the kprint interface, these tags are redundant. Instead,
> > the macros SUBSYSTEM and KBUILD_MODNAME are used and recorded along
> > with each log message. Therefore, each source file should define the
> > macro SUBSYSTEM before any of the kprint functions are used. If this
> > macro is not defined, the recorded subsystem will be an empty string.
> > [6][7]
>
> This changes to KPRINT_SUBSYSTEM and KPRINT_DRIVER. The KPRINT_ prefix
> is to clearly say that this is something related to logging. The

Nice. Although the word "DRIVER" may not represent every module, I
think it is the correct option as I suggested, as the word is
meaningful (speaks by itself) and almost every message in the kernel
is printed out by drivers (whatever the big subsystem they belong to:
drivers/, fs/, net/ ...).

> reason we can't use KBUILD_MODNAME is that this is defined on the
> command line. The declaration inside the header would thus be horribly
> wrong. We can, however, use KBUILD_MODNAME as a default value for
> KPRINT_DRIVER, like:
> static const char *KPRINT_DRIVER = KBUILD_MODNAME;
> which would pre-process to something like:
> static const char *KPRINT_DRIVER = "bcm43xx";
>
> This value can still be overridden using #define KPRINT_DRIVER "new
> name". In this case, it is possible that the original KPRINT_DRIVER
> symbol can cause an "unused variable"-warning. I guess this is fixable
> with the gcc "unused" variable attribute.

Yep, then, in a year or two, we will be able to delete such attribute.

Will there be a team to change main subsystems/drivers to the new API?

--
Miguel Ojeda
http://maxextreme.googlepages.com/index.htm

2007-09-28 13:55:36

by Vegard Nossum

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

On 9/28/07, Miguel Ojeda <[email protected]> wrote:
> On 9/28/07, Vegard Nossum <[email protected]> wrote:
> > reason we can't use KBUILD_MODNAME is that this is defined on the
> > command line. The declaration inside the header would thus be horribly
> > wrong. We can, however, use KBUILD_MODNAME as a default value for
> > KPRINT_DRIVER, like:
> > static const char *KPRINT_DRIVER = KBUILD_MODNAME;
> > which would pre-process to something like:
> > static const char *KPRINT_DRIVER = "bcm43xx";
> >
> > This value can still be overridden using #define KPRINT_DRIVER "new
> > name". In this case, it is possible that the original KPRINT_DRIVER
> > symbol can cause an "unused variable"-warning. I guess this is fixable
> > with the gcc "unused" variable attribute.
>
> Yep, then, in a year or two, we will be able to delete such attribute.

Actually, no, since it will throw a warning only if a source file
#defines KPRINT_SUBSYSTEM (i.e. overrides the constant variable
(oxymoron!) with the same name). What you're hoping is that some time
in the future, EVERY source file will come equipped with these
definitions, and yes, at that point, the entire declaration can be
removed, BUT I think that's... well. Yes.

> Will there be a team to change main subsystems/drivers to the new API?

No. First of all, this is a specification draft; there is no code yet.
Also, very possibly, this is such a violent change that nobody really
wants to use it anyway. But we can hope. ;-)

Vegard

2007-09-28 14:00:28

by Miguel Ojeda

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

On 9/28/07, Vegard Nossum <[email protected]> wrote:
> On 9/28/07, Miguel Ojeda <[email protected]> wrote:
> > On 9/28/07, Vegard Nossum <[email protected]> wrote:
> > > reason we can't use KBUILD_MODNAME is that this is defined on the
> > > command line. The declaration inside the header would thus be horribly
> > > wrong. We can, however, use KBUILD_MODNAME as a default value for
> > > KPRINT_DRIVER, like:
> > > static const char *KPRINT_DRIVER = KBUILD_MODNAME;
> > > which would pre-process to something like:
> > > static const char *KPRINT_DRIVER = "bcm43xx";
> > >
> > > This value can still be overridden using #define KPRINT_DRIVER "new
> > > name". In this case, it is possible that the original KPRINT_DRIVER
> > > symbol can cause an "unused variable"-warning. I guess this is fixable
> > > with the gcc "unused" variable attribute.
> >
> > Yep, then, in a year or two, we will be able to delete such attribute.
>
> Actually, no, since it will throw a warning only if a source file
> #defines KPRINT_SUBSYSTEM (i.e. overrides the constant variable
> (oxymoron!) with the same name). What you're hoping is that some time
> in the future, EVERY source file will come equipped with these
> definitions, and yes, at that point, the entire declaration can be
> removed, BUT I think that's... well. Yes.

Yes, that was my point. Far far far away, but possible, and if this
RFC ever meets the real kernel, then bringing every source file to the
API should be a objective. A good project for kernel janitors, for
example.

>
> > Will there be a team to change main subsystems/drivers to the new API?
>
> No. First of all, this is a specification draft; there is no code yet.
> Also, very possibly, this is such a violent change that nobody really
> wants to use it anyway. But we can hope. ;-)

Sure, this is speculation. :)

>
> Vegard
>


--
Miguel Ojeda
http://maxextreme.googlepages.com/index.htm

2007-09-28 16:30:44

by Rob Landley

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

On Friday 28 September 2007 7:11:03 am Vegard Nossum wrote:
> wrong. We can, however, use KBUILD_MODNAME as a default value for
> KPRINT_DRIVER, like:
> static const char *KPRINT_DRIVER = KBUILD_MODNAME;
> which would pre-process to something like:
> static const char *KPRINT_DRIVER = "bcm43xx";

Which has been known to result in the string getting written out to the .o
file even if it's never used, just in case something tries to take its
address. This is not the same as a #define.

> This value can still be overridden using #define KPRINT_DRIVER "new
> name".

Not with -D on the command line though. Your #define would have to come after
the declaration or else the declaration turns into 'char *"fred" = "george";'
and you have a syntax error. Again, not synonymous with a #define...

Rob
--
"One of my most productive days was throwing away 1000 lines of code."
- Ken Thompson.

2007-09-28 16:41:36

by Vegard Nossum

[permalink] [raw]
Subject: Re: [RFC] New kernel-message logging API (take 2)

On 9/28/07, Rob Landley <[email protected]> wrote:
> On Friday 28 September 2007 7:11:03 am Vegard Nossum wrote:
> > wrong. We can, however, use KBUILD_MODNAME as a default value for
> > KPRINT_DRIVER, like:
> > static const char *KPRINT_DRIVER = KBUILD_MODNAME;
> > which would pre-process to something like:
> > static const char *KPRINT_DRIVER = "bcm43xx";
>
> Which has been known to result in the string getting written out to the .o
> file even if it's never used, just in case something tries to take its
> address. This is not the same as a #define.

Logic tells me that an unused static variable should never go into the
.o. If something tries to take its address, it's no longer unused.

> > This value can still be overridden using #define KPRINT_DRIVER "new
> > name".
>
> Not with -D on the command line though. Your #define would have to come after
> the declaration or else the declaration turns into 'char *"fred" = "george";'
> and you have a syntax error. Again, not synonymous with a #define...

Yeah, that's exactly what my e-mail was about. The macros
KPRINT_SUBSYSTEM and KPRINT_DRIVER are not defined on the command
line, but in each source file that wants this prefix, after the
variables with the same names have been declared in kprint.h. This is
intentional; they can be overridden with a define, otherwise, they'll
default to static-const string variables.

Vegard