Re: [PATCH 11/18] hugetlb_vmemmap: Use try_populate_vmemmap_pmd for replacing in-use PMDs

[James Houghton]

On Tue, Aug 18, 2026 at 8:53 AM Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
>
> On Wed, Jul 08, 2026 at 03:11:21AM +0000, James Houghton wrote:
> > diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h
> > index 1b4509d3382c..c8946250d431 100644
> > --- a/arch/arm64/include/asm/pgalloc.h
> > +++ b/arch/arm64/include/asm/pgalloc.h
> > @@ -121,4 +121,13 @@ pmd_populate(struct mm_struct *mm, pmd_t *pmdp, pgtable_t ptep)
> >                      PMD_TYPE_TABLE | PMD_TABLE_AF | PMD_TABLE_PXN);
> >  }
> >
> > +#define __HAVE_ARCH_TRY_POPULATE_VMEMMAP_PMD
> > +static inline int try_populate_vmemmap_pmd(pmd_t *pmdp, pte_t *pgtable,
> > +                                        unsigned long addr)
> > +{
> > +     /* BBML2_NOABORT is required. Its presence has been checked. */
> > +     pmd_populate_kernel(&init_mm, pmdp, pgtable);
> > +     return 0;
> > +}
>
> This patch adds the same function in 4 different arch files. Can we not
> have a single generic implementation in linux/pgalloc.h, bracketed by
> some ARCH_WANTS_* (macro or config)?
>
> I think the same goes for patch 4 with try_update_vmemmap_pte(), we can
> reduce the code duplication.

Yeah putting the generic version behind an ARCH_WANTS ifdef is so much
better. Then I can just nix the EOPNOTSUPP branch; a compile error is
always better.

> > diff --git a/include/linux/pgalloc.h b/include/linux/pgalloc.h
> > index 9174fa59bbc5..ed446d95ca37 100644
> > --- a/include/linux/pgalloc.h
> > +++ b/include/linux/pgalloc.h
> > @@ -26,4 +26,24 @@
> >                       arch_sync_kernel_mappings(addr, addr);          \
> >       } while (0)
> >
> > +#ifndef __HAVE_ARCH_TRY_POPULATE_VMEMMAP_PMD
> > +/*
> > + * try_populate_vmemmap_pmd - Populate a PMD that is in use by the vmemmap.
> > + * @addr: Base address of the remapped PMD.
> > + * @pmdp: Page table pointer to be overwritten.
> > + * @pgtable: Pointer to the page table that the new PMD will point to.
> > + *
> > + * This function is only to be used to update PMDs that map the vmemmap to
> > + * point to a page of already-populated PTEs that map the same pages.
> > + *
> > + * Implementations of this function must ensure that, while the update is taking
> > + * place, CPUs will not fault on the remapped virtual address range.
> > + */
> > +static inline int try_populate_vmemmap_pmd(pmd_t *pmdp, pte_t *pgtable,
> > +                                        unsigned long addr)
>
> Nit: if we follow try_update_vmemmap_pte(), we could place 'addr' first.
> It matches pgd_populate_kernel() as well in this file.

Will do.

> > +{
> > +     return -EOPNOTSUPP;
> > +}
> > +#endif
> > +
> >  #endif /* _LINUX_PGALLOC_H */
> > diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> > index 977249e22ed0..b445febac0d2 100644
> > --- a/mm/hugetlb_vmemmap.c
> > +++ b/mm/hugetlb_vmemmap.c
> > @@ -55,6 +55,7 @@ static int vmemmap_split_pmd(pmd_t *pmd, struct page *head, unsigned long start,
> >                            struct vmemmap_remap_walk *walk)
> >  {
> >       pmd_t __pmd;
> > +     int ret;
> >       int i;
> >       unsigned long addr = start;
> >       pte_t *pgtable;
> > @@ -74,8 +75,15 @@ static int vmemmap_split_pmd(pmd_t *pmd, struct page *head, unsigned long start,
> >               set_pte_at(&init_mm, addr, pte, entry);
> >       }
> >
> > +     ret = 0;
> >       spin_lock(&init_mm.page_table_lock);
> >       if (likely(pmd_leaf(*pmd))) {
> > +             /* Make pte visible before pmd. See comment in pmd_install(). */
> > +             smp_wmb();
> > +             ret = try_populate_vmemmap_pmd(pmd, pgtable, start);
> > +             if (ret)
> > +                     goto free;
> > +
> >               /*
> >                * Higher order allocations from buddy allocator must be able to
> >                * be treated as independent small pages (as they can be freed
> > @@ -84,21 +92,17 @@ static int vmemmap_split_pmd(pmd_t *pmd, struct page *head, unsigned long start,
> >               if (!PageReserved(head))
> >                       split_page(head, get_order(PMD_SIZE));
> >
> > -             /* Make pte visible before pmd. See comment in pmd_install(). */
> > -             smp_wmb();
> > -             /*
> > -              * On arm64, this requires BBML2_NOABORT. Its support has
> > -              * already been checked.
> > -              */
> > -             pmd_populate_kernel(&init_mm, pmd, pgtable);
> >               if (!(walk->flags & VMEMMAP_SPLIT_NO_TLB_FLUSH))
> >                       flush_tlb_kernel_range(start, start + PMD_SIZE);
> > -     } else {
> > -             pte_free_kernel(&init_mm, pgtable);
> > -     }
> > -     spin_unlock(&init_mm.page_table_lock);
> > +     } else
> > +             goto free;
> >
> > -     return 0;
> > +out:
> > +     spin_unlock(&init_mm.page_table_lock);
> > +     return ret;
> > +free:
> > +     pte_free_kernel(&init_mm, pgtable);
> > +     goto out;
> >  }
>
> Would 'free' first with fall through 'out' read better?

Yes, yes it would. I think the current ordering is just a holdover
from earlier attempts at writing this function.

Thanks!