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: <fca6bb0bd48a0292a0ace2fadd0f44579a060cbb.1666335603.git.baolin.wang@linux.alibaba.com>
 <87pmeiq6qc.fsf@nvidia.com>
 <4fcb9065-0d0c-7bd3-d4f9-5830792cce60@linux.alibaba.com>
 <87lep5r7t9.fsf@nvidia.com>
 <f2cc7bb4-f296-766e-16b5-0d70498669eb@linux.alibaba.com>
User-agent: mu4e 1.8.10; emacs 28.2
From:   Alistair Popple <apopple@nvidia.com>
To:     Baolin Wang <baolin.wang@linux.alibaba.com>
Cc:     akpm@linux-foundation.org, david@redhat.com, ying.huang@intel.com,
        ziy@nvidia.com, shy828301@gmail.com, jingshan@linux.alibaba.com,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] mm: migrate: Fix return value if all subpages of
 THPs are migrated successfully
Date:   Mon, 24 Oct 2022 19:06:57 +1100
In-reply-to: <f2cc7bb4-f296-766e-16b5-0d70498669eb@linux.alibaba.com>
Message-ID: <87h6ztr6f9.fsf@nvidia.com>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ne4KPCexsU1JnkY43nUmaUYkox1cXYsG8VyKCpiPUsbJh+XbnkvaSMJEHU1R?=
 =?us-ascii?Q?IXErwSH1vC38OQnKhR5n55AcbOgDYwpxb1zi5DSExROgOvZKI7T8ThzT1feY?=
 =?us-ascii?Q?rdw1tjaaewBc2OyFTm9Chlalp45VCtHnAtI6a8ydX7HwCc6G6LiWQQgOi6XX?=
 =?us-ascii?Q?kjUM+Lpr9o2+JbDBno+lzaMclrDKOHwStwcVHPUUP6F9S/Neh15PkQjUgS/X?=
 =?us-ascii?Q?bCuxD89H9BAMG4ofI9HZcU0PDsvRm6qJU2JviahvNGr0mkBUTBrkdGYsKFb0?=
 =?us-ascii?Q?rj18tkexvroRtd2Bp1hHyyMbQIg36m2pKYMQpw16KHT3BbHMpz639MB+ItxC?=
 =?us-ascii?Q?lF6dFBLEGkq1iWG2PepCLvA8wACFinpyVkDOz93qDykRWB9aMmXprOBXRVno?=
 =?us-ascii?Q?kbQFqt80dm8mgGzxMIw7j/Wf5ogIbSr+Mskr8WEGqYZHr8/rzEBDn9+YRyIK?=
 =?us-ascii?Q?uXcThMrrzn6G3yAy0KggKJoYsznEiQT9UVYm8ANf3TB+HoM+NBLjUoMoxm+L?=
 =?us-ascii?Q?Sg4ATZjIHfTlHmirkPkS3DqHdrubZvAFdFz5A15Xu6+TjRpEgByY6eI0lS7+?=
 =?us-ascii?Q?5KZu+9DG5y6raFkTFB71DQPmYN7/lz2kSEm4kZP+pILAwMbNO8OHQYe3375Q?=
 =?us-ascii?Q?Ysvb85987LayWKwJ4QEARDZ+QU33qqIHnji1bOHC06rufexnMtdgY1zViXdh?=
 =?us-ascii?Q?lD1hiiJcoZTgnH0+QR8Eqncme16YTtjDSoErZTD8hVkHSN6rqQ7PJmsCmv0F?=
 =?us-ascii?Q?qI6nSDfoHVwRoMPJV6G52990h57A/QBjJ/GLVNtzdyuqkDRYIRxrlf5gjx/D?=
 =?us-ascii?Q?g+j5iIsp1tdkpsADW749y3ZX+ktmz5MN6jTUv+tKtswiBP23cyD39wN5Bo7F?=
 =?us-ascii?Q?3TIep2xH+uI3ynK67qecPA3IlcJ4MT318jj/YkkH/O7Q1S/a0DEayypHgoia?=
 =?us-ascii?Q?0hpb2rxOXyh4A8IMPOUTkTYdTM7Vd2+YmiIG40F1NI5zrxAu8GLNCQPlCf5l?=
 =?us-ascii?Q?ViA0VTGsv3DFBr8xz9rdcLAU11AzIYCfCeRp4Zt/vJYS9e9XUKmW8qHwHgei?=
 =?us-ascii?Q?BXpY83djwj4o7sKUzpfTXAruJCwxOmq8Gj+1HyZvhIuKgUDp9MuM14kxCenn?=
 =?us-ascii?Q?u+OuSQbb9lKLtNe3M99vFMr0iJ39UhbDDeyUh5fxSOF29PrMxGNgFrzMUifq?=
 =?us-ascii?Q?0Aaq+YIH/q4rDYuUHc7rNQZRHAvyxFJxEds5k+ktMhHALL/LRNkJotG5PMQr?=
 =?us-ascii?Q?0EwaHjnZsnmJi91XBuwACJJN8ncgkTv5gQ/UfpxMdaXveL26nRB60fOafkZl?=
 =?us-ascii?Q?kPs2kMi5paIL1jfZoUK5KgzsLm/5744+Tlo6VVNUL4fk+Eoa4YCMCcyaaHqJ?=
 =?us-ascii?Q?3uQy+b/Y49gsf2pwSmrsgub53cxgwWUcMJsLVGaGuEOZP397WOX/9xOgYqom?=
 =?us-ascii?Q?plYIycDiC5X6Cy8PLDe9wCen+2x9NcYgWgg+Hzzzwsxheh6NHRN4aomAIIiZ?=
 =?us-ascii?Q?rpeQ5ihXL7qgdX30mfZD/fkRiunnWjdYKnBELZGk+q6HERHFVRg3D9rvlUko?=
 =?us-ascii?Q?YNAJSlRvJAPaP8kZ+eQ+NEgxE+UsfrE0OpZ/QPAr?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d7507185-8445-4736-140b-08dab596c9e9
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Oct 2022 08:07:26.4969
 (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: FDZO0PN44chhJ3HdpFBRm60jRi61dnTd1j1/h2DPmpTy3NK6jTKpdxsjioFf6uIF2LvvP1AB4CHariMWDYxOmQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR12MB6126
X-Spam-Status: No, score=-1.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=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


Baolin Wang <baolin.wang@linux.alibaba.com> writes:

> On 10/24/2022 3:24 PM, Alistair Popple wrote:
>> Baolin Wang <baolin.wang@linux.alibaba.com> writes:
>>
>>> On 10/24/2022 10:36 AM, Alistair Popple wrote:
>>>> Baolin Wang <baolin.wang@linux.alibaba.com> writes:
>>>>
>>>>> When THP migration, if THPs are split and all subpages are migrated successfully
>>>>> , the migrate_pages() will still return the number of THP that were not migrated.
>>>>> That will confuse the callers of migrate_pages(), for example, which will make
>>>>> the longterm pinning failed though all pages are migrated successfully.
>>>>>
>>>>> Thus we should return 0 to indicate all pages are migrated in this case.
>>>>>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>>>> ---
>>>>> Changes from v1:
>>>>> - Fix the return value of migrate_pages() instead of fixing the
>>>>>     callers' validation.
>>>>> ---
>>>>>    mm/migrate.c | 7 +++++++
>>>>>    1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>> index 8e5eb6e..1da0dbc 100644
>>>>> --- a/mm/migrate.c
>>>>> +++ b/mm/migrate.c
>>>>> @@ -1582,6 +1582,13 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>>    	 */
>>>>>    	list_splice(&ret_pages, from);
>>>>>
>>>>> +	/*
>>>>> +	 * Return 0 in case all subpages of fail-to-migrate THPs are
>>>>> +	 * migrated successfully.
>>>>> +	 */
>>>>> +	if (nr_thp_split && list_empty(from))
>>>>> +		rc = 0;
>>>> Why do you need to check nr_thp_split? Wouldn't list_empty(from) == True
>>>
>>> Only in the case of THP split, we can meet this abnormal case. So if no THP
>>> split, just return the original 'rc' instead of validating the list, since the
>>> 'nr_thp_split' validation is cheaper than the list_empty() validation IMHO.
>> Is it really that much cheaper? We're already retrying migrations
>> multiple times, etc. so surely the difference here would be marginal at
>> best, and IMHO the code would be much clearer if we always set rc = 0
>> when list_empty(from) = True.
>
> Yeah, the difference is marginal and I have no strong preference. OK, will drop
> the 'nr_thp_split' in next version. Thanks.

Thanks. With that change feel free to add:

Reviewed-by: Alistair Popple <apopple@nvidia.com>

>>>> imply success? And if it doesn't imply success wouldn't it be possible
>>>> to end up with nr_thp_split && list_empty(from) whilst still having
>>>> pages that failed to migrate?
>>>> The list management and return code logic from unmap_and_move() has
>>>> gotten pretty difficult to follow and could do with some rework IMHO.
>>>
>>> Yes, Huang Ying has sent a RFC patchset[1] doing some code refactor, which seems
>>> a good start.
>> Thanks for pointing that out, I looked at it a while back but missed the
>> clean ups. I was kind of waiting for the non-RFC version before taking
>> another closer look.
>>
>>> [1] https://lore.kernel.org/all/20220921060616.73086-1-ying.huang@intel.com/