x86: make save_stack_address() !CONFIG_FRAME_POINTER friendly

If CONFIG_FRAME_POINTER=n, print_context_stack() shouldn't neglect the non-reliable addresses on stack, this is all we have if dump_trace(bp) is called with the wrong or zero bp.

For example, /proc/pid/stack doesn't work if CONFIG_FRAME_POINTER=n.

This patch obviously has no effect if CONFIG_FRAME_POINTER=y, otherwise it reverts 1650743c "x86: don't save unreliable stack trace entries".

Also, remove the unnecessary type-cast.

Signed-off-by Oleg Nesterov <[email protected]>

If CONFIG_FRAME_POINTER=n, print_context_stack() shouldn't neglect the non-reliable addresses on stack, this is all we have if dump_trace(bp) is called with the wrong or zero bp.

For example, /proc/pid/stack doesn't work if CONFIG_FRAME_POINTER=n.

This patch obviously has no effect if CONFIG_FRAME_POINTER=y, otherwise it reverts 1650743c "x86: don't save unreliable stack trace entries".

Also, remove the unnecessary type-cast.

Signed-off-by Oleg Nesterov<[email protected]>

---

  arch/x86/kernel/stacktrace.c |    6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

arch/x86/kernel/stacktrace.c renamed from arch/x86/kernel/stacktrace.c~SAVE_STACK_WO_FP +5 -1

@@ -26,8 +26,10 @@ static int save_stack_stack(void *data,
  static void save_stack_address(void *data, unsigned long addr, int reliable)
  {
  	struct stack_trace *trace = data;
+#ifdef CONFIG_FRAME_POINTER
  	if (!reliable)
  		return;
+#endif
  	if (trace->skip>  0) {
  		trace->skip--;
  		return;

@@ -39,9 +41,11 @@ static void save_stack_address(void *dat
  static void
  save_stack_address_nosched(void *data, unsigned long addr, int reliable)
  {
-	struct stack_trace *trace = (struct stack_trace *)data;
+	struct stack_trace *trace = data;
+#ifdef CONFIG_FRAME_POINTER
  	if (!reliable)
  		return;
+#endif
  	if (in_sched_functions(addr))
  		return;
  	if (trace->skip>  0) {

Quoted 4 lines · Oleg Nesterov

If CONFIG_FRAME_POINTER=n, print_context_stack() shouldn't neglect the non-reliable addresses on stack, this is all we have if dump_trace(bp) is called with the wrong or zero bp.

Quoted 5 lines

For example, /proc/pid/stack doesn't work if CONFIG_FRAME_POINTER=n.

This patch obviously has no effect if CONFIG_FRAME_POINTER=y, otherwise it reverts 1650743c "x86: don't save unreliable stack trace entries".

would be nice if there was a compile time thing to detect if frame pointers are on ratehr than an ifdef.

you're now also changing the rules; until now, you would ALWAYS get a backtrace without noise....
now that's changing quite a bit. How are various tools (like perf and sysprof) going to cope with that?

Quoted 5 lines · Arjan van de Ven

Quoted 4 lines · Oleg Nesterov

If CONFIG_FRAME_POINTER=n, print_context_stack() shouldn't neglect the non-reliable addresses on stack, this is all we have if dump_trace(bp) is called with the wrong or zero bp.

Quoted 7 lines

Quoted 4 lines

For example, /proc/pid/stack doesn't work if CONFIG_FRAME_POINTER=n.

This patch obviously has no effect if CONFIG_FRAME_POINTER=y, otherwise it reverts 1650743c "x86: don't save unreliable stack trace entries".

would be nice if there was a compile time thing to detect if frame pointers are on ratehr than an ifdef.

I wanted to suggest that too, but since only one place got the ifdef after the second patch.

But yeah, something like this could be reused:

	if (reliable_frame_pointer(reliable))
		return ...;

Quoted 4 lines

you're now also changing the rules; until now, you would ALWAYS get a backtrace without noise....
now that's changing quite a bit. How are various tools (like perf and sysprof) going to cope with that?

perf and sysprof have their own stacktrace ops, so they aren't affected. I think the rest is /proc/pid/task, lockdep, latencytop, ftrace, kmemleak, etc...

For the kernel parts it's in fact desired.
And with ftrace we are changing some rules, but this is desired too, without frame pointers we would have nothing anyway. And it's quite easy to find out a stacktrace is not entirely reliable at a glance.

Quoted 18 lines · Frederic Weisbecker

Quoted 5 lines · Arjan van de Ven

Quoted 4 lines · Oleg Nesterov

If CONFIG_FRAME_POINTER=n, print_context_stack() shouldn't neglect the non-reliable addresses on stack, this is all we have if dump_trace(bp) is called with the wrong or zero bp.

Quoted 7 lines

Quoted 4 lines

For example, /proc/pid/stack doesn't work if CONFIG_FRAME_POINTER=n.

This patch obviously has no effect if CONFIG_FRAME_POINTER=y, otherwise it reverts 1650743c "x86: don't save unreliable stack trace entries".

would be nice if there was a compile time thing to detect if frame pointers are on ratehr than an ifdef.

I wanted to suggest that too, but since only one place got the ifdef after the second patch.

Quoted 4 lines

But yeah, something like this could be reused:

	if (reliable_frame_pointer(reliable))
		return ...;

Do you mean it makes sense to add the helper which depends on FRAME_POINTER ?

Quoted 7 lines

Quoted 4 lines

you're now also changing the rules; until now, you would ALWAYS get a backtrace without noise....
now that's changing quite a bit. How are various tools (like perf and sysprof) going to cope with that?

Quoted 8 lines

perf and sysprof have their own stacktrace ops, so they aren't affected. I think the rest is /proc/pid/task, lockdep, latencytop, ftrace, kmemleak, etc...

For the kernel parts it's in fact desired.
And with ftrace we are changing some rules, but this is desired too, without frame pointers we would have nothing anyway. And it's quite easy to find out a stacktrace is not entirely reliable at a glance.

Frederic, Arjan. Honestly, I have no opinion if this change makes things better or worse for, say, lockdep.

But note that this only affects the !CONFIG_FRAME_POINTER case. Looking into Kconfig's I don't even understand how the bug reporters managed to set CONFIG_STACKTRACE without !CONFIG_FRAME_POINTER.

So, should I redo this patch to fix /proc/pid/stack ? Say, we can change the meaning of stack_trace-<skip, if it is < 0, then save_stack_address() ignores reliable. Yes, this is hack.

Quoted 3 lines · Oleg Nesterov

Do you mean it makes sense to add the helper which depends on FRAME_POINTER ?

Having in asm/stacktrace.h:

#ifdef CONFIG_FRAME_POINTER
static inline int reliable_frame_pointer(int reliable)

{
	return reliable;
}
#else
static inline int reliable_frame_pointer(int reliable)
{
	return 1;
}

#endif

But if we have only one user I'm not sure it's worth it.

Quoted 7 lines

Frederic, Arjan. Honestly, I have no opinion if this change makes things better or worse for, say, lockdep.

But note that this only affects the !CONFIG_FRAME_POINTER case. Looking into Kconfig's I don't even understand how the bug reporters managed to set CONFIG_STACKTRACE without !CONFIG_FRAME_POINTER.

Quoted 3 lines

So, should I redo this patch to fix /proc/pid/stack ? Say, we can change the meaning of stack_trace-<skip, if it is < 0, then save_stack_address() ignores reliable. Yes, this is hack.

No, people may want to ignore reliable and also to skip entries.

I think your patches as is are the right way to go: by default provide as much information as we can.

And those who care about reliability can use their own stack ops, which is what perf does for example. If needed we can still add a new save_stack_trace_reliable() in the future.