From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id A95F6C61CE8 for ; Sun, 15 Jun 2025 11:14:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CDBC06B0088; Sun, 15 Jun 2025 07:14:30 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CB36A6B0089; Sun, 15 Jun 2025 07:14:30 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C17A06B008A; Sun, 15 Jun 2025 07:14:30 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id B7DCD6B0088 for ; Sun, 15 Jun 2025 07:14:30 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 6D1571D7225 for ; Sun, 15 Jun 2025 11:14:30 +0000 (UTC) X-FDA: 83557376700.14.24185A2 Received: from out30-110.freemail.mail.aliyun.com (out30-110.freemail.mail.aliyun.com [115.124.30.110]) by imf19.hostedemail.com (Postfix) with ESMTP id A07A81A000F for ; Sun, 15 Jun 2025 11:14:27 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=linux.alibaba.com header.s=default header.b=cRj1qZfg; spf=pass (imf19.hostedemail.com: domain of baolin.wang@linux.alibaba.com designates 115.124.30.110 as permitted sender) smtp.mailfrom=baolin.wang@linux.alibaba.com; dmarc=pass (policy=none) header.from=linux.alibaba.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1749986069; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=x8P/eQrzTlQYqpos4YBZX8g3SFUnmIqtpEpLS+GA/Wo=; b=zcUCTmxK0CxDfJq0kyL2904z+Tx6KG5OgohS84odhwWBcHPL2XbUFQwrroRKnCWx7S2Rdc uzdpINXcxZGx27GueY0HAd23kt1i7VdZxAAJ+jcxtw1y0+t9d0wcRq0WclckPz0HzUuUrZ 6EJJxJDHLGMyvuuF9F4ygnzH0cCx/8g= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=linux.alibaba.com header.s=default header.b=cRj1qZfg; spf=pass (imf19.hostedemail.com: domain of baolin.wang@linux.alibaba.com designates 115.124.30.110 as permitted sender) smtp.mailfrom=baolin.wang@linux.alibaba.com; dmarc=pass (policy=none) header.from=linux.alibaba.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1749986069; a=rsa-sha256; cv=none; b=ZcJF5N+TMhP+Jt6y2cIUu47S5DsZoEJVD2UjQSoCu4ixeqSGb4dAkh0QR7GeZFeqGNZQ1l SfHx9LgXExxB7ZS5GkKTn34pxn2AYnfTljoHDHOM6CxZLRW1pDb6thnaaWMK70fVh131fn 3qNXtakJFTZzdiVNJAx4GVsPjTW63+8= DKIM-Signature:v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.alibaba.com; s=default; t=1749986063; h=Message-ID:Date:MIME-Version:Subject:To:From:Content-Type; bh=x8P/eQrzTlQYqpos4YBZX8g3SFUnmIqtpEpLS+GA/Wo=; b=cRj1qZfg5tVu2hmbiEH8OT/RlEke4JmrcxCaDqp0YP9escf1U7ylqcfw7QQzqeancFy2LSJolDwy97B4PRCrN+2ke1h/0QEI5FyZeYFS6XSsgn2My3D9EfBBCG6ANyd/fMtk6MX+rDBA6OxSrJCz8EF6Q2E2+ZfDZoHMrjk07Kg= Received: from 30.39.221.146(mailfrom:baolin.wang@linux.alibaba.com fp:SMTPD_---0WdpiC5g_1749986061 cluster:ay36) by smtp.aliyun-inc.com; Sun, 15 Jun 2025 19:14:21 +0800 Message-ID: <2d175a55-84e3-489f-8c93-66bedaa859a6@linux.alibaba.com> Date: Sun, 15 Jun 2025 19:14:20 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] mm: huge_memory: fix the check for allowed huge orders in shmem To: Lorenzo Stoakes Cc: akpm@linux-foundation.org, david@redhat.com, ziy@nvidia.com, Liam.Howlett@oracle.com, npache@redhat.com, ryan.roberts@arm.com, dev.jain@arm.com, baohua@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <529affb3220153d0d5a542960b535cdfc33f51d7.1749804835.git.baolin.wang@linux.alibaba.com> From: Baolin Wang In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: A07A81A000F X-Rspamd-Server: rspam07 X-Stat-Signature: o5z5pywg4ttbs3u43ccerdey5wt6qmf5 X-Rspam-User: X-HE-Tag: 1749986067-505026 X-HE-Meta: U2FsdGVkX18XG7cRFneELtjhjfx2CgM1I0KGLA0FcMD19IbBzKNPvefaEJbB1RZCFS4JAbJl3qOxqlpCAMQU5xyjnvJebvKQGoBofA3Ga0DrJWcgbahiwiZj+rXSC8pzpDegeB5/uE9pjXdxaM09z7GPD1Pt6tcqycgWV1lEmbQUNoEcG3mYJGp5CwdT6BaRSoRbfgjp9M1PDd+ER59wMvs7GwhcnzKhrlUktxxs6Ngs39aqglhbUHMlnbq+9nxlgceFOKuaSg/C9lWlQk7TCrxw5WFLpC31quKadeQrVm17eL5q2FUHTAam4+xCmfHedPQHrnATnUEgx59wlHMJPMcmDcNOrk2LBIwgbEs3RUf3CEbe6M0E50MRwJTT0b47AT2RuIu3fa3zefwfu0WP4+gn0h2NwLWFStmhe3w2B2C9fI+IYKuxwzb/rfB7REDy3Jd0LygSEFTBRncdc73mV3RIjqmajvvx9hw2al7sKfdmlYsbz0nW0P2eMnVD8IILcVhbzxnRkFxEipLm34A58OS4/QN2F1GI8RLl/5FE23zQbo/y0/guSuqcIRutmdSlCd0jhs/MBLpENZYt91Q6pQeHhPLNrJ+08MjOhUuxNJ/sUNUAE2yITJQC3gFGFDwVJEnEQRuG51X6hLC2UFK59TQ9RC0+STtALqILoGBQewVp6qpspN6xsUVIRLwzDDQdebdz1aV9bZV9LYrmXWmqxx96vNcA/z8/XJcKo+ortdP2RavRPvZ64rZowlzC8Ud7IPwCG+W1GHOl9mFmoOe8vJzNnmJpB1Ryh/sC66er8b7MQoAvIecOK/PRkzRxDTFalKiVZrXs88yZISK4QoNx2zqaJyltC/0u1DYbOMzq5Z0CveASdDRDCwin7Ceaq/esn9h78yRMt6By2Ets9+kR9iy11feDSStG8aIVKMzTXlIhhgkJhpNp7e1muJmFu63oIg/SIXxcEOI= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 2025/6/13 19:16, Lorenzo Stoakes wrote: > On Fri, Jun 13, 2025 at 05:12:19PM +0800, Baolin Wang wrote: >> Shmem already supports mTHP, and shmem_allowable_huge_orders() will return >> the huge orders allowed by shmem. However, there is no check against the >> 'orders' parameter passed by __thp_vma_allowable_orders(), which can lead >> to incorrect check results for __thp_vma_allowable_orders(). >> >> For example, when a user wants to check if shmem supports PMD-sized THP >> by thp_vma_allowable_order(), if shmem only enables 64K mTHP, the current >> logic would cause thp_vma_allowable_order() to return true, implying that >> shmem allows PMD-sized THP allocation, which it actually does not. >> >> I don't think this will cause a significant impact on users, and this will >> only have some impact on the shmem THP collapse. That is to say, even though >> the shmem sysfs setting does not enable the PMD-sized THP, the >> thp_vma_allowable_order() still indicates that shmem allows PMD-sized collapse, >> meaning it might successfully collapse into THP, or it might not (for example, >> thp_vma_suitable_order() check failed in the collapse process). However, this >> still does not align with the shmem sysfs configuration, fix it. > > Can you explain why? > > It's a bit painful to trace through the code paths, but why do you think only > MADV_COLLAPSE will be impacted? Surely everywhere that checks this is? For shmem, thp_vma_allowable_order() and its wrapper are only used in show_smap() and shmem collapse (which includes khugepaged and madvise_collapse()). For shmem collapse, as I mentioned, the impact might not be very significant. For show_smap(), since it will use the 'THP_ORDERS_ALL', it will not affect the results of show_smap(). >> Fixes: 26c7d8413aaf ("mm: thp: support "THPeligible" semantics for mTHP with anonymous shmem") >> Signed-off-by: Baolin Wang > > I can't see how this can be incorrect, as we really should be restricting > ourselves to the orders requested. > > So: > > Reviewed-by: Lorenzo Stoakes Thanks. > >> --- >> Note: this general change is suitable to be split out as a bugfix patch >> based on the discussions in the previous thread[1]. >> >> [1] https://lore.kernel.org/all/86bf2dcd-4be9-4fd9-98cc-da55aea52be0@lucifer.local/ >> --- >> mm/huge_memory.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index d3e66136e41a..a8cfa37cae72 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -166,7 +166,7 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, >> * own flags. >> */ >> if (!in_pf && shmem_file(vma->vm_file)) >> - return shmem_allowable_huge_orders(file_inode(vma->vm_file), >> + return orders & shmem_allowable_huge_orders(file_inode(vma->vm_file), >> vma, vma->vm_pgoff, 0, > > I mean this seems correct to me, but what a massive oversight. > > I wish we had a sensible way of testing this... It might not be easy to write test cases because it requires dynamically toggling the mTHP sysfs setting for shmem. However, as khugepaged supports mTHP collapse in the future, we can try to add more tests. >> !enforce_sysfs > This whole code path is entirely indicative of what a complete mess this whole > thing is. > > The fact shmem separately calls this function is just ugh. I'm talking myself > into some mega refactoring here :) Yes, Shmem has its own separate mTHP sysfs interfaces, with more complex logic :)