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 3C1B3C35274 for ; Mon, 18 Dec 2023 14:58:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A57808D0007; Mon, 18 Dec 2023 09:58:25 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A06C48D0001; Mon, 18 Dec 2023 09:58:25 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8A6E78D0007; Mon, 18 Dec 2023 09:58:25 -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 772CC8D0001 for ; Mon, 18 Dec 2023 09:58:25 -0500 (EST) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 40AA4161356 for ; Mon, 18 Dec 2023 14:58:25 +0000 (UTC) X-FDA: 81580244970.06.F899423 Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by imf08.hostedemail.com (Postfix) with ESMTP id 5887C160022 for ; Mon, 18 Dec 2023 14:58:22 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b=TdEHdT6k; spf=pass (imf08.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.128.48 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1702911502; 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=T2lzs4wznIp2xbwMuaXpnCrk161AzmSWNsE1VAdzIps=; b=bGyEK6k7aE0e1M9ZiRII6nV+N8v1dUcJ588ESt1MWPLK3VT53x43fFg32IyBcB4PEf03TX wXYY5fo2tSSTCejFfqXIPTMTC+mUBOtyVzPYMtBctzSz5IKuMERiljKAXjCqUa1np/kDDX 6EWELIMqSbLlxL+pulbhMktt8Iitx6c= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b=TdEHdT6k; spf=pass (imf08.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.128.48 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1702911502; a=rsa-sha256; cv=none; b=RpJaRiVoNBPhDDk4cZwq11Y19eN2gbp7sM+gjhA9+C2yZRzNqTkSL5oPwTyS4k607ibrCR 18gqdzjTU+nU79SdhDHueGCIWFX9zvVKyUaERuQRt17zumsINPJgxur1VWrnp/0Y0ih7OD 6YvOobwbIi7BcEhwxfsW4pCfrrTzsVg= Received: by mail-wm1-f48.google.com with SMTP id 5b1f17b1804b1-40c41b43e1eso39757055e9.1 for ; Mon, 18 Dec 2023 06:58:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20230601.gappssmtp.com; s=20230601; t=1702911501; x=1703516301; darn=kvack.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=T2lzs4wznIp2xbwMuaXpnCrk161AzmSWNsE1VAdzIps=; b=TdEHdT6kKRPPulR5KsHQA4yBCZ5danlqjVDF4xXV48oJrS2b/R5jDu3v3CC5ZOgaV2 XTsc9/lmpo2f3v6F0bKqv9rR8C93C2wO+Fn9Ut+Jr+ByhoH32xuv+vJ3ULQE9vlfj75r KzalEJUKPXIRRtgGlc+3heLX3qP6k/YrWDcMvt9f6QQC26I5LGnO2sD8T+ZW6+wmbEQy OHTpHuS/jbftfjt+K2RBJEEjxiWSwWqWzk+s9tuH86vXPW7z0NWov7ioNL+3Ip106jkJ dbSHkneYSGPCyTBz29ScVTILTROd12c0WHvDgq1op18wIytw1ni4Te6YpVEx6nnShknn Q58g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702911501; x=1703516301; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=T2lzs4wznIp2xbwMuaXpnCrk161AzmSWNsE1VAdzIps=; b=cWVt6q4S6a/J878GRYS/WUi5Q5cvKj4V+YDWPahxWvIG4CE5fwsLNcUgtB8KLLlUDk bD848Zt0eEIRDxHgW/2Dr1VdhSu0Rsmj+WicKEuuLwiPlDfnKDRay2/fEERDtRk3dme0 pzcpjmPLSCpXLk2JiQXJ65nL5tw4X0LCrDB4tPpTB69VJ6ykSSISCrsw7HmqMSTyrJPv WHeqiHJds3jVzv2sUUAoOj3f6Z83Kx0rnRez703wF1M2jvo8uLX05mcCb9AhMD8mK9+v QTZiGI51TG+eNByT1d50vgF4RUquCR4nXapyvIHBTm6NgQOQiDmmMfyK7KdEKxSgxDSd 5VUg== X-Gm-Message-State: AOJu0YyN6I54xsBTrHgCkSecpATiHE8arZGCTJg9Pux2YgqB1nnevyLw q7SRhRVuzq2UrIL+EHKF0/WCfjN9NhM++Q4MYEs= X-Google-Smtp-Source: AGHT+IHm5i4U4iCbK1WO2wBoHSz6xtDXsSSWbqr+KjfCPJ/PiFtLd2RWtKwkraO4kBXCUu/g7w6Hvg== X-Received: by 2002:a1c:6a1a:0:b0:40c:6a3f:6bc2 with SMTP id f26-20020a1c6a1a000000b0040c6a3f6bc2mr2796111wmc.30.1702911500633; Mon, 18 Dec 2023 06:58:20 -0800 (PST) Received: from localhost ([2620:10d:c091:400::5:bfe6]) by smtp.gmail.com with ESMTPSA id f18-20020a05600c4e9200b0040d18ffbeeasm5305205wmq.31.2023.12.18.06.58.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Dec 2023 06:58:20 -0800 (PST) Date: Mon, 18 Dec 2023 15:58:15 +0100 From: Johannes Weiner To: Yosry Ahmed Cc: Andrew Morton , Chengming Zhou , Nhat Pham , Chris Li , Seth Jennings , Dan Streetman , Vitaly Wool , linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH 5/5] mm/zswap: cleanup zswap_reclaim_entry() Message-ID: <20231218145815.GA21073@cmpxchg.org> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspamd-Queue-Id: 5887C160022 X-Rspam-User: X-Stat-Signature: 6ft38it7hq96d5j5rjemaremaiezihee X-Rspamd-Server: rspam01 X-HE-Tag: 1702911502-257511 X-HE-Meta: U2FsdGVkX1+DYcIEL7D+t2F+baKjixZDvSDkS9oYcHfxeNZTrB0WBxxyAutO6nJ/GmkXwMId6RQCR9vQmm6enR23EHQzBbFmw2sGrW97nmyM2Mg3++9a24sW5+iZDEz29VwNJTbOMwc5p7bFie8iBht32kmCj0KqG/DmbSWtbzhy0Q9goIi2Jfz5NQPk/JsGQBEj98xAwvoIVWN55t7pK8SlSG6RVnRwhqQMQ28SxObO2YQzKNfQsqtBGdDm4PorCYNbfWGEpxI1lCivvflEo7TZPhd4jPTnyy/qFNeKEtTCqMeyNBNqDcEZlstNyz1oW4R2jGj46dyUZUWzwKJKgbXJtwxoIBcKFN33aZzvxBpdyP7EiZat8XyvXWyjShE3GAo8ygjY4hup/eSf/0EF+UIWvegekIXPIIdUw57712OEtnBRA9N04eG9M3nEAqD8ka0HS6zMImD/4Bh8B6isjseB4VysTsHx4nyHSowWpmpJ51CiK1A+tGRwzjcKO8638tIJ9ZdpzRYtUuZL2OAXs3sVdI3EjIrG6OtcU5tYqavHvaWGcrPPzujdEfwBU5Ma9CTXgpWHl4rj+2jJKR/2TSxDQyJSFcREvlcgbphZZzufIgYXk1UtreVZ9AnwStvQ0QMwZc2Juq9QRVNDtPEQArmC0n7Ak0s0Ys+2IADvTKD3u0kqnoBOGOqLmhGzOJoTlDybagf/kqz9NjNbXX2pdaYQMXNHI4iXFycoV2mnEyVjwsbTNP4Kl1EtwZKLmdRdJuK7XgMOE47ulkX1DyayK0UKMf9W7NyBkYR8NZYOi18joSzLaBWcOcwQaMV/aQ9sgWIe4VVMmQN2Y41WCe0BljN0We+WJExaYITwnNHf+whkKeQs2fHrL9CUXJ0kkIc7pAB3bMdjne3fzisa8gY2E7z8BjiXYGSxF348EQxOFEiCg+nt6nNEQP/dM6SgP2aQ6KnWwhNz60A86t1wnVx Abuz8I/F 6uVP5RlQJAZJRCOY6EVmPtkOEb0oIOC03KCA6nKz2HOyKKes/0kslp0I0ZiTv8OAtr2TJ/2MkMXHTDghWSMpXYlq7rpat6ODc6AqmllsV28gvKS8+5wabQeacGa7fBfz7qhaowXoJrp7QTkorgGfJcxp0HS9QhQ0SC9pgf9Tt8jvKWXx12/IpefQeRCjQ0iv8m9JWVPty633xi2h+iAdY6UdDyJ3QGTkuNU6Yk+958mEjXEECqTl+qEsy45/smT8bzuxLrJO3MIvFcQdr+BSx/810KxvXl1Ubb23wRkdgyXy0F++hxcYNXIAWaX55V3i0YiscqCnheQ0eDJMcAAI+YbOfk+VBf+tPoLpP6ymvXUJ2tBKXaQ7+yRpZq2I7itkQdguX8Ty5y0nKeM3p0qqX65qXexYY6LMn5RJWFQ7NAhs9xrWWy1GLcM/qAcaB+nhTo1uwu9vkKZYq2/4d4J4jKJcwus65JaNyRMv0Hvwr+SEKcc+KzsZRJioQwc/B8TxuJxWx+YlfcrgJTSmy8b/skWKOdoNt1hGmdr+qi6/R7zEdvcNTXPep6idcu5HIHz6QCEX/7nIbLvNH2yBqKbzpwxxFhUze5NsPMLVkCaq/8lGo93KOU3gVY2Qd0DmjgU+HVoFjyrLlcICMuIsCWAnWlcKPR2zsNKIowDC+ZmbwViOFdHmBCxX/RrFSdyihM0TTRkuHMlc0cjaTlPgkyxLsUmnIOE+9CXQyB3pWZDN9NZWU76Iy/W/ICmiGhw== 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 06:39:13AM -0800, Yosry Ahmed wrote: > On Mon, Dec 18, 2023 at 6:03 AM Johannes Weiner wrote: > > > > On Thu, Dec 14, 2023 at 02:41:26PM -0800, Yosry Ahmed wrote: > > > On Thu, Dec 14, 2023 at 2:23 PM Andrew Morton wrote: > > > > > > > > On Wed, 13 Dec 2023 17:02:25 -0800 Yosry Ahmed wrote: > > > > > > > > > On Tue, Dec 12, 2023 at 8:18 PM 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 swapcache, 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 = get_task_policy(current); > > > > page = __read_swap_cache_async(swpentry, GFP_KERNEL, mpol, > > > > NO_INTERLEAVE_INDEX, &page_was_allocated, 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?