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 DA21FC27C5F for ; Fri, 7 Jun 2024 17:43:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4AA366B009C; Fri, 7 Jun 2024 13:43:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 45A4B6B009D; Fri, 7 Jun 2024 13:43:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3215D6B009E; Fri, 7 Jun 2024 13:43:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 1392F6B009C for ; Fri, 7 Jun 2024 13:43:59 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 82F921219C1 for ; Fri, 7 Jun 2024 17:43:58 +0000 (UTC) X-FDA: 82204815756.12.8428C0D Received: from mail-ej1-f46.google.com (mail-ej1-f46.google.com [209.85.218.46]) by imf10.hostedemail.com (Postfix) with ESMTP id 9D7DBC000B for ; Fri, 7 Jun 2024 17:43:56 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=C8OavDcZ; spf=pass (imf10.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.46 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=1717782236; a=rsa-sha256; cv=none; b=frtQfgjKipjPXNPSySVlQasqapg64jxke+DoQrau5YQXMtKmzwlFLmi8sBkQ001fwpIrBb fE5IuZpb+YtTYu0eYTAEKhW2siN0MyxYFxW74UwYxEdQP8tw8Lf4gB6+NfQ79s1SDnaiZP 0CmQIzU6SxRvtLv9CbVv4KxnbNr5RQI= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=C8OavDcZ; spf=pass (imf10.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.46 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=1717782236; 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=GHxIdxIvUyWbgQDNhvFlgl5LM/lfPBj3QHKybJCJWNU=; b=lj0Yp6BDmLx1Lkj6y09rbZMopJrhYFUgvc7+gCq/pgK0+wH6OkoMvoXWILpBG/OKUtYDkC ak8gvZT1VvPBqZiTQaau0uZRbd9XsjhVeq3rPPUpQXlrJyetoqKA2bdmm5F2Kw4vEzFz+P ijax5y6n+yXqu/UuLNgTm8K4h1fXB+Y= Received: by mail-ej1-f46.google.com with SMTP id a640c23a62f3a-a6267778b3aso293213766b.3 for ; Fri, 07 Jun 2024 10:43:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1717782235; x=1718387035; 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=GHxIdxIvUyWbgQDNhvFlgl5LM/lfPBj3QHKybJCJWNU=; b=C8OavDcZXmK/8kjXLUU6DyK2NZLtIjzI91S/o+vHJNQYn6UIfPZZfr0LEK/kYxaWC4 68VSCNN/w9dW0KSamd6NdHSPaPqb9Ax67zotFkwO8V3+ELJz1tO9i6G6XazkmtOcbXKA owcTToeZUmJ1HxEyCiTRc03P3oKZSYv6lTTBOESOq4xM4rEMOEysHHXsydbTycXf+GZt Yk4QzUO7/lboPawc0/spSq7rkmTzr2EleWgVZOt+NSujYJT0CpQ4YbZSvZMu6Hx6Qbof dYLkz0r+s23QaQJUgvGL9+N0+kesQZ6QM1v2drxBsDXcMGQO/Dzf6i2GPEJKAWtr90AA +Dzw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717782235; x=1718387035; 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=GHxIdxIvUyWbgQDNhvFlgl5LM/lfPBj3QHKybJCJWNU=; b=WdM28ZZT2mc4AejNkMWElQBXnR28YAPGOeXcQTUsRIoM3H9WA9+GmQTEcrAMVQriiI rb4zgTng2FwTPdbPjmIHx1XXfH71tbmHfoCRDHjH/UF0gW3tQ8TJ4jDm10MkPM6Tc9ei n8cJjD2g4PkjVRWYTS+TCBEUxaaA3+tYd7V8Unjm5o4f6VS488vlm4HQxOnCzQp8EoAM ZFVtSVHbr61P6x4Ez2LZOaZ8Y4I5xAVA8+wNEunnDBXn4meLs0Tq6TZs1ovxunOovMBy GoVuAmFsyGSI1knmDG6lBHtq2ifuVlKJ5leQUm9n7PcFigqghD581vMCRWCKPaVa/8BQ YNaA== X-Forwarded-Encrypted: i=1; AJvYcCU5Esq9M0erH0h44Cl8cJxDnsDJz3z2yqb3mRhEarhjunQqBNHzUkpIncL4aUsQGTX3cOYw7EqXpF+V/+OKaMOtS64= X-Gm-Message-State: AOJu0Ywxu/sZ8FPPTEJLiy7djHQygyWazLp6SkjuHgpRk/NAWuFO+uy3 j5eANg1yV6ycAhHqsSP3uIctPGJGH8J/bPq7Ko7eijGwm9AoTHloFOsNE/T390IbksbfOthginc ICyi2NebpOpHH6XIw2sk3ZRi90pAM+T1XCQoc X-Google-Smtp-Source: AGHT+IFmXnedXaVPyjmrFAOaHxx15wNLRbJ4QTjwP83kqQYCY1tghWoHbftW3ost19NsRKWJ9Ol51YXKoh5LO3SRtWI= X-Received: by 2002:a17:906:fd85:b0:a69:edd:3332 with SMTP id a640c23a62f3a-a6cd5616814mr288461766b.12.1717782234660; Fri, 07 Jun 2024 10:43:54 -0700 (PDT) MIME-Version: 1.0 References: <20240606184818.1566920-1-yosryahmed@google.com> <84d78362-e75c-40c8-b6c2-56d5d5292aa7@redhat.com> <7507d075-9f4d-4a9b-836c-1fbb2fbd2257@redhat.com> <9374758d-9f81-4e4f-8405-1f972234173e@redhat.com> In-Reply-To: <9374758d-9f81-4e4f-8405-1f972234173e@redhat.com> From: Yosry Ahmed Date: Fri, 7 Jun 2024 10:43:16 -0700 Message-ID: Subject: Re: [PATCH] mm: zswap: add VM_BUG_ON() if large folio swapin is attempted To: David Hildenbrand Cc: Barry Song <21cnbao@gmail.com>, Andrew Morton , Johannes Weiner , Nhat Pham , Chengming Zhou , Baolin Wang , Chris Li , Ryan Roberts , Matthew Wilcox , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 9D7DBC000B X-Rspam-User: X-Rspamd-Server: rspam12 X-Stat-Signature: u6f99x9bsihg3x1633ic8aktwui3ad3y X-HE-Tag: 1717782236-47043 X-HE-Meta: U2FsdGVkX1+H+ag5Dem8E0pR+ZRrkE72UhLuN+HRaZ+tk0p0qjy5YYJ+6DpPyvNkWl326S4oyCbAVpFBCNCV37LU0it7LV5dgOY4UHxccoFsIRUTX3CtgW36RotOW0lWzDvPyO95DW6K8aZaYi89jMj2exO0Du1DORIV9gsPzU/SrKAuj/X58zTM4U/HoHHQITzf818vGJMvftiA7XlgFEtZifleGbXMyQaCycurF85q2a/7d3AAuAOcQtCSC6urkmGG/OOj25zlsDVMBLvKqN7X+vFPZFUAU5CEW5BZaqjHVi40/YPQi7VUgCjmi3GNMCpMbVAcYX3v3vp5fELRKGyUc9kFd/K40h+EshrnFBXH0ny/bvDysU0RA3U/MMmNa6hk0S2OKLUZTolv305A8fvvCwtVQW4I2wO8jPFTt9j3T3Nvn8z7V8TD1dMvg3On+ej/HfbWIzsFOpaW/tfTvRqiOrB908hlflDEEX1IQgTYtC0vcv2lkWAKWt2ERJKXIlDmxaQ0WkE6y2tTqd6oeJK0834fXMq/ZjLsvW7j3fDZNo0O6yZHp+B3Px2aGiHbuYF/J6ujmLhhcf5sOcchXAFxqLoOA10kfMQIIIcTKX0dLoWPICsgP6zB6RQhN8RtpisK/S6v0rf9eYmo0iPq5VOM6dS1EK/79DxqQbyvHYbl5DbWLHXO0NQFk7e++yc8z2IuK/tApaxdEroN28/rZFtOpogXO0buIBaBrtkyIXG2zeO6OpIoSKQpUvKIE6frbNlxkIEXe7AUhrNIfosORHVokQJGRIA1XMIjMvmq5fzc4nmaGNEYsrRhQFtkTlgW+LX+CBmdOy+TKVpLlEex8aUIDIwy0o4EGmuOmmHvCxXOo6NN35MnaHXo2KDPLMzsgFu4ziIgZBipJy2e1/G/n6WSaCqQxA1Efz4yv4GiuY4rMd+CFK5vN49VepiLxX5RBSaEBObEu3QotnQlNU4 T3i0crif ya9hX0Str++kyB9orSjawclJT6OSK7LYxQop+GUunfFBXBz8N4GDpSxdbBoDcCDbpfuyqkNB/OAj7Tg1zYyWhO5W5rj/1qaGxJEYycYuUN0ktQnj73ljFkT+s5vNh0TGjw0gy0IE9uytudBOODE8NDWe6uv/x0yUNjyMaSm5HLderToXje9pMt1Fxk3kuKk3vKPZFIcYn8hH3isyIq/BwrMG0EMcx++teXYgqacQaxJ1CxRp1YmrW4yCl3zShKgc89kAg/dKFCCJp9AveZVfKufFscaN15Ju2gZCi+dvXGbzDwL4v/3lzB8S9s3wNXu2kIVzFI4nVzGCxzfsaxfWQqfsM0vvoI/hl6gKS 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 Fri, Jun 7, 2024 at 12:11=E2=80=AFAM David Hildenbrand wrote: > > On 06.06.24 23:53, Barry Song wrote: > > On Fri, Jun 7, 2024 at 9:37=E2=80=AFAM Yosry Ahmed wrote: > >> > >> On Thu, Jun 6, 2024 at 2:30=E2=80=AFPM Barry Song <21cnbao@gmail.com> = wrote: > >>> > >>> On Fri, Jun 7, 2024 at 9:17=E2=80=AFAM David Hildenbrand wrote: > >>>> > >>>> On 06.06.24 22:31, Yosry Ahmed wrote: > >>>>> On Thu, Jun 6, 2024 at 1:22=E2=80=AFPM David Hildenbrand wrote: > >>>>>> > >>>>>> On 06.06.24 20:48, Yosry Ahmed wrote: > >>>>>>> With ongoing work to support large folio swapin, it is important = to make > >>>>>>> sure we do not pass large folios to zswap_load() without implemen= ting > >>>>>>> proper support. > >>>>>>> > >>>>>>> For example, if a swapin fault observes that contiguous PTEs are > >>>>>>> pointing to contiguous swap entries and tries to swap them in as = a large > >>>>>>> folio, swap_read_folio() will pass in a large folio to zswap_load= (), but > >>>>>>> zswap_load() will only effectively load the first page in the fol= io. If > >>>>>>> the first page is not in zswap, the folio will be read from disk,= even > >>>>>>> though other pages may be in zswap. > >>>>>>> > >>>>>>> In both cases, this will lead to silent data corruption. > >>>>>>> > >>>>>>> Proper large folio swapin support needs to go into zswap before z= swap > >>>>>>> can be enabled in a system that supports large folio swapin. > >>>>>>> > >>>>>>> Looking at callers of swap_read_folio(), it seems like they are e= ither > >>>>>>> allocated from __read_swap_cache_async() or do_swap_page() in the > >>>>>>> SWP_SYNCHRONOUS_IO path. Both of which allocate order-0 folios, s= o we > >>>>>>> are fine for now. > >>>>>>> > >>>>>>> Add a VM_BUG_ON() in zswap_load() to make sure that we detect cha= nges in > >>>>>>> the order of those allocations without proper handling of zswap. > >>>>>>> > >>>>>>> Alternatively, swap_read_folio() (or its callers) can be updated = to have > >>>>>>> a fallback mechanism that splits large folios or reads subpages > >>>>>>> separately. Similar logic may be needed anyway in case part of a = large > >>>>>>> folio is already in the swapcache and the rest of it is swapped o= ut. > >>>>>>> > >>>>>>> Signed-off-by: Yosry Ahmed > >>>>>>> --- > >>>>>>> > >>>>>>> Sorry for the long CC list, I just found myself repeatedly lookin= g at > >>>>>>> new series that add swap support for mTHPs / large folios, making= sure > >>>>>>> they do not break with zswap or make incorrect assumptions. This = debug > >>>>>>> check should give us some peace of mind. Hopefully this patch wil= l also > >>>>>>> raise awareness among people who are working on this. > >>>>>>> > >>>>>>> --- > >>>>>>> mm/zswap.c | 3 +++ > >>>>>>> 1 file changed, 3 insertions(+) > >>>>>>> > >>>>>>> diff --git a/mm/zswap.c b/mm/zswap.c > >>>>>>> index b9b35ef86d9be..6007252429bb2 100644 > >>>>>>> --- a/mm/zswap.c > >>>>>>> +++ b/mm/zswap.c > >>>>>>> @@ -1577,6 +1577,9 @@ bool zswap_load(struct folio *folio) > >>>>>>> if (!entry) > >>>>>>> return false; > >>>>>>> > >>>>>>> + /* Zswap loads do not handle large folio swapins correctly = yet */ > >>>>>>> + VM_BUG_ON(folio_test_large(folio)); > >>>>>>> + > >>>>>> > >>>>>> There is no way we could have a WARN_ON_ONCE() and recover, right? > >>>>> > >>>>> Not without making more fundamental changes to the surrounding swap > >>>>> code. Currently zswap_load() returns either true (folio was loaded > >>>>> from zswap) or false (folio is not in zswap). > >>>>> > >>>>> To handle this correctly zswap_load() would need to tell > >>>>> swap_read_folio() which subpages are in zswap and have been loaded, > >>>>> and then swap_read_folio() would need to read the remaining subpage= s > >>>>> from disk. This of course assumes that the caller of swap_read_foli= o() > >>>>> made sure that the entire folio is swapped out and protected agains= t > >>>>> races with other swapins. > >>>>> > >>>>> Also, because swap_read_folio() cannot split the folio itself, othe= r > >>>>> swap_read_folio_*() functions that are called from it should be > >>>>> updated to handle swapping in tail subpages, which may be questiona= ble > >>>>> in its own right. > >>>>> > >>>>> An alternative would be that zswap_load() (or a separate interface) > >>>>> could tell swap_read_folio() that the folio is partially in zswap, > >>>>> then we can just bail and tell the caller that it cannot read the > >>>>> large folio and that it should be split. > >>>>> > >>>>> There may be other options as well, but the bottom line is that it = is > >>>>> possible, but probably not something that we want to do right now. > >>>>> > >>>>> A stronger protection method would be to introduce a config option = or > >>>>> boot parameter for large folio swapin, and then make CONFIG_ZSWAP > >>>>> depend on it being disabled, or have zswap check it at boot and ref= use > >>>>> to be enabled if it is on. > >>>> > >>>> Right, sounds like the VM_BUG_ON() really is not that easily avoidab= le. > >>>> > >>>> I was wondering, if we could WARN_ON_ONCE and make the swap code det= ect > >>>> this like a read-error from disk. > >>>> > >>>> I think do_swap_page() detects that by checking if the folio is not > >>>> uptodate: > >>>> > >>>> if (unlikely(!folio_test_uptodate(folio))) { > >>>> ret =3D VM_FAULT_SIGBUS; > >>>> goto out_nomap; > >>>> } > >>>> > >>>> So maybe WARN_ON_ONCE() + triggering that might be a bit nicer to th= e > >>>> system (but the app would crash either way, there is no way around i= t). > >>>> > >>> > >>> I'd rather fallback to small folios swapin instead crashing apps till= we fix > >>> the large folio swapin in zswap :-) > >> > >> I think David is referring to catching the buggy cases that do not > >> properly fallback to small folios with zswap, not as an alternative to > >> the fallback. This is at least what I had in mind with the patch. > > > > Cool. Thanks for the clarification. I'm fine with keeping the fallback, > > whether it's the current VM_BUG_ON or David's recommended > > SIGBUS. > > > > Currently, mainline doesn't support large folios swap-in, so I see > > your patch as a helpful warning for those attempting large folio > > swap-in to avoid making mistakes like loading large folios from > > zswap. > > > > In fact, I spent a week trying to figure out why my app was crashing > > before I added 'if (zswap_is_enabled()) goto fallback'. If your patch > > had come earlier, it would have saved me that week of work :-) > > > > To me, a direct crash seems like a more straightforward way to > > prompt people to fallback when attempting large folio swap-ins. > > So, I am slightly in favor of your current patch. > > BUG_ON and friends are frowned-upon, in particular in scenarios where we > can recover or that are so unexpected that they can be found during > early testing. > > I have no strong opinion on this one, but likely a VM_WARN_ON would also > be sufficient to find such issues early during testing. No need to crash > the machine. I thought VM_BUG_ON() was less frowned-upon than BUG_ON(), but after some digging I found your patches to checkpatch and Linus clearly stating that it isn't. How about something like the following (untested), it is the minimal recovery we can do but should work for a lot of cases, and does nothing beyond a warning if we can swapin the large folio from disk: diff --git a/mm/page_io.c b/mm/page_io.c index f1a9cfab6e748..8f441dd8e109f 100644 --- a/mm/page_io.c +++ b/mm/page_io.c @@ -517,7 +517,6 @@ void swap_read_folio(struct folio *folio, struct swap_iocb **plug) delayacct_swapin_start(); if (zswap_load(folio)) { - folio_mark_uptodate(folio); folio_unlock(folio); } else if (data_race(sis->flags & SWP_FS_OPS)) { swap_read_folio_fs(folio, plug); diff --git a/mm/zswap.c b/mm/zswap.c index 6007252429bb2..cc04db6bb217e 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -1557,6 +1557,22 @@ bool zswap_load(struct folio *folio) VM_WARN_ON_ONCE(!folio_test_locked(folio)); + /* + * Large folios should not be swapped in while zswap is being used,= as + * they are not properly handled. + * + * If any of the subpages are in zswap, reading from disk would res= ult + * in data corruption, so return true without marking the folio upt= odate + * so that an IO error is emitted (e.g. do_swap_page() will sigfaul= t). + * + * Otherwise, return false and read the folio from disk. + */ + if (WARN_ON_ONCE(folio_test_large(folio))) { + if (xa_find(tree, &offset, offset + folio_nr_pages(folio) - 1, 0)) + return true; + return false; + } + /* * When reading into the swapcache, invalidate our entry. The * swapcache can be the authoritative owner of the page and @@ -1593,7 +1609,7 @@ bool zswap_load(struct folio *folio) zswap_entry_free(entry); folio_mark_dirty(folio); } - + folio_mark_uptodate(folio); return true; } One problem is that even if zswap was never enabled, the warning will be emitted just if CONFIG_ZSWAP is on. Perhaps we need a variable or static key if zswap was "ever" enabled. Barry, I suspect your is_zswap_enabled() check is deficient for similar reasons, zswap could have been enabled before then became disabled.