[PATCH 1/2] Add thread_info_cache_init() to all archs

[PATCH 1/2] Add thread_info_cache_init() to all archs

[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 <benh@kernel.crashing.org>
---

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.000000000 +1000
+++ linux-work/init/main.c	2008-04-10 13:11:19.000000000 +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.000000000 +1000
+++ linux-work/include/linux/sched.h	2008-04-11 09:35:58.000000000 +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_xxxx flags available
  */

[PATCH 1/2] Add thread_info_cache_init() to all archs

[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 <benh@kernel.crashing.org>
---

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.000000000 +1000
+++ linux-work/init/main.c	2008-04-10 13:11:19.000000000 +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.000000000 +1000
+++ linux-work/include/linux/sched.h	2008-04-10 13:12:05.000000000 +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_xxxx flags available
  */

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Benjamin Herrenschmidt]

> +#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.

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Andrew Morton]

On Thu, 10 Apr 2008 13:22:56 +1000 Benjamin Herrenschmidt <benh@ozlabs.org> 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 <benh@kernel.crashing.org>
> ---
> 
> 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.000000000 +1000
> +++ linux-work/init/main.c	2008-04-10 13:11:19.000000000 +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.000000000 +1000
> +++ linux-work/include/linux/sched.h	2008-04-10 13:12:05.000000000 +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.

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Benjamin Herrenschmidt]

> > +#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.

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Andrew Morton]

On Mon, 14 Apr 2008 10:38:26 +1000 Benjamin Herrenschmidt <benh@ozlabs.org> 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().

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Benjamin Herrenschmidt]

> > > 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.000000000 +1100
+++ linux-work/init/main.c	2008-04-18 13:10:35.000000000 +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.000000000 +1100
+++ linux-work/include/linux/sched.h	2008-04-18 13:11:10.000000000 +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_xxxx flags available
  */

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Andrew Morton]

On Fri, 18 Apr 2008 13:58:06 +1000 Benjamin Herrenschmidt <benh@ozlabs.org> 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))/

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]

On Thu, 2008-04-17 at 21:19 -0700, Andrew Morton wrote:
> On Fri, 18 Apr 2008 13:58:06 +1000 Benjamin Herrenschmidt <benh@ozlabs.org> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Benjamin Herrenschmidt]

> > 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.

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Benjamin Herrenschmidt]

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.

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Andrew Morton]

On Wed, 21 May 2008 13:56:25 -0400 Benjamin Herrenschmidt <benh@kernel.crashing.org> 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.

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Sam Ravnborg]

On Wed, May 21, 2008 at 11:41:47AM -0700, Andrew Morton wrote:
> On Wed, 21 May 2008 13:56:25 -0400 Benjamin Herrenschmidt <benh@kernel.crashing.org> 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

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Benjamin Herrenschmidt]

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.

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Benjamin Herrenschmidt]

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.

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Andrew Morton]

On Wed, 21 May 2008 15:44:41 -0400
Benjamin Herrenschmidt <benh@kernel.crashing.org> 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.

Re: [PATCH 1/2] Add thread_info_cache_init() to all archs

[Kyle McMartin]

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