Re: [PATCH v2 02/29] mm/migrate: Add migrate_device_prepopulated_range

[Alistair Popple <apopple@nvidia.com>]

Matthew Brost <matthew.brost@intel.com> writes:

> On Wed, Oct 16, 2024 at 04:46:52AM +0000, Matthew Brost wrote:
>> On Wed, Oct 16, 2024 at 03:04:06PM +1100, Alistair Popple wrote:

[...]

>> > > +{
>> > > +	unsigned long i;
>> > > +
>> > > +	for (i = 0; i < npages; i++) {
>> > > +		struct page *page = pfn_to_page(src_pfns[i]);
>> > > +
>> > > +		if (!get_page_unless_zero(page)) {
>> > > +			src_pfns[i] = 0;
>> > > +			continue;
>> > > +		}
>> > > +
>> > > +		if (!trylock_page(page)) {
>> > > +			src_pfns[i] = 0;
>> > > +			put_page(page);
>> > > +			continue;
>> > > +		}
>> > > +
>> > > +		src_pfns[i] = migrate_pfn(src_pfns[i]) | MIGRATE_PFN_MIGRATE;
>> > 
>> > This needs to be converted to use a folio like
>> > migrate_device_range(). But more importantly this should be split out as
>> > a function that both migrate_device_range() and this function can call
>> > given this bit is identical.
>> > 
>> 
>> Missed the folio conversion and agree a helper shared between this
>> function and migrate_device_range would be a good idea. Let add that.
>> 
>
> Alistair,
>
> Ok, I think now I want to go slightly different direction here to give
> GPUSVM a bit more control over several eviction scenarios.
>
> What if I exported the helper discussed above, e.g.,
>
>  905 unsigned long migrate_device_pfn_lock(unsigned long pfn)
>  906 {
>  907         struct folio *folio;
>  908
>  909         folio = folio_get_nontail_page(pfn_to_page(pfn));
>  910         if (!folio)
>  911                 return 0;
>  912
>  913         if (!folio_trylock(folio)) {
>  914                 folio_put(folio);
>  915                 return 0;
>  916         }
>  917
>  918         return migrate_pfn(pfn) | MIGRATE_PFN_MIGRATE;
>  919 }
>  920 EXPORT_SYMBOL(migrate_device_pfn_lock);
>
> And then also export migrate_device_unmap.
>
> The usage here would be let a driver collect the device pages in virtual
> address range via hmm_range_fault, lock device pages under notifier
> lock ensuring device pages are valid, drop the notifier lock and call
> migrate_device_unmap.

I'm still working through this series but that seems a bit dubious, the
locking here is pretty subtle and easy to get wrong so seeing some code
would help me a lot in understanding what you're suggesting.

> Sima has strongly suggested avoiding a CPUVMA
> lookup during eviction cases and this would let me fixup
> drm_gpusvm_range_evict in [1] to avoid this.

That sounds reasonable but for context do you have a link to the
comments/discussion on this? I couldn't readily find it, but I may have
just missed it.

> It would also make the function exported in this patch unnecessary too
> as non-contiguous pfns can be setup on driver side via
> migrate_device_pfn_lock and then migrate_device_unmap can be called.
> This also another eviction usage in GPUSVM, see drm_gpusvm_evict_to_ram
> in [1].
>
> Do you see an issue exporting migrate_device_pfn_lock,
> migrate_device_unmap?

If there is a good justification for it I can't see a problem with
exporting it. That said I don't really understand why you would
want/need to split those steps up but I'll wait to see the code.

 - Alistair

> Matt
>
> [1] https://patchwork.freedesktop.org/patch/619809/?series=137870&rev=2
>
>> Matt
>> 
>> > > +	}
>> > > +
>> > > +	migrate_device_unmap(src_pfns, npages, NULL);
>> > > +
>> > > +	return 0;
>> > > +}
>> > > +EXPORT_SYMBOL(migrate_device_prepopulated_range);
>> > > +
>> > >  /*
>> > >   * Migrate a device coherent folio back to normal memory. The caller should have
>> > >   * a reference on folio which will be copied to the new folio if migration is
>> >