reduce stack usage of sanitize_e820_map
PATCH 1/1 reduce stack usage of sanitize_e820_map
Hello,
Currently, sanitize_e820_map uses 0x738 bytes of stack. The patch below moves the arrays into __initdata, reducing stack usage to 0x34 bytes.
-ben
:r ~/patches/v2.5/v2.5.31-stack-e820.diff
arch/i386/kernel/setup.c +9 -8
@@ -275,16 +275,17 @@* replaces the original e820 map with a new one, removing overlaps. * */ +struct change_member { +struct e820entry *pbios;/* pointer to original bios entry */ +unsigned long long addr;/* address for this change point */ +}; +struct change_member change_point_list[2 *E820MAX ]__initdata ; +struct change_member *change_point[2 *E820MAX ]__initdata ; +struct e820entry *overlap_list[E820MAX ]__initdata ; +struct e820entry new_bios[E820MAX ]__initdata ; +static int __init sanitize_e820_map (struct e820entry * biosmap,char * pnr_map) { -struct change_member { -struct e820entry *pbios;/* pointer to original bios entry */ -unsigned long long addr;/* address for this change point */ - }; -struct change_member change_point_list[2 *E820MAX ]; -struct change_member *change_point[2 *E820MAX ]; -struct e820entry *overlap_list[E820MAX ]; -struct e820entry new_bios[E820MAX ];struct change_member *change_tmp;unsigned long current_type, last_type;unsigned long long last_addr;
Quoted 4 lines · Benjamin LaHaise
Hello,
Currently,
sanitize_e820_mapuses0x738bytes of stack. The patch below moves the arrays into__initdata, reducing stack usage to0x34bytes.
Is that a real problem? sanitize_e820_map will be called just once at init time...
arch/i386/kernel/setup.c line 285
Quoted 4 lines
arch/i386/kernel/setup.c +4
-0+struct change_member change_point_list[2 *E820MAX ]__initdata ; +struct change_member *change_point[2 *E820MAX ]__initdata ; +struct e820entry *overlap_list[E820MAX ]__initdata ; +struct e820entry new_bios[E820MAX ]__initdata ;
Does this enlarge on-disk kernel image?
Shouldn't they be static?
arch/i386/kernel/setup.c line 291
Quoted 13 lines
static int
__initsanitize_e820_map(struct e820entry * biosmap, char *pnr_map) {arch/i386/kernel/setup.c +0
-8-struct change_member { -struct e820entry *pbios;/* pointer to original bios entry */ -unsigned long long addr;/* address for this change point */ - }; -struct change_member change_point_list[2 *E820MAX ]; -struct change_member *change_point[2 *E820MAX ]; -struct e820entry *overlap_list[E820MAX ]; -struct e820entry new_bios[E820MAX ];struct change_member *change_tmp;unsigned long current_type, last_type;unsigned long long last_addr;
--
vda
Quoted 6 lines · Denis Vlasenko
Quoted 3 lines
Currently,
sanitize_e820_mapuses0x738bytes of stack. The patch below moves the arrays into__initdata, reducing stack usage to0x34bytes.Is that a real problem?
sanitize_e820_mapwill be called just once at init time...
Well, I would actually want us to have some tools that just say "you can't do that". A flag to gcc that says "-Wstack-depth=200" that just makes gcc refuse to compile functions that have too big of a stack requirement, so that we'd see some of these things immediately.
Somebody had a script that greps the kernel disassembly for big stack changes, I suspect Ben used something like that. That's obviously equivalent, but doesn't force developers to be careful.
Of course, the ultimate thing checks what the dynamic depths are by looking at the call graph, but since you can avoid the worst stuff by just checking for static issues I htink that's worth it.
Linus
Quoted 4 lines · Benjamin LaHaise
Hello,
Currently,
sanitize_e820_mapuses0x738bytes of stack. The patch below moves the arrays into__initdata, reducing stack usage to0x34bytes.
Can we keep the arrays in sanitize_e820_map and just mark then static and __initdata? That would appear to be a cleaner solution. Polluting the global kernel name space with these is not nice.
Eric
Quoted 10 lines · Eric W. Biederman
Quoted 4 lines · Benjamin LaHaise
Hello,
Currently,
sanitize_e820_mapuses0x738bytes of stack. The patch below moves the arrays into__initdata, reducing stack usage to0x34bytes.Can we keep the arrays in
sanitize_e820_mapand just mark then static and__initdata? That would appear to be a cleaner solution. Polluting the global kernel name space with these is not nice.
Nope. static conflicts with __initdata. If namespace pollution is a concern, just prefix them with e820_.
-ben
Signature · 1 line
"You will be reincarnated as a toad; and you will be much happier."
Quoted 2 lines · Benjamin LaHaise
Nope. static conflicts with
__initdata. If namespace pollution is a concern, just prefix them withe820_.
Erm, foot-in-mouth. I'd tried adding __initdata within the function, which conflicts with the auto attribute. Making it static within the function and adding __initdata works here and has the same effect on stack usage. Linus, could you apply this? Thanks.
-ben
arch/i386/kernel/setup.c +8 -9
@@ -275,17 +275,16 @@* replaces the original e820 map with a new one, removing overlaps. * */ -struct change_member { -struct e820entry *pbios;/* pointer to original bios entry */ -unsigned long long addr;/* address for this change point */ -}; -struct change_member change_point_list[2 *E820MAX ]__initdata ; -struct change_member *change_point[2 *E820MAX ]__initdata ; -struct e820entry *overlap_list[E820MAX ]__initdata ; -struct e820entry new_bios[E820MAX ]__initdata ; -static int __init sanitize_e820_map (struct e820entry * biosmap,char * pnr_map) { +struct change_member { +struct e820entry *pbios;/* pointer to original bios entry */ +unsigned long long addr;/* address for this change point */ + }; +static struct change_member change_point_list[2 *E820MAX ]__initdata ; +static struct change_member *change_point[2 *E820MAX ]__initdata ; +static struct e820entry *overlap_list[E820MAX ]__initdata ; +static struct e820entry new_bios[E820MAX ]__initdata ;struct change_member *change_tmp;unsigned long current_type, last_type;unsigned long long last_addr;