From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 743AA6EC40; Mon, 7 Jun 2021 11:28:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 743AA6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1623054497; bh=WuIcuwH2JZvKwBXsvLRfbe6zxCXbpz4sroLVAXn8ntQ=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=sEADkhOLPxiy2qJWvHrXPW1rmjrtrL3joZWSTShM6waC4g7Ij1E91s1T0rzdEmVxD CBBUgOj7xhEgTELmZCDCs4mxJUHoiHZS8kxwRxohClHlnk/rVeR8sfFzrTLvqiagI7 RiXxx+7WKvR6JJ1WnFpAoi/vdrslr8Dt0msjaGKA= Received: from mail-lf1-f48.google.com (mail-lf1-f48.google.com [209.85.167.48]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D035C6EC40 for ; Mon, 7 Jun 2021 11:28:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D035C6EC40 Received: by mail-lf1-f48.google.com with SMTP id w33so24909911lfu.7 for ; Mon, 07 Jun 2021 01:28:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=6xNV8PuUwajnKwMt9G8ZQWtzwUELWnBOzhX0NwIZZyk=; b=LVdjLH+i3H0z9Rj6P85DZxMPFXU8TghRweODtcFbn37qr7RyNKfQlBQ60cil1jmSKO qa+sV8x4rgyZ3b2oAyHUbkbnYFPye2Wqfe7lo9F0cV6UFqcAtTc/P79NwySHDnl6gc03 j8ztvZ+FPPdhnVyJtqAIycZ4gt83xbeN/fPuop6hPFuh67P9HW+1suzmx7m/yWroCpJs jFxF0DgUCsGyPjhuRoo1jzodEztguscmSCfug3BQlZWq7Jf4iKpNhNCmuOUNwjfjBnVa qxpzfebEo/jopZcv9nHJFCTxoUOIwv5xSJaTWMuWNM70xcnBOEhy6f3OmIjPz4UCAd80 32Og== X-Gm-Message-State: AOAM5333GKuEs01BBJJT9c2SUIi/tDfELZ+shDhEHt3xiVVbnmen0q5h 2iTI1b7m9OQVpwO2HHb2bE0tqlQs1m0= X-Google-Smtp-Source: ABdhPJz2lPM/B7YC3Hr7Fzqp0DYY8Prn5XP73pPQnP+cZEOHppQbEz5yMLi3wowmpvBVDA4xLu++rw== X-Received: by 2002:ac2:4d11:: with SMTP id r17mr11230622lfi.331.1623054494597; Mon, 07 Jun 2021 01:28:14 -0700 (PDT) Received: from grain.localdomain ([5.18.171.94]) by smtp.gmail.com with ESMTPSA id f22sm192548lfe.33.2021.06.07.01.28.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 07 Jun 2021 01:28:13 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id A40275A0042; Mon, 7 Jun 2021 11:28:12 +0300 (MSK) Date: Mon, 7 Jun 2021 11:28:12 +0300 To: eelchinov@tarantool.org Message-ID: References: <8894403bf01395ae2b3082b005ff48178cde3081.1622792861.git.elchinov.es@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8894403bf01395ae2b3082b005ff48178cde3081.1622792861.git.elchinov.es@gmail.com> User-Agent: Mutt/2.0.7 (2021-05-04) Subject: Re: [Tarantool-patches] [PATCH 1/2] fiber: add PoC for fiber creation backtrace X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Cc: Egor2001 , TML , Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Fri, Jun 04, 2021 at 02:13:10PM +0300, Egor Elchinov via Tarantool-patches wrote: > From: Egor2001 > > 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 #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 */