Skip to contentSkip to conversation

arm64:mm: free the useless initial page table

PATCH New zhichang.yuan at linaro.org 2 participants 5 messages Nov 21, 2014 – Nov 29, 2014 arm64

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 arm64:mm: free the useless initial page table

From: "zhichang.yuan" <[email protected]>

For 64K page system, after mapping a PMD section, the corresponding initial page table is not needed any more. That page can be freed.

---
 arch/arm64/mm/mmu.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
arch/arm64/mm/mmu.c +4 -1
index f4f8b50..12a336b 100644
@@ -191,8 +191,11 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
 			 * Check for previous table entries created during
 			 * boot (__create_page_tables) and flush them.
 			 */
-			if (!pmd_none(old_pmd))
+			if (!pmd_none(old_pmd)) {
 				flush_tlb_all();
+				if (pmd_table(old_pmd))
+					memblock_free(pte_pfn(pmd_pte(old_pmd)) << PAGE_SHIFT, PAGE_SIZE);
+			}
 		} else {
 			alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys),
 				       prot_pte);
Signature · 1 line
1.7.9.5
maintainer
#1 lore raw

PATCH 1/1 arm64:mm: free the useless initial page table

Quoted 15 lines · zhichang.yuan at linaro.org

From: "zhichang.yuan" <[email protected]>

For 64K page system, after mapping a PMD section, the corresponding initial page table is not needed any more. That page can be freed.

---
 arch/arm64/mm/mmu.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
arch/arm64/mm/mmu.c +0 -0
index f4f8b50..12a336b 100644

arch/arm64/mm/mmu.c static void __init alloc_init_pmd(pud_t *pud, unsigned long addr, · line 198

Quoted 9 lines
arch/arm64/mm/mmu.c +4 -1
@@ -191,8 +191,11 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
 			 * Check for previous table entries created during
 			 * boot (__create_page_tables) and flush them.
 			 */
-			if (!pmd_none(old_pmd))
+			if (!pmd_none(old_pmd)) {
 				flush_tlb_all();
+				if (pmd_table(old_pmd))
+					memblock_free(pte_pfn(pmd_pte(old_pmd)) << PAGE_SHIFT, PAGE_SIZE);
+			}

For consistency with alloc_init_pud(), could you do:

	phys_addr_t table = __pa(pte_offset(&old_pmd, 0));
	memblock_free(table, PAGE_SIZE);

Since you are at this, for alloc_init_pud() could you please move the flush_tlb_all() before memblock_free()? Theoretical problem really but it's nice for consistency.

Thanks.

Signature · 1 line
Catalin
#2 lore raw

PATCH 1/1 arm64:mm: free the useless initial page table

Quoted 27 lines · Catalin Marinas
Quoted 15 lines · zhichang.yuan at linaro.org

From: "zhichang.yuan" <[email protected]>

For 64K page system, after mapping a PMD section, the corresponding initial page table is not needed any more. That page can be freed.

---
 arch/arm64/mm/mmu.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
arch/arm64/mm/mmu.c +0 -0
index f4f8b50..12a336b 100644

arch/arm64/mm/mmu.c static void __init alloc_init_pmd(pud_t *pud, unsigned long addr, · line 198

Quoted 9 lines
arch/arm64/mm/mmu.c +4 -1
@@ -191,8 +191,11 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
 			 * Check for previous table entries created during
 			 * boot (__create_page_tables) and flush them.
 			 */
-			if (!pmd_none(old_pmd))
+			if (!pmd_none(old_pmd)) {
 				flush_tlb_all();
+				if (pmd_table(old_pmd))
+					memblock_free(pte_pfn(pmd_pte(old_pmd)) << PAGE_SHIFT, PAGE_SIZE);
+			}

For consistency with alloc_init_pud(), could you do:

Quoted 2 lines
	phys_addr_t table = __pa(pte_offset(&old_pmd, 0));
	memblock_free(table, PAGE_SIZE);

Just as Laura comments, the pte_offset will convert the PA to VA, and then convert VA to PA. It seems to be verbose. That is why i use pte_pfn. Anyway, the consistency is better, i will revise it.

Quoted 6 lines

Since you are at this, for alloc_init_pud() could you please move the flush_tlb_all() before memblock_free()? Theoretical problem really but it's nice for consistency.

Thanks.

As for this issue, i have a small question. Since the new PMD entry had been set by set_pmd, why not do the tlb flush as soon as we can?

Thanks!

maintainer
#3 lore raw

PATCH 1/1 arm64:mm: free the useless initial page table

Quoted 35 lines · zhichang.yuan
Quoted 27 lines · Catalin Marinas
Quoted 15 lines · zhichang.yuan at linaro.org

From: "zhichang.yuan" <[email protected]>

For 64K page system, after mapping a PMD section, the corresponding initial page table is not needed any more. That page can be freed.

---
 arch/arm64/mm/mmu.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
arch/arm64/mm/mmu.c +0 -0
index f4f8b50..12a336b 100644

arch/arm64/mm/mmu.c static void __init alloc_init_pmd(pud_t *pud, unsigned long addr, · line 198

Quoted 9 lines
arch/arm64/mm/mmu.c +4 -1
@@ -191,8 +191,11 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
 			 * Check for previous table entries created during
 			 * boot (__create_page_tables) and flush them.
 			 */
-			if (!pmd_none(old_pmd))
+			if (!pmd_none(old_pmd)) {
 				flush_tlb_all();
+				if (pmd_table(old_pmd))
+					memblock_free(pte_pfn(pmd_pte(old_pmd)) << PAGE_SHIFT, PAGE_SIZE);
+			}

For consistency with alloc_init_pud(), could you do:

Quoted 2 lines
	phys_addr_t table = __pa(pte_offset(&old_pmd, 0));
	memblock_free(table, PAGE_SIZE);

Just as Laura comments, the pte_offset will convert the PA to VA, and then convert VA to PA. It seems to be verbose. That is why i use pte_pfn. Anyway, the consistency is better, i will revise it.

Quoted 6 lines
Quoted 3 lines

Since you are at this, for alloc_init_pud() could you please move the flush_tlb_all() before memblock_free()? Theoretical problem really but it's nice for consistency.

As for this issue, i have a small question. Since the new PMD entry had been set by set_pmd, why not do the tlb flush as soon as we can?

So we need to flush the tlb shortly _after_ the new PMD or PUD has been set to guarantee that a TLB load will see the new page table value. In theory, we also need to flush it _before_ the old one is freed in case the freed page is used by some other CPU (not a problem for boot code) or some debug code poisons the freed page with a value that happens to match a valid translation.

Signature · 1 line
Catalin
#4 lore raw

PATCH 1/1 arm64:mm: free the useless initial page table

Quoted 38 lines · Catalin Marinas
Quoted 37 lines · zhichang.yuan
Quoted 27 lines · Catalin Marinas
Quoted 15 lines · zhichang.yuan at linaro.org

From: "zhichang.yuan" <[email protected]>

For 64K page system, after mapping a PMD section, the corresponding initial page table is not needed any more. That page can be freed.

---
 arch/arm64/mm/mmu.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
arch/arm64/mm/mmu.c +0 -0
index f4f8b50..12a336b 100644

arch/arm64/mm/mmu.c static void __init alloc_init_pmd(pud_t *pud, unsigned long addr, · line 198

Quoted 9 lines
arch/arm64/mm/mmu.c +4 -1
@@ -191,8 +191,11 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
 			 * Check for previous table entries created during
 			 * boot (__create_page_tables) and flush them.
 			 */
-			if (!pmd_none(old_pmd))
+			if (!pmd_none(old_pmd)) {
 				flush_tlb_all();
+				if (pmd_table(old_pmd))
+					memblock_free(pte_pfn(pmd_pte(old_pmd)) << PAGE_SHIFT, PAGE_SIZE);
+			}

For consistency with alloc_init_pud(), could you do:

Quoted 2 lines
	phys_addr_t table = __pa(pte_offset(&old_pmd, 0));
	memblock_free(table, PAGE_SIZE);

Just as Laura comments, the pte_offset will convert the PA to VA, and then convert VA to PA. It seems to be verbose. That is why i use pte_pfn. Anyway, the consistency is better, i will revise it.

Quoted 3 lines

Since you are at this, for alloc_init_pud() could you please move the flush_tlb_all() before memblock_free()? Theoretical problem really but it's nice for consistency.

Quoted 9 lines
Quoted 2 lines

As for this issue, i have a small question. Since the new PMD entry had been set by set_pmd, why not do the tlb flush as soon as we can?

So we need to flush the tlb shortly _after_ the new PMD or PUD has been set to guarantee that a TLB load will see the new page table value. In theory, we also need to flush it _before_ the old one is freed in case the freed page is used by some other CPU (not a problem for boot code) or some debug code poisons the freed page with a value that happens to match a valid translation.

I misunderstood your comment before. I thought you ask me to make alloc_init_pmd consistent to alloc_init_pud. sorry for that!

I will adjust the line sequence in alloc_init_pud too.

Thanks!