Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
On Fri, 2008-04-18 at 16:44 +1000, Benjamin Herrenschmidt wrote: so what about the patch below ? I like it, but the compiler won't ;) If you're ok, I'll re-send with appropriate sob adapted powerpc part. Sure. +void __init __attribute__((weak) thread_info_cache_init(void) Back to this old subject... I'm having reports that this is not working... gcc is seeing the empty weak function and is optimizing it out before it gets a chance to link to the arch provided one. This would affect that and the other one next to it.. That seems pretty bad... it causes nasty crashes as we end up having no idea what the compiler decided to generate... I suppose we could keep the weak stubs out of the file where they are called but that sucks. ie. This is some form of gcc 4.1.1 Is that a known problem ? A gcc issue ? Not sure what is expected from those weak functions. Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
On Wed, 21 May 2008 13:56:25 -0400 Benjamin Herrenschmidt [EMAIL PROTECTED]
wrote:
On Fri, 2008-04-18 at 16:44 +1000, Benjamin Herrenschmidt wrote:
so what
about the patch below ?
I like it, but the compiler won't ;)
If you're ok, I'll re-send with appropriate sob
adapted powerpc part.
Sure.
+void __init __attribute__((weak) thread_info_cache_init(void)
Back to this old subject...
I'm having reports that this is not working...
gcc is seeing the empty weak function and is optimizing it out
before it gets a chance to link to the arch provided one.
This would affect that and the other one next to it..
That seems pretty bad... it causes nasty crashes as we end up having no
idea what the compiler decided to generate... I suppose we could keep
the weak stubs out of the file where they are called but that sucks.
ie. This is some form of gcc 4.1.1
Is that a known problem ? A gcc issue ? Not sure what is expected from
those weak functions.
yup, gcc bug. Discussed recently on lkml, Subject: Re: huge gcc
4.1.{0,1} __weak problem. I don't think anything ended up happening
about it though.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
On Wed, May 21, 2008 at 11:41:47AM -0700, Andrew Morton wrote:
On Wed, 21 May 2008 13:56:25 -0400 Benjamin Herrenschmidt [EMAIL PROTECTED]
wrote:
On Fri, 2008-04-18 at 16:44 +1000, Benjamin Herrenschmidt wrote:
so what
about the patch below ?
I like it, but the compiler won't ;)
If you're ok, I'll re-send with appropriate sob
adapted powerpc part.
Sure.
+void __init __attribute__((weak) thread_info_cache_init(void)
Back to this old subject...
I'm having reports that this is not working...
gcc is seeing the empty weak function and is optimizing it out
before it gets a chance to link to the arch provided one.
This would affect that and the other one next to it..
That seems pretty bad... it causes nasty crashes as we end up having no
idea what the compiler decided to generate... I suppose we could keep
the weak stubs out of the file where they are called but that sucks.
ie. This is some form of gcc 4.1.1
Is that a known problem ? A gcc issue ? Not sure what is expected from
those weak functions.
yup, gcc bug. Discussed recently on lkml, Subject: Re: huge gcc
4.1.{0,1} __weak problem. I don't think anything ended up happening
about it though.
It was discussed to add some run-time checks for this issue.
But the examples given were a bit fluffy so I never integrated anything
i kbuild to detect this.
As this is only a bug for const weak functions they could be made non-const
if they are seldomly used?
Sam
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
On Wed, 2008-05-21 at 11:41 -0700, Andrew Morton wrote:
yup, gcc bug. Discussed recently on lkml, Subject: Re: huge gcc
4.1.{0,1} __weak problem. I don't think anything ended up happening
about it though.
Hrm... do you think we should work around ? ie. move the stubs to a
separate .c file ?
Ben.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
On Wed, 2008-05-21 at 21:06 +0200, Sam Ravnborg wrote:
It was discussed to add some run-time checks for this issue.
But the examples given were a bit fluffy so I never integrated
anything
i kbuild to detect this.
As this is only a bug for const weak functions they could be made
non-const
if they are seldomly used?
With the asm() trick ?
I suppose, but I'm also happy to just reject the bad gcc...
It shouldn't be too hard to do a test case made of 2 files.
test_foo.c
int foo(void)
{
printf(good\n);
}
test_bar.c
int foo(void) __weak
{
}
int main(void)
{
foo();
return 0;
}
And check for good in the output of said program..
Can somebody test that ? Luke, you have a broken compiler, can you make
up some test that could be integrated in the kernel build system
easily ?
(I'm travelling right now, no time to play much with it myself).
Cheers,
Ben.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
On Wed, 21 May 2008 15:44:41 -0400
Benjamin Herrenschmidt [EMAIL PROTECTED] wrote:
On Wed, 2008-05-21 at 11:41 -0700, Andrew Morton wrote:
yup, gcc bug. Discussed recently on lkml, Subject: Re: huge gcc
4.1.{0,1} __weak problem. I don't think anything ended up happening
about it though.
Hrm... do you think we should work around ? ie. move the stubs to a
separate .c file ?
istr that sticking an asm(); in the weak function was a reliable
workaround. If we are going to to that it should be via
/* comment goes here */
#define gcc_screws_up_weak_stuff() asm()
but that approach didn't seem very popular. It's a _bit_ fragile I
guess, but it's pretty easy to grep for missed workarounds.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
so what about the patch below ? I like it, but the compiler won't ;) If you're ok, I'll re-send with appropriate sob adapted powerpc part. Sure. +void __init __attribute__((weak) thread_info_cache_init(void) s/weak)/weak))/ Yeah, missing quilt ref :-) I'll send the proper patches in a minute. Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
otoh, if only one .c file will ever call this function then I think that
all problems are solved by
a) moving the above ifdeffery into the .c file
b) adding a comment explaining which arch file must provide the override
c) directly including that file from within the .c file.
I can definitely do that. I have no problem either way. I can add to all
archs too, it's just that whatever way I choose, some people won't be
happy with it :-)
Anyway, I'll move the ifdeferry to init/main.c then.
Thanks ;)
I'm still wounded by my recent encounter with set_softirq_pending()
and or_softirq_pending().
Well, looking there, I saw we already used weak symbols for that so what
about the patch below ? If you're ok, I'll re-send with appropriate sob
adapted powerpc part.
Cheers,
Ben.
Index: linux-work/init/main.c
===
--- linux-work.orig/init/main.c 2008-03-26 10:39:25.0 +1100
+++ linux-work/init/main.c 2008-04-18 13:10:35.0 +1000
@@ -504,6 +504,10 @@ void __init __attribute__((weak)) smp_se
{
}
+void __init __attribute__((weak) thread_info_cache_init(void)
+{
+}
+
asmlinkage void __init start_kernel(void)
{
char * command_line;
@@ -623,6 +627,7 @@ asmlinkage void __init start_kernel(void
if (efi_enabled)
efi_enter_virtual_mode();
#endif
+ thread_info_cache_init();
fork_init(num_physpages);
proc_caches_init();
buffer_init();
Index: linux-work/include/linux/sched.h
===
--- linux-work.orig/include/linux/sched.h 2008-04-02 09:47:56.0
+1100
+++ linux-work/include/linux/sched.h2008-04-18 13:11:10.0 +1000
@@ -1893,6 +1893,8 @@ static inline unsigned long *end_of_stac
#endif
+extern void thread_info_cache_init(void);
+
/* set thread flags in other task's structures
* - see asm/thread_info.h for TIF_ flags available
*/
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
On Fri, 18 Apr 2008 13:58:06 +1000 Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: otoh, if only one .c file will ever call this function then I think that all problems are solved by a) moving the above ifdeffery into the .c file b) adding a comment explaining which arch file must provide the override c) directly including that file from within the .c file. I can definitely do that. I have no problem either way. I can add to all archs too, it's just that whatever way I choose, some people won't be happy with it :-) Anyway, I'll move the ifdeferry to init/main.c then. Thanks ;) I'm still wounded by my recent encounter with set_softirq_pending() and or_softirq_pending(). Well, looking there, I saw we already used weak symbols for that Yes, `weak' is a nice solution. It does add a few bytes of text which we could avoid with compile-time trickery, but only a very few. Plus this is __init anyway, although I don't know how well the combination of `weak' and __init works. so what about the patch below ? I like it, but the compiler won't ;) If you're ok, I'll re-send with appropriate sob adapted powerpc part. Sure. +void __init __attribute__((weak) thread_info_cache_init(void) s/weak)/weak))/ ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
On Fri, Apr 18, 2008 at 01:58:06PM +1000, Benjamin Herrenschmidt wrote: Well, looking there, I saw we already used weak symbols for that so what about the patch below ? If you're ok, I'll re-send with appropriate sob adapted powerpc part. This is definitely the cleanest way to do this from my pov. Although I'm slightly concerned about the proliferation of weak symbols, I'm much more concerned with adding more include-order-dependent arch overrides. :) speaking with the parisc dunce-cap on, Kyle ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
On Thu, 2008-04-17 at 21:19 -0700, Andrew Morton wrote: On Fri, 18 Apr 2008 13:58:06 +1000 Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: otoh, if only one .c file will ever call this function then I think that all problems are solved by a) moving the above ifdeffery into the .c file b) adding a comment explaining which arch file must provide the override c) directly including that file from within the .c file. I can definitely do that. I have no problem either way. I can add to all archs too, it's just that whatever way I choose, some people won't be happy with it :-) Anyway, I'll move the ifdeferry to init/main.c then. Thanks ;) I'm still wounded by my recent encounter with set_softirq_pending() and or_softirq_pending(). Well, looking there, I saw we already used weak symbols for that Yes, `weak' is a nice solution. It does add a few bytes of text which we could avoid with compile-time trickery, but only a very few. Plus this is __init anyway, although I don't know how well the combination of `weak' and __init works. +void __init __attribute__((weak) thread_info_cache_init(void) s/weak)/weak))/ There's also a #define of this called __weak if you like, less typing and less ugly. cheers -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person signature.asc Description: This is a digitally signed message part ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
On Thu, 10 Apr 2008 13:22:56 +1000 Benjamin Herrenschmidt [EMAIL PROTECTED]
wrote:
Some architecture need to maintain a kmem cache for thread info
structures. (next patch adds that to powerpc to fix an alignment
problem).
There is no good arch callback to use to initialize that cache
that I can find, so this adds a new one and adds an empty macro
for when it's not implemented.
Signed-off-by: Benjamin Herrenschmidt [EMAIL PROTECTED]
---
So we have the choice here between:
- the ifdef on the func name that I did, consistent with what
I did before for iomap, which iirc Linus liked
- add some more ARCH_HAS_* or HAVE_* (yuck)
- add an empty definition to all archs .h (pain in the neck but I
can do it, though it will be an annoying patch to keep around)
- do a weak function (will slightly bloat everybody for no good reason)
So unless there is strong complaints, I'd like to stick to my
current approach.
include/linux/sched.h |4
init/main.c |1 +
2 files changed, 5 insertions(+)
--- linux-work.orig/init/main.c 2008-04-10 13:11:06.0 +1000
+++ linux-work/init/main.c2008-04-10 13:11:19.0 +1000
@@ -623,6 +623,7 @@ asmlinkage void __init start_kernel(void
if (efi_enabled)
efi_enter_virtual_mode();
#endif
+ thread_info_cache_init();
fork_init(num_physpages);
proc_caches_init();
buffer_init();
Index: linux-work/include/linux/sched.h
===
--- linux-work.orig/include/linux/sched.h 2008-04-10 13:11:44.0
+1000
+++ linux-work/include/linux/sched.h 2008-04-10 13:12:05.0 +1000
@@ -1893,6 +1893,10 @@ static inline unsigned long *end_of_stac
#endif
+#ifndef thread_info_cache_init
+#define thread_info_cache_init do { } while(0)
+#endif
This trick does cause a bit of a problem: it is undefined which arch header
file is to provide the alternative definition of thread_info_cache_init.
So we can (and have) ended up in the situation where the override appears
in different files on different architectures and various screwups ensue.
So I'd suggest that we have a bigfatcomment telling implementors which file
the override should be implemented in. And make sure that this arch file is
directly included from within sched.h.
I have a suspicion that we can still get in a mess if .c files include the
per-arch file and don't include sched.h, but I forget where this happened
and why it broke stuff.
Sigh. A nice, coded-in-C implementation within each and every architecture
remains the best implementation, and all the little tricks-to-save-typing
have failure modes.
otoh, if only one .c file will ever call this function then I think that
all problems are solved by
a) moving the above ifdeffery into the .c file
b) adding a comment explaining which arch file must provide the override
c) directly including that file from within the .c file.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
+#ifndef thread_info_cache_init
+#define thread_info_cache_init do { } while(0)
+#endif
This trick does cause a bit of a problem: it is undefined which arch header
file is to provide the alternative definition of thread_info_cache_init.
I this case it's well defined: thread_info.h. Maybe I should add a
comment ?
So we can (and have) ended up in the situation where the override appears
in different files on different architectures and various screwups ensue.
Yup.
So I'd suggest that we have a bigfatcomment telling implementors which file
the override should be implemented in. And make sure that this arch file is
directly included from within sched.h.
Will do.
I have a suspicion that we can still get in a mess if .c files include the
per-arch file and don't include sched.h, but I forget where this happened
and why it broke stuff.
In this case, there's only one call site and will only every be one, so
that shouldn't be a problem. I don't see init/main.c not including
sched.h
Sigh. A nice, coded-in-C implementation within each and every architecture
remains the best implementation, and all the little tricks-to-save-typing
have failure modes.
Well, I started doing it in all arch, and people around here told me
that was not a good idea , that it would be trouble if the prototype
ever had to change (adding an arg, etc... though very unlikely to happen
in that case, granted).
otoh, if only one .c file will ever call this function then I think that
all problems are solved by
a) moving the above ifdeffery into the .c file
b) adding a comment explaining which arch file must provide the override
c) directly including that file from within the .c file.
I can definitely do that. I have no problem either way. I can add to all
archs too, it's just that whatever way I choose, some people won't be
happy with it :-)
Anyway, I'll move the ifdeferry to init/main.c then.
Cheers,
Ben.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
On Mon, 14 Apr 2008 10:38:26 +1000 Benjamin Herrenschmidt [EMAIL PROTECTED]
wrote:
+#ifndef thread_info_cache_init
+#define thread_info_cache_init do { } while(0)
+#endif
This trick does cause a bit of a problem: it is undefined which arch header
file is to provide the alternative definition of thread_info_cache_init.
I this case it's well defined: thread_info.h. Maybe I should add a
comment ?
So we can (and have) ended up in the situation where the override appears
in different files on different architectures and various screwups ensue.
Yup.
So I'd suggest that we have a bigfatcomment telling implementors which file
the override should be implemented in. And make sure that this arch file is
directly included from within sched.h.
Will do.
I have a suspicion that we can still get in a mess if .c files include the
per-arch file and don't include sched.h, but I forget where this happened
and why it broke stuff.
In this case, there's only one call site and will only every be one, so
that shouldn't be a problem. I don't see init/main.c not including
sched.h
As long as init.c directly includes sched.h, and as long as sched.h
directly includes thread_info.h and as long as all architectures which
provide the override put it in their thread_info.h, and as long as the same
applies to all future .c users, we're good. That's a lot of as long as's ;)
Sigh. A nice, coded-in-C implementation within each and every architecture
remains the best implementation, and all the little tricks-to-save-typing
have failure modes.
Well, I started doing it in all arch, and people around here told me
that was not a good idea , that it would be trouble if the prototype
ever had to change (adding an arg, etc... though very unlikely to happen
in that case, granted).
Bah. Use of grep and basic typing skills: not so hard.
otoh, if only one .c file will ever call this function then I think that
all problems are solved by
a) moving the above ifdeffery into the .c file
b) adding a comment explaining which arch file must provide the override
c) directly including that file from within the .c file.
I can definitely do that. I have no problem either way. I can add to all
archs too, it's just that whatever way I choose, some people won't be
happy with it :-)
Anyway, I'll move the ifdeferry to init/main.c then.
Thanks ;)
I'm still wounded by my recent encounter with set_softirq_pending()
and or_softirq_pending().
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
+#ifndef thread_info_cache_init
+#define thread_info_cache_init do { } while(0)
+#endif
+
Blah ! Missing a pair of () here. Ooops. I'll send a fixed patch.
Ben.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH 1/2] Add thread_info_cache_init() to all archs
Some architecture need to maintain a kmem cache for thread info
structures. (next patch adds that to powerpc to fix an alignment
problem).
There is no good arch callback to use to initialize that cache
that I can find, so this adds a new one and adds an empty macro
for when it's not implemented.
Signed-off-by: Benjamin Herrenschmidt [EMAIL PROTECTED]
---
So we have the choice here between:
- the ifdef on the func name that I did, consistent with what
I did before for iomap, which iirc Linus liked
- add some more ARCH_HAS_* or HAVE_* (yuck)
- add an empty definition to all archs .h (pain in the neck but I
can do it, though it will be an annoying patch to keep around)
- do a weak function (will slightly bloat everybody for no good reason)
So unless there is strong complaints, I'd like to stick to my
current approach.
(This one fixes a stupid typo, missing () in the macro def.)
include/linux/sched.h |4
init/main.c |1 +
2 files changed, 5 insertions(+)
--- linux-work.orig/init/main.c 2008-04-10 13:11:06.0 +1000
+++ linux-work/init/main.c 2008-04-10 13:11:19.0 +1000
@@ -623,6 +623,7 @@ asmlinkage void __init start_kernel(void
if (efi_enabled)
efi_enter_virtual_mode();
#endif
+ thread_info_cache_init();
fork_init(num_physpages);
proc_caches_init();
buffer_init();
Index: linux-work/include/linux/sched.h
===
--- linux-work.orig/include/linux/sched.h 2008-04-10 13:11:44.0
+1000
+++ linux-work/include/linux/sched.h2008-04-11 09:35:58.0 +1000
@@ -1893,6 +1893,10 @@ static inline unsigned long *end_of_stac
#endif
+#ifndef thread_info_cache_init
+#define thread_info_cache_init() do { } while(0)
+#endif
+
/* set thread flags in other task's structures
* - see asm/thread_info.h for TIF_ flags available
*/
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH 1/2] Add thread_info_cache_init() to all archs
Some architecture need to maintain a kmem cache for thread info
structures. (next patch adds that to powerpc to fix an alignment
problem).
There is no good arch callback to use to initialize that cache
that I can find, so this adds a new one and adds an empty macro
for when it's not implemented.
Signed-off-by: Benjamin Herrenschmidt [EMAIL PROTECTED]
---
So we have the choice here between:
- the ifdef on the func name that I did, consistent with what
I did before for iomap, which iirc Linus liked
- add some more ARCH_HAS_* or HAVE_* (yuck)
- add an empty definition to all archs .h (pain in the neck but I
can do it, though it will be an annoying patch to keep around)
- do a weak function (will slightly bloat everybody for no good reason)
So unless there is strong complaints, I'd like to stick to my
current approach.
include/linux/sched.h |4
init/main.c |1 +
2 files changed, 5 insertions(+)
--- linux-work.orig/init/main.c 2008-04-10 13:11:06.0 +1000
+++ linux-work/init/main.c 2008-04-10 13:11:19.0 +1000
@@ -623,6 +623,7 @@ asmlinkage void __init start_kernel(void
if (efi_enabled)
efi_enter_virtual_mode();
#endif
+ thread_info_cache_init();
fork_init(num_physpages);
proc_caches_init();
buffer_init();
Index: linux-work/include/linux/sched.h
===
--- linux-work.orig/include/linux/sched.h 2008-04-10 13:11:44.0
+1000
+++ linux-work/include/linux/sched.h2008-04-10 13:12:05.0 +1000
@@ -1893,6 +1893,10 @@ static inline unsigned long *end_of_stac
#endif
+#ifndef thread_info_cache_init
+#define thread_info_cache_init do { } while(0)
+#endif
+
/* set thread flags in other task's structures
* - see asm/thread_info.h for TIF_ flags available
*/
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
