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

PATCH New Dima Zavin 3 participants 7 messages Jan 20, 2011 – Feb 9, 2011 Arm (32-bit)

Share & tools

Dima Zavin

author

2011-01-20 23:47 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---
 arch/arm/mm/init.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

arch/arm/mm/init.c +5 -1

index 5164069..98de5d8 100644
@@ -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

Signature · 1 line

1.7.3.1

Larry Bassel

2011-01-21 23:02 UTC #1 lore raw

PATCH 1/1 ARM: mm: enforce pageblock alignment when freeing memmap entries at init

Quoted 39 lines · Dima Zavin

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 <[email protected]>
---
 arch/arm/mm/init.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
arch/arm/mm/init.c +5 -1
index 5164069..98de5d8 100644
@@ -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
Signature · 7 lines
1.7.3.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <[email protected]>

Signature · 2 lines

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

Mel Gorman

2011-01-26 08:29 UTC #2 lore raw

PATCH 1/1 ARM: mm: enforce pageblock alignment when freeing memmap entries at init

In reply to Dima Zavin

Quoted 6 lines · Dima Zavin

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.

Quoted 28 lines

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

Signed-off-by Dima Zavin <[email protected]>

---
 arch/arm/mm/init.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

arch/arm/mm/init.c +5 -1

index 5164069..98de5d8 100644
@@ -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

Signature · 1 line

1.7.3.1

Signature · 3 lines

Mel Gorman
Linux Technology Center
IBM Dublin Software Lab

Dima Zavin

author

2011-01-26 21:57 UTC #3 lore raw

PATCH 1/1 ARM: mm: enforce pageblock alignment when freeing memmap entries at init

Quoted 8 lines · Mel Gorman

Quoted 6 lines · Dima Zavin
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.

Quoted 4 lines

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 10 lines

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 35 lines

Quoted 24 lines
Round down bank_start to pageblock boundary so that whole pageblocks always have valid mappings.
Signed-off-by Dima Zavin <[email protected]>
---
?arch/arm/mm/init.c | ? ?6 +++++-
?1 files changed, 5 insertions(+), 1 deletions(-)
arch/arm/mm/init.c +5 -1
index 5164069..98de5d8 100644
@@ -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);
 
Quoted 5 lines
? ? ? ? ? ? ? /*
? ? ? ? ? ? ? ?* If we had a previous bank, and there is a space --
1.7.3.1
--
Mel Gorman
Linux Technology Center
IBM Dublin Software Lab

Dima Zavin

author

2011-01-29 18:35 UTC #4 lore raw

PATCH 1/1 ARM: mm: enforce pageblock alignment when freeing memmap entries at init

ping?

Quoted 77 lines · Dima Zavin

Quoted 8 lines · Mel Gorman
Quoted 6 lines · Dima Zavin
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.
Quoted 4 lines
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 10 lines
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 35 lines
Quoted 24 lines
Round down bank_start to pageblock boundary so that whole pageblocks always have valid mappings.
Signed-off-by Dima Zavin <[email protected]>
---
?arch/arm/mm/init.c | ? ?6 +++++-
?1 files changed, 5 insertions(+), 1 deletions(-)
arch/arm/mm/init.c +5 -1
index 5164069..98de5d8 100644
@@ -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);
 
Quoted 5 lines
? ? ? ? ? ? ? /*
? ? ? ? ? ? ? ?* If we had a previous bank, and there is a space --
1.7.3.1
--
Mel Gorman
Linux Technology Center
IBM Dublin Software Lab

Mel Gorman

2011-02-03 13:30 UTC #5 lore raw

PATCH 1/1 ARM: mm: enforce pageblock alignment when freeing memmap entries at init

In reply to Dima Zavin

Quoted 9 lines · Dima Zavin

Quoted 8 lines · Mel Gorman
Quoted 6 lines · Dima Zavin
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.

Quoted 5 lines

Quoted 4 lines
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.

Quoted 4 lines

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 19 lines

Quoted 10 lines
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.

Quoted 2 lines

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.

Signature · 1 line

Mel Gorman

Dima Zavin

author

2011-02-09 21:39 UTC #6 lore raw

PATCH 1/1 ARM: mm: enforce pageblock alignment when freeing memmap entries at init

Mel,

Quoted 3 lines

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 :)

Quoted 4 lines

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