Re: [PATCH] __init in mm/slab.c

Re: [PATCH] __init in mm/slab.c

[Manfred Spraul]

 From the bk commit log:

>ChangeSet 1.2132, 2004/11/13 20:59:55-08:00, Andries.Brouwer@cwi.nl
>
>	[PATCH] __init in mm/slab.c
>	
>	The below removes an __initdata
>	(for initarray_generic that is referenced in non-init code).
>  
>
I think the patch is wrong and should be reverted: initarray_generic is 
referenced, but never used:

> if (g_cpucache_up == NONE) {
>         /* Note: the first kmem_cache_create must create
>          * the cache that's used by kmalloc(24), otherwise
>          * the creation of further caches will BUG().
>          */
>         cachep->array[smp_processor_id()] = &initarray_generic.cache;
>         g_cpucache_up = PARTIAL;
> } else {
>         cachep->array[smp_processor_id()] = kmalloc(sizeof(struct 
> arraycache_init),GFP_KERNEL);
> }

g_cpucache_up is NONE during bootstrap and FULL after boot. Thus the 
initarray is never accessed after boot.

Andries, why did you propose this change? Does the current code trigger 
an automatic test?

--
    Manfred

Re: [PATCH] __init in mm/slab.c

[Andries Brouwer]

On Sun, Nov 14, 2004 at 09:18:00AM +0100, Manfred Spraul wrote:

> g_cpucache_up is NONE during bootstrap and FULL after boot. Thus the 
> initarray is never accessed after boot.
> 
> Andries, why did you propose this change? Does the current code trigger 
> an automatic test?

Yes. I was a bit more explicit in the timer patch:

"The i386 timers use a struct timer_opts that has a field init
pointing at a __init function. The rest of the struct is not __init.
Nothing is wrong, but if we want to avoid having references to init stuff
in non-init sections, some reshuffling is needed."

So yesterday's series of __init patches is not because there were
bugs, but because it is desirable to have the situation where
static inspection of the object code shows absence of references
to .init stuff. Much better than having to reason that there is
a reference but that it will not be used.

Where the memory win is important the code should be rewritten a bit.

Andries

Re: [PATCH] __init in mm/slab.c

[Linus Torvalds]

On Sun, 14 Nov 2004, Andries Brouwer wrote:
>
> So yesterday's series of __init patches is not because there were
> bugs, but because it is desirable to have the situation where
> static inspection of the object code shows absence of references
> to .init stuff. Much better than having to reason that there is
> a reference but that it will not be used.

And I agree heartily with this. I love static checking (after all, that's 
all that sparse does), and if you can make sure that there is one less 
thing to be worried about, all the better.

Of course, another option to just removing/fixing the __init is to have 
some way to let the static checker know things are ok, but in this case, 
especially with fairly small data structures, it seems much easier to just 
make the checker happy.

		Linus

Re: [PATCH] __init in mm/slab.c

[Manfred Spraul]

Linus Torvalds wrote:

>On Sun, 14 Nov 2004, Andries Brouwer wrote:
>  
>
>>So yesterday's series of __init patches is not because there were
>>bugs, but because it is desirable to have the situation where
>>static inspection of the object code shows absence of references
>>to .init stuff. Much better than having to reason that there is
>>a reference but that it will not be used.
>>    
>>
>
>And I agree heartily with this. I love static checking (after all, that's 
>all that sparse does), and if you can make sure that there is one less 
>thing to be worried about, all the better.
>
>Of course, another option to just removing/fixing the __init is to have 
>some way to let the static checker know things are ok, but in this case, 
>especially with fairly small data structures, it seems much easier to just 
>make the checker happy.
>
>  
>
I agree, but a comment would have been nice. Now there are two identical 
structures that are used for the same purpose, one __init, one not __init.

I'd bet that sooner or later someone will ask why.

--
    Manfred

[PATCH] __init in mm/slab.c

[Andries Brouwer]

The below removes an __initdata
(for initarray_generic that is referenced in non-init code).

diff -uprN -X /linux/dontdiff a/include/linux/slab.h b/include/linux/slab.h
--- a/include/linux/slab.h	2004-10-30 21:44:07.000000000 +0200
+++ b/include/linux/slab.h	2004-11-13 22:40:51.000000000 +0100
@@ -13,6 +13,7 @@ typedef struct kmem_cache_s kmem_cache_t
 
 #include	<linux/config.h>	/* kmalloc_sizes.h needs CONFIG_ options */
 #include	<linux/gfp.h>
+#include	<linux/init.h>
 #include	<linux/types.h>
 #include	<asm/page.h>		/* kmalloc_sizes.h needs PAGE_SIZE */
 #include	<asm/cache.h>		/* kmalloc_sizes.h needs L1_CACHE_BYTES */
@@ -53,7 +54,7 @@ typedef struct kmem_cache_s kmem_cache_t
 #define	SLAB_CTOR_VERIFY	0x004UL		/* tell constructor it's a verify call */
 
 /* prototypes */
-extern void kmem_cache_init(void);
+extern void __init kmem_cache_init(void);
 
 extern kmem_cache_t *kmem_cache_create(const char *, size_t, size_t, unsigned long,
 				       void (*)(void *, kmem_cache_t *, unsigned long),
diff -uprN -X /linux/dontdiff a/mm/slab.c b/mm/slab.c
--- a/mm/slab.c	2004-10-30 21:44:09.000000000 +0200
+++ b/mm/slab.c	2004-11-13 22:40:51.000000000 +0100
@@ -506,7 +506,7 @@ static struct cache_names __initdata cac
 
 static struct arraycache_init initarray_cache __initdata =
 	{ { 0, BOOT_CPUCACHE_ENTRIES, 1, 0} };
-static struct arraycache_init initarray_generic __initdata =
+static struct arraycache_init initarray_generic =
 	{ { 0, BOOT_CPUCACHE_ENTRIES, 1, 0} };
 
 /* internal cache of cache description objs */