Received-SPF: pass (google.com: domain of linux-kernel+bounces-200752-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Zi Yan <ziy@nvidia.com>
To: David Hildenbrand <david@redhat.com>
Cc: xu.xin16@zte.com.cn, akpm@linux-foundation.org, v-songbaohua@oppo.com,
 mhocko@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org,
 yang.yang29@zte.com.cn
Subject: Re: [PATCH linux-next] mm: huge_memory: fix misused
 mapping_large_folio_support() for anon folios
Date: Tue, 04 Jun 2024 06:52:54 -0700
Message-ID: <E30FDAA1-B4D4-41C9-993C-0AD5044F30C0@nvidia.com>
In-Reply-To: <fe35d494-b54e-4302-8c75-24abc9094ea1@redhat.com>
References: <20240604134738264WKaKYb3q_YTE32hNAy2lz@zte.com.cn>
 <fe35d494-b54e-4302-8c75-24abc9094ea1@redhat.com>
Content-Type: multipart/signed;
 boundary="=_MailMate_1DA8CF4E-A6E2-4232-9624-4A4273D7299D_=";
 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?DFsqfUigX3vbOSy2otm25TSMAdsh9/wjslJjKzPdogL/ppAj0foBj4Wk8ZZj?=
 =?us-ascii?Q?5CTNfsUSX5QYYrM8vgTnKrCtncBGKTehiTB8vZcuTks9AX/ZlH9TsijCdz+E?=
 =?us-ascii?Q?nTEaM7sUWSgrygknEe++wXGgwOgvWpLEzRFs99r7coP51HKC4B9o8+HnPUqh?=
 =?us-ascii?Q?1DHwlTHHDFzH5YG55Y18OkODN2yll/dV+BymRgKfyNWy57xARot+dzpZPz0+?=
 =?us-ascii?Q?P0LjIJpz+F0yLsNk5aLBPjVyk9E3sjJWY8HYHjb0ok5h98RIzDIaJK7/oIPM?=
 =?us-ascii?Q?8IQ2bz4QmBnskd2Qa1q+NwUQn1JK1gVECJfDWYgLpgyw/s/N1J54urcAZi1N?=
 =?us-ascii?Q?7IeMKv6ANWorrLse2yUbT2lYR7/xVWxgE2dIIdgUp8mBOlvaEqj25gjnGoPH?=
 =?us-ascii?Q?vk5/pe4UWtAI1e/TO0G0bC2kw765BwOoV/2VRhY3IoIq88qhnWWcKEvGZhPV?=
 =?us-ascii?Q?Wb2zEFuQR2++GwmQEua9XRrtKQ1bfF10oLh4jiEMUWSloe8giVf8EWRRyY45?=
 =?us-ascii?Q?yCKOexgxwOfkVWpIWQ06APAUVbMnooXbftXXA3qu9EMjnmf5dcwvJpe20XI9?=
 =?us-ascii?Q?OycQnD+6Tv8W4Jhoa3O1RwT/TP+eHgtIj80rrX8Sif2fXRRnxKEpeXi0zAUY?=
 =?us-ascii?Q?YbNZTVQ0su7dwxhjAV49HvA9kc3Jv0xqPKlvfoa4pJuuRY51WGDHffEcz35H?=
 =?us-ascii?Q?uLke0AynnezZ/KyzDekcs2CNWE6n2IZjDLc+J/UDeywkLFMU4Ek+0TBTIO/E?=
 =?us-ascii?Q?68a82W+YIYx+rL4H4XYqo3j0Szs4kkeACPUhMaf0ZH1N2vtjN6tf9NEhGHtS?=
 =?us-ascii?Q?IgeR80X2XHkDCnR0iThwapMTVC+Sd/0DL96dzgYuByNRLmxR+1byTLWL7ATw?=
 =?us-ascii?Q?jZDJDbPH25uws4ZpDwyiCyEkj2jPGu7Enbcm+DCeIpEBe4y3vjiQ/Hlotzcq?=
 =?us-ascii?Q?31Sn0KrBYHDKrcQjbYe4FLittl06P/lef7sMwQVQgX0IVy481lLU+ynR5tbe?=
 =?us-ascii?Q?xSOJNyIDqW7Et+ftygleqNOjq0t05U8KYha01EZVTGg3TKn/mJ/L+DPkhnQ8?=
 =?us-ascii?Q?+6r16oUGMMIXKtctbov9xnWlWjAXoCEdmeJBxaGj/lpXTKnqbPfgkwXx8dhT?=
 =?us-ascii?Q?ce4d5pt/+yrpSg78K5nqCxIJtKPOcQI/rmMgatsyI/yYfYlIO/z3/jNzXwRE?=
 =?us-ascii?Q?NU8nDd1i94n6v2cWuFsW1KIW4+nJgqz3tC78kEZ+s5ksUh9S5fMn6u/zyOCB?=
 =?us-ascii?Q?B8qbh23ZajAbpGKcLmdGIhGQ/8UsserPsIdCW+eU9ZA/L9yAn/tx3c77KYRz?=
 =?us-ascii?Q?09k3aDnOefzvStHgC5Vrhq45wurwUhdEew7IseoSSVv53oV23ZTmY6ATU7X1?=
 =?us-ascii?Q?srYIBnevLZnXnHfVU2hRXGYHJhy5e91QgpDLMGdGTaoBk7H84xxaF9fpUd8o?=
 =?us-ascii?Q?LHmlSf0t6syl9req7jziTXRp0YdEjp5DgvBmEh/80/NofwwnS4SRkCRM/TM8?=
 =?us-ascii?Q?tb95PTfpvt+Np8PJ41XZ4aTTMuJakh7UXDAsFq6uPrnCAkei5z3cX4QTBJvg?=
 =?us-ascii?Q?rAigpkXoXTUVa5dXSuo=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cc1dd1c7-70bb-425e-24f9-08dc849da45f
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5744.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jun 2024 13:52:58.3382
 (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: 2v8q6J29SscJqdNBPd39XSt1ES02C23J3w4xvfNt5GnOcgHuRyqQEcJ2FETol1/v
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB7876

--=_MailMate_1DA8CF4E-A6E2-4232-9624-4A4273D7299D_=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On 4 Jun 2024, at 0:57, David Hildenbrand wrote:

> On 04.06.24 07:47, xu.xin16@zte.com.cn wrote:
>> From: Ran Xiaokai <ran.xiaokai@zte.com.cn>
>>
>> When I did a large folios split test, a WARNING
>> "[ 5059.122759][  T166] Cannot split file folio to non-0 order"
>> was triggered. But my test cases are only for anonmous folios.
>> while mapping_large_folio_support() is only reasonable for page
>> cache folios.
>
> Agreed.
>
> I wonder if mapping_large_folio_support() should either
>
> a) Complain if used for anon folios, so we can detect the wrong use mor=
e easily. (VM_WARN_ON_ONCE())

This is much better.

>
> b) Return "true" for anonymous mappings, although that's more debatable=
=2E

This might fix the warning here, but the function might get wrong uses ea=
sily.

>
>>
>> In split_huge_page_to_list_to_order(), the folio passed to
>> mapping_large_folio_support() maybe anonmous folio. The
>> folio_test_anon() check is missing. So the split of the anonmous THP
>> is failed. This is also the same for shmem_mapping(). We'd better add
>> a check for both. But the shmem_mapping() in __split_huge_page() is
>> not involved, as for anonmous folios, the end parameter is set to -1, =
so
>> (head[i].index >=3D end) is always false. shmem_mapping() is not calle=
d.
>>
>> Using /sys/kernel/debug/split_huge_pages to verify this, with this
>> patch, large anon THP is successfully split and the warning is ceased.=

>>
>> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
>> Cc: xu xin <xu.xin16@zte.com.cn>
>> Cc: Yang Yang <yang.yang29@zte.com.cn>
>> ---
>>   mm/huge_memory.c | 38 ++++++++++++++++++++------------------
>>   1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 317de2afd371..4c9c7e5ea20c 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -3009,31 +3009,33 @@ int split_huge_page_to_list_to_order(struct pa=
ge *page, struct list_head *list,
>>   	if (new_order >=3D folio_order(folio))
>>   		return -EINVAL;
>>
>> -	/* Cannot split anonymous THP to order-1 */
>> -	if (new_order =3D=3D 1 && folio_test_anon(folio)) {
>> -		VM_WARN_ONCE(1, "Cannot split to order-1 folio");
>> -		return -EINVAL;
>> -	}
>> -
>>   	if (new_order) {
>>   		/* Only swapping a whole PMD-mapped folio is supported */
>>   		if (folio_test_swapcache(folio))
>>   			return -EINVAL;
>> -		/* Split shmem folio to non-zero order not supported */
>> -		if (shmem_mapping(folio->mapping)) {
>> -			VM_WARN_ONCE(1,
>> -				"Cannot split shmem folio to non-0 order");
>> -			return -EINVAL;
>> -		}
>> -		/* No split if the file system does not support large folio */
>> -		if (!mapping_large_folio_support(folio->mapping)) {
>> -			VM_WARN_ONCE(1,
>> -				"Cannot split file folio to non-0 order");
>> -			return -EINVAL;
>> +
>> +		if (folio_test_anon(folio)) {
>> +			/* Cannot split anonymous THP to order-1 */
>> +			if (new_order =3D=3D 1) {
>> +				VM_WARN_ONCE(1, "Cannot split to order-1 folio");
>> +				return -EINVAL;
>> +			}
>> +		} else {
>> +			/* Split shmem folio to non-zero order not supported */
>> +			if (shmem_mapping(folio->mapping)) {
>> +				VM_WARN_ONCE(1,
>> +					"Cannot split shmem folio to non-0 order");
>> +				return -EINVAL;
>> +			}
>> +			/* No split if the file system does not support large folio */
>> +			if (!mapping_large_folio_support(folio->mapping)) {
>> +				VM_WARN_ONCE(1,
>> +					"Cannot split file folio to non-0 order");
>> +				return -EINVAL;
>> +			}
>>   		}
>>   	}
>
> What about the following sequence:
>
> if (folio_test_anon(folio)) {
> 	if (new_order =3D=3D 1)
> 		...
> } else if (new_order) {
> 	if (shmem_mapping(...))
> 		...
> 	...
> }
>
> if (folio_test_swapcache(folio) && new_order)
> 	return -EINVAL;
>
> Should result in less churn and reduce indentation level.

Yeah, this looks better to me.

Best Regards,
Yan, Zi

--=_MailMate_1DA8CF4E-A6E2-4232-9624-4A4273D7299D_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature; name=signature.asc

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

iQJDBAEBCgAtFiEEh7yFAW3gwjwQ4C9anbJR82th+ooFAmZfHDYPHHppeUBudmlk
aWEuY29tAAoJEJ2yUfNrYfqK4p8P/3Z00wHEG3jJ5l4vHPNvPA1fb9aeUraW5Tvn
Y0xAcbNViss4cuFknKE6z0Aw65b3nb4lQZ9Ux7m8lbpeW8JKFraj2qAjg1yMl8ZT
+e44orvEwbbUA9ga0ZlcATkWETb/hMYb1E1Wa090lToXU1CjtrWSLZeU0FTAeRQT
SIoNqX4faWAs2p4hOAle+YUTzzdqbXTrowQwVhXxV8efB0x+RAv8qdJd4frWtio1
oBuSNTs/iUqFykXDsQekxDon1l4SW9bWvMKP2w9PsrIoP7iH49i1rqeOYNgCuQwW
TbWiiQYzjm/8tN9VP4NVNR6ZKGIXS5RR48pIoIeqX9J8QsHM6S913NM+87wZL+0k
AXFXKzRAIxvyJxMCw+GpfpZ22TQrlaTrtVRpX0J0cHiDSq/24Ff2vpDxB4r1aigp
WbBhFSWgD1FBjouj5i2Op/yGhxL3XhZD4FV4TFx6XCzPJGUTY6aZ3iqFQ1pMIxLO
uSdWQq37GmvcD4yxjoou1QaiutbfMo10otc8g9uVEpzAjOFwoKiON9lvxT9OrYJB
wI1pitQv3CPXXkQcvdFRnFD/Xc569TOHkdAOn30peh9oYbzyhxmsHDR09eYf1s64
HHdG6QkrlR+01iCyzdVUOuAEa985ca7cM/CXO8Dk4tVT8OsLqA/AObTRZrwyMpDg
KxhiF9mf
=VAWf
-----END PGP SIGNATURE-----

--=_MailMate_1DA8CF4E-A6E2-4232-9624-4A4273D7299D_=--