memory-hotplug : suppres "Trying to free nonexistent resource <XXXXXXXXXXXXXXXX-YYYYYYYYYYYYYYYY>" warning

When our x86 box calls __remove_pages(), release_mem_region() shows
many warnings. And x86 box cannot unregister iomem_resource.

"Trying to free nonexistent resource <XXXXXXXXXXXXXXXX-YYYYYYYYYYYYYYYY>"

release_mem_region() has been changed as called in each PAGES_PER_SECTION
chunk since applying a patch(de7f0cba96786c). Because powerpc registers
iomem_resource in each PAGES_PER_SECTION chunk. But when I hot add memory
on x86 box, iomem_resource is register in each _CRS not PAGES_PER_SECTION
chunk. So x86 box unregisters iomem_resource.

The patch fixes the problem.

CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <redacted>
CC: Len Brown <redacted>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <redacted>
CC: Christoph Lameter <redacted>
Cc: Minchan Kim <redacted>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <redacted>
CC: Wen Congyang <redacted>
Signed-off-by: Yasuaki Ishimatsu <redacted>
---
 arch/powerpc/platforms/pseries/hotplug-memory.c |   13 +++++++++----
 mm/memory_hotplug.c                             |    4 ++--
 2 files changed, 11 insertions(+), 6 deletions(-)

Index: linux-3.6/arch/powerpc/platforms/pseries/hotplug-memory.c
===================================================================

--- linux-3.6.orig/arch/powerpc/platforms/pseries/hotplug-memory.c	2012-10-04 14:22:59.833520792 +0900
+++ linux-3.6/arch/powerpc/platforms/pseries/hotplug-memory.c	2012-10-04 14:23:05.150521411 +0900

@@ -77,7 +77,8 @@ static int pseries_remove_memblock(unsig
 {
 	unsigned long start, start_pfn;
 	struct zone *zone;
-	int ret;
+	int i, ret;
+	int sections_to_remove;
 
 	start_pfn = base >> PAGE_SHIFT;
 

@@ -97,9 +98,13 @@ static int pseries_remove_memblock(unsig
 	 * to sysfs "state" file and we can't remove sysfs entries
 	 * while writing to it. So we have to defer it to here.
 	 */
-	ret = __remove_pages(zone, start_pfn, memblock_size >> PAGE_SHIFT);
-	if (ret)
-		return ret;
+	sections_to_remove = (memblock_size >> PAGE_SHIFT) / PAGES_PER_SECTION;
+	for (i = 0; i < sections_to_remove; i++) {
+		unsigned long pfn = start_pfn + i * PAGES_PER_SECTION;
+		ret = __remove_pages(zone, start_pfn,  PAGES_PER_SECTION);
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * Update memory regions for memory remove

Index: linux-3.6/mm/memory_hotplug.c
===================================================================

--- linux-3.6.orig/mm/memory_hotplug.c	2012-10-04 14:22:59.829520788 +0900
+++ linux-3.6/mm/memory_hotplug.c	2012-10-04 14:23:25.860527278 +0900

@@ -362,11 +362,11 @@ int __remove_pages(struct zone *zone, un
 	BUG_ON(phys_start_pfn & ~PAGE_SECTION_MASK);
 	BUG_ON(nr_pages % PAGES_PER_SECTION);
 
+	release_mem_region(phys_start_pfn << PAGE_SHIFT, nr_pages * PAGE_SIZE);
+
 	sections_to_remove = nr_pages / PAGES_PER_SECTION;
 	for (i = 0; i < sections_to_remove; i++) {
 		unsigned long pfn = phys_start_pfn + i*PAGES_PER_SECTION;
-		release_mem_region(pfn << PAGE_SHIFT,
-				   PAGES_PER_SECTION << PAGE_SHIFT);
 		ret = __remove_section(zone, __pfn_to_section(pfn));
 		if (ret)
 			break;

CC to Dave Hanse.

Hello Dave,

I think following patch works both x86 and ppc. but I'm not ppc
expert. So I'm glad
if you double check it.

thank you.


<intentional full quote>

On Thu, 4 Oct 2012 14:31:09 +0900
Yasuaki Ishimatsu [off-list ref] wrote:

It is inappropriate that `i' have a signed 32-bit type.  I doubt if
there's any possibility of an overflow bug here, but using a consistent
and well-chosen type would eliminate all doubt.

Note that __remove_pages() does use an unsigned long for this, although
it stupidly calls that variable "i", despite the C programmers'
expectation that a variable called "i" has type "int".

The same applies to `sections_to_remove', but __remove_pages() went and
decided to use an `int' for that variable.  Sigh.

Anyway, please have a think, and see if we can come up with the best
and most accurate choice of types and identifiers in this code.

Hi Andrew,

2012/10/06 6:09, Andrew Morton wrote:

On Thu, 4 Oct 2012 14:31:09 +0900
Yasuaki Ishimatsu [off-list ref] wrote:

quoted

When our x86 box calls __remove_pages(), release_mem_region() shows
many warnings. And x86 box cannot unregister iomem_resource.

"Trying to free nonexistent resource <XXXXXXXXXXXXXXXX-YYYYYYYYYYYYYYYY>"

release_mem_region() has been changed as called in each PAGES_PER_SECTION
chunk since applying a patch(de7f0cba96786c). Because powerpc registers
iomem_resource in each PAGES_PER_SECTION chunk. But when I hot add memory
on x86 box, iomem_resource is register in each _CRS not PAGES_PER_SECTION
chunk. So x86 box unregisters iomem_resource.

The patch fixes the problem.

--- linux-3.6.orig/arch/powerpc/platforms/pseries/hotplug-memory.c	2012-10-04 14:22:59.833520792 +0900
+++ linux-3.6/arch/powerpc/platforms/pseries/hotplug-memory.c	2012-10-04 14:23:05.150521411 +0900

@@ -77,7 +77,8 @@ static int pseries_remove_memblock(unsig
  {
  	unsigned long start, start_pfn;
  	struct zone *zone;
-	int ret;
+	int i, ret;
+	int sections_to_remove;

  	start_pfn = base >> PAGE_SHIFT;

@@ -97,9 +98,13 @@ static int pseries_remove_memblock(unsig
  	 * to sysfs "state" file and we can't remove sysfs entries
  	 * while writing to it. So we have to defer it to here.
  	 */
-	ret = __remove_pages(zone, start_pfn, memblock_size >> PAGE_SHIFT);
-	if (ret)
-		return ret;
+	sections_to_remove = (memblock_size >> PAGE_SHIFT) / PAGES_PER_SECTION;
+	for (i = 0; i < sections_to_remove; i++) {
+		unsigned long pfn = start_pfn + i * PAGES_PER_SECTION;
+		ret = __remove_pages(zone, start_pfn,  PAGES_PER_SECTION);
+		if (ret)
+			return ret;
+	}

It is inappropriate that `i' have a signed 32-bit type.  I doubt if
there's any possibility of an overflow bug here, but using a consistent
and well-chosen type would eliminate all doubt.

Note that __remove_pages() does use an unsigned long for this, although
it stupidly calls that variable "i", despite the C programmers'
expectation that a variable called "i" has type "int".

The same applies to `sections_to_remove', but __remove_pages() went and
decided to use an `int' for that variable.  Sigh.

Anyway, please have a think, and see if we can come up with the best
and most accurate choice of types and identifiers in this code.

Your concern is right. Overflow bug may occur in the future.
So I changed type of "i" and "sections_to_remove" to "unsigned long".
Please merge it into your tree instead of previous patch.

__remove_pages() also has same concern. So I'll fix it.

-----------------------------------------------------------------------
When our x86 box calls __remove_pages(), release_mem_region() shows
many warnings. And x86 box cannot unregister iomem_resource.

"Trying to free nonexistent resource <XXXXXXXXXXXXXXXX-YYYYYYYYYYYYYYYY>"

release_mem_region() has been changed as called in each PAGES_PER_SECTION
chunk since applying a patch(de7f0cba96786c). Because powerpc registers
iomem_resource in each PAGES_PER_SECTION chunk. But when I hot add memory
on x86 box, iomem_resource is register in each _CRS not PAGES_PER_SECTION
chunk. So x86 box unregisters iomem_resource.

The patch fixes the problem.

CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <redacted>
CC: Len Brown <redacted>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <redacted>
CC: Christoph Lameter <redacted>
Cc: Minchan Kim <redacted>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <redacted>
CC: Wen Congyang <redacted>
Signed-off-by: Yasuaki Ishimatsu <redacted>
---
  arch/powerpc/platforms/pseries/hotplug-memory.c |   11 ++++++++---
  mm/memory_hotplug.c                             |    4 ++--
  2 files changed, 10 insertions(+), 5 deletions(-)

Index: linux-3.6/arch/powerpc/platforms/pseries/hotplug-memory.c
===================================================================

--- linux-3.6.orig/arch/powerpc/platforms/pseries/hotplug-memory.c	2012-10-05 14:33:09.516197839 +0900
+++ linux-3.6/arch/powerpc/platforms/pseries/hotplug-memory.c	2012-10-09 11:27:50.555709827 +0900

@@ -78,6 +78,7 @@ static int pseries_remove_memblock(unsig
  	unsigned long start, start_pfn;
  	struct zone *zone;
  	int ret;
+	unsigned long i, sections_to_remove;
  
  	start_pfn = base >> PAGE_SHIFT;
  

@@ -97,9 +98,13 @@ static int pseries_remove_memblock(unsig
  	 * to sysfs "state" file and we can't remove sysfs entries
  	 * while writing to it. So we have to defer it to here.
  	 */
-	ret = __remove_pages(zone, start_pfn, memblock_size >> PAGE_SHIFT);
-	if (ret)
-		return ret;
+	sections_to_remove = (memblock_size >> PAGE_SHIFT) / PAGES_PER_SECTION;
+	for (i = 0; i < sections_to_remove; i++) {
+		unsigned long pfn = start_pfn + i * PAGES_PER_SECTION;
+		ret = __remove_pages(zone, start_pfn,  PAGES_PER_SECTION);
+		if (ret)
+			return ret;
+	}
  
  	/*
  	 * Update memory regions for memory remove

Index: linux-3.6/mm/memory_hotplug.c
===================================================================

--- linux-3.6.orig/mm/memory_hotplug.c	2012-10-05 15:21:42.856325965 +0900
+++ linux-3.6/mm/memory_hotplug.c	2012-10-05 15:21:43.047326148 +0900

@@ -596,11 +596,11 @@ int __remove_pages(struct zone *zone, un
  	BUG_ON(phys_start_pfn & ~PAGE_SECTION_MASK);
  	BUG_ON(nr_pages % PAGES_PER_SECTION);
  
+	release_mem_region(phys_start_pfn << PAGE_SHIFT, nr_pages * PAGE_SIZE);
+
  	sections_to_remove = nr_pages / PAGES_PER_SECTION;
  	for (i = 0; i < sections_to_remove; i++) {
  		unsigned long pfn = phys_start_pfn + i*PAGES_PER_SECTION;
-		release_mem_region(pfn << PAGE_SHIFT,
-				   PAGES_PER_SECTION << PAGE_SHIFT);
  		ret = __remove_section(zone, __pfn_to_section(pfn));
  		if (ret)
  			break;

On Tue, 9 Oct 2012 11:51:38 +0900
Yasuaki Ishimatsu [off-list ref] wrote:

quoted

Anyway, please have a think, and see if we can come up with the best
and most accurate choice of types and identifiers in this code.

Your concern is right. Overflow bug may occur in the future.
So I changed type of "i" and "sections_to_remove" to "unsigned long".
Please merge it into your tree instead of previous patch.

Too late, the original patch was merged.  So I generated the delta.

I remain allergic to the `i' identifier so I renamed it to `section'. 
That's not 100% accurate, but it is better.

__remove_pages() also has same concern. So I'll fix it.

Thanks.



From: Yasuaki Ishimatsu <redacted>
Subject: arch/powerpc/platforms/pseries/hotplug-memory.c: section removal cleanups

Followups to d760afd4d25 ("memory-hotplug: suppress "Trying to free
nonexistent resource <XXXXXXXXXXXXXXXX-YYYYYYYYYYYYYYYY>" warning").

- use unsigned long type, as overflows are conceivable

- rename `i' to the less-misleading and more informative `section'

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/powerpc/platforms/pseries/hotplug-memory.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff -puN arch/powerpc/platforms/pseries/hotplug-memory.c~arch-powerpc-platforms-pseries-hotplug-memoryc-section-removal-cleanups arch/powerpc/platforms/pseries/hotplug-memory.c

--- a/arch/powerpc/platforms/pseries/hotplug-memory.c~arch-powerpc-platforms-pseries-hotplug-memoryc-section-removal-cleanups
+++ a/arch/powerpc/platforms/pseries/hotplug-memory.c

@@ -77,8 +77,9 @@ static int pseries_remove_memblock(unsig
 {
 	unsigned long start, start_pfn;
 	struct zone *zone;
-	int i, ret;
-	int sections_to_remove;
+	int ret;
+	unsigned long section;
+	unsigned long sections_to_remove;
 
 	start_pfn = base >> PAGE_SHIFT;
 

@@ -99,8 +100,8 @@ static int pseries_remove_memblock(unsig
 	 * while writing to it. So we have to defer it to here.
 	 */
 	sections_to_remove = (memblock_size >> PAGE_SHIFT) / PAGES_PER_SECTION;
-	for (i = 0; i < sections_to_remove; i++) {
-		unsigned long pfn = start_pfn + i * PAGES_PER_SECTION;
+	for (section = 0; section < sections_to_remove; section++) {
+		unsigned long pfn = start_pfn + section * PAGES_PER_SECTION;
 		ret = __remove_pages(zone, start_pfn,  PAGES_PER_SECTION);
 		if (ret)
 			return ret;

diff -puN mm/memory_hotplug.c~arch-powerpc-platforms-pseries-hotplug-memoryc-section-removal-cleanups mm/memory_hotplug.c
_