Re: [RFC PATCH 6/6] mm: Remove device private pages from the physical address space

[Alistair Popple <apopple@nvidia.com>]

On 2025-12-02 at 13:28 +1100, Jordan Niethe <jniethe@nvidia.com> wrote...
> Hi,
> 
> On 29/11/25 04:51, Jason Gunthorpe wrote:
> > On Fri, Nov 28, 2025 at 03:41:46PM +1100, Jordan Niethe wrote:
> > > Introduce helpers:
> > > 
> > >    - device_private_page_to_offset()
> > >    - device_private_folio_to_offset()
> > > 
> > > to take a given device private page / folio and return its offset within
> > > the device private address space (this is essentially a PFN within the
> > > device private address space).
> > 
> > It would be nice if we rarely/never needed to see number space outside
> > the pte itself or the internal helpers..
> 
> Outside of the PTE itself, one of the use cases for the PFNs themselves
> is range checking. Like we see in mm/page_vma_mapped.c:check_pte().
> 
> > 
> > Like, I don't think there should be stuff like this:
> > 
> > >   					entry = make_writable_migration_device_private_entry(
> > > -								page_to_pfn(page));
> > > +								device_private_page_to_offset(page));
> > 
> > make_writable_migration_device_private_entry() should accept the
> > struct page as the handle?
> 
> That would be more clean - I'll give it a try.
> 
> > 
> > If it really is needed I think it should have its own dedicated type
> > and not be intermixed with normal pfns..
> 
> One consideration here is for things like range checking the PFNs, the
> logic remains the same for device PFNs and the normal PFNs.
> If we represent the device PFNs as a unique type, ideally we'd like to
> still avoid introducing too much special handling.

Right, Jordan and I went back and forth on this a little bit prior to posting
but in the end I thought it wasn't worth the overhead of a new type for such a
limited number of use cases for which the actual logic ends up being the same
anyway.

Getting rid of passing the pfn to make_writable_migration_device_private_entry()
makes sense though and should address most of these cases.

> Potentially I could see something like a tagged union for memory indices
> like ...
> 
> enum memory_index_type {
>         MEMORY_INDEX_TYPE_PFN,
>         MEMORY_INDEX_TYPE_DEVICE_MEMORY_INDEX,
> };
> 
> union memory_index {
>         unsigned long pfn;
>         unsigned long device_memory_index;
>         enum memory_index_type type;
> };
> 
> ... if we wanted to introduce a dedicated type.
> 
> Another possibility could be to avoid exposing the PFN for cases like
> this.
> 
> For example if we went back to struct page_vma_mapped_walk containing a
> folio / struct page instead of a passing in a pfn then we could introduce
> some helper
> like ...
> 
>         bool swap_entry_contains_folio(struct folio *folio, swp_entry_t
> entry);
> 
> ... that handles both device memory and normal memory and use that in
> check_pte().
> 
> Thanks,
> Jordan.
> 
> 
> > 
> > Jason
>