Received-SPF: pass (google.com: domain of linux-kernel+bounces-73237-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Zi Yan <ziy@nvidia.com>
To: David Hildenbrand <david@redhat.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
 "\"Huang, Ying\"" <ying.huang@intel.com>,
 Ryan Roberts <ryan.roberts@arm.com>,
 Andrew Morton <akpm@linux-foundation.org>,
 "\"Matthew Wilcox (Oracle)\"" <willy@infradead.org>,
 "\"Yin, Fengwei\"" <fengwei.yin@intel.com>, Yu Zhao <yuzhao@google.com>,
 Vlastimil Babka <vbabka@suse.cz>,
 "\"Kirill A . Shutemov\"" <kirill.shutemov@linux.intel.com>,
 Johannes Weiner <hannes@cmpxchg.org>,
 Baolin Wang <baolin.wang@linux.alibaba.com>,
 Kemeng Shi <shikemeng@huaweicloud.com>,
 Mel Gorman <mgorman@techsingularity.net>,
 Rohan Puri <rohan.puri15@gmail.com>, Mcgrof Chamberlain <mcgrof@kernel.org>,
 Adam Manzanares <a.manzanares@samsung.com>,
 "\"Vishal Moola (Oracle)\"" <vishal.moola@gmail.com>
Subject: Re: [PATCH v6 2/4] mm/compaction: enable compacting >0 order folios.
Date: Tue, 20 Feb 2024 10:27:33 -0500
Message-ID: <42FD6CCE-C3F7-4E5D-86C1-C2FE2EF06818@nvidia.com>
In-Reply-To: <416d1450-6480-4113-b778-689a8f1d4e42@redhat.com>
References: <20240216170432.1268753-1-zi.yan@sent.com>
 <20240216170432.1268753-3-zi.yan@sent.com>
 <416d1450-6480-4113-b778-689a8f1d4e42@redhat.com>
Content-Type: multipart/signed;
 boundary="=_MailMate_EFFE56B8-C35C-45B0-B11A-1A1A9CF07B67_=";
 micalg=pgp-sha512; protocol="application/pgp-signature"
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?sv7OHzEgX5R0Gp14bnLrayq0vO+hHS8TN+oE3kT1z5NN01Z9/V20GngGo0TK?=
 =?us-ascii?Q?HjyMzR8O2rXd3zX8yB+FUI4cvUp7YqCSS9DL21Itgq0QkLnIyrOvjxdlodls?=
 =?us-ascii?Q?B/ecZVHWZOlxQA9uqASRrZ8mPoahaJiCIAKpIn+qmXLe3xsT6nlzLgvBDNxr?=
 =?us-ascii?Q?kWUA1Va25GoMi4UTl3ZXpntufOXsYGIHe0saOVk1fEqLYeO3LP1mmTaVXnKN?=
 =?us-ascii?Q?XnAF/7OFTeYgWAIFWj2AIhLZcD2AiUErgLLLq3AIaWGABGzPpfAGYPrT9Z9T?=
 =?us-ascii?Q?jnwlHi17FcqXBl1MbSLbMg0XkwcfdIZ/ifKV0sCbpyO5VQe0wk+YkkyKSps8?=
 =?us-ascii?Q?ONNiG0n2ym5gshVVrIsxj2uGlujqWHxKUeHr7rff0H6idAa3Weho0vl31v1F?=
 =?us-ascii?Q?wdmBpwQ8IGDUhuRW22RxiGIFuPOGXOWxeB68yimj6+qt5+Rq7ohkETytfin4?=
 =?us-ascii?Q?WxlI1EmLQz9S5pvFl1cP0h9mUkCBlKk2M9OB7NsLyGhFYzHzEd4EKosz99t9?=
 =?us-ascii?Q?BqbGhEXBUBnWZUWnjfVPnSq3qOrwzuDqwl8MerP1HDNKgwpI12hcapxC72yD?=
 =?us-ascii?Q?zyeF5tiIOmK3xPJ7cC3E8ME3r+j5TIf5l0y+QpusGlQqBUbOmHUeq+OJrjcx?=
 =?us-ascii?Q?e51nv/loKxjr30bPFc0TjcuOIKuSfst2HrfGL+Mz/XglGH0rcFWGLtx3pdb/?=
 =?us-ascii?Q?I3SgxrwPxM69RBEHnEa4ZhtpmGi1eq2g5I11aPb/rb7LjRjQO8/oShJxJWyk?=
 =?us-ascii?Q?ZzxCDgfnwapqsAhqXEhnoIJdM+XuVBtaOYOl6mjmD0hLbhDrjHckOGCwo8xl?=
 =?us-ascii?Q?aTfSAzDRWQgVz0ub+T1OaDogeqFI6Mdwsfqqdoy7jhLGSbaSsIqvsnPM3Ge6?=
 =?us-ascii?Q?FGOcp6aT/Nm8mh/a0ARZvBbz9jXkUz9cSxQWay0WK5uVtBhprErKnemopWDK?=
 =?us-ascii?Q?tE7b4Gp/1wOOghY69Ev4YbO0iEyW0tKAR/bhHzIIhbIOZx3WqJJZ4Noo1B65?=
 =?us-ascii?Q?ifpuBUBWTZVBmCYEj8KUUaxmroZKrnXagwtDEGEHtktC8U2uEEl8A2EpYFz/?=
 =?us-ascii?Q?BF1a9iPpooHR/H1gyTrX2h0FcmW20kQS7Rw2rc7EvnRwWdQGySn+/wssHOxB?=
 =?us-ascii?Q?1IZEPfaytpjxAia3QqpyDQVO7Zz4aA8Q4wrgJ9Dh1odMsihxkwnNrXI0u3nQ?=
 =?us-ascii?Q?naEQwUZcBzuw5Z3DbKtI8bShLmMcgr9hJDw4vtyr6/LD8Wl/WumobgJJLMls?=
 =?us-ascii?Q?HaHfl5JiyFiy6kRRLPqiHBjXJ11gB9MT7gR030JgCPS+hoD8SAue+flg5Q39?=
 =?us-ascii?Q?/bxRII6tkdwp5WMr1a2mC3bgI1sh4raVoMrCpLLrCBRAE5iJUJhDD1P5Vd48?=
 =?us-ascii?Q?98DBrAFJH8ihpZtPezhLxRY8opWedfaOk+daFVj+D6wBU5sHmSEqeg3bPCWf?=
 =?us-ascii?Q?LS22U7/3iIPM81c7IpHPVrBatkhfZj6gH6z5LQxyFrnilYUgEHbXVOa0NB4/?=
 =?us-ascii?Q?QVzERGwjnJJxPaPPjfF75dFoktH+ttc7GIafaiG+xZNmQWhxhLNdpiJLbb+4?=
 =?us-ascii?Q?KpnE4Nr91eNXwcVktBQ=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 69b66417-2430-4cc2-8c8c-08dc322876fb
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5744.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2024 15:27:35.7871
 (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: l8wN/2Izu/lGcujts9Js9zy/mKFheOro5rs/N6bSR+CR1kXZ8b76Es0KuslsOCwb
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR12MB5058

--=_MailMate_EFFE56B8-C35C-45B0-B11A-1A1A9CF07B67_=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On 20 Feb 2024, at 4:03, David Hildenbrand wrote:

> On 16.02.24 18:04, Zi Yan wrote:
>> From: Zi Yan <ziy@nvidia.com>
>>
>> migrate_pages() supports >0 order folio migration and during compactio=
n,
>> even if compaction_alloc() cannot provide >0 order free pages,
>> migrate_pages() can split the source page and try to migrate the base
>> pages from the split.  It can be a baseline and start point for adding=

>> support for compacting >0 order folios.
>>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> Suggested-by: Huang Ying <ying.huang@intel.com>
>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
>> Tested-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> Tested-by: Yu Zhao <yuzhao@google.com>
>> Cc: Adam Manzanares <a.manzanares@samsung.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Kemeng Shi <shikemeng@huaweicloud.com>
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Cc: Luis Chamberlain <mcgrof@kernel.org>
>> Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
>> Cc: Mel Gorman <mgorman@techsingularity.net>
>> Cc: Ryan Roberts <ryan.roberts@arm.com>
>> Cc: Vishal Moola (Oracle) <vishal.moola@gmail.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Yin Fengwei <fengwei.yin@intel.com>
>> ---
>>   mm/compaction.c | 66 ++++++++++++++++++++++++++++++++++++++---------=
--
>>   1 file changed, 52 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index cc801ce099b4..aa6aad805c4d 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -816,6 +816,21 @@ static bool too_many_isolated(struct compact_cont=
rol *cc)
>>   	return too_many;
>>   }
>>  +/*
>
>
> Can't you add these comments to the respective checks? Like
>
> static bool skip_isolation_on_order(int order, int target_order)
> {
> 	/*
> 	 * Unless we are performing global compaction (targert_order <
> 	 * 0), skip any folios that are larger than the target order: we
> 	 * wouldn't be here if we'd have a free folio with the desired
> 	 * target_order, so migrating this folio would likely fail
> 	 * later.
> 	 */
> 	if (target_order !=3D -1 && order >=3D target_order)
> 		return true;
> 	/*
> 	 * We limit memory compaction to pageblocks and won't try
> 	 * creating free blocks of memory that are larger than that.
> 	 */
> 	return order >=3D pageblock_order;
> }
>
> Then, add a simple expressive function documentation (if really require=
d) that doesn't contain all these details.
>

OK. No problem.

>> + * 1. if the page order is larger than or equal to target_order (i.e.=
,
>> + * cc->order and when it is not -1 for global compaction), skip it si=
nce
>> + * target_order already indicates no free page with larger than targe=
t_order
>> + * exists and later migrating it will most likely fail;
>> + *
>> + * 2. compacting > pageblock_order pages does not improve memory frag=
mentation,
>
> I'm pretty sure you meant "reduce" ?

Yes.

>
>> + * skip them;
>> + */
>> +static bool skip_isolation_on_order(int order, int target_order)
>> +{
>> +	return (target_order !=3D -1 && order >=3D target_order) ||
>> +		order >=3D pageblock_order;
>> +}
>> +
>>   /**
>>    * isolate_migratepages_block() - isolate all migrate-able pages wit=
hin
>>    *				  a single pageblock
>> @@ -947,7 +962,22 @@ isolate_migratepages_block(struct compact_control=
 *cc, unsigned long low_pfn,
>>   			valid_page =3D page;
>>   		}
>>  -		if (PageHuge(page) && cc->alloc_contig) {
>> +		if (PageHuge(page)) {
>> +			/*
>> +			 * skip hugetlbfs if we are not compacting for pages
>> +			 * bigger than its order. THPs and other compound pages
>> +			 * are handled below.
>> +			 */
>> +			if (!cc->alloc_contig) {
>> +				const unsigned int order =3D compound_order(page);
>> +
>> +				if (order <=3D MAX_PAGE_ORDER) {
>> +					low_pfn +=3D (1UL << order) - 1;
>> +					nr_scanned +=3D (1UL << order) - 1;
>> +				}
>> +				goto isolate_fail;
>> +			}
>> +			/* for alloc_contig case */
>>   			if (locked) {
>>   				unlock_page_lruvec_irqrestore(locked, flags);
>>   				locked =3D NULL;
>> @@ -1008,21 +1038,24 @@ isolate_migratepages_block(struct compact_cont=
rol *cc, unsigned long low_pfn,
>>   		}
>>    		/*
>> -		 * Regardless of being on LRU, compound pages such as THP and
>> -		 * hugetlbfs are not to be compacted unless we are attempting
>> -		 * an allocation much larger than the huge page size (eg CMA).
>> -		 * We can potentially save a lot of iterations if we skip them
>> -		 * at once. The check is racy, but we can consider only valid
>> -		 * values and the only danger is skipping too much.
>> +		 * Regardless of being on LRU, compound pages such as THP
>> +		 * (hugetlbfs is handled above) are not to be compacted unless
>> +		 * we are attempting an allocation larger than the compound
>> +		 * page size. We can potentially save a lot of iterations if we
>> +		 * skip them at once. The check is racy, but we can consider
>> +		 * only valid values and the only danger is skipping too much.
>>   		 */
>>   		if (PageCompound(page) && !cc->alloc_contig) {
>>   			const unsigned int order =3D compound_order(page);
>>  -			if (likely(order <=3D MAX_PAGE_ORDER)) {
>> -				low_pfn +=3D (1UL << order) - 1;
>> -				nr_scanned +=3D (1UL << order) - 1;
>> +			/* Skip based on page order and compaction target order. */
>> +			if (skip_isolation_on_order(order, cc->order)) {
>> +				if (order <=3D MAX_PAGE_ORDER) {
>> +					low_pfn +=3D (1UL << order) - 1;
>> +					nr_scanned +=3D (1UL << order) - 1;
>> +				}
>> +				goto isolate_fail;
>>   			}
>> -			goto isolate_fail;
>>   		}
>>    		/*
>> @@ -1165,10 +1198,11 @@ isolate_migratepages_block(struct compact_cont=
rol *cc, unsigned long low_pfn,
>>   			}
>>    			/*
>> -			 * folio become large since the non-locked check,
>> -			 * and it's on LRU.
>> +			 * Check LRU folio order under the lock
>>   			 */
>> -			if (unlikely(folio_test_large(folio) && !cc->alloc_contig)) {
>> +			if (unlikely(skip_isolation_on_order(folio_order(folio),
>> +							     cc->order) &&
>> +				     !cc->alloc_contig)) {
>>   				low_pfn +=3D folio_nr_pages(folio) - 1;
>>   				nr_scanned +=3D folio_nr_pages(folio) - 1;
>>   				folio_set_lru(folio);
>> @@ -1788,6 +1822,10 @@ static struct folio *compaction_alloc(struct fo=
lio *src, unsigned long data)
>>   	struct compact_control *cc =3D (struct compact_control *)data;
>>   	struct folio *dst;
>>  +	/* this makes migrate_pages() split the source page and retry */
>> +	if (folio_test_large(src) > 0)
>> +		return NULL;
>
> Why the "> 0 " check ? Either it's large or it isn't.
Will fix it.

> Apart from that LGTM, but I am no compaction expert.

Thanks.


--
Best Regards,
Yan, Zi

--=_MailMate_EFFE56B8-C35C-45B0-B11A-1A1A9CF07B67_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iQJDBAEBCgAtFiEE6rR4j8RuQ2XmaZol4n+egRQHKFQFAmXUxOUPHHppeUBudmlk
aWEuY29tAAoJEOJ/noEUByhUfp8QAKK8/Mw+zVIYCOx0SuZJk+rY/IWYOr/QVWoc
+lMXSZ50ZvlzpmsJsBCdg+zrl0zPg1cPa66j6OrF8A8nQPvW5F/UDwMhUR1jrHJx
HjYC3qI7eA7G72J+t1FEPq41sBtS/ypiAQbRwiK6KYME0GL42oMpeEPjyi67mixl
SDv7RAOd3YsSi33LXLuSrCQ9BvrMvLMRTesLKHbi+g1xnpdxBdpoF9UdsGH63Xuk
M5ASBmbftbakOQbRvtutmW3sMkL497PToKpXwYzs2aY11zWKwcdVLTivCreFiJ4X
vAWnuY/rTEAQ0AL6sUIF7XUhGcLLsQKiaiAX8ez4IVOdkYun4qXZL8PGXKpo0BfD
DgfJJLB3nLDy1/3iruThx5pFQXDkKR9qDkUg1kFsFuAmOXUVeSp6XSBfjXNz83OH
N7/S8GwZTArb+YV0keMlmFBG2eu6ym5nWYUoOQk+KyfwbF3mF7oYyUyUAxXMYWht
gwHtA02jqyXaAXDiYNMUOtg+A4jJyPG/Al9cMQmp7EU3Z19jKYDeYC8Xe4TheUfs
ZxUD5NrQ0I3rqNpTdws/roHhD5Q+6Z4Mk7wbEcosvZ0yWJY3rKrAxa4h5i82nAEH
zlgnB7M3IlKOAfNe4xgyfjti8wFxEJvKK0dc8Gkw5Ni8z6BElirM7wsSFLYJqi2T
P59gLVCp
=FCD4
-----END PGP SIGNATURE-----

--=_MailMate_EFFE56B8-C35C-45B0-B11A-1A1A9CF07B67_=--