linux-kernel - Re: [PATCH v5 3/5] perf callchain: Add support for cross-platform unwind

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5747F380.4060107@huawei.com>
Date:	Fri, 27 May 2016 15:13:04 +0800
From:	Hekuang <hekuang@...wei.com>
To:	Jiri Olsa <jolsa@...hat.com>
CC:	<peterz@...radead.org>, <mingo@...hat.com>, <acme@...nel.org>,
	<alexander.shishkin@...ux.intel.com>, <wangnan0@...wei.com>,
	<jpoimboe@...hat.com>, <ak@...ux.intel.com>, <eranian@...gle.com>,
	<namhyung@...nel.org>, <adrian.hunter@...el.com>,
	<sukadev@...ux.vnet.ibm.com>, <masami.hiramatsu.pt@...achi.com>,
	<tumanova@...ux.vnet.ibm.com>, <kan.liang@...el.com>,
	<penberg@...nel.org>, <dsahern@...il.com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 3/5] perf callchain: Add support for cross-platform
 unwind

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 <util/unwind-libunwind-local.c>
>      ...
>      ----
>
>      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
>