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 8D488CF9C69 for ; Tue, 24 Sep 2024 18:12:27 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1C7106B00AD; Tue, 24 Sep 2024 14:12:27 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 14F106B00AF; Tue, 24 Sep 2024 14:12:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F0B496B00B1; Tue, 24 Sep 2024 14:12:26 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id CE8156B00AD for ; Tue, 24 Sep 2024 14:12:26 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 647CC802E1 for ; Tue, 24 Sep 2024 18:12:26 +0000 (UTC) X-FDA: 82600426692.26.2A1AF01 Received: from mail-ed1-f46.google.com (mail-ed1-f46.google.com [209.85.208.46]) by imf23.hostedemail.com (Postfix) with ESMTP id 8731314001D for ; Tue, 24 Sep 2024 18:12:24 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="W/ozpsaa"; spf=pass (imf23.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.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=1727201509; 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=+qB1t1K4I7ZeMGtdl1ixYAWu/UtPcYu5Rsyq5zbhgsc=; b=FMgy1Ivkrbjo1TW8uUSSq2EvI//XtQng3vd+n245EKCOmKKtBboQ205nfwpMY0JT8hHxui ZHmtmN5dDKGkoUJaU9VG9mVdEGAaDGCsGR5t59wBFdONg54mOs5YzAviHxw9i3KGrHU7wn m8XairjTOF1SfdJ1y9vLgu6H0VeszCI= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="W/ozpsaa"; spf=pass (imf23.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.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=1727201509; a=rsa-sha256; cv=none; b=0tG8rbSudcZXRwTZjFQSDNnwD+wuifAxJmBbqsaRXGJVGTK30vhid/Xpl23FdopwYtTZhY TqIxl3WNAkN/ErDJip328jjwmw4Dh4YujLRBcD3KmB4jA9VwlyqtwcDAITcfiDozAHN4n4 my1KYONAT3Y04pg8b3es6JkcyLZBsPo= Received: by mail-ed1-f46.google.com with SMTP id 4fb4d7f45d1cf-5c42e7adbe0so6794621a12.2 for ; Tue, 24 Sep 2024 11:12:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1727201543; x=1727806343; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=+qB1t1K4I7ZeMGtdl1ixYAWu/UtPcYu5Rsyq5zbhgsc=; b=W/ozpsaan+kaFrrUs2yd+ZWAQu6bfysTRLgxsFCvcUqUbmTC73Vm62BNv9YU0s0yee 7XNO8opDpoJfjXyQaGL4gpcJl3PhoKS4cJkDYPXi1S2C3sDQtYWFD5pEIQ0MwIaypHma DsVIsCktauexXPlbkM+yRKuscH7LHf5oQMXpCEg1oIHlGelF0DTolqVBhtMRndk25Xdp GCjipDKv5QuPUr7kKEeqGBVH/Dq2NJ6613bN2qkZh7ucNyu6K0bR07wL/f6QN2x6Vgc8 eamV3ZDXSpGCDF7UeJ12sFiMEH8V43Si+fgE3j1LHhnOnoLdEH9XHRzaE7DdyAm5/HRn yFcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727201543; x=1727806343; h=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=+qB1t1K4I7ZeMGtdl1ixYAWu/UtPcYu5Rsyq5zbhgsc=; b=moEiWgyjzMgWpZ8DPeThrjOAdqXBM4keFvtgY9lw//bAnDXdeZ4lu8X6vDwUC6aBSz MRAvJTlu2A0FTv6+4E3hBjR4BwJx9HUH7qs5fdufXBUjrHSbGOS1GzLvuVzIjVMgLXUK Dy47jb5+SYnwiA5MFWFRZiJSk0KQWYKwas0OK/aHjcCU4Odizg9APdwyizMfvlcSWu18 hTCILUtaC2acbaDgf+ZUJeg+Oax8QHSdeMyKKAQDseuP8Ss1dfXWQ/zjwwmfmO6G2XYd 3ec+amC2WcVCcCbHCahoMHNZ8gOjMHPcHqNmojtNMIJVT74vV4VXYo68WxR0tmMh7+8v QwQw== X-Forwarded-Encrypted: i=1; AJvYcCUfUDjX9vgSJS5TSJXKyzoe9jiWMwkKifuEWXVdK1yF9+DLi0hP3jka42oxpLDPOpoL2O5ZKhYYmQ==@kvack.org X-Gm-Message-State: AOJu0Yy1cEYvRBe1Ic+dCc7d8CeHDheC5MLEjRO3mfI7rt+21ZA6HF7A ZpvQMQXo3KRR/pU/rek2Vy+z82psqd9ly5nqkAU+W0nga4KuJm/HtlwaXBJrr/OOD3pJKDmPhHc WZGzUmKrTMvdIH/svIZw2YQZBhBbQeNGIrvlB X-Google-Smtp-Source: AGHT+IE9Ekz7xYrCGBs7cxBQBO3hTmkiDjymkALmjwUg918MsPF80cAQc1DNmeOrdr7cncncnBc7E8NYe+6tgHkQYHU= X-Received: by 2002:a17:906:4789:b0:a8a:58c5:78f1 with SMTP id a640c23a62f3a-a93a031d511mr16315866b.11.1727201542642; Tue, 24 Sep 2024 11:12:22 -0700 (PDT) MIME-Version: 1.0 References: <20240923231142.4155415-1-nphamcs@gmail.com> <4d38c65d-760c-43a5-bb47-8e0235c13a51@linux.alibaba.com> <9a110f20-42ad-468b-96c6-683e162452a9@linux.alibaba.com> In-Reply-To: From: Yosry Ahmed Date: Tue, 24 Sep 2024 11:11:45 -0700 Message-ID: Subject: Re: [RFC PATCH 0/2] remove SWAP_MAP_SHMEM To: Nhat Pham , Barry Song <21cnbao@gmail.com> Cc: Baolin Wang , akpm@linux-foundation.org, hannes@cmpxchg.org, hughd@google.com, shakeel.butt@linux.dev, ryan.roberts@arm.com, ying.huang@intel.com, chrisl@kernel.org, david@redhat.com, kasong@tencent.com, willy@infradead.org, viro@zeniv.linux.org.uk, baohua@kernel.org, chengming.zhou@linux.dev, linux-mm@kvack.org, kernel-team@meta.com, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Rspam-User: X-Stat-Signature: 5u7ta1uwxxfsm9acjanh8iizighdgixm X-Rspamd-Queue-Id: 8731314001D X-Rspamd-Server: rspam11 X-HE-Tag: 1727201544-93563 X-HE-Meta: U2FsdGVkX1+5UTo25NQoBtKwOxFVdNHa92h0vjMVwqjKDV+AIdtdMvFg/y0mOBSjB5D4Kqo2umWA6CjQVrr1DUMJt2FqUgS0I6D66XKzIzUlec7qozHgK6sZN2ffkowX9xTMfjJ0kkPvbZ9tkSDo7kn6D1ziinmrpm56cQZZThfmFEaTRvSAZH8m5HbSgPcFMxznJ02UNOq9Mk7W1M1VRYuwvt3AdljUKhliHbiELsqXGhM0RSTbMseAJUAMyFc+kBfh6IAscRVLwiThoXL7PNq80RYj6Qe0H7UWPmS8nFCA+tYdPuroK0xquKVIPv8BVjBqyt4cLjdMJURBtCuxVuSZJWqFaWTJST+P1QI7ENYG8yc0WpObE5PezTPmFZtqCsiGWvZgk6e7+RAAswLMToV93qKHEHnq3UnMXadz1eMSsqaZNct3WF9SRgkC2r4D1yco9iQKSjyXjA6LidKJHXXxeBEXfiTGKU1JcCk7Cj2ixCjjGC07upzSDsvpa+6e1l0Lp3F8z7ixaqN+aFv8f9KrlPtaRFCYKZ8yTvFuddNL4g5cqoNjpkYQ6jnAGfnoOvTASKd0VKbcFWmOBYruDP0NjjXaGcxj4d9qVJo717VyJtr7RYEPL3vZFw6rSxrbqkOa++NmN1simH/TrwuD162RT/uB/z6Dc1OmgU09/1fpbPV+FsyHegf+JmNwUL/BtUPFvZxfCNJBEVFMi4WOUIdxpOUeTLtrFUyE4o6Mjn0xF4B5DzUX+EjJGyUlWtBhyHLFfNASwFN3XsaffT/dBY4qxij9wqlVW0mnar5p25+zBAEm/wElWZ64qyjgJzyTt0PV5upvxB7JDNXkZf43/36675NIwxlg365Uvy3QEoSJUWE6zOgXePFKe0VfrV9XNsbLFYiyvZx98qb6KP8GddnL6UMb3Cj3rFtt+YPw70BfOACLn1/2AS2u75+RD4askTFzUyyHKJALudrlgn6 IB8pkJoz e6054HfyhFgISKqvaxHMxHWxkQIfvBUFKJtcHGf0DBB4AWMNL/qYDGHRwGAK6u4NBUcqLmBwtjzknu4hbTKvpSEfBLALr3OfJIKcTqIUfECL2KaAPs/fM9M8oe1MPzKrVSVYSacoowQQ4y0aaj9tKoN/E20lBC5x2HKTbg6UxaDJiFG2xebWGzVJT4Wm6EKp/P/PKqO7ybTYT8BXKY8MKdWJGOkNtzej+Bnf3qlm20oqowURSPvTwLt/RUw== 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: [..] > > > > > Apparently __swap_duplicate() does not currently handle increasing the > > > > > swap count for multiple swap entries by 1 (i.e. usage == 1) because it > > > > > does not handle rolling back count increases when > > > > > swap_count_continued() fails. > > > > > > > > > > I guess this voids my Reviewed-by until we sort this out. Technically > > > > > swap_count_continued() won't ever be called for shmem because we only > > > > > ever increment the count by 1, but there is no way to know this in > > > > > __swap_duplicate() without SWAP_HAS_SHMEM. > > > > > > Ah this is my bad. I compiled with CONFIG_THP_SWAP, but forgot to > > > remove the swapfile check (that's another can of worms, but I need > > > data before submitting the patch to remove it...) > > > > > > One thing we can do is instead of warning here, we can handle it in > > > the for loop check, where we have access to count - that's the point > > > of having that for-loop check anyway? :) > > > > > > There's a couple of ways to go about it: > > > > > > 1. VM_WARN_ON(usage == 1 && nr > 1 && count != 0 ); > > > > Hmm that should work, although it's a bit complicated tbh. > > > > > (or more accurately, (count & ~COUNT_CONTINUED) >= SWAP_MAP_MAX)) > > > > I think this will make the warning very hard to hit if there's a > > misuse of __swap_duplicate(). It will only be hit when an entry needs > > count continuation, which I am not sure is very common. If there's a > > bug, the warning will potentially catch it too late, if ever. > > > > The side effect here is failing to decrement the swap count of some > > swap entries which will lead to them never being freed, essentially > > leaking swap capacity slowly over time. I am not sure if there are > > more detrimental effects. > > > > > > > > 2. Alternatively, instead of warning here, we can simply return > > > -ENOMEM. Then, at shmem callsite, have a VM_WARN_ON/VM_BUG_ON(), since > > > this MUST succeed. > > > > We still fail to rollback incremented counts though when we return > > -ENOMEM, right? Maybe I didn't get what you mean. > > My understanding now is that there are two for loops. One for loop > that checks the entry's states, and one for loop that does the actual > incrementing work (or state modification). > > We can check in the first for loop, if it is safe to proceed: > > if (!count && !has_cache) { > err = -ENOENT; > } else if (usage == SWAP_HAS_CACHE) { > if (has_cache) > err = -EEXIST; > } else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX) { > err = -EINVAL; > } else if (usage == 1 && nr > 1 && (count & ~COUNT_CONTINUED) >= > SWAP_MAP_MAX)) { > /* the batched variants currently do not support rollback */ > err = -ENOMEM; > } Hmm yeah I think something like this should work and is arguably better than just warning, although this needs cleaning up: - We already know usage != SWAP_HAS_CACHE, so no need to check if usage == 1. - We already know (count & ~COUNT_CONTINUED) is larger than SWAP_MAP_MAX, so we should check if it's equal to SWAP_MAP_MAX. - We should probably just calculate count & ~COUNT_CONTINUED above the if conditions at this point. I would also like to hear what Barry thinks since he added this (and I just realized he is not CC'd).