RE: [PATCH 23/31] perf tools: Introduce regs_query_register_offset() for x86
> From: Wang Nan [mailto:wangn...@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.
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: pi3or...@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",
>
Re: [PATCH 23/31] perf tools: Introduce regs_query_register_offset() for x86
On 2015/9/1 19:47, 平松雅巳 / HIRAMATU，MASAMI wrote:
From: Wang Nan [mailto:wangn...@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.
So you want to move it from ./arch/x86/kernel/ptrace.c to arch/x86/lib and
let perf link against arch/x86/lib/lib.a to use it?
I think it worth a specific work to do it. Currently we lake
scaffold to compile and link against the kernel side library. Moreover,
we should also consider other archs. Seems not very easy.
This is not the only one file which can benifite from kernel's arch/x86/lib
Newly introduced tools/perf/util/intel-pt-decoder/insn.c, and I believe
there's
more. Therefore I think it should be a separated work from perf BPF patches.
So how about keep this patch at this time? Or do you have some idea?
Thank you.
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: pi3or...@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_
Re: [PATCH 23/31] perf tools: Introduce regs_query_register_offset() for x86
Em Tue, Sep 01, 2015 at 11:47:41AM +, 平松雅巳 / HIRAMATU，MASAMI escreveu: > > From: Wang Nan [mailto:wangn...@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... - 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: pi3or...@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 > > + *x8
Re: [PATCH 23/31] perf tools: Introduce regs_query_register_offset() for x86
Em Tue, Sep 01, 2015 at 09:52:30PM +0800, Wangnan (F) escreveu: > On 2015/9/1 19:47, 平松雅巳 / HIRAMATU，MASAMI wrote: > >>From: Wang Nan [mailto:wangn...@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. > So you want to move it from ./arch/x86/kernel/ptrace.c to arch/x86/lib and > let perf link against arch/x86/lib/lib.a to use it? > I think it worth a specific work to do it. Currently we lake > scaffold to compile and link against the kernel side library. Moreover, > we should also consider other archs. Seems not very easy. > This is not the only one file which can benifite from kernel's arch/x86/lib > Newly introduced tools/perf/util/intel-pt-decoder/insn.c, and I believe > there's > more. Therefore I think it should be a separated work from perf BPF patches. > So how about keep this patch at this time? Or do you have some idea? I would go with what Wang did at this time, its a step in the right direction in the sense that we're trying to use the same function names and semantics, and, as much as possible, possibly in verbatim form, using the same implementation. Doing the work to fully share it is something being discussed, but that should not get in the way of eBPF work, IMHO. - Arnaldo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 23/31] perf tools: Introduce regs_query_register_offset() for x86
> From: Arnaldo Carvalho de Melo [mailto:a...@redhat.com] > > Em Tue, Sep 01, 2015 at 11:47:41AM +, 平松雅巳 / HIRAMATU，MASAMI escreveu: > > > From: Wang Nan [mailto:wangn...@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. 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: pi3or...@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. > > > +
Re: [PATCH 23/31] perf tools: Introduce regs_query_register_offset() for x86
On 2015/9/1 23:54, 平松雅巳 / HIRAMATU，MASAMI wrote: From: Arnaldo Carvalho de Melo [mailto:a...@redhat.com] Em Tue, Sep 01, 2015 at 11:47:41AM +, 平松雅巳 / HIRAMATU，MASAMI escreveu: From: Wang Nan [mailto:wangn...@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: pi3or...@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 + *
