2009-07-31 21:03:18

by Tim Abbott

[permalink] [raw]
Subject: [PATCH 1/2] alpha: use .data.init_task instead of .data.init_thread.

alpha is the only architecture that uses the section name
.data.init_thread instead of .data.init_task. So convert alpha to use
.data.init_task like everything else.

.data.init_task does not need a separate output section; this change
also moves it into the .data output section.

Signed-off-by: Tim Abbott <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: [email protected]
---
arch/alpha/kernel/init_task.c | 5 ++---
arch/alpha/kernel/vmlinux.lds.S | 7 ++-----
2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/alpha/kernel/init_task.c b/arch/alpha/kernel/init_task.c
index 19b8632..6f80ca4 100644
--- a/arch/alpha/kernel/init_task.c
+++ b/arch/alpha/kernel/init_task.c
@@ -13,6 +13,5 @@ static struct sighand_struct init_sighand = INIT_SIGHAND(init_sighand);
struct task_struct init_task = INIT_TASK(init_task);
EXPORT_SYMBOL(init_task);

-union thread_union init_thread_union
- __attribute__((section(".data.init_thread")))
- = { INIT_THREAD_INFO(init_task) };
+union thread_union init_thread_union __init_task_data =
+ { INIT_THREAD_INFO(init_task) };
diff --git a/arch/alpha/kernel/vmlinux.lds.S b/arch/alpha/kernel/vmlinux.lds.S
index b9d6568..a4bb75e 100644
--- a/arch/alpha/kernel/vmlinux.lds.S
+++ b/arch/alpha/kernel/vmlinux.lds.S
@@ -1,5 +1,6 @@
#include <asm-generic/vmlinux.lds.h>
#include <asm/page.h>
+#include <asm/thread_info.h>

OUTPUT_FORMAT("elf64-alpha")
OUTPUT_ARCH(alpha)
@@ -92,11 +93,6 @@ SECTIONS
__init_end = .;
/* Freed after init ends here */

- /* Note 2 page alignment above. */
- .data.init_thread : {
- *(.data.init_thread)
- }
-
. = ALIGN(PAGE_SIZE);
.data.page_aligned : {
*(.data.page_aligned)
@@ -110,6 +106,7 @@ SECTIONS
_data = .;
/* Data */
.data : {
+ INIT_TASK_DATA(THREAD_SIZE)
DATA_DATA
CONSTRUCTORS
}
--
1.6.3.3


2009-07-31 21:04:09

by Tim Abbott

[permalink] [raw]
Subject: [PATCH 2/2] alpha: Clean up linker script using new linker script macros.

From: Geoffrey Thomas <[email protected]>

Note that .data.page_aligned and .data.cacheline_aligned are now after
_data; it was probably a bug that they were before it.

Also, some explicit ALIGN(8)'s between various initcall sections were
removed; this should be harmless as the implicit alignment of
initcall_t was already 8.

Signed-off-by: Geoffrey Thomas <[email protected]>
Signed-off-by: Tim Abbott <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Sam Ravnborg <[email protected]>
---
arch/alpha/kernel/vmlinux.lds.S | 88 +++-----------------------------------
1 files changed, 7 insertions(+), 81 deletions(-)

diff --git a/arch/alpha/kernel/vmlinux.lds.S b/arch/alpha/kernel/vmlinux.lds.S
index a4bb75e..7836aa3 100644
--- a/arch/alpha/kernel/vmlinux.lds.S
+++ b/arch/alpha/kernel/vmlinux.lds.S
@@ -32,84 +32,19 @@ SECTIONS
} :kernel

RODATA
-
- /* Exception table */
- . = ALIGN(16);
- __ex_table : {
- __start___ex_table = .;
- *(__ex_table)
- __stop___ex_table = .;
- }
+ EXCEPTION_TABLE(16)

/* Will be freed after init */
- . = ALIGN(PAGE_SIZE);
- /* Init code and data */
- __init_begin = .;
- .init.text : {
- _sinittext = .;
- INIT_TEXT
- _einittext = .;
- }
- .init.data : {
- INIT_DATA
- }
-
- . = ALIGN(16);
- .init.setup : {
- __setup_start = .;
- *(.init.setup)
- __setup_end = .;
- }
-
- . = ALIGN(8);
- .initcall.init : {
- __initcall_start = .;
- INITCALLS
- __initcall_end = .;
- }
-
-#ifdef CONFIG_BLK_DEV_INITRD
- . = ALIGN(PAGE_SIZE);
- .init.ramfs : {
- __initramfs_start = .;
- *(.init.ramfs)
- __initramfs_end = .;
- }
-#endif
-
- . = ALIGN(8);
- .con_initcall.init : {
- __con_initcall_start = .;
- *(.con_initcall.init)
- __con_initcall_end = .;
- }
-
- . = ALIGN(8);
- SECURITY_INIT
-
+ __init_begin = ALIGN(PAGE_SIZE);
+ INIT_TEXT_SECTION(PAGE_SIZE)
+ INIT_DATA_SECTION(16)
PERCPU(PAGE_SIZE)
-
- . = ALIGN(2 * PAGE_SIZE);
+ . = ALIGN(PAGE_SIZE);
__init_end = .;
/* Freed after init ends here */

- . = ALIGN(PAGE_SIZE);
- .data.page_aligned : {
- *(.data.page_aligned)
- }
-
- . = ALIGN(64);
- .data.cacheline_aligned : {
- *(.data.cacheline_aligned)
- }
-
_data = .;
- /* Data */
- .data : {
- INIT_TASK_DATA(THREAD_SIZE)
- DATA_DATA
- CONSTRUCTORS
- }
+ RW_DATA_SECTION(64, PAGE_SIZE, THREAD_SIZE)

.got : {
*(.got)
@@ -119,16 +54,7 @@ SECTIONS
}
_edata = .; /* End of data section */

- __bss_start = .;
- .sbss : {
- *(.sbss)
- *(.scommon)
- }
- .bss : {
- *(.bss)
- *(COMMON)
- }
- __bss_stop = .;
+ BSS_SECTION(0, 0, 0)
_end = .;

/* Sections to be discarded */
--
1.6.3.3

2009-07-31 21:34:13

by Richard Henderson

[permalink] [raw]
Subject: Re: [PATCH 1/2] alpha: use .data.init_task instead of .data.init_thread.

On 07/31/2009 01:56 PM, Tim Abbott wrote:
> - /* Note 2 page alignment above. */
> - .data.init_thread : {
> - *(.data.init_thread)
> - }

NACK.

You can change the section name, sure, but you cannot remove the 2 page
alignment that we had via the alignment at the end of the init sections.
You'll break current_thread_info which is always computed as
(kernel-stack-pointer & -(2*PAGE_SIZE)).

Similarly it is *not* a bug that the page_aligned sections were before
data, because we already knew we had 2 page alignment from the end of
init + 2 pages of init_thread.


r~

2009-07-31 21:38:49

by Richard Henderson

[permalink] [raw]
Subject: Re: [PATCH 1/2] alpha: use .data.init_task instead of .data.init_thread.

On 07/31/2009 02:23 PM, Richard Henderson wrote:
> Similarly it is *not* a bug that the page_aligned sections were before
> data, because we already knew we had 2 page alignment from the end of
> init + 2 pages of init_thread.

Indeed, I'll go further and say that the common definition of
RW_DATA_SECTION is buggy.

> #define RW_DATA_SECTION(cacheline, nosave, pagealigned, inittask) \
> . = ALIGN(PAGE_SIZE); \
> .data : AT(ADDR(.data) - LOAD_OFFSET) { \
> INIT_TASK(inittask) \
> CACHELINE_ALIGNED_DATA(cacheline) \
> READ_MOSTLY_DATA(cacheline) \
> DATA_DATA \
> CONSTRUCTORS \
> NOSAVE_DATA(nosave) \
> PAGE_ALIGNED_DATA(pagealigned) \
> }

Given that we align the entire .data section why have interior padding
to re-align for page-aligned data? Surely a better ordering would be

. = ALIGN(PAGE_SIZE);
.data : AT(ADDR(.data) - LOAD_OFFSET) {
INIT_TASK(inittask)
NOSAVE_DATA
PAGE_ALIGNED_DATA(pagealigned)
CACHELINE_ALIGNED_DATA(cacheline)
READ_MOSTLY_DATA(cacheline)
DATA_DATA
CONSTRUCTORS
}

with that change, and without

> - . = ALIGN(2 * PAGE_SIZE);
> + . = ALIGN(PAGE_SIZE);
> __init_end = .;

the patch looks like it'd be ok. All you're doing with this fragment is
failing to free a page of padding. You could change it to

. = ALIGN(THREAD_SIZE)

if it makes you feel better.


r~

2009-07-31 22:02:31

by Tim Abbott

[permalink] [raw]
Subject: Re: [PATCH 1/2] alpha: use .data.init_task instead of .data.init_thread.

On Fri, 31 Jul 2009, Richard Henderson wrote:

> On 07/31/2009 01:56 PM, Tim Abbott wrote:
> > - /* Note 2 page alignment above. */
> > - .data.init_thread : {
> > - *(.data.init_thread)
> > - }
>
> NACK.
>
> You can change the section name, sure, but you cannot remove the 2 page
> alignment that we had via the alignment at the end of the init sections.
> You'll break current_thread_info which is always computed as
> (kernel-stack-pointer & -(2*PAGE_SIZE)).

The INIT_TASK_DATA(THREAD_SIZE) macro call aligns to THREAD_SIZE (=
2*PAGE_SIZE). So I'm not removing the 2 page alignment; I'm just moving
it along with the code that needs to be aligned.

This change:

- . = ALIGN(2 * PAGE_SIZE);
+ . = ALIGN(PAGE_SIZE);
__init_end = .;

removes the now-unnecessary (2 * PAGE_SIZE) alignment for __init_end
caused by moving .data.init_task (it should have been in the first patch).

-Tim Abbott

2009-07-31 23:31:05

by Richard Henderson

[permalink] [raw]
Subject: Re: [PATCH 1/2] alpha: use .data.init_task instead of .data.init_thread.

On 07/31/2009 03:02 PM, Tim Abbott wrote:
> The INIT_TASK_DATA(THREAD_SIZE) macro call aligns to THREAD_SIZE (=
> 2*PAGE_SIZE). So I'm not removing the 2 page alignment; I'm just moving
> it along with the code that needs to be aligned.

Sure.

>
> This change:
>
> - . = ALIGN(2 * PAGE_SIZE);
> + . = ALIGN(PAGE_SIZE);
> __init_end = .;
>
> removes the now-unnecessary (2 * PAGE_SIZE) alignment for __init_end
> caused by moving .data.init_task (it should have been in the first patch).

While it's technically unnecessary, it's also very much desired.

Think about it. Suppose we have 9 pages of init, followed by the
two-page-aligned INIT_TASK_DATA. So we get a page worth of padding
added. It's better to have the two-page-alignment within the init
section so as to get 10 pages of init followed by no padding. In
this way the page of padding gets freed with the rest of init.

Are you following me at all here?


r~

2009-07-31 23:44:41

by Tim Abbott

[permalink] [raw]
Subject: Re: [PATCH 1/2] alpha: use .data.init_task instead of .data.init_thread.

On Fri, 31 Jul 2009, Richard Henderson wrote:

> On 07/31/2009 02:23 PM, Richard Henderson wrote:
> > Similarly it is *not* a bug that the page_aligned sections were before
> > data, because we already knew we had 2 page alignment from the end of
> > init + 2 pages of init_thread.
>
> Indeed, I'll go further and say that the common definition of RW_DATA_SECTION
> is buggy.
[...]
> Given that we align the entire .data section why have interior padding to
> re-align for page-aligned data? Surely a better ordering would be
[...]

Agreed. I actually commented on this issue in one of the drafts of
RW_DATA_SECTION, but forgot to check again whether Sam had addressed it
when reviewing the final version. Fortunately we're talking about a small
inefficiency, not something more serious. Below is your proposed change
in patch form; I hope Sam will take a look at it.

-Tim Abbott

Optimize the ordering of sections in RW_DATA_SECTION.

The current RW_DATA_SECTION macro doesn't place the various
PAGE_SIZE-aligned sections next to each other. This could result in up to
a page of memory being wasted realigning to PAGE_SIZE twice.

Signed-off-by: Tim Abbott <[email protected]>
---
include/asm-generic/vmlinux.lds.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 6ad76bf..146815d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -706,12 +706,12 @@
. = ALIGN(PAGE_SIZE); \
.data : AT(ADDR(.data) - LOAD_OFFSET) { \
INIT_TASK_DATA(inittask) \
+ NOSAVE_DATA \
+ PAGE_ALIGNED_DATA(pagealigned) \
CACHELINE_ALIGNED_DATA(cacheline) \
READ_MOSTLY_DATA(cacheline) \
DATA_DATA \
CONSTRUCTORS \
- NOSAVE_DATA \
- PAGE_ALIGNED_DATA(pagealigned) \
}

#define INIT_TEXT_SECTION(inittext_align) \
--
1.6.3.3

2009-07-31 23:57:03

by Tim Abbott

[permalink] [raw]
Subject: Re: [PATCH 1/2] alpha: use .data.init_task instead of .data.init_thread.

On Fri, 31 Jul 2009, Richard Henderson wrote:

> On 07/31/2009 03:02 PM, Tim Abbott wrote:
> >
> > This change:
> >
> > - . = ALIGN(2 * PAGE_SIZE);
> > + . = ALIGN(PAGE_SIZE);
> > __init_end = .;
> >
> > removes the now-unnecessary (2 * PAGE_SIZE) alignment for __init_end
> > caused by moving .data.init_task (it should have been in the first patch).
>
> While it's technically unnecessary, it's also very much desired.
>
> Think about it. Suppose we have 9 pages of init, followed by the
> two-page-aligned INIT_TASK_DATA. So we get a page worth of padding
> added. It's better to have the two-page-alignment within the init
> section so as to get 10 pages of init followed by no padding. In
> this way the page of padding gets freed with the rest of init.
>
> Are you following me at all here?

Yeah, okay, that makes total sense (nice optimization). New version of
the patch below, with the alignment changed to THREAD_SIZE (and I added a
comment explaining why).

-Tim Abbott

From: Geoffrey Thomas <[email protected]>

alpha: Clean up linker script using new linker script macros.

Note that .data.page_aligned and .data.cacheline_aligned are now after
_data; it was probably a bug that they were before it.

Also, some explicit ALIGN(8)'s between various initcall sections were
removed; this should be harmless as the implicit alignment of
initcall_t was already 8.

Signed-off-by: Geoffrey Thomas <[email protected]>
Signed-off-by: Tim Abbott <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Sam Ravnborg <[email protected]>
---
arch/alpha/kernel/vmlinux.lds.S | 90 ++++-----------------------------------
1 files changed, 9 insertions(+), 81 deletions(-)

diff --git a/arch/alpha/kernel/vmlinux.lds.S b/arch/alpha/kernel/vmlinux.lds.S
index a4bb75e..a4f9001 100644
--- a/arch/alpha/kernel/vmlinux.lds.S
+++ b/arch/alpha/kernel/vmlinux.lds.S
@@ -32,84 +32,21 @@ SECTIONS
} :kernel

RODATA
-
- /* Exception table */
- . = ALIGN(16);
- __ex_table : {
- __start___ex_table = .;
- *(__ex_table)
- __stop___ex_table = .;
- }
+ EXCEPTION_TABLE(16)

/* Will be freed after init */
- . = ALIGN(PAGE_SIZE);
- /* Init code and data */
- __init_begin = .;
- .init.text : {
- _sinittext = .;
- INIT_TEXT
- _einittext = .;
- }
- .init.data : {
- INIT_DATA
- }
-
- . = ALIGN(16);
- .init.setup : {
- __setup_start = .;
- *(.init.setup)
- __setup_end = .;
- }
-
- . = ALIGN(8);
- .initcall.init : {
- __initcall_start = .;
- INITCALLS
- __initcall_end = .;
- }
-
-#ifdef CONFIG_BLK_DEV_INITRD
- . = ALIGN(PAGE_SIZE);
- .init.ramfs : {
- __initramfs_start = .;
- *(.init.ramfs)
- __initramfs_end = .;
- }
-#endif
-
- . = ALIGN(8);
- .con_initcall.init : {
- __con_initcall_start = .;
- *(.con_initcall.init)
- __con_initcall_end = .;
- }
-
- . = ALIGN(8);
- SECURITY_INIT
-
+ __init_begin = ALIGN(PAGE_SIZE);
+ INIT_TEXT_SECTION(PAGE_SIZE)
+ INIT_DATA_SECTION(16)
PERCPU(PAGE_SIZE)
-
- . = ALIGN(2 * PAGE_SIZE);
+ /* Align to THREAD_SIZE rather than PAGE_SIZE here so any padding page
+ needed for the THREAD_SIZE aligned init_task gets freed after init */
+ . = ALIGN(THREAD_SIZE);
__init_end = .;
/* Freed after init ends here */

- . = ALIGN(PAGE_SIZE);
- .data.page_aligned : {
- *(.data.page_aligned)
- }
-
- . = ALIGN(64);
- .data.cacheline_aligned : {
- *(.data.cacheline_aligned)
- }
-
_data = .;
- /* Data */
- .data : {
- INIT_TASK_DATA(THREAD_SIZE)
- DATA_DATA
- CONSTRUCTORS
- }
+ RW_DATA_SECTION(64, PAGE_SIZE, THREAD_SIZE)

.got : {
*(.got)
@@ -119,16 +56,7 @@ SECTIONS
}
_edata = .; /* End of data section */

- __bss_start = .;
- .sbss : {
- *(.sbss)
- *(.scommon)
- }
- .bss : {
- *(.bss)
- *(COMMON)
- }
- __bss_stop = .;
+ BSS_SECTION(0, 0, 0)
_end = .;

/* Sections to be discarded */
--
1.6.3.3