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 75DE56EC40; Mon, 7 Jun 2021 19:39:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 75DE56EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1623083988; bh=MMlHUckMOa6KKIMe4Sp68bxEvPaYZpR/YLbHtprfwjA=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=xScYXXFD5/SlTF+efDPF63UIjeFtIAYmWV8ze51evLHotjIQcksWN3glQycQwecpF AtjiEgvtC/s2g6FUsvu2IIRCzMFKCE5/WErh3Zah7XhfMFBcQggGoiy7zRwI5F/2zj spLeoTwgcOfLHIUCg12VOpnGMl1s+3OUTmC/IFto= Received: from smtp30.i.mail.ru (smtp30.i.mail.ru [94.100.177.90]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1D8436EC40 for ; Mon, 7 Jun 2021 19:39:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1D8436EC40 Received: by smtp30.i.mail.ru with esmtpa (envelope-from ) id 1lqIHx-0005BQ-U3; Mon, 07 Jun 2021 19:39:46 +0300 To: Cyrill Gorcunov References: <8894403bf01395ae2b3082b005ff48178cde3081.1622792861.git.elchinov.es@gmail.com> Message-ID: Date: Mon, 7 Jun 2021 19:39:45 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------00916B424FB37CC17053FBC8" Content-Language: en-US X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D5B0DA836B685C5423CDB5763716BB867A249A0077715520182A05F538085040354926288DB1776B264421C789C07B8961A1D45408611C89DDAC7711D2E48BC9 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7FDE19FEC90BA7BD7C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE73230F712CF4B3924EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BD6CF32B5F8F9D4045401B88B7F9FB86E8B11D094089A32B6CC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8B861051D4BA689FCF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C0072D29C69FDE18F9735652A29929C6C4AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C3FF6525F957CDD8C6BA3038C0950A5D36B5C8C57E37DE458B0BC6067A898B09E46D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7ED6B002596C5A4F0731C566533BA786AA5CC5B56E945C8DA X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C978314FF4EB640BE80CF7C527CFD643BC0A42 X-C1DE0DAB: 0D63561A33F958A56C8D9464372E8B04E04F8E6DB2B6825E36D89D138D499779D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75FBC5FED0552DA851410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34806D3522FB05EB39BA7AEBB58967352356F90D6A2C80B3279D2C96E0FB571BC9658062E5A4B04F3E1D7E09C32AA3244C820B3D2A7B519FEEFD2D0DABC1E6B1ED6C248321276684228D5DD81C2BAB7D1D X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj/099YK7Sav2U+MwcVtSxMA== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A76903EBF70B0024981A264421C789C07B89F7F5837242FA16B258570E9BDA2331C06F53C80213D1719C2C26F88BABE1618CA23003C376F5F1387402F9BA4338D657ED14614B50AE0675 X-Mras: Ok 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: Egor Elchinov via Tarantool-patches Reply-To: Egor Elchinov Cc: Egor2001 , tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------00916B424FB37CC17053FBC8 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Hi! Thanks for the review! On 6/7/21 11:28 AM, Cyrill Gorcunov wrote: > 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. Thanks, fixed. > 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? Good point, I've consulted with Alexander Lyapunov and it was decided to add a dynamic setting enabling this feature. >> +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. Thanks, fixed. >> + int frame_no = 0; > This is shor routine and I think plain `n` would be more than enough. For now that's true, but `frame_no` name was chosen mostly for consistency with the present `backtrace` routines. Also, routine will be more complicated after addition of DARWIN `#ifdef` and demangling support. >> + 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? It's ok, because if `proc` gets non-NULL value then `acc->get_proc_name` routine is present in the system and this part of code will never be called. Otherwise `proc` can't be retrieved by the libunwind API and has to be NULL. >> + } 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. Thanks a lot for your diff! Also I haven't decided yet how to grab Lua stacks of fiber creation in a cheap way. I would be glad for any suggestions on this issue too. --------------00916B424FB37CC17053FBC8 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit 
Hi! Thanks for the review!
On 6/7/21 11:28 AM, Cyrill Gorcunov wrote:
On Fri, Jun 04, 2021 at 02:13:10PM +0300, Egor Elchinov via Tarantool-patches wrote:

From: Egor2001 <elchinov.es@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.

Thanks, fixed.

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?

Good point, I've consulted with Alexander Lyapunov and 
it was decided to add a dynamic setting enabling this feature.


      

        
+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.


    

    
Thanks, fixed.


    

      

      

        
+	int frame_no = 0;


      

      
This is shor routine and I think plain `n` would be more than enough.


    

    
For now that's true, but `frame_no` name was chosen mostly
for consistency with the present `backtrace` routines. 
Also, routine will be more complicated after addition of
DARWIN `#ifdef` and demangling support.


    

      

        
+	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?


    

    
It's ok, because if `proc` gets non-NULL value then
`acc->get_proc_name` routine is present in the system and
this part of code will never be called.
Otherwise `proc` can't be retrieved by the
libunwind API and has to be NULL.


    

      

        
+		} 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.


    

    
Thanks a lot for your diff!
Also I haven't decided yet how to grab Lua stacks of 
fiber creation in a cheap way.
I would be glad for any suggestions on this issue too.


  


--------------00916B424FB37CC17053FBC8--