[PATCH] x86: mm: accurate the comments for STEP_SIZE_SHIFT macro

[PATCH] x86: mm: accurate the comments for STEP_SIZE_SHIFT macro

[Lin Feng]

For x86 PUD_SHIFT is 30 and PMD_SHIFT is 21, so the consequence of
(PUD_SHIFT-PMD_SHIFT)/2 is 4. Update the comments to the code.

Cc: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Lin Feng <linfeng@cn.fujitsu.com>
---
 arch/x86/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 59b7fc4..637a95b 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -389,7 +389,7 @@ static unsigned long __init init_range_memory_mapping(
 	return mapped_ram_size;
 }
 
-/* (PUD_SHIFT-PMD_SHIFT)/2 */
+/* (PUD_SHIFT-PMD_SHIFT)/2+1 */
 #define STEP_SIZE_SHIFT 5
 void __init init_mem_mapping(void)
 {
-- 
1.8.0.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86: mm: accurate the comments for STEP_SIZE_SHIFT macro

[Yinghai Lu]

On Mon, Mar 18, 2013 at 3:21 AM, Lin Feng <linfeng@cn.fujitsu.com> wrote:
> For x86 PUD_SHIFT is 30 and PMD_SHIFT is 21, so the consequence of
> (PUD_SHIFT-PMD_SHIFT)/2 is 4. Update the comments to the code.
>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Lin Feng <linfeng@cn.fujitsu.com>
> ---
>  arch/x86/mm/init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index 59b7fc4..637a95b 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -389,7 +389,7 @@ static unsigned long __init init_range_memory_mapping(
>         return mapped_ram_size;
>  }
>
> -/* (PUD_SHIFT-PMD_SHIFT)/2 */
> +/* (PUD_SHIFT-PMD_SHIFT)/2+1 */
>  #define STEP_SIZE_SHIFT 5
>  void __init init_mem_mapping(void)
>  {

9/2=4.5, so it becomes 5.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86: mm: accurate the comments for STEP_SIZE_SHIFT macro

[H. Peter Anvin]

On 03/18/2013 11:53 AM, Yinghai Lu wrote:
> On Mon, Mar 18, 2013 at 3:21 AM, Lin Feng <linfeng@cn.fujitsu.com> wrote:
>> For x86 PUD_SHIFT is 30 and PMD_SHIFT is 21, so the consequence of
>> (PUD_SHIFT-PMD_SHIFT)/2 is 4. Update the comments to the code.
>>
>> Cc: Yinghai Lu <yinghai@kernel.org>
>> Signed-off-by: Lin Feng <linfeng@cn.fujitsu.com>
>> ---
>>  arch/x86/mm/init.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
>> index 59b7fc4..637a95b 100644
>> --- a/arch/x86/mm/init.c
>> +++ b/arch/x86/mm/init.c
>> @@ -389,7 +389,7 @@ static unsigned long __init init_range_memory_mapping(
>>         return mapped_ram_size;
>>  }
>>
>> -/* (PUD_SHIFT-PMD_SHIFT)/2 */
>> +/* (PUD_SHIFT-PMD_SHIFT)/2+1 */
>>  #define STEP_SIZE_SHIFT 5
>>  void __init init_mem_mapping(void)
>>  {
> 
> 9/2=4.5, so it becomes 5.
> 

No, it doesn't.  This is C, not elementary school  Now I'm really bothered.

The comment doesn't say *why* (PUD_SHIFT-PMD_SHIFT)/2 or any other
variant is correct, furthermore I suspect that the +1 is misplaced.
However, what is really needed is:

1. Someone needs to explain what the logic should be and why, and
2. replace the macro with a symbolic macro, not with a constant and a
   comment explaining, incorrectly, how that value was derived.

	-hpa

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86: mm: accurate the comments for STEP_SIZE_SHIFT macro

[Yinghai Lu]

On Mon, Mar 18, 2013 at 11:59 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 03/18/2013 11:53 AM, Yinghai Lu wrote:
>> On Mon, Mar 18, 2013 at 3:21 AM, Lin Feng <linfeng@cn.fujitsu.com> wrote:
>>> For x86 PUD_SHIFT is 30 and PMD_SHIFT is 21, so the consequence of
>>> (PUD_SHIFT-PMD_SHIFT)/2 is 4. Update the comments to the code.
>>>
>>> Cc: Yinghai Lu <yinghai@kernel.org>
>>> Signed-off-by: Lin Feng <linfeng@cn.fujitsu.com>
>>> ---
>>>  arch/x86/mm/init.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
>>> index 59b7fc4..637a95b 100644
>>> --- a/arch/x86/mm/init.c
>>> +++ b/arch/x86/mm/init.c
>>> @@ -389,7 +389,7 @@ static unsigned long __init init_range_memory_mapping(
>>>         return mapped_ram_size;
>>>  }
>>>
>>> -/* (PUD_SHIFT-PMD_SHIFT)/2 */
>>> +/* (PUD_SHIFT-PMD_SHIFT)/2+1 */
>>>  #define STEP_SIZE_SHIFT 5
>>>  void __init init_mem_mapping(void)
>>>  {
>>
>> 9/2=4.5, so it becomes 5.
>>
>
> No, it doesn't.  This is C, not elementary school  Now I'm really bothered.
>
> The comment doesn't say *why* (PUD_SHIFT-PMD_SHIFT)/2 or any other
> variant is correct, furthermore I suspect that the +1 is misplaced.
> However, what is really needed is:
>
> 1. Someone needs to explain what the logic should be and why, and
> 2. replace the macro with a symbolic macro, not with a constant and a
>    comment explaining, incorrectly, how that value was derived.

yes, we should find out free_mem_size instead to decide next step size.

But that will come out page table size estimation problem again.

Yinghai

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86: mm: accurate the comments for STEP_SIZE_SHIFT macro

[H. Peter Anvin]

On 03/18/2013 12:13 PM, Yinghai Lu wrote:
>>
>> No, it doesn't.  This is C, not elementary school  Now I'm really bothered.
>>
>> The comment doesn't say *why* (PUD_SHIFT-PMD_SHIFT)/2 or any other
>> variant is correct, furthermore I suspect that the +1 is misplaced.
>> However, what is really needed is:
>>
>> 1. Someone needs to explain what the logic should be and why, and
>> 2. replace the macro with a symbolic macro, not with a constant and a
>>    comment explaining, incorrectly, how that value was derived.
> 
> yes, we should find out free_mem_size instead to decide next step size.
> 
> But that will come out page table size estimation problem again.
> 

Sorry, that comment is double nonsense for someone who isn't intimately
familiar with the code, and it sounds like it is just plain wrong.

Instead, try to explain why 5 is the correct value in the current code
and how it is (or should be!) derived.

	-hpa



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] x86: mm: accurate the comments for STEP_SIZE_SHIFT macro

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]

On Mon, Mar 18, 2013 at 12:14 PM, H. Peter Anvin <hpa@zytor.com> wrote:

> Instead, try to explain why 5 is the correct value in the current code
> and how it is (or should be!) derived.

initial mapped size is PMD_SIZE, aka 2M.
if we use step_size to be PUD_SIZE aka 1G, as most worse case
that 1G is cross the 1G boundary, and PG_LEVEL_2M is not set,
we will need 1+1+512 pages (aka 2M + 8k) to map 1G range with PTE.
So i picked (30-21)/2 to get 5.

Please check attached patch.

Thanks

Yinghai

[-- Attachment #2: add_comment_for_step_size.patch --]
[-- Type: application/octet-stream, Size: 1551 bytes --]

Subject: [PATCH] x86, mm: Add comments for step_size shift

As request by hpa, add comments for why we choose 5 for
step size shift.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/mm/init.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Index: linux-2.6/arch/x86/mm/init.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/init.c
+++ linux-2.6/arch/x86/mm/init.c
@@ -389,8 +389,23 @@ static unsigned long __init init_range_m
 	return mapped_ram_size;
 }
 
-/* (PUD_SHIFT-PMD_SHIFT)/2 */
-#define STEP_SIZE_SHIFT 5
+static unsigned long __init get_new_step_size(unsigned long step_size)
+{
+	/*
+	 * initial mapped size is PMD_SIZE, aka 2M.
+	 * We can not set step_size to be PUD_SIZE aka 1G yet.
+	 * In worse case, when 1G is cross the 1G boundary, and
+	 * PG_LEVEL_2M is not set, we will need 1+1+512 pages (aka 2M + 8k)
+	 * to map 1G range with PTE. Use 5 as shift for now.
+	 */
+	unsigned long new_step_size = step_size << 5;
+
+	if (new_step_size > step_size)
+		step_size = new_step_size;
+
+	return  step_size;
+}
+
 void __init init_mem_mapping(void)
 {
 	unsigned long end, real_end, start, last_start;
@@ -432,7 +447,7 @@ void __init init_mem_mapping(void)
 		min_pfn_mapped = last_start >> PAGE_SHIFT;
 		/* only increase step_size after big range get mapped */
 		if (new_mapped_ram_size > mapped_ram_size)
-			step_size <<= STEP_SIZE_SHIFT;
+			step_size = get_new_step_size(step_size);
 		mapped_ram_size += new_mapped_ram_size;
 	}
 

Re: [PATCH] x86: mm: accurate the comments for STEP_SIZE_SHIFT macro

[H. Peter Anvin]

On 03/18/2013 02:19 PM, Yinghai Lu wrote:
> On Mon, Mar 18, 2013 at 12:14 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> 
>> Instead, try to explain why 5 is the correct value in the current code
>> and how it is (or should be!) derived.
> 
> initial mapped size is PMD_SIZE, aka 2M.
> if we use step_size to be PUD_SIZE aka 1G, as most worse case
> that 1G is cross the 1G boundary, and PG_LEVEL_2M is not set,
> we will need 1+1+512 pages (aka 2M + 8k) to map 1G range with PTE.
> So i picked (30-21)/2 to get 5.
> 
> Please check attached patch.
> 
> Thanks
> 
> Yinghai
> 

This still seems very opaque.  I need to look at it and see if it makes
more sense in context.

	-hpa

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>