Message-ID: <CACePvbWzyqJJxP8BFXS_NDLcXCz-YXkt8eYBxv3CER9RpnJVXA@mail.gmail.com>
Date: Wed, 27 Aug 2025 10:33:38 -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

On Wed, Aug 27, 2025 at 9:18 AM SeongJae Park <sj@...nel.org> wrote:
>
> On Tue, 26 Aug 2025 08:52:35 -0700 Chris Li <chrisl@...nel.org> wrote:
>
> > Hi SeongJae,
> >
> > I did another pass review on it. This time with the editor so I saw
> > more source code context and had more feedback.
> > Mostly just nitpicks. See the detailed comments below.
>
> Thank you for your review!

Thank you for the good work.

>
> >
> > On Fri, Aug 22, 2025 at 12:08 PM SeongJae Park <sj@...nel.org> wrote:
> > > @@ -971,8 +975,26 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
> > >          */
> > >         comp_ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
> > >         dlen = acomp_ctx->req->dlen;
> > > -       if (comp_ret)
> > > -               goto unlock;
> > > +
> > > +       /*
> > > +        * If a page cannot be compressed into a size smaller than PAGE_SIZE,
> > > +        * save the content as is without a compression, to keep the LRU order
> > > +        * of writebacks.  If writeback is disabled, reject the page since it
> > > +        * only adds metadata overhead.  swap_writeout() will put the page back
> > > +        * to the active LRU list in the case.
> > > +        */
> > > +       if (comp_ret || !dlen)
> > > +               dlen = PAGE_SIZE;
> > > +       if (dlen >= PAGE_SIZE) {
> >
> > I think these two if can be simplify as:
> >
> > if (comp_ret || !dlen || dlen >= PAGE_SIZE) {
> >               dlen = PAGE_SIZE;
> >
> > then you do the following check.
> > That way when goto unlock happens, you have dlen = PAGE_SIZE related
> > to my other feedback in kunmap_local()
> >
> > > +               if (!mem_cgroup_zswap_writeback_enabled(
> > > +                                       folio_memcg(page_folio(page)))) {
> > > +                       comp_ret = comp_ret ? comp_ret : -EINVAL;
> > > +                       goto unlock;
> > > +               }
> > > +               comp_ret = 0;
> > > +               dlen = PAGE_SIZE;
> >
> > Delete this line if you use the above suggestion on: dlen = PAGE_SIZE;
>
> Thank you for nice suggestion!
>
> >
> > > +               dst = kmap_local_page(page);
> > > +       }
> > >
> > >         zpool = pool->zpool;
> > >         gfp = GFP_NOWAIT | __GFP_NORETRY | __GFP_HIGHMEM | __GFP_MOVABLE;
> > > @@ -985,6 +1007,8 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
> > >         entry->length = dlen;
> > >
> > >  unlock:
> > > +       if (dst != acomp_ctx->buffer)
> > > +               kunmap_local(dst);
> >
> > I think this has a hidden assumption that kmap_local_page() will
> > return a different value than acomp_ctx->buffer. That might be true. I
> > haven't checked the internals. Otherwise you are missing a
> > kunmap_local(). It also looks a bit strange in the sense that
> > kumap_local() should be paired with kmap_local_page() in the same
> > condition. The same condition is not obvious here.
>
> Good point, I agree.
>
> > How about this to
> > make it more obvious and get rid of that assumption above:
> >
> > if (dlen = PAGE_SIZE)
> >                kunmap_local(dst);
>
> However, if the execution reached here because compression failed and writeback
> was disabled, kmap_local_page() wouldn't be called, so we could try to unmap
> unmapped memory.

Ah, thanks for catching that. That is why having more reviewers the
bug can be obvious.

>
> What do you think about adding another bool vairable for recording if
> kunmap_local() need to be executed?  For example,

Sound good.

>
> """
> --- 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 dst_need_unmap = false;

A bit nitpicky. That variable name is too long as a C local variable.
We want local auto variables to be short and sweet. That is why you
have "dst" rather than  "u8 *destination_compressed_buffer;"
The local variable name is too long and it can hurt the reading as well.
Can we have something shorter? e.g. "mapped" or "has_map"

>
>         acomp_ctx = acomp_ctx_get_cpu_lock(pool);
>         dst = acomp_ctx->buffer;
> @@ -994,6 +995,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
>                 comp_ret = 0;
>                 dlen = PAGE_SIZE;
>                 dst = kmap_local_page(page);
> +               dst_need_unmap = true;
>         }
>
>         zpool = pool->zpool;
> @@ -1007,7 +1009,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
>         entry->length = dlen;
>
>  unlock:
> -       if (dst != acomp_ctx->buffer)
> +       if (dst_need_unmap)
>                 kunmap_local(dst);

Yes, that is good. Make the kmap and kumap very obvious as a pair.

>         if (comp_ret == -ENOSPC || alloc_ret == -ENOSPC)
>                 zswap_reject_compress_poor++;
> """
>
> >
> > That assumes you also take my suggestion above to assign dlen PAGE_SIZE earlier.
> >
> >
> > >         if (comp_ret = -ENOSPC || alloc_ret = -ENOSPC)
> > >                 zswap_reject_compress_poor++;
> > >         else if (comp_ret)
> > > @@ -1007,6 +1031,14 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> > >         acomp_ctx = acomp_ctx_get_cpu_lock(entry->pool);
> > >         obj = zpool_obj_read_begin(zpool, entry->handle, acomp_ctx->buffer);
> > >
> > > +       /* zswap entries of length PAGE_SIZE are not compressed. */
> > > +       if (entry->length = PAGE_SIZE) {
> > > +               memcpy_to_folio(folio, 0, obj, entry->length);
> >
> > The following read_end() followed by acomp unlock() duplicates the
> > normal decompress ending sequence. It will create complications when
> > we modify the normal ending sequence in the future, we need to match
> > it here.How about just goto the ending sequence and share the same
> > return path as normal:
> >
> >  +                  goto read_done;
> >
> > Then insert the read_done label at ending sequence:
> >
> >         dlen = acomp_ctx->req->dlen;
> >
> > + read_done:
> >         zpool_obj_read_end(zpool, entry->handle, obj);
> >         acomp_ctx_put_unlock(acomp_ctx);
>
> I agree your concern and this looks good to me :)
>
> >
> > If you adopt that, you also will need to init the comp_ret to "0"
> > instead of no init value in the beginning of the function:
> >
> >         struct crypto_acomp_ctx *acomp_ctx;
> > -        int decomp_ret, dlen;
> > +       int decomp_ret = 0, dlen;
> >         u8 *src, *obj;
>
> We may also need to initialize 'dlen' as PAGE_SIZE ?

If there is a code path can lead to dlen use not initialized value? If
not then we don't have to assign it.

> >
> >
> > > +               zpool_obj_read_end(zpool, entry->handle, obj);
> > > +               acomp_ctx_put_unlock(acomp_ctx);
> > > +               return true;
> >
> > Delete the above 3 lines inside uncompress if case.
> >
> > Looks good otherwise.
> >
> > Thanks for the good work.
>
> Thank you for your kind review and nice suggestions!  Since the change is
> simple, I will post a fixup patch as reply to this, for adopting your
> suggestions with my additional changes (adding dst_need_unmap bool variable on
> zswap_compress(), and initializing dlen on zswap_decompress()) if you have no
> objection or different suggestions for the my addition of the changes.  Please
> let me know if you have any concern or other suggestions for my suggested
> additional changes.

I am fine with a fix up patch or a new version. Does not matter to me
in the slightest. I care more about the final landing code itself more
than which vehicle to carry the code.  Assume you do all those fix up
you mention above, you can have my Ack in your fix up:

Acked-by: Chris Li <chrisl@...nel.org>

Chris

Powered by blists - more mailing lists