[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAF8kJuNFEc6GpjzrTAF59jU7Z9OiGujqHm3086aUhG+Y141iyw@mail.gmail.com>
Date: Fri, 29 Aug 2025 11:14:15 -0700
From: Chris Li <chrisl@...nel.org>
To: SeongJae Park <sj@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Chengming Zhou <chengming.zhou@...ux.dev>,
Herbert Xu <herbert@...dor.apana.org.au>, Johannes Weiner <hannes@...xchg.org>,
Nhat Pham <nphamcs@...il.com>, Yosry Ahmed <yosry.ahmed@...ux.dev>, kernel-team@...a.com,
linux-kernel@...r.kernel.org, linux-mm@...ck.org,
Takero Funaki <flintglass@...il.com>, David Hildenbrand <david@...hat.com>, Baoquan He <bhe@...hat.com>,
Barry Song <baohua@...nel.org>, Kairui Song <kasong@...cent.com>
Subject: Re: [PATCH v5] mm/zswap: store <PAGE_SIZE compression failed page as-is
Acked-by: Chris Li <chrisl@...nel.org>
Chris
On Thu, Aug 28, 2025 at 9:39 AM SeongJae Park <sj@...nel.org> wrote:
>
> On Wed, 27 Aug 2025 14:16:37 -0700 Chris Li <chrisl@...nel.org> wrote:
>
> > On Wed, Aug 27, 2025 at 10:45 AM SeongJae Park <sj@...nel.org> wrote:
> [...]
> > Anyway, I just opened the editor to check again. Yes, if we goto the
> > read_done, the if condition using dlen can introduce a new incoming
> > edge that has len uninitialized value to be used. Yes, need to
> > initialize dlen = PAGE_SIZE or you initialize it at the memcpy of
> > page.
>
> Thank you for confirming.
>
> >
> > > I will post the fixup by tomorrow morning (Pacific time) unless I
> > > hear other opinions or find my mistakes on the above plan by tonight.
> >
> > You are too humble, that is the normal reviewing process. You can take
> > your time.
>
> No worry, I just wanted to give you an expectation of the time line :)
>
> Andrew, could you please pick the below attached cleanup patch as a fixup of
> the original patch? Please feel free to let me know if you prefer posting a
> new version of the patch instead or any other approach.
>
>
> Thanks,
> SJ
>
> [...]
>
> ==== Attachment 0 (0001-mm-zswap-cleanup-incompressible-pages-handling-code.patch) ====
> From 867c3789fa80ac163427f1d7804bf2c8667684ca Mon Sep 17 00:00:00 2001
> From: SeongJae Park <sj@...nel.org>
> Date: Wed, 27 Aug 2025 13:18:38 -0700
> Subject: [PATCH] mm/zswap: cleanup incompressible pages handling code
>
> Following Chris Li's suggestions[1], make the code easier to read and
> manage.
>
> [1] https://lore.kernel.org/CACePvbWGPApYr7G29FzbmWzRw-BJE39WH7kUHSaHs+Lnw8=-qQ@mail.gmail.com
>
> Signed-off-by: SeongJae Park <sj@...nel.org>
> Acked-by: Chris Li <chrisl@...nel.org>
> ---
> mm/zswap.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index f865a930dc88..e5e1f5687f5e 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -952,6 +952,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
> struct zpool *zpool;
> gfp_t gfp;
> u8 *dst;
> + bool mapped = false;
>
> acomp_ctx = acomp_ctx_get_cpu_lock(pool);
> dst = acomp_ctx->buffer;
> @@ -983,9 +984,8 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
> * only adds metadata overhead. swap_writeout() will put the page back
> * to the active LRU list in the case.
> */
> - if (comp_ret || !dlen)
> + if (comp_ret || !dlen || dlen >= PAGE_SIZE) {
> dlen = PAGE_SIZE;
> - if (dlen >= PAGE_SIZE) {
> if (!mem_cgroup_zswap_writeback_enabled(
> folio_memcg(page_folio(page)))) {
> comp_ret = comp_ret ? comp_ret : -EINVAL;
> @@ -994,6 +994,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
> comp_ret = 0;
> dlen = PAGE_SIZE;
> dst = kmap_local_page(page);
> + mapped = true;
> }
>
> zpool = pool->zpool;
> @@ -1007,7 +1008,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
> entry->length = dlen;
>
> unlock:
> - if (dst != acomp_ctx->buffer)
> + if (mapped)
> kunmap_local(dst);
> if (comp_ret == -ENOSPC || alloc_ret == -ENOSPC)
> zswap_reject_compress_poor++;
> @@ -1025,7 +1026,7 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> struct zpool *zpool = entry->pool->zpool;
> struct scatterlist input, output;
> struct crypto_acomp_ctx *acomp_ctx;
> - int decomp_ret, dlen;
> + int decomp_ret = 0, dlen = PAGE_SIZE;
> u8 *src, *obj;
>
> acomp_ctx = acomp_ctx_get_cpu_lock(entry->pool);
> @@ -1034,9 +1035,7 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> /* zswap entries of length PAGE_SIZE are not compressed. */
> if (entry->length == PAGE_SIZE) {
> memcpy_to_folio(folio, 0, obj, entry->length);
> - zpool_obj_read_end(zpool, entry->handle, obj);
> - acomp_ctx_put_unlock(acomp_ctx);
> - return true;
> + goto read_done;
> }
>
> /*
> @@ -1059,6 +1058,7 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> decomp_ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait);
> dlen = acomp_ctx->req->dlen;
>
> +read_done:
> zpool_obj_read_end(zpool, entry->handle, obj);
> acomp_ctx_put_unlock(acomp_ctx);
>
> --
> 2.39.5
>
>
Powered by blists - more mailing lists