Mailing List Archive

On Fri, 25 Jul 2008 11:39:43 +0300 Adrian Bunk <bunk@kernel.org> wrote:

> Commit 27ac792ca0b0a1e7e65f20342260650516c95864
> (PAGE_ALIGN(): correctly handle 64-bit values on 32-bit architectures)
> causes on some architectures (e.g. avr32 and mips) compile errors
> like the following in some configurations starting with:
>
> <-- snip -->
>
> ...
> CC init/main.o
> In file included from
> /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/utsname.h:35,
> from /home/bunk/linux/kernel-2.6/git/linux-2.6/init/main.c:20:
> /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/sched.h: In function 'arch_pick_mmap_layout':
> /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/sched.h:2149: error: implicit declaration of function 'PAGE_ALIGN'
> make[2]: *** [init/main.o] Error 1
>

pls test:

diff -puN include/linux/sched.h~a include/linux/sched.h
--- a/include/linux/sched.h~a
+++ a/include/linux/sched.h
@@ -2139,16 +2139,7 @@ static inline void set_task_cpu(struct t

#endif /* CONFIG_SMP */

-#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
extern void arch_pick_mmap_layout(struct mm_struct *mm);
-#else
-static inline void arch_pick_mmap_layout(struct mm_struct *mm)
-{
- mm->mmap_base = TASK_UNMAPPED_BASE;
- mm->get_unmapped_area = arch_get_unmapped_area;
- mm->unmap_area = arch_unmap_area;
-}
-#endif

#ifdef CONFIG_TRACING
extern void
diff -puN mm/mmap.c~a mm/mmap.c
--- a/mm/mmap.c~a
+++ a/mm/mmap.c
@@ -2268,3 +2268,12 @@ int install_special_mapping(struct mm_st

return 0;
}
+
+#ifndef HAVE_ARCH_PICK_MMAP_LAYOUT
+void arch_pick_mmap_layout(struct mm_struct *mm)
+{
+ mm->mmap_base = TASK_UNMAPPED_BASE;
+ mm->get_unmapped_area = arch_get_unmapped_area;
+ mm->unmap_area = arch_unmap_area;
+}
+#endif
_

>
> and more nasty problems follow later.
>
> My suggestion is to:
> - revert commit 27ac792ca0b0a1e7e65f20342260650516c95864 and then
> - fix all PAGE_ALIGN() instances without moving them.

Every time this patch blew up (and it did it often), fixing it resulted
in overall improvements.

> Unifying code is a good thing, but in this case it is not worth the
> trouble it causes by poking into the heart of our headers mess.

Well. If we leave it a mess, it'll stay a mess.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, Jul 25, 2008 at 01:55:37AM -0700, Andrew Morton wrote:
> On Fri, 25 Jul 2008 11:39:43 +0300 Adrian Bunk <bunk@kernel.org> wrote:
>
> > Commit 27ac792ca0b0a1e7e65f20342260650516c95864
> > (PAGE_ALIGN(): correctly handle 64-bit values on 32-bit architectures)
> > causes on some architectures (e.g. avr32 and mips) compile errors
> > like the following in some configurations starting with:
> >
> > <-- snip -->
> >
> > ...
> > CC init/main.o
> > In file included from
> > /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/utsname.h:35,
> > from /home/bunk/linux/kernel-2.6/git/linux-2.6/init/main.c:20:
> > /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/sched.h: In function 'arch_pick_mmap_layout':
> > /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/sched.h:2149: error: implicit declaration of function 'PAGE_ALIGN'
> > make[2]: *** [init/main.o] Error 1
> >
>
> pls test:
>
> diff -puN include/linux/sched.h~a include/linux/sched.h
> --- a/include/linux/sched.h~a
> +++ a/include/linux/sched.h
> @@ -2139,16 +2139,7 @@ static inline void set_task_cpu(struct t
>
> #endif /* CONFIG_SMP */
>
> -#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
> extern void arch_pick_mmap_layout(struct mm_struct *mm);
> -#else
> -static inline void arch_pick_mmap_layout(struct mm_struct *mm)
> -{
> - mm->mmap_base = TASK_UNMAPPED_BASE;
> - mm->get_unmapped_area = arch_get_unmapped_area;
> - mm->unmap_area = arch_unmap_area;
> -}
> -#endif
>
> #ifdef CONFIG_TRACING
> extern void
> diff -puN mm/mmap.c~a mm/mmap.c
> --- a/mm/mmap.c~a
> +++ a/mm/mmap.c
> @@ -2268,3 +2268,12 @@ int install_special_mapping(struct mm_st
>
> return 0;
> }
> +
> +#ifndef HAVE_ARCH_PICK_MMAP_LAYOUT
> +void arch_pick_mmap_layout(struct mm_struct *mm)
> +{
> + mm->mmap_base = TASK_UNMAPPED_BASE;
> + mm->get_unmapped_area = arch_get_unmapped_area;
> + mm->unmap_area = arch_unmap_area;
> +}
> +#endif

Nice, this seems to fix the problem.

> > and more nasty problems follow later.
> >
> > My suggestion is to:
> > - revert commit 27ac792ca0b0a1e7e65f20342260650516c95864 and then
> > - fix all PAGE_ALIGN() instances without moving them.
>
> Every time this patch blew up (and it did it often), fixing it resulted
> in overall improvements.
>
> > Unifying code is a good thing, but in this case it is not worth the
> > trouble it causes by poking into the heart of our headers mess.
>
> Well. If we leave it a mess, it'll stay a mess.

An interesting question is whether the PAGE_ALIGN() move makes the mess
bigger or smaller.

Ideally, all headers should be self-contained. IOW, they should #include
everything they use.

But TASK_UNMAPPED_BASE in asm/processor.h on some architectures uses
PAGE_ALIGN() that got moved from asm/page.h to linux/mm.h .

cu
Adrian

--

"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, 25 Jul 2008 12:14:55 +0300 Adrian Bunk <bunk@kernel.org> wrote:

> Ideally, all headers should be self-contained. IOW, they should #include
> everything they use.

Yup. And the core reason for our headers mess is that the headers do
too much stuff, and cnosequently demand a large dependency trail.

> But TASK_UNMAPPED_BASE in asm/processor.h on some architectures uses
> PAGE_ALIGN() that got moved from asm/page.h to linux/mm.h .

Probably mm.h should be split up - put the simple things (usually
declarations) into one "early" header file and leave the more
heavyweight things (usually implementations) in mm.h.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, Jul 25, 2008 at 12:14:55PM +0300, Adrian Bunk wrote:
> On Fri, Jul 25, 2008 at 01:55:37AM -0700, Andrew Morton wrote:
>...
> > pls test:
> >
> > diff -puN include/linux/sched.h~a include/linux/sched.h
> > --- a/include/linux/sched.h~a
> > +++ a/include/linux/sched.h
> > @@ -2139,16 +2139,7 @@ static inline void set_task_cpu(struct t
> >
> > #endif /* CONFIG_SMP */
> >
> > -#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
> > extern void arch_pick_mmap_layout(struct mm_struct *mm);
> > -#else
> > -static inline void arch_pick_mmap_layout(struct mm_struct *mm)
> > -{
> > - mm->mmap_base = TASK_UNMAPPED_BASE;
> > - mm->get_unmapped_area = arch_get_unmapped_area;
> > - mm->unmap_area = arch_unmap_area;
> > -}
> > -#endif
> >
> > #ifdef CONFIG_TRACING
> > extern void
> > diff -puN mm/mmap.c~a mm/mmap.c
> > --- a/mm/mmap.c~a
> > +++ a/mm/mmap.c
> > @@ -2268,3 +2268,12 @@ int install_special_mapping(struct mm_st
> >
> > return 0;
> > }
> > +
> > +#ifndef HAVE_ARCH_PICK_MMAP_LAYOUT
> > +void arch_pick_mmap_layout(struct mm_struct *mm)
> > +{
> > + mm->mmap_base = TASK_UNMAPPED_BASE;
> > + mm->get_unmapped_area = arch_get_unmapped_area;
> > + mm->unmap_area = arch_unmap_area;
> > +}
> > +#endif
>
> Nice, this seems to fix the problem.
>...

Further testing revealed that you should choose a file that also gets
compiled on MMU-less architectures:

<-- snip -->

...
LD vmlinux
fs/built-in.o: In function `flush_old_exec':
(.text+0x6ae8): undefined reference to `arch_pick_mmap_layout'
fs/built-in.o: In function `flush_old_exec':
(.text+0x6cf0): undefined reference to `arch_pick_mmap_layout'
make[1]: *** [vmlinux] Error 1

<-- snip -->

cu
Adrian

--

"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, 25 Jul 2008 12:27:48 +0300 Adrian Bunk <bunk@kernel.org> wrote:

> Further testing revealed that you should choose a file that also gets
> compiled on MMU-less architectures:

We don't have one :(

I guess util.c will have to do.

diff -puN include/linux/sched.h~uninline-arch_pick_mmap_layout include/linux/sched.h
--- a/include/linux/sched.h~uninline-arch_pick_mmap_layout
+++ a/include/linux/sched.h
@@ -2139,16 +2139,7 @@ static inline void set_task_cpu(struct t

#endif /* CONFIG_SMP */

-#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
extern void arch_pick_mmap_layout(struct mm_struct *mm);
-#else
-static inline void arch_pick_mmap_layout(struct mm_struct *mm)
-{
- mm->mmap_base = TASK_UNMAPPED_BASE;
- mm->get_unmapped_area = arch_get_unmapped_area;
- mm->unmap_area = arch_unmap_area;
-}
-#endif

#ifdef CONFIG_TRACING
extern void
diff -puN mm/util.c~uninline-arch_pick_mmap_layout mm/util.c
--- a/mm/util.c~uninline-arch_pick_mmap_layout
+++ a/mm/util.c
@@ -1,3 +1,4 @@
+#include <linux/mm.h>
#include <linux/slab.h>
#include <linux/string.h>
#include <linux/module.h>
@@ -160,3 +161,12 @@ char *strndup_user(const char __user *s,
return p;
}
EXPORT_SYMBOL(strndup_user);
+
+#ifndef HAVE_ARCH_PICK_MMAP_LAYOUT
+void arch_pick_mmap_layout(struct mm_struct *mm)
+{
+ mm->mmap_base = TASK_UNMAPPED_BASE;
+ mm->get_unmapped_area = arch_get_unmapped_area;
+ mm->unmap_area = arch_unmap_area;
+}
+#endif
_


We should make arch_pick_mmap_layout __weak and nuke that ifdef.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Andrew Morton wrote:
> On Fri, 25 Jul 2008 12:14:55 +0300 Adrian Bunk <bunk@kernel.org> wrote:
>
>> Ideally, all headers should be self-contained. IOW, they should #include
>> everything they use.
>
> Yup. And the core reason for our headers mess is that the headers do
> too much stuff, and cnosequently demand a large dependency trail.
>
>> But TASK_UNMAPPED_BASE in asm/processor.h on some architectures uses
>> PAGE_ALIGN() that got moved from asm/page.h to linux/mm.h .
>
> Probably mm.h should be split up - put the simple things (usually
> declarations) into one "early" header file and leave the more
> heavyweight things (usually implementations) in mm.h.

IMHO splitting mm.h is probably the best solution. If I'm not wrong
Paul (CCed) already suggested to move the stuff like PAGE_ALIGN() outside
mm.h the first time I submitted this patch.

In this way we could even include the "lightweight" mm.h (mm_define.h??)
in all the asm-*/page.h, preserving also the backward compatibility.

-Andrea
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/