From: Sam Ravnborg <sam@ravnborg.org>
To: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
linux-arch@vger.kernel.org
Subject: Re: [patch 1/6] Use EXTRA_RWDATA in architectures
Date: Tue, 29 May 2007 20:54:38 +0200 [thread overview]
Message-ID: <20070529185438.GB5846@uranus.ravnborg.org> (raw)
In-Reply-To: <20070529183758.910835109@polymtl.ca>
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
next prev parent reply other threads:[~2007-05-29 18:54 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-05-29 18:37 [patch 0/6] Linux Kernel Markers Mathieu Desnoyers
2007-05-29 18:37 ` [patch 1/6] Use EXTRA_RWDATA in architectures Mathieu Desnoyers
2007-05-29 18:54 ` Sam Ravnborg [this message]
2007-05-30 13:06 ` Mathieu Desnoyers
2007-05-29 18:37 ` [patch 2/6] Linux Kernel Markers, architecture independent code Mathieu Desnoyers
2007-05-29 18:37 ` [patch 3/6] Linux Kernel Markers - Hash Table Mathieu Desnoyers
2007-05-29 18:37 ` [patch 4/6] Linux Kernel Markers - Add kconfig menus for the marker code Mathieu Desnoyers
2007-05-29 18:37 ` [patch 5/6] Linux Kernel Markers - Documentation Mathieu Desnoyers
2007-05-29 18:37 ` [patch 6/6] Port of blktrace to the Linux Kernel Markers Mathieu Desnoyers
2007-05-30 12:16 ` [patch 0/6] " Mathieu Desnoyers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20070529185438.GB5846@uranus.ravnborg.org \
--to=sam@ravnborg.org \
--cc=akpm@linux-foundation.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@polymtl.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®