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 D9679C35274 for ; Mon, 18 Dec 2023 20:53:24 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 40FD28D0002; Mon, 18 Dec 2023 15:53:24 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 3BFA48D0001; Mon, 18 Dec 2023 15:53:24 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2865D8D0002; Mon, 18 Dec 2023 15:53:24 -0500 (EST) 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 15EAB8D0001 for ; Mon, 18 Dec 2023 15:53:24 -0500 (EST) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id D7F66C032F for ; Mon, 18 Dec 2023 20:53:23 +0000 (UTC) X-FDA: 81581139486.22.297C9BF Received: from mail-ed1-f53.google.com (mail-ed1-f53.google.com [209.85.208.53]) by imf06.hostedemail.com (Postfix) with ESMTP id CEE7418000B for ; Mon, 18 Dec 2023 20:53:20 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=KLHkVXwc; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf06.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.53 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1702932800; 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=3QsdbGbSxsszPpaiGKZHTHrV8rAdzIP6V+qMuxlnTDw=; b=u4cBjiMZiQVP0MUsPBx/oLeMKceNv/CeTLSuSNbej0+vUzhox11ERIsuVDz0m0bAHBJ0py R3CRzwCFHQrBwsF/8VHK65m4vVuxBxEZrfZR7LaCszPSVEfniIooahDOtQiIxWvMCwNM2S PnZ3d3G09Nyf6SBHeMGBAtTZ2kqKuzY= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=KLHkVXwc; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf06.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.53 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1702932800; a=rsa-sha256; cv=none; b=vyN/H+NWOmlqWUorZZ58JnzbuPKpsZPR2T11lVnpLV46YmCv00SpiyT3uFZDJ/yCFIMH5r ocLduMLiIgiDyTzpx1KCxy8PHeOZY2nV54vsas3qP2vO17EEE9p3111xRr3g+qPMHluS02 cLv+UjSdLFIDEa3Lliiy7mLIpT0J+d4= Received: by mail-ed1-f53.google.com with SMTP id 4fb4d7f45d1cf-548ce39b101so4528828a12.2 for ; Mon, 18 Dec 2023 12:53:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1702932799; x=1703537599; 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=3QsdbGbSxsszPpaiGKZHTHrV8rAdzIP6V+qMuxlnTDw=; b=KLHkVXwcJOExMaeVr+3TzBdX66s47dN/mOprIy478GfMJGE9PB/3eGjloEvqhkN2v3 SHvvob5QCaUFq8OfRyvsXya+wbhnJGy5wZGWnKME6NCUEWVj79kPy4QXuQx2xlO4Jtbs GQvn+jTS5DdKXcGI/cWKbLGYwleC6oWNBD+J5Z3CQXq44JBoH+nx0TX8XLu2PNY4ptd+ KmrtKXBA+4yN/M2FiyAA/rE2zmfVN0GfMDyCb+MMiWTc13QktXqEhvsYtR4QGz72/FDB gOZp4FjqRQ0YUSP2D06weLvjnJSyWVAnEZDzn3DMWVNAbJu15Q9xSm2mBbLrw3iMYUqp wOKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702932799; x=1703537599; 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=3QsdbGbSxsszPpaiGKZHTHrV8rAdzIP6V+qMuxlnTDw=; b=LtJIqNpQ1PFBhbs0zrgeDP6lPVR42AsC0GTL26ixmiC65vWi1/NXcIZGJM0Im2mLq8 3EsrRP0LK1fFVIxlWGbAWtCBy6SIggHy8+GzmvlcuH/B0X8GPZ86fijLQVHweFKr6O83 jZ0gqlTE+FAhHTrnYPfqcz+sS/mVlV3a4+GoLHKFoDpNCFUWp4rwhuqSdS/+TAuYjl8R DGLP24RGhQ2UI6NtTdGVqdlA5qRIBE3HtOYmFYfFLYAlRi59My2LoDP7i9zsyTPfwX5u Xct5p42tCxdOFf7VQyiWy/H/XyES2ZsU1KLlmZFhTe+2nRVgQP6/OVP4Lm8hoU0NUet6 euQw== X-Gm-Message-State: AOJu0YwIWTOU6KZ9hDlTcdpWDTNbxcwB8+4NIeD/XkpHatvL/YTvuMrl uHfiY5rWXnMV3c1O0iBfMvNoSayKX1P3sLNQSJGw/A== X-Google-Smtp-Source: AGHT+IFAlpTWtObKtyA6+//MnW75oNmM97bzaqn8nKtpZT6NFC3S8Lw42wbMozqITvlLij86MbxeoPoAZHdX898UWWw= X-Received: by 2002:a17:906:73c3:b0:a22:feea:a69e with SMTP id n3-20020a17090673c300b00a22feeaa69emr3534950ejl.2.1702932798922; Mon, 18 Dec 2023 12:53:18 -0800 (PST) MIME-Version: 1.0 References: <20231213-zswap-dstmem-v1-0-896763369d04@bytedance.com> <20231213-zswap-dstmem-v1-5-896763369d04@bytedance.com> <20231214142320.f5cf319e619dbb2127c423e9@linux-foundation.org> <20231218140313.GA19167@cmpxchg.org> <20231218145815.GA21073@cmpxchg.org> In-Reply-To: <20231218145815.GA21073@cmpxchg.org> From: Yosry Ahmed Date: Mon, 18 Dec 2023 12:52:40 -0800 Message-ID: Subject: Re: [PATCH 5/5] mm/zswap: cleanup zswap_reclaim_entry() To: Johannes Weiner Cc: Andrew Morton , Chengming Zhou , Nhat Pham , Chris Li , Seth Jennings , Dan Streetman , Vitaly Wool , linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: CEE7418000B X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: n93u3xpab3mdruq3j9ihcwjrxsg8zz4z X-HE-Tag: 1702932800-256488 X-HE-Meta: U2FsdGVkX19hKkPOE8P7rdp+AJOJO18Nc6iSgwUv3P4qO59X+eBLb6hV/v0eql9tIQKHIYsN/xisk4SBNNIlQN7t2hAEXWAKeni2atXlLxDW2aF8GC8P8nBkGBFWjj+wccI5qCouVzvlclNGegQDGlq/+IXcIqRIW6SXfs0eTk1EeT79V468ifk8gKDBSji8dYBmEA8RTUZtmjanE4HSS0cw8cT9JoqIzxDJHeJwgRg1woD9PmISsBCf6IsJe/DBLKDt/70GGBAQh+IXkzIKSGjfUc2bgLlO+NaZ4P51ghKekiINH9gZ3f9UsXaf83uB60uGgW6m7aVRS5IucY/+XNKyA7x+hs/aZPxGaarhXfJTisyEYVlCL8MQRuK2LK07Ivm45OcwEBvZi4spfyU5KuM47yBwigKnIbHuHWUcG9HZUIZjY6LYLbGja5pUpa7xCB4WTu/5x5sp7B0HKKuxsVz9DCWPJRngBy826G0S+UTz+mfMMDGeFN0LqKVv2Y8TKBu62XqZn4FV3Tml4uIOPI6SjKVi+LK+1WUI41zgV1JpQa6SVhvMHMdl4f3tnwGfJz2xqLSNFcYsU6ccbkDkaHrkwDeOYooJP1/d7nIXnEd401vkCUmpmOt8OyVxBjSQ2oQ+I3rlYoZQHfno+Y8BPbLKBzUqWLKZdM5h37yiEf+3ezQPbtgtJhUnWj5B8OnIaroj5jIGsA+Ym+BJr/jlmWEYMFQd5YfoTMyiXKKFgncXkgtWiNb7/qstFhY/zvGdrU5N1DPy28YiSd2KWVQ2aShDMaFqdknS8albFo2t5kMeLbyyRyOq/AS/1JT9EJNVdjmOXhj2GQC3Rh9nxiSGKf7z7yr7agv5nFDfEFnSPp63+Yh1xv8xOx1h2iA4s3gitCKAaRczqTkkYjqcm3iQ/PAJ/XRXiipe9Un1cBW7Z/F2D6YHxa8fm7Pwb1FvCwFEj5GG0Jt/1oUY9xdp2FR GywX4ONL 7BmquhEHrY8lCg01S70zX1Qj90Pm13yrwiUbjdZfBQ0bxSHiTtXuD7wn76P5cDZw0CAhrr8IptPg0t57STtNaAho8XLhlN4ZLrRfGXxXJoc2WKq1ZX9FshAy9o8ZDoQQXAU6dHdPBrFT122SkDFR9S/ZrygKe/wC199mEf1obamSANMTv0LKqcbq2PqSYSju7w/yR3uNNGGhtTjxedjBTDJ3WH2mrDt8wJzLyGtocV53iGim2c1cbuXq6cHXmmPvp9bvxWVGNsxVEELTex47aSa9KVBCoo7838cSETNPwwjZwdaJ9FkbwlbijFWebw5E/QUDobXoHQ3Zpa9qyiFxZ3n2+L3ittFwuMR4x8/ovw8Kor3d7Uaa1+0JIbGzvR86SkcXBWeAFs+8FzbpDeSuAlpR6+tB1a5cAZIIHl788woISXtRS8CwDcM80T/7bEFUPSgYfcJ6FH5Q3I/vcDxVMpoYAhXHMc5kcQTwB 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 Mon, Dec 18, 2023 at 6:58=E2=80=AFAM Johannes Weiner wrote: > > On Mon, Dec 18, 2023 at 06:39:13AM -0800, Yosry Ahmed wrote: > > On Mon, Dec 18, 2023 at 6:03=E2=80=AFAM Johannes Weiner wrote: > > > > > > On Thu, Dec 14, 2023 at 02:41:26PM -0800, Yosry Ahmed wrote: > > > > On Thu, Dec 14, 2023 at 2:23=E2=80=AFPM Andrew Morton wrote: > > > > > > > > > > On Wed, 13 Dec 2023 17:02:25 -0800 Yosry Ahmed wrote: > > > > > > > > > > > On Tue, Dec 12, 2023 at 8:18=E2=80=AFPM Chengming Zhou > > > > > > wrote: > > > > > > > > > > > > > > Also after the common decompress part goes to __zswap_load(),= we can > > > > > > > cleanup the zswap_reclaim_entry() a little. > > > > > > > > > > > > I think you mean zswap_writeback_entry(), same for the commit t= itle. > > > > > > > > > > I updated my copy of the changelog, thanks. > > > > > > > > > > > > - /* > > > > > > > - * If we get here because the page is already in swap= cache, a > > > > > > > - * load may be happening concurrently. It is safe and= okay to > > > > > > > - * not free the entry. It is also okay to return !0. > > > > > > > - */ > > > > > > > > > > > > This comment should be moved above the failure check of > > > > > > __read_swap_cache_async() above, not completely removed. > > > > > > > > > > This? > > > > > > > > Yes, thanks a lot. Although I think a new version is needed anyway = to > > > > address other comments. > > > > > > > > > > > > > > --- a/mm/zswap.c~mm-zswap-cleanup-zswap_reclaim_entry-fix > > > > > +++ a/mm/zswap.c > > > > > @@ -1457,8 +1457,14 @@ static int zswap_writeback_entry(struct > > > > > mpol =3D get_task_policy(current); > > > > > page =3D __read_swap_cache_async(swpentry, GFP_KERNEL, mp= ol, > > > > > NO_INTERLEAVE_INDEX, &page_was_al= located, true); > > > > > - if (!page) > > > > > + if (!page) { > > > > > + /* > > > > > + * If we get here because the page is already in = swapcache, a > > > > > + * load may be happening concurrently. It is safe= and okay to > > > > > + * not free the entry. It is also okay to return = !0. > > > > > + */ > > > > > return -ENOMEM; > > > > > + } > > > > > > > > > > /* Found an existing page, we raced with load/swapin */ > > > > > if (!page_was_allocated) { > > > > > > That's the wrong branch, no? > > > > > > !page -> -ENOMEM > > > > > > page && !page_was_allocated -> already in swapcache > > > > Ah yes, my bad. > > > > > > > > Personally, I don't really get the comment. What does it mean that > > > it's "okay" not to free the entry? There is a put, which may or may > > > not free the entry if somebody else is using it. Is it explaining how > > > lifetime works for refcounted objects? I'm similarly confused by the > > > "it's okay" to return non-zero. What is that trying to convey? > > > > > > Deletion seemed like the right choice here, IMO ;) > > > > It's not the clearest of comments for sure. I think it is just trying > > to say that it is okay not to write back the entry from zswap and to > > fail, because the caller will just try another page. I did not like > > silently deleting the comment during the refactoring. How about > > rewriting it to something like: > > > > /* > > * If we get here because the page is already in the swapcache, a > > * load may be happening concurrently. Skip this page, the caller > > * will move on to a different page. > > */ > > Well there is this one already on the branch: > > /* Found an existing page, we raced with load/swapin */ > > which covers the first half. The unspoken assumption there is that > writeback is an operation for an aged out page, while swapin means the > age just got reset to 0. Maybe it makes sense to elaborate on that? How about the following diff? This applies on top of Andrew's fix: diff --git a/mm/zswap.c b/mm/zswap.c index e8f8f47596dae..8228a0b370979 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -1458,15 +1458,14 @@ static int zswap_writeback_entry(struct zswap_entry *entry, page =3D __read_swap_cache_async(swpentry, GFP_KERNEL, mpol, NO_INTERLEAVE_INDEX, &page_was_allocated, t= rue); if (!page) { - /* - * If we get here because the page is already in swapcache,= a - * load may be happening concurrently. It is safe and okay = to - * not free the entry. It is also okay to return !0. - */ return -ENOMEM; } - /* Found an existing page, we raced with load/swapin */ + /* + * Found an existing page, we raced with load/swapin. We generally + * writeback cold pages from zswap, and swapin means the page just + * became hot. Skip this page and let the caller find another one. + */ if (!page_was_allocated) { put_page(page); return -EEXIST;