| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f92e2dfe-f033-9b09-e83c-203052b491e1@nvidia.com>
Date: Mon, 25 Oct 2021 17:57:06 -0700
From: Ralph Campbell <rcampbell@...dia.com>
To: Alistair Popple <apopple@...dia.com>, akpm@...ux-foundation.org
Cc: kvm-ppc@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
Felix.Kuehling@....com, alexander.deucher@....com,
linux-mm@...ck.org, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
amd-gfx@...ts.freedesktop.org, jglisse@...hat.com,
jhubbard@...dia.com, ziy@...dia.com, hch@....de, bskeggs@...hat.com
Subject: Re: [PATCH] mm/migrate.c: Remove MIGRATE_PFN_LOCKED
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@...dia.com>
You can add:
Reviewed-by: Ralph Campbell <rcampbell@...dia.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);
>
Powered by blists - more mailing lists