From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932331AbcE0HST (ORCPT ); Fri, 27 May 2016 03:18:19 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:4606 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755273AbcE0HSS (ORCPT ); Fri, 27 May 2016 03:18:18 -0400 Subject: Re: [PATCH v5 3/5] perf callchain: Add support for cross-platform unwind To: Jiri Olsa References: <1464081629-137191-1-git-send-email-hekuang@huawei.com> <1464081629-137191-4-git-send-email-hekuang@huawei.com> <20160526174255.GA11246@krava> CC: , , , , , , , , , , , , , , , , From: Hekuang Message-ID: <5747F380.4060107@huawei.com> Date: Fri, 27 May 2016 15:13:04 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160526174255.GA11246@krava> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.110.55.166] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090204.5747F39B.0078,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 1becde9cc0972388007593ead09a5a45 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org hi 在 2016/5/27 1:42, Jiri Olsa 写道: > On Tue, May 24, 2016 at 09:20:27AM +0000, He Kuang wrote: >> Use thread specific unwind ops to unwind cross-platform callchains. >> >> Currently, unwind methods is suitable for local unwind, this patch >> changes the fixed methods to thread/map related. Each time a map is >> inserted, we find the target arch and see if this platform can be >> remote unwind. We test for x86 platform and only show proper >> messages. The real unwind methods are not implemented, will be >> introduced in next patch. >> >> CONFIG_LIBUNWIND/NO_LIBUNWIND are changed to >> CONFIG_LOCAL_LIBUNWIND/NO_LOCAL_LIBUNWIND for retaining local unwind >> features. CONFIG_LIBUNWIND stands for either local or remote or both >> unwind are supported and NO_LIBUNWIND means neither local nor remote >> libunwind are supported. > hi, > I think this is too complex and error prone, I'd rather see it split > to several pieces. Basically every logicaly single piece should be > in separate patch for better readablebility and review. > > I might be missing some but I'd mainly sepatate following: > > - introducing struct unwind_libunwind_ops for local unwind > - moving unwind__prepare_access from thread_new into thread__insert_map > - keep unwind__prepare_access name instead of unwind__get_arch > and keep the return value, we need to bail out in case of error > - I wouldn't use null ops.. just check for for ops == NULL in wrapper function OK > - I understand we need to compile 3 objects from unwind-libunwind.c, > how about we create 3 files like: > > util/unwind-libunwind-local.c > util/unwind-libunwind-x86_32.c > util/unwind-libunwind-arm64.c > > which would setup all necessary defines and include unwind-libunwind.c like: > > --- > /* comments explaining every define ;-) */ > ... > #define LOCAL... REMOTE.. > ... > #include > ... > ---- > > this way we will keep all the special setup for given unwind object > in one place and you can also use simple rule in the Build file like > without defining special rule: > > libperf-$(CONFIG_LIBUNWIND_X86) += unwind-libunwind_x86_32.o > libperf-$(CONFIG_LIBUNWIND_AARCH64) += unwind-libunwind_arm64.o > > the same way for the arch object: > > arch/x86/util/unwind-libunwind-local.c > arch/x86/util/unwind-libunwind-x86_32.c > > > Not sure I thought everything through, but I think this way > we'll keep it more maintainable and readable.. > > let me know what you think The only concern is that, if later we support more platforms, there will be too much files named as 'tools/perf/util/unwind-libunwind*.c' Is it acceptable or not? And I thought all files belongs to specific archs should go to folder under 'tools/perf/arch/xxx', is that right? Thanks. > thanks, > jirka >