Hello.
I found there is a bug in grow_zone_span() and grow_pgdat_span().
Please apply.
---------
If hot-added memory's address is smaller than old area,
spanned_pages will not be updated. It must be fixed.
example) Old zone_start_pfn = 0x60000, and spanned_pages = 0x10000
Added new memory's start_pfn = 0x50000, and end_pfn = 0x60000
new spanned_pages will be still 0x10000 by old code.
(It should be updated to 0x20000.) Because old_zone_end_pfn will be
0x70000, and end_pfn smaller than it. So, spanned_pages will not be
updated.
In current code, spanned_pages is updated only when end_pfn is updated.
But, it should be updated even if end_pfn is not updated, because
start_pfn might be changed.
This is for 2.6.17-rc4-mm3.
I tested this patch on Tiger4 with my node emulation.
Signed-off-by: Yasunori Goto <[email protected]>
------------------------------------------------
mm/memory_hotplug.c | 18 +++++++++++-------
1 files changed, 11 insertions(+), 7 deletions(-)
Index: pgdat15/mm/memory_hotplug.c
===================================================================
--- pgdat15.orig/mm/memory_hotplug.c 2006-05-23 14:58:21.000000000 +0900
+++ pgdat15/mm/memory_hotplug.c 2006-05-23 14:58:29.000000000 +0900
@@ -95,16 +95,18 @@ EXPORT_SYMBOL_GPL(__add_pages);
static void grow_zone_span(struct zone *zone,
unsigned long start_pfn, unsigned long end_pfn)
{
- unsigned long old_zone_end_pfn;
+ unsigned long new_zone_end_pfn;
zone_span_writelock(zone);
- old_zone_end_pfn = zone->zone_start_pfn + zone->spanned_pages;
+ new_zone_end_pfn = zone->zone_start_pfn + zone->spanned_pages;
if (start_pfn < zone->zone_start_pfn)
zone->zone_start_pfn = start_pfn;
- if (end_pfn > old_zone_end_pfn)
- zone->spanned_pages = end_pfn - zone->zone_start_pfn;
+ if (end_pfn > new_zone_end_pfn)
+ new_zone_end_pfn = end_pfn;
+
+ zone->spanned_pages = new_zone_end_pfn - zone->zone_start_pfn;
zone_span_writeunlock(zone);
}
@@ -112,14 +114,16 @@ static void grow_zone_span(struct zone *
static void grow_pgdat_span(struct pglist_data *pgdat,
unsigned long start_pfn, unsigned long end_pfn)
{
- unsigned long old_pgdat_end_pfn =
+ unsigned long new_pgdat_end_pfn =
pgdat->node_start_pfn + pgdat->node_spanned_pages;
if (start_pfn < pgdat->node_start_pfn)
pgdat->node_start_pfn = start_pfn;
- if (end_pfn > old_pgdat_end_pfn)
- pgdat->node_spanned_pages = end_pfn - pgdat->node_start_pfn;
+ if (end_pfn > new_pgdat_end_pfn)
+ new_pgdat_end_pfn = end_pfn;
+
+ pgdat->node_spanned_pages = new_pgdat_end_pfn - pgdat->node_start_pfn;
}
int online_pages(unsigned long pfn, unsigned long nr_pages)
--
Yasunori Goto
On Tue, 2006-05-23 at 17:29 +0900, Yasunori Goto wrote:
> Hello.
>
> I found there is a bug in grow_zone_span() and grow_pgdat_span().
...
> @@ -95,16 +95,18 @@ EXPORT_SYMBOL_GPL(__add_pages);
> static void grow_zone_span(struct zone *zone,
> unsigned long start_pfn, unsigned long end_pfn)
> {
> - unsigned long old_zone_end_pfn;
> + unsigned long new_zone_end_pfn;
>
> zone_span_writelock(zone);
>
> - old_zone_end_pfn = zone->zone_start_pfn + zone->spanned_pages;
> + new_zone_end_pfn = zone->zone_start_pfn + zone->spanned_pages;
I really don't like the idea of having this variable called "new_"
something. That implies that this is what the new end_pfn is going to
be. The *new* one. In reality, it is what it _might_ have been. How
about "tmp_zone_end_pfn"?
This practice of dealing with spanned_pages is a real pain.
I generally try to avoid max/min in code, but this struck me as possibly
being useful. Do you find this easier to read, or your patch?
diff -puN mm/memory_hotplug.c~fix-spanned-pages mm/memory_hotplug.c
--- work/mm/memory_hotplug.c~fix-spanned-pages 2006-05-23 09:04:31.000000000 -0700
+++ work-dave/mm/memory_hotplug.c 2006-05-23 09:22:18.000000000 -0700
@@ -91,8 +91,8 @@ static void grow_zone_span(struct zone *
if (start_pfn < zone->zone_start_pfn)
zone->zone_start_pfn = start_pfn;
- if (end_pfn > old_zone_end_pfn)
- zone->spanned_pages = end_pfn - zone->zone_start_pfn;
+ zone->spanned_pages = max(old_zone_end_pfn, end_pfn) -
+ zone->zone_start_pfn);
zone_span_writeunlock(zone);
}
@@ -106,8 +106,8 @@ static void grow_pgdat_span(struct pglis
if (start_pfn < pgdat->node_start_pfn)
pgdat->node_start_pfn = start_pfn;
- if (end_pfn > old_pgdat_end_pfn)
- pgdat->node_spanned_pages = end_pfn - pgdat->node_start_pfn;
+ pgdat->node_spanned_pages = max(old_pgdat_end_pfn, end_pfn) -
+ pgdat->node_start_pfn;
}
int online_pages(unsigned long pfn, unsigned long nr_pages)
_
-- Dave
> I really don't like the idea of having this variable called "new_"
> something. That implies that this is what the new end_pfn is going to
> be. The *new* one. In reality, it is what it _might_ have been. How
> about "tmp_zone_end_pfn"?
>
> This practice of dealing with spanned_pages is a real pain.
>
> I generally try to avoid max/min in code, but this struck me as possibly
> being useful. Do you find this easier to read, or your patch?
Ah, I think using max is better due to smaller code, indeed.
>
> diff -puN mm/memory_hotplug.c~fix-spanned-pages mm/memory_hotplug.c
> --- work/mm/memory_hotplug.c~fix-spanned-pages 2006-05-23 09:04:31.000000000 -0700
> +++ work-dave/mm/memory_hotplug.c 2006-05-23 09:22:18.000000000 -0700
> @@ -91,8 +91,8 @@ static void grow_zone_span(struct zone *
> if (start_pfn < zone->zone_start_pfn)
> zone->zone_start_pfn = start_pfn;
>
> - if (end_pfn > old_zone_end_pfn)
> - zone->spanned_pages = end_pfn - zone->zone_start_pfn;
> + zone->spanned_pages = max(old_zone_end_pfn, end_pfn) -
> + zone->zone_start_pfn);
^
this parentheses is redundant. :-)
Could you fix and repost it? Or should I?
--
Yasunori Goto
On Wed, 2006-05-24 at 10:18 +0900, Yasunori Goto wrote:
> Could you fix and repost it? Or should I?
I'd appreciate it if you could test it, and forward it along
afterword.
-- Dave
> On Wed, 2006-05-24 at 10:18 +0900, Yasunori Goto wrote:
> > Could you fix and repost it? Or should I?
>
> I'd appreciate it if you could test it, and forward it along
> afterword.
Ok. I'll do it. :-)
--
Yasunori Goto
I fixed redundant parentheses and tested this fix updating
spanned_pages patch.
Andrew-san.
Please apply.
------------------
If hot-added memory's address is smaller than old area,
spanned_pages will not be updated. It must be fixed.
example) Old zone_start_pfn = 0x60000, and spanned_pages = 0x10000
Added new memory's start_pfn = 0x50000, and end_pfn = 0x60000
new spanned_pages will be still 0x10000 by old code.
(It should be updated to 0x20000.) Because old_zone_end_pfn will be
0x70000, and end_pfn smaller than it. So, spanned_pages will not be
updated.
In current code, spanned_pages is updated only when end_pfn is updated.
But, it should be updated by subtraction between bigger end_pfn and new
zone_start_pfn.
This is for 2.6.17-rc4-mm3.
I tested this patch on Tiger4 with my node emulation.
Signed-off-by: Yasunori Goto <[email protected]>
Signed-off-by: Dave Hansen <[email protected]>
mm/memory_hotplug.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
Index: pgdat15/mm/memory_hotplug.c
===================================================================
--- pgdat15.orig/mm/memory_hotplug.c 2006-05-25 12:22:54.000000000 +0900
+++ pgdat15/mm/memory_hotplug.c 2006-05-25 12:23:18.000000000 +0900
@@ -103,8 +103,8 @@ static void grow_zone_span(struct zone *
if (start_pfn < zone->zone_start_pfn)
zone->zone_start_pfn = start_pfn;
- if (end_pfn > old_zone_end_pfn)
- zone->spanned_pages = end_pfn - zone->zone_start_pfn;
+ zone->spanned_pages = max(old_zone_end_pfn, end_pfn) -
+ zone->zone_start_pfn;
zone_span_writeunlock(zone);
}
@@ -118,8 +118,8 @@ static void grow_pgdat_span(struct pglis
if (start_pfn < pgdat->node_start_pfn)
pgdat->node_start_pfn = start_pfn;
- if (end_pfn > old_pgdat_end_pfn)
- pgdat->node_spanned_pages = end_pfn - pgdat->node_start_pfn;
+ pgdat->node_spanned_pages = max(old_pgdat_end_pfn, end_pfn) -
+ pgdat->node_start_pfn;
}
int online_pages(unsigned long pfn, unsigned long nr_pages)
--
Yasunori Goto