[PATCH] x86: fix nodes_cover_memory

[PATCH] x86: fix nodes_cover_memory

[Yinghai Lu]

found one system that missed one entry for one node in SRAT, and that SRAT is not
rejected by nodes_cover_memory()

it turns out that we can not use absent_page_in_range to calaulate
e820ram, bacause that will use early_node_map and that is AND result of
e820 and SRAT.

revert back to use e820_hole_size instead.

also change that difference checking to 1M instead of 4G,
because e820ram, and pxmram are in pages.

[ Impact: reject wrong SRAT tables ]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Mel Gorman <mel@csn.ul.ie>

---
 arch/x86/mm/srat_64.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/mm/srat_64.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/srat_64.c
+++ linux-2.6/arch/x86/mm/srat_64.c
@@ -345,9 +345,9 @@ static int __init nodes_cover_memory(con
 			pxmram = 0;
 	}
 
-	e820ram = max_pfn - absent_pages_in_range(0, max_pfn);
+	e820ram = max_pfn - (e820_hole_size(0, max_pfn<<PAGE_SHIFT)>>PAGE_SHIFT);
 	/* We seem to lose 3 pages somewhere. Allow a bit of slack. */
-	if ((long)(e820ram - pxmram) >= 1*1024*1024) {
+	if ((long)(e820ram - pxmram) >= (1<<(20 - PAGE_SHIFT))) {
 		printk(KERN_ERR
 	"SRAT: PXMs only cover %luMB of your %luMB e820 RAM. Not used.\n",
 			(pxmram << PAGE_SHIFT) >> 20,

Re: [PATCH] x86: fix nodes_cover_memory

[Mel Gorman]

On Wed, May 06, 2009 at 09:53:35AM -0700, Yinghai Lu wrote:
> 
> found one system that missed one entry for one node in SRAT, and that SRAT is not
> rejected by nodes_cover_memory()
> 
> it turns out that we can not use absent_page_in_range to calaulate
> e820ram, bacause that will use early_node_map and that is AND result of
> e820 and SRAT.
> 

Correct, good spot.

> revert back to use e820_hole_size instead.
> 

I think the patch fixing this part of the problem is good, but the changelog
could be better. It took me a while to figure out what the problem was and
why this patch addressed it.

How about something like the following?

====
Sanity check the e820 against the SRAT table using only information from the e820 map

node_cover_memory() sanity checks the SRAT table by ensuring that all
PXMs cover the memory reported in the e820. However, when calculating
the size of the holes in the e820, it uses the early_node_map[] which
contains information taken from both SRAT and e820. If the SRAT is
missing an entry, then it is not detected that the SRAT table is
incorrect and missing entries.

This patch uses the e820 map to calculate the holes instead of
early_node_map[].
====

As an aside, it strikes me as odd that we discard an entire SRAT because it
is missing an entry in the e820. The impact may only be that the affinity
for a range of memory is incorrect, but it does not necessarily mean that the
entire table is incorrect. The intention of the code appears to be "if there is
any error in the SRAT, it's best ignored" though so maybe it's best left alone.

> also change that difference checking to 1M instead of 4G,
> because e820ram, and pxmram are in pages.
> 

While I agree with you, this should be a separate patch with its own
changelog. Something like

===
Allow 1MB of slack between the e820 map and SRAT, not 4GB

It is expected that there be slight differences between the e820 map and
the SRAT table and the intention was that 1MB of slack be allowed. The
calculation comparing e820ram and pxmram assumes the units are bytes,
when they are in fact pages. This means 4GB of slack is being allowed,
not 1MB. This patch makes the correct comparison
===

(1<<(20 - PAGE_SHIFT)) is a bit unreadable. At the very least, change the
comment above from "Allow a bit of slack" to "Allow 1MB of slack" so the
next reader knows what the intention of (1<<(20 - PAGE_SHIFT)) is.

Thanks

> [ Impact: reject wrong SRAT tables ]
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Mel Gorman <mel@csn.ul.ie>
> 
> ---
>  arch/x86/mm/srat_64.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/arch/x86/mm/srat_64.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/mm/srat_64.c
> +++ linux-2.6/arch/x86/mm/srat_64.c
> @@ -345,9 +345,9 @@ static int __init nodes_cover_memory(con
>  			pxmram = 0;
>  	}
>  
> -	e820ram = max_pfn - absent_pages_in_range(0, max_pfn);
> +	e820ram = max_pfn - (e820_hole_size(0, max_pfn<<PAGE_SHIFT)>>PAGE_SHIFT);
>  	/* We seem to lose 3 pages somewhere. Allow a bit of slack. */
> -	if ((long)(e820ram - pxmram) >= 1*1024*1024) {
> +	if ((long)(e820ram - pxmram) >= (1<<(20 - PAGE_SHIFT))) {
>  		printk(KERN_ERR
>  	"SRAT: PXMs only cover %luMB of your %luMB e820 RAM. Not used.\n",
>  			(pxmram << PAGE_SHIFT) >> 20,
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH] x86: fix nodes_cover_memory

[Ingo Molnar]

* Mel Gorman <mel@csn.ul.ie> wrote:

> On Wed, May 06, 2009 at 09:53:35AM -0700, Yinghai Lu wrote:
> > 
> > found one system that missed one entry for one node in SRAT, and that SRAT is not
> > rejected by nodes_cover_memory()
> > 
> > it turns out that we can not use absent_page_in_range to calaulate
> > e820ram, bacause that will use early_node_map and that is AND result of
> > e820 and SRAT.
> > 
> 
> Correct, good spot.
> 
> > revert back to use e820_hole_size instead.
> > 
> 
> I think the patch fixing this part of the problem is good, but the changelog
> could be better. It took me a while to figure out what the problem was and
> why this patch addressed it.
> 
> How about something like the following?
> 
> ====
> Sanity check the e820 against the SRAT table using only information from the e820 map
> 
> node_cover_memory() sanity checks the SRAT table by ensuring that all
> PXMs cover the memory reported in the e820. However, when calculating
> the size of the holes in the e820, it uses the early_node_map[] which
> contains information taken from both SRAT and e820. If the SRAT is
> missing an entry, then it is not detected that the SRAT table is
> incorrect and missing entries.
> 
> This patch uses the e820 map to calculate the holes instead of
> early_node_map[].
> ====
> 
> As an aside, it strikes me as odd that we discard an entire SRAT because it
> is missing an entry in the e820. The impact may only be that the affinity
> for a range of memory is incorrect, but it does not necessarily mean that the
> entire table is incorrect. The intention of the code appears to be "if there is
> any error in the SRAT, it's best ignored" though so maybe it's best left alone.
> 
> > also change that difference checking to 1M instead of 4G,
> > because e820ram, and pxmram are in pages.
> > 
> 
> While I agree with you, this should be a separate patch with its own
> changelog. Something like
> 
> ===
> Allow 1MB of slack between the e820 map and SRAT, not 4GB
> 
> It is expected that there be slight differences between the e820 map and
> the SRAT table and the intention was that 1MB of slack be allowed. The
> calculation comparing e820ram and pxmram assumes the units are bytes,
> when they are in fact pages. This means 4GB of slack is being allowed,
> not 1MB. This patch makes the correct comparison
> ===
> 
> (1<<(20 - PAGE_SHIFT)) is a bit unreadable. At the very least, change the
> comment above from "Allow a bit of slack" to "Allow 1MB of slack" so the
> next reader knows what the intention of (1<<(20 - PAGE_SHIFT)) is.
> 
> Thanks

thanks Mel!

Yinghai, mind resending the patch as two patches, with Mel's 
changelogs in place and with Mel's Acked-by as well?

Thanks,

	Ingo

Re: [PATCH] x86: fix nodes_cover_memory

[Mel Gorman]

On Thu, May 07, 2009 at 04:21:21PM +0200, Ingo Molnar wrote:
> 
> * Mel Gorman <mel@csn.ul.ie> wrote:
> 
> > On Wed, May 06, 2009 at 09:53:35AM -0700, Yinghai Lu wrote:
> > > 
> > > found one system that missed one entry for one node in SRAT, and that SRAT is not
> > > rejected by nodes_cover_memory()
> > > 
> > > it turns out that we can not use absent_page_in_range to calaulate
> > > e820ram, bacause that will use early_node_map and that is AND result of
> > > e820 and SRAT.
> > > 
> > 
> > Correct, good spot.
> > 
> > > revert back to use e820_hole_size instead.
> > > 
> > 
> > I think the patch fixing this part of the problem is good, but the changelog
> > could be better. It took me a while to figure out what the problem was and
> > why this patch addressed it.
> > 
> > How about something like the following?
> > 
> > ====
> > Sanity check the e820 against the SRAT table using only information from the e820 map
> > 
> > node_cover_memory() sanity checks the SRAT table by ensuring that all
> > PXMs cover the memory reported in the e820. However, when calculating
> > the size of the holes in the e820, it uses the early_node_map[] which
> > contains information taken from both SRAT and e820. If the SRAT is
> > missing an entry, then it is not detected that the SRAT table is
> > incorrect and missing entries.
> > 
> > This patch uses the e820 map to calculate the holes instead of
> > early_node_map[].
> > ====
> > 
> > As an aside, it strikes me as odd that we discard an entire SRAT because it
> > is missing an entry in the e820. The impact may only be that the affinity
> > for a range of memory is incorrect, but it does not necessarily mean that the
> > entire table is incorrect. The intention of the code appears to be "if there is
> > any error in the SRAT, it's best ignored" though so maybe it's best left alone.
> > 
> > > also change that difference checking to 1M instead of 4G,
> > > because e820ram, and pxmram are in pages.
> > > 
> > 
> > While I agree with you, this should be a separate patch with its own
> > changelog. Something like
> > 
> > ===
> > Allow 1MB of slack between the e820 map and SRAT, not 4GB
> > 
> > It is expected that there be slight differences between the e820 map and
> > the SRAT table and the intention was that 1MB of slack be allowed. The
> > calculation comparing e820ram and pxmram assumes the units are bytes,
> > when they are in fact pages. This means 4GB of slack is being allowed,
> > not 1MB. This patch makes the correct comparison
> > ===
> > 
> > (1<<(20 - PAGE_SHIFT)) is a bit unreadable. At the very least, change the
> > comment above from "Allow a bit of slack" to "Allow 1MB of slack" so the
> > next reader knows what the intention of (1<<(20 - PAGE_SHIFT)) is.
> > 
> > Thanks
> 
> thanks Mel!
> 
> Yinghai, mind resending the patch as two patches, with Mel's 
> changelogs in place and with Mel's Acked-by as well?
> 

My bad for leaving it out, but yes. Assuming there are two patches with the
updated changelog, I'm fine with adding the

Acked-by: Mel Gorman <mel@csn.ul.ie>

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

[PATCH 1/2] x86: Sanity check the e820 against the SRAT table using e820 map only

[Yinghai Lu]

node_cover_memory() sanity checks the SRAT table by ensuring that all
PXMs cover the memory reported in the e820. However, when calculating
the size of the holes in the e820, it uses the early_node_map[] which
contains information taken from both SRAT and e820. If the SRAT is
missing an entry, then it is not detected that the SRAT table is
incorrect and missing entries.

This patch uses the e820 map to calculate the holes instead of
early_node_map[].

comment is from Mel.

[ Impact: reject wrong SRAT tables ]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Mel Gorman <mel@csn.ul.ie>

---
 arch/x86/mm/srat_64.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/arch/x86/mm/srat_64.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/srat_64.c
+++ linux-2.6/arch/x86/mm/srat_64.c
@@ -345,7 +345,7 @@ static int __init nodes_cover_memory(con
 			pxmram = 0;
 	}
 
-	e820ram = max_pfn - absent_pages_in_range(0, max_pfn);
+	e820ram = max_pfn - (e820_hole_size(0, max_pfn<<PAGE_SHIFT)>>PAGE_SHIFT);
 	/* We seem to lose 3 pages somewhere. Allow a bit of slack. */
 	if ((long)(e820ram - pxmram) >= 1*1024*1024) {
 		printk(KERN_ERR

[PATCH 2/2] x86: Allow 1MB of slack between the e820 map and SRAT, not 4GB

[Yinghai Lu]

It is expected that there be slight differences between the e820 map and
the SRAT table and the intention was that 1MB of slack be allowed. The
calculation comparing e820ram and pxmram assumes the units are bytes,
when they are in fact pages. This means 4GB of slack is being allowed,
not 1MB. This patch makes the correct comparison

comment is from Mel.

[ Impact: don't accept the srat that could dump 2g etc ram ]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Mel Gorman <mel@csn.ul.ie>

---
 arch/x86/mm/srat_64.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/mm/srat_64.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/srat_64.c
+++ linux-2.6/arch/x86/mm/srat_64.c
@@ -346,8 +346,8 @@ static int __init nodes_cover_memory(con
 	}
 
 	e820ram = max_pfn - (e820_hole_size(0, max_pfn<<PAGE_SHIFT)>>PAGE_SHIFT);
-	/* We seem to lose 3 pages somewhere. Allow a bit of slack. */
-	if ((long)(e820ram - pxmram) >= 1*1024*1024) {
+	/* We seem to lose 3 pages somewhere. Allow 1M of slack. */
+	if ((long)(e820ram - pxmram) >= (1<<(20 - PAGE_SHIFT))) {
 		printk(KERN_ERR
 	"SRAT: PXMs only cover %luMB of your %luMB e820 RAM. Not used.\n",
 			(pxmram << PAGE_SHIFT) >> 20,

Re: [PATCH 1/2] x86: Sanity check the e820 against the SRAT table using e820 map only

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> -	e820ram = max_pfn - absent_pages_in_range(0, max_pfn);
> +	e820ram = max_pfn - (e820_hole_size(0, max_pfn<<PAGE_SHIFT)>>PAGE_SHIFT);

btw., it would be nice to have a debug check that prints a warning 
if the SRAT does not cover all RAM, or if it covers RAM that is not 
present in the e820 map. Such a warning might alert us to double 
check all the PXM settings in the SRAT and could uncover more quirks 
like the above ...

	Ingo

[tip:x86/mm] x86: Sanity check the e820 against the SRAT table using e820 map only

[tip-bot for Yinghai Lu]

Commit-ID:  b37ab91907e9002925f4217e3bbd496aa12c2fa3
Gitweb:     http://git.kernel.org/tip/b37ab91907e9002925f4217e3bbd496aa12c2fa3
Author:     Yinghai Lu <yinghai@kernel.org>
AuthorDate: Fri, 8 May 2009 00:36:44 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 11 May 2009 11:35:07 +0200

x86: Sanity check the e820 against the SRAT table using e820 map only

node_cover_memory() sanity checks the SRAT table by ensuring that all
PXMs cover the memory reported in the e820.

However, when calculating the size of the holes in the e820, it uses
the early_node_map[] which contains information taken from both SRAT
and e820. If the SRAT is missing an entry, then it is not detected
that the SRAT table is incorrect and missing entries.

This patch uses the e820 map to calculate the holes instead of
early_node_map[].

comment is from Mel.

[ Impact: reject incorrect SRAT tables ]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Mel Gorman <mel@csn.ul.ie>
Cc: Andrew Morton <akpm@linux-foundation.org>
LKML-Reference: <4A03E10C.60906@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/mm/srat_64.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c
index 0176595..c7a18aa 100644
--- a/arch/x86/mm/srat_64.c
+++ b/arch/x86/mm/srat_64.c
@@ -345,7 +345,7 @@ static int __init nodes_cover_memory(const struct bootnode *nodes)
 			pxmram = 0;
 	}
 
-	e820ram = max_pfn - absent_pages_in_range(0, max_pfn);
+	e820ram = max_pfn - (e820_hole_size(0, max_pfn<<PAGE_SHIFT)>>PAGE_SHIFT);
 	/* We seem to lose 3 pages somewhere. Allow a bit of slack. */
 	if ((long)(e820ram - pxmram) >= 1*1024*1024) {
 		printk(KERN_ERR

[tip:x86/mm] x86: Allow 1MB of slack between the e820 map and SRAT, not 4GB

[tip-bot for Yinghai Lu]

Commit-ID:  0964b0562bb9c93194e852b47bab2397b9e11c18
Gitweb:     http://git.kernel.org/tip/0964b0562bb9c93194e852b47bab2397b9e11c18
Author:     Yinghai Lu <yinghai@kernel.org>
AuthorDate: Fri, 8 May 2009 00:37:34 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 11 May 2009 11:38:21 +0200

x86: Allow 1MB of slack between the e820 map and SRAT, not 4GB

It is expected that there might be slight differences between the e820
map and the SRAT table and the intention was that 1MB of slack be allowed.

The calculation comparing e820ram and pxmram assumes the units are bytes,
when they are in fact pages. This means 4GB of slack is being allowed,
not 1MB. This patch makes the correct comparison.

comment is from Mel.

[ Impact: don't accept buggy SRATs that could dump up to 4G of RAM ]

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: Mel Gorman <mel@csn.ul.ie>
Cc: Andrew Morton <akpm@linux-foundation.org>
LKML-Reference: <4A03E13E.6050107@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/mm/srat_64.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c
index c7a18aa..87b45bf 100644
--- a/arch/x86/mm/srat_64.c
+++ b/arch/x86/mm/srat_64.c
@@ -346,8 +346,8 @@ static int __init nodes_cover_memory(const struct bootnode *nodes)
 	}
 
 	e820ram = max_pfn - (e820_hole_size(0, max_pfn<<PAGE_SHIFT)>>PAGE_SHIFT);
-	/* We seem to lose 3 pages somewhere. Allow a bit of slack. */
-	if ((long)(e820ram - pxmram) >= 1*1024*1024) {
+	/* We seem to lose 3 pages somewhere. Allow 1M of slack. */
+	if ((long)(e820ram - pxmram) >= (1<<(20 - PAGE_SHIFT))) {
 		printk(KERN_ERR
 	"SRAT: PXMs only cover %luMB of your %luMB e820 RAM. Not used.\n",
 			(pxmram << PAGE_SHIFT) >> 20,

Re: [PATCH 1/2] x86: Sanity check the e820 against the SRAT table using e820 map only

[Yinghai Lu]

Ingo Molnar wrote:
> * Yinghai Lu <yinghai@kernel.org> wrote:
> 
>> -	e820ram = max_pfn - absent_pages_in_range(0, max_pfn);
>> +	e820ram = max_pfn - (e820_hole_size(0, max_pfn<<PAGE_SHIFT)>>PAGE_SHIFT);
> 
> btw., it would be nice to have a debug check that prints a warning 
> if the SRAT does not cover all RAM, or if it covers RAM that is not 
> present in the e820 map. Such a warning might alert us to double 
> check all the PXM settings in the SRAT and could uncover more quirks 
> like the above ...

we trust e820 than SRAT mem entries.

so early_node_map already are result of in e820 range AND srat.  (the range in SRAT that is not in e820 is dumped)

just check srat table (without AND) with e820 will have some confusing. because SRAT would cover the hole below 4g that is not in E820.

YH