RE: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions
> From: Ingo Molnar [mailto:mingo.kernel@gmail.com] On Behalf Of Ingo Molnar > > > > sure, np you can use my ack > > > > I'm also OK for this patch. I just concern that is OK for Adrian too? > > Since this ensures all the copied code should be dead copy (not modified > > anymore), > > if we want a different instruction decoding routine, we have to break the > > test > > anyway. > > So the idea would be to not break anything, only warn in a non-fatal question. > This protects against unbisectable universes being created via simple git > merges > where updates meet but testing of tooling isn't done. I see, so I give my ack :) Acked-by: Masami Hiramatsu Thank you! > > Thanks, > > Ingo
Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions
* 平松雅巳 / HIRAMATU,MASAMI wrote: > > sure, np you can use my ack > > I'm also OK for this patch. I just concern that is OK for Adrian too? > Since this ensures all the copied code should be dead copy (not modified > anymore), > if we want a different instruction decoding routine, we have to break the test > anyway. So the idea would be to not break anything, only warn in a non-fatal question. This protects against unbisectable universes being created via simple git merges where updates meet but testing of tooling isn't done. Thanks, Ingo -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
> From: Jiri Olsa [mailto:jo...@redhat.com] > > On Tue, Sep 01, 2015 at 04:57:16PM -0300, Arnaldo Carvalho de Melo wrote: > > Em Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter escreveu: > > > On 01/09/15 11:54, Ingo Molnar wrote: > > > > > > > > * Adrian Hunter wrote: > > > > > > > >> Hi > > > >> > > > >> perf tools has a copy of the x86 instruction decoder for decoding > > > >> Intel PT. [...] > > > > > > > > So that's the arch/x86/lib/insn.c instruction length decoder that the > > > > kernel uses > > > > for kprobes et al - and the two versions already forked slightly: > > > > > > > > -#include "inat.h" > > > > -#include "insn.h" > > > > +#include > > > > +#include > > > > > > > > it would be nice to add a diff check to the perf build, and > > > > (non-fatally) warn > > > > during the build if the two versions depart from each other? > > > > > > I had a go and came up with this. Arnaldo, Jiri any comments? > > > > So, I was going to try and merge this series, can you please collect the > > Acks by Masami and Jiri and resubmit? > > > > I'd say no need to stop this just to get a build function to use with > > this, the test below should do the trick _for this specific instance_ > > and then, after we get this, you should use it as the initial usecase > > for adding the build function, d'accord? > > > > Jiri, are you ok with this? > > sure, np you can use my ack I'm also OK for this patch. I just concern that is OK for Adrian too? Since this ensures all the copied code should be dead copy (not modified anymore), if we want a different instruction decoding routine, we have to break the test anyway. Thank you, > > jirka > > > > > - Arnaldo > > > > > diff --git a/tools/perf/util/intel-pt-decoder/Build > > > b/tools/perf/util/intel-pt-decoder/Build > > > index 240730d682c1..1b8a32de8504 100644 > > > --- a/tools/perf/util/intel-pt-decoder/Build > > > +++ b/tools/perf/util/intel-pt-decoder/Build > > > @@ -6,6 +6,17 @@ inat_tables_maps = > > > util/intel-pt-decoder/x86-opcode-map.txt > > > $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) > > > $(inat_tables_maps) > > > @$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) > > > $(inat_tables_maps) > $@ || rm -f $@ > > > > > > -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: > > > util/intel-pt-decoder/inat.c > $(OUTPUT)util/intel-pt-decoder/inat-tables.c > > > +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: > > > util/intel-pt-decoder/intel-pt-insn-decoder.c > util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c > > > + @test -d ../../arch/x86 && (( \ > > > + diff -B -I'^#include' util/intel-pt-decoder/insn.c > > > ../../arch/x86/lib/insn.c >/dev/null && \ > > > + diff -B -I'^#include' util/intel-pt-decoder/inat.c > > > ../../arch/x86/lib/inat.c >/dev/null && \ > > > + diff -B util/intel-pt-decoder/x86-opcode-map.txt > > > ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \ > > > + diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk > > > ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \ > > > + diff -B -I'^#include' util/intel-pt-decoder/insn.h > > > ../../arch/x86/include/asm/insn.h >/dev/null && \ > > > + diff -B -I'^#include' util/intel-pt-decoder/inat.h > > > ../../arch/x86/include/asm/inat.h >/dev/null && \ > > > + diff -B -I'^#include' util/intel-pt-decoder/inat_types.h > > > ../../arch/x86/include/asm/inat_types.h >/dev/null) \ > > > + || echo "Warning: Intel PT: x86 instruction decoder differs from > > > kernel" >&2 ) > > > + $(call rule_mkdir) > > > + $(call if_changed_dep,cc_o_c) > > > > > > CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder > > > -Wno-override-init > > > -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
On Tue, Sep 01, 2015 at 04:57:16PM -0300, Arnaldo Carvalho de Melo wrote: > Em Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter escreveu: > > On 01/09/15 11:54, Ingo Molnar wrote: > > > > > > * Adrian Hunter wrote: > > > > > >> Hi > > >> > > >> perf tools has a copy of the x86 instruction decoder for decoding > > >> Intel PT. [...] > > > > > > So that's the arch/x86/lib/insn.c instruction length decoder that the > > > kernel uses > > > for kprobes et al - and the two versions already forked slightly: > > > > > > -#include "inat.h" > > > -#include "insn.h" > > > +#include > > > +#include > > > > > > it would be nice to add a diff check to the perf build, and (non-fatally) > > > warn > > > during the build if the two versions depart from each other? > > > > I had a go and came up with this. Arnaldo, Jiri any comments? > > So, I was going to try and merge this series, can you please collect the > Acks by Masami and Jiri and resubmit? > > I'd say no need to stop this just to get a build function to use with > this, the test below should do the trick _for this specific instance_ > and then, after we get this, you should use it as the initial usecase > for adding the build function, d'accord? > > Jiri, are you ok with this? sure, np you can use my ack jirka > > - Arnaldo > > > diff --git a/tools/perf/util/intel-pt-decoder/Build > > b/tools/perf/util/intel-pt-decoder/Build > > index 240730d682c1..1b8a32de8504 100644 > > --- a/tools/perf/util/intel-pt-decoder/Build > > +++ b/tools/perf/util/intel-pt-decoder/Build > > @@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt > > $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) > > $(inat_tables_maps) > > @$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) > > $(inat_tables_maps) > $@ || rm -f $@ > > > > -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: > > util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c > > +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: > > util/intel-pt-decoder/intel-pt-insn-decoder.c util/intel-pt-decoder/inat.c > > $(OUTPUT)util/intel-pt-decoder/inat-tables.c > > + @test -d ../../arch/x86 && (( \ > > + diff -B -I'^#include' util/intel-pt-decoder/insn.c > > ../../arch/x86/lib/insn.c >/dev/null && \ > > + diff -B -I'^#include' util/intel-pt-decoder/inat.c > > ../../arch/x86/lib/inat.c >/dev/null && \ > > + diff -B util/intel-pt-decoder/x86-opcode-map.txt > > ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \ > > + diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk > > ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \ > > + diff -B -I'^#include' util/intel-pt-decoder/insn.h > > ../../arch/x86/include/asm/insn.h >/dev/null && \ > > + diff -B -I'^#include' util/intel-pt-decoder/inat.h > > ../../arch/x86/include/asm/inat.h >/dev/null && \ > > + diff -B -I'^#include' util/intel-pt-decoder/inat_types.h > > ../../arch/x86/include/asm/inat_types.h >/dev/null) \ > > + || echo "Warning: Intel PT: x86 instruction decoder differs from > > kernel" >&2 ) > > + $(call rule_mkdir) > > + $(call if_changed_dep,cc_o_c) > > > > CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder > > -Wno-override-init > > -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
Em Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter escreveu: > On 01/09/15 11:54, Ingo Molnar wrote: > > > > * Adrian Hunter wrote: > > > >> Hi > >> > >> perf tools has a copy of the x86 instruction decoder for decoding > >> Intel PT. [...] > > > > So that's the arch/x86/lib/insn.c instruction length decoder that the > > kernel uses > > for kprobes et al - and the two versions already forked slightly: > > > > -#include "inat.h" > > -#include "insn.h" > > +#include > > +#include > > > > it would be nice to add a diff check to the perf build, and (non-fatally) > > warn > > during the build if the two versions depart from each other? > > I had a go and came up with this. Arnaldo, Jiri any comments? So, I was going to try and merge this series, can you please collect the Acks by Masami and Jiri and resubmit? I'd say no need to stop this just to get a build function to use with this, the test below should do the trick _for this specific instance_ and then, after we get this, you should use it as the initial usecase for adding the build function, d'accord? Jiri, are you ok with this? - Arnaldo > diff --git a/tools/perf/util/intel-pt-decoder/Build > b/tools/perf/util/intel-pt-decoder/Build > index 240730d682c1..1b8a32de8504 100644 > --- a/tools/perf/util/intel-pt-decoder/Build > +++ b/tools/perf/util/intel-pt-decoder/Build > @@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt > $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) > $(inat_tables_maps) > @$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) > $(inat_tables_maps) > $@ || rm -f $@ > > -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: > util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c > +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: > util/intel-pt-decoder/intel-pt-insn-decoder.c util/intel-pt-decoder/inat.c > $(OUTPUT)util/intel-pt-decoder/inat-tables.c > + @test -d ../../arch/x86 && (( \ > + diff -B -I'^#include' util/intel-pt-decoder/insn.c > ../../arch/x86/lib/insn.c >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/inat.c > ../../arch/x86/lib/inat.c >/dev/null && \ > + diff -B util/intel-pt-decoder/x86-opcode-map.txt > ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \ > + diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk > ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/insn.h > ../../arch/x86/include/asm/insn.h >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/inat.h > ../../arch/x86/include/asm/inat.h >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/inat_types.h > ../../arch/x86/include/asm/inat_types.h >/dev/null) \ > + || echo "Warning: Intel PT: x86 instruction decoder differs from > kernel" >&2 ) > + $(call rule_mkdir) > + $(call if_changed_dep,cc_o_c) > > CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder > -Wno-override-init > -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
> From: Adrian Hunter [mailto:adrian.hun...@intel.com] > > On 01/09/15 14:38, 平松雅巳 / HIRAMATU,MASAMI wrote: > >> From: Ingo Molnar [mailto:mingo.kernel@gmail.com] On Behalf Of Ingo > >> Molnar > >> > >> > >> * Adrian Hunter wrote: > >> > >>> Hi > >>> > >>> perf tools has a copy of the x86 instruction decoder for decoding > >>> Intel PT. [...] > >> > >> So that's the arch/x86/lib/insn.c instruction length decoder that the > >> kernel uses > >> for kprobes et al - and the two versions already forked slightly: > >> > >> -#include "inat.h" > >> -#include "insn.h" > >> +#include > >> +#include > >> > >> it would be nice to add a diff check to the perf build, and (non-fatally) > >> warn > >> during the build if the two versions depart from each other? > >> > >> This will make sure the two versions are fully in sync in the long run as > >> well. > >> > >> ( Alternatively we could perhaps also librarize it into tools/lib/, and > >> teach the > >> kernel build to pick that one up? ) > > > > Agreed, what I concern is that someone finds a bug and fixes one of them and > > another is not fixed. > > > > I'll see the forked version and check if it can be merged into the kernel. > > Ever since Linus complained about perf tools including kernel headers, I > have assumed we should have separate source code. That email thread was not > cc'ed to a mailing list but here is a quote: > > Em Sat, Jul 04, 2015 at 08:53:46AM -0700, Linus Torvalds escreveu: > > So this is more fundamental, and looks like it's just due to perf > > abusing the kernel headers, and now that rbtree has rcu support > > ("rbtree: Make lockless searches non-fatal"), it gets tons of headers > > included that really don't work from user space. > > > > There might be other things going on, but the rbtree one seems to be a > > big one. I think perf needs to get its own rbtree header or something, > > instead of doing that insane "let's include random core kernel > > headers" thing. OK, now I see what happened... Hmm, so at this point, I'll just port the test to arch/x86/tools/, since the kernel should have that. Thanks, -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
Em Tue, Sep 01, 2015 at 03:59:16PM +0200, Jiri Olsa escreveu: > On Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter wrote: > > On 01/09/15 11:54, Ingo Molnar wrote: > > > it would be nice to add a diff check to the perf build, and (non-fatally) > > > warn > > > during the build if the two versions depart from each other? > > I had a go and came up with this. Arnaldo, Jiri any comments? > > diff --git a/tools/perf/util/intel-pt-decoder/Build > > b/tools/perf/util/intel-pt-decoder/Build > > + diff -B -I'^#include' util/intel-pt-decoder/insn.h > > ../../arch/x86/include/asm/insn.h >/dev/null && \ > > + diff -B -I'^#include' util/intel-pt-decoder/inat.h > > ../../arch/x86/include/asm/inat.h >/dev/null && \ > > + diff -B -I'^#include' util/intel-pt-decoder/inat_types.h > > ../../arch/x86/include/asm/inat_types.h >/dev/null) \ > > + || echo "Warning: Intel PT: x86 instruction decoder differs from > > kernel" >&2 ) > > + $(call rule_mkdir) > > + $(call if_changed_dep,cc_o_c) > seems ok, but it might be nicer to have make function for that > so we could use it on other places like rbtree.h That will pose some more hurdles, as there are things like EXPORT_SYMBOL() and RCU stuff that are ok in the kernel sources, but not in the tools/ copy... I.e. fully sharing will put a new burden for kernel developers working on the to-be-shared code, which is something that is not desired. I was ok with, hey, tools/ broke because you're sharing code with the kernel, as probably a tools/ developer would notice that and fix things, but Linus advised against that. - 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
On Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter wrote: > On 01/09/15 11:54, Ingo Molnar wrote: > > > > * Adrian Hunter wrote: > > > >> Hi > >> > >> perf tools has a copy of the x86 instruction decoder for decoding > >> Intel PT. [...] > > > > So that's the arch/x86/lib/insn.c instruction length decoder that the > > kernel uses > > for kprobes et al - and the two versions already forked slightly: > > > > -#include "inat.h" > > -#include "insn.h" > > +#include > > +#include > > > > it would be nice to add a diff check to the perf build, and (non-fatally) > > warn > > during the build if the two versions depart from each other? > > I had a go and came up with this. Arnaldo, Jiri any comments? > > diff --git a/tools/perf/util/intel-pt-decoder/Build > b/tools/perf/util/intel-pt-decoder/Build > index 240730d682c1..1b8a32de8504 100644 > --- a/tools/perf/util/intel-pt-decoder/Build > +++ b/tools/perf/util/intel-pt-decoder/Build > @@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt > $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) > $(inat_tables_maps) > @$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) > $(inat_tables_maps) > $@ || rm -f $@ > > -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: > util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c > +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: > util/intel-pt-decoder/intel-pt-insn-decoder.c util/intel-pt-decoder/inat.c > $(OUTPUT)util/intel-pt-decoder/inat-tables.c > + @test -d ../../arch/x86 && (( \ > + diff -B -I'^#include' util/intel-pt-decoder/insn.c > ../../arch/x86/lib/insn.c >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/inat.c > ../../arch/x86/lib/inat.c >/dev/null && \ > + diff -B util/intel-pt-decoder/x86-opcode-map.txt > ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \ > + diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk > ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/insn.h > ../../arch/x86/include/asm/insn.h >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/inat.h > ../../arch/x86/include/asm/inat.h >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/inat_types.h > ../../arch/x86/include/asm/inat_types.h >/dev/null) \ > + || echo "Warning: Intel PT: x86 instruction decoder differs from > kernel" >&2 ) > + $(call rule_mkdir) > + $(call if_changed_dep,cc_o_c) > seems ok, but it might be nicer to have make function for that so we could use it on other places like rbtree.h jirka -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
Em Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter escreveu: > On 01/09/15 11:54, Ingo Molnar wrote: > > * Adrian Hunter wrote: > >> perf tools has a copy of the x86 instruction decoder for decoding > >> Intel PT. [...] > > So that's the arch/x86/lib/insn.c instruction length decoder that the > > kernel uses > > for kprobes et al - and the two versions already forked slightly: > > -#include "inat.h" > > -#include "insn.h" > > +#include > > +#include > > it would be nice to add a diff check to the perf build, and (non-fatally) > > warn > > during the build if the two versions depart from each other? > I had a go and came up with this. Arnaldo, Jiri any comments? It looks ok, but then, if the people doing the original work, i.e. Masami, IIRC, manages to make these files something shared, then this becomes moot, right? 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 and 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 changed they made doesn't break tools/ living code, nor any out of the blue breakage on tools/ developers when changes are made on the kernel "side". I.e. have something like what you did, but not limited to these intel-pt decoder bits, we share more than that :-) So, I would apply your patch and move forward, at least these intel-pt bits would be covered, Ingo? - Arnaldo > diff --git a/tools/perf/util/intel-pt-decoder/Build > b/tools/perf/util/intel-pt-decoder/Build > index 240730d682c1..1b8a32de8504 100644 > --- a/tools/perf/util/intel-pt-decoder/Build > +++ b/tools/perf/util/intel-pt-decoder/Build > @@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt > $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) > $(inat_tables_maps) > @$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) > $(inat_tables_maps) > $@ || rm -f $@ > > -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: > util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c > +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: > util/intel-pt-decoder/intel-pt-insn-decoder.c util/intel-pt-decoder/inat.c > $(OUTPUT)util/intel-pt-decoder/inat-tables.c > + @test -d ../../arch/x86 && (( \ > + diff -B -I'^#include' util/intel-pt-decoder/insn.c > ../../arch/x86/lib/insn.c >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/inat.c > ../../arch/x86/lib/inat.c >/dev/null && \ > + diff -B util/intel-pt-decoder/x86-opcode-map.txt > ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \ > + diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk > ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/insn.h > ../../arch/x86/include/asm/insn.h >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/inat.h > ../../arch/x86/include/asm/inat.h >/dev/null && \ > + diff -B -I'^#include' util/intel-pt-decoder/inat_types.h > ../../arch/x86/include/asm/inat_types.h >/dev/null) \ > + || echo "Warning: Intel PT: x86 instruction decoder differs from > kernel" >&2 ) > + $(call rule_mkdir) > + $(call if_changed_dep,cc_o_c) > > CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder > -Wno-override-init > -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
* Adrian Hunter wrote: > > Agreed, what I concern is that someone finds a bug and fixes one of them > > and > > another is not fixed. > > > > I'll see the forked version and check if it can be merged into the kernel. > > Ever since Linus complained about perf tools including kernel headers, I have > assumed we should have separate source code. That email thread was not cc'ed > to > a mailing list but here is a quote: > > Em Sat, Jul 04, 2015 at 08:53:46AM -0700, Linus Torvalds escreveu: > > > So this is more fundamental, and looks like it's just due to perf abusing > > the > > kernel headers, and now that rbtree has rcu support ("rbtree: Make lockless > > searches non-fatal"), it gets tons of headers included that really don't > > work > > from user space. > > > > There might be other things going on, but the rbtree one seems to be a big > > one. I think perf needs to get its own rbtree header or something, instead > > of > > doing that insane "let's include random core kernel headers" thing. Note that even plain copying and occasional back-merges isn't a bad solution: it's better than 'messy sharing' of code. But we can also share code in a bit more organized fashion, and any of the two solutions I proposed solve these complications: - if we do the diff -u check warning during perf build then the forked versions won't stay forked for long. This is the simplest variant. - if we librarize this functionality into tools/lib/x86/decode/ (and make sure it's a library that can be linked into the kernel) then we are back to shared code. The problem wasn't to share code per se, the problem was to share code in a messy way, without making it apparent that it's shared code: which made it easy to break the tools/perf build via harmless looking kernel side changes. Thanks, Ingo -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
On 01/09/15 11:54, Ingo Molnar wrote: > > * Adrian Hunter wrote: > >> Hi >> >> perf tools has a copy of the x86 instruction decoder for decoding >> Intel PT. [...] > > So that's the arch/x86/lib/insn.c instruction length decoder that the kernel > uses > for kprobes et al - and the two versions already forked slightly: > > -#include "inat.h" > -#include "insn.h" > +#include > +#include > > it would be nice to add a diff check to the perf build, and (non-fatally) > warn > during the build if the two versions depart from each other? I had a go and came up with this. Arnaldo, Jiri any comments? diff --git a/tools/perf/util/intel-pt-decoder/Build b/tools/perf/util/intel-pt-decoder/Build index 240730d682c1..1b8a32de8504 100644 --- a/tools/perf/util/intel-pt-decoder/Build +++ b/tools/perf/util/intel-pt-decoder/Build @@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) $(inat_tables_maps) @$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@ || rm -f $@ -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/intel-pt-insn-decoder.c util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c + @test -d ../../arch/x86 && (( \ + diff -B -I'^#include' util/intel-pt-decoder/insn.c ../../arch/x86/lib/insn.c >/dev/null && \ + diff -B -I'^#include' util/intel-pt-decoder/inat.c ../../arch/x86/lib/inat.c >/dev/null && \ + diff -B util/intel-pt-decoder/x86-opcode-map.txt ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \ + diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \ + diff -B -I'^#include' util/intel-pt-decoder/insn.h ../../arch/x86/include/asm/insn.h >/dev/null && \ + diff -B -I'^#include' util/intel-pt-decoder/inat.h ../../arch/x86/include/asm/inat.h >/dev/null && \ + diff -B -I'^#include' util/intel-pt-decoder/inat_types.h ../../arch/x86/include/asm/inat_types.h >/dev/null) \ + || echo "Warning: Intel PT: x86 instruction decoder differs from kernel" >&2 ) + $(call rule_mkdir) + $(call if_changed_dep,cc_o_c) CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder -Wno-override-init -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
On 01/09/15 14:38, 平松雅巳 / HIRAMATU,MASAMI wrote: >> From: Ingo Molnar [mailto:mingo.kernel@gmail.com] On Behalf Of Ingo >> Molnar >> >> >> * Adrian Hunter wrote: >> >>> Hi >>> >>> perf tools has a copy of the x86 instruction decoder for decoding >>> Intel PT. [...] >> >> So that's the arch/x86/lib/insn.c instruction length decoder that the kernel >> uses >> for kprobes et al - and the two versions already forked slightly: >> >> -#include "inat.h" >> -#include "insn.h" >> +#include >> +#include >> >> it would be nice to add a diff check to the perf build, and (non-fatally) >> warn >> during the build if the two versions depart from each other? >> >> This will make sure the two versions are fully in sync in the long run as >> well. >> >> ( Alternatively we could perhaps also librarize it into tools/lib/, and >> teach the >> kernel build to pick that one up? ) > > Agreed, what I concern is that someone finds a bug and fixes one of them and > another is not fixed. > > I'll see the forked version and check if it can be merged into the kernel. Ever since Linus complained about perf tools including kernel headers, I have assumed we should have separate source code. That email thread was not cc'ed to a mailing list but here is a quote: Em Sat, Jul 04, 2015 at 08:53:46AM -0700, Linus Torvalds escreveu: > So this is more fundamental, and looks like it's just due to perf > abusing the kernel headers, and now that rbtree has rcu support > ("rbtree: Make lockless searches non-fatal"), it gets tons of headers > included that really don't work from user space. > > There might be other things going on, but the rbtree one seems to be a > big one. I think perf needs to get its own rbtree header or something, > instead of doing that insane "let's include random core kernel > headers" thing. -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
> From: Ingo Molnar [mailto:mingo.kernel@gmail.com] On Behalf Of Ingo Molnar > > > * Adrian Hunter wrote: > > > Hi > > > > perf tools has a copy of the x86 instruction decoder for decoding > > Intel PT. [...] > > So that's the arch/x86/lib/insn.c instruction length decoder that the kernel > uses > for kprobes et al - and the two versions already forked slightly: > > -#include "inat.h" > -#include "insn.h" > +#include > +#include > > it would be nice to add a diff check to the perf build, and (non-fatally) warn > during the build if the two versions depart from each other? > > This will make sure the two versions are fully in sync in the long run as > well. > > ( Alternatively we could perhaps also librarize it into tools/lib/, and teach > the > kernel build to pick that one up? ) Agreed, what I concern is that someone finds a bug and fixes one of them and another is not fixed. I'll see the forked version and check if it can be merged into the kernel. Thank you, > > Thanks, > > Ingo -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
* Adrian Hunter wrote: > Hi > > perf tools has a copy of the x86 instruction decoder for decoding > Intel PT. [...] So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses for kprobes et al - and the two versions already forked slightly: -#include "inat.h" -#include "insn.h" +#include +#include it would be nice to add a diff check to the perf build, and (non-fatally) warn during the build if the two versions depart from each other? This will make sure the two versions are fully in sync in the long run as well. ( Alternatively we could perhaps also librarize it into tools/lib/, and teach the kernel build to pick that one up? ) Thanks, Ingo -- 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 0/4] x86/insn: perf tools: Add a few new x86 instructions
Em Mon, Aug 31, 2015 at 04:58:38PM +0300, Adrian Hunter escreveu: > Hi > > perf tools has a copy of the x86 instruction decoder for decoding > Intel PT. This patch set adds a perf tools test to use it to > test new instructions. Subsequent patches add a few new x86 > instructions, or very slightly modify them in the case of MPX. > Those changes affect both perf tools and x86/insn. > > I suggest Arnaldo takes all these patches as they mainly affect > perf tools, at least in terms of lines-of-code. I'll process them, anyone thinking this shouldn't be the case, holler. - Arnaldo > > Adrian Hunter (4): > perf tools: Add a test for decoding of new x86 instructions > x86/insn: perf tools: Pedantically tweak opcode map for MPX instructions > x86/insn: perf tools: Add new SHA instructions > x86/insn: perf tools: Add new memory instructions > > arch/x86/lib/x86-opcode-map.txt| 19 +- > tools/perf/tests/Build | 3 + > tools/perf/tests/builtin-test.c| 8 + > tools/perf/tests/gen-insn-x86-dat.awk | 75 ++ > tools/perf/tests/gen-insn-x86-dat.sh | 43 ++ > tools/perf/tests/insn-x86-dat-32.c | 640 > tools/perf/tests/insn-x86-dat-64.c | 738 ++ > tools/perf/tests/insn-x86-dat-src.c| 835 > + > tools/perf/tests/insn-x86.c| 180 + > tools/perf/tests/tests.h | 1 + > .../perf/util/intel-pt-decoder/x86-opcode-map.txt | 19 +- > 11 files changed, 2553 insertions(+), 8 deletions(-) > create mode 100644 tools/perf/tests/gen-insn-x86-dat.awk > create mode 100755 tools/perf/tests/gen-insn-x86-dat.sh > create mode 100644 tools/perf/tests/insn-x86-dat-32.c > create mode 100644 tools/perf/tests/insn-x86-dat-64.c > create mode 100644 tools/perf/tests/insn-x86-dat-src.c > create mode 100644 tools/perf/tests/insn-x86.c > > > Regards > Adrian -- 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/
[PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions
Hi perf tools has a copy of the x86 instruction decoder for decoding Intel PT. This patch set adds a perf tools test to use it to test new instructions. Subsequent patches add a few new x86 instructions, or very slightly modify them in the case of MPX. Those changes affect both perf tools and x86/insn. I suggest Arnaldo takes all these patches as they mainly affect perf tools, at least in terms of lines-of-code. Adrian Hunter (4): perf tools: Add a test for decoding of new x86 instructions x86/insn: perf tools: Pedantically tweak opcode map for MPX instructions x86/insn: perf tools: Add new SHA instructions x86/insn: perf tools: Add new memory instructions arch/x86/lib/x86-opcode-map.txt| 19 +- tools/perf/tests/Build | 3 + tools/perf/tests/builtin-test.c| 8 + tools/perf/tests/gen-insn-x86-dat.awk | 75 ++ tools/perf/tests/gen-insn-x86-dat.sh | 43 ++ tools/perf/tests/insn-x86-dat-32.c | 640 tools/perf/tests/insn-x86-dat-64.c | 738 ++ tools/perf/tests/insn-x86-dat-src.c| 835 + tools/perf/tests/insn-x86.c| 180 + tools/perf/tests/tests.h | 1 + .../perf/util/intel-pt-decoder/x86-opcode-map.txt | 19 +- 11 files changed, 2553 insertions(+), 8 deletions(-) create mode 100644 tools/perf/tests/gen-insn-x86-dat.awk create mode 100755 tools/perf/tests/gen-insn-x86-dat.sh create mode 100644 tools/perf/tests/insn-x86-dat-32.c create mode 100644 tools/perf/tests/insn-x86-dat-64.c create mode 100644 tools/perf/tests/insn-x86-dat-src.c create mode 100644 tools/perf/tests/insn-x86.c Regards Adrian -- 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/