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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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_() is better than kprint( >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_() 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 () instead of 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 *)) = 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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(, "MAC ");
> for (i = 0; i < 6; i++) {
> card->mac[i] = obtain_mac_byte_from_hw(i);
> kprint_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 () instead of 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] New kernel-message logging API (take 2)
On Sep 27 2007 23:18, Vegard Nossum wrote: > * kprint_() is better than kprint(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_() 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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---
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 () instead of 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 *)) = LOGLEVEL;
Cheers,
Kyle Moffett
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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(, KPRINT_INFO);
> kprint_block(, "Hello world");
> kprint_block_flush();
>
> /* Would pre-process into this: */
> if(6 < 4) {
> kprint_real_block_init(, 6);
> kprint_real_block(, "Hello world");
> kprint_block_flush();
> }
> }
If-blocks spanning macros are really dangerous!
E.g. an Ethernet driver may want to do:
kprint_block(, "MAC ");
for (i = 0; i < 6; i++) {
card->mac[i] = obtain_mac_byte_from_hw(i);
kprint_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 () instead of 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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---
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] New kernel-message logging API (take 2)
> Example: {
> struct kprint_block out;
> kprint_block_init(, KPRINT_DEBUG);
> kprint_block(, "Stack trace:");
>
> while(unwind_stack()) {
> kprint_block(, "%p %s", address, symbol);
> }
> kprint_block_flush();
> }
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(, KPRINT_##level); \
do { \
code ; \
kprint_block_printit(); \
while (0); \
kprint_block_abort(); \
} 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.)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[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_() is better than kprint( 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(, KPRINT_INFO);
kprint_block(, "Hello world");
kprint_block_flush();
/* Would pre-process into this: */
if(6 < 4) {
kprint_real_block_init(, 6);
kprint_real_block(, "Hello world");
kprint_block_flush();
}
}
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)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[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()
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.)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
