Messages in this thread | | | Date | Thu, 18 May 2006 15:42:05 +0200 | From | "Jan Beulich" <> | Subject | Re: [PATCH 1/3] reliable stack trace support |
| |
>> +#ifdef CONFIG_STACK_UNWIND >> +#include <asm/unwind.h> >> +#else >> +#include <asm-generic/unwind.h> >> +#endif > >Normally the other archs should get stub includes that include asm-generic/unwind.h
This would make them appear kind of supporting unwind, even though they really don't, wouldn't it?
>> + unwind_init(); > >Stupid q. but what happens when we get a crash before unwind_init? >Is the failure benign?
You'll get old fashioned stack traces, which I consider benign.
>> +#ifdef MODULE_UNWIND_INFO >> +#include <asm/unwind.h> >> +#endif > >It should be possible to include this without the ifdef, no?
Only if all arch-s get an asm/unwind.h, which I consider ill (see above).
>> +#ifdef MODULE_UNWIND_INFO >> + unwind_remove_table(mod->unwind_info, 0); >> +#endif > >Might be better to stub it in the include
It is stubbed, but as above - the include isn't always there.
>> +static const struct { >> + unsigned offs:BITS_PER_LONG / 2; >> + unsigned width:BITS_PER_LONG / 2; >> +} reg_info[] = { >> + UNW_REGISTER_INFO >> +}; >> + >> +#undef PTREGS_INFO >> +#undef EXTRA_INFO > >Where are they actually used? I can't find UNW_REGISTER_INFO >in the patch.
These are defined per architecture.
>In general it looks a bit overcomplicated. Can you just >use the values directly in the unwinder code?
Which values? The offsets into pt_regs are clearly architecture specific, so I don't think it'd be nice to put them into generic code.
>> +#ifndef REG_INVALID > >Who would set it?
Again, an architecture if the default definition isn't sufficient.
>> +#define DW_CFA_nop 0x00 > >I guess it would be useful to have them in some include. >Maybe linux/dwarf2.h ?
Do you think they might be re-used by anyone else? I generally prefer keeping stuff used only in a single place out of sight for anyone else.
>In general please replace all uintN_t with uN
Why that? What are these types for then? After all, they're standard mandated, and one more of my preferences is to use standard types where-ever possible.
>> + >> +static struct unwind_table * >> +find_table(unsigned long pc) > >Should be on one line. More further down.
Make the code uglier in my opinion, especially when the parameter declarations are quite long.
>> + atomic_inc(&table->users); >> + break; >> + } >> + atomic_dec(&lookups); >> + } while (atomic_read(&removals) != old_removals); > >This looks like a seq lock? Use the real thing?
The code here should get away without taking *any* locks, otherwise you may end up not seeing any backtrace at all when the system dies.
>> + table = kmalloc(sizeof(*table), GFP_USER); > >GFP_KERNEL.
Not sure about the significance - I took this from respective ia64 code.
>> + if (table) { >> + while (atomic_read(&table->users) || atomic_read(&lookups)) >> + msleep(1); > >Can't this livelock? > >I suspect it isn't needed anyways because module unload uses stop_machine() >already and that should be enough to stop the lockups which don't block.
That is what I wasn't sure of - if these functions are indeed meant to be called only from the module loader (which I think they are), then table_lock isn't needed (serialized by module_mutex).
>> +#ifdef UNW_FP > >This should be CONFIG_FRAME_POINTER
No - there are arch-s (ia64, while clearly not going to be getting here, immediately comes to mind) where you cannot define what register the frame pointer is in. I rather think x86 is special in that you actually can.
> + unsigned long top, bottom; > +#endif > + > + drop_table(table); > +#ifdef UNW_FP > + top = STACK_TOP(frame->task); > + bottom = STACK_BOTTOM(frame->task); > +# if FRAME_RETADDR_OFFSET < 0
Nasty ifdefs. Can you perhaps isolate that < 0 case in a separate function. Also when does it happen anyways? A little bit cleanup here would be good.
>> +EXPORT_SYMBOL_GPL(unwind_init_frame_info); > >I would actually use EXPORT_SYMBOL(). Would be unfair to not give an unwinder >to any modules.
Fine with me - I just followed other people's demands (on other occasions) to only use GPL exports for new symbols.
>> config UNWIND_INFO >> bool "Compile the kernel with frame unwind information" >> - depends on !IA64 >> + depends on !IA64 && !PARISC > >Why PARISC?
Because it, like ia64, has its own unwinding logic.
Jan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |