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

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

[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 <dima@android.com>
---
 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

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

[Larry Bassel]

On 20 Jan 11 15:47, 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.
> 
> Round down bank_start to pageblock boundary so that whole
> pageblocks always have valid mappings.
> 
> Signed-off-by: Dima Zavin <dima@android.com>
> ---
>  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
> 
> 
> _______________________________________________
> 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 <lbassel@codeaurora.org>

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

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

[Mel Gorman]

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.

> Round down bank_start to pageblock boundary so that whole
> pageblocks always have valid mappings.
> 
> Signed-off-by: Dima Zavin <dima@android.com>
> ---
>  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

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

[Dima Zavin]

On Wed, Jan 26, 2011 at 12:29 AM, Mel Gorman <mel@csn.ul.ie> wrote:
> 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;

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

>
>> Round down bank_start to pageblock boundary so that whole
>> pageblocks always have valid mappings.
>>
>> Signed-off-by: Dima Zavin <dima@android.com>
>> ---
>> ?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
>

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

[Dima Zavin]

ping?

On Wed, Jan 26, 2011 at 1:57 PM, Dima Zavin <dima@android.com> wrote:
> On Wed, Jan 26, 2011 at 12:29 AM, Mel Gorman <mel@csn.ul.ie> wrote:
>> 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;
>
> 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
>
>>
>>> Round down bank_start to pageblock boundary so that whole
>>> pageblocks always have valid mappings.
>>>
>>> Signed-off-by: Dima Zavin <dima@android.com>
>>> ---
>>> ?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
>>
>

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

[Mel Gorman]

On Wed, Jan 26, 2011 at 01:57:53PM -0800, Dima Zavin wrote:
> On Wed, Jan 26, 2011 at 12:29 AM, Mel Gorman <mel@csn.ul.ie> wrote:
> > 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;
> 

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.

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

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

[Dima Zavin]

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