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: <165490039431.944052.12458624139225785964.stgit@omen>
 <bb120b4a-e6f6-de81-35f0-9803acf9b0be@redhat.com>
 <20220615155659.GA7684@nvidia.com>
 <cb7efb0f-5537-5ce4-7aec-bb10ea81d5de@redhat.com>
 <20220623142139.462a0841.alex.williamson@redhat.com>
 <20220623204712.GG4147@nvidia.com>
User-agent: mu4e 1.6.9; emacs 27.1
From:   Alistair Popple <apopple@nvidia.com>
To:     Jason Gunthorpe <jgg@nvidia.com>
Cc:     Alex Williamson <alex.williamson@redhat.com>,
        David Hildenbrand <david@redhat.com>,
        akpm@linux-foundation.org, minchan@kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, paulmck@kernel.org,
        jhubbard@nvidia.com, joaodias@google.com
Subject: Re: [PATCH] mm: Re-allow pinning of zero pfns
Date:   Fri, 24 Jun 2022 10:11:01 +1000
In-reply-to: <20220623204712.GG4147@nvidia.com>
Message-ID: <87a6a2zyxk.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?kvp8DHabdD6SFutCwRxIEuA7rszKrxw3PioxUSNsGTlK29pTOfVzjGcHqfhb?=
 =?us-ascii?Q?uIunKUF4IQZt/rvg/Tak59EcCulMHbW8INWXXXb7q0/ABzYUJphCl2rIiUvh?=
 =?us-ascii?Q?NRFAHFIz9uPrZ4Igg3yPTDPXdNah3tQExAhwvs1wpYT+hMGIHNqiAIXPgqR+?=
 =?us-ascii?Q?wfG6CFzbCOwvj8zapSiKUN+zkYNI1tLCxT528YU+BwK+no1C18lmlXGCjK+b?=
 =?us-ascii?Q?NU+/jQoEUJJYjWQ7lzd5qBxYb/ZhZEs3EF4iaK+3DDHZdqfUxKwk2+zpV0pg?=
 =?us-ascii?Q?YP6iMU2nCpv31izqDQ38WeU+WlQL6BhX7a8QIfRIhSKgRfl9dwMydj3DtVRQ?=
 =?us-ascii?Q?RieiE4IRy/vUlZoPNDEuficiJVDEv+QAonTMalaI0Lzr4G/ggk8lajiX3w6g?=
 =?us-ascii?Q?ulbI0Wq2N16wQcSnYSACo0z3J8nAAEseaxkeMP4UEetzfsmKpqubvwLWPM/w?=
 =?us-ascii?Q?UhypjIJM8l+uPtWCVZ3ufWapDUxqZ55iJQNr7fVhvxU5YwqC39DwOMg/Atvh?=
 =?us-ascii?Q?VaUwfGcwzUNGHQXJPptacAWn8aeSDWqRNlpBd/QqiUZfZ/25VHfB6PZfv0sO?=
 =?us-ascii?Q?TMHUHMkQ+zTO0MZGGCs2QBMOwFn9ioG1kPcPxnEHi6WV/ULIIBL4u4+wHX9f?=
 =?us-ascii?Q?4kpi/YImJQNLFw1Z8vKXzJ5r8wjr8TBRe4MOAm/9mFc9qUfjfXP8I/PY2KBY?=
 =?us-ascii?Q?MMHv27C04YErJRx9ai4sgS4giMyPS1RBwlJz+miktdQYpZX6f0Q1vnhDeBwn?=
 =?us-ascii?Q?lTzl23ErIENqDq3r4Kbx6PbghJyJDrq6Rt+h6njr4aBdDGEFa9Y1KnkPddGF?=
 =?us-ascii?Q?RFMCbJWA0t4IWfwDzmYyx+Fbl3SXECz8s3/sDE7LHn52sEG/wLYNTM48cNkk?=
 =?us-ascii?Q?Hsj5494j0LvzFcAfoomkY2gIt77c9jrTOwlxazknt4aiCgpz3In4E+mBhNMg?=
 =?us-ascii?Q?UMdns6X7wkWRdzikz5RjRGSkUXua1XW4UuXnP90RzsUbMxsdax80zzE8AUK/?=
 =?us-ascii?Q?gBlsOR9wDGo89ppzmqB5FebJJ7ChvriIJpG2Rder/dJkbtbaR5EigcbmVjSq?=
 =?us-ascii?Q?Zk5zMhpjKtEie0cmCtpMouEzraOKYXEHu2v6+MBEG8uzW2UmEVsHrMbgbWy+?=
 =?us-ascii?Q?AizBdotFK2UQtAkvqibkWwooMb/GuAnC1e8kDcmXvy+qkZj+75O7Y8jsMDwL?=
 =?us-ascii?Q?ANS0gjKQ7L3lIW/ik7QutTXb4U8qG/V+/aZD2KndX3Hv23cD7YemCjKSu5Ca?=
 =?us-ascii?Q?yFoxkERyP35Yi/CA57L1KDDtKz6yeo4Cm/RB1/fGlmoFxvOpGQDJErCiBmMi?=
 =?us-ascii?Q?7QSLfaMluav5vSUKbaj+HLC07W/o8sUZ73CpCOC6pwdOOjWEH3BMOzVLeniw?=
 =?us-ascii?Q?dTPiSF7wFh7U8W3htJpUdRdarLdhEakiWQwhGXWCiCkR9dlzeMa74Wahj8Xa?=
 =?us-ascii?Q?a2DbFIDUeu0fl4HflHW2+Nn83aCWNdezuW1g36jqHl05AcLyh0qiTFsPk2b/?=
 =?us-ascii?Q?Ehzs1V+z+zeGLNwaTPWJlMcgGjTnH/MWkREIjh1UuWjXiEzW9bgSAzVZnSBl?=
 =?us-ascii?Q?1N92DP60lQM1U3kWW4F3WRGizODU65vJckskgPnR?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a509fe0a-45d6-494d-f6c7-08da55789c2d
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Jun 2022 00:29:33.2915
 (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: iqEcVyF1SN+PEB4dxYgiGjzRh6PJx2w5OjAjcQOEjlNXUVDjP7pRoB/3vpC3gmrF/ubkid1oRTmgqe0vHC7H0A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR12MB6598
X-Spam-Status: No, score=-1.7 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,
        T_SCC_BODY_TEXT_LINE autolearn=no 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 Thu, Jun 23, 2022 at 02:21:39PM -0600, Alex Williamson wrote:
>
>> check_and_migrate_movable_pages() perpetually returns zero.  I believe
>> this is because folio_is_pinnable() previously returned true, and now
>> returns false.
>
> Indeed, it is a bug that check_and_migrate_movable_pages() returns
> 0 when it didn't do anything. It should return an error code.
>
> Hum.. Alistair, maybe you should look at this as well, I'm struggling
> alot to understand how it is safe to drop the reference on the page
> but hold a pointer to it on the movable_page_list - sure it was
> isolated - but why does that mean it won't be concurrently unmapped
> and freed?

folio_isolate_lru() takes a reference on the page so you're safe from it
being freed. If it gets unmapped it will be freed when the matching
putback_movable_pages() is called.

> Anyhow, it looks like the problem is the tortured logic in this
> function, what do you think about this:

At a glance it seems reasonable, although I fear it might conflict with
my changes for device coherent migration. Agree the whole
check_and_migrate_movable_pages() logic is pretty tortured though, and I
don't think I'm making it better so would be happy to try cleaning it up
futher once the device coherent changes are in.

> diff --git a/mm/gup.c b/mm/gup.c
> index 5512644076246d..2ffcb3f4ff4a7b 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1879,10 +1879,15 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  					    unsigned int gup_flags)
>  {
>  	unsigned long isolation_error_count = 0, i;
> +	struct migration_target_control mtc = {
> +		.nid = NUMA_NO_NODE,
> +		.gfp_mask = GFP_USER | __GFP_NOWARN,
> +	};
>  	struct folio *prev_folio = NULL;
>  	LIST_HEAD(movable_page_list);
>  	bool drain_allow = true;
> -	int ret = 0;
> +	int not_migrated;
> +	int ret;
>
>  	for (i = 0; i < nr_pages; i++) {
>  		struct folio *folio = page_folio(pages[i]);
> @@ -1919,16 +1924,13 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  				    folio_nr_pages(folio));
>  	}
>
> -	if (!list_empty(&movable_page_list) || isolation_error_count)
> -		goto unpin_pages;
> -
>  	/*
>  	 * If list is empty, and no isolation errors, means that all pages are
> -	 * in the correct zone.
> +	 * in the correct zone, nothing to do.
>  	 */
> -	return nr_pages;
> +	if (list_empty(&movable_page_list) && !isolation_error_count)
> +		return nr_pages;
>
> -unpin_pages:
>  	if (gup_flags & FOLL_PIN) {
>  		unpin_user_pages(pages, nr_pages);
>  	} else {
> @@ -1936,20 +1938,22 @@ 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,
> -		};
> +	if (isolation_error_count) {
> +		ret = -EINVAL;
> +		goto err_putback;
> +	}
>
> -		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 err_putback;
>  	}
> +	return 0;
>
> -	if (ret && !list_empty(&movable_page_list))
> +err_putback:
> +	if (!list_empty(&movable_page_list))
>  		putback_movable_pages(&movable_page_list);
>  	return ret;
>  }
>
>> If I generate an errno here, QEMU reports failing on the pc.rom memory
>> region at 0xc0000.  Thanks,
>
> Ah, a ROM region that is all zero'd makes some sense why it has gone
> unnoticed as a bug.
>
> Jason