Messages in this thread Patch in this message | | | Date | Mon, 25 Oct 2021 17:57:06 -0700 | Subject | Re: [PATCH] mm/migrate.c: Remove MIGRATE_PFN_LOCKED | From | Ralph Campbell <> |
| |
On 10/24/21 21:16, Alistair Popple wrote: > MIGRATE_PFN_LOCKED is used to indicate to migrate_vma_prepare() that a > source page was already locked during migrate_vma_collect(). If it > wasn't then the a second attempt is made to lock the page. However if > the first attempt failed it's unlikely a second attempt will succeed, > and the retry adds complexity. So clean this up by removing the retry > and MIGRATE_PFN_LOCKED flag. > > Destination pages are also meant to have the MIGRATE_PFN_LOCKED flag > set, but nothing actually checks that. > > Signed-off-by: Alistair Popple <apopple@nvidia.com>
You can add: Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
> --- > Documentation/vm/hmm.rst | 2 +- > arch/powerpc/kvm/book3s_hv_uvmem.c | 4 +- > drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 2 - > drivers/gpu/drm/nouveau/nouveau_dmem.c | 4 +- > include/linux/migrate.h | 1 - > lib/test_hmm.c | 5 +- > mm/migrate.c | 145 +++++------------------ > 7 files changed, 35 insertions(+), 128 deletions(-) > > diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst > index a14c2938e7af..f2a59ed82ed3 100644 > --- a/Documentation/vm/hmm.rst > +++ b/Documentation/vm/hmm.rst > @@ -360,7 +360,7 @@ between device driver specific code and shared common code: > system memory page, locks the page with ``lock_page()``, and fills in the > ``dst`` array entry with:: > > - dst[i] = migrate_pfn(page_to_pfn(dpage)) | MIGRATE_PFN_LOCKED; > + dst[i] = migrate_pfn(page_to_pfn(dpage)); > > Now that the driver knows that this page is being migrated, it can > invalidate device private MMU mappings and copy device private memory > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > index a7061ee3b157..28c436df9935 100644 > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > @@ -560,7 +560,7 @@ static int __kvmppc_svm_page_out(struct vm_area_struct *vma, > gpa, 0, page_shift); > > if (ret == U_SUCCESS) > - *mig.dst = migrate_pfn(pfn) | MIGRATE_PFN_LOCKED; > + *mig.dst = migrate_pfn(pfn); > else { > unlock_page(dpage); > __free_page(dpage); > @@ -774,7 +774,7 @@ static int kvmppc_svm_page_in(struct vm_area_struct *vma, > } > } > > - *mig.dst = migrate_pfn(page_to_pfn(dpage)) | MIGRATE_PFN_LOCKED; > + *mig.dst = migrate_pfn(page_to_pfn(dpage)); > migrate_vma_pages(&mig); > out_finalize: > migrate_vma_finalize(&mig); > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c > index 4a16e3c257b9..41d9417f182b 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c > @@ -300,7 +300,6 @@ svm_migrate_copy_to_vram(struct amdgpu_device *adev, struct svm_range *prange, > migrate->dst[i] = svm_migrate_addr_to_pfn(adev, dst[i]); > svm_migrate_get_vram_page(prange, migrate->dst[i]); > migrate->dst[i] = migrate_pfn(migrate->dst[i]); > - migrate->dst[i] |= MIGRATE_PFN_LOCKED; > src[i] = dma_map_page(dev, spage, 0, PAGE_SIZE, > DMA_TO_DEVICE); > r = dma_mapping_error(dev, src[i]); > @@ -580,7 +579,6 @@ svm_migrate_copy_to_ram(struct amdgpu_device *adev, struct svm_range *prange, > dst[i] >> PAGE_SHIFT, page_to_pfn(dpage)); > > migrate->dst[i] = migrate_pfn(page_to_pfn(dpage)); > - migrate->dst[i] |= MIGRATE_PFN_LOCKED; > j++; > } > > diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c > index 92987daa5e17..3828aafd3ac4 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c > +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c > @@ -166,7 +166,7 @@ static vm_fault_t nouveau_dmem_fault_copy_one(struct nouveau_drm *drm, > goto error_dma_unmap; > mutex_unlock(&svmm->mutex); > > - args->dst[0] = migrate_pfn(page_to_pfn(dpage)) | MIGRATE_PFN_LOCKED; > + args->dst[0] = migrate_pfn(page_to_pfn(dpage)); > return 0; > > error_dma_unmap: > @@ -602,7 +602,7 @@ static unsigned long nouveau_dmem_migrate_copy_one(struct nouveau_drm *drm, > ((paddr >> PAGE_SHIFT) << NVIF_VMM_PFNMAP_V0_ADDR_SHIFT); > if (src & MIGRATE_PFN_WRITE) > *pfn |= NVIF_VMM_PFNMAP_V0_W; > - return migrate_pfn(page_to_pfn(dpage)) | MIGRATE_PFN_LOCKED; > + return migrate_pfn(page_to_pfn(dpage)); > > out_dma_unmap: > dma_unmap_page(dev, *dma_addr, PAGE_SIZE, DMA_BIDIRECTIONAL); > diff --git a/include/linux/migrate.h b/include/linux/migrate.h > index c8077e936691..479b861ae490 100644 > --- a/include/linux/migrate.h > +++ b/include/linux/migrate.h > @@ -119,7 +119,6 @@ static inline int migrate_misplaced_page(struct page *page, > */ > #define MIGRATE_PFN_VALID (1UL << 0) > #define MIGRATE_PFN_MIGRATE (1UL << 1) > -#define MIGRATE_PFN_LOCKED (1UL << 2) > #define MIGRATE_PFN_WRITE (1UL << 3) > #define MIGRATE_PFN_SHIFT 6 > > diff --git a/lib/test_hmm.c b/lib/test_hmm.c > index c259842f6d44..e2ce8f9b7605 100644 > --- a/lib/test_hmm.c > +++ b/lib/test_hmm.c > @@ -613,8 +613,7 @@ static void dmirror_migrate_alloc_and_copy(struct migrate_vma *args, > */ > rpage->zone_device_data = dmirror; > > - *dst = migrate_pfn(page_to_pfn(dpage)) | > - MIGRATE_PFN_LOCKED; > + *dst = migrate_pfn(page_to_pfn(dpage)); > if ((*src & MIGRATE_PFN_WRITE) || > (!spage && args->vma->vm_flags & VM_WRITE)) > *dst |= MIGRATE_PFN_WRITE; > @@ -1137,7 +1136,7 @@ static vm_fault_t dmirror_devmem_fault_alloc_and_copy(struct migrate_vma *args, > lock_page(dpage); > xa_erase(&dmirror->pt, addr >> PAGE_SHIFT); > copy_highpage(dpage, spage); > - *dst = migrate_pfn(page_to_pfn(dpage)) | MIGRATE_PFN_LOCKED; > + *dst = migrate_pfn(page_to_pfn(dpage)); > if (*src & MIGRATE_PFN_WRITE) > *dst |= MIGRATE_PFN_WRITE; > } > diff --git a/mm/migrate.c b/mm/migrate.c > index a6a7743ee98f..915e969811d0 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -2369,7 +2369,6 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp, > * can't be dropped from it). > */ > get_page(page); > - migrate->cpages++;
Why not move the get_page() into the "if (trylock_page())" instead of calling put_page() in the else case.
> > /* > * Optimize for the common case where page is only mapped once > @@ -2379,7 +2378,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp, > if (trylock_page(page)) { > pte_t swp_pte; > > - mpfn |= MIGRATE_PFN_LOCKED; > + migrate->cpages++; > ptep_get_and_clear(mm, addr, ptep);
I was looking at try_to_migrate_one() and looking at the differences with the code here to insert the migration PTE and noticed that instead of ptet_get_and_clear() it has: pteval = ptep_clear_flush(vma, address, pvmw.pte);
/* Move the dirty bit to the page. Now the pte is gone. */ if (pte_dirty(pteval)) set_page_dirty(page); update_hiwater_rss(mm);
I know that is pre-existing, probably a separate patch if it is an issue.
> > /* Setup special migration page table entry */ > @@ -2413,6 +2412,9 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp, > > if (pte_present(pte)) > unmapped++; > + } else { > + put_page(page); > + mpfn = 0; > } > > next: > @@ -2517,15 +2519,17 @@ static bool migrate_vma_check_page(struct page *page) > } > > /* > - * migrate_vma_prepare() - lock pages and isolate them from the lru > + * migrate_vma_unmap() - replace page mapping with special migration pte entry > * @migrate: migrate struct containing all migration information > * > - * This locks pages that have been collected by migrate_vma_collect(). Once each > - * page is locked it is isolated from the lru (for non-device pages). Finally, > - * the ref taken by migrate_vma_collect() is dropped, as locked pages cannot be > - * migrated by concurrent kernel threads. > + * Isolate pages from the LRU and replace mappings (CPU page table pte) with a > + * special migration pte entry and check if it has been pinned. Pinned pages are > + * restored because we cannot migrate them. > + * > + * This is the last step before we call the device driver callback to allocate > + * destination memory and copy contents of original page over to new page. > */ > -static void migrate_vma_prepare(struct migrate_vma *migrate) > +static void migrate_vma_unmap(struct migrate_vma *migrate) > { > const unsigned long npages = migrate->npages; > const unsigned long start = migrate->start; > @@ -2534,32 +2538,12 @@ static void migrate_vma_prepare(struct migrate_vma *migrate) > > lru_add_drain(); > > - for (i = 0; (i < npages) && migrate->cpages; i++) { > + for (i = 0; i < npages; i++) { > struct page *page = migrate_pfn_to_page(migrate->src[i]); > - bool remap = true; > > if (!page) > continue; > > - if (!(migrate->src[i] & MIGRATE_PFN_LOCKED)) { > - /* > - * Because we are migrating several pages there can be > - * a deadlock between 2 concurrent migration where each > - * are waiting on each other page lock. > - * > - * Make migrate_vma() a best effort thing and backoff > - * for any page we can not lock right away. > - */ > - if (!trylock_page(page)) { > - migrate->src[i] = 0; > - migrate->cpages--; > - put_page(page); > - continue; > - } > - remap = false; > - migrate->src[i] |= MIGRATE_PFN_LOCKED; > - } > - > /* ZONE_DEVICE pages are not on LRU */ > if (!is_zone_device_page(page)) { > if (!PageLRU(page) && allow_drain) { > @@ -2569,16 +2553,9 @@ static void migrate_vma_prepare(struct migrate_vma *migrate) > } > > if (isolate_lru_page(page)) { > - if (remap) { > - migrate->src[i] &= ~MIGRATE_PFN_MIGRATE; > - migrate->cpages--; > - restore++; > - } else { > - migrate->src[i] = 0; > - unlock_page(page); > - migrate->cpages--; > - put_page(page); > - } > + migrate->src[i] &= ~MIGRATE_PFN_MIGRATE; > + migrate->cpages--; > + restore++; > continue; > } > > @@ -2586,80 +2563,20 @@ static void migrate_vma_prepare(struct migrate_vma *migrate) > put_page(page); > } > > - if (!migrate_vma_check_page(page)) { > - if (remap) { > - migrate->src[i] &= ~MIGRATE_PFN_MIGRATE; > - migrate->cpages--; > - restore++; > - > - if (!is_zone_device_page(page)) { > - get_page(page); > - putback_lru_page(page); > - } > - } else { > - migrate->src[i] = 0; > - unlock_page(page); > - migrate->cpages--; > + if (page_mapped(page)) > + try_to_migrate(page, 0); > > - if (!is_zone_device_page(page)) > - putback_lru_page(page); > - else > - put_page(page); > + if (page_mapped(page) || !migrate_vma_check_page(page)) { > + if (!is_zone_device_page(page)) { > + get_page(page); > + putback_lru_page(page); > } > - } > - } > - > - for (i = 0, addr = start; i < npages && restore; i++, addr += PAGE_SIZE) { > - struct page *page = migrate_pfn_to_page(migrate->src[i]); > - > - if (!page || (migrate->src[i] & MIGRATE_PFN_MIGRATE)) > - continue; > > - remove_migration_pte(page, migrate->vma, addr, page); > - > - migrate->src[i] = 0; > - unlock_page(page); > - put_page(page); > - restore--; > - } > -} > - > -/* > - * migrate_vma_unmap() - replace page mapping with special migration pte entry > - * @migrate: migrate struct containing all migration information > - * > - * Replace page mapping (CPU page table pte) with a special migration pte entry > - * and check again if it has been pinned. Pinned pages are restored because we > - * cannot migrate them. > - * > - * This is the last step before we call the device driver callback to allocate > - * destination memory and copy contents of original page over to new page. > - */ > -static void migrate_vma_unmap(struct migrate_vma *migrate) > -{ > - const unsigned long npages = migrate->npages; > - const unsigned long start = migrate->start; > - unsigned long addr, i, restore = 0; > - > - for (i = 0; i < npages; i++) { > - struct page *page = migrate_pfn_to_page(migrate->src[i]); > - > - if (!page || !(migrate->src[i] & MIGRATE_PFN_MIGRATE)) > + migrate->src[i] &= ~MIGRATE_PFN_MIGRATE; > + migrate->cpages--; > + restore++; > continue; > - > - if (page_mapped(page)) { > - try_to_migrate(page, 0); > - if (page_mapped(page)) > - goto restore; > } > - > - if (migrate_vma_check_page(page)) > - continue; > - > -restore: > - migrate->src[i] &= ~MIGRATE_PFN_MIGRATE; > - migrate->cpages--; > - restore++; > } > > for (addr = start, i = 0; i < npages && restore; addr += PAGE_SIZE, i++) { > @@ -2672,12 +2589,8 @@ static void migrate_vma_unmap(struct migrate_vma *migrate) > > migrate->src[i] = 0; > unlock_page(page); > + put_page(page); > restore--; > - > - if (is_zone_device_page(page)) > - put_page(page); > - else > - putback_lru_page(page); > } > } > > @@ -2700,8 +2613,8 @@ static void migrate_vma_unmap(struct migrate_vma *migrate) > * it for all those entries (ie with MIGRATE_PFN_VALID and MIGRATE_PFN_MIGRATE > * flag set). Once these are allocated and copied, the caller must update each > * corresponding entry in the dst array with the pfn value of the destination > - * page and with the MIGRATE_PFN_VALID and MIGRATE_PFN_LOCKED flags set > - * (destination pages must have their struct pages locked, via lock_page()). > + * page and with MIGRATE_PFN_VALID. Destination pages must be locked via > + * lock_page(). > * > * Note that the caller does not have to migrate all the pages that are marked > * with MIGRATE_PFN_MIGRATE flag in src array unless this is a migration from > @@ -2770,8 +2683,6 @@ int migrate_vma_setup(struct migrate_vma *args) > > migrate_vma_collect(args); > > - if (args->cpages) > - migrate_vma_prepare(args); > if (args->cpages) > migrate_vma_unmap(args); >
| |