From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751246AbbIFGDQ (ORCPT ); Sun, 6 Sep 2015 02:03:16 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:44333 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751140AbbIFGDO (ORCPT ); Sun, 6 Sep 2015 02:03:14 -0400 Message-ID: <55EBD70D.9080301@huawei.com> Date: Sun, 6 Sep 2015 14:02:53 +0800 From: "Wangnan (F)" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: =?UTF-8?B?5bmz5p2+6ZuF5bezIC8gSElSQU1BVFXvvIxNQVNBTUk=?= , "'Arnaldo Carvalho de Melo'" CC: "acme@kernel.org" , "linux-kernel@vger.kernel.org" , He Kuang , "Alexei Starovoitov" , Brendan Gregg , Daniel Borkmann , David Ahern , Jiri Olsa , Kaixu Xia , Namhyung Kim , Paul Mackerras , Peter Zijlstra , Zefan Li , "pi3orama@163.com" Subject: Re: [PATCH 23/31] perf tools: Introduce regs_query_register_offset() for x86 References: <1440822125-52691-23-git-send-email-wangnan0@huawei.com> <1441090789-108382-1-git-send-email-wangnan0@huawei.com> <1441090789-108382-2-git-send-email-wangnan0@huawei.com> <50399556C9727B4D88A595C8584AAB37524FF4B0@GSjpTKYDCembx32.service.hitachi.net> <20150901141414.GA22331@redhat.com> <50399556C9727B4D88A595C8584AAB37524FFC71@GSjpTKYDCembx32.service.hitachi.net> In-Reply-To: <50399556C9727B4D88A595C8584AAB37524FFC71@GSjpTKYDCembx32.service.hitachi.net> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.111.66.109] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2015/9/1 23:54, 平松雅巳 / HIRAMATU，MASAMI wrote: >> From: Arnaldo Carvalho de Melo [mailto:acme@redhat.com] >> >> Em Tue, Sep 01, 2015 at 11:47:41AM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu: >>>> From: Wang Nan [mailto:wangnan0@huawei.com] >>>> >>>> regs_query_register_offset() is a helper function which converts >>>> register name like "%rax" to offset of a register in 'struct pt_regs', >>>> which is required by BPF prologue generator. Since the function is >>>> identical, try to reuse the code in arch/x86/kernel/ptrace.c. >>>> >>>> Comment inside dwarf-regs.c list the differences between this >>>> implementation and kernel code. >>> Hmm, this also introduce a duplication of the code... >>> It might be a good time to move them into arch/x86/lib/ and >>> reuse it directly from perf code. >> It is strange to, having tried sharing stuff directly from the kernel, >> to be now in a position where I advocate against it... >> >> Copy'n'pasting what I said in another message: >> >> ----- >> We would go back to sharing stuff with the kernel, but this time around >> we would be using something that everybody knows is being shared, which >> doesn't elliminates the possibility that at some point changes made with >> the kernel in mind would break the tools/ using code. >> >> Perhaps it is better to keep copying what we want and introduce >> infrastructure to check for differences and warn us as soon as possible >> so that we would do the copy, test if it doesn't break what we use, etc. >> >> I.e. we wouldn't be putting any new burden on the "kernel people", i.e. >> the burden of having to check that changes they make don't break tools/ >> living code, nor any out of the blue breakage on tools/ for tools/ >> developers to fix when changes are made on the kernel "side" ----- >> --- >> >> The "stop sharing directly stuff with the kernel" stance was taken after >> a report from Linus about breakage due to tools/ using kernel files >> directly and then a change made in some RCU files broke the tools/perf/ >> build, even with tools/perf/ not using anything RCU related so far. >> >> Looking at tools/perf/MANIFEST, the file used to create a detached >> tarball so that perf can be built outside the kernel sources there are >> still some kernel source files listed, but those probably need to be >> copied too... > OK, so let this apply. > > Acked-by: Masami Hiramatsu > > And also we'll need a testcase for this. I created a testcase for the whole BPF prologue, so I think this can be covered? I'll post them by replying this mail. Thank you. > Thank you, > >> - Arnaldo >> >>> Thank you, >>> >>>> get_arch_regstr() switches to regoffset_table and the old string table >>>> is dropped. >>>> >>>> Signed-off-by: Wang Nan >>>> Signed-off-by: He Kuang >>>> Cc: Alexei Starovoitov >>>> Cc: Brendan Gregg >>>> Cc: Daniel Borkmann >>>> Cc: David Ahern >>>> Cc: He Kuang >>>> Cc: Jiri Olsa >>>> Cc: Kaixu Xia >>>> Cc: Masami Hiramatsu >>>> Cc: Namhyung Kim >>>> Cc: Paul Mackerras >>>> Cc: Peter Zijlstra >>>> Cc: Zefan Li >>>> Cc: pi3orama@163.com >>>> Cc: Arnaldo Carvalho de Melo >>>> --- >>>> tools/perf/arch/x86/Makefile | 1 + >>>> tools/perf/arch/x86/util/Build | 1 + >>>> tools/perf/arch/x86/util/dwarf-regs.c | 122 ++++++++++++++++++++++++---------- >>>> 3 files changed, 90 insertions(+), 34 deletions(-) >>>> >>>> diff --git a/tools/perf/arch/x86/Makefile b/tools/perf/arch/x86/Makefile >>>> index 21322e0..09ba923 100644 >>>> --- a/tools/perf/arch/x86/Makefile >>>> +++ b/tools/perf/arch/x86/Makefile >>>> @@ -2,3 +2,4 @@ ifndef NO_DWARF >>>> PERF_HAVE_DWARF_REGS := 1 >>>> endif >>>> HAVE_KVM_STAT_SUPPORT := 1 >>>> +PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET := 1 >>>> diff --git a/tools/perf/arch/x86/util/Build b/tools/perf/arch/x86/util/Build >>>> index 2c55e1b..d4d1f23 100644 >>>> --- a/tools/perf/arch/x86/util/Build >>>> +++ b/tools/perf/arch/x86/util/Build >>>> @@ -4,6 +4,7 @@ libperf-y += pmu.o >>>> libperf-y += kvm-stat.o >>>> >>>> libperf-$(CONFIG_DWARF) += dwarf-regs.o >>>> +libperf-$(CONFIG_BPF_PROLOGUE) += dwarf-regs.o >>>> >>>> libperf-$(CONFIG_LIBUNWIND) += unwind-libunwind.o >>>> libperf-$(CONFIG_LIBDW_DWARF_UNWIND) += unwind-libdw.o >>>> diff --git a/tools/perf/arch/x86/util/dwarf-regs.c b/tools/perf/arch/x86/util/dwarf-regs.c >>>> index a08de0a..de5b936 100644 >>>> --- a/tools/perf/arch/x86/util/dwarf-regs.c >>>> +++ b/tools/perf/arch/x86/util/dwarf-regs.c >>>> @@ -21,55 +21,109 @@ >>>> */ >>>> >>>> #include >>>> +#include /* for EINVAL */ >>>> +#include /* for strcmp */ >>>> +#include /* for struct pt_regs */ >>>> +#include /* for offsetof */ >>>> #include >>>> >>>> /* >>>> - * Generic dwarf analysis helpers >>>> + * See arch/x86/kernel/ptrace.c. >>>> + * Different from it: >>>> + * >>>> + * - Since struct pt_regs is defined differently for user and kernel, >>>> + * but we want to use 'ax, bx' instead of 'rax, rbx' (which is struct >>>> + * field name of user's pt_regs), we make REG_OFFSET_NAME to accept >>>> + * both string name and reg field name. >>>> + * >>>> + * - Since accessing x86_32's pt_regs from x86_64 building is difficult >>>> + * and vise versa, we simply fill offset with -1, so >>>> + * get_arch_regstr() still works but regs_query_register_offset() >>>> + * returns error. >>>> + * The only inconvenience caused by it now is that we are not allowed >>>> + * to generate BPF prologue for a x86_64 kernel if perf is built for >>>> + * x86_32. This is really a rare usecase. >>>> + * >>>> + * - Order is different from kernel's ptrace.c for get_arch_regstr(), which >>>> + * is defined by dwarf. >>>> */ >>>> >>>> -#define X86_32_MAX_REGS 8 >>>> -const char *x86_32_regs_table[X86_32_MAX_REGS] = { >>>> - "%ax", >>>> - "%cx", >>>> - "%dx", >>>> - "%bx", >>>> - "$stack", /* Stack address instead of %sp */ >>>> - "%bp", >>>> - "%si", >>>> - "%di", >>>> +struct pt_regs_offset { >>>> + const char *name; >>>> + int offset; >>>> +}; >>>> + >>>> +#define REG_OFFSET_END {.name = NULL, .offset = 0} >>>> + >>>> +#ifdef __x86_64__ >>>> +# define REG_OFFSET_NAME_64(n, r) {.name = n, .offset = offsetof(struct pt_regs, r)} >>>> +# define REG_OFFSET_NAME_32(n, r) {.name = n, .offset = -1} >>>> +#else >>>> +# define REG_OFFSET_NAME_64(n, r) {.name = n, .offset = -1} >>>> +# define REG_OFFSET_NAME_32(n, r) {.name = n, .offset = offsetof(struct pt_regs, r)} >>>> +#endif >>>> + >>>> +static const struct pt_regs_offset x86_32_regoffset_table[] = { >>>> + REG_OFFSET_NAME_32("%ax", eax), >>>> + REG_OFFSET_NAME_32("%cx", ecx), >>>> + REG_OFFSET_NAME_32("%dx", edx), >>>> + REG_OFFSET_NAME_32("%bx", ebx), >>>> + REG_OFFSET_NAME_32("$stack", esp), /* Stack address instead of %sp */ >>>> + REG_OFFSET_NAME_32("%bp", ebp), >>>> + REG_OFFSET_NAME_32("%si", esi), >>>> + REG_OFFSET_NAME_32("%di", edi), >>>> + REG_OFFSET_END, >>>> }; >>>> >>>> -#define X86_64_MAX_REGS 16 >>>> -const char *x86_64_regs_table[X86_64_MAX_REGS] = { >>>> - "%ax", >>>> - "%dx", >>>> - "%cx", >>>> - "%bx", >>>> - "%si", >>>> - "%di", >>>> - "%bp", >>>> - "%sp", >>>> - "%r8", >>>> - "%r9", >>>> - "%r10", >>>> - "%r11", >>>> - "%r12", >>>> - "%r13", >>>> - "%r14", >>>> - "%r15", >>>> +static const struct pt_regs_offset x86_64_regoffset_table[] = { >>>> + REG_OFFSET_NAME_64("%ax", rax), >>>> + REG_OFFSET_NAME_64("%dx", rdx), >>>> + REG_OFFSET_NAME_64("%cx", rcx), >>>> + REG_OFFSET_NAME_64("%bx", rbx), >>>> + REG_OFFSET_NAME_64("%si", rsi), >>>> + REG_OFFSET_NAME_64("%di", rdi), >>>> + REG_OFFSET_NAME_64("%bp", rbp), >>>> + REG_OFFSET_NAME_64("%sp", rsp), >>>> + REG_OFFSET_NAME_64("%r8", r8), >>>> + REG_OFFSET_NAME_64("%r9", r9), >>>> + REG_OFFSET_NAME_64("%r10", r10), >>>> + REG_OFFSET_NAME_64("%r11", r11), >>>> + REG_OFFSET_NAME_64("%r12", r12), >>>> + REG_OFFSET_NAME_64("%r13", r13), >>>> + REG_OFFSET_NAME_64("%r14", r14), >>>> + REG_OFFSET_NAME_64("%r15", r15), >>>> + REG_OFFSET_END, >>>> }; >>>> >>>> /* TODO: switching by dwarf address size */ >>>> #ifdef __x86_64__ >>>> -#define ARCH_MAX_REGS X86_64_MAX_REGS >>>> -#define arch_regs_table x86_64_regs_table >>>> +#define regoffset_table x86_64_regoffset_table >>>> #else >>>> -#define ARCH_MAX_REGS X86_32_MAX_REGS >>>> -#define arch_regs_table x86_32_regs_table >>>> +#define regoffset_table x86_32_regoffset_table >>>> #endif >>>> >>>> +/* Minus 1 for the ending REG_OFFSET_END */ >>>> +#define ARCH_MAX_REGS ((sizeof(regoffset_table) / sizeof(regoffset_table[0])) - 1) >>>> + >>>> /* Return architecture dependent register string (for kprobe-tracer) */ >>>> const char *get_arch_regstr(unsigned int n) >>>> { >>>> - return (n < ARCH_MAX_REGS) ? arch_regs_table[n] : NULL; >>>> + return (n < ARCH_MAX_REGS) ? regoffset_table[n].name : NULL; >>>> +} >>>> + >>>> +/* Reuse code from arch/x86/kernel/ptrace.c */ >>>> +/** >>>> + * regs_query_register_offset() - query register offset from its name >>>> + * @name: the name of a register >>>> + * >>>> + * regs_query_register_offset() returns the offset of a register in struct >>>> + * pt_regs from its name. If the name is invalid, this returns -EINVAL; >>>> + */ >>>> +int regs_query_register_offset(const char *name) >>>> +{ >>>> + const struct pt_regs_offset *roff; >>>> + for (roff = regoffset_table; roff->name != NULL; roff++) >>>> + if (!strcmp(roff->name, name)) >>>> + return roff->offset; >>>> + return -EINVAL; >>>> } >>>> -- >>>> 1.8.3.4