| Message ID | 1430824337-15339-1-git-send-email-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 5 May 2015 21:12:12 +1000 Michael Ellerman <mpe@ellerman.id.au> wrote: > Arch code can set a "dump stack arch description string" which is > displayed with oops output to describe the hardware platform. > > It is useful to initialise this as early as possible, so that an early > oops will have the hardware description. > > However in practice we discover the hardware platform in stages, so it > would be useful to be able to incrementally fill in the hardware > description as we discover it. > > This patch adds that ability, by creating dump_stack_add_arch_desc(). > > If there is no existing string it behaves exactly like > dump_stack_set_arch_desc(). However if there is an existing string it > appends to it, with a leading space. > > This makes it easy to call it multiple times from different parts of the > code and get a reasonable looking result. Some example output in the changelog would be useful, to help people understand the value. In particular, is there any convention for how these fields should be presented? "name:value name:value", etc? Or it is just put random stuff in there, hopefully with self-evident meanings. We're going to blow out the 128 byte dump_stack_arch_desc_str[] pretty quickly. Is dynamic allocation a possibility? > /** > + * dump_stack_add_arch_desc - add arch-specific info to show with task dumps > + * @fmt: printf-style format string > + * @...: arguments for the format string > + * > + * See dump_stack_set_arch_desc() for why you'd want to use this. > + * > + * This version adds to any existing string already created with either > + * dump_stack_set_arch_desc() or dump_stack_add_arch_desc(). If there is an > + * existing string a space will be prepended to the passed string. > + */ > +void __init dump_stack_add_arch_desc(const char *fmt, ...) > +{ > + va_list args; > + int pos, len; > + char *p; > + > + /* > + * If there's an existing string we snprintf() past the end of it, and > + * then turn the terminating NULL of the existing string into a space > + * to create one string separated by a space. > + * > + * If there's no existing string we just snprintf() to the buffer, like > + * dump_stack_set_arch_desc(), but without calling it because we'd need > + * a varargs version. > + */ > + > + len = strnlen(dump_stack_arch_desc_str, sizeof(dump_stack_arch_desc_str)); > + pos = len; > + > + if (len) > + pos++; > + > + if (pos >= sizeof(dump_stack_arch_desc_str)) > + return; /* Ran out of space */ > + > + p = &dump_stack_arch_desc_str[pos]; > + > + va_start(args, fmt); > + vsnprintf(p, sizeof(dump_stack_arch_desc_str) - pos, fmt, args); > + va_end(args); This code is almost race-free. A (documented) smp_wmb() in here would make that 100%? > + if (len) > + dump_stack_arch_desc_str[len] = ' '; > +} > +
On Tue, 2015-05-05 at 14:16 -0700, Andrew Morton wrote: > On Tue, 5 May 2015 21:12:12 +1000 Michael Ellerman <mpe@ellerman.id.au> wrote: > > > Arch code can set a "dump stack arch description string" which is > > displayed with oops output to describe the hardware platform. > > > > It is useful to initialise this as early as possible, so that an early > > oops will have the hardware description. > > > > However in practice we discover the hardware platform in stages, so it > > would be useful to be able to incrementally fill in the hardware > > description as we discover it. > > > > This patch adds that ability, by creating dump_stack_add_arch_desc(). > > > > If there is no existing string it behaves exactly like > > dump_stack_set_arch_desc(). However if there is an existing string it > > appends to it, with a leading space. > > > > This makes it easy to call it multiple times from different parts of the > > code and get a reasonable looking result. > > Some example output in the changelog would be useful, to help people > understand the value. OK, an example from a power system is: Hardware name: POWER8E (raw) 8286-42A PowerNV skiboot-5.0 MI SV830_027 SV810_085 SV830_027 ^ ^ ^ ^ | | | firmware version cpu name | platform name device tree model Those are all added in separate parts of the code. > In particular, is there any convention for how these fields should be > presented? "name:value name:value", etc? Or it is just put random stuff in > there, hopefully with self-evident meanings. I was thinking there was no convention and it's basically up to arch/platform authors to put what they want in there. I see it as a human readable bag of stuff that might be helpful in diagnosing an oops, with the emphasis being on having more information rather than precisely specified fields & values. Having said that I did use name:value in one of my patches, just to differentiate the various version numbers. So maybe I should do that for all the fields. Though then we have the problem that the values contain spaces ... > We're going to blow out the 128 byte dump_stack_arch_desc_str[] pretty > quickly. Is dynamic allocation a possibility? Are we? I think that's enough for us on most powerpc systems, and ideally it's not too long or it will wrap when printed to the console. I'd be inclined to avoid dynamic allocation until we absolutely need it, if anything we could just bump it to 256. > > /** > > + * dump_stack_add_arch_desc - add arch-specific info to show with task dumps > > + * @fmt: printf-style format string > > + * @...: arguments for the format string > > + * > > + * See dump_stack_set_arch_desc() for why you'd want to use this. > > + * > > + * This version adds to any existing string already created with either > > + * dump_stack_set_arch_desc() or dump_stack_add_arch_desc(). If there is an > > + * existing string a space will be prepended to the passed string. > > + */ > > +void __init dump_stack_add_arch_desc(const char *fmt, ...) > > +{ > > + va_list args; > > + int pos, len; > > + char *p; > > + > > + /* > > + * If there's an existing string we snprintf() past the end of it, and > > + * then turn the terminating NULL of the existing string into a space > > + * to create one string separated by a space. > > + * > > + * If there's no existing string we just snprintf() to the buffer, like > > + * dump_stack_set_arch_desc(), but without calling it because we'd need > > + * a varargs version. > > + */ > > + > > + len = strnlen(dump_stack_arch_desc_str, sizeof(dump_stack_arch_desc_str)); > > + pos = len; > > + > > + if (len) > > + pos++; > > + > > + if (pos >= sizeof(dump_stack_arch_desc_str)) > > + return; /* Ran out of space */ > > + > > + p = &dump_stack_arch_desc_str[pos]; > > + > > + va_start(args, fmt); > > + vsnprintf(p, sizeof(dump_stack_arch_desc_str) - pos, fmt, args); > > + va_end(args); > > This code is almost race-free. A (documented) smp_wmb() in here would > make that 100%? Yeah good idea. I'll send a v2 with that change and an updated changelog. cheers
On Tue, 2015-05-05 at 14:16 -0700, Andrew Morton wrote: > On Tue, 5 May 2015 21:12:12 +1000 Michael Ellerman <mpe@ellerman.id.au> wrote: > > > Arch code can set a "dump stack arch description string" which is > > displayed with oops output to describe the hardware platform. > > + > > + len = strnlen(dump_stack_arch_desc_str, sizeof(dump_stack_arch_desc_str)); > > + pos = len; > > + > > + if (len) > > + pos++; > > + > > + if (pos >= sizeof(dump_stack_arch_desc_str)) > > + return; /* Ran out of space */ > > + > > + p = &dump_stack_arch_desc_str[pos]; > > + > > + va_start(args, fmt); > > + vsnprintf(p, sizeof(dump_stack_arch_desc_str) - pos, fmt, args); > > + va_end(args); > > This code is almost race-free. A (documented) smp_wmb() in here would > make that 100%? > > > + if (len) > > + dump_stack_arch_desc_str[len] = ' '; > > +} On second thoughts I don't think it would. It would order the stores in vsnprintf() vs the store of the space. The idea being you never see a partially printed string. But for that to actually work you need a barrier on the read side, and where do you put it? The cpu printing the buffer could speculate the load of the tail of the buffer, seeing something half printed from vsnprintf(), and then load the head of the buffer and see the space, unless you order those loads. So I don't think we can prevent a crashing cpu seeing a semi-printed buffer without a lock, and we don't want to add a lock. The other issue would be that a reader could miss the trailing NULL from the vsnprintf() but see the space, meaning it would wander off the end of the buffer. But the buffer's in BSS to start with, and we're careful not to print off the end of it, so it should always be NULL terminated. cheers
diff --git a/include/linux/printk.h b/include/linux/printk.h index 9b30871c9149..7539fd417be0 100644 --- a/include/linux/printk.h +++ b/include/linux/printk.h @@ -165,6 +165,7 @@ u32 log_buf_len_get(void); void log_buf_kexec_setup(void); void __init setup_log_buf(int early); void dump_stack_set_arch_desc(const char *fmt, ...); +void dump_stack_add_arch_desc(const char *fmt, ...); void dump_stack_print_info(const char *log_lvl); void show_regs_print_info(const char *log_lvl); #else @@ -219,6 +220,10 @@ static inline void dump_stack_set_arch_desc(const char *fmt, ...) { } +static inline void dump_stack_add_arch_desc(const char *fmt, ...) +{ +} + static inline void dump_stack_print_info(const char *log_lvl) { } diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index c099b082cd02..11d7d587e252 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -3028,6 +3028,52 @@ void __init dump_stack_set_arch_desc(const char *fmt, ...) } /** + * dump_stack_add_arch_desc - add arch-specific info to show with task dumps + * @fmt: printf-style format string + * @...: arguments for the format string + * + * See dump_stack_set_arch_desc() for why you'd want to use this. + * + * This version adds to any existing string already created with either + * dump_stack_set_arch_desc() or dump_stack_add_arch_desc(). If there is an + * existing string a space will be prepended to the passed string. + */ +void __init dump_stack_add_arch_desc(const char *fmt, ...) +{ + va_list args; + int pos, len; + char *p; + + /* + * If there's an existing string we snprintf() past the end of it, and + * then turn the terminating NULL of the existing string into a space + * to create one string separated by a space. + * + * If there's no existing string we just snprintf() to the buffer, like + * dump_stack_set_arch_desc(), but without calling it because we'd need + * a varargs version. + */ + + len = strnlen(dump_stack_arch_desc_str, sizeof(dump_stack_arch_desc_str)); + pos = len; + + if (len) + pos++; + + if (pos >= sizeof(dump_stack_arch_desc_str)) + return; /* Ran out of space */ + + p = &dump_stack_arch_desc_str[pos]; + + va_start(args, fmt); + vsnprintf(p, sizeof(dump_stack_arch_desc_str) - pos, fmt, args); + va_end(args); + + if (len) + dump_stack_arch_desc_str[len] = ' '; +} + +/** * dump_stack_print_info - print generic debug info for dump_stack() * @log_lvl: log level *
Arch code can set a "dump stack arch description string" which is displayed with oops output to describe the hardware platform. It is useful to initialise this as early as possible, so that an early oops will have the hardware description. However in practice we discover the hardware platform in stages, so it would be useful to be able to incrementally fill in the hardware description as we discover it. This patch adds that ability, by creating dump_stack_add_arch_desc(). If there is no existing string it behaves exactly like dump_stack_set_arch_desc(). However if there is an existing string it appends to it, with a leading space. This makes it easy to call it multiple times from different parts of the code and get a reasonable looking result. Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- include/linux/printk.h | 5 +++++ kernel/printk/printk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) If people are happy with this, ideally we'd take this via the powerpc tree with the rest of the series. There also might be a better way to implement it, if so I'm all ears, but I think what I have here is at least correct.