Received-SPF: pass (google.com: domain of linux-kernel+bounces-205093-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: Barry Song <21cnbao@gmail.com>
Cc: xu.xin16@zte.com.cn, david@redhat.com, v-songbaohua@oppo.com,
 akpm@linux-foundation.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org,
 mhocko@kernel.org, yang.yang29@zte.com.cn, ran.xiaokai@zte.com.cn,
 Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH linux-next v2] mm: huge_memory: fix misused
 mapping_large_folio_support() for anon folios
Date: Thu, 06 Jun 2024 14:42:49 -0700
Message-ID: <360B6C7B-B700-461E-A607-691FE35A7623@nvidia.com>
In-Reply-To: <CAGsJ_4xu9jfC7UWdOqP5_saEXRE26mo6Msx_=JGCKaWac57=BQ@mail.gmail.com>
References: <20240606174203124_OW-VQZ_ZLm4lGEimA-K9@zte.com.cn>
 <51DED95F-832A-4836-AA81-556968F6B645@nvidia.com>
 <CAGsJ_4w9cRZUEF7PaVjz1HQoUr1pxHEO15gpbsnAoJHMZG0djQ@mail.gmail.com>
 <B7796F09-BAA6-4555-A9FE-F44DF1CBFA6F@nvidia.com>
 <CAGsJ_4xu9jfC7UWdOqP5_saEXRE26mo6Msx_=JGCKaWac57=BQ@mail.gmail.com>
Content-Type: multipart/signed;
 boundary="=_MailMate_B5159FCD-377D-42FA-9377-8FE173AFCB33_=";
 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:
	=?utf-8?B?b2JBQUZUSXBxUXhOdE5vcWFjMjZSN20yRTVjUERzUGl5ZUQ5UnQwVEh6RGVR?=
 =?utf-8?B?U0UrUEkwbk5ScXloRnA4YUZPV01HamVEY0Rlb0Y0RFhTUFJkVWxDVGFVUXp2?=
 =?utf-8?B?NG41bmV4K2dJVDJHVVlCcFJPWTdjOHJSRzhMT0FNYTc4TnN3TC80MldNOHpH?=
 =?utf-8?B?ZjFQRnBxMGdhWEd5TmxWaVFjbHdmTERmcHF2bVcyMi9GRi8wR0JXWnFXTXdy?=
 =?utf-8?B?N09pSDNNekNJUEMyUlFIckxNQlVQQUMxT0tJdkRyNG11WkltbitKVTkvcEhE?=
 =?utf-8?B?WGkrRFl6aEZDYjZ6YnhJcmIzalo1ekIva0t2S2Z5M0VnYmlQRHdXaUZKa2Rv?=
 =?utf-8?B?aEhLYktPU2RlTHpSOFBGRTRYTmREbHMxUkF5T3VwMHdORkx5SnM5cStIOFQ3?=
 =?utf-8?B?WW1CU21VY2dQMCt6SnlQOGpTYU9aSjJkMmFWejVYVFI2Y1VyeDNnQXdHRGI4?=
 =?utf-8?B?M2RudVAyQjFKblkrT1JUbmZHMUs3YmV6SUFPRXFtZ0lUZXRSVHc1VkdGeGx3?=
 =?utf-8?B?bXZ1djcrU2djSHZkWlBYVGpSTURVaURibmQ1UEhXQW1zT0RkQjVyUXI0WnVW?=
 =?utf-8?B?TjN3aHpONkRRSnR2OXJ3dXRINnhnSU53a2V2c2hLRFpUbzh5S1NkMDFmNFJN?=
 =?utf-8?B?SHh1TXBCQUE2bzdxUXUzRVgySE1kRHd1cmJUbVdzbGZKSHo5WnVza3d0blJx?=
 =?utf-8?B?SUpRdWR0ZVgyTjhiYkNEQUM0RU9ZUFdyWHEvV2V5SDhaMk9ScmNzUENMRzB4?=
 =?utf-8?B?Vk13TGxpMGIzYzNpYVAvcm9nYzNCMzZCb1JRRGlRaU16NTNJeHJyZ09uUHdr?=
 =?utf-8?B?WFdWaGMvcjB2RnhkY1owMnFpZkJ4aTZ4c3c2bXF2QlVzYnZKaHY3dXVMUG1Z?=
 =?utf-8?B?QUVyWWtrOEVvVHdXamd0WGJKRHlKeTR6U0tpNFJrSTdCVllFOEVmWm13RnhO?=
 =?utf-8?B?b0ZpTzBaRkVJYWNObDduOUNZeWNseFdUU0FLQ29vRURWaTExU2daSVlzTHFm?=
 =?utf-8?B?Z0Y3ZnJnbThaY2Y1Y3MxZUlhUGVXR3VNWmMyK1NucG03bzZ5bGZtellJU0tN?=
 =?utf-8?B?NjV3TVVvbUloeVFyQmNyMDNhNXlGVkI4MmRXOVZlclhnN2lXTFhDWUNyUTdN?=
 =?utf-8?B?aFpaRWtLb0ZBYVR6YTRuWjFjbml6MzU3QkZpalZPRjFtOXBnZUZZSmFpUk1M?=
 =?utf-8?B?LzIxQXBFMUd3RWpJTUdyOE81bFpubmQwZlFENVVidEFaUUhNc1kxVkQxaVdE?=
 =?utf-8?B?NmFPcUFaNW1Jc08zelowellxaGV1bll4MG9IdU1rTE43TGJIZnFWMVU3VGxs?=
 =?utf-8?B?MjByMWZGc2NMTm1hc0lvL0I0LzVBdzVEN2M4blRpV285QnhkUGFRUUx3aFVP?=
 =?utf-8?B?WmxHSGV5YlEvZkhob2xiQnRISGVOR0Z2bHcvQkFaWmdIaVVXY2IyY3VJNFRx?=
 =?utf-8?B?ZnJOZTZSSGllaEQ4MU1xZDVhK3pCazJhQ1Q1RTg1MTM2elhTRlNXMzZndUZE?=
 =?utf-8?B?MXo0RGRHM2VlTVJyOGhlcmtOajNWY1AwbmFWWWgzN0NiM24zZytoYzd5bWJn?=
 =?utf-8?B?eEIrczNjVTdEV1FDcElVVmZjQ0Q3VllVVVBJZVlBWmtGd1NSVjJ5NW1vekVr?=
 =?utf-8?B?NlFkZDZRc3dlMzMxTjloVXBSOFFxc09wYXdqNU9tUm5DamhLL3BselFIZEtv?=
 =?utf-8?B?dExwTExZZlJhd21JUWFNaHJZM2FNM2pkYkVkZkpRVjJJcmE5SFYxOU9WVGI0?=
 =?utf-8?B?TWttT2xLcDVzbVlFdk9SK1JxUHl5RjZnZ3VyZWVocnUzeUdvQjVQeUlDSml4?=
 =?utf-8?B?aHVhb1E0K2JaaEdhR3NML0FmVW9oa3F4VTJVVDh6b1plWmxZbEdyNkVwNmhi?=
 =?utf-8?B?anhBS21NNHVURWVwNG84ZkMzWS9mN3hna1F2UmtUVzZ4M2NVY1d4TEtxTHVY?=
 =?utf-8?B?U1ZFNDVQcnRrcEJva3VUQ1NuZWx6MW9qN1cxS1JZNHp5WWxkam9CcU4zUEh5?=
 =?utf-8?B?Z01lUUxHWFMxbHlHWFpvQ0MwNTFrYmozdkNaSWo2ejlMZCtvVnRHSGxuWWtM?=
 =?utf-8?B?OUJrZlNnS243Z0pxZlBDUnFPQ3lPUlNPTU9EZE5kWWdWQ3hIR0IyQ2oxck90?=
 =?utf-8?Q?nFeg=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 94c9fff0-d149-456c-8af5-08dc86719eed
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5744.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jun 2024 21:42:53.7090
 (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: rKFxr7Wj0/sM6SViCfrXdQBcJfCKiFveJ+Mf2tDJbqdpOdCzaaxZY8U+VQRNLlrr
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB8813

--=_MailMate_B5159FCD-377D-42FA-9377-8FE173AFCB33_=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On 6 Jun 2024, at 14:33, Barry Song wrote:

> On Fri, Jun 7, 2024 at 9:24=E2=80=AFAM Zi Yan <ziy@nvidia.com> wrote:
>>
>> On 6 Jun 2024, at 14:00, Barry Song wrote:
>>
>>> On Fri, Jun 7, 2024 at 2:35=E2=80=AFAM Zi Yan <ziy@nvidia.com> wrote:=

>>>>
>>>> +Matthew
>>>>
>>>> For mapping_large_folio_support() changes.
>>>>
>>>> On 6 Jun 2024, at 2:42, 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 the test cases are only for anonmous folios.
>>>>> while mapping_large_folio_support() is only reasonable for page
>>>>> cache folios.
>>>>>
>>>>> 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 TH=
P
>>>>> is failed. This is also the same for shmem_mapping(). We'd better a=
dd
>>>>> 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 ca=
lled.
>>>>>
>>>>> Also add a VM_WARN_ON_ONCE() in mapping_large_folio_support()
>>>>> for anon mapping, So we can detect the wrong use more easily.
>>>>>
>>>>> THP folios maybe exist in the pagecache even the file system doesn'=
t
>>>>> support large folio, it is because when CONFIG_TRANSPARENT_HUGEPAGE=

>>>>> is enabled, khugepaged will try to collapse read-only file-backed p=
ages
>>>>> to THP. But the mapping does not actually support multi order
>>>>> large folios properly.
>>>>>
>>>>> Using /sys/kernel/debug/split_huge_pages to verify this, with this
>>>>> patch, large anon THP is successfully split and the warning is ceas=
ed.
>>>>>
>>>>> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
>>>>> ---
>>>>>  include/linux/pagemap.h |  4 ++++
>>>>>  mm/huge_memory.c        | 27 ++++++++++++++++-----------
>>>>>  2 files changed, 20 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
>>>>> index ee633712bba0..59f1df0cde5a 100644
>>>>> --- a/include/linux/pagemap.h
>>>>> +++ b/include/linux/pagemap.h
>>>>> @@ -381,6 +381,10 @@ static inline void mapping_set_large_folios(st=
ruct address_space *mapping)
>>>>>   */
>>>>>  static inline bool mapping_large_folio_support(struct address_spac=
e *mapping)
>>>>>  {
>>>>> +     /* AS_LARGE_FOLIO_SUPPORT is only reasonable for pagecache fo=
lios */
>>>>> +     VM_WARN_ONCE((unsigned long)mapping & PAGE_MAPPING_ANON,
>>>>> +                     "Anonymous mapping always supports large foli=
o");
>>>>> +
>>>>>       return IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
>>>>>               test_bit(AS_LARGE_FOLIO_SUPPORT, &mapping->flags);
>>>>>  }
>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>> index 317de2afd371..62d57270b08e 100644
>>>>> --- a/mm/huge_memory.c
>>>>> +++ b/mm/huge_memory.c
>>>>> @@ -3009,30 +3009,35 @@ int split_huge_page_to_list_to_order(struct=
 page *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 supporte=
d */
>>>>> -             if (folio_test_swapcache(folio))
>>>>> +     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 foli=
o");
>>>>>                       return -EINVAL;
>>>>> +             }
>>>>> +     } else if (new_order) {
>>>>>               /* 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 or=
der");
>>>>>                       return -EINVAL;
>>>>>               }
>>>>> -             /* No split if the file system does not support large=
 folio */
>>>>> -             if (!mapping_large_folio_support(folio->mapping)) {
>>>>> +             /* No split if the file system does not support large=
 folio.
>>>>> +              * Note that we might still have THPs in such mapping=
s due to
>>>>> +              * CONFIG_READ_ONLY_THP_FOR_FS. But in that case, the=
 mapping
>>>>> +              * does not actually support large folios properly.
>>>>> +              */
>>>>> +             if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) &&
>>>>> +                     !mapping_large_folio_support(folio->mapping))=
 {
>>>>
>>>> Shouldn=E2=80=99t this be
>>>>
>>>> if (!IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) &&
>>>>         !mapping_large_folio_support(folio->mapping)) {
>>>>
>>>> ?
>>>>
>>>> When CONFIG_READ_ONLY_THP_FOR_FS is not set, we need to check
>>>> mapping_large_folio_support(), otherwise we do not.
>>>
>>> while CONFIG_READ_ONLY_THP_FOR_FS is not set, that is no way
>>> a large folio can be mapped to a filesystem which doesn't support
>>> large folio mapping. i think
>>
>> That is why we have the warning below to catch this undesired
>> case.
>>
>>> if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS)) is correct.
>>
>> When it is set, khugepaged can create a large pagecache folio
>> on a filesystem without large folio support and the warning
>> will be triggered once the created large pagecache folio
>> is split. That is not what we want.
>
> yes. This is exactly why we need if (IS_ENABLED(CONFIG_READ_ONLY_THP_FO=
R_FS))
> but not if (!IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS)) .
>
> because if (!IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS)), folio is definit=
ely
> pointing to a file system supporting large folio. otherwise, it is a bu=
g.

Oh, got it. Thanks for the explanation. :)

>>
>>>
>>> The below means a BUG which has never a chance to happen if it
>>> is true.
>>>
>>> !IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) &&
>>>         !mapping_large_folio_support(folio->mapping));
>>>
>>>>
>>>>>                       VM_WARN_ONCE(1,
>>>>>                               "Cannot split file folio to non-0 ord=
er");
>>>>>                       return -EINVAL;
>>>>>               }
>>>>>       }
>>>>>
>>>>> +     /* Only swapping a whole PMD-mapped folio is supported */
>>>>> +     if (folio_test_swapcache(folio) && new_order)
>>>>> +             return -EINVAL;
>>>>>
>>>>>       is_hzp =3D is_huge_zero_folio(folio);
>>>>>       if (is_hzp) {
>>>>> --
>>>>> 2.15.2
>>>>
>>>>
>>>> Best Regards,
>>>> Yan, Zi
>>>
>>> Thanks
>>> Barry
>>
>>
>> Best Regards,
>> Yan, Zi


Best Regards,
Yan, Zi

--=_MailMate_B5159FCD-377D-42FA-9377-8FE173AFCB33_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature; name=signature.asc

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

iQJDBAEBCgAtFiEEh7yFAW3gwjwQ4C9anbJR82th+ooFAmZiLVkPHHppeUBudmlk
aWEuY29tAAoJEJ2yUfNrYfqKVp8QAKs/vlj//wZhlJvxTltbn0ONsD0rSanX1Lof
Q3TVjG9exlSdjXG6dcN2HO+xcaRkRoPp4Q9PE7HLbCBFZ7CMGmqFhM0UyTDBweQd
0C6BCNDpsydBqe0QXf2NrtiYDl4zdOfC9yloecW3Wr4bq7Ayiy40OyNICEnWE77a
Rl8hzs3CbTveI7+0n8ln32oSjVtKorGW9YQnH5YHvLH8w/KmxmCkKpXy2DoW8532
rGp2zpCK0hPpjmjYKy3C2sFegBnYxDq0iUJMj46P4StU2nRUxLuegJYceAlwCNd6
U/byZ3lxSG3DOsOw2tKOlL6gBPGhNg9RO92giHu+8hmDTUGD+leUT2jzGcGJnT6G
DjcgOSBKxpmiIjRSttfnLsPwpsgnPSnuvNTkvjV8u/RjJkRA96e8aa50sKvgBJbs
U2IjW3aNw2Au33ZiftWKLa+jUSKHM1g+3liRPjj7ENi5QyyBNfNmZUqICJpXWHyB
SELgHqMLM71+6rgXfLAH/OinyLAPL56ci6xnZ4fO/ZUFLw0Y7KgIS6CHkwxZv4LC
Zlnra/MuiPDTMUlHtUkBP9oBoO/O0sxdUqkNXKhd23gHDmjF89rf1/tIVFCT3GGF
4B/+BywL3cU3xJ+e3rV825exshELpRq/HQuaNp/P7i7DaNw6Z7WVEQnaJWJdz+M2
ybsYnu33
=7gjo
-----END PGP SIGNATURE-----

--=_MailMate_B5159FCD-377D-42FA-9377-8FE173AFCB33_=--