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: <487960bf67c7273ff5606c76f73bb51271bc7b90.1660269441.git-series.apopple@nvidia.com>
 <6b61e9bf7c65f78524db32ba3e65a7eb6b8a76a0.1660269441.git-series.apopple@nvidia.com>
 <YvpxOyrDBUHagpC6@nvidia.com>
User-agent: mu4e 1.6.9; emacs 27.1
From:   Alistair Popple <apopple@nvidia.com>
To:     Jason Gunthorpe <jgg@nvidia.com>
Cc:     linux-mm@kvack.org, akpm@linux-foundation.org,
        linux-kernel@vger.kernel.org,
        "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com>,
        Chaitanya Kulkarni <kch@nvidia.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Felix Kuehling <Felix.Kuehling@amd.com>,
        John Hubbard <jhubbard@nvidia.com>,
        Logan Gunthorpe <logang@deltatee.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Ralph Campbell <rcampbell@nvidia.com>,
        David Hildenbrand <david@redhat.com>,
        Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH v4 2/2] mm/gup.c: Refactor
 check_and_migrate_movable_pages()
Date:   Tue, 16 Aug 2022 15:29:20 +1000
In-reply-to: <YvpxOyrDBUHagpC6@nvidia.com>
Message-ID: <87edxgvixy.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?zsqrawdnXk6YmWZRsgO9I6nrlcuCieCoL5/QMYbNzCTgv9QMX4IerA88/jwR?=
 =?us-ascii?Q?mXRs7hOSUM4oHBUumeXEOt3+YG8m+sq95EDWhhiqyEfC9Plf/2gZ7s4z7sHs?=
 =?us-ascii?Q?C34+Va8U1hR7RXJFCbzvgAISgumvbrJ+DVYryikXrOmltqt2SALGrPf7lNip?=
 =?us-ascii?Q?xbvjn8Ah756p31E2xjPi/JsQT8MivX/8LwmQC3uuqN0byODAKx2SEhJHAF7A?=
 =?us-ascii?Q?jAElYPMVNcyXTqqfwj/BP3fAMRx3X11/yQsb/XNKLoi+NlkU1Piu50frUeeX?=
 =?us-ascii?Q?zLFTULaUhH8Lr9DWH5YSA/bDn92/NNPPdi60fMtC946EVc6hwRZReM9OzZ55?=
 =?us-ascii?Q?9uXcjyuQo3uTENC3oR2Qazr+nAh0DYg97fzEp5ZTBj1feUX5pcCTKm8DV68a?=
 =?us-ascii?Q?DsFPsWo8x/SfZFr0TYvcWwvMbUpCrVXWy3kxTEYRsxYNq5WzHaNa8kcS1Ifo?=
 =?us-ascii?Q?NC+bDhsOCn1bPNoVPE3bqIwpzi4bqhtAgAB9POvS4JvgMp8cmV5R57UNrFp8?=
 =?us-ascii?Q?o9GYqkFbrLB90jzyx6aAKZwVCGwOkLjI3oi/vuiZz9BvjOhsFAQbwJ+7+nas?=
 =?us-ascii?Q?++1ApoeXzXCNpnJQX0r+SAiHAb1Zwnf8eK9xcifxK6aVdzrsRrluR2HZHcT2?=
 =?us-ascii?Q?H+f5UvlxEhzaYzJEmeUnA+/H7iTiPvqEBgrZAD0d4fzmIx8zdr/RrFj0//Is?=
 =?us-ascii?Q?bNWVthFDOcYHM0WOWXv1K5XARTWHTFULtA3ZcHGh2c+3v4rc6Y+VXSVRHQKJ?=
 =?us-ascii?Q?3TWHhZxeu7t0KRb6/ag/kOy7HvRcYrIysJCDjhrgsvG95q/VlrzGxgvJowx1?=
 =?us-ascii?Q?Q1xZhHBU1PLugdR2aT/skNLk11WYWWLwOQ6DucQUxnCCRdXzIPTrvpZ51jtr?=
 =?us-ascii?Q?LvocTA/1HPAKa+enngSkXuAYwRSnO1qcpNAf3pNpiGgfsVyPVaFzvFkUgTd0?=
 =?us-ascii?Q?OAFuGK7byaO0Sy8tLDu2nQYoQWgl1//zcqBlMC79e315WQJI6I3b5i3q08K8?=
 =?us-ascii?Q?X616Zd2nojRjtUe8eTjiKCp6bMy0bBLK0Lx8i9MnIHRgwsENFTONDnqXx9r8?=
 =?us-ascii?Q?+AiUYMJQ2LQWolbckNcdFvFxUndF4RDL3OJ7IwF4an19dv6aoejLRRLRmh7B?=
 =?us-ascii?Q?S6sdMTahqF1ZWIGTo6ltIAXv1fKpk5AjUa0vxG95CW2MqJF6KAr+3xf+noC8?=
 =?us-ascii?Q?BAkO7gYRS/oFmnnixsnkh4FoMY9AVfo4sYfm9LoMSHz5quJukYu+3h5O10vD?=
 =?us-ascii?Q?oEyGk83b+PiyuEHDXj/JNDpvT7oY94QTIp37Qmf0H+fl5eC3vYs8Prv5FzTH?=
 =?us-ascii?Q?7u9tFZc/e0n9r5cujyyccdmpz4sxW28iHqQORbmUBF+oJKeY6zsXCbX5g+A5?=
 =?us-ascii?Q?AwUZqAat/XtlErgev9kAOF0bW6gcjfMqy57cvreYdbkKeP+WfAkubA42AEeI?=
 =?us-ascii?Q?7rosivnp73aDTACQOWMXkTZcSgvFrQra8YwHJDwklxasVY8iMC2jx5mGlwyH?=
 =?us-ascii?Q?7sch870Igqx0hXBu35AI69vAOYqG6aWhX0N2VaXW8DBeO7fJ5ThGMV2aMskb?=
 =?us-ascii?Q?He0x4Jux22gdVpnyywiI3pOX5hC1rcPMQ95WYP11?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 993b42ed-8ce7-40aa-fed1-08da7f4ac150
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Aug 2022 05:47:07.7089
 (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: AEdcbINCF+/3NQqNbMTu/DTmflL8rYqPcJrExiGcEwfQGn6sCQ1+l4Iul09XAOMs3GfXv/VI1WbHCwsakXMsVQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR12MB1143
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        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 Fri, Aug 12, 2022 at 12:13:09PM +1000, Alistair Popple wrote:
>> When pinning pages with FOLL_LONGTERM check_and_migrate_movable_pages()
>> is called to migrate pages out of zones which should not contain any
>> longterm pinned pages.
>>
>> When migration succeeds all pages will have been unpinned so pinning
>> needs to be retried. Migration can also fail, in which case the pages
>> will also have been unpinned but the operation should not be retried. If
>> all pages are in the correct zone nothing will be unpinned and no retry
>> is required.
>>
>> The logic in check_and_migrate_movable_pages() tracks unnecessary state
>> and the return codes for each case are difficult to follow. Refactor the
>> code to clean this up. No behaviour change is intended.
>>
>> Signed-off-by: Alistair Popple <apopple@nvidia.com>
>>
>> ---
>
> This seems like the cleanest version yet!

Thanks, the feedback from John and yourself has been very useful!

>> +static long check_and_migrate_movable_pages(unsigned long nr_pages,
>> +					    struct page **pages)
>> +{
>> +	int ret;
>> +	unsigned long collected;
>> +	LIST_HEAD(movable_page_list);
>> +
>> +	collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
>> +	if (!collected)
>> +		return 0;
>> +
>> +	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
>> +	if (!ret)
>> +		return -EAGAIN;
>> +	else
>> +		return ret;
>
> I would drop the else path and just return zero

Unless I've misunderstood you I don't think that's correct though.
check_and_migrate_movable_pages() needs to return one of 3 conditions:

 - 0: All pages are in the correct zone and are still pinned (ie. "success").
 - -EAGAIN: Some pages were migrated, all pages need re-pinning.
 - errno: Migration failed, pins were dropped and PUP should fail.

John's suggested comment update spells this out more clearly.

> Arguably migrate_longterm_unpinnable_pages() should do the same?

migrate_longterm_unpinnable_pages() returns 0 for success, errno for
some kind of "permanent" failure that needs propagating.

>> @@ -2051,10 +2079,10 @@ static long __gup_longterm_locked(struct mm_struct *mm,
>>  			break;
>>
>>  		rc = check_and_migrate_movable_pages(rc, pages);
>> -	} while (!rc);
>> +	} while (rc == -EAGAIN);
>
> Since the only reader only cares about errno or not errno..

See above. Errors other than -EAGAIN should not be retried.

> But no biggie either way

So will leave this as for now at least.

> Thanks,
> Jason