[PATCH] pseries: phyp dump: Variable size reserve space.

[PATCH] pseries: phyp dump: Variable size reserve space.

[Manish Ahuja]

A small proposed change in the amount of reserve space we allocate during boot.
Currently we reserve 256MB only. 
The proposed change does one of the 3 things.

A. It checks to see if there is cmdline variable set and if found sets the
   value to it. OR
B. It computes 5% of total ram and rounds it down to multiples of 256MB. AND
C. Compares the rounded down value and returns larger of two values, the new
   computed value or 256MB.

Again this is for large systems who have excess memory. 

Signed-off-by: Manish Ahuja <mahuja@us.ibm.com>

---
 arch/powerpc/kernel/prom.c                 |   35 +++++++++++++++++++++++++++--
 arch/powerpc/platforms/pseries/phyp_dump.c |    9 +++++++
 include/asm-powerpc/phyp_dump.h            |    4 ++-
 3 files changed, 45 insertions(+), 3 deletions(-)

Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c
===================================================================
--- 2.6.25-rc1.orig/arch/powerpc/platforms/pseries/phyp_dump.c	2008-04-02 23:36:51.000000000 -0500
+++ 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c	2008-04-02 23:48:34.000000000 -0500
@@ -496,3 +496,12 @@ static int __init early_phyp_dump_enable
 }
 early_param("phyp_dump", early_phyp_dump_enabled);
 
+/* Look for phyp_dump_reserve_size= cmdline option */
+static int __init early_phyp_dump_reserve_size(char *p)
+{
+        if (p)
+		phyp_dump_info->phyp_dump_reserve_bootvar = memparse(p, &p);
+
+        return 0;
+}
+early_param("phyp_dump_reserve_size", early_phyp_dump_reserve_size);
Index: 2.6.25-rc1/include/asm-powerpc/phyp_dump.h
===================================================================
--- 2.6.25-rc1.orig/include/asm-powerpc/phyp_dump.h	2008-04-02 23:36:49.000000000 -0500
+++ 2.6.25-rc1/include/asm-powerpc/phyp_dump.h	2008-04-02 23:37:57.000000000 -0500
@@ -24,8 +24,10 @@ struct phyp_dump {
 	/* Memory that is reserved during very early boot. */
 	unsigned long init_reserve_start;
 	unsigned long init_reserve_size;
-	/* Check status during boot if dump supported, active & present*/
+	/* cmd line options during boot */
+	unsigned long phyp_dump_reserve_bootvar;
 	unsigned long phyp_dump_at_boot;
+	/* Check status during boot if dump supported, active & present*/
 	unsigned long phyp_dump_configured;
 	unsigned long phyp_dump_is_active;
 	/* store cpu & hpte size */
Index: 2.6.25-rc1/arch/powerpc/kernel/prom.c
===================================================================
--- 2.6.25-rc1.orig/arch/powerpc/kernel/prom.c	2008-04-02 23:36:49.000000000 -0500
+++ 2.6.25-rc1/arch/powerpc/kernel/prom.c	2008-04-08 00:01:34.000000000 -0500
@@ -1042,6 +1042,33 @@ static void __init early_reserve_mem(voi
 
 #ifdef CONFIG_PHYP_DUMP
 /**
+ * phyp_dump_calculate_reserve_size() - reserve variable boot area 5% or arg
+ *
+ * Function to find the largest size we need to reserve
+ * during early boot process.
+ *
+ * It either looks for boot param and returns that OR
+ * returns larger of 256 or 5% rounded down to multiples of 256MB.
+ *
+ */
+static inline unsigned long phyp_dump_calculate_reserve_size(void)
+{
+	unsigned long tmp;
+
+	if (phyp_dump_info->phyp_dump_reserve_bootvar)
+		return phyp_dump_info->phyp_dump_reserve_bootvar;
+
+	/* divide by 20 to get 5% of value */
+	tmp = lmb_end_of_DRAM();
+	do_div(tmp, 20);
+
+	/* round it down in multiples of 256 */
+	tmp = tmp & ~0x000000001FFFFFFF;
+
+	return (tmp > PHYP_DUMP_RMR_END ? tmp : PHYP_DUMP_RMR_END);
+}
+
+/**
  * phyp_dump_reserve_mem() - reserve all not-yet-dumped mmemory
  *
  * This routine may reserve memory regions in the kernel only
@@ -1054,6 +1081,8 @@ static void __init early_reserve_mem(voi
 static void __init phyp_dump_reserve_mem(void)
 {
 	unsigned long base, size;
+	unsigned long variable_reserve_size;
+
 	if (!phyp_dump_info->phyp_dump_configured) {
 		printk(KERN_ERR "Phyp-dump not supported on this hardware\n");
 		return;
@@ -1064,9 +1093,11 @@ static void __init phyp_dump_reserve_mem
 		return;
 	}
 
+	variable_reserve_size = phyp_dump_calculate_reserve_size();
+
 	if (phyp_dump_info->phyp_dump_is_active) {
 		/* Reserve *everything* above RMR.Area freed by userland tools*/
-		base = PHYP_DUMP_RMR_END;
+		base = variable_reserve_size;
 		size = lmb_end_of_DRAM() - base;
 
 		/* XXX crashed_ram_end is wrong, since it may be beyond
@@ -1078,7 +1109,7 @@ static void __init phyp_dump_reserve_mem
 	} else {
 		size = phyp_dump_info->cpu_state_size +
 			phyp_dump_info->hpte_region_size +
-			PHYP_DUMP_RMR_END;
+			variable_reserve_size;
 		base = lmb_end_of_DRAM() - size;
 		lmb_reserve(base, size);
 		phyp_dump_info->init_reserve_start = base;

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Olof Johansson]

Hi,

Just a few nitpicks, no comments on the functional parts:

On Mon, Apr 07, 2008 at 06:45:37PM -0500, Manish Ahuja wrote:
> A small proposed change in the amount of reserve space we allocate during boot.
> Currently we reserve 256MB only. 
> The proposed change does one of the 3 things.
> 
> A. It checks to see if there is cmdline variable set and if found sets the
>    value to it. OR
> B. It computes 5% of total ram and rounds it down to multiples of 256MB. AND
> C. Compares the rounded down value and returns larger of two values, the new
>    computed value or 256MB.

...

> +/* Look for phyp_dump_reserve_size= cmdline option */
> +static int __init early_phyp_dump_reserve_size(char *p)
> +{
> +        if (p)
> +		phyp_dump_info->phyp_dump_reserve_bootvar = memparse(p, &p);
[...]
> @@ -24,8 +24,10 @@ struct phyp_dump {
>  	/* Memory that is reserved during very early boot. */
>  	unsigned long init_reserve_start;
>  	unsigned long init_reserve_size;
> -	/* Check status during boot if dump supported, active & present*/
> +	/* cmd line options during boot */
> +	unsigned long phyp_dump_reserve_bootvar;
>  	unsigned long phyp_dump_at_boot;
> +	/* Check status during boot if dump supported, active & present*/
>  	unsigned long phyp_dump_configured;
>  	unsigned long phyp_dump_is_active;
>  	/* store cpu & hpte size */

These make for some really long variable names and lines. I know from
experience, since I've picked unneccessary long driver names in the past
myself. :)

How about just naming the new variables reserve_bootvar, etc? The name
of the struct they're in makes it obvious what they're for.


> +static inline unsigned long phyp_dump_calculate_reserve_size(void)
> +{
> +	unsigned long tmp;
> +
> +	if (phyp_dump_info->phyp_dump_reserve_bootvar)
> +		return phyp_dump_info->phyp_dump_reserve_bootvar;
> +
> +	/* divide by 20 to get 5% of value */
> +	tmp = lmb_end_of_DRAM();
> +	do_div(tmp, 20);
> +
> +	/* round it down in multiples of 256 */
> +	tmp = tmp & ~0x000000001FFFFFFF;

That's 512MB, isn't it?


-Olof

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Manish Ahuja]

Olof Johansson wrote:
> These make for some really long variable names and lines. I know from
> experience, since I've picked unneccessary long driver names in the past
> myself. :)
> 
> How about just naming the new variables reserve_bootvar, etc? The name
> of the struct they're in makes it obvious what they're for.
> 

Yeah, I guess thats a good suggestion. Will truncate it.

> 
>> +static inline unsigned long phyp_dump_calculate_reserve_size(void)
>> +{
>> +	unsigned long tmp;
>> +
>> +	if (phyp_dump_info->phyp_dump_reserve_bootvar)
>> +		return phyp_dump_info->phyp_dump_reserve_bootvar;
>> +
>> +	/* divide by 20 to get 5% of value */
>> +	tmp = lmb_end_of_DRAM();
>> +	do_div(tmp, 20);
>> +
>> +	/* round it down in multiples of 256 */
>> +	tmp = tmp & ~0x000000001FFFFFFF;
> 
> That's 512MB, isn't it?
> 

No, its 5 % of memory and then rounded down to 256 MB multiples.
so if you 4GB its 256MB.
if you have 8 GB its 512 MB etc.

> 
> -Olof

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Manish Ahuja]

Olof Johansson wrote:
>> +static inline unsigned long phyp_dump_calculate_reserve_size(void)
>> +{
>> +	unsigned long tmp;
>> +
>> +	if (phyp_dump_info->phyp_dump_reserve_bootvar)
>> +		return phyp_dump_info->phyp_dump_reserve_bootvar;
>> +
>> +	/* divide by 20 to get 5% of value */
>> +	tmp = lmb_end_of_DRAM();
>> +	do_div(tmp, 20);
>> +
>> +	/* round it down in multiples of 256 */
>> +	tmp = tmp & ~0x000000001FFFFFFF;
> 
> That's 512MB, isn't it?

My calculations in the example I gave in the last email were wrong.

In mentally did 10% instead of 5%. But the premise is same.

So assuming 5% of some memory is 400 MB, it rounds it down to 256MB etc.

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Olof Johansson]

On Wed, Apr 09, 2008 at 12:37:51PM -0500, Manish Ahuja wrote:
> Olof Johansson wrote:
> >> +static inline unsigned long phyp_dump_calculate_reserve_size(void)
> >> +{
> >> +	unsigned long tmp;
> >> +
> >> +	if (phyp_dump_info->phyp_dump_reserve_bootvar)
> >> +		return phyp_dump_info->phyp_dump_reserve_bootvar;
> >> +
> >> +	/* divide by 20 to get 5% of value */
> >> +	tmp = lmb_end_of_DRAM();
> >> +	do_div(tmp, 20);
> >> +
> >> +	/* round it down in multiples of 256 */
> >> +	tmp = tmp & ~0x000000001FFFFFFF;
> > 
> > That's 512MB, isn't it?
> 
> My calculations in the example I gave in the last email were wrong.
> 
> In mentally did 10% instead of 5%. But the premise is same.
> 
> So assuming 5% of some memory is 400 MB, it rounds it down to 256MB etc.

But 0x1fffffff is 512MB, not 256MB. So you're rounding it down to a
multiple of 512MB.


-Olof

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Manish Ahuja]

Hmmm,

You are possibly right.

Okay I can check and fix that.

-Manish

Olof Johansson wrote:
>>> That's 512MB, isn't it?
>> My calculations in the example I gave in the last email were wrong.
>>
>> In mentally did 10% instead of 5%. But the premise is same.
>>
>> So assuming 5% of some memory is 400 MB, it rounds it down to 256MB etc.
> 
> But 0x1fffffff is 512MB, not 256MB. So you're rounding it down to a
> multiple of 512MB.
> 
> 
> -Olof

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Olof Johansson]

On Wed, Apr 09, 2008 at 01:43:51PM -0500, Manish Ahuja wrote:
> Hmmm,
> 
> You are possibly right.
> 
> Okay I can check and fix that.

Well, fix the comments if you prefer, I just pointed out the
discreptancy.


-Olof

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Segher Boessenkool]

>>> +	tmp = tmp & ~0x000000001FFFFFFF;

Note that this only works as you expect because the constant is
signed here -- the extra zeroes do not magically make it a 64-bit
number.  So it goes 32-bit 0x1fffffff -> 32-bit -0x20000000 ->
64-bit -0x20000000.

Please consider writing it with an "L" suffix, or "UL" even, to
reduce trickiness and surprises if ever that number is changed.


Segher

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Paul Mackerras]

Segher Boessenkool writes:

> >>> +	tmp = tmp & ~0x000000001FFFFFFF;
> 
> Note that this only works as you expect because the constant is
> signed here -- the extra zeroes do not magically make it a 64-bit
> number.  So it goes 32-bit 0x1fffffff -> 32-bit -0x20000000 ->
> 64-bit -0x20000000.

Huh?  It's not big enough to be negative when considered as a 32-bit
number...

But yes, adding a UL would be a good idea.

Paul.

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Segher Boessenkool]

>>>>> +	tmp = tmp & ~0x000000001FFFFFFF;
>>
>> Note that this only works as you expect because the constant is
>> signed here -- the extra zeroes do not magically make it a 64-bit
>> number.  So it goes 32-bit 0x1fffffff -> 32-bit -0x20000000 ->
>> 64-bit -0x20000000.
>
> Huh?  It's not big enough to be negative when considered as a 32-bit
> number...

0x1fffffff isn't, right; and that makes it a signed int.  Applying
the complement operator to it turns it negative.  And then the
implicit cast (because tmp is 64-bit) makes it 64-bit, still the
same negative value.  Which is the expected result, but via a way
too tricky (and fragile) path :-)


Segher

[PATCH] pseries: phyp dump: Variable size reserve space.

[Manish Ahuja]

Reposting patch with following changes:
1. Changed phyp_dump_reserve_bootvar to just reserve_bootvar.
2. Changed 0x00000001fffffff to 0x0fffffffUL.

Paulus,
If you think this is okay can you send this upstream ?
Many thanks,
Manish

A small proposed change in the amount of reserve space we allocate during boot.
Currently we reserve 256MB only. 
The proposed change does one of the 3 things.

A. It checks to see if there is boot variable set and if found sets the
   value to it.
B. It computers 5% of total ram and rounds it down to multiples of 256MB.
C. Compares the rounded down value and returns larger of 256MB or the new
   computed value.

Again this is for large systems who have excess memory. 

Signed-off-by: Manish Ahuja <mahuja@us.ibm.com>

---
 arch/powerpc/kernel/prom.c                 |   35 +++++++++++++++++++++++++++--
 arch/powerpc/platforms/pseries/phyp_dump.c |    9 +++++++
 include/asm-powerpc/phyp_dump.h            |    4 ++-
 3 files changed, 45 insertions(+), 3 deletions(-)

Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c
===================================================================
--- 2.6.25-rc1.orig/arch/powerpc/platforms/pseries/phyp_dump.c	2008-04-02 23:36:51.000000000 -0500
+++ 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c	2008-04-11 23:54:34.000000000 -0500
@@ -496,3 +496,12 @@ static int __init early_phyp_dump_enable
 }
 early_param("phyp_dump", early_phyp_dump_enabled);
 
+/* Look for phyp_dump_reserve_size= cmdline option */
+static int __init early_phyp_dump_reserve_size(char *p)
+{
+        if (p)
+		phyp_dump_info->reserve_bootvar = memparse(p, &p);
+
+        return 0;
+}
+early_param("phyp_dump_reserve_size", early_phyp_dump_reserve_size);
Index: 2.6.25-rc1/include/asm-powerpc/phyp_dump.h
===================================================================
--- 2.6.25-rc1.orig/include/asm-powerpc/phyp_dump.h	2008-04-02 23:36:49.000000000 -0500
+++ 2.6.25-rc1/include/asm-powerpc/phyp_dump.h	2008-04-11 23:53:10.000000000 -0500
@@ -24,8 +24,10 @@ struct phyp_dump {
 	/* Memory that is reserved during very early boot. */
 	unsigned long init_reserve_start;
 	unsigned long init_reserve_size;
-	/* Check status during boot if dump supported, active & present*/
+	/* cmd line options during boot */
+	unsigned long reserve_bootvar;
 	unsigned long phyp_dump_at_boot;
+	/* Check status during boot if dump supported, active & present*/
 	unsigned long phyp_dump_configured;
 	unsigned long phyp_dump_is_active;
 	/* store cpu & hpte size */
Index: 2.6.25-rc1/arch/powerpc/kernel/prom.c
===================================================================
--- 2.6.25-rc1.orig/arch/powerpc/kernel/prom.c	2008-04-02 23:36:49.000000000 -0500
+++ 2.6.25-rc1/arch/powerpc/kernel/prom.c	2008-04-11 23:53:48.000000000 -0500
@@ -1042,6 +1042,33 @@ static void __init early_reserve_mem(voi
 
 #ifdef CONFIG_PHYP_DUMP
 /**
+ * phyp_dump_calculate_reserve_size() - reserve variable boot area 5% or arg
+ *
+ * Function to find the largest size we need to reserve
+ * during early boot process.
+ *
+ * It either looks for boot param and returns that OR
+ * returns larger of 256 or 5% rounded down to multiples of 256MB.
+ *
+ */
+static inline unsigned long phyp_dump_calculate_reserve_size(void)
+{
+	unsigned long tmp;
+
+	if (phyp_dump_info->reserve_bootvar)
+		return phyp_dump_info->reserve_bootvar;
+
+	/* divide by 20 to get 5% of value */
+	tmp = lmb_end_of_DRAM();
+	do_div(tmp, 20);
+
+	/* round it down in multiples of 256 */
+	tmp = tmp & ~0x0FFFFFFFUL;
+
+	return (tmp > PHYP_DUMP_RMR_END ? tmp : PHYP_DUMP_RMR_END);
+}
+
+/**
  * phyp_dump_reserve_mem() - reserve all not-yet-dumped mmemory
  *
  * This routine may reserve memory regions in the kernel only
@@ -1054,6 +1081,8 @@ static void __init early_reserve_mem(voi
 static void __init phyp_dump_reserve_mem(void)
 {
 	unsigned long base, size;
+	unsigned long variable_reserve_size;
+
 	if (!phyp_dump_info->phyp_dump_configured) {
 		printk(KERN_ERR "Phyp-dump not supported on this hardware\n");
 		return;
@@ -1064,9 +1093,11 @@ static void __init phyp_dump_reserve_mem
 		return;
 	}
 
+	variable_reserve_size = phyp_dump_calculate_reserve_size();
+
 	if (phyp_dump_info->phyp_dump_is_active) {
 		/* Reserve *everything* above RMR.Area freed by userland tools*/
-		base = PHYP_DUMP_RMR_END;
+		base = variable_reserve_size;
 		size = lmb_end_of_DRAM() - base;
 
 		/* XXX crashed_ram_end is wrong, since it may be beyond
@@ -1078,7 +1109,7 @@ static void __init phyp_dump_reserve_mem
 	} else {
 		size = phyp_dump_info->cpu_state_size +
 			phyp_dump_info->hpte_region_size +
-			PHYP_DUMP_RMR_END;
+			variable_reserve_size;
 		base = lmb_end_of_DRAM() - size;
 		lmb_reserve(base, size);
 		phyp_dump_info->init_reserve_start = base;

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Paul Mackerras]

Manish Ahuja writes:

> B. It computers 5% of total ram and rounds it down to multiples of 256MB.
> C. Compares the rounded down value and returns larger of 256MB or the new
>    computed value.

So if we have 10GB of memory or more we'll use reserve more than
256MB.  What is the advantage of reserving more than 256MB of memory?

Paul.

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Manish Ahuja]

Paul,

The aim is to have more flex space for the kernel on machines with more resources. Although the dump will be collected pretty fast and the memory released really early on allowing the machine to have the full memory available, this alleviates any issues that can be caused by having way too little memory on very very large systems during those few minutes.

-Manish



Paul Mackerras wrote:
> Manish Ahuja writes:
> 
>> B. It computers 5% of total ram and rounds it down to multiples of 256MB.
>> C. Compares the rounded down value and returns larger of 256MB or the new
>>    computed value.
> 
> So if we have 10GB of memory or more we'll use reserve more than
> 256MB.  What is the advantage of reserving more than 256MB of memory?
> 
> Paul.

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Joel Schopp]

> The aim is to have more flex space for the kernel on machines with more resources. Although the dump will be collected pretty fast and the memory released really early on allowing the machine to have the full memory available, this alleviates any issues that can be caused by having way too little memory on very very large systems during those few minutes.
>
> -Manish
I think this would be an issue for distro kernels that have minimum 
requirements for memory above 256MB.  It seems like a reasonable attempt 
to have good defaults.  The user can always override it with boot args.  
I'm not sure where the exact numbers should be but the general statement 
that larger memory systems should have more memory to boot with seems 
like a good one.

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Linas Vepstas]

On 07/04/2008, Manish Ahuja <ahuja@austin.ibm.com> wrote:
> A small proposed change in the amount of reserve space we allocate during boot.
>  Currently we reserve 256MB only.
>  The proposed change does one of the 3 things.
>
>  A. It checks to see if there is cmdline variable set and if found sets the
>    value to it. OR
>  B. It computes 5% of total ram and rounds it down to multiples of 256MB. AND
>  C. Compares the rounded down value and returns larger of two values, the new
>    computed value or 256MB.
>
>  Again this is for large systems who have excess memory.
>
[...]
>   early_param("phyp_dump", early_phyp_dump_enabled);

I'm pretty sure you will want to document this boot param in the documentation,
as well as add a few words about why it might be interesting to users (i.e.
that its for large systems...)

--linas

Re: [PATCH] pseries: phyp dump: Variable size reserve space.

[Manish Ahuja]

Yeah, that makes sense, I will shortly send a documentation patch for all the boot vars
that I have added.

Thanks for reminding.

-Manish




Linas Vepstas wrote:
> On 07/04/2008, Manish Ahuja <ahuja@austin.ibm.com> wrote:
>> A small proposed change in the amount of reserve space we allocate during boot.
>>  Currently we reserve 256MB only.
>>  The proposed change does one of the 3 things.
>>
>>  A. It checks to see if there is cmdline variable set and if found sets the
>>    value to it. OR
>>  B. It computes 5% of total ram and rounds it down to multiples of 256MB. AND
>>  C. Compares the rounded down value and returns larger of two values, the new
>>    computed value or 256MB.
>>
>>  Again this is for large systems who have excess memory.
>>
> [...]
>>   early_param("phyp_dump", early_phyp_dump_enabled);
> 
> I'm pretty sure you will want to document this boot param in the documentation,
> as well as add a few words about why it might be interesting to users (i.e.
> that its for large systems...)
> 
> --linas