Re: [PATCH 4/4] mm/sparse-vmemmap: drop @pgmap from vmemmap population APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm/sparse-vmemmap: section_activate() ignores ARCH_WANT_OPTIMIZE_DAX_VMEMMAP
- [Medium] mm/sparse-vmemmap: section_activate() stores non-optimizable compound order
- [Low] powerpc/mm: missing vmemmap-optimization.h include in init_64.c
--

commit 48cc74f276edacd74ae0108659b8fbbf37ab5193
Author: Muchun Song <muchun.song@linux.dev>

mm/sparse-vmemmap: drop @pgmap from vmemmap population APIs

This commit removes the @pgmap parameter from the vmemmap population helpers
and related memory hotplug interfaces, relying on mem_section state and
architecture-specific population code to determine whether vmemmap mappings
can be optimized.

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 4fb48fbe712d2..b4b63f89d1884 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -5345,8 +5345,7 @@ static inline void print_vma_addr(char *prefix, unsigned long rip)
>  #endif
>  
>  struct page * __populate_section_memmap(unsigned long pfn,
> -		unsigned long nr_pages, int nid, struct vmem_altmap *altmap,
> -		struct dev_pagemap *pgmap);
> +		unsigned long nr_pages, int nid, struct vmem_altmap *altmap);

[Severity: Low]
Does building arch/powerpc/mm/init_64.c with CONFIG_SPARSEMEM_VMEMMAP=y fail
due to an implicit declaration of section_set_compound_order() after the
earlier changes in this series?

In commit 84948c20ec18 ("powerpc/mm: drop powerpc vmemmap_can_optimize()"),
vmemmap_populate() in arch/powerpc/mm/init_64.c was updated to call
section_set_compound_order():

arch/powerpc/mm/init_64.c:vmemmap_populate() {
	...
	section_set_compound_order(__pfn_to_section(page_to_pfn((struct page *)start)), 0);
	return __vmemmap_populate(start, end, node, altmap);
}

Then commit 4171e4b9fc2a ("mm/sparse-vmemmap: drop vmemmap_can_optimize()")
removed the linux/vmemmap-optimization.h include from include/linux/mm.h:

include/linux/mm.h:
 #include <linux/bitops.h>
 #include <linux/iommu-debug-pagealloc.h>
 #include <linux/kcsan-checks.h>
-#include <linux/vmemmap-optimization.h>

Because neither arch/powerpc/mm/init_64.c nor any header it includes pulls in
linux/vmemmap-optimization.h, should arch/powerpc/mm/init_64.c include
linux/vmemmap-optimization.h directly?

[ ... ]

> diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
> index 43559593107c9..df71125a6ee23 100644
> --- a/mm/sparse-vmemmap.c
> +++ b/mm/sparse-vmemmap.c

[ ... ]

> @@ -739,9 +736,9 @@ static struct page * __meminit section_activate(int nid, unsigned long pfn,
>  		return pfn_to_page(pfn);
>  
>  	section_set_compound_order_range(pfn, nr_pages, order);

[Severity: Medium]
What happens in section_activate() when pgmap->vmemmap_shift is non-zero, but
smaller than VMEMMAP_OPTIMIZATION_MIN_ORDER (or sizeof(struct page) is not a
power of two)?

Previously, section_activate() gated order on vmemmap_can_optimize(altmap,
pgmap), leaving ms->compound_page_order as 0 whenever the order was not
eligible for vmemmap optimization. With commit 4171e4b9fc2a ("mm/sparse-vmemmap:
drop vmemmap_can_optimize()") and this commit, section_activate() sets:

mm/sparse-vmemmap.c:section_activate() {
	...
	unsigned int order = pgmap ? pgmap->vmemmap_shift : 0;
	int rc;

	if (nr_pages < PAGES_PER_SECTION && section_compound_order(ms))
		return ERR_PTR(-EOPNOTSUPP);
	...
	section_set_compound_order_range(pfn, nr_pages, order);
	memmap = populate_section_memmap(pfn, nr_pages, nid, altmap);
}

And __populate_section_memmap() only clears compound_page_order when both
altmap and section_vmemmap_optimizable(ms) are true:

mm/sparse-vmemmap.c:__populate_section_memmap() {
	...
	if (altmap && section_vmemmap_optimizable(ms))
		section_set_compound_order(ms, 0);
}

If 0 < pgmap->vmemmap_shift < VMEMMAP_OPTIMIZATION_MIN_ORDER (for instance, a
2MB PMD-aligned devdax range with vmemmap_shift = 5 on 64KB-page PowerPC
where VMEMMAP_OPTIMIZATION_MIN_ORDER is 11), section_vmemmap_optimizable(ms)
returns false. However, ms->compound_page_order remains set to that non-zero
order even though the section's vmemmap is not optimized.

If a subsection of that section is later deactivated (leaving the section
non-empty) and re-added with nr_pages < PAGES_PER_SECTION, won't the
section_compound_order(ms) check in section_activate() falsely reject the
subsection activation with -EOPNOTSUPP?

> -	memmap = populate_section_memmap(pfn, nr_pages, nid, altmap, pgmap);
> +	memmap = populate_section_memmap(pfn, nr_pages, nid, altmap);

[Severity: High]
Could dropping vmemmap_can_optimize() and @pgmap unintentionally enable DAX
vmemmap optimization on architectures that select
ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP but not ARCH_WANT_OPTIMIZE_DAX_VMEMMAP,
such as 64-bit LoongArch?

Previously, __vmemmap_can_optimize() was guarded by
CONFIG_ARCH_WANT_OPTIMIZE_DAX_VMEMMAP, so section_activate() kept order = 0
for dev_pagemap mappings when that option was disabled. After commit
4171e4b9fc2a ("mm/sparse-vmemmap: drop vmemmap_can_optimize()") and this
commit, CONFIG_ARCH_WANT_OPTIMIZE_DAX_VMEMMAP is no longer checked. Instead,
section_activate() unconditionally records pgmap->vmemmap_shift via
section_set_compound_order_range(), and section_vmemmap_optimizable() ->
vmemmap_optimizable_order() only checks IS_ENABLED(CONFIG_VMEMMAP_OPTIMIZATION).

On 64-bit LoongArch, ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP is selected (enabling
CONFIG_VMEMMAP_OPTIMIZATION when CONFIG_HUGETLB_PAGE=y), but
ARCH_WANT_OPTIMIZE_DAX_VMEMMAP is not selected, and vmemmap_free() is an
empty stub:

arch/loongarch/mm/init.c:
void vmemmap_free(unsigned long start, unsigned long end, struct vmem_altmap *altmap)
{
}

When memremap_pages() maps a dev_pagemap with vmemmap_shift >=
VMEMMAP_OPTIMIZATION_MIN_ORDER on LoongArch:

memremap_pages()
  pagemap_range()
    arch_add_memory() / add_pages()
      __add_pages()
        sparse_add_section()
          section_activate()
            populate_section_memmap()
              __populate_section_memmap()
                vmemmap_populate()
                  vmemmap_populate_hugepages()
                    vmemmap_populate_basepages()
                      vmemmap_pte_populate()
                        vmemmap_alloc_pte()

vmemmap_pte_populate() now installs read-only shared tail PTEs and takes
page references on the shared tail page. When memunmap_pages() later runs,
LoongArch's no-op vmemmap_free() leaves those PTEs mapped and leaks the
vmemmap pages while section_deactivate() resets compound_page_order to 0,
causing subsequent hot-add or memremap_pages() calls on that PFN range to
trigger WARN_ON_ONCE(vmemmap_optimizable_pfn(pfn)) in vmemmap_pte_populate()
or fault when writing to read-only vmemmap pages.

Should section_activate() still gate pgmap->vmemmap_shift on
IS_ENABLED(CONFIG_ARCH_WANT_OPTIMIZE_DAX_VMEMMAP) (as well as
!altmap && vmemmap_optimizable_order(pgmap->vmemmap_shift))?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030140720.1256062-1-songmuchun@bytedance.com?part=4