Messages in this thread | | | Date | Tue, 16 May 2006 18:01:31 +0200 | From | "Jan Beulich" <> | Subject | Re: [PATCH 1/3] reliable stack trace support |
| |
>>> Ingo Molnar <mingo@elte.hu> 16.05.06 17:05 >>> >* Jan Beulich <jbeulich@novell.com> wrote: >> +#ifdef CONFIG_STACK_UNWIND >> +#include <asm/unwind.h> >> +#else >> +#include <asm-generic/unwind.h> >> +#endif > >this wants to become include/linux/unwind.h?
Not really, at least not until IA64 and PARISC get adopted to the same (architecture independent) interface.
>> +#ifdef MODULE_UNWIND_INFO >> +#include <asm/unwind.h> >> +#endif > >this too could then include <linux/unwind.h>
As above.
>> +DEFINE_SPINLOCK(table_lock); > >static?
Oh, yes.
>> +static struct unwind_table * >> +find_table(unsigned long pc) >> +{ >> + int old_removals; >> + struct unwind_table *table = NULL; >> + >> + do { >> + if (table) >> + atomic_dec(&table->users); >> + old_removals = atomic_read(&removals); > >racy? wants to become rcu?
I don't think so. As far as I can tell, this isn't going to be a problem, it may just result in an extra, normally unneeded, re-run of the loop.
>> + spin_lock(&table_lock); > >spin_lock_irq?
Why?
>> + if (init_only && table == last_table) { >> + table->init.pc = 0; >> + table->init.range = 0; >> + return; >> + } > >SMP and PREEMPT unsafe.
I don't think so, given that this can be called only from the module loader. As Andi pointed out elsewhere, it may even be unnecessary to do the locking at all.
>> + spin_lock(&table_lock); > >spin_lock_irq().
Again, why?
>> + if (table) { >> + while (atomic_read(&table->users) || atomic_read(&lookups)) >> + msleep(1); >> + kfree(table); >> + } > >ugh!
???
>> +//todo case DW_CFA_def_cfa_expression: >> +//todo case DW_CFA_expression: >> +//todo case DW_CFA_val_expression: > >hm?
This means what it says - it needs to be done, and I have no clear understanding of how these expressions are to be treated, as I've never seen them in use anywhere.
>> +{ >> + info->task = current; >> + arch_unwind_init_running(info, callback, arg); >> + return 0; > >newline before the return. (this happens in a couple of other places >too)
Surely can do that, although I don't see why this should be needed in functions this small.
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/
| |