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
<-- snip -->
and more nasty problems follow later.
My suggestion is to:
- revert commit 27ac792ca0b0a1e7e65f20342260650516c95864 and then
- fix all PAGE_ALIGN() instances without moving them.
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.
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
On Fri, 25 Jul 2008 11:39:43 +0300 Adrian Bunk <[email protected]> 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.
On Fri, Jul 25, 2008 at 01:55:37AM -0700, Andrew Morton wrote:
> On Fri, 25 Jul 2008 11:39:43 +0300 Adrian Bunk <[email protected]> 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
On Fri, 25 Jul 2008 12:14:55 +0300 Adrian Bunk <[email protected]> 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.
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
On Fri, 25 Jul 2008 12:27:48 +0300 Adrian Bunk <[email protected]> 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.
On Fri, Jul 25, 2008 at 02:34:55AM -0700, Andrew Morton wrote:
> We should make arch_pick_mmap_layout __weak and nuke that ifdef.
I strongly disagree. I find it makes it harder to follow code flow
when __weak functions are involved. Ifdefs are ugly, no question, but
they're easier to grep for, see when they'll be defined and know which of
the arch_pick_mmap_layout() functions will be called. __weak certainly
has its uses (eg the sys_ni_syscall is great) but I find it's becoming
overused.
My basic point here is that __weak makes the code easier to write but
harder to read, and we're supposed to be optimising for easier to read.
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Fri, 25 Jul 2008 06:24:54 -0600 Matthew Wilcox <[email protected]> wrote:
> On Fri, Jul 25, 2008 at 02:34:55AM -0700, Andrew Morton wrote:
> > We should make arch_pick_mmap_layout __weak and nuke that ifdef.
>
> I strongly disagree. I find it makes it harder to follow code flow
> when __weak functions are involved. Ifdefs are ugly, no question, but
> they're easier to grep for, see when they'll be defined and know which of
> the arch_pick_mmap_layout() functions will be called. __weak certainly
> has its uses (eg the sys_ni_syscall is great) but I find it's becoming
> overused.
>
> My basic point here is that __weak makes the code easier to write but
> harder to read, and we're supposed to be optimising for easier to read.
>
If you see
void __weak arch_foo(...)
and can't immediately work out what's going on then converting it to an
ifdef maze won't save you.
Andrew Morton wrote:
> On Fri, 25 Jul 2008 12:14:55 +0300 Adrian Bunk <[email protected]> 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
On Fri, 25 Jul 2008, Matthew Wilcox wrote:
> On Fri, Jul 25, 2008 at 02:34:55AM -0700, Andrew Morton wrote:
> > We should make arch_pick_mmap_layout __weak and nuke that ifdef.
>
> I strongly disagree. I find it makes it harder to follow code flow
> when __weak functions are involved. Ifdefs are ugly, no question, but
> they're easier to grep for
Hell no, they're not.
Our use of random HAVE_ARCH_xyz or ARCH_SUPPORTS_xyz etc stuff makes
things _totally_ impossible to grep for.
In contrast, it we did this code as
#ifndef arch_pick_mmap_layout
void __weak 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
then trying to grep for arch_pick_mmap_layout() would show EVERY SINGLE
RELEVANT CASE! And it would show the "__weak" there too, so that once
people get used to this convention, they'd have a really easy time
figuring out the rules from just the output of the 'grep'.
I really think that whoever started that 'HAVE_ARCH_x'/'ARCH_HAS_x' mess
with totally random symbols that have NOTHING WHAT-SO-EVER to do with the
actual symbols in question (so they do _not_ show up in grep'ing for some
use) should be shot.
We should never _ever_ use that model. And we use it way too much.
We should generally strive for the simpler and much more obvious
/* Generic definition */
#ifndef symbol
int symbol(..)
...
#endif
and then architecture code can do
#define symbol(x) ...
or if they want to do a function, and you _really_ don't like the '__weak'
part (or you want to make it an inline function and don't want the clash
with the real declaration), then you can just do
static inline int symbol(x)
{
...
}
#define symbol symbol
and again it all works fine WITHOUT having to introduce some idiotic new
and unrelated element called ARCH_HAS_SYMBOL.
And now when you do 'git grep symbol' you really will see the rules. ALL
the rules. Not some random collection of uses that don't actually explain
why there are five different definitions of the same thing and then you
have to figure out which one gets used.
> My basic point here is that __weak makes the code easier to write but
> harder to read, and we're supposed to be optimising for easier to read.
But your basic point is flawed. The thing you advocate is actually harder
to read.
Yes, if you don't follow the codign style, and you write
int __weak
symbol(x)
{
you are (a) a moronic rebel you never understood why the declaration
should be on one line and (b) as a result your 'grep' won't see the __weak
and you'll be confused about the rules.
But if we _consistently_ used
- '#ifndef symbol' to avoid redeclaring something that the architecture
overrides
- and '__weak' to allow architectures to just override functions without
extra work and rules
then after a while people would simply _know_ that very simple set of
rules, and a 'grep' would work so much better than it does now.
Really. Try it. Try it with 'arch_pick_mmap_layout' (with Andrews patch in
place). And then imagine that you'd be used to '__weak', and seeing that
additional
mm/util.c:#ifndef arch_pick_mmap_layout
mm/util.c:void __weak arch_pick_mmap_layout(struct mm_struct *mm)
in the output. Be honest now - wouldn't that actually _tell_ you something
relevant about that particular declaration? And make the fact that some
architectures override it _less_ confusing?
IOW, you could tell directly from the grep output that it's a "default
fallback". Which you definitely cannot tell right now, because we have
insane models for doing it.
Linus
On Fri, Jul 25, 2008 at 02:34:55AM -0700, Andrew Morton wrote:
> On Fri, 25 Jul 2008 12:27:48 +0300 Adrian Bunk <[email protected]> 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.
>...
Requires the fix below.
cu
Adrian
<-- snip -->
This patch fixes the following build error:
<-- snip -->
...
CC mm/util.o
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c: In function 'arch_pick_mmap_layout':
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c:144: error: dereferencing pointer to incomplete type
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c:145: error: 'arch_get_unmapped_area' undeclared (first use in this function)
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c:145: error: (Each undeclared identifier is reported only once
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c:145: error: for each function it appears in.)
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c:146: error: 'arch_unmap_area' undeclared (first use in this function)
make[2]: *** [mm/util.o] Error 1
<-- snip -->
Signed-off-by: Adrian Bunk <[email protected]>
---
f003405997b99d300c99f7b1eae408fe42c07126
diff --git a/mm/util.c b/mm/util.c
index 0efd830..d8d0221 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -3,6 +3,7 @@
#include <linux/string.h>
#include <linux/module.h>
#include <linux/err.h>
+#include <linux/sched.h>
#include <asm/uaccess.h>
/**
Reaching back into an old discussion....
On Sat, Jul 26, 2008 at 12:38 PM, Linus Torvalds
<[email protected]> wrote:
>
>
> On Fri, 25 Jul 2008, Matthew Wilcox wrote:
>
>> On Fri, Jul 25, 2008 at 02:34:55AM -0700, Andrew Morton wrote:
>> > We should make arch_pick_mmap_layout __weak and nuke that ifdef.
>>
>> I strongly disagree. ?I find it makes it harder to follow code flow
>> when __weak functions are involved. ?Ifdefs are ugly, no question, but
>> they're easier to grep for
>
> Hell no, they're not.
>
> Our use of random HAVE_ARCH_xyz or ARCH_SUPPORTS_xyz etc stuff makes
> things _totally_ impossible to grep for.
>
> In contrast, it we did this code as
>
> ? ? ? ?#ifndef arch_pick_mmap_layout
> ? ? ? ?void __weak 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
>
> then trying to grep for arch_pick_mmap_layout() would show EVERY SINGLE
> RELEVANT CASE! And it would show the "__weak" there too, so that once
> people get used to this convention, they'd have a really easy time
> figuring out the rules from just the output of the 'grep'.
[...]
Question. If I use this pattern, and use the __weak attribute on core
code functions wrapped with a #ifndef, then how does it mesh with
EXPORT_SYMBOL*() statements? Do both the core code, and the arch
override need to do EXPORT_SYMBOL(), or should EXPORT_SYMBOL() only
appear at the core code site?
I also assume that at the core code site, the EXPORT_SYMBOL() must
appear inside the #ifndef block so that a #define override doesn't
break things. Correct?
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
On Wed, 17 Feb 2010, Grant Likely wrote:
>
> Question. If I use this pattern, and use the __weak attribute on core
> code functions wrapped with a #ifndef, then how does it mesh with
> EXPORT_SYMBOL*() statements?
You can't. Or at least not the traditional way, which is to put the
EXPORT_SYMBOL next to the definition of what gets exported.
If you use __weak, you need to make sure that all users of said weak
symbol are in another file. There are some compiler and/or binutils bugs
with using weak symbols in the same translation unit, so the rule is that
a weak definition hes to be somewhere else from the use - including
EXPORT_SYMBOL.
> I also assume that at the core code site, the EXPORT_SYMBOL() must
> appear inside the #ifndef block so that a #define override doesn't
> break things. Correct?
See above. You can't put it inside the _same_ #ifndef block. You'd have to
put it in a different file, but yes, inside an ifndef. At which point the
linker will just pick the right definition.
However, in general, this probably gets ugly enough that the whole __weak
thing is not great for exported stuff. It's likely mostly useful for some
"generic" arch functionality that is always compiled in.
Linus