Re: [PATCH v7 04/14] mm: add device coherent vma selection for memory migration

David Hildenbrand
2022-06-30T10:04:25Z

On 29.06.22 05:54, Alex Sierra wrote:
> This case is used to migrate pages from device memory, back to system
> memory. Device coherent type memory is cache coherent from device and CPU
> point of view.
>
> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
> Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>
> Reviewed-by: Alistair Poppple <apopple@nvidia.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I'm not too familiar with this code, please excuse my naive questions:

> @@ -148,15 +148,21 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
> if (is_writable_device_private_entry(entry))
> mpfn |= MIGRATE_PFN_WRITE;
> } else {
> - if (!(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
> - goto next;

Why not exclude MIGRATE_VMA_SELECT_DEVICE_PRIVATE here? IIRC that would
have happened before this change.

> pfn = pte_pfn(pte);
> - if (is_zero_pfn(pfn)) {
> + if (is_zero_pfn(pfn) &&
> + (migrate->flags & MIGRATE_VMA_SELECT_SYSTEM)) {
> mpfn = MIGRATE_PFN_MIGRATE;
> migrate->cpages++;
> goto next;
> }
> page = vm_normal_page(migrate->vma, addr, pte);
> + if (page && !is_zone_device_page(page) &&

I'm wondering if that check logically belongs into patch #2.

> + !(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
> + goto next;
> + else if (page && is_device_coherent_page(page) &&
> + (!(migrate->flags & MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
> + page->pgmap->owner != migrate->pgmap_owner))

In general LGTM

--
Thanks,

David / dhildenb