Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
From:   Alistair Popple <apopple@nvidia.com>
To:     <akpm@linux-foundation.org>, Ralph Campbell <rcampbell@nvidia.com>
CC:     <kvm-ppc@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>,
        <Felix.Kuehling@amd.com>, <alexander.deucher@amd.com>,
        <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>,
        <dri-devel@lists.freedesktop.org>, <nouveau@lists.freedesktop.org>,
        <amd-gfx@lists.freedesktop.org>, <jglisse@redhat.com>,
        <jhubbard@nvidia.com>, <ziy@nvidia.com>, <hch@lst.de>,
        <bskeggs@redhat.com>
Subject: Re: [PATCH] mm/migrate.c: Remove MIGRATE_PFN_LOCKED
Date:   Thu, 28 Oct 2021 12:27:17 +1100
Message-ID: <2564177.V13TkMiDjn@nvdebian>
In-Reply-To: <f92e2dfe-f033-9b09-e83c-203052b491e1@nvidia.com>
References: <20211025041608.289017-1-apopple@nvidia.com> <f92e2dfe-f033-9b09-e83c-203052b491e1@nvidia.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Oct 2021 01:27:23.6187
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 872876ee-ae33-4e26-2a6b-08d999b21822
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT067.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB3782
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tuesday, 26 October 2021 11:57:06 AM AEDT Ralph Campbell wrote:
> 
> 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>

Thanks!

> >   
> >   		/*
> >   		 * 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.

I don't think it is an issue today because migrate_vma only supports private,
non-shared pages. At some point though it may be extended to support
file-backed pages and this would be easy to miss so will put a patch together.

> >   
> >   			/* 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);
> >   
>