RE: [PATCH v3] perf tools: Introduce perf_thread for backtrace

From: åæéå / HIRAMATUïMASAMI
Date: Thu Dec 03 2015 - 02:13:28 EST

Next message: Joonsoo Kim: "[PATCH v3 0/7] mm/compaction: redesign compaction: part1"
Previous message: Joonsoo Kim: "[PATCH v3 3/7] mm/compaction: initialize compact_order_failed to MAX_ORDER"
In reply to: Namhyung Kim: "Re: [PATCH v3] perf tools: Introduce perf_thread for backtrace"
Next in thread: Namhyung Kim: "Re: [PATCH v3] perf tools: Introduce perf_thread for backtrace"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

From: Namhyung Kim [mailto:namhyung@xxxxxxxxxx]
>
>On Thu, Dec 03, 2015 at 12:15:12AM +0000, åæéå / HIRAMATUïMASAMI wrote:
>> >From: Namhyung Kim [mailto:namhyung@xxxxxxxxx] On Behalf Of Namhyung Kim
>> >
>> >Backtrace is a crucial info for debugging. And upcoming refcnt
>> >tracking facility also wants to use it.
>>
>> Note that the refcnt backtrace symbol resolution will work at
>> exit. This means that it can not depend on the feature in perf
>> tools itself. (and of course, since the refcnt tries to find unused
>> objects in perf tools at exit, if we use perf_thread, it will
>> detect the objects related to the perf_thread are leaked)
>
>Hmm.. right.
>
>I think we can leave the perf_thread outside of refcnt infrastructure.
>IOW it should be created before refcnt debugging is activated and
>released after refcnt is done. What do you think?

Would you mean we don't debug the objects related to a perf_thread?
It will mean that you don't debug anything, since perf_thread involves
most of refcnt using objects, like dso, map, map_groups etc. And some
bugs are actually found at where those objects are handled.

I would not like to care about the output quality of the backtrace_symbols.
I only need the top 2-3 addresses of the backtrace buffer, because I have
(eu-)addr2line command to find the actual source code lines from those
addresses :). If you need, I can also provide an address decoder awk/shell
script for that.

Instead, I prefer to avoid complexity on the "debugging feature"(because
it can introduce new bugs,) and make it more robust (e.g. if we failed to
get symbol, just shows the raw address)

BTW, the robustness is a key point for debugging. Please consider the case
that you hit an error on the objects in the perf_thread, it could cause
double fault(segv again) on the same object. That is what I actually don't
want.

Thank you,

>
>Thanks,
>Namhyung
>
>
>>
>> >
>> >So instead of relying on glibc's backtrace_symbols[_fd] which misses
>> >some (static) functions , use our own symbol searching mechanism. To
>> >do that, add perf_thread global variable to keep its maps and symbols.
>> >
>> >The backtrace output from TUI is changed like below. (I made a key
>> >action to generate a segfault):
>> >
>> >Before:
>> > perf: Segmentation fault
>> > -------- backtrace --------
>> > perf[0x544a8b]
>> > /usr/lib/libc.so.6(+0x33680)[0x7fc46420b680]
>> > perf[0x54041b]
>> > perf(perf_evlist__tui_browse_hists+0x91)[0x5432e1]
>> > perf(cmd_report+0x1d20)[0x43cb10]
>> > perf[0x487073]
>> > perf(main+0x62f)[0x42cb1f]
>> > /usr/lib/libc.so.6(__libc_start_main+0xf0)[0x7fc4641f8610]
>> > perf(_start+0x29)[0x42cc39]
>> >
>> >After:
>> > perf: Segmentation fault
>> > -------- backtrace --------
>> > perf_evsel__hists_browse(+0x43b) in perf [0x54066b]
>> > perf_evlist__tui_browse_hists(+0x91) in perf [0x543531]
>> > cmd_report(+0x1d20) in perf [0x43cb50]
>> > run_builtin(+0x53) in perf [0x4870b3]
>> > main(+0x634) in perf [0x42cb54]
>> > __libc_start_main(+0xf0) in libc-2.22.so [0x7fea3577c610]
>> > _start(+0x29) in perf [0x42cc79]
>> >
>> >Cc: Frederic Weisbecker <fweisbec@xxxxxxxxx>
>> >Cc: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
>> >Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
>> >---
>> >v3) check return value of create_perf_thread()
>> >
>> >v2) fallback to glibc's backtrace_symbols_fd() if failed
>> >
>> > tools/perf/perf.c | 10 +++++++-
>> > tools/perf/ui/tui/setup.c | 31 +++++++++++++++++++++--
>> > tools/perf/util/util.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>> > tools/perf/util/util.h | 5 ++++
>> > 4 files changed, 106 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/tools/perf/perf.c b/tools/perf/perf.c
>> >index 4bee53c3f796..f27dc5fa9534 100644
>> >--- a/tools/perf/perf.c
>> >+++ b/tools/perf/perf.c
>> >@@ -604,6 +604,11 @@ int main(int argc, const char **argv)
>> > */
>> > pthread__block_sigwinch();
>> >
>> >+ if (create_perf_thread() < 0) {
>> >+ fprintf(stderr, "Failed to reserve information for backtrace\n");
>> >+ goto out;
>> >+ }
>> >+
>> > while (1) {
>> > static int done_help;
>> > int was_alias = run_argv(&argc, &argv);
>> >@@ -615,7 +620,7 @@ int main(int argc, const char **argv)
>> > fprintf(stderr, "Expansion of alias '%s' failed; "
>> > "'%s' is not a perf-command\n",
>> > cmd, argv[0]);
>> >- goto out;
>> >+ goto out_destroy;
>> > }
>> > if (!done_help) {
>> > cmd = argv[0] = help_unknown_cmd(cmd);
>> >@@ -626,6 +631,9 @@ int main(int argc, const char **argv)
>> >
>> > fprintf(stderr, "Failed to run command '%s': %s\n",
>> > cmd, strerror_r(errno, sbuf, sizeof(sbuf)));
>> >+
>> >+out_destroy:
>> >+ destroy_perf_thread();
>> > out:
>> > return 1;
>> > }
>> >diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
>> >index 7dfeba0a91f3..ba0ff1c4e6b7 100644
>> >--- a/tools/perf/ui/tui/setup.c
>> >+++ b/tools/perf/ui/tui/setup.c
>> >@@ -13,6 +13,8 @@
>> > #include "../libslang.h"
>> > #include "../keysyms.h"
>> > #include "tui.h"
>> >+#include "../../util/symbol.h"
>> >+#include "../../util/thread.h"
>> >
>> > static volatile int ui__need_resize;
>> >
>> >@@ -96,14 +98,39 @@ int ui__getch(int delay_secs)
>> > static void ui__signal_backtrace(int sig)
>> > {
>> > void *stackdump[32];
>> >- size_t size;
>> >+ size_t size, i;
>> >
>> > ui__exit(false);
>> > psignal(sig, "perf");
>> >
>> > printf("-------- backtrace --------\n");
>> > size = backtrace(stackdump, ARRAY_SIZE(stackdump));
>> >- backtrace_symbols_fd(stackdump, size, STDOUT_FILENO);
>> >+ /* skip first two stack frame (for this function and signal stack) */
>> >+ for (i = 2; i < size; i++) {
>> >+ struct addr_location al = {
>> >+ .sym = NULL,
>> >+ };
>> >+
>> >+ /* valid callchain ends with 0 address */
>> >+ if (stackdump[i] == NULL && i == (size - 1))
>> >+ continue;
>> >+
>> >+ thread__find_addr_location(perf_thread, PERF_RECORD_MISC_USER,
>> >+ MAP__FUNCTION, (long)stackdump[i], &al);
>> >+
>> >+ if (!al.sym)
>> >+ break;
>> >+
>> >+ printf("%s(+0x%"PRIx64") in %s [0x%lx]\n", al.sym->name,
>> >+ map__map_ip(al.map, (u64)stackdump[i]) - al.sym->start,
>> >+ al.map->dso->short_name, (unsigned long)stackdump[i]);
>> >+ }
>> >+
>> >+ if (i != size) {
>> >+ printf("\nperf backtrace seems broken, try again with glibc\n");
>> >+ printf("-------- backtrace --------\n");
>> >+ backtrace_symbols_fd(stackdump, size, STDOUT_FILENO);
>> >+ }
>> >
>> > exit(0);
>> > }
>> >diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
>> >index 75759aebc7b8..b1e591b13131 100644
>> >--- a/tools/perf/util/util.c
>> >+++ b/tools/perf/util/util.c
>> >@@ -16,6 +16,9 @@
>> > #include <linux/kernel.h>
>> > #include <unistd.h>
>> > #include "callchain.h"
>> >+#include "machine.h"
>> >+#include "thread.h"
>> >+#include "thread_map.h"
>> >
>> > struct callchain_param callchain_param = {
>> > .mode = CHAIN_GRAPH_ABS,
>> >@@ -696,3 +699,63 @@ fetch_kernel_version(unsigned int *puint, char *str,
>> > *puint = (version << 16) + (patchlevel << 8) + sublevel;
>> > return 0;
>> > }
>> >+
>> >+
>> >+static int process_event(struct perf_tool *tool, union perf_event *event,
>> >+ struct perf_sample *sample, struct machine *machine)
>> >+{
>> >+ switch (event->header.type) {
>> >+ case PERF_RECORD_COMM:
>> >+ return tool->comm(tool, event, sample, machine);
>> >+ case PERF_RECORD_MMAP:
>> >+ return tool->mmap(tool, event, sample, machine);
>> >+ case PERF_RECORD_MMAP2:
>> >+ return tool->mmap2(tool, event, sample, machine);
>> >+ default:
>> >+ break;
>> >+ }
>> >+ return 0;
>> >+}
>> >+
>> >+struct thread *perf_thread;
>> >+
>> >+int create_perf_thread(void)
>> >+{
>> >+ struct perf_tool tool = {
>> >+ .comm = perf_event__process_comm,
>> >+ .mmap = perf_event__process_mmap,
>> >+ .mmap2 = perf_event__process_mmap2,
>> >+ };
>> >+ struct thread_map *tm;
>> >+ struct machine *machine;
>> >+ int pid = getpid();
>> >+
>> >+ machine = machine__new_host();
>> >+ if (machine == NULL)
>> >+ return -1;
>> >+
>> >+ tm = thread_map__new_dummy();
>> >+ if (tm == NULL) {
>> >+ machine__delete(machine);
>> >+ return -1;
>> >+ }
>> >+
>> >+ thread_map__set_pid(tm, 0, pid);
>> >+
>> >+ perf_event__synthesize_thread_map(&tool, tm, process_event, machine,
>> >+ false, 500);
>> >+
>> >+ perf_thread = machine__find_thread(machine, pid, pid);
>> >+ BUG_ON(perf_thread == NULL);
>> >+
>> >+ thread_map__put(tm);
>> >+ return 0;
>> >+}
>> >+
>> >+void destroy_perf_thread(void)
>> >+{
>> >+ struct machine *machine = perf_thread->mg->machine;
>> >+
>> >+ machine__delete_threads(machine);
>> >+ machine__delete(machine);
>> >+}
>> >diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
>> >index dcc659017976..769986044a7a 100644
>> >--- a/tools/perf/util/util.h
>> >+++ b/tools/perf/util/util.h
>> >@@ -358,4 +358,9 @@ int fetch_kernel_version(unsigned int *puint,
>> > #define KVER_FMT "%d.%d.%d"
>> > #define KVER_PARAM(x) KVER_VERSION(x), KVER_PATCHLEVEL(x), KVER_SUBLEVEL(x)
>> >
>> >+extern struct thread *perf_thread;
>> >+
>> >+int create_perf_thread(void);
>> >+void destroy_perf_thread(void);
>> >+
>> > #endif /* GIT_COMPAT_UTIL_H */
>> >--
>> >2.6.2
>>

Next message: Joonsoo Kim: "[PATCH v3 0/7] mm/compaction: redesign compaction: part1"
Previous message: Joonsoo Kim: "[PATCH v3 3/7] mm/compaction: initialize compact_order_failed to MAX_ORDER"
In reply to: Namhyung Kim: "Re: [PATCH v3] perf tools: Introduce perf_thread for backtrace"
Next in thread: Namhyung Kim: "Re: [PATCH v3] perf tools: Introduce perf_thread for backtrace"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]