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 B0C56C77B61 for ; Mon, 24 Apr 2023 15:37:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E68736B0071; Mon, 24 Apr 2023 11:37:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E18D56B0074; Mon, 24 Apr 2023 11:37:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CB9756B0078; Mon, 24 Apr 2023 11:37:45 -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 B87BB6B0071 for ; Mon, 24 Apr 2023 11:37:45 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id A9C7D14026C for ; Mon, 24 Apr 2023 15:37:44 +0000 (UTC) X-FDA: 80716689648.26.17ADA54 Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by imf11.hostedemail.com (Postfix) with ESMTP id 4FECF40019 for ; Mon, 24 Apr 2023 15:37:41 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=ibm.com header.s=pp1 header.b="ON6gp/ar"; spf=pass (imf11.hostedemail.com: domain of tsahu@linux.ibm.com designates 148.163.156.1 as permitted sender) smtp.mailfrom=tsahu@linux.ibm.com; dmarc=pass (policy=none) header.from=ibm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1682350661; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=HdggoQsHxwYoIKSgaW8mjgzyiJ7jKhJof5hERdguU8Q=; b=C4z4udj61WAakSjHT7zvLFdlQFSaWDGl9gK0ldADGLLsif4l/v8CipR6Su03t2ZYorKGqH QSOU35O/xSpcETy7cDv3WMWUfj6a3xE7nyVYiiUGbfRhVu5fOtgzgcWxEvs6NBMPJ6SF37 NIczzqe0yWcXC9iV57Wgr59GDWt60ko= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=ibm.com header.s=pp1 header.b="ON6gp/ar"; spf=pass (imf11.hostedemail.com: domain of tsahu@linux.ibm.com designates 148.163.156.1 as permitted sender) smtp.mailfrom=tsahu@linux.ibm.com; dmarc=pass (policy=none) header.from=ibm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1682350661; a=rsa-sha256; cv=none; b=NS9ZlKJ3pzDGGtCnjmHDWNGr4WAC+O7/4FC+sEERWSAX3gTAwt07u/DZ0lJfgbkONefh7c 10HZMv5938Xl340VswnEfpg+LI5fNV+QMmEcZFXQfk62AoP/xMnRExYgOojBNCx5OdgapY ghhWO6rqBBec6j2icovKE9RZ8ycdV9o= Received: from pps.filterd (m0353727.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 33OFaI8X000941; Mon, 24 Apr 2023 15:36:41 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=from : to : cc : subject : in-reply-to : references : date : message-id : content-type : mime-version; s=pp1; bh=HdggoQsHxwYoIKSgaW8mjgzyiJ7jKhJof5hERdguU8Q=; b=ON6gp/artNflu0D6S+eFqhgQqrcRJqPc2CTxYn/PQUY4TbUFFdFenuEnN7qeFpMU7n4D DCgs2PPwzmpBVr7J8X1GKBE5G6dwW6i1426CU9kqFJMtiyujCaPMzuJlcVj/4fLyNF6z bpI5qmjA0/XY3+Dtyg40EF2MhBvea6DwzbGgBXaYt+5HU2TMyloCAr+fIRTWXiCUYLAI gHrQqVz1zeQafDRHgSB/+nJ7YI1r3JZ7e6MCdGMhwEIGkM/2DbxYo8Cqig7WwsIBzplg 84rNI3l7/+EZgdiEiBUvWZM6e4BU0eFoN6fIpl2E2LQ0UhRn/MqtB62JRPlbXVdYCqdp JQ== Received: from ppma03fra.de.ibm.com (6b.4a.5195.ip4.static.sl-reverse.com [149.81.74.107]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3q47d5c1u1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 24 Apr 2023 15:36:30 +0000 Received: from pps.filterd (ppma03fra.de.ibm.com [127.0.0.1]) by ppma03fra.de.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 33ODxXJ6014897; Mon, 24 Apr 2023 15:31:33 GMT Received: from smtprelay04.fra02v.mail.ibm.com ([9.218.2.228]) by ppma03fra.de.ibm.com (PPS) with ESMTPS id 3q477710fm-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 24 Apr 2023 15:31:33 +0000 Received: from smtpav01.fra02v.mail.ibm.com (smtpav01.fra02v.mail.ibm.com [10.20.54.100]) by smtprelay04.fra02v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 33OFVUqR37028176 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 24 Apr 2023 15:31:30 GMT Received: from smtpav01.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 21F7A20043; Mon, 24 Apr 2023 15:31:30 +0000 (GMT) Received: from smtpav01.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id CD58120040; Mon, 24 Apr 2023 15:31:25 +0000 (GMT) Received: from tarunpc (unknown [9.43.70.159]) by smtpav01.fra02v.mail.ibm.com (Postfix) with ESMTPS; Mon, 24 Apr 2023 15:31:25 +0000 (GMT) From: Tarun Sahu To: Matthew Wilcox Cc: linux-mm@kvack.org, akpm@linux-foundation.org, muchun.song@linux.dev, mike.kravetz@oracle.com, aneesh.kumar@linux.ibm.com, sidhartha.kumar@oracle.com, gerald.schaefer@linux.ibm.com, linux-kernel@vger.kernel.org, jaypatel@linux.ibm.com Subject: Re: [PATCH] mm/folio: Avoid special handling for order value 0 in folio_set_order In-Reply-To: References: <20230414194832.973194-1-tsahu@linux.ibm.com> Date: Mon, 24 Apr 2023 21:01:21 +0530 Message-ID: <877cu15mba.fsf@linux.ibm.com> Content-Type: text/plain X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: a_eVeiLGKeWZ_lcJSOZMPKnEumvBOeK- X-Proofpoint-GUID: a_eVeiLGKeWZ_lcJSOZMPKnEumvBOeK- X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-04-24_09,2023-04-21_01,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=0 phishscore=0 spamscore=0 impostorscore=0 bulkscore=0 lowpriorityscore=0 adultscore=0 priorityscore=1501 clxscore=1015 mlxscore=0 malwarescore=0 mlxlogscore=679 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303200000 definitions=main-2304240140 X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 4FECF40019 X-Stat-Signature: 9rxk54nyxpqjtfoc8jkwbhm49z1iqys1 X-HE-Tag: 1682350661-663008 X-HE-Meta: U2FsdGVkX1+OFkCUPwThETNEMowp9T23wLH9GN18Qqmc4fQHRJNQhUWjbhVKSdSJKNR9bgIPSeBt+B7byTNFY4Ij9o6j5tVqqpdh6Ppb6DEpFXSUq9wV3QcQSYQHGtgk5RFbU33dhrzQ/0pOr/BD9qzy3KcxxtXUNzBJ0it+lWJ1wkPqP5+91GCCaOMiun98b8OVkFTBr7MCb5pIp32PFjnWiZ7Vz1W3NOki7ZzbokC6W1bJSlDMDZLVQnOeMwCOCB6C7dJ0Sa9COKdIxEL8SukID04Zazf87a36FA2C7voc9ST7PNECyhFwY+ivCSy/xNgsCI/aJVPpNzzVcrYCmypbAKsXdjI2GQ6Qm8JsXypee7Nwo8PrKb+qkujHJLAFT3Iqnsnvy3gv1VGAlFCbNsm7Gi76PzfXVzyjf6AifhObTcCMjFpH/iNlYMXASz7UCcPb9s+6ucPNCK8Mw1EqfLpV6lRcxXNfizW6pYlKQfjMXuDBIUVAX95SjwRrlQn/lNIc3/KJEWssZiBc6mdvvEkYnxYkhx0orgEbECZQ8j+oR1NwH0BD0gOiyFkWFTVgOyOni4MLsg7rHOnBUPrYBH3ancE4y8/fCA8eFJpwTXO/NQAQWYQRNpzyoCzjzqM7cpEJXu0rt7a2A4XjepLbsgdsuPV2LrE1L7sVY1qLzi+U/ZKaBGzcd4EL++2BRJA2XqpFKlswtaNPN8gHBZDyab4XIO1I7t/NglSWYki2lJ6ob7dS3Lhj1n4D0NecQE+cKRx+AoKajzBPNWiwhncxhfM5rZFCPyr3Ohh5QwL1WhvI12CzHMIf13O8628VGW5Os3luO6A2WVNZy8DryQaTIEO37XZ6ewUWOmhn63dwTmtxCG8uG/yUqsUwrLH6ehsXF0Xb/ce+BD8vY4+wu0nsTrd4Ra1m5JlzZGk0FHM4qz6uWF3EE4VBQ9Dy2UkXsjTgMsZGyu1Cgzz8loKRLD1 DKYwqS7w HCmUCXBNDgGqwyxbTZErjDd00fs4CTAKvoNHN/pv5v3pdNWgWu8mG/ZOT295mULM6E3vNruawmOWQxWgyqsJVwHnhDmX14rMxlXf/6nHpYB0x4FBWglhGCheblxUff9EpwuzirCyML0xx+n/6wzdBhT5G5A9tgJNbUJomHM6xNWCQWgjnu/pX2kohViFQ4V/36mF2xQvudM/5U14gHKgl8HeiqYSqX1KDMdsMLj0/wgMRl57UuFdc86suHFUWRiw8Ae/2iabmPOZn8IM= 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: Hi, Mathew, I am not sure If I was clear about my intention behind the patch. Here, I attempt to answer it again. Please lemme know if you agree. Matthew Wilcox writes: > On Sat, Apr 15, 2023 at 01:18:32AM +0530, Tarun Sahu wrote: >> folio_set_order(folio, 0); which is an abuse of folio_set_order as 0-order >> folio does not have any tail page to set order. > > I think you're missing the point of how folio_set_order() is used. > When splitting a large folio, we need to zero out the folio_nr_pages > in the tail, so it does have a tail page, and that tail page needs to > be zeroed. We even assert that there is a tail page: > > if (WARN_ON_ONCE(!folio_test_large(folio))) > return; > > Or maybe you need to explain yourself better. > In the upstream, I don't see folio_set_order(folio, 0) being called in splitting path. IIUC, we had to zero out _folio_nr_pages during freeing gigantic folio as described by Commit ba9c1201beaa ("mm/hugetlb: clear compound_nr before freeing gigantic pages"). I agree that folio_set_order(folio, 0) is called with folio having tail pages. But I meant only that, in general, it is just confusing to have setting the folio order to 0. With this patch, I would like to draw attention to the point that there is no need to call folio_set_order(folio, 0) anymore to zero out _folio_order and _folio_nr_pages. In past, it was needed because page->mapping used to overlap with _folio_nr_pages and _folio_order. So if these fields were left uncleared during freeing gigantic hugepages, they were causing "BUG: bad page state" due to non-zero page->mapping. Now, After Commit a01f43901cfb ("hugetlb: be sure to free demoted CMA pages to CMA") page->mapping has explicitly been cleared out for tail pages. Also, _folio_order and _folio_nr_pages no longer overlaps with page->mapping. struct page { ... struct address_space * mapping; /* 24 8 */ ... } struct folio { ... union { struct { long unsigned int _flags_1; /* 64 8 */ long unsigned int _head_1; /* 72 8 */ unsigned char _folio_dtor; /* 80 1 */ unsigned char _folio_order; /* 81 1 */ /* XXX 2 bytes hole, try to pack */ atomic_t _entire_mapcount; /* 84 4 */ atomic_t _nr_pages_mapped; /* 88 4 */ atomic_t _pincount; /* 92 4 */ unsigned int _folio_nr_pages; /* 96 4 */ }; /* 64 40 */ struct page __page_1 __attribute__((__aligned__(8))); /* 64 64 */ } ... } So, folio_set_order(folio, 0) can be removed from freeing gigantic folio path (__destroy_compound_gigantic_folio). Another place, where folio_set_order(folio, 0) is called inside __prep_compound_gigantic_folio during error path. Here, folio_set_order(folio, 0) can also be removed if we move folio_set_order(folio, order) after for loop. Also, Mike confirmed that it is safe to move the call. ~Tarun >> folio->_folio_nr_pages is >> set to 0 for order 0 in folio_set_order. It is required because >> _folio_nr_pages overlapped with page->mapping and leaving it non zero >> caused "bad page" error while freeing gigantic hugepages. This was fixed in >> Commit ba9c1201beaa ("mm/hugetlb: clear compound_nr before freeing gigantic >> pages"). Also commit a01f43901cfb ("hugetlb: be sure to free demoted CMA >> pages to CMA") now explicitly clear page->mapping and hence we won't see >> the bad page error even if _folio_nr_pages remains unset. Also the order 0 >> folios are not supposed to call folio_set_order, So now we can get rid of >> folio_set_order(folio, 0) from hugetlb code path to clear the confusion. > > ... this is all very confusing. > >> The patch also moves _folio_set_head and folio_set_order calls in >> __prep_compound_gigantic_folio() such that we avoid clearing them in the >> error path. > > But don't we need those bits set while we operate on the folio to set it > up? It makes me nervous if we don't have those bits set because we can > end up with speculative references that point to a head page while that > page is not marked as a head page. It may not be a problem, but I want > to see some air-tight analysis of that. > >> Testing: I have run LTP tests, which all passes. and also I have written >> the test in LTP which tests the bug caused by compound_nr and page->mapping >> overlapping. >> >> https://lore.kernel.org/all/20230413090753.883953-1-tsahu@linux.ibm.com/ >> >> Running on older kernel ( < 5.10-rc7) with the above bug this fails while >> on newer kernel and, also with this patch it passes. >> >> Signed-off-by: Tarun Sahu >> --- >> mm/hugetlb.c | 9 +++------ >> mm/internal.h | 8 ++------ >> 2 files changed, 5 insertions(+), 12 deletions(-) >> >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index f16b25b1a6b9..e2540269c1dc 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c >> @@ -1489,7 +1489,6 @@ static void __destroy_compound_gigantic_folio(struct folio *folio, >> set_page_refcounted(p); >> } >> >> - folio_set_order(folio, 0); >> __folio_clear_head(folio); >> } >> >> @@ -1951,9 +1950,6 @@ static bool __prep_compound_gigantic_folio(struct folio *folio, >> struct page *p; >> >> __folio_clear_reserved(folio); >> - __folio_set_head(folio); >> - /* we rely on prep_new_hugetlb_folio to set the destructor */ >> - folio_set_order(folio, order); >> for (i = 0; i < nr_pages; i++) { >> p = folio_page(folio, i); >> >> @@ -1999,6 +1995,9 @@ static bool __prep_compound_gigantic_folio(struct folio *folio, >> if (i != 0) >> set_compound_head(p, &folio->page); >> } >> + __folio_set_head(folio); >> + /* we rely on prep_new_hugetlb_folio to set the destructor */ >> + folio_set_order(folio, order); >> atomic_set(&folio->_entire_mapcount, -1); >> atomic_set(&folio->_nr_pages_mapped, 0); >> atomic_set(&folio->_pincount, 0); >> @@ -2017,8 +2016,6 @@ static bool __prep_compound_gigantic_folio(struct folio *folio, >> p = folio_page(folio, j); >> __ClearPageReserved(p); >> } >> - folio_set_order(folio, 0); >> - __folio_clear_head(folio); >> return false; >> } >> >> diff --git a/mm/internal.h b/mm/internal.h >> index 18cda26b8a92..0d96a3bc1d58 100644 >> --- a/mm/internal.h >> +++ b/mm/internal.h >> @@ -425,16 +425,12 @@ int split_free_page(struct page *free_page, >> */ >> static inline void folio_set_order(struct folio *folio, unsigned int order) >> { >> - if (WARN_ON_ONCE(!folio_test_large(folio))) >> + if (WARN_ON_ONCE(!order || !folio_test_large(folio))) >> return; >> >> folio->_folio_order = order; >> #ifdef CONFIG_64BIT >> - /* >> - * When hugetlb dissolves a folio, we need to clear the tail >> - * page, rather than setting nr_pages to 1. >> - */ >> - folio->_folio_nr_pages = order ? 1U << order : 0; >> + folio->_folio_nr_pages = 1U << order; >> #endif >> } >> >> -- >> 2.31.1 >>