[PATCH 5/5] SRAT cleanup: make calculations and indenting level more sane

[PATCH 5/5] SRAT cleanup: make calculations and indenting level more sane

[Dave Hansen]

Using the assumption that all addresses in the SRAT are ascending,
the calculations can get a bit simpler, and remove the 
"been_here_before" variable.

This also breaks that calculation out into its own function, which
further simplifies the look of the code.

Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
---

 sparse-dave/arch/i386/kernel/srat.c |   61 ++++++++++++++++++------------------
 1 files changed, 32 insertions(+), 29 deletions(-)

diff -puN arch/i386/kernel/srat.c~A3.3-srat-cleanup arch/i386/kernel/srat.c
--- sparse/arch/i386/kernel/srat.c~A3.3-srat-cleanup	2005-02-24 08:56:41.000000000 -0800
+++ sparse-dave/arch/i386/kernel/srat.c	2005-02-24 08:56:41.000000000 -0800
@@ -181,6 +181,35 @@ static __init void chunk_to_zones(unsign
 	}
 }
 
+/*
+ * The SRAT table always lists ascending addresses, so can always
+ * assume that the first "start" address that you see is the real
+ * start of the node, and that the current "end" address is after
+ * the previous one.
+ */
+static __init void node_read_chunk(int nid, struct node_memory_chunk_s *memory_chunk)
+{
+	/*
+	 * Only add present memory as told by the e820.
+	 * There is no guarantee from the SRAT that the memory it
+	 * enumerates is present at boot time because it represents
+	 * *possible* memory hotplug areas the same as normal RAM.
+	 */
+	if (memory_chunk->start_pfn >= max_pfn) {
+		printk (KERN_INFO "Ignoring SRAT pfns: 0x%08lx -> %08lx\n",
+			memory_chunk->start_pfn, memory_chunk->end_pfn);
+		return;
+	}
+	if (memory_chunk->nid != nid)
+		return;
+
+	/* is the node currently empty? */
+	if (!node_start_pfn[nid] && !node_end_pfn[nid])
+		node_start_pfn[nid] = memory_chunk->start_pfn;
+
+	node_end_pfn[nid] = memory_chunk->end_pfn;
+}
+
 /* Parse the ACPI Static Resource Affinity Table */
 static int __init acpi20_parse_srat(struct acpi_table_srat *sratp)
 {
@@ -267,35 +296,9 @@ static int __init acpi20_parse_srat(stru
 		       node_memory_chunk[j].end_pfn);
 	}
  
-	/*calculate node_start_pfn/node_end_pfn arrays*/
-	for_each_online_node(nid) {
-		int been_here_before = 0;
-
-		for (j = 0; j < num_memory_chunks; j++){
-			/*
-			 * Only add present memroy to node_end/start_pfn
-			 * There is no guarantee from the srat that the memory
-			 * is present at boot time.
-			 */
-			if (node_memory_chunk[j].start_pfn >= max_pfn) {
-				printk (KERN_INFO "Ignoring chunk of memory reported in the SRAT (could be hot-add zone?)\n");
-				printk (KERN_INFO "chunk is reported from pfn %04x to %04x\n",
-					node_memory_chunk[j].start_pfn, node_memory_chunk[j].end_pfn);
-				continue;
-			}
-			if (node_memory_chunk[j].nid == nid) {
-				if (been_here_before == 0) {
-					node_start_pfn[nid] = node_memory_chunk[j].start_pfn;
-					node_end_pfn[nid] = node_memory_chunk[j].end_pfn;
-					been_here_before = 1;
-				} else { /* We've found another chunk of memory for the node */
-					if (node_start_pfn[nid] < node_memory_chunk[j].start_pfn) {
-						node_end_pfn[nid] = node_memory_chunk[j].end_pfn;
-					}
-				}
-			}
-		}
-	}
+	for_each_online_node(nid)
+		for (j = 0; j < num_memory_chunks; j++)
+			node_read_chunk(nid, &node_memory_chunk[j]);
 	for_each_online_node(nid) {
 		unsigned long start = node_start_pfn[nid];
 		unsigned long end = node_end_pfn[nid];
_
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH 5/5] SRAT cleanup: make calculations and indenting level more sane

[keith]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

On Thu, 2005-02-24 at 09:29, Dave Hansen wrote:
> Using the assumption that all addresses in the SRAT are ascending,
> the calculations can get a bit simpler, and remove the 
> "been_here_before" variable.
> 
> This also breaks that calculation out into its own function, which
> further simplifies the look of the code.
> 
> Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
> ---
> 
>  sparse-dave/arch/i386/kernel/srat.c |   61 ++++++++++++++++++------------------
>  1 files changed, 32 insertions(+), 29 deletions(-)

(snip)

This looks a lot better then the existing code.  Thanks. 
 
Why not take it one step further??  Something like the attached patch.
There is no reason to loop over the nodes as the srat entries contain
node info and we can use the the new node_has_online_mem. 

This booted ok on my hot-add enabled 8-way. 
 I am not %100 sure it is ok to make the assumption that the memory is
always reported linearly but that is the assumption of the previous code
so it must be for all know examples. 

Keith 


[-- Attachment #2: patch-srat-cleanup-v2 --]
[-- Type: text/x-patch, Size: 2000 bytes --]

diff -urN linux-2.6.11-rc4-fix7/arch/i386/kernel/srat.c linux-2.6.11-rc4-fix7-srat/arch/i386/kernel/srat.c
--- linux-2.6.11-rc4-fix7/arch/i386/kernel/srat.c	2005-02-21 13:56:28.000000000 -0800
+++ linux-2.6.11-rc4-fix7-srat/arch/i386/kernel/srat.c	2005-02-24 11:10:09.000000000 -0800
@@ -269,34 +269,24 @@
 	}
  
 	/*calculate node_start_pfn/node_end_pfn arrays*/
-	for_each_online_node(nid) {
-		int been_here_before = 0;
-
-		for (j = 0; j < num_memory_chunks; j++){
-			/*
-			 *Only add present memroy to node_end/start_pfn 
-			 *There is no guarantee from the srat that the memory 
-			 *is present at boot time. 
-			 */
-			if (node_memory_chunk[j].start_pfn >= max_pfn) {
-				printk (KERN_INFO "Ignoring chunk of memory reported in the SRAT (could be hot-add zone?)\n");
-				printk (KERN_INFO "chunk is reported from pfn %04x to %04x\n",
-					node_memory_chunk[j].start_pfn, node_memory_chunk[j].end_pfn);
-				continue;
-			}
-			if (node_memory_chunk[j].nid == nid) {
-				if (been_here_before == 0) {
-					node_start_pfn[nid] = node_memory_chunk[j].start_pfn;
-					node_end_pfn[nid] = node_memory_chunk[j].end_pfn;
-					been_here_before = 1;
-				} else { /* We've found another chunk of memory for the node */
-					if (node_start_pfn[nid] < node_memory_chunk[j].start_pfn) {
-						node_end_pfn[nid] = node_memory_chunk[j].end_pfn;
-					}
-				}
-			}
+	for (j = 0; j < num_memory_chunks; j++){
+		int nid = node_memory_chunk[j].nid;
+		/*
+		 *Only add present memroy to node_end/start_pfn
+		 *There is no guarantee from the srat that the memory
+		 *is present at boot time.
+		 */
+		if (node_memory_chunk[j].start_pfn >= max_pfn) {
+			printk (KERN_INFO "Ignoring SRAT pfns: 0x%08lx -> %08lx\n",
+				node_memory_chunk[j].start_pfn, node_memory_chunk[j].end_pfn);
+			continue;
 		}
+		if (!node_has_online_mem(nid))
+			node_start_pfn[nid] = node_memory_chunk[j].start_pfn;
+			
+		node_end_pfn[nid] = node_memory_chunk[j].end_pfn;
 	}
+
 	return 1;
 out_fail:
 	return 0;

Re: [PATCH 5/5] SRAT cleanup: make calculations and indenting level more sane

[Dave Hansen]

On Thu, 2005-02-24 at 11:30 -0800, keith wrote:
> Why not take it one step further??  Something like the attached patch.
> There is no reason to loop over the nodes as the srat entries contain
> node info and we can use the the new node_has_online_mem. 

You took away my function :)

Seriously, though, that does look better.  Although, I still wouldn't
mind seeing it kept broken out in another function like my patch.

> This booted ok on my hot-add enabled 8-way. 
>  I am not %100 sure it is ok to make the assumption that the memory is
> always reported linearly but that is the assumption of the previous code
> so it must be for all know examples. 

Hey James, didn't we decide at some point that the SRAT could only have
chunks with ascending addresses?

-- Dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH 5/5] SRAT cleanup: make calculations and indenting level more sane

[James Cleverdon]

No, I don't think we could rely on that.  Our BIOS did ascending 
addresses, but I don't recall that being spelled out in the ACPI spec.

Of course, there's a new ACPI spec out.  Maybe it makes it a 
requirement.  I'd take a look, but I can't afford the loss of sanity 
caused by gazing on the dread visage of ACPI 3.0.   ;^)


On Thursday 24 February 2005 11:54 am, Dave Hansen wrote:
> On Thu, 2005-02-24 at 11:30 -0800, keith wrote:
> > Why not take it one step further??  Something like the attached
> > patch. There is no reason to loop over the nodes as the srat
> > entries contain node info and we can use the the new
> > node_has_online_mem.
>
> You took away my function :)
>
> Seriously, though, that does look better.  Although, I still wouldn't
> mind seeing it kept broken out in another function like my patch.
>
> > This booted ok on my hot-add enabled 8-way.
> >  I am not %100 sure it is ok to make the assumption that the memory
> > is always reported linearly but that is the assumption of the
> > previous code so it must be for all know examples.
>
> Hey James, didn't we decide at some point that the SRAT could only
> have chunks with ascending addresses?
>
> -- Dave

-- 
James Cleverdon
IBM LTC (xSeries Linux Solutions)
{jamesclv(Unix, preferred), cleverdj(Notes)} at us dot ibm dot comm
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH 5/5] SRAT cleanup: make calculations and indenting level more sane

[keith]

On Thu, 2005-02-24 at 12:49, James Cleverdon wrote:
> No, I don't think we could rely on that.  Our BIOS did ascending 
> addresses, but I don't recall that being spelled out in the ACPI spec.
> 
> Of course, there's a new ACPI spec out.  Maybe it makes it a 
> requirement.  I'd take a look, but I can't afford the loss of sanity 
> caused by gazing on the dread visage of ACPI 3.0.   ;^)


The SRAT exists outside of the ACPI spec.  It is something made up by
folks in Kirkland.  I just reread the SRAT spec and I don't seen any
mention of requirements for linear order.  Still yet we have yet to find
a box/bios version that breaks this assumption. All I know of is the IBM
summit boxes but maybe there is something else.  

Maybe AMD x86_64 booting into 32 bit have SRATs as well?

Anyways maybe we could add some check to catch new hardware with less
friendly SRAT tables. 

after the  node_has_online_mem(nid) check 

if (node_start_pfn[nid] > node_memory_chunk[j].start_pfn) {
	printk (KERN_WARN "You need to rework the srat.c code\n");
	continue;
}

Keith 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH 5/5] SRAT cleanup: make calculations and indenting level more sane

[James Cleverdon]

Actually, SRAT was cooked up by the folks at Redmond or Bellevue.  If 
it's still outside of the main ACPI doc tree, well, that says a lot.


On Thursday 24 February 2005 02:02 pm, keith wrote:
> On Thu, 2005-02-24 at 12:49, James Cleverdon wrote:
> > No, I don't think we could rely on that.  Our BIOS did ascending
> > addresses, but I don't recall that being spelled out in the ACPI
> > spec.
> >
> > Of course, there's a new ACPI spec out.  Maybe it makes it a
> > requirement.  I'd take a look, but I can't afford the loss of
> > sanity caused by gazing on the dread visage of ACPI 3.0.   ;^)
>
> The SRAT exists outside of the ACPI spec.  It is something made up by
> folks in Kirkland.  I just reread the SRAT spec and I don't seen any
> mention of requirements for linear order.  Still yet we have yet to
> find a box/bios version that breaks this assumption. All I know of is
> the IBM summit boxes but maybe there is something else.
>
> Maybe AMD x86_64 booting into 32 bit have SRATs as well?
>
> Anyways maybe we could add some check to catch new hardware with less
> friendly SRAT tables.
>
> after the  node_has_online_mem(nid) check
>
> if (node_start_pfn[nid] > node_memory_chunk[j].start_pfn) {
> 	printk (KERN_WARN "You need to rework the srat.c code\n");
> 	continue;
> }
>
> Keith

-- 
James Cleverdon
IBM LTC (xSeries Linux Solutions)
{jamesclv(Unix, preferred), cleverdj(Notes)} at us dot ibm dot comm
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [PATCH 5/5] SRAT cleanup: make calculations and indenting level more sane

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 151 bytes --]

Keith, James,

How about this one?  I think it handles non-sequential entries, and it
uses node_has_online_mem() instead of open-coding it.  

-- Dave

[-- Attachment #2: A3.3-srat-cleanup.patch --]
[-- Type: text/x-patch, Size: 3685 bytes --]


Using the assumption that all addresses in the SRAT are ascending,
the calculations can get a bit simpler, and remove the 
"been_here_before" variable.

This also breaks that calculation out into its own function, which
further simplifies the look of the code.

Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
---

 sparse-dave/arch/i386/kernel/srat.c |   67 ++++++++++++++++++------------------
 1 files changed, 35 insertions(+), 32 deletions(-)

diff -puN arch/i386/kernel/srat.c~A3.3-srat-cleanup arch/i386/kernel/srat.c
--- sparse/arch/i386/kernel/srat.c~A3.3-srat-cleanup	2005-02-25 10:18:19.000000000 -0800
+++ sparse-dave/arch/i386/kernel/srat.c	2005-02-25 10:18:26.000000000 -0800
@@ -181,6 +181,38 @@ static __init void chunk_to_zones(unsign
 	}
 }
 
+/*
+ * The SRAT table always lists ascending addresses, so can always
+ * assume that the first "start" address that you see is the real
+ * start of the node, and that the current "end" address is after
+ * the previous one.
+ */
+static __init void node_read_chunk(int nid, struct node_memory_chunk_s *memory_chunk)
+{
+	/*
+	 * Only add present memory as told by the e820.
+	 * There is no guarantee from the SRAT that the memory it
+	 * enumerates is present at boot time because it represents
+	 * *possible* memory hotplug areas the same as normal RAM.
+	 */
+	if (memory_chunk->start_pfn >= max_pfn) {
+		printk (KERN_INFO "Ignoring SRAT pfns: 0x%08lx -> %08lx\n",
+			memory_chunk->start_pfn, memory_chunk->end_pfn);
+		return;
+	}
+	if (memory_chunk->nid != nid)
+		return;
+
+	if (!node_has_online_mem(nid))
+		node_start_pfn[nid] = memory_chunk->start_pfn;
+
+	if (node_start_pfn[nid] > memory_chunk->start_pfn)
+		node_start_pfn[nid] = memory_chunk->start_pfn;
+
+	if (node_end_pfn[nid] < memory_chunk->end_pfn)
+		node_end_pfn[nid] = memory_chunk->end_pfn;
+}
+
 /* Parse the ACPI Static Resource Affinity Table */
 static int __init acpi20_parse_srat(struct acpi_table_srat *sratp)
 {
@@ -261,41 +293,12 @@ static int __init acpi20_parse_srat(stru
 	printk("Number of memory chunks in system = %d\n", num_memory_chunks);
 
 	for (j = 0; j < num_memory_chunks; j++){
+		struct node_memory_chunk_s * chunk = &node_memory_chunk[j];
 		printk("chunk %d nid %d start_pfn %08lx end_pfn %08lx\n",
-		       j, node_memory_chunk[j].nid,
-		       node_memory_chunk[j].start_pfn,
-		       node_memory_chunk[j].end_pfn);
+		       j, chunk->nid, chunk->start_pfn, chunk->end_pfn);
+		node_read_chunk(chunk->nid, chunk);
 	}
  
-	/*calculate node_start_pfn/node_end_pfn arrays*/
-	for_each_online_node(nid) {
-		int been_here_before = 0;
-
-		for (j = 0; j < num_memory_chunks; j++){
-			/*
-			 * Only add present memroy to node_end/start_pfn
-			 * There is no guarantee from the srat that the memory
-			 * is present at boot time.
-			 */
-			if (node_memory_chunk[j].start_pfn >= max_pfn) {
-				printk (KERN_INFO "Ignoring chunk of memory reported in the SRAT (could be hot-add zone?)\n");
-				printk (KERN_INFO "chunk is reported from pfn %04x to %04x\n",
-					node_memory_chunk[j].start_pfn, node_memory_chunk[j].end_pfn);
-				continue;
-			}
-			if (node_memory_chunk[j].nid == nid) {
-				if (been_here_before == 0) {
-					node_start_pfn[nid] = node_memory_chunk[j].start_pfn;
-					node_end_pfn[nid] = node_memory_chunk[j].end_pfn;
-					been_here_before = 1;
-				} else { /* We've found another chunk of memory for the node */
-					if (node_start_pfn[nid] < node_memory_chunk[j].start_pfn) {
-						node_end_pfn[nid] = node_memory_chunk[j].end_pfn;
-					}
-				}
-			}
-		}
-	}
 	for_each_online_node(nid) {
 		unsigned long start = node_start_pfn[nid];
 		unsigned long end = node_end_pfn[nid];
_

[PATCH 5/5] SRAT cleanup: make calculations and indenting level more sane

[Dave Hansen]

Using the assumption that all addresses in the SRAT are ascending,
the calculations can get a bit simpler, and remove the 
"been_here_before" variable.

This also breaks that calculation out into its own function, which
further simplifies the look of the code.

Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
---

 sparse-dave/arch/i386/kernel/srat.c |   67 ++++++++++++++++++------------------
 1 files changed, 35 insertions(+), 32 deletions(-)

diff -puN arch/i386/kernel/srat.c~A3.3-srat-cleanup arch/i386/kernel/srat.c
--- sparse/arch/i386/kernel/srat.c~A3.3-srat-cleanup	2005-02-25 10:18:19.000000000 -0800
+++ sparse-dave/arch/i386/kernel/srat.c	2005-02-25 10:18:26.000000000 -0800
@@ -181,6 +181,38 @@ static __init void chunk_to_zones(unsign
 	}
 }
 
+/*
+ * The SRAT table always lists ascending addresses, so can always
+ * assume that the first "start" address that you see is the real
+ * start of the node, and that the current "end" address is after
+ * the previous one.
+ */
+static __init void node_read_chunk(int nid, struct node_memory_chunk_s *memory_chunk)
+{
+	/*
+	 * Only add present memory as told by the e820.
+	 * There is no guarantee from the SRAT that the memory it
+	 * enumerates is present at boot time because it represents
+	 * *possible* memory hotplug areas the same as normal RAM.
+	 */
+	if (memory_chunk->start_pfn >= max_pfn) {
+		printk (KERN_INFO "Ignoring SRAT pfns: 0x%08lx -> %08lx\n",
+			memory_chunk->start_pfn, memory_chunk->end_pfn);
+		return;
+	}
+	if (memory_chunk->nid != nid)
+		return;
+
+	if (!node_has_online_mem(nid))
+		node_start_pfn[nid] = memory_chunk->start_pfn;
+
+	if (node_start_pfn[nid] > memory_chunk->start_pfn)
+		node_start_pfn[nid] = memory_chunk->start_pfn;
+
+	if (node_end_pfn[nid] < memory_chunk->end_pfn)
+		node_end_pfn[nid] = memory_chunk->end_pfn;
+}
+
 /* Parse the ACPI Static Resource Affinity Table */
 static int __init acpi20_parse_srat(struct acpi_table_srat *sratp)
 {
@@ -261,41 +293,12 @@ static int __init acpi20_parse_srat(stru
 	printk("Number of memory chunks in system = %d\n", num_memory_chunks);
 
 	for (j = 0; j < num_memory_chunks; j++){
+		struct node_memory_chunk_s * chunk = &node_memory_chunk[j];
 		printk("chunk %d nid %d start_pfn %08lx end_pfn %08lx\n",
-		       j, node_memory_chunk[j].nid,
-		       node_memory_chunk[j].start_pfn,
-		       node_memory_chunk[j].end_pfn);
+		       j, chunk->nid, chunk->start_pfn, chunk->end_pfn);
+		node_read_chunk(chunk->nid, chunk);
 	}
  
-	/*calculate node_start_pfn/node_end_pfn arrays*/
-	for_each_online_node(nid) {
-		int been_here_before = 0;
-
-		for (j = 0; j < num_memory_chunks; j++){
-			/*
-			 * Only add present memroy to node_end/start_pfn
-			 * There is no guarantee from the srat that the memory
-			 * is present at boot time.
-			 */
-			if (node_memory_chunk[j].start_pfn >= max_pfn) {
-				printk (KERN_INFO "Ignoring chunk of memory reported in the SRAT (could be hot-add zone?)\n");
-				printk (KERN_INFO "chunk is reported from pfn %04x to %04x\n",
-					node_memory_chunk[j].start_pfn, node_memory_chunk[j].end_pfn);
-				continue;
-			}
-			if (node_memory_chunk[j].nid == nid) {
-				if (been_here_before == 0) {
-					node_start_pfn[nid] = node_memory_chunk[j].start_pfn;
-					node_end_pfn[nid] = node_memory_chunk[j].end_pfn;
-					been_here_before = 1;
-				} else { /* We've found another chunk of memory for the node */
-					if (node_start_pfn[nid] < node_memory_chunk[j].start_pfn) {
-						node_end_pfn[nid] = node_memory_chunk[j].end_pfn;
-					}
-				}
-			}
-		}
-	}
 	for_each_online_node(nid) {
 		unsigned long start = node_start_pfn[nid];
 		unsigned long end = node_end_pfn[nid];
_
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>