Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20220729024645.764366-1-apopple@nvidia.com>
 <YuQ4mJqjIUncf7iF@nvidia.com>
User-agent: mu4e 1.6.9; emacs 27.1
From:   Alistair Popple <apopple@nvidia.com>
To:     Jason Gunthorpe <jgg@nvidia.com>
Cc:     akpm@linux-foundation.org, minchan@kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, jhubbard@nvidia.com,
        pasha.tatashin@soleen.com
Subject: Re: [PATCH] mm/gup.c: Simplify and fix
 check_and_migrate_movable_pages() return codes
Date:   Mon, 01 Aug 2022 12:18:53 +1000
In-reply-to: <YuQ4mJqjIUncf7iF@nvidia.com>
Message-ID: <87wnbsd6yp.fsf@nvdebian.thelocal>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?n6nihQf7sMoRLouvH9okDAWwIRgIlZ9oBegbom5h12YMlRUaVidEGVj4VVaK?=
 =?us-ascii?Q?tEWB5913JiFWbnZbKjgiTtfVnVi0sscSB3GTVtzn1sLkvT9QQGittF49M7oZ?=
 =?us-ascii?Q?hL2AsiN4zgjdv5Mm9T3YefsZ1l+citA0XUevLu8j6CMT3di8+I5GHNAte5qn?=
 =?us-ascii?Q?Zq4RfxI00V7j8ypAC56H1X/389MXUii2kLdmteTANHYEdEZVEUGQbhqBFYkN?=
 =?us-ascii?Q?BoOyRy5DkOSdVr7gh/cNJPz1Atn5dXa4IlFkrBLg4w85vB6Zpu9dvKpnQcf4?=
 =?us-ascii?Q?muhzs5lyVo0ukABhECI4HjQyBMLXHo7g3ws3UfbTyTSekMMSCICCxtrFgt0i?=
 =?us-ascii?Q?SYFHrBXfjebfJJw5UsSBtAxKxDWyv9vQew0QtP8eDWva0QC6D0Irz2NZOM8N?=
 =?us-ascii?Q?YLQl1l26xrNZisYS+UE+0jiub2cFifVlEJa+Ou7vOWAoDomvMF5eU9ygo0MZ?=
 =?us-ascii?Q?oyy4lEx2FaJByuqxIgO+J/MzSeWeK6U1TQflDcHsS7HEpGczsNNVVOrMLIAy?=
 =?us-ascii?Q?zB6or2Z9Wik1Clu2+2nr3JHu3xIB98GwG6C0QBlWhV8zMYZ4zIQTXVIEe1JF?=
 =?us-ascii?Q?5M/CH+g1fW9bb6S0OfO8cs4YtcMvKbhVYWTN8kHl9zhnuID+gATrpEnI6NnU?=
 =?us-ascii?Q?NgDtz5UXLmB4AewI5z5W+6CXNGWZThECXEOWh7ba5KsFKE77L9YtCV9ll99y?=
 =?us-ascii?Q?4t0Zoss/rX30pI7SB7epM2TDlvMpWlbwRc/oyFORy45Mg+j0GNlJQRpjHT3I?=
 =?us-ascii?Q?ryWJAj5yiZI/aPkJxnDG0z1QR5ysVSxXBD6Vwuv1C0zI3kBWqEwgQo8U3ZcE?=
 =?us-ascii?Q?V2q4z8GLqC19dtouEahotaQypeTpjqPdHB8x+z31EBHTkBzjpzC7M+mdbVEb?=
 =?us-ascii?Q?c589OJl7Ksp6JXDNW9xBwz9NHUPAqEfZhuAMxVg/8PwAWlxwfx0PjU55Dop3?=
 =?us-ascii?Q?OsIbCAaNd5517rzHyQZjcwimYHA2BG68iHuoaFFWcutQOkGwfywREjGR1n6M?=
 =?us-ascii?Q?xB+gEp7B1RGMcgb9DMzAaOhTgs1PQEY1hI328pRfpN6Zu8idE70pg0OQxD89?=
 =?us-ascii?Q?UqZK3vYTIIUcdOXF6r+sU3e8D64DIeOnn04NxGxYToI8DrVju04tk83KChvm?=
 =?us-ascii?Q?Uan3CJAL9Z2UZI/dEkLOQ1s30xIrBtojf3HcSbWkHmeJm9DVM8TXBFuipOQR?=
 =?us-ascii?Q?Wiv5VDivwbiuCZqohuygPzGIfwRBt/H4sHRnWotPvGMRQkPI6XLXE5rtW5rC?=
 =?us-ascii?Q?Gcc8nMVsnxDFrHhcrqS0gVE6XxuO3u4zLv6JK05kF06Tm5LnTUT1h4GzVQ5D?=
 =?us-ascii?Q?CkoqjeaCKKqDEPmBhy8fzeWFp7S+ngYRYasuwuQexcNWugfppMhv6/raFQu8?=
 =?us-ascii?Q?ZBAYKJJZLt4Dikvnd5b5jG6Id9nLUlWcb72aVkNrc64f6ioHvz7irYAL1RFC?=
 =?us-ascii?Q?A1rHd0RDiFP7Mp4TtnQ5+La9p3cKMhwulX6o6vSjiqm1f2OEUy7jwb1400JB?=
 =?us-ascii?Q?SItRet54J3Sze1cYebcqHgZWP6eTLFD0cOTR5fvoOiInxND8CZqT+4tT0rID?=
 =?us-ascii?Q?pdfphtsFpHL9GKCdQRdUndEd/ECBvfubxWZPuqWT?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e7c38368-6f37-4a71-9dff-08da7366d79a
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Aug 2022 02:37:56.9314
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: mGz2sx6yONjaqLv0gRxgQm8MVGTY+YCeYpwNqjnyWVCdVJTJ/iFGGOrSVgIKaOUCpfK1WUm21ovMHciolAWhNg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5365
X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        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-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Jason Gunthorpe <jgg@nvidia.com> writes:

> On Fri, Jul 29, 2022 at 12:46:45PM +1000, Alistair Popple wrote:

[...]

> I have to say I prefer the usual style where all the places that error
> exit do 'goto error' instead of trying to keep track in 'ret'

Ok. Part of the complexity was my understanding from the documentation
for migrate_pages() is that putback_movable_pages() should only be
called if migrate_pages() != 0:

 * It is caller's responsibility to call putback_movable_pages() to return pages
 * to the LRU or free list only if ret != 0.

But I think it should be fine to do regardless, because on success the
pages will be deleted from movable_page_list. Eg. From unmap_and_move():

	if (rc != -EAGAIN) {
		/*
		 * A page that has been migrated has all references
		 * removed and will be freed. A page that has not been
		 * migrated will have kept its references and be restored.
		 */
		list_del(&page->lru);
	}

So will post a v2 doing this.

> AFAICT there is no reason to 'continue' in most of these paths since
> we intend to return to userspace with an error anyhow? Why try to
> isolate more pages?

The main reason would be if callers want to retry the operation. AFAIK
isolate_folio_lru() can have transient failures, so if callers want to
retry it makes sense to isolate and migrate as many pages as possible
rather than one page at a time as subsequent retries may find different
pages that can't be isolated.

Actually I should have called this out more clearly - the previous
behaviour on isolation failure was to retry indefinitely which is what
lead to looping in the kernel. This patch turns isolation failure into
an error and doesn't retry. I wonder though if we need to maintain a
retry count similar to what migrate_pages() does if there are unexpected
page refs?

>> @@ -1980,19 +1980,18 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>>  				    folio_nr_pages(folio));
>>  	}
>>
>> -	if (!list_empty(&movable_page_list) || isolation_error_count
>> -		|| coherent_pages)
>> -		goto unpin_pages;
>> -
>>  	/*
>>  	 * If list is empty, and no isolation errors, means that all pages are
>> -	 * in the correct zone.
>> +	 * in the correct zone. If there were device coherent pages some pages
>> +	 * have been unpinned.
>>  	 */
>
> That comment is a bit confusing.. I guess it is trying to explain why
> coherent_pages is doing?
>
> Maybe just:
>
> All the given pages are fine, nothing was done

Ok.

>> +	if (list_empty(&movable_page_list) && !ret && !coherent_pages)

Actually I think we can drop the coherent_pages variable too. At this
point coherent_pages will either be in the correct zone or we will have
jumped to the error label.

>> +		return 0;
>>
>> -unpin_pages:
>
> Now that this label is removed this if following it
>
> 	if (!list_empty(&movable_page_list)) {
>
> is also now unneeded because the above 'return 0' already checked it
>
> I came up with this ontop:

Thanks for the suggestions.

 - Alistair

> diff --git a/mm/gup.c b/mm/gup.c
> index 9e7c76d1e4ee3c..eddcf3c0eba727 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1912,11 +1912,15 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  					    struct page **pages,
>  					    unsigned int gup_flags)
>  {
> +	struct migration_target_control mtc = {
> +		.nid = NUMA_NO_NODE,
> +		.gfp_mask = GFP_USER | __GFP_NOWARN,
> +	};
>  	unsigned long i;
>  	struct folio *prev_folio = NULL;
>  	LIST_HEAD(movable_page_list);
>  	bool drain_allow = true, coherent_pages = false;
> -	int ret = 0;
> +	int ret = -EBUSY;
>
>  	for (i = 0; i < nr_pages; i++) {
>  		struct folio *folio = page_folio(pages[i]);
> @@ -1948,10 +1952,8 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  				unpin_user_page(&folio->page);
>  			}
>
> -			if (migrate_device_coherent_page(&folio->page)) {
> -				ret = -EBUSY;
> -				break;
> -			}
> +			if (migrate_device_coherent_page(&folio->page))
> +				goto error;
>  			continue;
>  		}
>
> @@ -1963,7 +1965,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  		if (folio_test_hugetlb(folio)) {
>  			if (isolate_hugetlb(&folio->page,
>  						&movable_page_list))
> -				ret = -EBUSY;
> +				goto error;
>  			continue;
>  		}
>
> @@ -1972,10 +1974,8 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  			drain_allow = false;
>  		}
>
> -		if (folio_isolate_lru(folio)) {
> -			ret = -EBUSY;
> -			continue;
> -		}
> +		if (folio_isolate_lru(folio))
> +			goto error;
>  		list_add_tail(&folio->lru, &movable_page_list);
>  		node_stat_mod_folio(folio,
>  				    NR_ISOLATED_ANON + folio_is_file_lru(folio),
> @@ -1987,7 +1987,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  	 * in the correct zone. If there were device coherent pages some pages
>  	 * have been unpinned.
>  	 */
> -	if (list_empty(&movable_page_list) && !ret && !coherent_pages)
> +	if (list_empty(&movable_page_list) && !coherent_pages)
>  		return 0;
>
>  	/*
> @@ -2005,23 +2005,19 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  			put_page(pages[i]);
>  	}
>
> -	if (!list_empty(&movable_page_list)) {
> -		struct migration_target_control mtc = {
> -			.nid = NUMA_NO_NODE,
> -			.gfp_mask = GFP_USER | __GFP_NOWARN,
> -		};
> -
> -		ret = migrate_pages(&movable_page_list, alloc_migration_target,
> -				    NULL, (unsigned long)&mtc, MIGRATE_SYNC,
> -				    MR_LONGTERM_PIN, NULL);
> -		if (ret > 0) /* number of pages not migrated */
> -			ret = -ENOMEM;
> +	not_migrated = migrate_pages(&movable_page_list, alloc_migration_target,
> +				     NULL, (unsigned long)&mtc, MIGRATE_SYNC,
> +				     MR_LONGTERM_PIN, NULL);
> +	if (not_migrated > 0) {
> +		ret = -ENOMEM;
> +		goto error;
>  	}
> +	return -EAGAIN;
>
> -	if (ret && !list_empty(&movable_page_list))
> +error:
> +	if (!list_empty(&movable_page_list))
>  		putback_movable_pages(&movable_page_list);
> -
> -	return ret ? ret : -EAGAIN;
> +	return ret;
>  }
>  #else
>  static long check_and_migrate_movable_pages(unsigned long nr_pages,