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

[Ingo Molnar <mingo@elte.hu>]

* Jan Beulich <jbeulich@novell.com> wrote:

> These are the generic bits needed to enable reliable stack traces 
> based on Dwarf2-like (.eh_frame) unwind information. Subsequent 
> patches will enable x86-64 and i386 to make use of this.

some more detailed review:

> +#ifdef CONFIG_STACK_UNWIND
> +#include <asm/unwind.h>
> +#else
> +#include <asm-generic/unwind.h>
> +#endif

this wants to become include/linux/unwind.h?

> +#ifdef MODULE_UNWIND_INFO
> +#include <asm/unwind.h>
> +#endif

this too could then include <linux/unwind.h>

> +DEFINE_SPINLOCK(table_lock);

static?

> +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?

> +	spin_lock(&table_lock);

spin_lock_irq?

> +	if (init_only && table == last_table) {
> +		table->init.pc = 0;
> +		table->init.range = 0;
> +		return;
> +	}

SMP and PREEMPT unsafe.

> +	spin_lock(&table_lock);

spin_lock_irq().

> +	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?

> +{
> +	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)

	Ingo
-
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/