[RFC] Merging ftrace_stack, perf_callchain, oprofile->backtrace and stack_trace

* [RFC] Merging ftrace_stack, perf_callchain, oprofile->backtrace and stack_trace
@ 2018-10-21  9:31 Aleksa Sarai
  2018-10-21  9:41 ` Aleksa Sarai
  2018-10-21 20:13 ` Peter Zijlstra
  0 siblings, 2 replies; 4+ messages in thread
From: Aleksa Sarai @ 2018-10-21  9:31 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, Steven Rostedt,
	Robert Richter, Brendan Gregg
  Cc: linux-kernel, oprofile-list, cyphar

[-- Attachment #1: Type: text/plain, Size: 2320 bytes --]

Hi all,

I'm currently working on a patchset to make kretprobes produce
reasonable stack traces[1], and it appears this is a generic problem
across the entire kernel -- you can see the same kretprobe_trampoline()
issue when using ftrace just as much as bpf_trace.

However, in working on this patch, I've noticed that there appear to be
several different implementations of "get the stack trace from this
pt_regs" which all appear quite similar. Namely:

  * struct ftrace_stack;
  * struct perf_callchain_entry; [**]
  * struct stack_trace;

  * oprofile_operations->backtrace [This is not related to the kretprobe
	problem, very tangential, but since it's usage is not very
	complicated -- logging to dmesg -- it wouldn't be too bad to
	refactor this one too].

Would there be a strong objection to me trying to merge together these
so that they all use 'struct stack_trace', and no longer have
arch-specific code that is doing (what appears to be) the same unwind
code? Or is this something that was intentionally avoided because there
are some differences that I'm not seeing?

The reason I ask is because the kretprobes patch would require saving
the stacktrace during pre_handler_kretprobe() -- and so in order for all
of the tracing subsystems to take advantage of it they'd need to be able
to use that saved stack trace.

The only other option I can see would be to implement some sort of
translation from 'struct stack_trace' to the others. This wouldn't be
too bad, but I imagine it would be uglier than refactoring them all to
use the same struct.

[**] perf_callchain_entry has the concept of "marking" a context in the
     stack trace. But I wonder whether this is something that we could
	 do with 'struct stack_trace' -- after all it's just magic ->ip
	 values. *But* then the question is what is the purpose of
	 sysctl_perf_event_max_contexts_per_stack? It limits the number of
	 contexts, but isn't that already implicitly limited by the number
	 of stack entries? We could also implement this with 'struct
	 stack_trace' but it would require wrapping 'struct stack_trace' to
	 make it efficient.

[1]: https://github.com/iovisor/bpftrace/issues/101

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread