x86/mm: Simplify memory mapping PFN calculation

PATCH Superseded Pekka Enberg 3 participants 8 messages Jul 14, 2012

Subsystems (2)

x86 x86 MM

lore

Share & tools

lore

Pekka Enberg

author

2012-07-14 13:41 UTC #0 lore raw

PATCH 1/3 x86/mm: Simplify memory mapping PFN calculation

Introduce two new helper functions, addr_to_pmd_pfn() and addr_to_pud_pfn(), to simplify init_memory_mapping() code flow.

Cc Tejun Heo <[email protected]>

Cc Yinghai Lu <[email protected]>

Signed-off-by Pekka Enberg <[email protected]>

---
 arch/x86/mm/init.c |   38 +++++++++++++++++++++-----------------
 1 files changed, 21 insertions(+), 17 deletions(-)

arch/x86/mm/init.c +21 ~~-17~~

index bc4e9d8..9eb53c2 100644
@@ -117,6 +117,16 @@ static int __meminit save_mr(struct map_range *mr, int nr_range,
 	return nr_range;
 }
 
+static unsigned long addr_to_pmd_pfn(unsigned long addr)
+{
+	return (addr >> PMD_SHIFT) << (PMD_SHIFT - PAGE_SHIFT);
+}
+
+static unsigned long addr_to_pud_pfn(unsigned long addr)
+{
+	return (addr >> PUD_SHIFT) << (PUD_SHIFT - PAGE_SHIFT);
+}
+
 /*
  * Setup the direct mapping of the physical memory at PAGE_OFFSET.
  * This runs before bootmem is initialized and gets pages directly from

@@ -180,11 +190,9 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
 	if (pos == 0)
 		end_pfn = 1<<(PMD_SHIFT - PAGE_SHIFT);
 	else
-		end_pfn = ((pos + (PMD_SIZE - 1))>>PMD_SHIFT)
-				 << (PMD_SHIFT - PAGE_SHIFT);
+		end_pfn = addr_to_pmd_pfn(pos + (PMD_SIZE - 1));
 #else /* CONFIG_X86_64 */
-	end_pfn = ((pos + (PMD_SIZE - 1)) >> PMD_SHIFT)
-			<< (PMD_SHIFT - PAGE_SHIFT);
+	end_pfn = addr_to_pmd_pfn(pos + (PMD_SIZE - 1));
 #endif
 	if (end_pfn > (end >> PAGE_SHIFT))
 		end_pfn = end >> PAGE_SHIFT;

@@ -194,15 +202,13 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
 	}
 
 	/* big page (2M) range */
-	start_pfn = ((pos + (PMD_SIZE - 1))>>PMD_SHIFT)
-			 << (PMD_SHIFT - PAGE_SHIFT);
+	start_pfn = addr_to_pmd_pfn(pos + (PMD_SIZE - 1));
 #ifdef CONFIG_X86_32
-	end_pfn = (end>>PMD_SHIFT) << (PMD_SHIFT - PAGE_SHIFT);
+	end_pfn = addr_to_pmd_pfn(end);
 #else /* CONFIG_X86_64 */
-	end_pfn = ((pos + (PUD_SIZE - 1))>>PUD_SHIFT)
-			 << (PUD_SHIFT - PAGE_SHIFT);
-	if (end_pfn > ((end>>PMD_SHIFT)<<(PMD_SHIFT - PAGE_SHIFT)))
-		end_pfn = ((end>>PMD_SHIFT)<<(PMD_SHIFT - PAGE_SHIFT));
+	end_pfn = addr_to_pud_pfn(pos + (PUD_SIZE - 1));
+	if (end_pfn > addr_to_pmd_pfn(end))
+		end_pfn = addr_to_pmd_pfn(end);
 #endif
 
 	if (start_pfn < end_pfn) {

@@ -213,9 +219,8 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
 
 #ifdef CONFIG_X86_64
 	/* big page (1G) range */
-	start_pfn = ((pos + (PUD_SIZE - 1))>>PUD_SHIFT)
-			 << (PUD_SHIFT - PAGE_SHIFT);
-	end_pfn = (end >> PUD_SHIFT) << (PUD_SHIFT - PAGE_SHIFT);
+	start_pfn = addr_to_pud_pfn(pos + (PUD_SIZE - 1));
+	end_pfn = addr_to_pud_pfn(end);
 	if (start_pfn < end_pfn) {
 		nr_range = save_mr(mr, nr_range, start_pfn, end_pfn,
 				page_size_mask &

@@ -224,9 +229,8 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
 	}
 
 	/* tail is not big page (1G) alignment */
-	start_pfn = ((pos + (PMD_SIZE - 1))>>PMD_SHIFT)
-			 << (PMD_SHIFT - PAGE_SHIFT);
-	end_pfn = (end >> PMD_SHIFT) << (PMD_SHIFT - PAGE_SHIFT);
+	start_pfn = addr_to_pmd_pfn(pos + (PMD_SIZE - 1));
+	end_pfn = addr_to_pmd_pfn(end);
 	if (start_pfn < end_pfn) {
 		nr_range = save_mr(mr, nr_range, start_pfn, end_pfn,
 				page_size_mask & (1<<PG_LEVEL_2M));

Signature · 1 line

1.7.7.6

Pekka Enberg

author

2012-07-14 13:41 UTC #1 lore raw

PATCH 2/3 x86/mm: Simplify for-loop in free_init_pages()

As a cleanup, move initial "addr" assignment to the for-loop construct in free_init_pages().

Cc Tejun Heo <[email protected]>

Cc Yinghai Lu <[email protected]>

Signed-off-by Pekka Enberg <[email protected]>

---
 arch/x86/mm/init.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

arch/x86/mm/init.c +1 -3

index 9eb53c2..e270f94 100644
@@ -349,8 +349,6 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
 	if (begin >= end)
 		return;
 
-	addr = begin;
-
 	/*
 	 * If debugging page accesses then do not free this memory but
 	 * mark them not present - any buggy init-section access will

@@ -371,7 +369,7 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
 
 	printk(KERN_INFO "Freeing %s: %luk freed\n", what, (end - begin) >> 10);
 
-	for (; addr < end; addr += PAGE_SIZE) {
+	for (addr = begin; addr < end; addr += PAGE_SIZE) {
 		ClearPageReserved(virt_to_page(addr));
 		init_page_count(virt_to_page(addr));
 		memset((void *)addr, POISON_FREE_INITMEM, PAGE_SIZE);

Signature · 1 line

1.7.7.6

Yinghai Lu

Acked-by on v1 2/3

2012-07-14 20:36 UTC #2 lore raw

Re: [PATCH 2/3] x86/mm: Simplify for-loop in free_init_pages()

Quoted 6 lines · Pekka Enberg

As a cleanup, move initial "addr" assignment to the for-loop construct in free_init_pages().
Cc Tejun Heo <[email protected]>
Cc Yinghai Lu <[email protected]>
Signed-off-by Pekka Enberg <[email protected]>

Acked-by Yinghai Lu <[email protected]>

Quoted 29 lines

---
 arch/x86/mm/init.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

arch/x86/mm/init.c +1 -4

index 9eb53c2..e270f94 100644
@@ -349,8 +349,6 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
        if (begin >= end)
                return;
 
-       addr = begin;
-
        /*
         * If debugging page accesses then do not free this memory but
         * mark them not present - any buggy init-section access will

@@ -371,7 +369,7 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
 
        printk(KERN_INFO "Freeing %s: %luk freed\n", what, (end - begin) >> 10);
 
-       for (; addr < end; addr += PAGE_SIZE) {
+       for (addr = begin; addr < end; addr += PAGE_SIZE) {
                ClearPageReserved(virt_to_page(addr));
                init_page_count(virt_to_page(addr));
                memset((void *)addr, POISON_FREE_INITMEM, PAGE_SIZE);
--

1.7.7.6

Joe Perches

2012-07-14 20:43 UTC #3 lore raw

Re: [PATCH 2/3] x86/mm: Simplify for-loop in free_init_pages()

Quoted 3 lines · Yinghai Lu

Quoted 2 lines · Pekka Enberg
As a cleanup, move initial "addr" assignment to the for-loop construct in free_init_pages().

Not really a good idea.

Quoted 1 line

arch/x86/mm/init.c
Quoted 1 line
arch/x86/mm/init.c +0 -0

[]

Quoted 11 lines

arch/x86/mm/init.c +0 -2

@@ -349,8 +349,6 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
        if (begin >= end)
                return;
 
-       addr = begin;
-
        /*
         * If debugging page accesses then do not free this memory but
         * mark them not present - any buggy init-section access will

@@ -371,7 +369,7 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)

Quoted 8 lines

arch/x86/mm/init.c void free_init_pages(char *what, unsigned long begin, unsigned long end)

Quoted 8 lines

arch/x86/mm/init.c +1 -2

        printk(KERN_INFO "Freeing %s: %luk freed\n", what, (end - begin) >> 10);
 
-       for (; addr < end; addr += PAGE_SIZE) {
+       for (addr = begin; addr < end; addr += PAGE_SIZE) {
                ClearPageReserved(virt_to_page(addr));
                init_page_count(virt_to_page(addr));
                memset((void *)addr, POISON_FREE_INITMEM, PAGE_SIZE);
--

Now there's an unused variable warning when CONFIG_DEBUG_PAGEALLOC

Pekka Enberg

author

2012-07-14 20:48 UTC #4 lore raw

Re: [PATCH 2/3] x86/mm: Simplify for-loop in free_init_pages()

Quoted 6 lines · Joe Perches

Quoted 3 lines · Yinghai Lu
Quoted 2 lines · Pekka Enberg
As a cleanup, move initial "addr" assignment to the for-loop construct in free_init_pages().
Not really a good idea.

Of course it's a good idea. The current code flow makes no sense.

Quoted 15 lines

Quoted 1 line

arch/x86/mm/init.c
Quoted 1 line
arch/x86/mm/init.c +0 -0

[]

Quoted 13 lines

arch/x86/mm/init.c +0 -2

@@ -349,8 +349,6 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
        if (begin >= end)
                return;
 
-       addr = begin;
-
        /*
         * If debugging page accesses then do not free this memory but
         * mark them not present - any buggy init-section access will

@@ -371,7 +369,7 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
 
        printk(KERN_INFO "Freeing %s: %luk freed\n", what, (end - begin) >> 10);

Quoted 8 lines

Quoted 6 lines
arch/x86/mm/init.c void free_init_pages(char *what, unsigned long begin, unsigned long end)
Quoted 6 lines
arch/x86/mm/init.c +1 -2
-       for (; addr < end; addr += PAGE_SIZE) {
+       for (addr = begin; addr < end; addr += PAGE_SIZE) {
                ClearPageReserved(virt_to_page(addr));
                init_page_count(virt_to_page(addr));
                memset((void *)addr, POISON_FREE_INITMEM, PAGE_SIZE);
--
Now there's an unused variable warning when CONFIG_DEBUG_PAGEALLOC

Sure, that needs fixing. It'd probably be simplest to introduce a __free_init_pages() helper.

Joe Perches

2012-07-14 20:51 UTC #5 lore raw

Re: [PATCH 2/3] x86/mm: Simplify for-loop in free_init_pages()

Quoted 34 lines · Pekka Enberg

Quoted 6 lines · Joe Perches
Quoted 3 lines · Yinghai Lu
Quoted 2 lines · Pekka Enberg
As a cleanup, move initial "addr" assignment to the for-loop construct in free_init_pages().
Not really a good idea.
Of course it's a good idea. The current code flow makes no sense.
Quoted 15 lines
Quoted 1 line
arch/x86/mm/init.c
Quoted 1 line
arch/x86/mm/init.c +0 -0
[]
Quoted 13 lines
Quoted 13 lines
arch/x86/mm/init.c +0 -2
@@ -349,8 +349,6 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
        if (begin >= end)
                return;
 
-       addr = begin;
-
        /*
         * If debugging page accesses then do not free this memory but
         * mark them not present - any buggy init-section access will
@@ -371,7 +369,7 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
 
        printk(KERN_INFO "Freeing %s: %luk freed\n", what, (end - begin) >> 10);
 
Quoted 8 lines
Quoted 6 lines
arch/x86/mm/init.c void free_init_pages(char *what, unsigned long begin, unsigned long end)
Quoted 6 lines
arch/x86/mm/init.c +1 -2
-       for (; addr < end; addr += PAGE_SIZE) {
+       for (addr = begin; addr < end; addr += PAGE_SIZE) {
                ClearPageReserved(virt_to_page(addr));
                init_page_count(virt_to_page(addr));
                memset((void *)addr, POISON_FREE_INITMEM, PAGE_SIZE);
--
Now there's an unused variable warning when CONFIG_DEBUG_PAGEALLOC

Quoted 3 lines

Sure, that needs fixing. It'd probably be simplest to introduce a __free_init_pages() helper.

The proposed patch introduces a warning.
That's the only reason it's not a good idea.
There are lots of ways to fix it.

Joe Perches

2012-07-14 22:38 UTC #6 lore raw

Re: [PATCH 2/3] x86/mm: Simplify for-loop in free_init_pages()

In reply to Pekka Enberg

Quoted 34 lines · Pekka Enberg

Quoted 6 lines · Joe Perches
Quoted 3 lines · Yinghai Lu
Quoted 2 lines · Pekka Enberg
As a cleanup, move initial "addr" assignment to the for-loop construct in free_init_pages().
Not really a good idea.
Of course it's a good idea. The current code flow makes no sense.
Quoted 15 lines
Quoted 1 line
arch/x86/mm/init.c
Quoted 1 line
arch/x86/mm/init.c +0 -0
[]
Quoted 13 lines
Quoted 13 lines
arch/x86/mm/init.c +0 -2
@@ -349,8 +349,6 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
        if (begin >= end)
                return;
 
-       addr = begin;
-
        /*
         * If debugging page accesses then do not free this memory but
         * mark them not present - any buggy init-section access will
@@ -371,7 +369,7 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
 
        printk(KERN_INFO "Freeing %s: %luk freed\n", what, (end - begin) >> 10);
 
Quoted 8 lines
Quoted 6 lines
arch/x86/mm/init.c void free_init_pages(char *what, unsigned long begin, unsigned long end)
Quoted 6 lines
arch/x86/mm/init.c +1 -2
-       for (; addr < end; addr += PAGE_SIZE) {
+       for (addr = begin; addr < end; addr += PAGE_SIZE) {
                ClearPageReserved(virt_to_page(addr));
                init_page_count(virt_to_page(addr));
                memset((void *)addr, POISON_FREE_INITMEM, PAGE_SIZE);
--
Now there's an unused variable warning when CONFIG_DEBUG_PAGEALLOC

Quoted 3 lines

Sure, that needs fixing. It'd probably be simplest to introduce a __free_init_pages() helper.

Perhaps:

 arch/x86/mm/init.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

arch/x86/mm/init.c +6 -8

index bc4e9d8..1023484 100644
@@ -330,7 +330,6 @@ int devmem_is_allowed(unsigned long pagenr)
 
 void free_init_pages(char *what, unsigned long begin, unsigned long end)
 {
-	unsigned long addr;
 	unsigned long begin_aligned, end_aligned;
 
 	/* Make sure boundaries are page aligned */

@@ -345,8 +344,6 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
 	if (begin >= end)
 		return;
 
-	addr = begin;
-
 	/*
 	 * If debugging page accesses then do not free this memory but
 	 * mark them not present - any buggy init-section access will

@@ -367,12 +364,13 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
 
 	printk(KERN_INFO "Freeing %s: %luk freed\n", what, (end - begin) >> 10);
 
-	for (; addr < end; addr += PAGE_SIZE) {
-		ClearPageReserved(virt_to_page(addr));
-		init_page_count(virt_to_page(addr));
-		memset((void *)addr, POISON_FREE_INITMEM, PAGE_SIZE);
-		free_page(addr);
+	while (begin < end) {
+		ClearPageReserved(virt_to_page(begin));
+		init_page_count(virt_to_page(begin));
+		memset((void *)begin, POISON_FREE_INITMEM, PAGE_SIZE);
+		free_page(begin);
 		totalram_pages++;
+		begin += PAGE_SIZE;
 	}
 #endif
 }

Yinghai Lu

Acked-by on v1 1/3

2012-07-14 20:34 UTC #7 lore raw

In reply to Pekka Enberg

Quoted 6 lines · Pekka Enberg

Introduce two new helper functions, addr_to_pmd_pfn() and addr_to_pud_pfn(), to simplify init_memory_mapping() code flow.
Cc Tejun Heo <[email protected]>
Cc Yinghai Lu <[email protected]>
Signed-off-by Pekka Enberg <[email protected]>

Acked-by Yinghai Lu <[email protected]>

Quoted 87 lines

---
 arch/x86/mm/init.c |   38 +++++++++++++++++++++-----------------
 1 files changed, 21 insertions(+), 17 deletions(-)

arch/x86/mm/init.c +21 ~~-18~~

index bc4e9d8..9eb53c2 100644
@@ -117,6 +117,16 @@ static int __meminit save_mr(struct map_range *mr, int nr_range,
        return nr_range;
 }
 
+static unsigned long addr_to_pmd_pfn(unsigned long addr)
+{
+       return (addr >> PMD_SHIFT) << (PMD_SHIFT - PAGE_SHIFT);
+}
+
+static unsigned long addr_to_pud_pfn(unsigned long addr)
+{
+       return (addr >> PUD_SHIFT) << (PUD_SHIFT - PAGE_SHIFT);
+}
+
 /*
  * Setup the direct mapping of the physical memory at PAGE_OFFSET.
  * This runs before bootmem is initialized and gets pages directly from

@@ -180,11 +190,9 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
        if (pos == 0)
                end_pfn = 1<<(PMD_SHIFT - PAGE_SHIFT);
        else
-               end_pfn = ((pos + (PMD_SIZE - 1))>>PMD_SHIFT)
-                                << (PMD_SHIFT - PAGE_SHIFT);
+               end_pfn = addr_to_pmd_pfn(pos + (PMD_SIZE - 1));
 #else /* CONFIG_X86_64 */
-       end_pfn = ((pos + (PMD_SIZE - 1)) >> PMD_SHIFT)
-                       << (PMD_SHIFT - PAGE_SHIFT);
+       end_pfn = addr_to_pmd_pfn(pos + (PMD_SIZE - 1));
 #endif
        if (end_pfn > (end >> PAGE_SHIFT))
                end_pfn = end >> PAGE_SHIFT;

@@ -194,15 +202,13 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
        }
 
        /* big page (2M) range */
-       start_pfn = ((pos + (PMD_SIZE - 1))>>PMD_SHIFT)
-                        << (PMD_SHIFT - PAGE_SHIFT);
+       start_pfn = addr_to_pmd_pfn(pos + (PMD_SIZE - 1));
 #ifdef CONFIG_X86_32
-       end_pfn = (end>>PMD_SHIFT) << (PMD_SHIFT - PAGE_SHIFT);
+       end_pfn = addr_to_pmd_pfn(end);
 #else /* CONFIG_X86_64 */
-       end_pfn = ((pos + (PUD_SIZE - 1))>>PUD_SHIFT)
-                        << (PUD_SHIFT - PAGE_SHIFT);
-       if (end_pfn > ((end>>PMD_SHIFT)<<(PMD_SHIFT - PAGE_SHIFT)))
-               end_pfn = ((end>>PMD_SHIFT)<<(PMD_SHIFT - PAGE_SHIFT));
+       end_pfn = addr_to_pud_pfn(pos + (PUD_SIZE - 1));
+       if (end_pfn > addr_to_pmd_pfn(end))
+               end_pfn = addr_to_pmd_pfn(end);
 #endif
 
        if (start_pfn < end_pfn) {

@@ -213,9 +219,8 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
 
 #ifdef CONFIG_X86_64
        /* big page (1G) range */
-       start_pfn = ((pos + (PUD_SIZE - 1))>>PUD_SHIFT)
-                        << (PUD_SHIFT - PAGE_SHIFT);
-       end_pfn = (end >> PUD_SHIFT) << (PUD_SHIFT - PAGE_SHIFT);
+       start_pfn = addr_to_pud_pfn(pos + (PUD_SIZE - 1));
+       end_pfn = addr_to_pud_pfn(end);
        if (start_pfn < end_pfn) {
                nr_range = save_mr(mr, nr_range, start_pfn, end_pfn,
                                page_size_mask &

@@ -224,9 +229,8 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
        }
 
        /* tail is not big page (1G) alignment */
-       start_pfn = ((pos + (PMD_SIZE - 1))>>PMD_SHIFT)
-                        << (PMD_SHIFT - PAGE_SHIFT);
-       end_pfn = (end >> PMD_SHIFT) << (PMD_SHIFT - PAGE_SHIFT);
+       start_pfn = addr_to_pmd_pfn(pos + (PMD_SIZE - 1));
+       end_pfn = addr_to_pmd_pfn(end);
        if (start_pfn < end_pfn) {
                nr_range = save_mr(mr, nr_range, start_pfn, end_pfn,
                                page_size_mask & (1<<PG_LEVEL_2M));
--

1.7.7.6