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 A663EC46CCD for ; Tue, 19 Dec 2023 12:16:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E8F418D0002; Tue, 19 Dec 2023 07:16:20 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E3FA58D0001; Tue, 19 Dec 2023 07:16:20 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CE0348D0002; Tue, 19 Dec 2023 07:16:20 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id B743A8D0001 for ; Tue, 19 Dec 2023 07:16:20 -0500 (EST) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 7F223160AFC for ; Tue, 19 Dec 2023 12:16:20 +0000 (UTC) X-FDA: 81583465320.07.59ACD1B Received: from mail-pj1-f42.google.com (mail-pj1-f42.google.com [209.85.216.42]) by imf05.hostedemail.com (Postfix) with ESMTP id 9556E100015 for ; Tue, 19 Dec 2023 12:16:17 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=RJErKUrg; spf=pass (imf05.hostedemail.com: domain of zhouchengming@bytedance.com designates 209.85.216.42 as permitted sender) smtp.mailfrom=zhouchengming@bytedance.com; dmarc=pass (policy=quarantine) header.from=bytedance.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1702988178; 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=m5UnQ6ybd/a2078rBpVy5um9Db5Ao6jqQJgKdH3d4IQ=; b=gbzxsjDDbWuEFPU9ZSIppnpem+KSkJKH0WP4z+f+fGo/2aEcEX2iMJnUsL4wi0nvkkcDT5 NVO4Es5afhm4MRhlNQvIW0yytv9fknDM66t/bj+yos7WUFsdrU5GJv0AAARHwdxqODjU9s u9VT4XigzzISnQa9Iy6sfkfigerKsAc= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1702988178; a=rsa-sha256; cv=none; b=iKJXs8/73mRywJEh0vR6esLOJQA2TQmD/authhQLN0AGTycjFJbTTOKhI708yosTRSpgAK DgDHPOESDWbKMyzKiMxWggKCbqK+YRqGaD/2m/FgUcb+llf4LmJIWWfHDSVgrUiPm+945Q AglI/FO+rmqtkTL75DIaoGy0WKpq1mo= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=RJErKUrg; spf=pass (imf05.hostedemail.com: domain of zhouchengming@bytedance.com designates 209.85.216.42 as permitted sender) smtp.mailfrom=zhouchengming@bytedance.com; dmarc=pass (policy=quarantine) header.from=bytedance.com Received: by mail-pj1-f42.google.com with SMTP id 98e67ed59e1d1-28b400f08a4so2926774a91.1 for ; Tue, 19 Dec 2023 04:16:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1702988176; x=1703592976; darn=kvack.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=m5UnQ6ybd/a2078rBpVy5um9Db5Ao6jqQJgKdH3d4IQ=; b=RJErKUrg8cZYSKjjtJbVLuvSJgE5lxRka4stsK/LwduxvLAyaK9iLN+rUd7fJ4zMy4 2tlqoxlQ1n6r71W5Jft3Md5etf4v/uL9DeRhqfEX0IEsvPwmUxC8XQibbUc7ru4q1YSN +9TVgDcIPjPT55r3uhnc+ZqeYrkclf4JXQRjACYzC958a8Spu7aDIeLwAac8qMjnotcB Bz3fdBEcn7RNcSyXzegRavn2+JqKVj1sAxdhUGeJDIIIGwJ5EEJTk8GozmQvCzlvgiBS 6rWxLgVg77wHUXaHABbl+ID8xifULlexrFTWH9xQDdYTI17BhWdRSw6yoeL7QmokVPbz R34w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702988176; x=1703592976; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=m5UnQ6ybd/a2078rBpVy5um9Db5Ao6jqQJgKdH3d4IQ=; b=KIr/8wBkhdV2Oss1csaJgK1/xfKhoi4Nq4MAx2E+Rva3S7tskEMt3ZBADd2yEDnW8K JEAuck0AprH3oHfkF/tXG8jsuoPVp4Squ5NWAQuMDcOII3v5C9Fqpi8rwbXjVB5XKADF pRe/fm7z4Za/yWkWjno2F4dORtSkTSA64MBf1V0DtiRbG/4/Mb9sXQfoIFVBCYyqQjWY G8gDc92yIGe6epY+eSq+SsXbqw+fEjXBRG5MfyhmC0kjtA2XW/MDaEz4ompDQhJYLqCN vva8hdl5oCMp7DbIXDxuXbEWpDQi6TsiaNzDjjF+ypkSLtO2SwSXsOIxPuyYiZAL7fQS YOXw== X-Gm-Message-State: AOJu0YxRqU7MvbTgx5SBiiXAvJDPqw2OUI6z8U+iYaSifqKy0SIyk1b7 d+/BZolkFDL8WUZkqZyyro7NqA== X-Google-Smtp-Source: AGHT+IFd/S+IaKkm1WxAkooGuiUe3XbRP0ygCbuthZ/G8CtzqECihJGUpw6Ubay6EW2jeLrbtXeBKg== X-Received: by 2002:a17:90a:ee93:b0:28b:b87d:6661 with SMTP id i19-20020a17090aee9300b0028bb87d6661mr745170pjz.92.1702988176097; Tue, 19 Dec 2023 04:16:16 -0800 (PST) Received: from [10.254.3.151] ([139.177.225.245]) by smtp.gmail.com with ESMTPSA id ne19-20020a17090b375300b0028bc2e66f06sm509509pjb.54.2023.12.19.04.16.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 19 Dec 2023 04:16:15 -0800 (PST) Message-ID: <62bcf150-08ee-41e8-be77-57ef3bac116d@bytedance.com> Date: Tue, 19 Dec 2023 20:16:08 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 5/5] mm/zswap: cleanup zswap_reclaim_entry() Content-Language: en-US To: Yosry Ahmed , Johannes Weiner Cc: Andrew Morton , Nhat Pham , Chris Li , Seth Jennings , Dan Streetman , Vitaly Wool , linux-kernel@vger.kernel.org, linux-mm@kvack.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> <20231218145815.GA21073@cmpxchg.org> From: Chengming Zhou In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 9556E100015 X-Rspam-User: X-Stat-Signature: ukkardimgfwcisisyzh1ubu5fakp3i81 X-Rspamd-Server: rspam03 X-HE-Tag: 1702988177-843395 X-HE-Meta: U2FsdGVkX1+/5CoEpDUZkk6Tr5Zf51rA1aDVRnO/YzXx4a4Y9XZ3f1EWqkqi6zyZt5N15Mo7ROAQ//Xn53nWLiitoHvqLgN2KinZ/PCGit7DTcVyXtEaIFqiXBZ359S+Dpo2LBkXqUpT9IRTYQp+BifD+l+eLakOMJqO+wYQJTtdzUMNn6DBfpiW6htq8WGQKxIdslRKMUzYwsa3YixU2QrfPzDAgQU8zhRhBVOKEqVR0SpOHr6jwZGrA7exp+nSCx1wEGFeemr/EoyLcfJn40DS0EjdvOgW8QOmI63emVaTQk8KvJxlg30uI9fSt082Qhvyg6C5JHQC+O9UGyScGTEaqFyNf6+XJbzFgk7ipq0UKfeui8KYGP1j5bK3GoOKKxkwzsgGFTSHMr4V4ddYYmBdzl73MRiCvxMQs507clkWWV0/qvvdc7PBjnwyUtW2Czb4snVAveGwgaJBRIbxWMGWY6mOW/GbLIxuBHzxP0/YBJZfaDPEVszmXMgIYovsmy1Yq1gHALJQ14SUrltU09diPykfQ6qStYjsG8tsYAk9LsvBv/rSxcnSo3gNTXGx36sSvL5OXwgiLNp6b4jkBWLUqN6LCOmlcRMrJnmgdO+DjwISzP0scEbzhmv33+um7sH/kJj830j79nAV2mO2jd0UKlqxbepLjzfNXIteuDEIDyvoKfvAH+UVB0FADEKH+QEMmCE7cSRXlJdMtMTURYWKKJ06HduQiKKYc7Ef2cKSuiwQCJgwxP9CW7DhaGawyyBGKjOHvPKbafd+F+EPxA7kXXFmIK4YgSkBCnXKzjdE0dLwDVc2G0+M2JXRTs47N5u+BizCs7NeReiP4MKuM/cXP1xgSSNuogXxjKQbfzsKdgL+9SuvUEV/kEvqIGBSjIUv8fj34lJIFYZm/7HhsOuFj+WmbxMw9otRAUoqr3lO3dYFm3R1Ukh9/dfSXrmfxKwswMCFpGvhRxVLnQh 6bNto9js 9VMEuqGGZBOMcUyXq0DHOMIfCv0WyLc0Cybnb5JYXGmZVJzRpjRHQtjuDfidNhkdWSs3YRzFm/cQ/8fk0SbUk4j8Zm3KIE8ccWt2UD0D7qFMiNybYgPtNJwSpU7j+8Tu9zt4duSYP51IsGKgxhumrAHl7JsgO0HgfRtvqkOC7RkROFY3FvjWz4Bf2s0Yibq+rP2A8i/0IP1cGKUbTTYmnhnsOMXHQ1/O7sqr5ZNKxOccIuC5a7u/f2brlVbH7oPtCOdBod5MVYDlgGMvYMJhhUlvldUDDhyNhbp6G7b3hFqeNPnj60aq10wTdE5UjFLSwIaws9hqgR6HOZZbbZpt18xhDz7B+5DQ9eVfYxdlSrOYGo3GERXc2CWm3bO9YXCHqO83hT3LfBBtQPPSJza+UVm3ceA9vm7Q+dpt5+NiDduKSoj6JrZKAjgUmhCc4d3PZnekMaePNbLzQfk2dq08qxWuXPRqSFWhY7weQmM12TjAWx4X7EHDV1Bqdg/+zpvhS/7n0F36gyeYtm6ELxb8qUX2+ka6OAtcwEIy+ilus6G84YGufNU7VI8QQM02pzoXregveZfrBBmZWv/q1DajPtXrXdcSgVjvCV1PNkub+s/S2QpXPClZpXu7pmXPmOooECMVeLLuIAtcGTsTUH4UNyV+Xn1EqbtyKodyzZTm/uQxIg0PmdF2SrzkvoSQqUoVdpdYftp2eAN30c78= 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 2023/12/19 04:52, Yosry Ahmed wrote: > On Mon, Dec 18, 2023 at 6:58 AM Johannes Weiner wrote: >> >> 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? > > How about the following diff? This applies on top of Andrew's fix: > Reviewed-by: Chengming Zhou The latest v3 also put the comments on the wrong branch, and this diff could be folded to fix it. v3: https://lore.kernel.org/all/20231213-zswap-dstmem-v3-5-4eac09b94ece@bytedance.com/ > 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 = __read_swap_cache_async(swpentry, GFP_KERNEL, mpol, > NO_INTERLEAVE_INDEX, &page_was_allocated, true); > 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;