[patch] reduce stack usage of sanitize_e820_map

[patch] reduce stack usage of sanitize_e820_map

[Benjamin LaHaise]

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
diff -urN foo-v2.5.31/arch/i386/kernel/setup.c bar-v2.5.31/arch/i386/kernel/setup.c
--- foo-v2.5.31/arch/i386/kernel/setup.c	Mon Jun 17 15:41:13 2002
+++ bar-v2.5.31/arch/i386/kernel/setup.c	Thu Aug 15 17:47:41 2002
@@ -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;

Re: [patch] reduce stack usage of sanitize_e820_map

[Denis Vlasenko]

On 15 August 2002 19:48, Benjamin LaHaise wrote:
> Hello,
>
> Currently, sanitize_e820_map uses 0x738 bytes of stack.  The patch below
> moves the arrays into __initdata, reducing stack usage to 0x34 bytes.

Is that a real problem? sanitize_e820_map will be called just once at init
time...

> +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?

>  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;

--
vda

Re: [patch] reduce stack usage of sanitize_e820_map

[Linus Torvalds]

On Fri, 16 Aug 2002, Denis Vlasenko wrote:
> >
> > Currently, sanitize_e820_map uses 0x738 bytes of stack.  The patch below
> > moves the arrays into __initdata, reducing stack usage to 0x34 bytes.
> 
> Is that a real problem? sanitize_e820_map will 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

Re: [patch] reduce stack usage of sanitize_e820_map

[Eric W. Biederman]

Benjamin LaHaise <bcrl@redhat.com> writes:

> Hello,
> 
> Currently, sanitize_e820_map uses 0x738 bytes of stack.  The patch below 
> moves the arrays into __initdata, reducing stack usage to 0x34 bytes.

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

Re: [patch] reduce stack usage of sanitize_e820_map

[Benjamin LaHaise]

On Sat, Aug 17, 2002 at 11:18:11AM -0600, Eric W. Biederman wrote:
> Benjamin LaHaise <bcrl@redhat.com> writes:
> 
> > Hello,
> > 
> > Currently, sanitize_e820_map uses 0x738 bytes of stack.  The patch below 
> > moves the arrays into __initdata, reducing stack usage to 0x34 bytes.
> 
> 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. 

Nope.  static conflicts with __initdata.  If namespace pollution is a 
concern, just prefix them with e820_.

		-ben
-- 
"You will be reincarnated as a toad; and you will be much happier."

Re: [patch] reduce stack usage of sanitize_e820_map

[Benjamin LaHaise]

On Sat, Aug 17, 2002 at 03:17:04PM -0400, Benjamin LaHaise wrote:
> Nope.  static conflicts with __initdata.  If namespace pollution is a 
> concern, just prefix them with e820_.

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

--- foo-v2.5.31/arch/i386/kernel/setup.c	Sat Aug 17 15:31:33 2002
+++ test-v2.5.31/arch/i386/kernel/setup.c	Sat Aug 17 15:28:33 2002
@@ -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;