Re: [PATCH v2 0/6] macros for section name cleanup

[Tim Abbott]

On Fri, 1 May 2009, Sam Ravnborg wrote:

> This is the way I want to go where we have more complete
> definitions in the shared file and we try to keep the arch
> linker scripts to the arch specifc stuff.

I like the general look of this.  Indeed, I was planning to work on 
something like this as a follow-on to the linker script cleanup work I've 
done so far.

Some comments on the details are below.

> +#define PAGE_ALIGNED_DATA(page_align)                                \
> +     . = ALIGN((page_align));                                        \
> +     *(.data.page_aligned)

Why does this need an argument?  You should be able to just align to 
PAGE_SIZE.

Also, I'm not sure what you're trying to do with the double parentheses 
here (it also appears inconsistently in several other places in this 
patch).

> +/* use 0 as page_align if page_aligned data is not used */ 
> +#define RW_DATA(page_align, readmostly_align, cache_align)           \
> +     . = ALIGN(PAGE_SIZE);                                           \
> +     .data : AT(ADDR(.data) - LOAD_OFFSET) {                         \
> +             DATA_DATA                                               \
> +             CONSTRUCTORS                                            \
> +             NOSAVE_DATA                                             \
> +             PAGE_ALIGNED_DATA(page_align)                           \
> +             READMOSTLY_DATA((readmostly_align))                     \
> +             CACHELINE_ALIGNED_DATA((cache_align))                   \
> +     }

I think there are several architectures that have some other stuff in 
their .data output section (e.g. powerpc, frv, ia64), so we won't be able 
to use this on all architectures.  

Perhaps we want an intermediate macro between DATA_DATA and RW_DATA that 
is just the contents of .data here that those architectuere can use?

> +#define INITDATA(initsetup_align)					\
> +	.init.data : AT(ADDR(.init.data) - LOAD_OFFSET) {		\
> +		INIT_DATA						\
> +		. = ALIGN(initsetup_align);				\
> +               VMLINUX_SYMBOL(__setup_start) = .;                    \
> +               *(.init.setup)                                        \
[...]
> +#define INITDATA(initdata_align)					\
> +	. = ALIGN((initdata_align));					\
> +	.init.data : AT(ADDR(.init.data) - LOAD_OFFSET) {		\
> +		INIT_DATA						\
> +	}

You define two different macros called INITDATA; I'm not sure what the 
intention is here.

> +#define INIT_TASK                                                    \
> +     *(.data.init_task)
> +             
> +#define INIT_TASK_DATA(align)                                                
> +     . = ALIGN((align));                                             \
> +     .data.init_task : {                                             \
> +             INIT_TASK                                               \
> +     }
> +

You mentioned elsewhere you thought .data.init_task needs to be its own 
output section rather than part of the .data output section; why is that?  
There are several architectures on which it is part of the .data output 
section (e.g. sh, um, avr32).

Also, I think it is possible that INIT_TASK_DATA could just align to 
THREAD_SIZE rather than taking an argument.  While working on my patches 
for this I noticed there were only a couple of architectures where the 
alignment wasn't THREAD_SIZE (or a value equal to THREAD_SIZE was used).  
One exception was parisc, where the aligment is 16384 and THREAD_SIZE is 
always at least that but could be bigger in some configs where PAGE_SIZE 
is bigger.  I'm not sure whether this one exception is a bug.

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