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 4098EE77199 for ; Wed, 8 Jan 2025 04:28:27 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3DFFA6B0089; Tue, 7 Jan 2025 23:28:26 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 3914A6B008A; Tue, 7 Jan 2025 23:28:26 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1E36C6B008C; Tue, 7 Jan 2025 23:28:26 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id F1E156B0089 for ; Tue, 7 Jan 2025 23:28:25 -0500 (EST) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 91ABD1A038B for ; Wed, 8 Jan 2025 04:28:25 +0000 (UTC) X-FDA: 82983002970.15.8C4BBA3 Received: from mail-vk1-f179.google.com (mail-vk1-f179.google.com [209.85.221.179]) by imf12.hostedemail.com (Postfix) with ESMTP id B934140004 for ; Wed, 8 Jan 2025 04:28:23 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=Tlyx0RHo; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf12.hostedemail.com: domain of yosryahmed@google.com designates 209.85.221.179 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736310503; a=rsa-sha256; cv=none; b=ZCqF8R67DQ0Xq1c/K6/5ews5CSw5bjKT1TUv8nddNvxDb9HQxSBILhQTTB7yFotGE2Uy8k y6jcQfs8NsKhCzOrikxvvKJqJw/cG+TAwG1G3J/UoiUnJw2CX7BCi2RiYlJaJi04MiLNww M6PXIJ5NmqXoKxr06amFkXAwFMXk8vc= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=Tlyx0RHo; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf12.hostedemail.com: domain of yosryahmed@google.com designates 209.85.221.179 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=1736310503; 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=dGPpQ6VJIV9xnsCD1bMq8dKJH1L2LIGUbC/YN69ZewQ=; b=jivJCtNhPQ7bgqGQz7r9E/gHBYpCjkH4cgDowAmUJ50h1wY6yNbVZOw5X7DCdwBdcBenpH gyrfYIsFou5hsgPXxVFzz8PBkZHoIVwqDzA+6arn7qslG8yU1tR5tqmP9BRDuCU7e/1NyC Nb2Mnm3YJZmVaq3G5Z7kXQu96k9UHXo= Received: by mail-vk1-f179.google.com with SMTP id 71dfb90a1353d-5174d53437eso9604667e0c.2 for ; Tue, 07 Jan 2025 20:28:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1736310503; x=1736915303; 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=dGPpQ6VJIV9xnsCD1bMq8dKJH1L2LIGUbC/YN69ZewQ=; b=Tlyx0RHoGa/uE2ce5SnuJTD7DVgEh2kjiMg9egw+jJOvlVxAPX6wjZsyveZXWBBnaV WvbFDDzNW2j+5uiHCCFIwpzF+Zyunij5R3NeaKDfSBYMOXR68xpKtL+MsUJfKszr3G3j ES22TQ0G3el05MCtcCUwK78URmvscUOHw0j+5bAaAuEjyC7NFjvFKj6A4FOTqmBE0xJg 9lUx98MDyXAsnvMvALZympEtbuL2KT5sbOdWTn4qsQQzrQfVogt1DzofyL9j2MjqvLGV M1C0oVJrkGxQ32xSRiGCVy+UQxPwXUCXkKeluUHyBjl8AAM8dvXIyzH8JwRytCbDPhtN S7jw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736310503; x=1736915303; 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=dGPpQ6VJIV9xnsCD1bMq8dKJH1L2LIGUbC/YN69ZewQ=; b=sIiSmwb3X6bMdKskZQzmC2FNTI2oEti2l1ZtVnbh8eHOx5ZJnfiAoldFkQuoczdQhg /i06PSr6WMdjdDWSHb4Az0cd3gc4gAw9yd1f3XlzwS5opgRGnfVtOMcwcu1koUfOI18E 6DGtTGoo+K1/Qez733SY+JR9bciEt2Mr6SV8Nx1+VS6GPq2dRA3MQDA5tr2s4DlDbKCL A+T/Na0LBY/rk0bPQXUPfe1HjhM+aj4OsyV7UqkxlEakrzFtx5A4TBeJjERqKJhdoz1N VwkteEMWiujt5owX4BxUnvDkqDfiyKfUzjRQzJuW2gIX9WHJuyR3VsnUnwoOojlCmtgV RMhg== X-Forwarded-Encrypted: i=1; AJvYcCXUbbCcXtJIxa7jKHh2kBhGaklBcgknjWNxvC8kobuI3vXzDUB7Yn+23mWtELTIhtWI9hLWgj8xQQ==@kvack.org X-Gm-Message-State: AOJu0YxYyzQEXLavFZ6KquViJ8J75skTpnaC2rRQRhkFoUTSdNB+DYa/ f6mdJFYT+MFwn3+Gis3bv+MLTwxJ+klQBcnTDRpgkh8bvlKGWLs97o4HBrtMXfNXv0u1xiuLMEO wKPDldlHdES7RcShjFhn0fMrZ9EQLWo0Mb6ZvYJnFZQvvsJqnhr7c X-Gm-Gg: ASbGncsR4orM0LQ12CX+oOorv+aJRgIGzcYVuh7yjQjWAZv4vPaBN9Y7QaBk+umWnw+ Pku6HvXz1Gfv+AA1tWJuuQ9frd4pUIre5ACY= X-Google-Smtp-Source: AGHT+IFJk+LnjgzfAgjKMHkGNMZINBuze+9KKgJnWmwTidN5rr18hYT2maqgMy22Lp3yaIP9FHYwKKIkEhmdmvdMiN0= X-Received: by 2002:a05:6214:2263:b0:6d8:871d:49f1 with SMTP id 6a1803df08f44-6df9b2d875amr26384756d6.44.1736310185822; Tue, 07 Jan 2025 20:23:05 -0800 (PST) MIME-Version: 1.0 References: <20241221063119.29140-1-kanchana.p.sridhar@intel.com> <20241221063119.29140-12-kanchana.p.sridhar@intel.com> In-Reply-To: From: Yosry Ahmed Date: Tue, 7 Jan 2025 20:22:29 -0800 X-Gm-Features: AbW1kvahJqqOC4RLQWaNTlCxRD-xLkUmXDWJfgFp5dXM9_qBnCBSg5cYKQkB1W4 Message-ID: Subject: Re: [PATCH v5 11/12] mm: zswap: Restructure & simplify zswap_store() to make it amenable for batching. To: "Sridhar, Kanchana P" Cc: "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" , "hannes@cmpxchg.org" , "nphamcs@gmail.com" , "chengming.zhou@linux.dev" , "usamaarif642@gmail.com" , "ryan.roberts@arm.com" , "21cnbao@gmail.com" <21cnbao@gmail.com>, "akpm@linux-foundation.org" , "linux-crypto@vger.kernel.org" , "herbert@gondor.apana.org.au" , "davem@davemloft.net" , "clabbe@baylibre.com" , "ardb@kernel.org" , "ebiggers@google.com" , "surenb@google.com" , "Accardi, Kristen C" , "Feghali, Wajdi K" , "Gopal, Vinodh" Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: B934140004 X-Stat-Signature: cnreh6xs4gnc8uax7ioi5sj9ed85eazj X-Rspam-User: X-HE-Tag: 1736310503-154037 X-HE-Meta: U2FsdGVkX1+kJeJbUoGNbPm/KHLi4fqpd/aBWXoG8M8toxowLQuIgU2hsW+EBIvr7j2ylDWUHqqq1wMPAaOgZgayvznKybtWA0CXLfdzTo2k+hyPHF5W8ZdHlZt+vDuGTXhdzaENWYi0sbXToq7EvzkukSfaWxfFbHNF5sY+h8+0M86pjH7LXB10MPlgj5LbnUDItlRB2O15iS+DLNRYg9TW6szs/KJyiM2FRMykF3JYYXhpEMpdBjNcPR4OI1tHW9EhVJvo2RrD/uLTsw0xfGjCAK5ywGv7OLdr3d5C8zev4Z3zVtS/sQbvgDjnULOl1IuU+f8qfVkt2dNgjbCQtCoWUpREl6wp0Uk2p8V84q7yO9bshfQcC1BWL5gt3okQ7snEGawKlgafXkx9uayTJdTlWtoVqPCazXEBTMbGfaWf/BcA17LZ5tuM2jhoP1yyLPHbJS02qcdg4Bo6gakhI9T8H796jgH4lGRg1fFYZh+n5N/1AZVIxgPmuqZPABjlxcJVLFjieVgNgfSxioF1jy36qsUFV8vy9KgLar9ERuSyrpElV/FPJH9k5AIHNDMzyqEXA7CB6QNkgq2NqVA+om2iw71pq6plHe32ecFhAc6CUniw+ukkP0uFm9kHClNeYaTN3wkoGxF+8ECCIuRCMr3SgPM/JvfkBABfdmJsJ38dIiWmyZ4qL5F+c/S8CbZJ+Xob5Q8hnEjXHSez4LEOPhXBhnwZ6/E/uTuU2Q0PC/EfgmGjQWAd0Pzjee50b4EPDhbLGn4tITTZqBba/5inmK6NVg1SxS+Hb+cntAisw3KwuiE666puPHxsKwjcrwbmOsZmi+mELMaKB6faWslGxi2v3MljWxvggrTgQmA37PKLOCdJ0PUyxbjWXOaMfiGKlFzwtTRKpTCjT3dg6JuR2wyT5NXSQ0VecRwiKdyZzTe25l1qbdkhxzjwiCGQtrPVLNy6vzvyKMM55rMhrun eo0De70p zbjuCIDfFj4hGvbPNKuVMBTjjHenAVD56dNgi4ETJFLXceofkD/j70QaAMkddzfPyNZb5YLTZmdfOjC1xKLpGwiA1+ORZTD67s1+hucjQ9IS/TfnpJJx9jhSpuqR1+1ahBEef6qQRO0VVt7APCPuEwCSk5p3LuMFjOOk7Wr64idDVjGaabSmLhsvQMVX/pYY3IAIWVkz8C5xE1SVyTXN58rRoQBEnoVrUFhoiU7X3XHefs1yMB1O56/MayoHc7tx7rTKWpgvRuNK+DZqz767iXp7AGM8dYO1crOYx 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: [..] > > > diff --git a/mm/zswap.c b/mm/zswap.c > > > index 99cd78891fd0..1be0f1807bfc 100644 > > > --- a/mm/zswap.c > > > +++ b/mm/zswap.c > > > @@ -1467,77 +1467,129 @@ static void shrink_worker(struct work_struct > > *w) > > > * main API > > > **********************************/ > > > > > > -static ssize_t zswap_store_page(struct page *page, > > > - struct obj_cgroup *objcg, > > > - struct zswap_pool *pool) > > > +static bool zswap_compress_folio(struct folio *folio, > > > + struct zswap_entry *entries[], > > > + struct zswap_pool *pool) > > > { > > > - swp_entry_t page_swpentry = page_swap_entry(page); > > > - struct zswap_entry *entry, *old; > > > + long index, nr_pages = folio_nr_pages(folio); > > > > > > - /* allocate entry */ > > > - entry = zswap_entry_cache_alloc(GFP_KERNEL, page_to_nid(page)); > > > - if (!entry) { > > > - zswap_reject_kmemcache_fail++; > > > - return -EINVAL; > > > + for (index = 0; index < nr_pages; ++index) { > > > + struct page *page = folio_page(folio, index); > > > + > > > + if (!zswap_compress(page, entries[index], pool)) > > > + return false; > > > } > > > > > > - if (!zswap_compress(page, entry, pool)) > > > - goto compress_failed; > > > + return true; > > > +} > > > > > > - old = xa_store(swap_zswap_tree(page_swpentry), > > > - swp_offset(page_swpentry), > > > - entry, GFP_KERNEL); > > > - if (xa_is_err(old)) { > > > - int err = xa_err(old); > > > +/* > > > + * Store all pages in a folio. > > > + * > > > + * The error handling from all failure points is consolidated to the > > > + * "store_folio_failed" label, based on the initialization of the zswap > > entries' > > > + * handles to ERR_PTR(-EINVAL) at allocation time, and the fact that the > > > + * entry's handle is subsequently modified only upon a successful > > zpool_malloc() > > > + * after the page is compressed. > > > + */ > > > +static ssize_t zswap_store_folio(struct folio *folio, > > > + struct obj_cgroup *objcg, > > > + struct zswap_pool *pool) > > > +{ > > > + long index, nr_pages = folio_nr_pages(folio); > > > + struct zswap_entry **entries = NULL; > > > + int node_id = folio_nid(folio); > > > + size_t compressed_bytes = 0; > > > > > > - WARN_ONCE(err != -ENOMEM, "unexpected xarray error: %d\n", > > err); > > > - zswap_reject_alloc_fail++; > > > - goto store_failed; > > > + entries = kmalloc(nr_pages * sizeof(*entries), GFP_KERNEL); > > > > We can probably use kcalloc() here. > > I am a little worried about the latency penalty of kcalloc() in the reclaim path, > especially since I am not relying on zero-initialized memory for "entries".. Hmm good point, for a 2M THP we could be allocating an entire page here. [..] > > > @@ -1549,8 +1601,8 @@ bool zswap_store(struct folio *folio) > > > struct mem_cgroup *memcg = NULL; > > > struct zswap_pool *pool; > > > size_t compressed_bytes = 0; > > > + ssize_t bytes; > > > bool ret = false; > > > - long index; > > > > > > VM_WARN_ON_ONCE(!folio_test_locked(folio)); > > > VM_WARN_ON_ONCE(!folio_test_swapcache(folio)); > > > @@ -1584,15 +1636,11 @@ bool zswap_store(struct folio *folio) > > > mem_cgroup_put(memcg); > > > } > > > > > > - for (index = 0; index < nr_pages; ++index) { > > > - struct page *page = folio_page(folio, index); > > > - ssize_t bytes; > > > + bytes = zswap_store_folio(folio, objcg, pool); > > > + if (bytes < 0) > > > + goto put_pool; > > > > > > - bytes = zswap_store_page(page, objcg, pool); > > > - if (bytes < 0) > > > - goto put_pool; > > > - compressed_bytes += bytes; > > > - } > > > + compressed_bytes = bytes; > > > > What's the point of having both compressed_bytes and bytes now? > > The main reason was to cleanly handle a negative error value returned in "bytes" > (declared as ssize_t), as against a true total "compressed_bytes" (declared as size_t) > for the folio to use for objcg charging. This is similar to the current mainline > code where zswap_store() calls zswap_store_page(). I was hoping to avoid potential > issues with overflow/underflow, and for maintainability. Let me know if this is Ok. It makes sense in the current mainline because we store the return value of each call to zswap_store_page() in 'bytes', then check if it's an error value, then add it to 'compressed_bytes'. Now we have a single call to zswap_store_folio() and a single return value. AFAICT, there is currently no benefit to storing it in 'bytes', checking it, then moving it to 'compressed_bytes'. The compiler will probably optimize the variable away anyway, but it looks weird.