Messages in this thread Patch in this message | | | Date | Tue, 29 May 2007 20:54:38 +0200 | From | Sam Ravnborg <> | Subject | Re: [patch 1/6] Use EXTRA_RWDATA in architectures |
| |
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/
| |