Re: [PATCH 1/2] perf tools: Introduce memory mapping macros in mman-fix.h

[Arnaldo Carvalho de Melo]

Em Tue, Sep 06, 2016 at 11:28:35PM +0800, Wangnan (F) escreveu:
> On 2016/9/6 21:39, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Sep 06, 2016 at 09:25:47PM +0800, Wangnan (F) escreveu:
> > > 
> > > On 2016/9/6 20:59, Arnaldo Carvalho de Melo wrote:
> > > > Em Tue, Sep 06, 2016 at 05:48:51AM +0000, Wang Nan escreveu:
> > > > > tools/perf/trace/beauty/mmap.c, tools/perf/util/event.c and
> > > > > tools/perf/util/map.c depend on several macros in mman.h, which
> > > > > are lost on old systems like ubuntu 12.04. They are architecture
> > > > Not "lost on old systems", changing that to "not present on old systems"
> > > > 
> > > > > dependened macros. Importing ./arch/*/include/uapi/asm/mman.h
> > > > > into tools/ is not easy because mman.h for some unusual archs
> > > > > (like tile) have extra dependencies.
> > > > > This patch introduces only required macros into mman-fix.h.
> > > > > Macros list is gotten from tools/perf/trace/beauty/mmap.c.
> > > > ok, I'll try keeping the  include/uapi/asm-generic/mman.h and
> > > > arch/alpha/include/uapi/asm/mman.h with just what you added, so that at
> > > > least the .c files appears as using the headers were those things are
> > > > defined in the kernel sources.
> > > > 
> > > > Ah, and good eyes in realizing this is not the same value for all
> > > > arches!
> > > This is why we have to define these macros for each archs.
> > > Your solution in tools/perf/trace/beauty/mmap.c works for
> > > x86 only.
> > > 
> > > However, I don't like this patchset because we add 300+
> > > lines of code and 22 new files for less than 15 macros,
> > > and only one of them I really concern. Moreover, futhre
> > > code require more symbols in uapi and we have to add new
> > > headers, finally we will clone the whole uapi. We have
> > > to stop doing this...
> > Why? If we want to support older newer kernels running on older distros,
> > which is a valid goal, or recent tool source code on older distros,
> > where fallbacks will take place with their older kernels, and if we have
> > the build automated for that (I have) then why not keep the tools
> > building on as many different userspaces as possible?
> 
> Your automated building checker is great. I mean we have
> to find a way to stop pulling kernel headers to tools/include.

Well, if we want the source code to build on older systems, then we must
provide #defines for things not present in such older systems, we would
have to put it somewhere, so why not to put it in the same header as the
kernel puts?
 
> But since we haven't find such a way, I'll continously improve
> this mman.h stuff.

We could, for instance, use the kernel headers, then, building perf on
older systems would not be a problem, the kernel sources goes with the
perf tool sources, they are in the same repository.

But this requires that the kernel developers make sure the perf tool
builds, which was deemed a burden to them. Thus the copy.

> As your suggestion, I'll try to put them in uapi/mman.h, not

Thanks!

> mman-fix.h. This require a building check for each arch.

Well, for the things that are strict copies, yes, having those diff
based checks would help us to figure out new files in the kernel for
which we have copies, but that isn't so _strictly_ needed for cases
where we avoided some of the dependencies in the kernel headers by
copying just what we need in tools/.

- Arnaldo