Re: [PATCH 1/3] reliable stack trace support

[Jan Beulich]

>>> Ingo Molnar <mingo@xxxxxxx> 16.05.06 17:05 >>>
>* Jan Beulich <jbeulich@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/