Re: [patch 1/6] Use EXTRA_RWDATA in architectures

[Sam Ravnborg <sam@ravnborg.org>]

Hi Mathieu
> Adds a place to declare rw data that will not be far from the .data content,
> therefore limiting the impact on cache of data declared in sections part of the
> EXTRA_RWDATA.

No comments on the aim of the path - but a few implementation comments.

I'm glad to see asm-generic/vmlinux.lds.h used - thanks.
The general naming is USAGE_{DATA|TEXT}
To keep with this albait vague then established naming scheme
please consider using: EXTRARW_DATA


> Index: linux-2.6-lttng/arch/h8300/kernel/vmlinux.lds.S
> ===================================================================
> --- linux-2.6-lttng.orig/arch/h8300/kernel/vmlinux.lds.S	2007-05-15 18:41:05.000000000 -0400
> +++ linux-2.6-lttng/arch/h8300/kernel/vmlinux.lds.S	2007-05-15 18:44:20.000000000 -0400
> @@ -105,7 +105,9 @@
>  	. = ALIGN(0x4) ;
>  		*(.data)
>  	. = ALIGN(0x4) ;
> -		*(.data.*)	
> +		*(.data.*)
> +	. = ALIGN(0x4) ;
> +	EXTRA_RWDATA

The usage of ALIGN seems a bit arbitary. If there is a requirement to align
data for EXTRA_RWDATA usage then pass alignment requirment as parameter or do
alignment in the define.


> Index: linux-2.6-lttng/arch/ia64/kernel/vmlinux.lds.S
> ===================================================================
> --- linux-2.6-lttng.orig/arch/ia64/kernel/vmlinux.lds.S	2007-05-15 18:41:05.000000000 -0400
> +++ linux-2.6-lttng/arch/ia64/kernel/vmlinux.lds.S	2007-05-15 18:44:20.000000000 -0400
> @@ -214,7 +214,7 @@
>  
>    data : { } :data
>    .data : AT(ADDR(.data) - LOAD_OFFSET)
> -	{ *(.data) *(.data1) *(.gnu.linkonce.d*) CONSTRUCTORS }
> +	{ *(.data) *(.data1) *(.gnu.linkonce.d*) EXTRA_RWDATA CONSTRUCTORS }
>  
Try to think of linker scripts a C-files.
In C you would never do like this:

-	int i; struct type foo;
+	int i; struct type foo; struct type2 bar;

You would certainly place the new variable on a new line and maybe
fix the bad code-style in another patch.
My point is that you should not continue the bad-taste style used here
but just add EXTRA_RWDATA on a single line.

If no-one beats me I plan to go through all linker scrip files and add
some sanity with respect to indent style etc. so they start to follow
C-style for indent.


> Index: linux-2.6-lttng/arch/xtensa/kernel/vmlinux.lds.S
> ===================================================================
> --- linux-2.6-lttng.orig/arch/xtensa/kernel/vmlinux.lds.S	2007-05-15 18:41:05.000000000 -0400
> +++ linux-2.6-lttng/arch/xtensa/kernel/vmlinux.lds.S	2007-05-15 18:44:20.000000000 -0400
> @@ -144,7 +144,7 @@
>    _fdata = .;
>    .data :
>    {
> -    *(.data) CONSTRUCTORS
> +    *(.data) EXTRA_RWDATA CONSTRUCTORS
>      . = ALIGN(XCHAL_ICACHE_LINESIZE);
>      *(.data.cacheline_aligned)

again..
>    }
> Index: linux-2.6-lttng/include/asm-generic/vmlinux.lds.h
> ===================================================================
> --- linux-2.6-lttng.orig/include/asm-generic/vmlinux.lds.h	2007-05-15 18:44:11.000000000 -0400
> +++ linux-2.6-lttng/include/asm-generic/vmlinux.lds.h	2007-05-15 18:44:20.000000000 -0400
> @@ -143,6 +143,8 @@
>  									\
>  	. = ALIGN(4096);
>  
> +#define EXTRA_RWDATA
> +
Please add comment that describe the purpose of EXTRA_RWDATA - verbatim
copy from changlog is almost enough.

	Sam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/