Re: [PATCH V4 mm-hotfixes 2/3] mm: introduce and use {pgd,p4d}_populate_kernel()

[Harry Yoo <harry.yoo@oracle.com>]

On Mon, Aug 25, 2025 at 01:27:20PM +0200, Christophe Leroy wrote:
> 
> 
> Le 11/08/2025 à 07:34, Harry Yoo a écrit :
> > Introduce and use {pgd,p4d}_populate_kernel() in core MM code when
> > populating PGD and P4D entries for the kernel address space.
> > These helpers ensure proper synchronization of page tables when
> > updating the kernel portion of top-level page tables.
> > 
> > Until now, the kernel has relied on each architecture to handle
> > synchronization of top-level page tables in an ad-hoc manner.
> > For example, see commit 9b861528a801 ("x86-64, mem: Update all PGDs for
> > direct mapping and vmemmap mapping changes").
> > 
> > However, this approach has proven fragile for following reasons:
> > 
> >    1) It is easy to forget to perform the necessary page table
> >       synchronization when introducing new changes.
> >       For instance, commit 4917f55b4ef9 ("mm/sparse-vmemmap: improve memory
> >       savings for compound devmaps") overlooked the need to synchronize
> >       page tables for the vmemmap area.
> > 
> >    2) It is also easy to overlook that the vmemmap and direct mapping areas
> >       must not be accessed before explicit page table synchronization.
> >       For example, commit 8d400913c231 ("x86/vmemmap: handle unpopulated
> >       sub-pmd ranges")) caused crashes by accessing the vmemmap area
> >       before calling sync_global_pgds().
> > 
> > To address this, as suggested by Dave Hansen, introduce _kernel() variants
> > of the page table population helpers, which invoke architecture-specific
> > hooks to properly synchronize page tables. These are introduced in a new
> > header file, include/linux/pgalloc.h, so they can be called from common code.
> > 
> > They reuse existing infrastructure for vmalloc and ioremap.
> > Synchronization requirements are determined by ARCH_PAGE_TABLE_SYNC_MASK,
> > and the actual synchronization is performed by arch_sync_kernel_mappings().
> > 
> > This change currently targets only x86_64, so only PGD and P4D level
> > helpers are introduced. In theory, PUD and PMD level helpers can be added
> > later if needed by other architectures.
> 
> AFAIK pmd_populate_kernel() already exist on all architectures, and I'm not
> sure it does what you expect. Or am I missing something ?

It does not do what I expect.

Yes, if someone is going to introduce a PMD level helper, existing
pmd_populate_kernel() should be renamed or removed.

To be honest I'm not really sure why we need both pmd_populate() and
pmd_populate_kernel(). It is introduced by historical commit
3a0b82c08a0e8668 ("adds simple support for atomically-mapped PTEs.
On highmem systems this enables the allocation of the pagetables in
highmem.") [1], but as there's no explanation or comment so I can only
speculate.

Key differences I recognize is 1) the type of the last parameter is
pgtable_t (which can be either struct page * or pte_t * depending on
architecture) in pmd_populate() and pte_t * in pmd_populate_kernel(),
and 2) some architectures treat user and kernel page tables differently.

Regarding 1), I think a reasonable experience is that pmd_populate()
should take struct page * in some architectures because
with CONFIG_HIGHPTE=y pte_t * might not be accessible, but kernel
page tables are not allocated from highmem even with CONFIG_HIGHPTE=y
so pmd_populate_kernel() can take pte_t *, and that can save a few
instructions.

And some architectures (that does not support HIGHPTE?) define pgtable_t
as pte_t * to support sub-page page tables (Commit 2f569afd9ced
("CONFIG_HIGHPTE vs. sub-page page tables.")).

Maybe things to clean up in the future:

1) Once CONFIG_HIGHPTE is completely dropped (is that ever going to
   happen?), pte_t * can be used instead of struct page *. 

2) Convert users of pmd_populate_kernel() to use pmd_populate().
   But some architectures treat user and kernel page tables differently
   and that will be handled in pmd_populate()  (depending on
   (mm == &init_mm))

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=3a0b82c08a0e86683783c30d7fec9d1b06c2fe20

-- 
Cheers,
Harry / Hyeonggon