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

[Andi Kleen <ak@suse.de>]

On Thursday 18 May 2006 15:42, Jan Beulich wrote:
> >> +#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?

Stubs or something calling the normal unwinder are fine.


> >> +#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.

At some point they will probably, but ok - keep them private for now.

> >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.

Linux is not written in ISO-C, it's Linux C. And in that uN types are used, not uintN_t

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

Well it's like this all over the kernel. You can run Lindent if you prefer, but
usually it takes more work to clean up after it. It's also part of that "Linux C" thing.
 
> >> +				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.

seqlock doesn't really take a look in the classical sense. The only way to make it 
lock up would be to have another CPU livelocking on this in a loop.

Anyways most likely it isn't needed because of the stop_machine and can be removed
> 
> >> +	table = kmalloc(sizeof(*table), GFP_USER);
> >
> >GFP_KERNEL.
> 
> Not sure about the significance - I took this from respective ia64 code.

GFP_USER is for user space pages only.

> >> +#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.

Those never set CONFIG_FP
 
-Andi