Re: [PATCH v6 01/13] mm/migrate_device: Introduce migrate_pfn_from_page() helper

[Jordan Niethe]

Hi

On 2/3/26 20:22, David Hildenbrand (Arm) wrote:
> On 3/2/26 00:38, Jordan Niethe wrote:
> > Hi,
> >
> > On 28/2/26 08:11, David Hildenbrand (Arm) wrote:
> > > On 2/2/26 12:36, Jordan Niethe wrote:
> > > > To create a migrate entry from a given struct page, that page is first
> > > > converted to its pfn, before passing the pfn to migrate_pfn().
> > > >
> > > > A future change will remove device private pages from the physical
> > > > address space. This will mean that device private pages no longer have a
> > > > pfn and must be handled separately.
> > > >
> > > > Prepare for this with a new helper:
> > > >
> > > >       - migrate_pfn_from_page()
> > > >
> > > > This helper takes a struct page as parameter instead of a pfn. This will
> > > > allow more flexibility for handling the mpfn differently for device
> > > > private pages.
> > > >
> > > > Reviewed-by: Balbir Singh <balbirs@xxxxxxxxxx>
> > > > Acked-by: Felix Kuehling <felix.kuehling@xxxxxxx>
> > > > Signed-off-by: Jordan Niethe <jniethe@xxxxxxxxxx>
> > > > ---
> > >
> > > Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>
> I'll go through he remainder of the patchset this week.

Much appreciated.

> While skimming over patch #2, I was wondering whether
> "page_to_migration_pfn()" would better fit "migration_pfn_to_page".

I guess you were thinking about migration_pfn_/from/_page() rather than
"migration_pfn_to_page()"? Renaming it to page_to_migration_pfn() would be fine.

> ... and I was wondering why that code deals with pages instead of folios.

> E.g.,
>
> 	page = folio_page(folio, 0);
> 	mpfn[i] = migrate_pfn_from_page(page);
>
> Should just be
>
> 	mpfn[i] = folio_to_migration_pfn(folio);
>
> Right?

This patch is quite limited, essentially just converts usages of
migrate_pfn(page_to_pfn()) to migration_pfn_from_page().  However, I
agree, there could be scope for moving some of those usages to folios.

Was that example from drm_pagemap_migrate_populate_ram_pfn()?  There
'page' goes on to be used elsewhere in the function so we'd need some
further refactoring to fully benefit.

I see migrate_vma_collect_huge_pmd() as a candidate for a
folio_to_migration_pfn() function too.

Thanks,
Jordan.
>