[PATCH] ARM: mm: enforce pageblock alignment when freeing memmap entries at init

On some machines, the nodes do not always start on pageblock
boundaries. In these cases it is possible for free_unused_memmap
to free mappings for pages inside a pageblock with otherwise
valid pages. This presents problems for page migration since it
operates on whole pageblocks at a time.

Round down bank_start to pageblock boundary so that whole
pageblocks always have valid mappings.

Signed-off-by: Dima Zavin <redacted>
---
 arch/arm/mm/init.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 5164069..98de5d8 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c

@@ -418,7 +418,11 @@ static void __init free_unused_memmap(struct meminfo *mi)
 	for_each_bank(i, mi) {
 		struct membank *bank = &mi->bank[i];
 
-		bank_start = bank_pfn_start(bank);
+		/* Round bank_start down to the start of a pageblock so that
+		 * all pages in a pageblock always have a mapping.
+		 */
+		bank_start = round_down(bank_pfn_start(bank),
+					MAX_ORDER_NR_PAGES);
 
 		/*
 		 * If we had a previous bank, and there is a space

-- 
1.7.3.1

On 20 Jan 11 15:47, Dima Zavin wrote:

I tested this (booted, ran various commands for about 10 minutes)
on a MSM8x50 with the standard memory map and on a MSM8x50 given
a synthetic memory map which would be affected by this patch.

Tested-by: Larry Bassel <redacted>

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

On Thu, Jan 20, 2011 at 03:47:11PM -0800, Dima Zavin wrote:

On some machines, the nodes do not always start on pageblock
boundaries. In these cases it is possible for free_unused_memmap
to free mappings for pages inside a pageblock with otherwise
valid pages. This presents problems for page migration since it
operates on whole pageblocks at a time.

This patch is not aligning on a pageblock boundary - it's aligning on
MAX_ORDER_NR_PAGES which is the boundary the buddy allocator works on.
This is a minor but important nit as different assumptions are made
about pageblocks and MAX_ORDER_NR_PAGES. Anyway;

If the node is not starting on the MAX_ORDER boundary then node_start_pfn
should also not be on the same boundary but the patch does not imply
that this has been broken. If anything it appears at a glance that there
is memmap *not* being freed because it was allocated on the
MAX_ORDER_NR_PAGES boundary and only partially freed.

The comment then is confusing because the function is freeing memmap but
rounding down to free more of it ensures that all pages get a mapping?
It's not clear at all to me what was broken or how this patch fixes it
but bear in mind that it's rare I look at how ARM is initialised.

-- 
Mel Gorman
Linux Technology Center
IBM Dublin Software Lab

On Wed, Jan 26, 2011 at 12:29 AM, Mel Gorman [off-list ref] wrote:

On Thu, Jan 20, 2011 at 03:47:11PM -0800, Dima Zavin wrote:

quoted

On some machines, the nodes do not always start on pageblock
boundaries. In these cases it is possible for free_unused_memmap
to free mappings for pages inside a pageblock with otherwise
valid pages. This presents problems for page migration since it
operates on whole pageblocks at a time.

This patch is not aligning on a pageblock boundary - it's aligning on
MAX_ORDER_NR_PAGES which is the boundary the buddy allocator works on.
This is a minor but important nit as different assumptions are made
about pageblocks and MAX_ORDER_NR_PAGES. Anyway;

Thank you for the nit. I'm kind of a VM noob so any insight is
appreciated. I naively equated the two in my head when writing the
description.

If the node is not starting on the MAX_ORDER boundary then node_start_pfn
should also not be on the same boundary but the patch does not imply
that this has been broken. If anything it appears at a glance that there
is memmap *not* being freed because it was allocated on the
MAX_ORDER_NR_PAGES boundary and only partially freed.

The comment then is confusing because the function is freeing memmap but
rounding down to free more of it ensures that all pages get a mapping?
It's not clear at all to me what was broken or how this patch fixes it
but bear in mind that it's rare I look at how ARM is initialised.

The function frees the mappings *between* the nodes. So when the patch
rounds down the start of the node, it preserves (i.e. frees less) the
mappings from MAX_ORDER_NR_PAGES boundary to the actual first valid
page. Otherwise, I would have to define CONFIG_HOLES_IN_ZONE and pay
the penalty. Looking through the history, a similar patch was already
taken to roundup the previous bank end to MAX_ORDER_NR_PAGES. I'm
doing the same to the start.

Am I totally off base?

--Dima

quoted

Round down bank_start to pageblock boundary so that whole
pageblocks always have valid mappings.

Signed-off-by: Dima Zavin <redacted>
---
?arch/arm/mm/init.c | ? ?6 +++++-
?1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 5164069..98de5d8 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c

@@ -418,7 +418,11 @@ static void __init free_unused_memmap(struct meminfo *mi)

? ? ? for_each_bank(i, mi) {
? ? ? ? ? ? ? struct membank *bank = &mi->bank[i];

- ? ? ? ? ? ? bank_start = bank_pfn_start(bank);
+ ? ? ? ? ? ? /* Round bank_start down to the start of a pageblock so that
+ ? ? ? ? ? ? ?* all pages in a pageblock always have a mapping.
+ ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? bank_start = round_down(bank_pfn_start(bank),
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? MAX_ORDER_NR_PAGES);

? ? ? ? ? ? ? /*
? ? ? ? ? ? ? ?* If we had a previous bank, and there is a space
--
1.7.3.1

--
Mel Gorman
Linux Technology Center
IBM Dublin Software Lab

ping?

On Wed, Jan 26, 2011 at 1:57 PM, Dima Zavin [off-list ref] wrote:

On Wed, Jan 26, 2011 at 12:29 AM, Mel Gorman [off-list ref] wrote:

quoted

On Thu, Jan 20, 2011 at 03:47:11PM -0800, Dima Zavin wrote:

quoted

On some machines, the nodes do not always start on pageblock
boundaries. In these cases it is possible for free_unused_memmap
to free mappings for pages inside a pageblock with otherwise
valid pages. This presents problems for page migration since it
operates on whole pageblocks at a time.

This patch is not aligning on a pageblock boundary - it's aligning on
MAX_ORDER_NR_PAGES which is the boundary the buddy allocator works on.
This is a minor but important nit as different assumptions are made
about pageblocks and MAX_ORDER_NR_PAGES. Anyway;

Thank you for the nit. I'm kind of a VM noob so any insight is
appreciated. I naively equated the two in my head when writing the
description.

quoted

If the node is not starting on the MAX_ORDER boundary then node_start_pfn
should also not be on the same boundary but the patch does not imply
that this has been broken. If anything it appears at a glance that there
is memmap *not* being freed because it was allocated on the
MAX_ORDER_NR_PAGES boundary and only partially freed.

The comment then is confusing because the function is freeing memmap but
rounding down to free more of it ensures that all pages get a mapping?
It's not clear at all to me what was broken or how this patch fixes it
but bear in mind that it's rare I look at how ARM is initialised.

The function frees the mappings *between* the nodes. So when the patch
rounds down the start of the node, it preserves (i.e. frees less) the
mappings from MAX_ORDER_NR_PAGES boundary to the actual first valid
page. Otherwise, I would have to define CONFIG_HOLES_IN_ZONE and pay
the penalty. Looking through the history, a similar patch was already
taken to roundup the previous bank end to MAX_ORDER_NR_PAGES. I'm
doing the same to the start.

Am I totally off base?

--Dima

quoted

quoted

Round down bank_start to pageblock boundary so that whole
pageblocks always have valid mappings.

Signed-off-by: Dima Zavin <redacted>
---
?arch/arm/mm/init.c | ? ?6 +++++-
?1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 5164069..98de5d8 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c

@@ -418,7 +418,11 @@ static void __init free_unused_memmap(struct meminfo *mi)

? ? ? for_each_bank(i, mi) {
? ? ? ? ? ? ? struct membank *bank = &mi->bank[i];

- ? ? ? ? ? ? bank_start = bank_pfn_start(bank);
+ ? ? ? ? ? ? /* Round bank_start down to the start of a pageblock so that
+ ? ? ? ? ? ? ?* all pages in a pageblock always have a mapping.
+ ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? bank_start = round_down(bank_pfn_start(bank),
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? MAX_ORDER_NR_PAGES);

? ? ? ? ? ? ? /*
? ? ? ? ? ? ? ?* If we had a previous bank, and there is a space
--
1.7.3.1

--
Mel Gorman
Linux Technology Center
IBM Dublin Software Lab

On Wed, Jan 26, 2011 at 01:57:53PM -0800, Dima Zavin wrote:

On Wed, Jan 26, 2011 at 12:29 AM, Mel Gorman [off-list ref] wrote:

quoted

On Thu, Jan 20, 2011 at 03:47:11PM -0800, Dima Zavin wrote:

quoted

On some machines, the nodes do not always start on pageblock
boundaries. In these cases it is possible for free_unused_memmap
to free mappings for pages inside a pageblock with otherwise
valid pages. This presents problems for page migration since it
operates on whole pageblocks at a time.

This patch is not aligning on a pageblock boundary - it's aligning on
MAX_ORDER_NR_PAGES which is the boundary the buddy allocator works on.
This is a minor but important nit as different assumptions are made
about pageblocks and MAX_ORDER_NR_PAGES. Anyway;

Sorry for the long delay. I'm in the process of changing jobs at the
moment and my schedule is heavily disrupted. It's likely to be a mess
for at least a month.

Thank you for the nit. I'm kind of a VM noob so any insight is
appreciated. I naively equated the two in my head when writing the
description.

It's fine. The distinction is very subtle.

quoted

If the node is not starting on the MAX_ORDER boundary then node_start_pfn
should also not be on the same boundary but the patch does not imply
that this has been broken. If anything it appears at a glance that there
is memmap *not* being freed because it was allocated on the
MAX_ORDER_NR_PAGES boundary and only partially freed.

The comment then is confusing because the function is freeing memmap but
rounding down to free more of it ensures that all pages get a mapping?
It's not clear at all to me what was broken or how this patch fixes it
but bear in mind that it's rare I look at how ARM is initialised.

The function frees the mappings *between* the nodes. So when the patch
rounds down the start of the node, it preserves (i.e. frees less) the
mappings from MAX_ORDER_NR_PAGES boundary to the actual first valid
page. Otherwise, I would have to define CONFIG_HOLES_IN_ZONE and pay
the penalty. Looking through the history, a similar patch was already
taken to roundup the previous bank end to MAX_ORDER_NR_PAGES. I'm
doing the same to the start.

Am I totally off base?

No, the description and patch comment is just misleading. Clarify that
you are catching the situation where a node boundary is in the middle of a
MAX_ORDER_NR_PAGES block of pages and you are preventing memmap belonging
to another node being freed and it's fine.

-- 
Mel Gorman

Mel,

Sorry for the long delay. I'm in the process of changing jobs at the
moment and my schedule is heavily disrupted. It's likely to be a mess
for at least a month.

I totally understand. Good luck with the new job :)

No, the description and patch comment is just misleading. Clarify that
you are catching the situation where a node boundary is in the middle of a
MAX_ORDER_NR_PAGES block of pages and you are preventing memmap belonging
to another node being freed and it's fine.

Will resend with new description.

Thanks

--Dima