RE: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions

2015-09-02 Thread 平松雅巳 / HIRAMATU，MASAMI 
> 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

2015-09-02 Thread Ingo Molnar 

* 平松雅巳 / 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

2015-09-02 Thread 平松雅巳 / HIRAMATU，MASAMI 
> 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

2015-09-02 Thread Ingo Molnar 

* 平松雅巳 / 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

2015-09-02 Thread 平松雅巳 / HIRAMATU，MASAMI 
> 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

2015-09-02 Thread 平松雅巳 / HIRAMATU，MASAMI 
> 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

2015-09-01 Thread Jiri Olsa 
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

2015-09-01 Thread Arnaldo Carvalho de Melo 
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

2015-09-01 Thread 平松雅巳 / HIRAMATU，MASAMI 
> 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

2015-09-01 Thread Arnaldo Carvalho de Melo 
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

2015-09-01 Thread Jiri Olsa 
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

2015-09-01 Thread Arnaldo Carvalho de Melo 
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

2015-09-01 Thread Ingo Molnar 

* 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

2015-09-01 Thread Adrian Hunter 
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

2015-09-01 Thread Adrian Hunter 
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

2015-09-01 Thread 平松雅巳 / HIRAMATU，MASAMI 
> 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

2015-09-01 Thread 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? )

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

2015-09-01 Thread Arnaldo Carvalho de Melo 
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

2015-09-01 Thread 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? )

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

2015-09-01 Thread 平松雅巳 / HIRAMATU，MASAMI 
> 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

2015-09-01 Thread Adrian Hunter 
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

2015-09-01 Thread Adrian Hunter 
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

2015-09-01 Thread Ingo Molnar 

* 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

2015-09-01 Thread Jiri Olsa 
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

2015-09-01 Thread Arnaldo Carvalho de Melo 
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

2015-09-01 Thread 平松雅巳 / HIRAMATU，MASAMI 
> 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

2015-09-01 Thread Arnaldo Carvalho de Melo 
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

2015-09-01 Thread Jiri Olsa 
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

2015-08-31 Thread Arnaldo Carvalho de Melo 
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

2015-08-31 Thread Adrian Hunter 
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/

Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions

2015-08-31 Thread Arnaldo Carvalho de Melo 
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

2015-08-31 Thread Adrian Hunter 
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/