Re: [PATCH v4 08/17] mm/sparse-vmemmap: support section-based vmemmap optimization

[Mike Rapoport]

> Teach sparse-vmemmap population code to use the compound page order
> when deciding whether a vmemmap page can be optimized.
> 
> With this information, the common sparse-vmemmap population path can
> allocate or reuse shared tail vmemmap pages directly instead of relying
> on HugeTLB-specific handling.
> 
> This centralizes vmemmap optimization logic in the sparse-vmemmap code,
> based on section metadata, and prepares for sharing the same mechanism
> across different users of vmemmap optimization, including HugeTLB and
> DAX.
> 
> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
>
> diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
> index b770fe2428fd..737d50bdd3ef 100644
> --- a/mm/sparse-vmemmap.c
> +++ b/mm/sparse-vmemmap.c
> @@ -186,6 +186,11 @@ static __meminit struct page *vmemmap_get_tail(unsigned int order, struct zone *
>  
>  	return tail;
>  }
> +#else
> +static inline struct page *vmemmap_get_tail(unsigned int order, struct zone *zone)
> +{
> +	return NULL;
> +}
>  #endif
>  
>  static pte_t * __meminit vmemmap_pte_populate(pmd_t *pmd, unsigned long addr, int node,
> @@ -193,12 +198,24 @@ static pte_t * __meminit vmemmap_pte_populate(pmd_t *pmd, unsigned long addr, in
>  				       unsigned long ptpfn, unsigned long flags)
>  {
>  	pte_t *pte = pte_offset_kernel(pmd, addr);
> +	unsigned long pfn = page_to_pfn((struct page *)addr);
> +
>  	if (pte_none(ptep_get(pte))) {
>  		pte_t entry;
> -		void *p;
> +
> +		if (vmemmap_optimizable_pfn(pfn) && ptpfn == (unsigned long)-1) {
> +			unsigned int order = pfn_to_section_order(pfn);
> +			struct zone *zone = pfn_to_zone(pfn, node);
> +			struct page *page = vmemmap_get_tail(order, zone);
> +
> +			if (!page)
> +				return NULL;
> +			ptpfn = page_to_pfn(page);
> +		}
>  
>  		if (ptpfn == (unsigned long)-1) {
> -			p = vmemmap_alloc_block_buf(PAGE_SIZE, node, altmap);
> +			void *p = vmemmap_alloc_block_buf(PAGE_SIZE, node, altmap);
> +

What do you think about adding a helper function here, e.g something
like

diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
index 0aa0ab4f8b11..933b122f9937 100644
--- a/mm/sparse-vmemmap.c
+++ b/mm/sparse-vmemmap.c
@@ -191,6 +191,23 @@ static inline struct page *vmemmap_get_tail(unsigned int order, struct zone *zon
 }
 #endif
 
+static void * __meminit vmemmap_alloc_pte(unsigned long pfn, int node,
+					  struct vmem_altmap *altmap)
+{
+	unsigned int order = pfn_to_section_order(pfn);
+	struct zone *zone = pfn_to_zone(pfn, node);
+	struct page *page;
+
+	if (!vmemmap_optimizable_pfn(pfn))
+		return vmemmap_alloc_block_buf(PAGE_SIZE, node, altmap);
+
+	page = vmemmap_get_tail(order, zone);
+	if (!page)
+		return NULL;
+
+	return page_address(page);
+}
+
 static pte_t * __meminit vmemmap_pte_populate(pmd_t *pmd, unsigned long addr, int node,
 				       struct vmem_altmap *altmap,
 				       unsigned long ptpfn, unsigned long flags)
@@ -201,19 +218,8 @@ static pte_t * __meminit vmemmap_pte_populate(pmd_t *pmd, unsigned long addr, in
 	if (pte_none(ptep_get(pte))) {
 		pte_t entry;
 
-		if (vmemmap_optimizable_pfn(pfn) && ptpfn == (unsigned long)-1) {
-			unsigned int order = pfn_to_section_order(pfn);
-			struct zone *zone = pfn_to_zone(pfn, node);
-			struct page *page = vmemmap_get_tail(order, zone);
-
-			if (!page)
-				return NULL;
-			ptpfn = page_to_pfn(page);
-		}
-
 		if (ptpfn == (unsigned long)-1) {
-			void *p = vmemmap_alloc_block_buf(PAGE_SIZE, node, altmap);
-
+			void *p = vmemmap_alloc_pte(pfn, node, altmap);
 			if (!p)
 				return NULL;
 			ptpfn = PHYS_PFN(__pa(p));

>  			if (!p)
>  				return NULL;
>  			ptpfn = PHYS_PFN(__pa(p));
> @@ -217,7 +234,8 @@ static pte_t * __meminit vmemmap_pte_populate(pmd_t *pmd, unsigned long addr, in
>  		}
>  		entry = pfn_pte(ptpfn, PAGE_KERNEL);
>  		set_pte_at(&init_mm, addr, pte, entry);
> -	}
> +	} else if (WARN_ON_ONCE(vmemmap_optimizable_pfn(pfn)))
> +		return NULL;
>  	return pte;
>  }
>  
> @@ -406,6 +424,9 @@ int __meminit vmemmap_populate_hugepages(unsigned long start, unsigned long end,
>  	pmd_t *pmd;
>  
>  	for (addr = start; addr < end; addr = next) {
> +		unsigned long pfn = page_to_pfn((struct page *)addr);
> +		const struct mem_section *ms = __pfn_to_section(pfn);
> +
>  		next = pmd_addr_end(addr, end);
>  
>  		pgd = vmemmap_pgd_populate(addr, node);
> @@ -421,7 +442,7 @@ int __meminit vmemmap_populate_hugepages(unsigned long start, unsigned long end,
>  			return -ENOMEM;
>  
>  		pmd = pmd_offset(pud, addr);
> -		if (pmd_none(pmdp_get(pmd))) {
> +		if (pmd_none(pmdp_get(pmd)) && !section_vmemmap_optimizable(ms)) {
>  			void *p;
>  
>  			p = vmemmap_alloc_block_buf(PMD_SIZE, node, altmap);
> @@ -439,8 +460,11 @@ int __meminit vmemmap_populate_hugepages(unsigned long start, unsigned long end,
>  				 */
>  				return -ENOMEM;
>  			}
> -		} else if (vmemmap_check_pmd(pmd, node, addr, next))
> +		} else if (vmemmap_check_pmd(pmd, node, addr, next)) {
> +			if (WARN_ON_ONCE(section_vmemmap_optimizable(ms)))
> +				return -EOPNOTSUPP;
>  			continue;
> +		}
>  		if (vmemmap_populate_basepages(addr, next, node, altmap))
>  			return -ENOMEM;
>  	}
> @@ -620,6 +644,33 @@ void __init sparse_init_subsection_map(void)
>  		sparse_init_subsection_map_range(start, end - start);
>  }
>  
> +int __meminit section_nr_vmemmap_pages(unsigned long pfn, unsigned long nr_pages,
> +		struct vmem_altmap *altmap, struct dev_pagemap *pgmap)
> +{

Can we do all code movements in separate patches?

Maybe even move this to the previous patch and update the changelog to
say "move code around in preparation ..."

-- 
Sincerely yours,
Mike.