[PATCH 0/3] A Solution to Re-enable hugetlb vmemmap optimize on Arm

[PATCH 0/3] A Solution to Re-enable hugetlb vmemmap optimize on Arm

[Nanyong Sun]

HVO was previously disabled on arm64 [1] due to the lack of necessary
BBM(break-before-make) logic when changing page tables.
This set of patches fix this by adding necessary BBM sequence when
changing page table, and supporting vmemmap page fault handling to
fixup kernel address fault if vmemmap is concurrently accessed.

[1] commit 060a2c92d1b6 ("arm64: mm: hugetlb: Disable HUGETLB_PAGE_OPTIMIZE_VMEMMAP")

Nanyong Sun (3):
  mm: HVO: introduce helper function to update and flush pgtable
  arm64: mm: HVO: support BBM of vmemmap pgtable safely
  arm64: mm: Re-enable OPTIMIZE_HUGETLB_VMEMMAP

 arch/arm64/Kconfig           |  1 +
 arch/arm64/include/asm/esr.h |  4 ++
 arch/arm64/include/asm/mmu.h | 20 ++++++++
 arch/arm64/mm/fault.c        | 94 ++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mmu.c          | 28 +++++++++++
 mm/hugetlb_vmemmap.c         | 55 ++++++++++++++++-----
 6 files changed, 190 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH 1/3] mm: HVO: introduce helper function to update and flush pgtable

[Nanyong Sun]

Add pmd/pte update and tlb flush helper function to update page
table. This refactoring patch is designed to facilitate each
architecture to implement its own special logic in preparation
for the arm64 architecture to follow the necessary break-before-make
sequence when updating page tables.

Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
---
 mm/hugetlb_vmemmap.c | 55 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index 87818ee7f01d..49e8b351def3 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -45,6 +45,37 @@ struct vmemmap_remap_walk {
 	unsigned long		flags;
 };
 
+#ifndef vmemmap_update_pmd
+static inline void vmemmap_update_pmd(unsigned long start,
+				      pmd_t *pmd, pte_t *pgtable)
+{
+	pmd_populate_kernel(&init_mm, pmd, pgtable);
+}
+#endif
+
+#ifndef vmemmap_update_pte
+static inline void vmemmap_update_pte(unsigned long addr,
+				      pte_t *pte, pte_t entry)
+{
+	set_pte_at(&init_mm, addr, pte, entry);
+}
+#endif
+
+#ifndef flush_tlb_vmemmap_all
+static inline void flush_tlb_vmemmap_all(void)
+{
+	flush_tlb_all();
+}
+#endif
+
+#ifndef flush_tlb_vmemmap_range
+static inline void flush_tlb_vmemmap_range(unsigned long start,
+					   unsigned long end)
+{
+	flush_tlb_kernel_range(start, end);
+}
+#endif
+
 static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, bool flush)
 {
 	pmd_t __pmd;
@@ -87,9 +118,9 @@ static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, bool flush)
 
 		/* Make pte visible before pmd. See comment in pmd_install(). */
 		smp_wmb();
-		pmd_populate_kernel(&init_mm, pmd, pgtable);
+		vmemmap_update_pmd(start, pmd, pgtable);
 		if (flush)
-			flush_tlb_kernel_range(start, start + PMD_SIZE);
+			flush_tlb_vmemmap_range(start, start + PMD_SIZE);
 	} else {
 		pte_free_kernel(&init_mm, pgtable);
 	}
@@ -217,7 +248,7 @@ static int vmemmap_remap_range(unsigned long start, unsigned long end,
 	} while (pgd++, addr = next, addr != end);
 
 	if (walk->remap_pte && !(walk->flags & VMEMMAP_REMAP_NO_TLB_FLUSH))
-		flush_tlb_kernel_range(start, end);
+		flush_tlb_vmemmap_range(start, end);
 
 	return 0;
 }
@@ -263,15 +294,15 @@ static void vmemmap_remap_pte(pte_t *pte, unsigned long addr,
 
 		/*
 		 * Makes sure that preceding stores to the page contents from
-		 * vmemmap_remap_free() become visible before the set_pte_at()
-		 * write.
+		 * vmemmap_remap_free() become visible before the
+		 * vmemmap_update_pte() write.
 		 */
 		smp_wmb();
 	}
 
 	entry = mk_pte(walk->reuse_page, pgprot);
 	list_add(&page->lru, walk->vmemmap_pages);
-	set_pte_at(&init_mm, addr, pte, entry);
+	vmemmap_update_pte(addr, pte, entry);
 }
 
 /*
@@ -310,10 +341,10 @@ static void vmemmap_restore_pte(pte_t *pte, unsigned long addr,
 
 	/*
 	 * Makes sure that preceding stores to the page contents become visible
-	 * before the set_pte_at() write.
+	 * before the vmemmap_update_pte() write.
 	 */
 	smp_wmb();
-	set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
+	vmemmap_update_pte(addr, pte, mk_pte(page, pgprot));
 }
 
 /**
@@ -576,7 +607,7 @@ long hugetlb_vmemmap_restore_folios(const struct hstate *h,
 	}
 
 	if (restored)
-		flush_tlb_all();
+		flush_tlb_vmemmap_all();
 	if (!ret)
 		ret = restored;
 	return ret;
@@ -744,7 +775,7 @@ void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct list_head *folio_l
 			break;
 	}
 
-	flush_tlb_all();
+	flush_tlb_vmemmap_all();
 
 	list_for_each_entry(folio, folio_list, lru) {
 		int ret = __hugetlb_vmemmap_optimize_folio(h, folio,
@@ -760,7 +791,7 @@ void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct list_head *folio_l
 		 * allowing more vmemmap remaps to occur.
 		 */
 		if (ret == -ENOMEM && !list_empty(&vmemmap_pages)) {
-			flush_tlb_all();
+			flush_tlb_vmemmap_all();
 			free_vmemmap_page_list(&vmemmap_pages);
 			INIT_LIST_HEAD(&vmemmap_pages);
 			__hugetlb_vmemmap_optimize_folio(h, folio,
@@ -769,7 +800,7 @@ void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct list_head *folio_l
 		}
 	}
 
-	flush_tlb_all();
+	flush_tlb_vmemmap_all();
 	free_vmemmap_page_list(&vmemmap_pages);
 }
 
-- 
2.25.1

[PATCH 2/3] arm64: mm: HVO: support BBM of vmemmap pgtable safely

[Nanyong Sun]

Implement vmemmap_update_pmd and vmemmap_update_pte on arm64 to do
BBM(break-before-make) logic when change the page table of vmemmap
address, they will under the init_mm.page_table_lock.
If a translation fault of vmemmap address concurrently happened after
pte/pmd cleared, vmemmap page fault handler will acquire the
init_mm.page_table_lock to wait for vmemmap update to complete,
by then the virtual address is valid again, so PF can return and
access can continue.
In other case, do the traditional kernel fault.

Implement flush_tlb_vmemmap_all and flush_tlb_vmemmap_range on arm64
with nothing to do because tlb already flushed in every single BBM.

Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
---
 arch/arm64/include/asm/esr.h |  4 ++
 arch/arm64/include/asm/mmu.h | 20 ++++++++
 arch/arm64/mm/fault.c        | 94 ++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mmu.c          | 28 +++++++++++
 4 files changed, 146 insertions(+)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index ae35939f395b..1c63256efd25 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -116,6 +116,10 @@
 #define ESR_ELx_FSC_SERROR	(0x11)
 #define ESR_ELx_FSC_ACCESS	(0x08)
 #define ESR_ELx_FSC_FAULT	(0x04)
+#define ESR_ELx_FSC_FAULT_L0    (0x04)
+#define ESR_ELx_FSC_FAULT_L1    (0x05)
+#define ESR_ELx_FSC_FAULT_L2    (0x06)
+#define ESR_ELx_FSC_FAULT_L3    (0x07)
 #define ESR_ELx_FSC_PERM	(0x0C)
 #define ESR_ELx_FSC_SEA_TTW0	(0x14)
 #define ESR_ELx_FSC_SEA_TTW1	(0x15)
diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
index 2fcf51231d6e..fcec5827f54f 100644
--- a/arch/arm64/include/asm/mmu.h
+++ b/arch/arm64/include/asm/mmu.h
@@ -76,5 +76,25 @@ extern bool kaslr_requires_kpti(void);
 #define INIT_MM_CONTEXT(name)	\
 	.pgd = init_pg_dir,
 
+#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
+void vmemmap_update_pmd(unsigned long start, pmd_t *pmd, pte_t *pgtable);
+#define vmemmap_update_pmd vmemmap_update_pmd
+void vmemmap_update_pte(unsigned long addr, pte_t *pte, pte_t entry);
+#define vmemmap_update_pte vmemmap_update_pte
+
+static inline void flush_tlb_vmemmap_all(void)
+{
+	/* do nothing, already flushed tlb in every single BBM */
+}
+#define flush_tlb_vmemmap_all flush_tlb_vmemmap_all
+
+static inline void flush_tlb_vmemmap_range(unsigned long start,
+					   unsigned long end)
+{
+	/* do nothing, already flushed tlb in every single BBM */
+}
+#define flush_tlb_vmemmap_range flush_tlb_vmemmap_range
+#endif
+
 #endif	/* !__ASSEMBLY__ */
 #endif
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 460d799e1296..7066a273c1e0 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -368,6 +368,97 @@ static bool is_el1_mte_sync_tag_check_fault(unsigned long esr)
 	return false;
 }
 
+#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
+static inline bool is_vmemmap_address(unsigned long addr)
+{
+	return (addr >= VMEMMAP_START) && (addr < VMEMMAP_END);
+}
+
+static inline bool vmemmap_fault_may_fixup(unsigned long addr,
+					   unsigned long esr)
+{
+	if (!is_vmemmap_address(addr))
+		return false;
+
+	/*
+	 * Only try to handle translation fault level 2 or level 3,
+	 * because hugetlb vmemmap optimize only clear pmd or pte.
+	 */
+	switch (esr & ESR_ELx_FSC) {
+	case ESR_ELx_FSC_FAULT_L2:
+	case ESR_ELx_FSC_FAULT_L3:
+		return true;
+	default:
+		return false;
+	}
+}
+
+/*
+ * PMD mapped vmemmap should has been split as PTE mapped
+ * by HVO now, here we only check this case, other cases
+ * should fail.
+ * Also should check the addr is healthy enough that will not cause
+ * a level2 or level3 translation fault again after page fault
+ * handled with success, so we need check both bits[1:0] of PMD and
+ * PTE as ARM Spec mentioned below:
+ * A Translation fault is generated if bits[1:0] of a translation
+ * table descriptor identify the descriptor as either a Fault
+ * encoding or a reserved encoding.
+ */
+static inline bool vmemmap_addr_healthy(unsigned long addr)
+{
+	pgd_t *pgdp;
+	p4d_t *p4dp;
+	pud_t *pudp, pud;
+	pmd_t *pmdp, pmd;
+	pte_t *ptep, pte;
+
+	pgdp = pgd_offset_k(addr);
+	if (pgd_none(READ_ONCE(*pgdp)))
+		return false;
+
+	p4dp = p4d_offset(pgdp, addr);
+	if (p4d_none(READ_ONCE(*p4dp)))
+		return false;
+
+	pudp = pud_offset(p4dp, addr);
+	pud = READ_ONCE(*pudp);
+	if (pud_none(pud))
+		return false;
+
+	pmdp = pmd_offset(pudp, addr);
+	pmd = READ_ONCE(*pmdp);
+	if (!pmd_table(pmd))
+		return false;
+
+	ptep = pte_offset_kernel(pmdp, addr);
+	pte = READ_ONCE(*ptep);
+	return (pte_val(pte) & PTE_TYPE_MASK) == PTE_TYPE_PAGE;
+}
+
+static bool vmemmap_handle_page_fault(unsigned long addr,
+				      unsigned long esr)
+{
+	bool ret = false;
+
+	if (likely(!vmemmap_fault_may_fixup(addr, esr)))
+		return false;
+
+	spin_lock(&init_mm.page_table_lock);
+	if (vmemmap_addr_healthy(addr))
+		ret = true;
+	spin_unlock(&init_mm.page_table_lock);
+
+	return ret;
+}
+#else
+static inline bool vmemmap_handle_page_fault(unsigned long addr,
+					     unsigned long esr)
+{
+	return false;
+}
+#endif /*CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP */
+
 static bool is_translation_fault(unsigned long esr)
 {
 	return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_FAULT;
@@ -409,6 +500,9 @@ static void __do_kernel_fault(unsigned long addr, unsigned long esr,
 		    kfence_handle_page_fault(addr, esr & ESR_ELx_WNR, regs))
 			return;
 
+		if (vmemmap_handle_page_fault(addr, esr))
+			return;
+
 		msg = "paging request";
 	}
 
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 15f6347d23b6..81a600ccac7c 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1146,6 +1146,34 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
 	return 1;
 }
 
+#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
+/*
+ * In the window between the page table entry is cleared and filled
+ * with a new value, other threads have the opportunity to concurrently
+ * access the vmemmap area then page translation fault occur.
+ * Therefore, we need to ensure that the init_mm.page_table_lock is held
+ * to synchronize the vmemmap page fault handling which will wait for
+ * this lock to be released to ensure that the page table entry has been
+ * refreshed with a new valid value.
+ */
+void vmemmap_update_pmd(unsigned long start, pmd_t *pmd, pte_t *pgtable)
+{
+	lockdep_assert_held(&init_mm.page_table_lock);
+	pmd_clear(pmd);
+	flush_tlb_kernel_range(start, start + PMD_SIZE);
+	pmd_populate_kernel(&init_mm, pmd, pgtable);
+}
+
+void vmemmap_update_pte(unsigned long addr, pte_t *pte, pte_t entry)
+{
+	spin_lock(&init_mm.page_table_lock);
+	pte_clear(&init_mm, addr, pte);
+	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+	set_pte_at(&init_mm, addr, pte, entry);
+	spin_unlock(&init_mm.page_table_lock);
+}
+#endif
+
 int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 		struct vmem_altmap *altmap)
 {
-- 
2.25.1

[PATCH 3/3] arm64: mm: Re-enable OPTIMIZE_HUGETLB_VMEMMAP

[Nanyong Sun]

Now update of vmemmap page table can follow the rule of
break-before-make safely for arm64 architecture, re-enable
HVO on arm64.

Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
---
 arch/arm64/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7b071a00425d..43e3d5576fb2 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -103,6 +103,7 @@ config ARM64
 	select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
 	select ARCH_WANT_FRAME_POINTERS
 	select ARCH_WANT_HUGE_PMD_SHARE if ARM64_4K_PAGES || (ARM64_16K_PAGES && !ARM64_VA_BITS_36)
+	select ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP
 	select ARCH_WANT_LD_ORPHAN_WARN
 	select ARCH_WANTS_NO_INSTR
 	select ARCH_WANTS_THP_SWAP if ARM64_4K_PAGES
-- 
2.25.1

Re: [PATCH 1/3] mm: HVO: introduce helper function to update and flush pgtable

[Muchun Song]

On 2023/12/14 15:39, Nanyong Sun wrote:
> Add pmd/pte update and tlb flush helper function to update page
> table. This refactoring patch is designed to facilitate each
> architecture to implement its own special logic in preparation
> for the arm64 architecture to follow the necessary break-before-make
> sequence when updating page tables.
>
> Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
> ---
>   mm/hugetlb_vmemmap.c | 55 ++++++++++++++++++++++++++++++++++----------
>   1 file changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> index 87818ee7f01d..49e8b351def3 100644
> --- a/mm/hugetlb_vmemmap.c
> +++ b/mm/hugetlb_vmemmap.c
> @@ -45,6 +45,37 @@ struct vmemmap_remap_walk {
>   	unsigned long		flags;
>   };
>   
> +#ifndef vmemmap_update_pmd
> +static inline void vmemmap_update_pmd(unsigned long start,
> +				      pmd_t *pmd, pte_t *pgtable)
> +{
> +	pmd_populate_kernel(&init_mm, pmd, pgtable);
> +}
> +#endif
> +
> +#ifndef vmemmap_update_pte
> +static inline void vmemmap_update_pte(unsigned long addr,
> +				      pte_t *pte, pte_t entry)
> +{
> +	set_pte_at(&init_mm, addr, pte, entry);
> +}
> +#endif
> +
> +#ifndef flush_tlb_vmemmap_all
> +static inline void flush_tlb_vmemmap_all(void)
> +{
> +	flush_tlb_all();
> +}
> +#endif
> +
> +#ifndef flush_tlb_vmemmap_range
> +static inline void flush_tlb_vmemmap_range(unsigned long start,
> +					   unsigned long end)
> +{
> +	flush_tlb_kernel_range(start, end);
> +}
> +#endif

I'd like to rename both tlb-flush helpers to vmemmap_flush_tlb_all/range
since other helpers all are prefixed with "vmemmap". It'll be more
consistent for me.

Otherwise LGTM. Thanks.

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

> +
>   static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, bool flush)
>   {
>   	pmd_t __pmd;
> @@ -87,9 +118,9 @@ static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, bool flush)
>   
>   		/* Make pte visible before pmd. See comment in pmd_install(). */
>   		smp_wmb();
> -		pmd_populate_kernel(&init_mm, pmd, pgtable);
> +		vmemmap_update_pmd(start, pmd, pgtable);
>   		if (flush)
> -			flush_tlb_kernel_range(start, start + PMD_SIZE);
> +			flush_tlb_vmemmap_range(start, start + PMD_SIZE);
>   	} else {
>   		pte_free_kernel(&init_mm, pgtable);
>   	}
> @@ -217,7 +248,7 @@ static int vmemmap_remap_range(unsigned long start, unsigned long end,
>   	} while (pgd++, addr = next, addr != end);
>   
>   	if (walk->remap_pte && !(walk->flags & VMEMMAP_REMAP_NO_TLB_FLUSH))
> -		flush_tlb_kernel_range(start, end);
> +		flush_tlb_vmemmap_range(start, end);
>   
>   	return 0;
>   }
> @@ -263,15 +294,15 @@ static void vmemmap_remap_pte(pte_t *pte, unsigned long addr,
>   
>   		/*
>   		 * Makes sure that preceding stores to the page contents from
> -		 * vmemmap_remap_free() become visible before the set_pte_at()
> -		 * write.
> +		 * vmemmap_remap_free() become visible before the
> +		 * vmemmap_update_pte() write.
>   		 */
>   		smp_wmb();
>   	}
>   
>   	entry = mk_pte(walk->reuse_page, pgprot);
>   	list_add(&page->lru, walk->vmemmap_pages);
> -	set_pte_at(&init_mm, addr, pte, entry);
> +	vmemmap_update_pte(addr, pte, entry);
>   }
>   
>   /*
> @@ -310,10 +341,10 @@ static void vmemmap_restore_pte(pte_t *pte, unsigned long addr,
>   
>   	/*
>   	 * Makes sure that preceding stores to the page contents become visible
> -	 * before the set_pte_at() write.
> +	 * before the vmemmap_update_pte() write.
>   	 */
>   	smp_wmb();
> -	set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
> +	vmemmap_update_pte(addr, pte, mk_pte(page, pgprot));
>   }
>   
>   /**
> @@ -576,7 +607,7 @@ long hugetlb_vmemmap_restore_folios(const struct hstate *h,
>   	}
>   
>   	if (restored)
> -		flush_tlb_all();
> +		flush_tlb_vmemmap_all();
>   	if (!ret)
>   		ret = restored;
>   	return ret;
> @@ -744,7 +775,7 @@ void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct list_head *folio_l
>   			break;
>   	}
>   
> -	flush_tlb_all();
> +	flush_tlb_vmemmap_all();
>   
>   	list_for_each_entry(folio, folio_list, lru) {
>   		int ret = __hugetlb_vmemmap_optimize_folio(h, folio,
> @@ -760,7 +791,7 @@ void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct list_head *folio_l
>   		 * allowing more vmemmap remaps to occur.
>   		 */
>   		if (ret == -ENOMEM && !list_empty(&vmemmap_pages)) {
> -			flush_tlb_all();
> +			flush_tlb_vmemmap_all();
>   			free_vmemmap_page_list(&vmemmap_pages);
>   			INIT_LIST_HEAD(&vmemmap_pages);
>   			__hugetlb_vmemmap_optimize_folio(h, folio,
> @@ -769,7 +800,7 @@ void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct list_head *folio_l
>   		}
>   	}
>   
> -	flush_tlb_all();
> +	flush_tlb_vmemmap_all();
>   	free_vmemmap_page_list(&vmemmap_pages);
>   }
>   

Re: [PATCH 2/3] arm64: mm: HVO: support BBM of vmemmap pgtable safely

[Muchun Song]

On 2023/12/14 15:39, Nanyong Sun wrote:
> Implement vmemmap_update_pmd and vmemmap_update_pte on arm64 to do
> BBM(break-before-make) logic when change the page table of vmemmap
> address, they will under the init_mm.page_table_lock.
> If a translation fault of vmemmap address concurrently happened after
> pte/pmd cleared, vmemmap page fault handler will acquire the
> init_mm.page_table_lock to wait for vmemmap update to complete,
> by then the virtual address is valid again, so PF can return and
> access can continue.
> In other case, do the traditional kernel fault.

Yes. BTW, we already use the same scheme to support arm64
in our internal production. So the whole approach LGTM.

> Implement flush_tlb_vmemmap_all and flush_tlb_vmemmap_range on arm64
> with nothing to do because tlb already flushed in every single BBM.
>
> Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
> ---
>   arch/arm64/include/asm/esr.h |  4 ++
>   arch/arm64/include/asm/mmu.h | 20 ++++++++
>   arch/arm64/mm/fault.c        | 94 ++++++++++++++++++++++++++++++++++++
>   arch/arm64/mm/mmu.c          | 28 +++++++++++
>   4 files changed, 146 insertions(+)
>
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index ae35939f395b..1c63256efd25 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -116,6 +116,10 @@
>   #define ESR_ELx_FSC_SERROR	(0x11)
>   #define ESR_ELx_FSC_ACCESS	(0x08)
>   #define ESR_ELx_FSC_FAULT	(0x04)
> +#define ESR_ELx_FSC_FAULT_L0    (0x04)
> +#define ESR_ELx_FSC_FAULT_L1    (0x05)
> +#define ESR_ELx_FSC_FAULT_L2    (0x06)
> +#define ESR_ELx_FSC_FAULT_L3    (0x07)
>   #define ESR_ELx_FSC_PERM	(0x0C)
>   #define ESR_ELx_FSC_SEA_TTW0	(0x14)
>   #define ESR_ELx_FSC_SEA_TTW1	(0x15)
> diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
> index 2fcf51231d6e..fcec5827f54f 100644
> --- a/arch/arm64/include/asm/mmu.h
> +++ b/arch/arm64/include/asm/mmu.h
> @@ -76,5 +76,25 @@ extern bool kaslr_requires_kpti(void);
>   #define INIT_MM_CONTEXT(name)	\
>   	.pgd = init_pg_dir,
>   
> +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
> +void vmemmap_update_pmd(unsigned long start, pmd_t *pmd, pte_t *pgtable);
> +#define vmemmap_update_pmd vmemmap_update_pmd
> +void vmemmap_update_pte(unsigned long addr, pte_t *pte, pte_t entry);
> +#define vmemmap_update_pte vmemmap_update_pte
> +
> +static inline void flush_tlb_vmemmap_all(void)
> +{
> +	/* do nothing, already flushed tlb in every single BBM */
> +}
> +#define flush_tlb_vmemmap_all flush_tlb_vmemmap_all
> +
> +static inline void flush_tlb_vmemmap_range(unsigned long start,
> +					   unsigned long end)
> +{
> +	/* do nothing, already flushed tlb in every single BBM */
> +}
> +#define flush_tlb_vmemmap_range flush_tlb_vmemmap_range
> +#endif
> +
>   #endif	/* !__ASSEMBLY__ */
>   #endif
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 460d799e1296..7066a273c1e0 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -368,6 +368,97 @@ static bool is_el1_mte_sync_tag_check_fault(unsigned long esr)
>   	return false;
>   }
>   
> +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
> +static inline bool is_vmemmap_address(unsigned long addr)
> +{
> +	return (addr >= VMEMMAP_START) && (addr < VMEMMAP_END);
> +}
> +
> +static inline bool vmemmap_fault_may_fixup(unsigned long addr,
> +					   unsigned long esr)
> +{
> +	if (!is_vmemmap_address(addr))
> +		return false;
> +
> +	/*
> +	 * Only try to handle translation fault level 2 or level 3,
> +	 * because hugetlb vmemmap optimize only clear pmd or pte.
> +	 */
> +	switch (esr & ESR_ELx_FSC) {
> +	case ESR_ELx_FSC_FAULT_L2:
> +	case ESR_ELx_FSC_FAULT_L3:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +/*
> + * PMD mapped vmemmap should has been split as PTE mapped
> + * by HVO now, here we only check this case, other cases
> + * should fail.
> + * Also should check the addr is healthy enough that will not cause
> + * a level2 or level3 translation fault again after page fault
> + * handled with success, so we need check both bits[1:0] of PMD and
> + * PTE as ARM Spec mentioned below:
> + * A Translation fault is generated if bits[1:0] of a translation
> + * table descriptor identify the descriptor as either a Fault
> + * encoding or a reserved encoding.
> + */
> +static inline bool vmemmap_addr_healthy(unsigned long addr)
> +{
> +	pgd_t *pgdp;
> +	p4d_t *p4dp;
> +	pud_t *pudp, pud;
> +	pmd_t *pmdp, pmd;
> +	pte_t *ptep, pte;
> +
> +	pgdp = pgd_offset_k(addr);
> +	if (pgd_none(READ_ONCE(*pgdp)))
> +		return false;
> +
> +	p4dp = p4d_offset(pgdp, addr);
> +	if (p4d_none(READ_ONCE(*p4dp)))
> +		return false;
> +
> +	pudp = pud_offset(p4dp, addr);
> +	pud = READ_ONCE(*pudp);
> +	if (pud_none(pud))
> +		return false;
> +
> +	pmdp = pmd_offset(pudp, addr);

We already make sure it is a translation fault of level 2 or 3
here, so we could use pmd_offset_k() macro to simplify the code
a little. Right?

> +	pmd = READ_ONCE(*pmdp);
> +	if (!pmd_table(pmd))
> +		return false;
> +
> +	ptep = pte_offset_kernel(pmdp, addr);
> +	pte = READ_ONCE(*ptep);

Please use ptep_get (which is supposed to do this) to access the
raw pte, see commit c33c794828f21217.

> +	return (pte_val(pte) & PTE_TYPE_MASK) == PTE_TYPE_PAGE;
> +}
> +
> +static bool vmemmap_handle_page_fault(unsigned long addr,
> +				      unsigned long esr)
> +{
> +	bool ret = false;
> +
> +	if (likely(!vmemmap_fault_may_fixup(addr, esr)))
> +		return false;
> +
> +	spin_lock(&init_mm.page_table_lock);
> +	if (vmemmap_addr_healthy(addr))
> +		ret = true;

It is to assign the return value to ret directly. Like:

         ret = vmemmap_addr_healthy(addr);

The the initializetion to ret also can be dropped.

> +	spin_unlock(&init_mm.page_table_lock);
> +
> +	return ret;
> +}
> +#else
> +static inline bool vmemmap_handle_page_fault(unsigned long addr,
> +					     unsigned long esr)
> +{
> +	return false;
> +}
> +#endif /*CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP */
           ^
Miss a blank between "*" and "C" here.

Thanks.

> +
>   static bool is_translation_fault(unsigned long esr)
>   {
>   	return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_FAULT;
> @@ -409,6 +500,9 @@ static void __do_kernel_fault(unsigned long addr, unsigned long esr,
>   		    kfence_handle_page_fault(addr, esr & ESR_ELx_WNR, regs))
>   			return;
>   
> +		if (vmemmap_handle_page_fault(addr, esr))
> +			return;
> +
>   		msg = "paging request";
>   	}
>   
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 15f6347d23b6..81a600ccac7c 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1146,6 +1146,34 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
>   	return 1;
>   }
>   
> +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
> +/*
> + * In the window between the page table entry is cleared and filled
> + * with a new value, other threads have the opportunity to concurrently
> + * access the vmemmap area then page translation fault occur.
> + * Therefore, we need to ensure that the init_mm.page_table_lock is held
> + * to synchronize the vmemmap page fault handling which will wait for
> + * this lock to be released to ensure that the page table entry has been
> + * refreshed with a new valid value.
> + */
> +void vmemmap_update_pmd(unsigned long start, pmd_t *pmd, pte_t *pgtable)
> +{
> +	lockdep_assert_held(&init_mm.page_table_lock);
> +	pmd_clear(pmd);
> +	flush_tlb_kernel_range(start, start + PMD_SIZE);
> +	pmd_populate_kernel(&init_mm, pmd, pgtable);
> +}
> +
> +void vmemmap_update_pte(unsigned long addr, pte_t *pte, pte_t entry)
> +{
> +	spin_lock(&init_mm.page_table_lock);
> +	pte_clear(&init_mm, addr, pte);
> +	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> +	set_pte_at(&init_mm, addr, pte, entry);
> +	spin_unlock(&init_mm.page_table_lock);
> +}
> +#endif
> +
>   int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>   		struct vmem_altmap *altmap)
>   {

Re: [PATCH 3/3] arm64: mm: Re-enable OPTIMIZE_HUGETLB_VMEMMAP

[Muchun Song]

> On Dec 14, 2023, at 15:39, Nanyong Sun <sunnanyong@huawei.com> wrote:
> 
> Now update of vmemmap page table can follow the rule of
> break-before-make safely for arm64 architecture, re-enable
> HVO on arm64.
> 
> Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH 1/3] mm: HVO: introduce helper function to update and flush pgtable

[Kefeng Wang]

On 2023/12/14 15:39, Nanyong Sun wrote:
> Add pmd/pte update and tlb flush helper function to update page
> table. This refactoring patch is designed to facilitate each
> architecture to implement its own special logic in preparation
> for the arm64 architecture to follow the necessary break-before-make
> sequence when updating page tables.
> 
> Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
> ---
>   mm/hugetlb_vmemmap.c | 55 ++++++++++++++++++++++++++++++++++----------
>   1 file changed, 43 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> index 87818ee7f01d..49e8b351def3 100644
> --- a/mm/hugetlb_vmemmap.c
> +++ b/mm/hugetlb_vmemmap.c
> @@ -45,6 +45,37 @@ struct vmemmap_remap_walk {
>   	unsigned long		flags;
>   };
>   
> +#ifndef vmemmap_update_pmd
> +static inline void vmemmap_update_pmd(unsigned long start,
> +				      pmd_t *pmd, pte_t *pgtable)

pgtable ->  ptep

> +{
> +	pmd_populate_kernel(&init_mm, pmd, pgtable);
> +}
> +#endif
> +
> +#ifndef vmemmap_update_pte
> +static inline void vmemmap_update_pte(unsigned long addr,
> +				      pte_t *pte, pte_t entry)

pte   -> ptep
entry -> pte

> +{
> +	set_pte_at(&init_mm, addr, pte, entry);
> +}
> +#endif

Re: [PATCH 2/3] arm64: mm: HVO: support BBM of vmemmap pgtable safely

[Kefeng Wang]

On 2023/12/15 12:36, Muchun Song wrote:
> 
> 
> On 2023/12/14 15:39, Nanyong Sun wrote:
>> Implement vmemmap_update_pmd and vmemmap_update_pte on arm64 to do
>> BBM(break-before-make) logic when change the page table of vmemmap
>> address, they will under the init_mm.page_table_lock.
>> If a translation fault of vmemmap address concurrently happened after
>> pte/pmd cleared, vmemmap page fault handler will acquire the
>> init_mm.page_table_lock to wait for vmemmap update to complete,
>> by then the virtual address is valid again, so PF can return and
>> access can continue.
>> In other case, do the traditional kernel fault.
> 
> Yes. BTW, we already use the same scheme to support arm64
> in our internal production. So the whole approach LGTM.
> 
>> Implement flush_tlb_vmemmap_all and flush_tlb_vmemmap_range on arm64
>> with nothing to do because tlb already flushed in every single BBM.
>>
>> Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
>> ---
>>   arch/arm64/include/asm/esr.h |  4 ++
>>   arch/arm64/include/asm/mmu.h | 20 ++++++++
>>   arch/arm64/mm/fault.c        | 94 ++++++++++++++++++++++++++++++++++++
>>   arch/arm64/mm/mmu.c          | 28 +++++++++++
>>   4 files changed, 146 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
>> index ae35939f395b..1c63256efd25 100644
>> --- a/arch/arm64/include/asm/esr.h
>> +++ b/arch/arm64/include/asm/esr.h
>> @@ -116,6 +116,10 @@
>>   #define ESR_ELx_FSC_SERROR    (0x11)
>>   #define ESR_ELx_FSC_ACCESS    (0x08)
>>   #define ESR_ELx_FSC_FAULT    (0x04)
>> +#define ESR_ELx_FSC_FAULT_L0    (0x04)
>> +#define ESR_ELx_FSC_FAULT_L1    (0x05)
>> +#define ESR_ELx_FSC_FAULT_L2    (0x06)
>> +#define ESR_ELx_FSC_FAULT_L3    (0x07)
>>   #define ESR_ELx_FSC_PERM    (0x0C)
>>   #define ESR_ELx_FSC_SEA_TTW0    (0x14)
>>   #define ESR_ELx_FSC_SEA_TTW1    (0x15)
>> diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
>> index 2fcf51231d6e..fcec5827f54f 100644
>> --- a/arch/arm64/include/asm/mmu.h
>> +++ b/arch/arm64/include/asm/mmu.h
>> @@ -76,5 +76,25 @@ extern bool kaslr_requires_kpti(void);
>>   #define INIT_MM_CONTEXT(name)    \
>>       .pgd = init_pg_dir,
>> +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
>> +void vmemmap_update_pmd(unsigned long start, pmd_t *pmd, pte_t 
>> *pgtable);
>> +#define vmemmap_update_pmd vmemmap_update_pmd
>> +void vmemmap_update_pte(unsigned long addr, pte_t *pte, pte_t entry);
>> +#define vmemmap_update_pte vmemmap_update_pte
>> +
>> +static inline void flush_tlb_vmemmap_all(void)
>> +{
>> +    /* do nothing, already flushed tlb in every single BBM */
>> +}
>> +#define flush_tlb_vmemmap_all flush_tlb_vmemmap_all
>> +
>> +static inline void flush_tlb_vmemmap_range(unsigned long start,
>> +                       unsigned long end)
>> +{
>> +    /* do nothing, already flushed tlb in every single BBM */
>> +}
>> +#define flush_tlb_vmemmap_range flush_tlb_vmemmap_range
>> +#endif
>> +
>>   #endif    /* !__ASSEMBLY__ */
>>   #endif
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 460d799e1296..7066a273c1e0 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -368,6 +368,97 @@ static bool 
>> is_el1_mte_sync_tag_check_fault(unsigned long esr)
>>       return false;
>>   }
>> +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
>> +static inline bool is_vmemmap_address(unsigned long addr)
>> +{
>> +    return (addr >= VMEMMAP_START) && (addr < VMEMMAP_END);
>> +}
>> +
>> +static inline bool vmemmap_fault_may_fixup(unsigned long addr,
>> +                       unsigned long esr)
>> +{
>> +    if (!is_vmemmap_address(addr))
>> +        return false;
>> +

Squash the vmemmap range check into this function.

>> +    /*
>> +     * Only try to handle translation fault level 2 or level 3,
>> +     * because hugetlb vmemmap optimize only clear pmd or pte.
>> +     */
>> +    switch (esr & ESR_ELx_FSC) {
>> +    case ESR_ELx_FSC_FAULT_L2:
>> +    case ESR_ELx_FSC_FAULT_L3:
>> +        return true;
>> +    default:
>> +        return false;
>> +    }
>> +}
>> +
>> +/*
>> + * PMD mapped vmemmap should has been split as PTE mapped
>> + * by HVO now, here we only check this case, other cases
>> + * should fail.
>> + * Also should check the addr is healthy enough that will not cause
>> + * a level2 or level3 translation fault again after page fault
>> + * handled with success, so we need check both bits[1:0] of PMD and
>> + * PTE as ARM Spec mentioned below:
>> + * A Translation fault is generated if bits[1:0] of a translation
>> + * table descriptor identify the descriptor as either a Fault
>> + * encoding or a reserved encoding.
>> + */
>> +static inline bool vmemmap_addr_healthy(unsigned long addr)
>> +{
>> +    pgd_t *pgdp;
>> +    p4d_t *p4dp;
>> +    pud_t *pudp, pud;
>> +    pmd_t *pmdp, pmd;
>> +    pte_t *ptep, pte;
>> +
>> +    pgdp = pgd_offset_k(addr);
>> +    if (pgd_none(READ_ONCE(*pgdp)))
>> +        return false;
>> +
>> +    p4dp = p4d_offset(pgdp, addr);
>> +    if (p4d_none(READ_ONCE(*p4dp)))
>> +        return false;
>> +
>> +    pudp = pud_offset(p4dp, addr);
>> +    pud = READ_ONCE(*pudp);
>> +    if (pud_none(pud))
>> +        return false;
>> +
>> +    pmdp = pmd_offset(pudp, addr);
> 
> We already make sure it is a translation fault of level 2 or 3
> here, so we could use pmd_offset_k() macro to simplify the code
> a little. Right?
> 
>> +    pmd = READ_ONCE(*pmdp);
>> +    if (!pmd_table(pmd))
>> +        return false;
>> +
>> +    ptep = pte_offset_kernel(pmdp, addr);
>> +    pte = READ_ONCE(*ptep);
> 
> Please use ptep_get (which is supposed to do this) to access the
> raw pte, see commit c33c794828f21217.
> 
>> +    return (pte_val(pte) & PTE_TYPE_MASK) == PTE_TYPE_PAGE;
>> +}
>> +
>> +static bool vmemmap_handle_page_fault(unsigned long addr,
>> +                      unsigned long esr)
>> +{
>> +    bool ret = false;
>> +
>> +    if (likely(!vmemmap_fault_may_fixup(addr, esr)))
>> +        return false;
>> +
>> +    spin_lock(&init_mm.page_table_lock);
>> +    if (vmemmap_addr_healthy(addr))
>> +        ret = true;
> 
> It is to assign the return value to ret directly. Like:
> 
>          ret = vmemmap_addr_healthy(addr);
> 
> The the initializetion to ret also can be dropped.
> 
>> +    spin_unlock(&init_mm.page_table_lock);
>> +
>> +    return ret;
>> +}
>> +#else
>> +static inline bool vmemmap_handle_page_fault(unsigned long addr,
>> +                         unsigned long esr)
>> +{
>> +    return false;
>> +}
>> +#endif /*CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP */
>            ^
> Miss a blank between "*" and "C" here.
> 
> Thanks.
> 
>> +
>>   static bool is_translation_fault(unsigned long esr)
>>   {
>>       return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_FAULT;
>> @@ -409,6 +500,9 @@ static void __do_kernel_fault(unsigned long addr, 
>> unsigned long esr,
>>               kfence_handle_page_fault(addr, esr & ESR_ELx_WNR, regs))
>>               return;
>> +        if (vmemmap_handle_page_fault(addr, esr))
>> +            return;
>> +

Better to move under is_translation_fault()?

if (is_translation_fault(esr) {
	if (kfence_handle_page_fault())
		return;
	if (vmemmap_handle_page_fault())
		return;
}



>>           msg = "paging request";
>>       }
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 15f6347d23b6..81a600ccac7c 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1146,6 +1146,34 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, 
>> int node,
>>       return 1;
>>   }
>> +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
>> +/*
>> + * In the window between the page table entry is cleared and filled
>> + * with a new value, other threads have the opportunity to concurrently
>> + * access the vmemmap area then page translation fault occur.
>> + * Therefore, we need to ensure that the init_mm.page_table_lock is held
>> + * to synchronize the vmemmap page fault handling which will wait for
>> + * this lock to be released to ensure that the page table entry has been
>> + * refreshed with a new valid value.
>> + */
>> +void vmemmap_update_pmd(unsigned long start, pmd_t *pmd, pte_t *pgtable)
>> +{
>> +    lockdep_assert_held(&init_mm.page_table_lock);
>> +    pmd_clear(pmd);
>> +    flush_tlb_kernel_range(start, start + PMD_SIZE);
>> +    pmd_populate_kernel(&init_mm, pmd, pgtable);
>> +}
>> +
>> +void vmemmap_update_pte(unsigned long addr, pte_t *pte, pte_t entry)
>> +{
>> +    spin_lock(&init_mm.page_table_lock);
>> +    pte_clear(&init_mm, addr, pte);
>> +    flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>> +    set_pte_at(&init_mm, addr, pte, entry);
>> +    spin_unlock(&init_mm.page_table_lock);
>> +}
>> +#endif
>> +
>>   int __meminit vmemmap_populate(unsigned long start, unsigned long 
>> end, int node,
>>           struct vmem_altmap *altmap)
>>   {
> 

Re: [PATCH 1/3] mm: HVO: introduce helper function to update and flush pgtable

[Nanyong Sun]

On 2023/12/15 11:36, Muchun Song wrote:
>
>
> On 2023/12/14 15:39, Nanyong Sun wrote:
>> Add pmd/pte update and tlb flush helper function to update page
>> table. This refactoring patch is designed to facilitate each
>> architecture to implement its own special logic in preparation
>> for the arm64 architecture to follow the necessary break-before-make
>> sequence when updating page tables.
>>
>> Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
>> ---
>>   mm/hugetlb_vmemmap.c | 55 ++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 43 insertions(+), 12 deletions(-)
>>
>> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
>> index 87818ee7f01d..49e8b351def3 100644
>> --- a/mm/hugetlb_vmemmap.c
>> +++ b/mm/hugetlb_vmemmap.c
>> @@ -45,6 +45,37 @@ struct vmemmap_remap_walk {
>>       unsigned long        flags;
>>   };
>>   +#ifndef vmemmap_update_pmd
>> +static inline void vmemmap_update_pmd(unsigned long start,
>> +                      pmd_t *pmd, pte_t *pgtable)
>> +{
>> +    pmd_populate_kernel(&init_mm, pmd, pgtable);
>> +}
>> +#endif
>> +
>> +#ifndef vmemmap_update_pte
>> +static inline void vmemmap_update_pte(unsigned long addr,
>> +                      pte_t *pte, pte_t entry)
>> +{
>> +    set_pte_at(&init_mm, addr, pte, entry);
>> +}
>> +#endif
>> +
>> +#ifndef flush_tlb_vmemmap_all
>> +static inline void flush_tlb_vmemmap_all(void)
>> +{
>> +    flush_tlb_all();
>> +}
>> +#endif
>> +
>> +#ifndef flush_tlb_vmemmap_range
>> +static inline void flush_tlb_vmemmap_range(unsigned long start,
>> +                       unsigned long end)
>> +{
>> +    flush_tlb_kernel_range(start, end);
>> +}
>> +#endif
>
> I'd like to rename both tlb-flush helpers to vmemmap_flush_tlb_all/range
> since other helpers all are prefixed with "vmemmap". It'll be more
> consistent for me.
>
> Otherwise LGTM. Thanks.
>
> Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Hi Muchun，

Thank you for your review on this patch set, I'll fix them and send out 
the v2 version later.

>
>> +
>>   static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, 
>> bool flush)
>>   {
>>       pmd_t __pmd;
>> @@ -87,9 +118,9 @@ static int split_vmemmap_huge_pmd(pmd_t *pmd, 
>> unsigned long start, bool flush)
>>             /* Make pte visible before pmd. See comment in 
>> pmd_install(). */
>>           smp_wmb();
>> -        pmd_populate_kernel(&init_mm, pmd, pgtable);
>> +        vmemmap_update_pmd(start, pmd, pgtable);
>>           if (flush)
>> -            flush_tlb_kernel_range(start, start + PMD_SIZE);
>> +            flush_tlb_vmemmap_range(start, start + PMD_SIZE);
>>       } else {
>>           pte_free_kernel(&init_mm, pgtable);
>>       }
>> @@ -217,7 +248,7 @@ static int vmemmap_remap_range(unsigned long 
>> start, unsigned long end,
>>       } while (pgd++, addr = next, addr != end);
>>         if (walk->remap_pte && !(walk->flags & 
>> VMEMMAP_REMAP_NO_TLB_FLUSH))
>> -        flush_tlb_kernel_range(start, end);
>> +        flush_tlb_vmemmap_range(start, end);
>>         return 0;
>>   }
>> @@ -263,15 +294,15 @@ static void vmemmap_remap_pte(pte_t *pte, 
>> unsigned long addr,
>>             /*
>>            * Makes sure that preceding stores to the page contents from
>> -         * vmemmap_remap_free() become visible before the set_pte_at()
>> -         * write.
>> +         * vmemmap_remap_free() become visible before the
>> +         * vmemmap_update_pte() write.
>>            */
>>           smp_wmb();
>>       }
>>         entry = mk_pte(walk->reuse_page, pgprot);
>>       list_add(&page->lru, walk->vmemmap_pages);
>> -    set_pte_at(&init_mm, addr, pte, entry);
>> +    vmemmap_update_pte(addr, pte, entry);
>>   }
>>     /*
>> @@ -310,10 +341,10 @@ static void vmemmap_restore_pte(pte_t *pte, 
>> unsigned long addr,
>>         /*
>>        * Makes sure that preceding stores to the page contents become 
>> visible
>> -     * before the set_pte_at() write.
>> +     * before the vmemmap_update_pte() write.
>>        */
>>       smp_wmb();
>> -    set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
>> +    vmemmap_update_pte(addr, pte, mk_pte(page, pgprot));
>>   }
>>     /**
>> @@ -576,7 +607,7 @@ long hugetlb_vmemmap_restore_folios(const struct 
>> hstate *h,
>>       }
>>         if (restored)
>> -        flush_tlb_all();
>> +        flush_tlb_vmemmap_all();
>>       if (!ret)
>>           ret = restored;
>>       return ret;
>> @@ -744,7 +775,7 @@ void hugetlb_vmemmap_optimize_folios(struct 
>> hstate *h, struct list_head *folio_l
>>               break;
>>       }
>>   -    flush_tlb_all();
>> +    flush_tlb_vmemmap_all();
>>         list_for_each_entry(folio, folio_list, lru) {
>>           int ret = __hugetlb_vmemmap_optimize_folio(h, folio,
>> @@ -760,7 +791,7 @@ void hugetlb_vmemmap_optimize_folios(struct 
>> hstate *h, struct list_head *folio_l
>>            * allowing more vmemmap remaps to occur.
>>            */
>>           if (ret == -ENOMEM && !list_empty(&vmemmap_pages)) {
>> -            flush_tlb_all();
>> +            flush_tlb_vmemmap_all();
>>               free_vmemmap_page_list(&vmemmap_pages);
>>               INIT_LIST_HEAD(&vmemmap_pages);
>>               __hugetlb_vmemmap_optimize_folio(h, folio,
>> @@ -769,7 +800,7 @@ void hugetlb_vmemmap_optimize_folios(struct 
>> hstate *h, struct list_head *folio_l
>>           }
>>       }
>>   -    flush_tlb_all();
>> +    flush_tlb_vmemmap_all();
>>       free_vmemmap_page_list(&vmemmap_pages);
>>   }
>
> .

Re: [PATCH 1/3] mm: HVO: introduce helper function to update and flush pgtable

[Nanyong Sun]

On 2023/12/15 14:16, Kefeng Wang wrote:
>
>
> On 2023/12/14 15:39, Nanyong Sun wrote:
>> Add pmd/pte update and tlb flush helper function to update page
>> table. This refactoring patch is designed to facilitate each
>> architecture to implement its own special logic in preparation
>> for the arm64 architecture to follow the necessary break-before-make
>> sequence when updating page tables.
>>
>> Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
>> ---
>>   mm/hugetlb_vmemmap.c | 55 ++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 43 insertions(+), 12 deletions(-)
>>
>> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
>> index 87818ee7f01d..49e8b351def3 100644
>> --- a/mm/hugetlb_vmemmap.c
>> +++ b/mm/hugetlb_vmemmap.c
>> @@ -45,6 +45,37 @@ struct vmemmap_remap_walk {
>>       unsigned long        flags;
>>   };
>>   +#ifndef vmemmap_update_pmd
>> +static inline void vmemmap_update_pmd(unsigned long start,
>> +                      pmd_t *pmd, pte_t *pgtable)
>
> pgtable ->  ptep
Hi Kefeng，

Thank you for your review on this patch set, I'll fix them and send out 
the v2 version later.

>
>> +{
>> +    pmd_populate_kernel(&init_mm, pmd, pgtable);
>> +}
>> +#endif
>> +
>> +#ifndef vmemmap_update_pte
>> +static inline void vmemmap_update_pte(unsigned long addr,
>> +                      pte_t *pte, pte_t entry)
>
> pte   -> ptep
> entry -> pte
>
>> +{
>> +    set_pte_at(&init_mm, addr, pte, entry);
>> +}
>> +#endif
>
> .

Re: [PATCH 2/3] arm64: mm: HVO: support BBM of vmemmap pgtable safely

[Jane Chu]

On 12/14/2023 8:36 PM, Muchun Song wrote:

>
> On 2023/12/14 15:39, Nanyong Sun wrote:
>> Implement vmemmap_update_pmd and vmemmap_update_pte on arm64 to do
>> BBM(break-before-make) logic when change the page table of vmemmap
>> address, they will under the init_mm.page_table_lock.
>> If a translation fault of vmemmap address concurrently happened after
>> pte/pmd cleared, vmemmap page fault handler will acquire the
>> init_mm.page_table_lock to wait for vmemmap update to complete,
>> by then the virtual address is valid again, so PF can return and
>> access can continue.
>> In other case, do the traditional kernel fault.
>
> Yes. BTW, we already use the same scheme to support arm64
> in our internal production. So the whole approach LGTM.

Glad to hear that.  I have a question to both of you then.

How long does it take to allocate 3/4 of 1TB memory to hugetlb pool?  
and to free them?

thanks!

-jane

>
>> Implement flush_tlb_vmemmap_all and flush_tlb_vmemmap_range on arm64
>> with nothing to do because tlb already flushed in every single BBM.
>>
>> Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
>> ---
>>   arch/arm64/include/asm/esr.h |  4 ++
>>   arch/arm64/include/asm/mmu.h | 20 ++++++++
>>   arch/arm64/mm/fault.c        | 94 ++++++++++++++++++++++++++++++++++++
>>   arch/arm64/mm/mmu.c          | 28 +++++++++++
>>   4 files changed, 146 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
>> index ae35939f395b..1c63256efd25 100644
>> --- a/arch/arm64/include/asm/esr.h
>> +++ b/arch/arm64/include/asm/esr.h
>> @@ -116,6 +116,10 @@
>>   #define ESR_ELx_FSC_SERROR    (0x11)
>>   #define ESR_ELx_FSC_ACCESS    (0x08)
>>   #define ESR_ELx_FSC_FAULT    (0x04)
>> +#define ESR_ELx_FSC_FAULT_L0    (0x04)
>> +#define ESR_ELx_FSC_FAULT_L1    (0x05)
>> +#define ESR_ELx_FSC_FAULT_L2    (0x06)
>> +#define ESR_ELx_FSC_FAULT_L3    (0x07)
>>   #define ESR_ELx_FSC_PERM    (0x0C)
>>   #define ESR_ELx_FSC_SEA_TTW0    (0x14)
>>   #define ESR_ELx_FSC_SEA_TTW1    (0x15)
>> diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
>> index 2fcf51231d6e..fcec5827f54f 100644
>> --- a/arch/arm64/include/asm/mmu.h
>> +++ b/arch/arm64/include/asm/mmu.h
>> @@ -76,5 +76,25 @@ extern bool kaslr_requires_kpti(void);
>>   #define INIT_MM_CONTEXT(name)    \
>>       .pgd = init_pg_dir,
>>   +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
>> +void vmemmap_update_pmd(unsigned long start, pmd_t *pmd, pte_t 
>> *pgtable);
>> +#define vmemmap_update_pmd vmemmap_update_pmd
>> +void vmemmap_update_pte(unsigned long addr, pte_t *pte, pte_t entry);
>> +#define vmemmap_update_pte vmemmap_update_pte
>> +
>> +static inline void flush_tlb_vmemmap_all(void)
>> +{
>> +    /* do nothing, already flushed tlb in every single BBM */
>> +}
>> +#define flush_tlb_vmemmap_all flush_tlb_vmemmap_all
>> +
>> +static inline void flush_tlb_vmemmap_range(unsigned long start,
>> +                       unsigned long end)
>> +{
>> +    /* do nothing, already flushed tlb in every single BBM */
>> +}
>> +#define flush_tlb_vmemmap_range flush_tlb_vmemmap_range
>> +#endif
>> +
>>   #endif    /* !__ASSEMBLY__ */
>>   #endif
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 460d799e1296..7066a273c1e0 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -368,6 +368,97 @@ static bool 
>> is_el1_mte_sync_tag_check_fault(unsigned long esr)
>>       return false;
>>   }
>>   +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
>> +static inline bool is_vmemmap_address(unsigned long addr)
>> +{
>> +    return (addr >= VMEMMAP_START) && (addr < VMEMMAP_END);
>> +}
>> +
>> +static inline bool vmemmap_fault_may_fixup(unsigned long addr,
>> +                       unsigned long esr)
>> +{
>> +    if (!is_vmemmap_address(addr))
>> +        return false;
>> +
>> +    /*
>> +     * Only try to handle translation fault level 2 or level 3,
>> +     * because hugetlb vmemmap optimize only clear pmd or pte.
>> +     */
>> +    switch (esr & ESR_ELx_FSC) {
>> +    case ESR_ELx_FSC_FAULT_L2:
>> +    case ESR_ELx_FSC_FAULT_L3:
>> +        return true;
>> +    default:
>> +        return false;
>> +    }
>> +}
>> +
>> +/*
>> + * PMD mapped vmemmap should has been split as PTE mapped
>> + * by HVO now, here we only check this case, other cases
>> + * should fail.
>> + * Also should check the addr is healthy enough that will not cause
>> + * a level2 or level3 translation fault again after page fault
>> + * handled with success, so we need check both bits[1:0] of PMD and
>> + * PTE as ARM Spec mentioned below:
>> + * A Translation fault is generated if bits[1:0] of a translation
>> + * table descriptor identify the descriptor as either a Fault
>> + * encoding or a reserved encoding.
>> + */
>> +static inline bool vmemmap_addr_healthy(unsigned long addr)
>> +{
>> +    pgd_t *pgdp;
>> +    p4d_t *p4dp;
>> +    pud_t *pudp, pud;
>> +    pmd_t *pmdp, pmd;
>> +    pte_t *ptep, pte;
>> +
>> +    pgdp = pgd_offset_k(addr);
>> +    if (pgd_none(READ_ONCE(*pgdp)))
>> +        return false;
>> +
>> +    p4dp = p4d_offset(pgdp, addr);
>> +    if (p4d_none(READ_ONCE(*p4dp)))
>> +        return false;
>> +
>> +    pudp = pud_offset(p4dp, addr);
>> +    pud = READ_ONCE(*pudp);
>> +    if (pud_none(pud))
>> +        return false;
>> +
>> +    pmdp = pmd_offset(pudp, addr);
>
> We already make sure it is a translation fault of level 2 or 3
> here, so we could use pmd_offset_k() macro to simplify the code
> a little. Right?
>
>> +    pmd = READ_ONCE(*pmdp);
>> +    if (!pmd_table(pmd))
>> +        return false;
>> +
>> +    ptep = pte_offset_kernel(pmdp, addr);
>> +    pte = READ_ONCE(*ptep);
>
> Please use ptep_get (which is supposed to do this) to access the
> raw pte, see commit c33c794828f21217.
>
>> +    return (pte_val(pte) & PTE_TYPE_MASK) == PTE_TYPE_PAGE;
>> +}
>> +
>> +static bool vmemmap_handle_page_fault(unsigned long addr,
>> +                      unsigned long esr)
>> +{
>> +    bool ret = false;
>> +
>> +    if (likely(!vmemmap_fault_may_fixup(addr, esr)))
>> +        return false;
>> +
>> +    spin_lock(&init_mm.page_table_lock);
>> +    if (vmemmap_addr_healthy(addr))
>> +        ret = true;
>
> It is to assign the return value to ret directly. Like:
>
>         ret = vmemmap_addr_healthy(addr);
>
> The the initializetion to ret also can be dropped.
>
>> + spin_unlock(&init_mm.page_table_lock);
>> +
>> +    return ret;
>> +}
>> +#else
>> +static inline bool vmemmap_handle_page_fault(unsigned long addr,
>> +                         unsigned long esr)
>> +{
>> +    return false;
>> +}
>> +#endif /*CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP */
>           ^
> Miss a blank between "*" and "C" here.
>
> Thanks.
>
>> +
>>   static bool is_translation_fault(unsigned long esr)
>>   {
>>       return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_FAULT;
>> @@ -409,6 +500,9 @@ static void __do_kernel_fault(unsigned long addr, 
>> unsigned long esr,
>>               kfence_handle_page_fault(addr, esr & ESR_ELx_WNR, regs))
>>               return;
>>   +        if (vmemmap_handle_page_fault(addr, esr))
>> +            return;
>> +
>>           msg = "paging request";
>>       }
>>   diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 15f6347d23b6..81a600ccac7c 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1146,6 +1146,34 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, 
>> int node,
>>       return 1;
>>   }
>>   +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
>> +/*
>> + * In the window between the page table entry is cleared and filled
>> + * with a new value, other threads have the opportunity to concurrently
>> + * access the vmemmap area then page translation fault occur.
>> + * Therefore, we need to ensure that the init_mm.page_table_lock is 
>> held
>> + * to synchronize the vmemmap page fault handling which will wait for
>> + * this lock to be released to ensure that the page table entry has 
>> been
>> + * refreshed with a new valid value.
>> + */
>> +void vmemmap_update_pmd(unsigned long start, pmd_t *pmd, pte_t 
>> *pgtable)
>> +{
>> +    lockdep_assert_held(&init_mm.page_table_lock);
>> +    pmd_clear(pmd);
>> +    flush_tlb_kernel_range(start, start + PMD_SIZE);
>> +    pmd_populate_kernel(&init_mm, pmd, pgtable);
>> +}
>> +
>> +void vmemmap_update_pte(unsigned long addr, pte_t *pte, pte_t entry)
>> +{
>> +    spin_lock(&init_mm.page_table_lock);
>> +    pte_clear(&init_mm, addr, pte);
>> +    flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>> +    set_pte_at(&init_mm, addr, pte, entry);
>> +    spin_unlock(&init_mm.page_table_lock);
>> +}
>> +#endif
>> +
>>   int __meminit vmemmap_populate(unsigned long start, unsigned long 
>> end, int node,
>>           struct vmem_altmap *altmap)
>>   {
>
>