Matthew Brost <matthew.brost@xxxxxxxxx> 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
>> >
