Re: [v6 02/15] mm/huge_memory: add device-private THP support to PMD operations

[Alistair Popple]

On 2025-09-25 at 19:53 +1000, David Hildenbrand <david@xxxxxxxxxx> wrote...
> On 25.09.25 02:25, Alistair Popple wrote:
> > On 2025-09-16 at 22:21 +1000, Balbir Singh <balbirs@xxxxxxxxxx> wrote...
> > > Extend core huge page management functions to handle device-private THP
> > > entries.  This enables proper handling of large device-private folios in
> > > fundamental MM operations.
> > > 
> > > The following functions have been updated:
> > > 
> > > - copy_huge_pmd(): Handle device-private entries during fork/clone
> > > - zap_huge_pmd(): Properly free device-private THP during munmap
> > > - change_huge_pmd(): Support protection changes on device-private THP
> > > - __pte_offset_map(): Add device-private entry awareness
> > > 
> > > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > > Signed-off-by: Balbir Singh <balbirs@xxxxxxxxxx>
> > > Cc: David Hildenbrand <david@xxxxxxxxxx>
> > > Cc: Zi Yan <ziy@xxxxxxxxxx>
> > > Cc: Joshua Hahn <joshua.hahnjy@xxxxxxxxx>
> > > Cc: Rakie Kim <rakie.kim@xxxxxx>
> > > Cc: Byungchul Park <byungchul@xxxxxx>
> > > Cc: Gregory Price <gourry@xxxxxxxxxx>
> > > Cc: Ying Huang <ying.huang@xxxxxxxxxxxxxxxxx>
> > > Cc: Alistair Popple <apopple@xxxxxxxxxx>
> > > Cc: Oscar Salvador <osalvador@xxxxxxx>
> > > Cc: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> > > Cc: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> > > Cc: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
> > > Cc: Nico Pache <npache@xxxxxxxxxx>
> > > Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
> > > Cc: Dev Jain <dev.jain@xxxxxxx>
> > > Cc: Barry Song <baohua@xxxxxxxxxx>
> > > Cc: Lyude Paul <lyude@xxxxxxxxxx>
> > > Cc: Danilo Krummrich <dakr@xxxxxxxxxx>
> > > Cc: David Airlie <airlied@xxxxxxxxx>
> > > Cc: Simona Vetter <simona@xxxxxxxx>
> > > Cc: Ralph Campbell <rcampbell@xxxxxxxxxx>
> > > Cc: Mika Penttilä <mpenttil@xxxxxxxxxx>
> > > Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
> > > Cc: Francois Dugast <francois.dugast@xxxxxxxxx>
> > > ---
> > >   include/linux/swapops.h | 32 +++++++++++++++++++++++
> > >   mm/huge_memory.c        | 56 ++++++++++++++++++++++++++++++++++-------
> > >   mm/pgtable-generic.c    |  2 +-
> > >   3 files changed, 80 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > > index 64ea151a7ae3..2687928a8146 100644
> > > --- a/include/linux/swapops.h
> > > +++ b/include/linux/swapops.h
> > > @@ -594,10 +594,42 @@ static inline int is_pmd_migration_entry(pmd_t pmd)
> > >   }
> > >   #endif  /* CONFIG_ARCH_ENABLE_THP_MIGRATION */
> > > +#if defined(CONFIG_ZONE_DEVICE) && defined(CONFIG_ARCH_ENABLE_THP_MIGRATION)
> > > +
> > > +/**
> > > + * is_pmd_device_private_entry() - Check if PMD contains a device private swap entry
> > > + * @pmd: The PMD to check
> > > + *
> > > + * Returns true if the PMD contains a swap entry that represents a device private
> > > + * page mapping. This is used for zone device private pages that have been
> > > + * swapped out but still need special handling during various memory management
> > > + * operations.
> > > + *
> > > + * Return: 1 if PMD contains device private entry, 0 otherwise
> > > + */
> > > +static inline int is_pmd_device_private_entry(pmd_t pmd)
> > > +{
> > > +	return is_swap_pmd(pmd) && is_device_private_entry(pmd_to_swp_entry(pmd));
> > > +}
> > > +
> > > +#else /* CONFIG_ZONE_DEVICE && CONFIG_ARCH_ENABLE_THP_MIGRATION */
> > > +
> > > +static inline int is_pmd_device_private_entry(pmd_t pmd)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +#endif /* CONFIG_ZONE_DEVICE && CONFIG_ARCH_ENABLE_THP_MIGRATION */
> > > +
> > >   static inline int non_swap_entry(swp_entry_t entry)
> > >   {
> > >   	return swp_type(entry) >= MAX_SWAPFILES;
> > >   }
> > > +static inline int is_pmd_non_present_folio_entry(pmd_t pmd)
> > 
> > I can't think of a better name either although I am curious why open-coding it
> > was so nasty given we don't have the equivalent for pte entries. Will go read
> > the previous discussion.
> 
> I think for PTEs we just handle all cases (markers, hwpoison etc) properly,
> manye not being supported yet on the PMD level. See copy_nonpresent_pte() as
> an example.
> 
> We don't even have helpers like is_pte_migration_entry().
> 
> > > diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> > > index 567e2d084071..0c847cdf4fd3 100644
> > > --- a/mm/pgtable-generic.c
> > > +++ b/mm/pgtable-generic.c
> > > @@ -290,7 +290,7 @@ pte_t *___pte_offset_map(pmd_t *pmd, unsigned long addr, pmd_t *pmdvalp)
> > >   	if (pmdvalp)
> > >   		*pmdvalp = pmdval;
> > > -	if (unlikely(pmd_none(pmdval) || is_pmd_migration_entry(pmdval)))
> > > +	if (unlikely(pmd_none(pmdval) || !pmd_present(pmdval)))
> > 
> > Why isn't is_pmd_non_present_folio_entry() used here?
> 
> 
> I thought I argued that
> 
> 	if (!pmd_present(pmdval)))
> 
> Should be sufficient here in my last review?

My bad, I'm a bit behind catching up on the last review comments. But agree it's
sufficient, was just curious why it wasn't used so will go read your previous
comments! Thanks.

> We want to detect page tables we can map after all.
> -- 
> Cheers
> 
> David / dhildenb
>