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: <20221227002859.27740-1-ying.huang@intel.com>
 <20221227002859.27740-3-ying.huang@intel.com> <87pmbttxmj.fsf@nvidia.com>
 <87pmbtedfp.fsf@yhuang6-desk2.ccr.corp.intel.com>
 <877cy1scg5.fsf@nvidia.com>
 <87k021bfpj.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-agent: mu4e 1.8.10; emacs 28.2
From:   Alistair Popple <apopple@nvidia.com>
To:     "Huang, Ying" <ying.huang@intel.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Zi Yan <ziy@nvidia.com>,
        Yang Shi <shy828301@gmail.com>,
        Baolin Wang <baolin.wang@linux.alibaba.com>,
        Oscar Salvador <osalvador@suse.de>,
        Matthew Wilcox <willy@infradead.org>,
        Bharata B Rao <bharata@amd.com>,
        haoxin <xhao@linux.alibaba.com>
Subject: Re: [PATCH 2/8] migrate_pages: separate hugetlb folios migration
Date:   Thu, 05 Jan 2023 18:39:59 +1100
In-reply-to: <87k021bfpj.fsf@yhuang6-desk2.ccr.corp.intel.com>
Message-ID: <87y1qhqv5k.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?b5DETm04At8LHjTRFkEeP0dFZA6CgK1UJucYAYTPaQ+y7SeREUzJKgkkMqKz?=
 =?us-ascii?Q?6dI5fpKdARG5kOnNJ+buJOfEAAP8b7N9T1PQQhhqZSnrZ8ng93PrjatqISqW?=
 =?us-ascii?Q?fiLHJBJwpN3qggl//E2KZlRkK3tOndEqjvDJ179Ym2HZt/ZslvRkkcoUzqHO?=
 =?us-ascii?Q?xIA1L2D9JRlCbKgAZFeAuqqokdp2WWPs/1qcyyqTYjnbdEAjfeWocODneH+Y?=
 =?us-ascii?Q?a7cf5db4ccZ9UZUVD3DOFDsi0rcg0Olm4URh1Z9KWXYT68/tc022Zpy4R0o2?=
 =?us-ascii?Q?Cp77rVtvnPnBb91UZztWbeyxPwxIFuLW5np6RJUaaCTJIwKPP0qM5QOCGbDD?=
 =?us-ascii?Q?26mvgjOD9y2ITM1YEZHvgkFHJnZ31XJeMgK3I1dkHwr92mAV3lw4fF6/5pV0?=
 =?us-ascii?Q?cSHovrdtfZz13Pn99yG5uMXD71AHmQZgB4xUJN3xNLwP33jZBpPL9Jqb+aGW?=
 =?us-ascii?Q?tXyZxfnLyBjg+ka0K8AeJ89A07OOlGOsD++H6dLekIvUAenb53Ar1PKqrFBT?=
 =?us-ascii?Q?RVi51n0ZbVB8hcUnSrFVGr1osVR2rOrhzU4hTepCLEnMUH6zg63w0FpBtESQ?=
 =?us-ascii?Q?RYEAvhQiGjM1Zp0kR4BGRkD7XP5mq5Nlskordk+kYO4ttas4twJDhhIwa2If?=
 =?us-ascii?Q?Vk3yei1ISIZPlvcpSL9DW42BCyQcrl8yeg6ELZkgkysTPFKBzkrsLFJkP2VY?=
 =?us-ascii?Q?tdvQyTaC3mDUh94ufXMMBOIjHZTyx8kIcH1Xyu8ZGMtuiWKLq6wO0JcNKf6U?=
 =?us-ascii?Q?1JHQpEshmOjz8rXeyhZTy5Dh04WwTLEGhvkS7RJsHqQ4Is+K051GFQLt2ymP?=
 =?us-ascii?Q?KfTHQKgjTnmkSEszoRK+2t7EkCdEWV1qDlSeA37rGgyw19E+AfS6Pkweu8CN?=
 =?us-ascii?Q?2Ccus+JrnBfAckrckEIaZKh1sNvKr12k8Xp4EnDCafuLEQzP1dwwCrYHdjH8?=
 =?us-ascii?Q?cyZE/qi6Jo+tmQSK5UZtU43nF/Sf3OhKLvTWCdFkr/ED9GXl8juJHm5JdojI?=
 =?us-ascii?Q?fFHMo5PQPzzYTgdUdgWE4CrBucbYligRyOg267rzH266xJBpCKtwkA7nIDSz?=
 =?us-ascii?Q?7xKoOeyeCyoIot1hF9kelt8rNxM24EUyj1RDvFY9f146HFWqSap4NVae3RnZ?=
 =?us-ascii?Q?JNNE7P/RRQIlX/3EeEy5Tl8EoKj3fAyBUc4jHLZPRJmrTZH/OGJTyKTcA20j?=
 =?us-ascii?Q?7D+aaBx9zrQsuICcPIbO0jhVl8EtrH39X750d5UOmR8FD6NRGbVuZWsWxBC3?=
 =?us-ascii?Q?B0XI0Qub3T8Y2w2Rs5mY6TSmEq33a094QyVcakU5JfLhKJT3MuYdW65Mnq9Q?=
 =?us-ascii?Q?fs3Ol+L24X8rU273uO4FJFHA2vdut1RLhMr1Nztm8WbuTo4b0EQWMTVvn59t?=
 =?us-ascii?Q?jZYvVozibIiDNBcWEPHlyvKiJZjQyp/OQEbNLVoJQKs+JlnQKHx7ZIb2gS5i?=
 =?us-ascii?Q?JF1kRO/yrm6wt0+VQOtkVDuOHZ2d6T9v02cmMVQQlLYrPCcppCasL9NM6U/D?=
 =?us-ascii?Q?H8SOQHY4GNYnZI+uguoz0WYWPYUP1VSA041YON0BjULE0eNNAWSyBpd9Kh6O?=
 =?us-ascii?Q?zctKjdgWilscPilLGY3Yp0Y0WW27aENocJMppuT8?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 14434115-ddb8-4624-3e2b-08daeef13c3a
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jan 2023 07:48:29.2575
 (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: yoSN4kxrXnHQw7SkIZn7AsVrycOfYHP62X7d4cbdQA97diOVMR8F6wt3hhSyOu6gsm+dg0lZAzqMrdZZDxk0SQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB7358
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,
        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


"Huang, Ying" <ying.huang@intel.com> writes:

> [snip]
>
>>
>>>>> @@ -1462,30 +1549,28 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>>  		nr_retry_pages = 0;
>>>>>  
>>>>>  		list_for_each_entry_safe(folio, folio2, from, lru) {
>>>>> +			if (folio_test_hugetlb(folio)) {
>>>>
>>>> How do we hit this case? Shouldn't migrate_hugetlbs() have already moved
>>>> any hugetlb folios off the from list?
>>>
>>> Retried hugetlb folios will be kept in from list.
>>
>> Couldn't migrate_hugetlbs() remove the failing retried pages from the
>> list on the final pass? That seems cleaner to me.
>
> To do that, we need to go through the folio list again to remove all
> hugetlb pages.  It could be time-consuming in some cases.  So I think
> that it's better to keep this.

Why? Couldn't we test pass == 9 and remove it from the list if it fails
the final retry in migrate_hugetlbs()? In any case if it's on the list
due to failed retries we have already passed over it 10 times, so the
extra loop hardly seems like a problem.

 - Alistair

> Best Regards,
> Huang, Ying
>
>>>>> +				list_move_tail(&folio->lru, &ret_folios);
>>>>> +				continue;
>>>>> +			}
>>>>> +
>>>>>  			/*
>>>>>  			 * Large folio statistics is based on the source large
>>>>>  			 * folio. Capture required information that might get
>>>>>  			 * lost during migration.
>>>>>  			 */
>>>>> -			is_large = folio_test_large(folio) && !folio_test_hugetlb(folio);
>>>>> +			is_large = folio_test_large(folio);
>>>>>  			is_thp = is_large && folio_test_pmd_mappable(folio);
>>>>>  			nr_pages = folio_nr_pages(folio);
>>>>> +
>>>>>  			cond_resched();
>>>>>  
>>>>> -			if (folio_test_hugetlb(folio))
>>>>> -				rc = unmap_and_move_huge_page(get_new_page,
>>>>> -						put_new_page, private,
>>>>> -						&folio->page, pass > 2, mode,
>>>>> -						reason,
>>>>> -						&ret_folios);
>>>>> -			else
>>>>> -				rc = unmap_and_move(get_new_page, put_new_page,
>>>>> -						private, folio, pass > 2, mode,
>>>>> -						reason, &ret_folios);
>>>>> +			rc = unmap_and_move(get_new_page, put_new_page,
>>>>> +					    private, folio, pass > 2, mode,
>>>>> +					    reason, &ret_folios);
>>>>>  			/*
>>>>>  			 * The rules are:
>>>>> -			 *	Success: non hugetlb folio will be freed, hugetlb
>>>>> -			 *		 folio will be put back
>>>>> +			 *	Success: folio will be freed
>>>>>  			 *	-EAGAIN: stay on the from list
>>>>>  			 *	-ENOMEM: stay on the from list
>>>>>  			 *	-ENOSYS: stay on the from list
>>>>> @@ -1512,7 +1597,6 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>>  						stats.nr_thp_split += is_thp;
>>>>>  						break;
>>>>>  					}
>>>>> -				/* Hugetlb migration is unsupported */
>>>>>  				} else if (!no_split_folio_counting) {
>>>>>  					nr_failed++;
>>>>>  				}