perf build broke by list_head changes...

perf build broke by list_head changes...

[David Miller]

Commit:

commit de5d9bf6541736dc7ad264d2b5cc99bc1b2ad958
Author: Chris Metcalf <cmetcalf@tilera.com>
Date:   Fri Jul 2 13:41:14 2010 -0400

    Move list types from <linux/list.h> to <linux/types.h>.

broke the build of 'perf'.

If you move "struct list_head" into types.h, this means perf stops
building because it depends upon being able to include linux/list.h
from a userland application and at the same time be able to get the
basic data types without defining __KERNEL__ or similar.

Now that no longer occurs because the bulk of types.h is __KERNEL__
protected and thus the build breaks since "struct list_head" is
never defined.

Re: perf build broke by list_head changes...

[Chris Metcalf]

On 8/10/2010 2:57 AM, David Miller wrote:
> Commit:
>
> commit de5d9bf6541736dc7ad264d2b5cc99bc1b2ad958
> Author: Chris Metcalf <cmetcalf@tilera.com>
> Date:   Fri Jul 2 13:41:14 2010 -0400
>
>     Move list types from <linux/list.h> to <linux/types.h>.
>
> broke the build of 'perf'.
>
> If you move "struct list_head" into types.h, this means perf stops
> building because it depends upon being able to include linux/list.h
> from a userland application and at the same time be able to get the
> basic data types without defining __KERNEL__ or similar.
>   

If necessary, it certainly would be easy to move the list.h types to
follow struct ustat, then bump the #endif up above them with a comment
about the perf system's use of them.

I'm confused, though, since <linux/list.h> isn't installed by
headers_install, so how was perf finding that definition before anyway?

I assume the requirement is something fairly stringent, like parsing
binary data structures out of a memory-mapped region or some such;
normally you wouldn't even want to expose the list_head declaration to
user space.  I haven't looked at the perf subsystem myself yet so I
don't really know.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: perf build broke by list_head changes...

[Sam Ravnborg]

On Tue, Aug 10, 2010 at 08:36:46AM -0400, Chris Metcalf wrote:
> On 8/10/2010 2:57 AM, David Miller wrote:
> > Commit:
> >
> > commit de5d9bf6541736dc7ad264d2b5cc99bc1b2ad958
> > Author: Chris Metcalf <cmetcalf@tilera.com>
> > Date:   Fri Jul 2 13:41:14 2010 -0400
> >
> >     Move list types from <linux/list.h> to <linux/types.h>.
> >
> > broke the build of 'perf'.
> >
> > If you move "struct list_head" into types.h, this means perf stops
> > building because it depends upon being able to include linux/list.h
> > from a userland application and at the same time be able to get the
> > basic data types without defining __KERNEL__ or similar.
> >   
> 
> If necessary, it certainly would be easy to move the list.h types to
> follow struct ustat, then bump the #endif up above them with a comment
> about the perf system's use of them.
> 
> I'm confused, though, since <linux/list.h> isn't installed by
> headers_install, so how was perf finding that definition before anyway?

kernel devs usually tell people not to use kernel
headers direct.
But perf does so a lot.

$ cd tools/perf/util; git grep '../../..'
header.h:#include "../../../include/linux/perf_event.h"
include/asm/byteorder.h:#include "../../../../include/linux/swab.h"
include/linux/hash.h:#include "../../../../include/linux/hash.h"
include/linux/list.h:#include "../../../../include/linux/list.h"
include/linux/poison.h:#include "../../../../include/linux/poison.h"
include/linux/rbtree.h:#include "../../../../include/linux/rbtree.h"
parse-events.c:#include "../../../include/linux/hw_breakpoint.h"
parse-events.c: snprintf(evt_path, MAXPATHLEN, "%s/%s/%s/id", debugfs_path,
parse-events.c: snprintf(evt_path, MAXPATHLEN, "%s/%s/%s/id", debugfs_path,
session.h:#include "../../../include/linux/perf_event.h"
util.h:#include "../../../include/linux/magic.h"
util.h:#include "../../../include/linux/stringify.h"

IMO the patch that moves list_head to types.h is fine.
And perf needs to learn good manner with respect to
kernel headers.

	Sam

Re: perf build broke by list_head changes...

[Arnaldo Carvalho de Melo]

Em Tue, Aug 10, 2010 at 02:44:45PM +0200, Sam Ravnborg escreveu:
> IMO the patch that moves list_head to types.h is fine.
> And perf needs to learn good manner with respect to
> kernel headers.

Idea is to try to have the perf tools, since they are hosted in the
kernel and developed mostly by people with kernel background, to use
code and practices used in the kernel proper.

It started just keeping private copies, I guess it should get back to
that since the reaction to this kind of same source repo code sharing
was, well, not good :-)

Alternatives?

- Arnaldo

Re: perf build broke by list_head changes...

[Arnd Bergmann]

On Tuesday 10 August 2010, Arnaldo Carvalho de Melo wrote:
> It started just keeping private copies, I guess it should get back to
> that since the reaction to this kind of same source repo code sharing
> was, well, not good :-)
> 
> Alternatives?

If perf wants to play tricks with the header files, we should probably
make them explicit, as in this ugly bit of code.

	Arnd

diff --git a/include/linux/types.h b/include/linux/types.h
index 01a082f..0e0998a 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -178,7 +178,7 @@ typedef __u64 __bitwise __be64;
 typedef __u16 __bitwise __sum16;
 typedef __u32 __bitwise __wsum;
 
-#ifdef __KERNEL__
+#if defined(__KERNEL__) || defined(__PERF__)
 typedef unsigned __bitwise__ gfp_t;
 typedef unsigned __bitwise__ fmode_t;
 
diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 26f626d..9806dc9 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -225,7 +225,7 @@ ifndef PERF_DEBUG
   CFLAGS_OPTIMIZE = -O6
 endif
 
-CFLAGS = -ggdb3 -Wall -Wextra -std=gnu99 -Werror $(CFLAGS_OPTIMIZE) -D_FORTIFY_SOURCE=2 $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
+CFLAGS = -ggdb3 -Wall -Wextra -std=gnu99 -Werror $(CFLAGS_OPTIMIZE) -D__PERF__ -D_FORTIFY_SOURCE=2 $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
 EXTLIBS = -lpthread -lrt -lelf -lm
 ALL_CFLAGS = $(CFLAGS) -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
 ALL_LDFLAGS = $(LDFLAGS)

Re: perf build broke by list_head changes...

[Arnaldo Carvalho de Melo]

Em Tue, Aug 10, 2010 at 04:29:53PM +0200, Arnd Bergmann escreveu:
> On Tuesday 10 August 2010, Arnaldo Carvalho de Melo wrote:
> > It started just keeping private copies, I guess it should get back to
> > that since the reaction to this kind of same source repo code sharing
> > was, well, not good :-)
> > 
> > Alternatives?
> 
> If perf wants to play tricks with the header files, we should probably
> make them explicit, as in this ugly bit of code.

Nah, no more ifdefs, the goal was more to share things with tools
developed/shipped in the kernel source repository, not to do something
just for perf.
 
> diff --git a/include/linux/types.h b/include/linux/types.h
> @@ -178,7 +178,7 @@ typedef __u64 __bitwise __be64;
>  typedef __u16 __bitwise __sum16;
>  typedef __u32 __bitwise __wsum;
>  
> -#ifdef __KERNEL__
> +#if defined(__KERNEL__) || defined(__PERF__)
>  typedef unsigned __bitwise__ gfp_t;

- Arnaldo

Re: perf build broke by list_head changes...

[Sam Ravnborg]

On Tue, Aug 10, 2010 at 10:46:00AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Aug 10, 2010 at 02:44:45PM +0200, Sam Ravnborg escreveu:
> > IMO the patch that moves list_head to types.h is fine.
> > And perf needs to learn good manner with respect to
> > kernel headers.
> 
> Idea is to try to have the perf tools, since they are hosted in the
> kernel and developed mostly by people with kernel background, to use
> code and practices used in the kernel proper.
> 
> It started just keeping private copies, I guess it should get back to
> that since the reaction to this kind of same source repo code sharing
> was, well, not good :-)
> 
> Alternatives?

I have not analyzed deeper in what parts perf uses so the
following may not fly at all.

One solution could be to let perf rely on a set of
exported userspace headers from the kernel.

And on top of this copy a small set of kernel internal
headers for use by perf only.
The copying will be a good way to document what is actually
used of the kernel internal stuff.

But I also realize that just copying over list.h does
not suffice. It pulls in lots of other stuff.
So this is most likely hard to do.

I guess first step is to identify what is really used
beside the exported stuff and start to conclude from there.

	Sam

Re: perf build broke by list_head changes...

[Arnaldo Carvalho de Melo]

Em Tue, Aug 10, 2010 at 04:32:41PM +0200, Sam Ravnborg escreveu:
> On Tue, Aug 10, 2010 at 10:46:00AM -0300, Arnaldo Carvalho de Melo wrote:
> > Idea is to try to have the perf tools, since they are hosted in the
> > kernel and developed mostly by people with kernel background, to use
> > code and practices used in the kernel proper.

> > It started just keeping private copies, I guess it should get back to
> > that since the reaction to this kind of same source repo code sharing
> > was, well, not good :-)

> > Alternatives?
 
> I have not analyzed deeper in what parts perf uses so the following
> may not fly at all.

> One solution could be to let perf rely on a set of exported userspace
> headers from the kernel.

Yeah, finding things that could be shared was part of this exercise, as some
people mentioned that lots of userspace programs out there already have
list.h copies.

Petrifying list.h (and other headers) for the sake of out of the kernel use
probably won't get many supporters, for things that ship and are developed in
the same source repo tho, perhaps can be made possible.

> And on top of this copy a small set of kernel internal headers for use
> by perf only.  The copying will be a good way to document what is
> actually used of the kernel internal stuff.

Yeah, that to me is the easier, non controversial path to take, like done with
hweight.c et all, I'll do that.
 
> But I also realize that just copying over list.h does not suffice. It
> pulls in lots of other stuff.  So this is most likely hard to do.
> 
> I guess first step is to identify what is really used beside the
> exported stuff and start to conclude from there.

This is what I did for hweight.c in:

[acme@doppio linux-2.6-tip]$ git log tools/perf/util/hweight.c 
commit fb72014d98afd51e85aab9c061344ef32d615606
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Fri Apr 30 19:31:12 2010 -0300

    perf tools: Don't use code surrounded by __KERNEL__
    
    We need to refactor code to be explicitely shared by the kernel and at
    least the tools/ userspace programs, so, till we do that, copy the bare
    minimum bitmap/bitops code needed by tools/perf.
    
    Reported-by: "H. Peter Anvin" <hpa@zytor.com>
    Cc: Frédéric Weisbecker <fweisbec@gmail.com>
    Cc: Mike Galbraith <efault@gmx.de>
    Cc: Paul Mackerras <paulus@samba.org>
    Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
    LKML-Reference: <new-submission>
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
[acme@doppio linux-2.6-tip]$

I will continue that exercise and leave explicitely sharing stuff with
the kernel for later, if ever.

Then we will just have to track changes to the initially shared stuff so
that we don't miss fixes like described in the cset where some of this
sharing was started:

commit 43cbcd8acb4c992cbd22d1ec8a08c0591be5d719
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Wed Jul 1 12:28:37 2009 -0300

    perf_counter tools: Share rbtree.with the kernel
    
    The tools/perf/util/rbtree.c copy already drifted by three
    csets:
    
     4b324126e0c6c3a5080ca3ec0981e8766ed6f1ee
     4c60117811171d867d4f27f17ea07d7419d45dae
     16c047add3ceaf0ab882e3e094d1ec904d02312d

    So remove the copy and use the lib/rbtree.c directly, sharing
    the source code while still generating a separate object file,
    since tools/perf uses a far more agressive -O6 switch.
    
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
    Cc: Mike Galbraith <efault@gmx.de>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Paul Mackerras <paulus@samba.org>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    LKML-Reference: <20090701152837.GG15682@ghostprotocols.net>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

- Arnaldo

Re: perf build broke by list_head changes...

[Matthew Wilcox]

On Mon, Aug 09, 2010 at 11:57:46PM -0700, David Miller wrote:
> Commit:
> 
> commit de5d9bf6541736dc7ad264d2b5cc99bc1b2ad958
> Author: Chris Metcalf <cmetcalf@tilera.com>
> Date:   Fri Jul 2 13:41:14 2010 -0400
> 
>     Move list types from <linux/list.h> to <linux/types.h>.
> 
> broke the build of 'perf'.
> 
> If you move "struct list_head" into types.h, this means perf stops
> building because it depends upon being able to include linux/list.h
> from a userland application and at the same time be able to get the
> basic data types without defining __KERNEL__ or similar.

Sorry about that.  Obviously, I didn't test-build this patch.

> Now that no longer occurs because the bulk of types.h is __KERNEL__
> protected and thus the build breaks since "struct list_head" is
> never defined.

list.h isn't a header-y file, so it's not an interface we expect userspace
to use.  So one reasonable way to fix this is for perf to take its own copy.

Reverting the patch gets us back to the former problem (of not being
able to use list.h in certain cases).

We could do a glibc-like _WANT_LIST_HEAD macro.  I don't think the
resulting uglification is reasonable.

perf could pretend that it's real userspace instead of being part of
the kernel and use a list implementation designed for userspace.  I
don't know of a good one though.

Any preferences which solution we take?