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 2BBDDC3DA4A for ; Thu, 22 Aug 2024 20:20:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6FE186B0199; Thu, 22 Aug 2024 16:20:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6AE7A6B019A; Thu, 22 Aug 2024 16:20:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 54E1E6B019C; Thu, 22 Aug 2024 16:20:41 -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 317DB6B0199 for ; Thu, 22 Aug 2024 16:20:41 -0400 (EDT) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id D1D551206DF for ; Thu, 22 Aug 2024 20:20:40 +0000 (UTC) X-FDA: 82480999440.20.A822EEC Received: from mail-ej1-f49.google.com (mail-ej1-f49.google.com [209.85.218.49]) by imf28.hostedemail.com (Postfix) with ESMTP id E00CFC0017 for ; Thu, 22 Aug 2024 20:20:38 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=UWhtQNFs; spf=pass (imf28.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.49 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1724357998; a=rsa-sha256; cv=none; b=ACkYTBE70K+dmgycTeqctpJ0FiI/8o/Y371A3lqbRt3SQhDsWovbiCqmhL/GIbNJ4O6gHK 33QlJIBTAIs6VFzAQtJ4re7mHKiZStJ1OqGp+DK1vut3eAmDo/M66tf01XKwjjGeOp19Np TRAYyIcR9DoqlsT8L0m+fc2Z9Sx8u/c= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=UWhtQNFs; spf=pass (imf28.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.49 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1724357998; 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=z4BpYDCBRf3GhJ+JefscMjttTkRWb7SwwgJbqDtQX14=; b=qQOpyCz4pq4yxaY1Mqyyzl406KVYgDj0u70mhJe93wqPt0iM+SMoXL1YQSSLG48FRCXJGX rxEcfDCHgwvrceSJXDFruq4Ev5oyOgtQ2MrXXjCx25QYn66ut/pE/i8kliqyRlK95DdqR0 v5K3prmltyv+0mTlILNuYuZ1Mgvn8Ac= Received: by mail-ej1-f49.google.com with SMTP id a640c23a62f3a-a7ab5fc975dso149414166b.1 for ; Thu, 22 Aug 2024 13:20:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1724358037; x=1724962837; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=z4BpYDCBRf3GhJ+JefscMjttTkRWb7SwwgJbqDtQX14=; b=UWhtQNFsxBAtz5ABgoLnpu0GvvW7U69RVJpd3qY9rhAd01v+axlXEEsUS2FPy5vj2s q0wMr2Ejs3yMl48HPZt5XR3qiO76MAU0y3b3WJHj4pvJLLtnu63z8lRJSKc9R/JrP2dw NSqj4UiuYgxzvB/ZKxL15H2Qcwdk0JGz3Mb57475pMVT4uhhyuLvnSXYygLh++FXUMfs jvzqZpBLSdbff3yUdsVHm2oMSWjKiqzYdp5Z7GjsDKObYigeWa+rT+B8pRqh4NTXdbPv ohfUyqwboXex2pcEB44SAhyQZJsVsTuZ/H1ryyKqocQNJ2vouRwk/FcTuacaQPH2IKPe xFVA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724358037; x=1724962837; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=z4BpYDCBRf3GhJ+JefscMjttTkRWb7SwwgJbqDtQX14=; b=krsUaHttylKkzkM4JWXm9U37ijmAJS1tJxlflqLxGxobuu/vEGUaFQBv/WBVPVM/1L epTeKbCj+OqtAd8DSJsSjQxLwrw2gtJ8L9YDlNMUMsh8KLq26/ac+d2W2R5Q0rI8XHiZ 52KsqR+fejuwfoAKUbOd+uW5y6+eCj6cbR45P94rrJot/WjYQPnUIjL7yc7gdZHDELwa FrXjQHm8BggARWf+7Ki5wnxKC4elAKZX3n4MGaGPregEPEDd1NO3mC6kSPXMdIuMyPZL uoLWV51FeNudZvT/I/yMHPpInEZDmt4KpOwMZu114vNa64Li0aJY4EqEMoZ3ehT/R4ot FtqA== X-Forwarded-Encrypted: i=1; AJvYcCVSrk4/2bR1IC/W4oGeq2PcPx1Eiw8S77J+0vRJdamoSPZTnlEVxojk1MNDeCTshw2hOfZQQuPlHA==@kvack.org X-Gm-Message-State: AOJu0YykhyQ0PqYzPwNmrHVq9kbiP+ZUnJMmCp/PeicFxDx00BDO21xy ZwpOb328oqtmdgKSYU5Cst84gtPUNqpQr7LhlIEZsWO/qcemS/BI3ihV2rXSpa8Q8yNDWncFafc 1YT03Tt5npgmgJLmYPilPdpjerWI/WdoV8iVe X-Google-Smtp-Source: AGHT+IGvYH5gdd4Q+SZLHa/8b3nOfvz1yRofw3F7hT4vkXLiQL5nXUvsI6pXMy1/wquOoEKgKfmfTVpkNd8K1aV3vrY= X-Received: by 2002:a17:907:9483:b0:a7a:8e98:890d with SMTP id a640c23a62f3a-a866f2fe311mr620238566b.16.1724358036265; Thu, 22 Aug 2024 13:20:36 -0700 (PDT) MIME-Version: 1.0 References: <20240821054921.43468-1-21cnbao@gmail.com> In-Reply-To: From: Yosry Ahmed Date: Thu, 22 Aug 2024 13:20:00 -0700 Message-ID: Subject: Re: [syzbot] [mm?] WARNING in zswap_swapoff To: Chris Li Cc: Kairui Song , Barry Song <21cnbao@gmail.com>, akpm@linux-foundation.org, chengming.zhou@linux.dev, hannes@cmpxchg.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, nphamcs@gmail.com, ryan.roberts@arm.com, syzbot+ce6029250d7fd4d0476d@syzkaller.appspotmail.com, syzkaller-bugs@googlegroups.com, ying.huang@intel.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: 47qg8hoxemrwryuzo885uakic9w357tb X-Rspamd-Queue-Id: E00CFC0017 X-Rspam-User: X-Rspamd-Server: rspam10 X-HE-Tag: 1724358038-559757 X-HE-Meta: U2FsdGVkX18FObL8P5AL0Lqojew7HMLgx3l7yZEFEHlY/M1K12bELuKmdOYC/ZMaRfkbR7YiYqandB/pCQSWoRA611bS8jyV4RKsJsQyyO4fT3yrze/lR4r0wQerXG0nFOLxrzeVbTw0PvvyunGW/yppwJKz5r/pDmUjdeohkwrIQr2WZm2thnizi1GS1zr1X0/uol/dylDwRWSew4VgvGnsldxpqDMLkzs0gtBbaWbezIO18jxzCb4vyOkDY85d1x82mu1lBraRlbcPQ4MZrKTrCy13DbcZusdvghGbM1T0O3AKXgAGQTMDdHTtiHB6FwwKQjImLjHvd4SJPSB8UUCJlOvESH/qGbkryhumv7QfFxjm7c86q+ChlzYl5WLyCBpM9zAGoMhs6D1LElR1Ona667qxlm68rR5h3tl56Zsk+b+6CkyOFEOQYtr1Ds90BeZMsU2d1NfSIdQtyyvJ7FnuAbWScytBOo+w8R6KCo9cP6GM1wDsDHHhWRXIt1UQgHocw3+js00HwYVeGwkNvnNAwzUjH68+huLgR/E+1kICkwzFrSwKHE/Sny7OyA8FcHamAeaDLAensOZVOiwN/PtR20ngcYm5aEFol2OofoC5+amxbwTcUUHZCIHxUQMDyOsrnM3R/uiGoYVyXoJ82TF/Gq1bz1YpfMijgeV481dNNYHpB9j9U1RvJj2Frvv+Qa+oCoE0w85ruNIiMb88S7fypewG/TRRFnPxhJmlRRgCLQEBYvZKthDY25inNWy1cpQhfkkjza/WNhu/Xqogu+3xiSb7IIKCLeFOP/nSdxomqbhAbyMZqvgWv0LS+5fsWc8mh8Zd5EeWT+YzovowyrqFX+DkWooH0JGYEoVgSWBAH6xh4Ic9/EzzF7Yz9bFisujzucSdOhyIdwFHmhxCSfnDAfqwWXp+MHzyuX5GIOulbWBHkpKfMyT4uQUqy/rS2mwN77RKm5OUpf7Awlx JSSbZyYy Uf4gu/wZdfRTDd/fF3/3bGEvZRGHf3+Mog4XnoIkZun6HDoDfdyIVu4hnucGypJLsTg3MxOvJCKBMhHKhkG4Dl7bOLTLmC4c/brIPW1kZHpxIqQlMtomI68FRwBG7YrIqDExjpRPjn5kFC634KGIfW4Sz1lZIoGKYtYrtSa5VXQ9r6tY58/fQN4rtUB69LfpHL56l8/RH2Lz5fyiiVl2aR40yUGlK2BsBoekZTS7I6BD4TWVuzLkIQTXEFumDoo7uJvGwQw1e/OCjVchVW90KLzbsQNPqfSuOqHIvabBCwddtF97cPQ7a5F95undwLwa2zPpShUWTNoP6XSQTiHCNSCcRksqGb1TetdkrEXnNzeKkCB2XODxg1pvUM6ceUN6k7iUSY6KjuZFaobgqZac4Dvi4k911rcLvw23UIILCVxEKD6dd2ArASvNe4KGh1VS9/2HZ4obKSsTA70S31Vq0wo7Y+FaWIdLdGS/zdDvQZbXuGb6uaOOZPIlEOQ5UoKVSKKY5TWQeZUdrJj5Y9eqkTadzqX4my/MNWPuTgjLRat3AXpeF9XOegCITCkZXXq7T7HRNKkYtK7z4zNDS11YdJqZWOIX4ug2oM3Jd2TZK5tWwR9UgHfPjhuONaLMoud6CNEgBguZorLgGnxkZtbBgTdo1XcqXk1h2mU4bZOvTC7pJQrs+mgkc5hOhho0AGsL1YeZikuYaAIpWgewnR2Dngt/f3OjUCqs0rXbIb7w3QVZPpb85Ptm5llReoTqVG7G4+dfd7zkTxmQbZIMX6jntYvnCCJaVftFcmMcdliZCZyKWpS4H09KB+eNWsL2GAfg5a7APnwczpOKL6t3Xn5hiFBXUS1TWTr6ZRQIS6AXg/GEkmPvOjeonXJ9CAXaCBXNSr+3Re0Hj2k+QLiY= 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 Thu, Aug 22, 2024 at 1:16=E2=80=AFPM Chris Li wrote: > > On Thu, Aug 22, 2024 at 11:13=E2=80=AFAM Yosry Ahmed wrote: > > > > On Tue, Aug 20, 2024 at 11:42=E2=80=AFPM Kairui Song = wrote: > > > > > > On Wed, Aug 21, 2024 at 1:49=E2=80=AFPM Barry Song <21cnbao@gmail.com= > wrote: > > > > > > > > On Tue, Aug 20, 2024 at 9:02=E2=80=AFPM Kairui Song wrote: > > > > > > > > > > On Tue, Aug 20, 2024 at 4:47=E2=80=AFPM Kairui Song wrote: > > > > > > > > > > > > On Tue, Aug 20, 2024 at 4:13=E2=80=AFAM Yosry Ahmed wrote: > > > > > > > On Fri, Aug 16, 2024 at 12:52=E2=80=AFPM syzbot > > > > > > > wrote= : > > > > > > > > > > > > > > > > Hello, > > > > > > > > > > > > > > > > syzbot found the following issue on: > > > > > > > > > > > > > > > > HEAD commit: 367b5c3d53e5 Add linux-next specific files = for 20240816 > > > > > > > > > > > > I can't find this commit, seems this commit is not in linux-nex= t any more? > > > > > > > > > > > > > > git tree: linux-next > > > > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x= =3D12489105980000 > > > > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x= =3D61ba6f3b22ee5467 > > > > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=3Dc= e6029250d7fd4d0476d > > > > > > > > compiler: Debian clang version 15.0.6, GNU ld (GNU Bi= nutils for Debian) 2.40 > > > > > > > > > > > > > > > > Unfortunately, I don't have any reproducer for this issue y= et. > > > > > > > > > > > > > > > > Downloadable assets: > > > > > > > > disk image: https://storage.googleapis.com/syzbot-assets/0b= 1b4e3cad3c/disk-367b5c3d.raw.xz > > > > > > > > vmlinux: https://storage.googleapis.com/syzbot-assets/5bb09= 0f7813c/vmlinux-367b5c3d.xz > > > > > > > > kernel image: https://storage.googleapis.com/syzbot-assets/= 6674cb0709b1/bzImage-367b5c3d.xz > > > > > > > > > > > > > > > > IMPORTANT: if you fix the issue, please add the following t= ag to the commit: > > > > > > > > Reported-by: syzbot+ce6029250d7fd4d0476d@syzkaller.appspotm= ail.com > > > > > > > > > > > > > > > > ------------[ cut here ]------------ > > > > > > > > WARNING: CPU: 0 PID: 11298 at mm/zswap.c:1700 zswap_swapoff= +0x11b/0x2b0 mm/zswap.c:1700 > > > > > > > > Modules linked in: > > > > > > > > CPU: 0 UID: 0 PID: 11298 Comm: swapoff Not tainted 6.11.0-r= c3-next-20240816-syzkaller #0 > > > > > > > > Hardware name: Google Google Compute Engine/Google Compute = Engine, BIOS Google 06/27/2024 > > > > > > > > RIP: 0010:zswap_swapoff+0x11b/0x2b0 mm/zswap.c:1700 > > > > > > > > Code: 74 05 e8 78 73 07 00 4b 83 7c 35 00 00 75 15 e8 1b bd= 9e ff 48 ff c5 49 83 c6 50 83 7c 24 0c 17 76 9b eb 24 e8 06 bd 9e ff 90 <0= f> 0b 90 eb e5 48 8b 0c 24 80 e1 07 80 c1 03 38 c1 7c 90 48 8b 3c > > > > > > > > RSP: 0018:ffffc9000302fa38 EFLAGS: 00010293 > > > > > > > > RAX: ffffffff81f4d66a RBX: dffffc0000000000 RCX: ffff88802c= 19bc00 > > > > > > > > RDX: 0000000000000000 RSI: 0000000000000002 RDI: ffff888015= 986248 > > > > > > > > RBP: 0000000000000000 R08: ffffffff81f4d620 R09: 1ffffffff1= d476ac > > > > > > > > R10: dffffc0000000000 R11: fffffbfff1d476ad R12: dffffc0000= 000000 > > > > > > > > R13: ffff888015986200 R14: 0000000000000048 R15: 0000000000= 000002 > > > > > > > > FS: 00007f9e628a5380(0000) GS:ffff8880b9000000(0000) knlGS= :0000000000000000 > > > > > > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > > > > > > > CR2: 0000001b30f15ff8 CR3: 000000006c5f0000 CR4: 0000000000= 3506f0 > > > > > > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000= 000000 > > > > > > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000= 000400 > > > > > > > > Call Trace: > > > > > > > > > > > > > > > > __do_sys_swapoff mm/swapfile.c:2837 [inline] > > > > > > > > __se_sys_swapoff+0x4653/0x4cf0 mm/swapfile.c:2706 > > > > > > > > do_syscall_x64 arch/x86/entry/common.c:52 [inline] > > > > > > > > do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83 > > > > > > > > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > > > > > > > RIP: 0033:0x7f9e629feb37 > > > > > > > > Code: 73 01 c3 48 8b 0d f1 52 0d 00 f7 d8 64 89 01 48 83 c8= ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 a8 00 00 00 0f 05 <4= 8> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c1 52 0d 00 f7 d8 64 89 01 48 > > > > > > > > RSP: 002b:00007fff17734f68 EFLAGS: 00000246 ORIG_RAX: 00000= 000000000a8 > > > > > > > > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f9e62= 9feb37 > > > > > > > > RDX: 00007f9e62a9e7e8 RSI: 00007f9e62b9beed RDI: 0000563090= 942a20 > > > > > > > > RBP: 0000563090942a20 R08: 0000000000000000 R09: 77872e07ed= 164f94 > > > > > > > > R10: 000000000000001f R11: 0000000000000246 R12: 00007fff17= 735188 > > > > > > > > R13: 00005630909422a0 R14: 0000563073724169 R15: 00007f9e62= bdda80 > > > > > > > > > > > > > > > > > > > > > > I am hoping syzbot would find a reproducer and bisect this fo= r us. > > > > > > > Meanwhile, from a high-level it looks to me like we are missi= ng a > > > > > > > zswap_invalidate() call in some paths. > > > > > > > > > > > > > > If I have to guess, I would say it's related to the latest mT= HP swap > > > > > > > changes, but I am not following closely. Perhaps one of the f= ollowing > > > > > > > things happened: > > > > > > > > > > > > > > (1) We are not calling zswap_invalidate() in some invalidatio= n paths. > > > > > > > It used to not be called for the cluster freeing path, so may= be we end > > > > > > > up with some order-0 swap entries in a cluster? or maybe ther= e is an > > > > > > > entirely new invalidation path that does not go through > > > > > > > free_swap_slot() for order-0 entries? > > > > > > > > > > > > > > (2) Some higher order swap entries (i.e. a cluster) end up in= zswap > > > > > > > somehow. zswap_store() has a warning to cover that though. Ma= ybe > > > > > > > somehow some swap entries are allocated as a cluster, but the= n pages > > > > > > > are swapped out one-by-one as order-0 (which can go to zswap)= , but > > > > > > > then we still free the swap entries as a cluster? > > > > > > > > > > > > Hi Yosry, thanks for the report. > > > > > > > > > > > > There are many mTHP related optimizations recently, for this pr= oblem I > > > > > > can reproduce this locally. Can confirm the problem is gone for= me > > > > > > after reverting: > > > > > > > > > > > > "mm: attempt to batch free swap entries for zap_pte_range()" > > > > > > > > > > > > Hi Barry, > > > > > > > > > > > > If a set of continuous slots are having the same value, they ar= e > > > > > > considered a mTHP and freed, bypassing the slot cache, and caus= ing > > > > > > zswap leak. > > > > > > This didn't happen in put_swap_folio because that function is > > > > > > expecting an actual mTHP folio behind the slots but > > > > > > free_swap_and_cache_nr is simply walking the slots. > > > > > > > > > > > > For the testing, I actually have to disable mTHP, because linux= -next > > > > > > will panic with mTHP due to lack of following fixes: > > > > > > https://lore.kernel.org/linux-mm/a4b1b34f-0d8c-490d-ab00-eaedbf= 3fe780@gmail.com/ > > > > > > https://lore.kernel.org/linux-mm/403b7f3c-6e5b-4030-ab1c-3198f3= 6e3f73@gmail.com/ > > > > > > > > > > > > > > > > > > > > I am not closely following the latest changes so I am not sur= e. CCing > > > > > > > folks who have done work in that area recently. > > > > > > > > > > > > > > I am starting to think maybe it would be more reliable to jus= t call > > > > > > > zswap_invalidate() for all freed swap entries anyway. Would t= hat be > > > > > > > too expensive? We used to do that before the zswap_invalidate= () call > > > > > > > was moved by commit 0827a1fb143f ("mm/zswap: invalidate zswap= entry > > > > > > > when swap entry free"), and that was before we started using = the > > > > > > > xarray (so it was arguably worse than it would be now). > > > > > > > > > > > > > > > > > > > That might be a good idea, I suggest moving zswap_invalidate to > > > > > > swap_range_free and call it for every freed slot. > > > > > > > > > > > > Below patch can be squash into or put before "mm: attempt to ba= tch > > > > > > free swap entries for zap_pte_range()". > > > > > > > > > > Hmm, on second thought, the commit message in the attachment comm= it > > > > > might be not suitable, current zswap_invalidate is also designed = to > > > > > only work for order 0 ZSWAP, so things are not clean even after t= his. > > > > > > > > Kairui, what about the below? we don't touch the path of __try_to_r= eclaim_swap() where > > > > you have one folio backed? > > > > > > > > diff --git a/mm/swapfile.c b/mm/swapfile.c > > > > index c1638a009113..8ff58be40544 100644 > > > > --- a/mm/swapfile.c > > > > +++ b/mm/swapfile.c > > > > @@ -1514,6 +1514,8 @@ static bool __swap_entries_free(struct swap_i= nfo_struct *si, > > > > unlock_cluster_or_swap_info(si, ci); > > > > > > > > if (!has_cache) { > > > > + for (i =3D 0; i < nr; i++) > > > > + zswap_invalidate(swp_entry(si->type, offset= + i)); > > > > spin_lock(&si->lock); > > > > swap_entry_range_free(si, entry, nr); > > > > spin_unlock(&si->lock); > > > > > > > > > > Hi Barry, > > > > > > Thanks for updating this thread, I'm thinking maybe something will > > > better be done at the zswap side? > > > > > > The concern of using zswap_invalidate is that it calls xa_erase which > > > requires the xa spin lock. But if we are calling zswap_invalidate in > > > swap_entry_range_free, and ensure the slot is HAS_CACHE pinned, doing > > > a lockless read first with xa_load should be OK for checking if the > > > slot needs a ZSWAP invalidation. The performance cost will be minimal > > > and we only need to call zswap_invalidate in one place, something lik= e > > > this (haven't tested, comments are welcome). Also ZSWAP mthp will > > > still store entried in order 0 so this should be OK for future. > > > > > > While I do agree with this change on a high level, it's essentially > > reverting commit 0827a1fb143f ("mm/zswap: invalidate zswap entry when > > swap entry free") which fixed a small problem with zswap writeback. > > I'd prefer that we don't if possible. > > > > One thing that I always wanted to do is to pull some of the work done > > in swap_entry_range_free() and swap_range_free() before the slots > > caching layer. The memcg uncharging, clearing shadow entries from the > > swap cache, arch invalidation, zswap invalidation, etc. If we can have > > a hook for these pre-free callbacks we can call it for single entries > > before we add them to the slots cache, and call them for the clusters > > as we do today. This should also reduce the amount of work done under > > the lock, and move more work to where the freeing is actually > > happening vs. the cache draining. > > > > I remember discussing this briefly with Ying before. Anyone have any th= oughts? > > Hi Yosry, > > If I understand correctly, the lock you are talking about is the > si->lock, right? > > Kairui has some WIP patches removing the swap slot cache in the swap > entry freeing path. Basically the si->lock is only used to protect the > cluster list. Most of the time freeing swap entry will only take the > ci->lock. No need to take the si->lock to change the cluster lists. > Only when the cluster moves to another list will it require the > si->lock e.g. the cluster moves to the free list when all 512 entries > are freed. Because each cluster has 512 entries. The need to take > si->lock is dramatically reduced. That patch is based on the new > cluster swap allocator series. Kairui can share more details. > > I don't think ci->lock has heavy contentions. Thanks for sharing these details. However, I think we should still pull the irrelevant logic from swap_entry_range_free() and swap_range_free() before the slots cache layer. There is no reason to defer this work until we drain the cache, and whoever ends up draining the cache ends up paying for all the work of previously freed slots. In the zswap case, deferring the invalidation causes a problem, which is why we moved it to free_swap_slot(). However, now with the mTHP changes more freeing paths are being added and it's easy to miss invalidating zswap in every case, so I think we need a more centralized place to execute pre-free hooks, and call it before the swap slots cache. > > Chris