[PATCH] revert to static = {0}

[PATCH] revert to static = {0}

[Hugh Dickins]

Please revert to static zero initialization of a const: when thus
initialized it's linked into a readonly cacheline shared between cpus;
otherwise it's linked into bss, likely to be in a dirty cacheline
bouncing between cpus.

--- 2.6.0-test2-bk/mm/shmem.c	Tue Aug  5 15:57:31 2003
+++ linux/mm/shmem.c	Tue Aug  5 16:16:55 2003
@@ -296,7 +296,7 @@
 	struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
 	struct page *page = NULL;
 	swp_entry_t *entry;
-	static const swp_entry_t unswapped;
+	static const swp_entry_t unswapped = {0};
 
 	if (sgp != SGP_WRITE &&
 	    ((loff_t) index << PAGE_CACHE_SHIFT) >= i_size_read(inode))

Re: [PATCH] revert to static = {0}

[Arjan van de Ven]

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

On Tue, 2003-08-05 at 17:48, Hugh Dickins wrote:
> Please revert to static zero initialization of a const: when thus
> initialized it's linked into a readonly cacheline shared between cpus;
> otherwise it's linked into bss, likely to be in a dirty cacheline
> bouncing between cpus.

how about adding a big fat comment here that says this?
Otherwise this keeps happening all the time...

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

Re: [PATCH] revert to static = {0}

[Randy.Dunlap]

On Tue, 5 Aug 2003 16:48:02 +0100 (BST) Hugh Dickins <hugh@veritas.com> wrote:

| Please revert to static zero initialization of a const: when thus
| initialized it's linked into a readonly cacheline shared between cpus;
| otherwise it's linked into bss, likely to be in a dirty cacheline
| bouncing between cpus.
| 
| --- 2.6.0-test2-bk/mm/shmem.c	Tue Aug  5 15:57:31 2003
| +++ linux/mm/shmem.c	Tue Aug  5 16:16:55 2003
| @@ -296,7 +296,7 @@
|  	struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
|  	struct page *page = NULL;
|  	swp_entry_t *entry;
| -	static const swp_entry_t unswapped;
| +	static const swp_entry_t unswapped = {0};

In all of the "don't init statics to 0" patches, should we
check for "const" also and leave those with 0 initializers
(with explanation as Arjan requested)?

--
~Randy

Re: [PATCH] revert to static = {0}

[Lou Langholtz]

Hugh Dickins wrote:

>Please revert to static zero initialization of a const: when thus
>initialized it's linked into a readonly cacheline shared between cpus;
>otherwise it's linked into bss, likely to be in a dirty cacheline
>bouncing between cpus.
>
>--- 2.6.0-test2-bk/mm/shmem.c	Tue Aug  5 15:57:31 2003
>+++ linux/mm/shmem.c	Tue Aug  5 16:16:55 2003
>@@ -296,7 +296,7 @@
> 	struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
> 	struct page *page = NULL;
> 	swp_entry_t *entry;
>-	static const swp_entry_t unswapped;
>+	static const swp_entry_t unswapped = {0};
> 
> 	if (sgp != SGP_WRITE &&
> 	    ((loff_t) index << PAGE_CACHE_SHIFT) >= i_size_read(inode))
>  
>
If this static zero initialization makes it into the kernel distro for 
the given reason, please also add a comment sharing your above mentioned 
reasoning.

Re: [PATCH] revert to static = {0}

[Jeff Garzik]

On Tue, Aug 05, 2003 at 04:48:02PM +0100, Hugh Dickins wrote:
> Please revert to static zero initialization of a const: when thus
> initialized it's linked into a readonly cacheline shared between cpus;
> otherwise it's linked into bss, likely to be in a dirty cacheline
> bouncing between cpus.
> 
> --- 2.6.0-test2-bk/mm/shmem.c	Tue Aug  5 15:57:31 2003
> +++ linux/mm/shmem.c	Tue Aug  5 16:16:55 2003
> @@ -296,7 +296,7 @@
>  	struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
>  	struct page *page = NULL;
>  	swp_entry_t *entry;
> -	static const swp_entry_t unswapped;
> +	static const swp_entry_t unswapped = {0};
>  
>  	if (sgp != SGP_WRITE &&
>  	    ((loff_t) index << PAGE_CACHE_SHIFT) >= i_size_read(inode))
> 

If it's const, it shouldn't be linked into anything at all... right?

	Jeff

[PATCH] revert to static = {0} + comment

[Hugh Dickins]

Please revert to static zero initialization of a const: when thus
initialized it's linked into a readonly cacheline shared between cpus;
otherwise it's linked into bss, likely to be in a dirty cacheline
bouncing between cpus.

On 5 Aug 2003, Arjan van de Ven wrote:
> how about adding a big fat comment here that says this?
> Otherwise this keeps happening all the time...
and others concurred.

Okay, by popular request, here it is with a small thin comment.

--- 2.6.0-test2-bk/mm/shmem.c	Tue Aug  5 15:57:31 2003
+++ linux/mm/shmem.c	Tue Aug  5 19:29:16 2003
@@ -296,7 +296,8 @@
 	struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
 	struct page *page = NULL;
 	swp_entry_t *entry;
-	static const swp_entry_t unswapped;
+	static const swp_entry_t unswapped = {0};
+	/* = {0} to go in readonly data, to avoid bss cacheline bounce */
 
 	if (sgp != SGP_WRITE &&
 	    ((loff_t) index << PAGE_CACHE_SHIFT) >= i_size_read(inode))

Re: [PATCH] revert to static = {0}

[Hugh Dickins]

On Tue, 5 Aug 2003, Randy.Dunlap wrote:
> 
> In all of the "don't init statics to 0" patches, should we
> check for "const" also and leave those with 0 initializers
> (with explanation as Arjan requested)?

It's certainly something to consider.  This was my "= {0}" so
I know the reasoning, but I've not looked at and won't speak for others.

Hugh

Re: [PATCH] revert to static = {0}

[Hugh Dickins]

On Tue, 5 Aug 2003, Jeff Garzik wrote:
> 
> If it's const, it shouldn't be linked into anything at all... right?

Sorry, Jeff, I don't get your point.

Hugh

Re: [PATCH] revert to static = {0}

[William Lee Irwin III]

On Tue, 5 Aug 2003, Jeff Garzik wrote:
>> If it's const, it shouldn't be linked into anything at all... right?

On Tue, Aug 05, 2003 at 07:51:41PM +0100, Hugh Dickins wrote:
> Sorry, Jeff, I don't get your point.

I suspect this assumes const values will get constant folded, which I'm
not sure is the case, though it certainly sounds legal and worthwhile
for a compiler to do when reasonable (i.e. for small structures and/or
extractions of small fields of const structures).


-- wli

Re: [PATCH] revert to static = {0}

[Jeff Garzik]

William Lee Irwin III wrote:
> On Tue, 5 Aug 2003, Jeff Garzik wrote:
> 
>>>If it's const, it shouldn't be linked into anything at all... right?
> 
> 
> On Tue, Aug 05, 2003 at 07:51:41PM +0100, Hugh Dickins wrote:
> 
>>Sorry, Jeff, I don't get your point.
> 
> 
> I suspect this assumes const values will get constant folded, which I'm
> not sure is the case, though it certainly sounds legal and worthwhile
> for a compiler to do when reasonable (i.e. for small structures and/or
> extractions of small fields of const structures).


Correct.  In fact, some Linux kernel code _assumes_ the compiler will 
fold constants...

	Jeff