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 5978FC48260 for ; Tue, 13 Feb 2024 16:27:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E48E16B0071; Tue, 13 Feb 2024 11:27:58 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id DF25C6B0075; Tue, 13 Feb 2024 11:27:58 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C94226B007E; Tue, 13 Feb 2024 11:27:58 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id B61F46B0071 for ; Tue, 13 Feb 2024 11:27:58 -0500 (EST) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 5C010140C4F for ; Tue, 13 Feb 2024 16:27:58 +0000 (UTC) X-FDA: 81787312236.28.F58D25B Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by imf04.hostedemail.com (Postfix) with ESMTP id EB0C740015 for ; Tue, 13 Feb 2024 16:27:54 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=quicinc.com header.s=qcppdkim1 header.b="W3 o6sjP"; dmarc=pass (policy=none) header.from=quicinc.com; spf=pass (imf04.hostedemail.com: domain of quic_charante@quicinc.com designates 205.220.168.131 as permitted sender) smtp.mailfrom=quic_charante@quicinc.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707841675; a=rsa-sha256; cv=none; b=I8zS91vlcIXNqfCRSknOjZPKOzK1Pu1KFKIWURipCILGLV9agnUcjy0D87jEevQEHhM4ZT lP9Ks6KrGh+lz40i+0KDQzRLHCwfpf92Ub8cVXAxn+rWwZUUP/UBBkYvePGuBOdOr1Ktni 9vtM7ahuaEowRkVmjzkVEFnkW+6MsmQ= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=quicinc.com header.s=qcppdkim1 header.b="W3 o6sjP"; dmarc=pass (policy=none) header.from=quicinc.com; spf=pass (imf04.hostedemail.com: domain of quic_charante@quicinc.com designates 205.220.168.131 as permitted sender) smtp.mailfrom=quic_charante@quicinc.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707841675; 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=gZZNfFALv+w2YYhu1FLg3jywnsLsE9Um5CpTaCn36P4=; b=UXHvMl+4mdgcExLSIaHauUzbeOqcYzBE6YzA3alATLUi7b9ex1WX1DvBbOR119fawWItoK t/zd/dZxF8i5laFi3lB+oHDIxw78onp03TtAoInQm8Wc8ZOL5G+XNhWpnfqmr6E5gr1WyT YM3MwZuCFP+KRPkUT9CyJLX+H8Av0KE= Received: from pps.filterd (m0279865.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.24/8.17.1.24) with ESMTP id 41DEbwlZ004674; Tue, 13 Feb 2024 16:27:46 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h= message-id:date:mime-version:subject:to:cc:references:from :in-reply-to:content-type:content-transfer-encoding; s= qcppdkim1; bh=gZZNfFALv+w2YYhu1FLg3jywnsLsE9Um5CpTaCn36P4=; b=W3 o6sjPQVb0GWe+stoSypTUZxJ/M5lmsXA6vRKCyCFKQJjho0rSb1+pYNYBqJYsIoy x9CpW75bQX23/bSTEMJ1FHSWPWLJDWmtm6PTV44EUHC5QommThwrImtqRs0n8GnU MV3NlFpVne5LMlCW7c88jIWJ33nTdKprDnik7YihurYJaDfljqch3jMwq50R6TM5 tAZDQbJw31GCZ+Ah4JaePnJI6yEeJm2sUzVNw3upRTWq8bZsNL4+Q2nCrymZUlEV 7LoFztwANTTQWw7XizRmcew9C+7RgzB6YGKUQb3kjyyWsZptwNBblwsloNjvTCLN iIvHl9VRbPjSlvK0cA8w== Received: from nalasppmta01.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3w7hewbbb3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 13 Feb 2024 16:27:45 +0000 (GMT) Received: from nalasex01a.na.qualcomm.com (nalasex01a.na.qualcomm.com [10.47.209.196]) by NALASPPMTA01.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 41DGRiEB015130 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 13 Feb 2024 16:27:44 GMT Received: from [10.216.11.8] (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.40; Tue, 13 Feb 2024 08:27:39 -0800 Message-ID: <8620c1a0-e091-46e9-418a-db66e621b9c4@quicinc.com> Date: Tue, 13 Feb 2024 21:57:34 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH] mm/huge_memory: fix swap entry values of tail pages of THP Content-Language: en-US To: David Hildenbrand , , , , , , CC: , , # see patch description References: <1707814102-22682-1-git-send-email-quic_charante@quicinc.com> From: Charan Teja Kalla In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nalasex01a.na.qualcomm.com (10.47.209.196) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: v8W7XKziphPLBlxM3EVEsOt7amI8esis X-Proofpoint-ORIG-GUID: v8W7XKziphPLBlxM3EVEsOt7amI8esis X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2024-02-13_09,2024-02-12_03,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 phishscore=0 adultscore=0 bulkscore=0 clxscore=1015 mlxlogscore=999 priorityscore=1501 malwarescore=0 suspectscore=0 mlxscore=0 lowpriorityscore=0 impostorscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.19.0-2401310000 definitions=main-2402130129 X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: EB0C740015 X-Stat-Signature: ecd8y76czqmjhtewkbx9ecgeuhmj9zsb X-HE-Tag: 1707841674-936607 X-HE-Meta: U2FsdGVkX19UqR8pcXYacYI9qScmtnLXsoKPc29S+kz1Zsu1TPV8AZc1hQEF5p8AMpJVwrKWKgrR6ARNm/ZfTUuVHV63wxdiITE46+uMTbnC8yGFwfg1WTxkyIS97vEtHAm7JoHKz1gMuRUWssmoGQcx4a0aU0Fget5Nb6kgthWcxkD2vWblMEjYdQQG9S3AJ0Ed5PLnn+xgmOV0jjAojaQDKQZrMPvjy+9X2ttGHnZSD2SysJkT0hV12f5dMwc2PKNDbuFWgfQetiuUXHFWgR4ip0rAO8DwZd4sPRF/bOIVi1Ec28JPdeDJopTON6SWkchWmGIF7AzKESOuF/j95k2Qpatd0tgw6MUNJgDzk5RqIKdnBFjk1aLKc2u8VxJzgvtjmnJY0+x2B71PCPBQFWcxTJavQLT4pp4iFAtkX5Br2LTHytE5Yql3MYPsZ3FZsqtyLIIi6wtKmzpiRt2Gzvati/jxTztwOFadUlr+o0AiUrQ8Vt6xNvzWpkt/c3XhlOm5EuUkPyCaN4f8B8oV58iRABQF8g62mf/RJC//iDjoZrv1TjqM1M0ADWzxwXYVvMQIPkWGrVWygofQHOZagz6aAT4G0AIREkyJuopvWHG8/LgS+RlVUMUKjwEDhdl7zbNYhTifpWPdd/c9gwfccuO7FGTh4yUPk6fPb97xzuYlainbpS9ECbsfNmocspHH0E4CcFNExjacyh1NDEOqBb+3SrXZiXWbirwLwi4oTquZ8xrYtGWHn81uwoRj8SuBmJ0vsYePL4OCg0cPCtbvG9+BnoT76tet30effgqcNozrBDkWRijTK5hhx38oKVHJqbVdvFKClpetBWsRtx+V7cZNGTX7HaTQQE72KT9yGV0Yj2Js3yZfiiIzPvyH5X0mOVCouR9TqlTkVXZk7aLJ8koGKFUrzEC2kmFUFWBhmIa6z5FhUV/OGea9FjHMH4arCXJGovPUy50iVMTKtB5 ihHl6Am2 lHa7Jha7k7pjAEhcGmwWN5Li9xCM81k2IL0NjQiFuUhj+cpZOx+9ik7s1jMHEKeKshGZmoQ1ftndfskc7uQc7JX3t25u1x5wbHYE4eUBYMsvgm67IkM857dfi9ogAywba2s47Gm3jf/vDJqoLuTeDIGWSe/k3aBO1UYl86QBYrECr+MBjGRXAcaKoL+xsv/rRasFZGEzXzH89s3Yr2z8GwOUhFXVDKgkV89CrcxnghBpWQu2vHxqPCYr23/NU6x6K2jPdRPsXcbbKPSxebvjmvIms1sgXCQNeb55Cs4E+UdO3dJxVcZhZQ+LfZZws46neK1lZ2Tp5qMdFolSu6q5dGLJ4p5Fa83PiMvbuUf0PIHavaO0UPaipRHX0DGHQndskY5VJOMEfRwLZlw0rLy+oix6BKK+x8BtONswmsOC7g0VE1NuHvqr9QLcKj410NCD5RBxSQnz9tS8bMU4= 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: Thanks David!! On 2/13/2024 2:55 PM, David Hildenbrand wrote: > On 13.02.24 09:48, Charan Teja Kalla wrote: >> An anon THP page is first added to swap cache before reclaiming it. >> Initially, each tail page contains the proper swap entry value(stored in >> ->private field) which is filled from add_to_swap_cache(). After > > This is a stable-only fix? In that case, it make sense to indicate that > in the patch subject [PATCH STABLE vX.Y]. > Noted. Will take care of this from next time. > But it's always odd to have stable-only fixes, the docs [1] don't cover > that (maybe they should? or I missed it). > Not sure If the below rules implicitly allows: - It or __an equivalent fix__ must already exist in Linus' tree (upstream). - It cannot be bigger than 100 lines, with context. > [1] > https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst> > So we are migrating a THP that was added to the swapcache. Do you have a > reproducer? > Yes, a bunch of them can be reproduced daily in our device farm which is being tested on 6.1. >> migrating the THP page sitting on the swap cache, only the swap entry of >> the head page is filled(see folio_migrate_mapping()). >> >> Now when this page is tried to split(one case is when this page is again >> migrated, see migrate_pages()->try_split_thp()), the tail pages >> ->private is not stored with proper swap entry values.  When this tail >> page is now try to be freed, as part of it delete_from_swap_cache() is >> called which operates on the wrong swap cache index and eventually >> replaces the wrong swap cache index with shadow/NULL value, frees the >> page. > > But what if we don't split the THP after migration. Is there anything > else that can go wrong? > > It's sufficient to look where upstream calls page_swap_entry(): > > For example, do_swap_page() will never be able to make progress, because > the swap entry of the page does not correspond to the swap entry in the > PTE? It can easily fault on a swap PTE that refers a THP subpage in the > swapcache. > > So unless I am missing something, only fixing this up during the split > is insufficient. You have to fix it up during migration. > I think you are right....My understanding is that below can make the do_swap_page() will never be able to make progress: do_swap_page: entry = pte_to_swp_entry(vmf->orig_pte); ...... if (unlikely(!folio_test_swapcache(folio) || page_private(page) != entry.val)) goto out_page; This current patch is posted looking at the series in upstream that looks simple, but that turn out to be not correct. BTW, the fix we've tested internally is below discussed in [1]. diff --git a/mm/migrate.c b/mm/migrate.c index 9f5f52d..8049f4e 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -427,10 +427,8 @@ int folio_migrate_mapping(struct address_space *mapping, folio_ref_add(newfolio, nr); /* add cache reference */ if (folio_test_swapbacked(folio)) { __folio_set_swapbacked(newfolio); - if (folio_test_swapcache(folio)) { + if (folio_test_swapcache(folio)) folio_set_swapcache(newfolio); - newfolio->private = folio_get_private(folio); - } entries = nr; } else { VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio); @@ -446,6 +444,8 @@ int folio_migrate_mapping(struct address_space *mapping, /* Swap cache still stores N entries instead of a high-order entry */ for (i = 0; i < entries; i++) { + set_page_private(folio_page(newfolio, i), + folio_page(folio, i)->private); xas_store(&xas, newfolio); xas_next(&xas); } [1] https://lore.kernel.org/linux-mm/69cb784f-578d-ded1-cd9f-c6db04696336@quicinc.com/ >> >> This leads to the state with a swap cache containing the freed page. >> This issue can manifest in many forms and the most common thing observed >> is the rcu stall during the swapin (see mapping_get_entry()). >> >> On the recent kernels, this issues is indirectly getting fixed with the >> series[1], to be specific[2]. >> >> When tried to back port this series, it is observed many merge >> conflicts and also seems dependent on many other changes. As backporting >> to LTS branches is not a trivial one, the similar change from [2] is >> picked as a fix. > > The fix is in > > commit cfeed8ffe55b37fa10286aaaa1369da00cb88440 > Author: David Hildenbrand > Date:   Mon Aug 21 18:08:46 2023 +0200 > >     mm/swap: stop using page->private on tail pages for THP_SWAP > >> >> [1] https://lore.kernel.org/all/20230821160849.531668-1-david@redhat.com/ >> [2] https://lore.kernel.org/all/20230821160849.531668-5-david@redhat.com/ >> >> Closes: >> https://lore.kernel.org/linux-mm/69cb784f-578d-ded1-cd9f-c6db04696336@quicinc.com/ >> Fixes: 3417013e0d18 ("mm/migrate: Add folio_migrate_mapping()") >> Cc: # see patch description, applicable to <=6.1 >> Signed-off-by: Charan Teja Kalla > > 3417013e0d18 went into 5.16. > > cfeed8ffe55b3 went into 6.6. > > So only 6.1 is affected. > I tried to dig why the older kernels don't have this issue. The issue persists on the older kernels too, AFAICS. See, migrate_page_move_mapping(). So, The Fixes: tag too is unique for 6.1 kernel. And older LTS kernel requires different tag it seems.. > > Isn't there a way to bite the bullet and backport that series to 6.1 > instead? My worry is that, because of merge conflicts, not sure If It can end up in inducing some other issues. Although we didn't test THP on older kernels, from the code walk, it seems issue persists to me on older to 6.1 kernel, unless I am missing something here. So back porting of this series to all those LTS kernels, may not be a straight forward? So, I am really not sure of what is the way forward here... > Thanks.