Add thread_info_cache_init() to all archs
Subsystems (2)
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.
---
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(+)
init/main.c +1 -0
@@ -623,6 +623,7 @@ asmlinkage void __init start_kernel(voidif (efi_enabled)efi_enter_virtual_mode ();#endif +thread_info_cache_init ();fork_init (num_physpages);proc_caches_init ();buffer_init ();
include/linux/sched.h +4 -0
@@ -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_xxxx flags available */
include/linux/sched.h static inline unsigned long *end_of_stac · line 1899
Quoted 5 lines
include/linux/sched.h +4
-0+#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.
Quoted 45 lines · On Thu, 10 Apr 2008 13:22:56 +1000 Benjamin Herrenschmidt…
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(+)init/main.c +1
-0@@ -623,6 +623,7 @@ asmlinkage void __init start_kernel(voidif (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
===================================================================
include/linux/sched.h static inline unsigned long *end_of_stac · line 1898
Quoted 6 lines
include/linux/sched.h +3
-0@@ -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.
Quoted 6 lines
include/linux/sched.h static inline unsigned long *end_of_stac · line 1898
Quoted 3 lines
include/linux/sched.h +3
-0+#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 ?
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 3 lines
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
Quoted 3 lines
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).
Quoted 6 lines
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.
Quoted 21 lines · On Mon, 14 Apr 2008 10:38:26 +1000 Benjamin Herrenschmidt…
Quoted 6 lines
include/linux/sched.h static inline unsigned long *end_of_stac · line 1898
Quoted 3 lines
include/linux/sched.h +3
-0+#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 ?Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 7 lines
Quoted 3 lines
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.cnot includingsched.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 ;)
Quoted 8 lines
Quoted 3 lines
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.
Quoted 7 lines
Quoted 6 lines
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.
Quoted 5 lines
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.cthen.
Thanks ;)
I'm still wounded by my recent encounter with set_softirq_pending() and or_softirq_pending().
Quoted 12 lines
Quoted 12 lines
Quoted 7 lines
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 :-)
Quoted 6 lines
Quoted 1 line
Anyway, I'll move the ifdeferry to
init/main.cthen.Thanks ;)
I'm still wounded by my recent encounter with
set_softirq_pending()andor_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.
init/main.c +5 -0
@@ -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(voidif (efi_enabled)efi_enter_virtual_mode ();#endif +thread_info_cache_init ();fork_init (num_physpages);proc_caches_init ();buffer_init ();
include/linux/sched.h +2 -0
@@ -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_xxxx flags available */
Quoted 12 lines · On Fri, 18 Apr 2008 13:58:06 +1000 Benjamin Herrenschmidt…
Quoted 12 lines
Quoted 12 lines
Quoted 7 lines
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 :-)
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Anyway, I'll move the ifdeferry to
init/main.cthen.Thanks ;)
I'm still wounded by my recent encounter with
set_softirq_pending()andor_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.
Quoted 2 lines
so what
about the patch below ?
I like it, but the compiler won't ;)
Quoted 2 lines
If you're ok, I'll re-send with appropriate sob
& adapted powerpc part.
Sure.
Quoted 1 line
+void
__init__attribute__((weak)thread_info_cache_init(void)
s/weak)/weak))/
Quoted 26 lines · Andrew Morton
Quoted 12 lines · On Fri, 18 Apr 2008 13:58:06 +1000 Benjamin Herrenschmidt…
Quoted 12 lines
Quoted 12 lines
Quoted 7 lines
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 :-)
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Anyway, I'll move the ifdeferry to
init/main.cthen.Thanks ;)
I'm still wounded by my recent encounter with
set_softirq_pending()andor_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.
Quoted 6 lines
Plus this is
__initanyway, although I don't know how well the combination of `weak' and__initworks.Quoted 1 line
+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
Signature · 8 lines
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
Quoted 6 lines
Quoted 3 lines
so what
about the patch below ?I like it, but the compiler won't ;)
Quoted 8 lines
Quoted 2 lines
If you're ok, I'll re-send with appropriate sob
& adapted powerpc part.Sure.
Quoted 1 line
+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.
Quoted 5 lines · Benjamin Herrenschmidt
Quoted 5 lines
Quoted 2 lines
so what
about the patch below ?I like it, but the compiler won't ;)
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
If you're ok, I'll re-send with appropriate sob
& adapted powerpc part.Sure.
Quoted 1 line
+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.
Quoted 24 lines · On Wed, 21 May 2008 13:56:25 -0400 Benjamin Herrenschmidt…
Quoted 5 lines · Benjamin Herrenschmidt
Quoted 5 lines
Quoted 2 lines
so what
about the patch below ?I like it, but the compiler won't ;)
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
If you're ok, I'll re-send with appropriate sob
& adapted powerpc part.Sure.
Quoted 1 line
+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..
Quoted 8 lines
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.
Quoted 30 lines · Andrew Morton
Quoted 28 lines · On Wed, 21 May 2008 13:56:25 -0400 Benjamin Herrenschmidt…
Quoted 5 lines · Benjamin Herrenschmidt
Quoted 5 lines
Quoted 2 lines
so what
about the patch below ?I like it, but the compiler won't ;)
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
If you're ok, I'll re-send with appropriate sob
& adapted powerpc part.Sure.
Quoted 1 line
+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.
Quoted 8 lines
Quoted 4 lines
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}
__weakproblem". 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
Quoted 8 lines · Sam Ravnborg
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.
Quoted 4 lines · Andrew Morton
yup, gcc bug. Discussed recently on lkml, "Subject: Re: huge gcc 4.1.{0,1}
__weakproblem". 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.
Quoted 10 lines · On Wed, 21 May 2008 15:44:41 -0400 Benjamin Herrenschmidt…
Quoted 4 lines · Andrew Morton
yup, gcc bug. Discussed recently on lkml, "Subject: Re: huge gcc 4.1.{0,1}
__weakproblem". 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.
Quoted 4 lines · Benjamin Herrenschmidt
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