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 03F68C46CCD for ; Mon, 18 Dec 2023 14:39:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2FDF26B0072; Mon, 18 Dec 2023 09:39:55 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2A5526B0075; Mon, 18 Dec 2023 09:39:55 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 16D016B0078; Mon, 18 Dec 2023 09:39:55 -0500 (EST) 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 0943E6B0072 for ; Mon, 18 Dec 2023 09:39:55 -0500 (EST) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id CB9BA80BA5 for ; Mon, 18 Dec 2023 14:39:54 +0000 (UTC) X-FDA: 81580198308.22.451030B Received: from mail-ej1-f54.google.com (mail-ej1-f54.google.com [209.85.218.54]) by imf18.hostedemail.com (Postfix) with ESMTP id 22A4F1C000C for ; Mon, 18 Dec 2023 14:39:52 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=bRMIxAVk; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf18.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.54 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=1702910393; 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=0pTaPp82Aa5VZmUHLjVfWpth1QdXOXHq0YobrMTPaUg=; b=H2JddzPGQ3MRsp1UZLm5eSi1MkPi57stEC0bCKGi0dsa3eYHg7W8r1JC+j1PAG6hvCvc/H YFu5E5EYRDLFRuBHjvQ31NzJPEinokmD31qQqg+xS267tqOArriyOi/1jHfuR7mn6DNCxI AFxV1KGlX2gfJHvOIq1mk+s85CXTaV0= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=bRMIxAVk; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf18.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.54 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1702910393; a=rsa-sha256; cv=none; b=nQf3UkHkV0mXO+VVNJ/gNaMR4D0Y6gCGPU96pX4lFzR5m0DWGpWWsPEgNm+Wl02yqM691z phChNzE32o7cuELNSpRllKV3rZuIq9mMqCLa3nThnj0m8NfRzQbTIlELpUaRHpqncvPTEY pHuZ88wD1nRZbvQ5fQK4r8p8V0Sj+0A= Received: by mail-ej1-f54.google.com with SMTP id a640c23a62f3a-a236de428a5so5339266b.0 for ; Mon, 18 Dec 2023 06:39:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1702910392; x=1703515192; 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=0pTaPp82Aa5VZmUHLjVfWpth1QdXOXHq0YobrMTPaUg=; b=bRMIxAVkC0V09yENs4JlbScoGw0FP0mEb7cjBsXm7IgXrcFpknySu0W0u288xTBDoz 2nWYIftzBEt3EBHjdu4jkxcnugdxRy1ZIlvpPVGLCty5orXW4H8AikdOKHT9p+T9NxU9 fJwvvpDaP/RK1qBbEb3hr5AK7ppAwBR1zQQng/Ab21MdZtoNgO5fcDB2GhLOXcOkxH0V btpzxzO5hQrih89EURbBuLJV2QClJNitKaO2On3F6l77AAYLgDUECEciWZL8Ay7a3cRQ QY15kAxpcjHL10mw+dJd2MTtvrHuXTJi+IfH0i91CIDUCaNoKbk7rCk3B//u/XgnYKDx KMaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702910392; x=1703515192; 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=0pTaPp82Aa5VZmUHLjVfWpth1QdXOXHq0YobrMTPaUg=; b=PHx3SpwD8eels4eq2dUMl/L5+PXMbjIGo1JkS9FJJrTkrvvZcOnZWQ/rK3t90s2g31 G8ABy4gtgIOJnZtWkSFtvoOoFCwz50SDsOPthenSjNNiSyQmkN5vj60CHCqL5qxxvKqi hVXbVQc6IPLvdbjM1SSmzdcdjquULqO569qES9HpL0j4o8mIAPMhluWDjtaFkQs6p2Pp z4jI7IWAA1xuOQ78XfISuSY3bsswf0Gf3VSgtd5j6bgceCwvmasdkJewQiYX/pFRtvud 6oeChaX+nwvvdbk9k9jUF/UgI0RceEPGt0GvTmg/7TE+0KCodtIMR4vdgQpCLVsLXrZ9 MUKg== X-Gm-Message-State: AOJu0YyxBckQ4uHnUzHU9vufx8W53c54pUTI9/J3W5wMlcAXSLzloTee L1W3lYrV+wwtUrQWoeykslLoSWMUiYsmp1nD3wXUxw== X-Google-Smtp-Source: AGHT+IErhmQSsH2j5h0Y+N6BksWa046h7FS1zKFIgfG1GpHJ04zswD4nFe+yo9vE93FPqIp1665sVEPOkMGUwGXIA10= X-Received: by 2002:a17:906:6847:b0:a23:5412:c4a5 with SMTP id a7-20020a170906684700b00a235412c4a5mr1280318ejs.61.1702910391612; Mon, 18 Dec 2023 06:39:51 -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> In-Reply-To: <20231218140313.GA19167@cmpxchg.org> From: Yosry Ahmed Date: Mon, 18 Dec 2023 06:39:13 -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: 22A4F1C000C X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: i4oqt5foddcw81zo1ap6ixn4457x47xx X-HE-Tag: 1702910392-58237 X-HE-Meta: U2FsdGVkX180kP7QrugkXWc7m6aSyeIAInbuYju4YAFrro2qmql7+cV7eToxz0rI+hE6u0i45d+9m0ErqdL+aAYLDk9G6iK91AdMz+x8N++FN7OPB+lykK+nGHhXf7OmHmYo36Teajy3dqtLX8lLSrUHsutmy+eANT59vSWnNuhG22jNvMTGIxuTfic08YzJfjRnW68uSYjPvUIqO7Of0WokccwW9MW7bQMTkbErgdUdCdfWgShuDUiPG4crfufl55zl71xHVq4BDj2vdtH1sgJw1uvRUVZ1EoZhCy4qv5J0bxezlUaEY4gopE0MV6aKR55hQvuh8uheji53zxIWr2J/FQEA13Vc19Xd/7yb5n6JU6I7E2yu365oSs0AOiP0dr1koOU+un+wfoPhaar366ibj/oLXqhdBZKPD2n9Z9n/wQNy1vxwTYLBZuLpWvYP4GcnGhpdF6SSiYCMRvR5Mk/G2YoNbW1X7yNWQXRFsNr1cIUVChoCSJctyjGxeQwtifamsYtea1kLt7ClRIDhbJLcPVpuwpQl+c0oPY1v4gXGrLbuxBnqoHpMDzCYFS0crUxpnyd6ncKr1ZNjGxjcbzTTqBCkR7PgWPwWbKX+WBnPuVEJ7h20erruN/eYNC15bElA8b9VHX8q8zir5updmLbsn93d7NT2389FowwwLv4Smx7XAYrNU/u026j8fuMu/YHqPQeXpGs3BkHGyhd8h6O85ogkQx0gSvBYRZeoVsfAceKJ+1O971YdQEhS0YvVJMCN/jqraHot8BGrKruY5GXnaFcpVs5uhmBH47jtC/iOI8i7cm2TmH1c3Qrkk/Wv5lJNWl51+aJDqLGN0g++Fky7OygrKmbMggNKnxUvS4RP6y5gKm3l+Kgk01coPwpAAPrcM7fx6pm9G3eBfw9fGFZRHwfKR7eA9xUMu7EEjdsNpWa/AWSGDKhdCusbpQhmb9NabhSPHe2hjuygOw6 9dD8GuNZ F08kkaR4TA+bnkagfAJsUA+DzGaawqks3fUqlN53gcDy0u38= 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: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 title= . > > > > > > I updated my copy of the changelog, thanks. > > > > > > > > - /* > > > > > - * If we get here because the page is already in swapcach= e, a > > > > > - * load may be happening concurrently. It is safe and oka= y 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, mpol, > > > NO_INTERLEAVE_INDEX, &page_was_alloca= ted, true); > > > - if (!page) > > > + if (!page) { > > > + /* > > > + * 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. > > > + */ > > > 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. */