[Tarantool-patches] [PATCH 1/2] fiber: add PoC for fiber creation backtrace
Cyrill Gorcunov
gorcunov at gmail.com
Mon Jun 7 11:28:12 MSK 2021
On Fri, Jun 04, 2021 at 02:13:10PM +0300, Egor Elchinov via Tarantool-patches wrote:
> From: Egor2001 <elchinov.es at gmail.com>
>
> For now fiber creation backtrace is stored
> in the separate subtable of fiber.info
> called backtrace_parent for convenience.
>
> Lua stacks of fiber creation
> aren't preserved in backtrace yet because
> of need to somehow handle parent Lua state
> inside the child fiber for this sake.
>
> Backtrace caching and demangling
> aren't present yet too as this is a
> proof-of-concept implementation.
>
> Needed for: #4002
> ---
>
> +/**
> + * Collect up to `limit' IP register values
> + * for frames of the current stack into `ip_buf'.
> + * Must be by far faster than usual backtrace according to the
> + * libunwind doc for unw_backtrace().
> + */
> +void
> +fast_trace_collect(void **ip_buf, int limit)
> +{
> + memset(ip_buf, 0, limit * sizeof(*ip_buf));
> + unw_backtrace(ip_buf, limit);
> +}
This is not guaranteed to be faster, so I would name it
backtrace_collect_ip. Also you have to mark it as NOINLINE,
otherwise IPs gonna be screwed. Moreover you put a special
offset into IPs resolving inside fast_trace_foreach to hide
the fast_trace_collect call itself, I think we should be
very loud about, otherwise other people might get confusing
what we're doing with frame numbers and why we skip the first
frame.
Also I personally not sure if we must collect fiber's creation
backtrace for every fiber in a system even if we never need it,
I'm pretty sure that backtrace is very far from cheap. But I
left it up to you to decide. I guess we might need some kind
of dynamic settings similar to fiber_top?
> +void
> +fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx)
> +{
> + static __thread char proc_name[BACKTRACE_NAME_MAX];
Why do you need it be per thread? There must be very strong reason why some
data is put into TLS.
> + int frame_no = 0;
This is shor routine and I think plain `n` would be more than enough.
> + unw_word_t ip = 0, offset = 0;
> + unw_proc_info_t pi;
> + int ret = 0;
> + char* proc = NULL;
> +
> + unw_accessors_t* acc = unw_get_accessors(unw_local_addr_space);
> + assert(acc);
> +
> + for (frame_no = 0; frame_no < limit && ip_buf[frame_no] != NULL;
> + ++frame_no) {
> + ip = (unw_word_t)ip_buf[frame_no];
> + if (acc->get_proc_name == NULL) {
> + ret = unw_get_proc_info_by_ip(unw_local_addr_space,
> + ip, &pi, NULL);
> + offset = ip - pi.start_ip;
Why proc is left untouched here? it may carry old value from
previous acc->get_proc_name call, is it ok?
> + } else {
> + ret = acc->get_proc_name(unw_local_addr_space, ip,
> + proc_name, sizeof(proc_name),
> + &offset, NULL);
> + proc = proc_name;
> + }
> +
> + if (ret != 0 || (frame_no > 0 &&
> + cb(frame_no - 1, (void *)ip, proc,
> + (size_t)offset, cb_ctx) != 0))
> + break;
> + }
Egor, I think this is very good PoC code! Here is the diff I made
on top of your patch to share ideas. Not insisting anyhow on code
refactoring, vars renaming and etc.
Also I CC Vlad, since he might have own vision on overall code design.
And since Vlad is the final line before code goes upstream better wait
for his opinion.
---
Index: tarantool.git/src/lib/core/backtrace.cc
===================================================================
--- tarantool.git.orig/src/lib/core/backtrace.cc
+++ tarantool.git/src/lib/core/backtrace.cc
@@ -438,8 +438,8 @@ out:
* Must be by far faster than usual backtrace according to the
* libunwind doc for unw_backtrace().
*/
-void
-fast_trace_collect(void **ip_buf, int limit)
+void NOINLINE
+backtrace_collect_ip(void **ip_buf, int limit)
{
memset(ip_buf, 0, limit * sizeof(*ip_buf));
unw_backtrace(ip_buf, limit);
@@ -457,35 +457,38 @@ fast_trace_collect(void **ip_buf, int li
* TODO: to add cache and demangling support
*/
void
-fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx)
+backtrace_foreach_ip(backtrace_cb cb, void **ip_buf, int limit,
+ void *cb_ctx)
{
- static __thread char proc_name[BACKTRACE_NAME_MAX];
- int frame_no = 0;
+ char proc_name[BACKTRACE_NAME_MAX];
unw_word_t ip = 0, offset = 0;
unw_proc_info_t pi;
- int ret = 0;
- char* proc = NULL;
+ int ret, n;
+ char *proc;
+
+ unw_accessors_t *acc = unw_get_accessors(unw_local_addr_space);
- unw_accessors_t* acc = unw_get_accessors(unw_local_addr_space);
- assert(acc);
+ /*
+ * RIPs collecting comes from inside a helper routine
+ * so we skip the collector function address itself thus
+ * start fetching functions with frame number = 1.
+ */
+ for (n = 1; n < limit && ip_buf[n] != NULL; n++) {
+ ip = (unw_word_t)ip_buf[n];
- for (frame_no = 0; frame_no < limit && ip_buf[frame_no] != NULL;
- ++frame_no) {
- ip = (unw_word_t)ip_buf[frame_no];
if (acc->get_proc_name == NULL) {
ret = unw_get_proc_info_by_ip(unw_local_addr_space,
ip, &pi, NULL);
offset = ip - pi.start_ip;
+ proc = NULL;
} else {
ret = acc->get_proc_name(unw_local_addr_space, ip,
- proc_name, sizeof(proc_name),
- &offset, NULL);
+ proc_name, sizeof(proc_name),
+ &offset, NULL);
proc = proc_name;
}
-
- if (ret != 0 || (frame_no > 0 &&
- cb(frame_no - 1, (void *)ip, proc,
- (size_t)offset, cb_ctx) != 0))
+ if (ret != 0 || cb(n - 1, (void *)ip, proc,
+ (size_t)offset, cb_ctx) != 0)
break;
}
Index: tarantool.git/src/lua/fiber.c
===================================================================
--- tarantool.git.orig/src/lua/fiber.c
+++ tarantool.git/src/lua/fiber.c
@@ -314,8 +314,9 @@ lbox_fiber_statof_map(struct fiber *f, v
tb_ctx.R = NULL;
lua_pushstring(L, "backtrace_parent");
lua_newtable(L);
- fast_trace_foreach(fiber_backtrace_cb, f->parent_bt_ip_buf,
- FIBER_PARENT_BT_MAX, &tb_ctx);
+ backtrace_foreach_ip(fiber_backtrace_cb,
+ f->parent_bt_ip_buf,
+ FIBER_PARENT_BT_MAX, &tb_ctx);
lua_settable(L, -3);
#endif /* ENABLE_BACKTRACE */
}
Index: tarantool.git/src/lib/core/fiber.h
===================================================================
--- tarantool.git.orig/src/lib/core/fiber.h
+++ tarantool.git/src/lib/core/fiber.h
@@ -654,7 +654,7 @@ struct fiber {
char inline_name[FIBER_NAME_INLINE];
#if ENABLE_BACKTRACE
/** Fiber creation backtrace chunk. */
- void* parent_bt_ip_buf[FIBER_PARENT_BT_MAX];
+ void *parent_bt_ip_buf[FIBER_PARENT_BT_MAX];
#endif /* ENABLE_BACKTRACE */
};
Index: tarantool.git/src/lib/core/fiber.c
===================================================================
--- tarantool.git.orig/src/lib/core/fiber.c
+++ tarantool.git/src/lib/core/fiber.c
@@ -1265,7 +1265,7 @@ fiber_new_ex(const char *name, const str
fiber->fid = cord->next_fid;
fiber_set_name(fiber, name);
#if ENABLE_BACKTRACE
- fast_trace_collect(fiber->parent_bt_ip_buf, FIBER_PARENT_BT_MAX);
+ backtrace_collect_ip(fiber->parent_bt_ip_buf, FIBER_PARENT_BT_MAX);
#endif /* ENABLE_BACKTRACE */
register_fid(fiber);
fiber->csw = 0;
Index: tarantool.git/src/lib/core/backtrace.h
===================================================================
--- tarantool.git.orig/src/lib/core/backtrace.h
+++ tarantool.git/src/lib/core/backtrace.h
@@ -31,6 +31,7 @@
* SUCH DAMAGE.
*/
#include "trivia/config.h"
+#include "trivia/util.h"
#include <stddef.h>
#if defined(__cplusplus)
@@ -55,11 +56,12 @@ backtrace_foreach(backtrace_cb cb, coro_
void
backtrace_proc_cache_clear(void);
-void
-fast_trace_collect(void **ip_buf, int limit);
+void NOINLINE
+backtrace_collect_ip(void **ip_buf, int limit);
void
-fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx);
+backtrace_foreach_ip(backtrace_cb cb, void **ip_buf, int limit,
+ void *cb_ctx);
#endif /* ENABLE_BACKTRACE */
More information about the Tarantool-patches
mailing list