Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <74dcaab4-7857-c181-44cf-f1c273843520@amd.com>
Date:   Fri, 15 Jul 2022 09:12:59 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH] mm/gup: migrate device coherent pages when pinning
 instead of failing
Content-Language: en-US
To:     Alistair Popple <apopple@nvidia.com>
Cc:     Matthew Wilcox <willy@infradead.org>, jgg@nvidia.com,
        Felix.Kuehling@amd.com, linux-mm@kvack.org, rcampbell@nvidia.com,
        linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org,
        amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        hch@lst.de, jglisse@redhat.com, akpm@linux-foundation.org
References: <4b2f9a61-ca67-6a34-41c9-c191cac756b3@redhat.com>
 <20220715021109.2225886-1-apopple@nvidia.com>
From:   "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com>
In-Reply-To: <20220715021109.2225886-1-apopple@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OUlQUFp1K2VYSys4OGo4QnN2alJsQ3B6WnFHWmpYWUVJbnhYYTNsaFBWRHpm?=
 =?utf-8?B?cmFwTS9Ydm5xb1hoL3hkUUpMUnpaYzdxU0syR2F5S1RoVE9DdTNhVWZEaGpy?=
 =?utf-8?B?c0Z4cUVEZTByNDhDd20wSnNWdWZ6TmdKT1ByQUYvZkZjMXZRRnpTVldoek14?=
 =?utf-8?B?MHBoUlZPTkNVby9KVHVNd2NoMVcrTjA5bDlETSt6Rk9GdWRYT2hJRUJyZk9s?=
 =?utf-8?B?eEw4SjZVRStsejgvMXlMQTFDMjZWQ2dIZ3RFZWttdVZyQnNCL08vZWxKeHc5?=
 =?utf-8?B?U2FPQVErZTZvRG94YThmbWIvWTBXRW5ibUtURlVxMnVoYjRWQzZ1LzhsN3lj?=
 =?utf-8?B?OEs0ZjYxOVZUMngwbUtsTWVVVVV4Skx1U0VwZURoWTdHY3VISXg5WlpsbzVQ?=
 =?utf-8?B?ZmcyQ1ZvYlN6aXVGZm45VHdjZm83VExxRUQ4YmlMeUJJWGhvR0l2UVpydUVv?=
 =?utf-8?B?NHZtRm1ma3dNeTlrWnhBK2haZzloN1JFQXFuUThhN25TcHdYbkVVWkNTd3Qr?=
 =?utf-8?B?N0F0SHVXWFJ2anc2cnAvTkE3WktGN0VKQUNVMnNKY0hFOHF6bHd3Nm15Nloy?=
 =?utf-8?B?cnpRSWpwcVpHQ2JNQTdSeVU2Y2RDU09Pd2x3VXhJTTRQM21McTRwbXUxTlNk?=
 =?utf-8?B?ZjI1UlpualM0VHJkTCsxZGIxWG1yc3QrZ25aQTBoUDczS1pSRzY4azUzMHFr?=
 =?utf-8?B?SEwyS3MvZElVRmlsM2xpNG0wTHQ2Q1FmbWUwazdsMTVCMUV1Wm9SakVwcjNF?=
 =?utf-8?B?UWFzT3oxMjlBTmU5bHJmNFNpK0ZuTy9OMEFKc0QzcDg5OEhjL3NOcGJtY1dv?=
 =?utf-8?B?WjNhTUFUTFVTUk5QUWlUbkpQbWJVRE1xYzBlS0ZoRmkwNXdVdmtqR3dtVGUr?=
 =?utf-8?B?OHNOWnMwb3JIWFZUTHZMUFA5c1NNWW5IbENDZHlZMHhTOXJDMW9vSFMzQlMx?=
 =?utf-8?B?VldhSGFDeTlablNJSkxDM1UyMjI3b3hPdmdwSFpBWE1udEN3eVlDYWpZTS9S?=
 =?utf-8?B?eFJwVWRQZmg4aXZIUDFrN0hJRGM2QnNjY1JMdkV1WHJzbFd3U0xraTQyMklP?=
 =?utf-8?B?aldYWUdSNDNtOWsya2FZcG1UVlRtSDBHbEEyNXRER0kyY2JQZkVGc2NDbGxC?=
 =?utf-8?B?clY3WDQrSkd6MGdFTnB4Sk1UOE9IU3M5eVpWK0lTcHNIZTNzTVFVMVVqbmFG?=
 =?utf-8?B?czdTZmZVRXd4RjZaQUpCdXh2YnE0SHFNVHpOR01LU2lkZThPT2grc1pEQXZC?=
 =?utf-8?B?Yjk1OU1OcmpHTUYwTkwreEp0Z3FMY2dxRElIVkxLbldCODArRnVRSjE3QnFj?=
 =?utf-8?B?Z2VsdG9JZUY4NWt6ZWpmZ0F5aDdnWXM5UmNaQy9GaHhRLzRHYUkwc0xlNVAw?=
 =?utf-8?B?VTlHbDlyUjRLS1RNczNIdHhoVU0zZ0kwbGZNekt1SittR2VmWmVMS3B2cGFq?=
 =?utf-8?B?M1AwSEhuZ28vdU9iSDJMKy9SWDZ6dVNLS01OUHIveUh0MmM1OWROWUl3ZXda?=
 =?utf-8?B?R0lMOVNyNHphZ2pxN1YrekFSQTl4b0d0NFptNzZ1Qi9GSHByWFJxVnNkUlcr?=
 =?utf-8?B?VHdjcjJUQUZycFdzeTA2bmJoUjY3S1FucmJXZS9JcFZsLzV4a2VoaE9XT1o4?=
 =?utf-8?B?NC9GL04zV0RURXpJL0l5cVFYYjllMXFUV0NTOE14V201ekRJQlpJcDdmd2Yw?=
 =?utf-8?B?TDVNd0diRXRCNWkwRlE4c0tBMVI1cTJUUVZwRzN0RnZ1QWNCdzRnTUF4MEtO?=
 =?utf-8?B?aEtVc0luQzAzMEJTM2RHeFltOS9SRWRxemRxN2lWaFF1TG9XNzYyVHJDNHBu?=
 =?utf-8?B?blFlVzVxbmhVWXZwWTArc1BNMyt1SmZUN3pHRjRGTTBTcmpZbVdBQTVzNWhx?=
 =?utf-8?B?RXRRQngraEp3SXh6bytaMEo5QmxMN1hLbXZUaEpYbVhGSHdPRFVMZU9QVFFw?=
 =?utf-8?B?QytaOGU1SmZQaERnNUlaNGgyQXNvMHpHOHVjc0tMR2VBN2UwVmRQRFVKQm93?=
 =?utf-8?B?MjZTY2NJa1RlQlB1TFpQUWppZDk0RHJha2FkR28wckhYUHhjRnFDZVB0SEV2?=
 =?utf-8?B?OEdJOUtMalBxSUZTSHN5YnpwbkFJM1ZneVVubHo0eHg1TFNoOGVROS9PUGJs?=
 =?utf-8?Q?C4TTch/PpGOSL62mpsHMzfnhK?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dfffa335-9f42-4f1e-a3bc-08da666c2158
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2717.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Jul 2022 14:13:03.0023
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: UhUxWggwswgipSfw3oz+YAg6tNY5+0nB7II23dH5bdIE3b60yNeATe4PV5JI09TAPug6Tq5ezfN9um7kMWXHlg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB5610
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org


On 7/14/2022 9:11 PM, Alistair Popple wrote:
> Currently any attempts to pin a device coherent page will fail. This is
> because device coherent pages need to be managed by a device driver, and
> pinning them would prevent a driver from migrating them off the device.
>
> However this is no reason to fail pinning of these pages. These are
> coherent and accessible from the CPU so can be migrated just like
> pinning ZONE_MOVABLE pages. So instead of failing all attempts to pin
> them first try migrating them out of ZONE_DEVICE.
>
> [hch: rebased to the split device memory checks,
>        moved migrate_device_page to migrate_device.c]
>
> Signed-off-by: Alistair Popple <apopple@nvidia.com>
> Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>
> This patch hopefully addresses all of David's comments. It replaces both my "mm:
> remove the vma check in migrate_vma_setup()" and "mm/gup: migrate device
> coherent pages when pinning instead of failing" patches. I'm not sure what the
> best way of including this is, perhaps Alex can respin the series with this
> patch instead?

For sure Alistair. I'll include this in my next patch series version.

Thanks,
Alex Sierra

>
>   - Alistair
>
>   mm/gup.c            | 50 +++++++++++++++++++++++++++++++++++++------
>   mm/internal.h       |  1 +
>   mm/migrate_device.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 96 insertions(+), 7 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index b65fe8bf5af4..22b97ab61cd9 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1881,7 +1881,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>   	unsigned long isolation_error_count = 0, i;
>   	struct folio *prev_folio = NULL;
>   	LIST_HEAD(movable_page_list);
> -	bool drain_allow = true;
> +	bool drain_allow = true, coherent_pages = false;
>   	int ret = 0;
>   
>   	for (i = 0; i < nr_pages; i++) {
> @@ -1891,9 +1891,38 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>   			continue;
>   		prev_folio = folio;
>   
> -		if (folio_is_longterm_pinnable(folio))
> +		/*
> +		 * Device coherent pages are managed by a driver and should not
> +		 * be pinned indefinitely as it prevents the driver moving the
> +		 * page. So when trying to pin with FOLL_LONGTERM instead try
> +		 * to migrate the page out of device memory.
> +		 */
> +		if (folio_is_device_coherent(folio)) {
> +			/*
> +			 * We always want a new GUP lookup with device coherent
> +			 * pages.
> +			 */
> +			pages[i] = 0;
> +			coherent_pages = true;
> +
> +			/*
> +			 * Migration will fail if the page is pinned, so convert
> +			 * the pin on the source page to a normal reference.
> +			 */
> +			if (gup_flags & FOLL_PIN) {
> +				get_page(&folio->page);
> +				unpin_user_page(&folio->page);
> +			}
> +
> +			ret = migrate_device_coherent_page(&folio->page);
> +			if (ret)
> +				goto unpin_pages;
> +
>   			continue;
> +		}
>   
> +		if (folio_is_longterm_pinnable(folio))
> +			continue;
>   		/*
>   		 * Try to move out any movable page before pinning the range.
>   		 */
> @@ -1919,7 +1948,8 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>   				    folio_nr_pages(folio));
>   	}
>   
> -	if (!list_empty(&movable_page_list) || isolation_error_count)
> +	if (!list_empty(&movable_page_list) || isolation_error_count
> +		|| coherent_pages)
>   		goto unpin_pages;
>   
>   	/*
> @@ -1929,10 +1959,16 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>   	return nr_pages;
>   
>   unpin_pages:
> -	if (gup_flags & FOLL_PIN) {
> -		unpin_user_pages(pages, nr_pages);
> -	} else {
> -		for (i = 0; i < nr_pages; i++)
> +	/*
> +	 * pages[i] might be NULL if any device coherent pages were found.
> +	 */
> +	for (i = 0; i < nr_pages; i++) {
> +		if (!pages[i])
> +			continue;
> +
> +		if (gup_flags & FOLL_PIN)
> +			unpin_user_page(pages[i]);
> +		else
>   			put_page(pages[i]);
>   	}
>   
> diff --git a/mm/internal.h b/mm/internal.h
> index c0f8fbe0445b..899dab512c5a 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -853,6 +853,7 @@ int numa_migrate_prep(struct page *page, struct vm_area_struct *vma,
>   		      unsigned long addr, int page_nid, int *flags);
>   
>   void free_zone_device_page(struct page *page);
> +int migrate_device_coherent_page(struct page *page);
>   
>   /*
>    * mm/gup.c
> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index 18bc6483f63a..7feeb447e3b9 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -686,6 +686,12 @@ void migrate_vma_pages(struct migrate_vma *migrate)
>   		}
>   
>   		if (!page) {
> +			/*
> +			 * The only time there is no vma is when called from
> +			 * migrate_device_coherent_page(). However this isn't
> +			 * called if the page could not be unmapped.
> +			 */
> +			VM_BUG_ON(!migrate->vma);
>   			if (!(migrate->src[i] & MIGRATE_PFN_MIGRATE))
>   				continue;
>   			if (!notified) {
> @@ -794,3 +800,49 @@ void migrate_vma_finalize(struct migrate_vma *migrate)
>   	}
>   }
>   EXPORT_SYMBOL(migrate_vma_finalize);
> +
> +/*
> + * Migrate a device coherent page back to normal memory. The caller should have
> + * a reference on page which will be copied to the new page if migration is
> + * successful or dropped on failure.
> + */
> +int migrate_device_coherent_page(struct page *page)
> +{
> +	unsigned long src_pfn, dst_pfn = 0;
> +	struct migrate_vma args;
> +	struct page *dpage;
> +
> +	WARN_ON_ONCE(PageCompound(page));
> +
> +	lock_page(page);
> +	src_pfn = migrate_pfn(page_to_pfn(page)) | MIGRATE_PFN_MIGRATE;
> +	args.src = &src_pfn;
> +	args.dst = &dst_pfn;
> +	args.cpages = 1;
> +	args.npages = 1;
> +	args.vma = NULL;
> +
> +	/*
> +	 * We don't have a VMA and don't need to walk the page tables to find
> +	 * the source page. So call migrate_vma_unmap() directly to unmap the
> +	 * page as migrate_vma_setup() will fail if args.vma == NULL.
> +	 */
> +	migrate_vma_unmap(&args);
> +	if (!(src_pfn & MIGRATE_PFN_MIGRATE))
> +		return -EBUSY;
> +
> +	dpage = alloc_page(GFP_USER | __GFP_NOWARN);
> +	if (dpage) {
> +		lock_page(dpage);
> +		dst_pfn = migrate_pfn(page_to_pfn(dpage));
> +	}
> +
> +	migrate_vma_pages(&args);
> +	if (src_pfn & MIGRATE_PFN_MIGRATE)
> +		copy_highpage(dpage, page);
> +	migrate_vma_finalize(&args);
> +
> +	if (src_pfn & MIGRATE_PFN_MIGRATE)
> +		return 0;
> +	return -EBUSY;
> +}