Re: [PATCH v2 2/3] mm/zswap: remove the memcpy if acomp is not sleepable

[Barry Song]

On Sat, Feb 17, 2024 at 8:36 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
> On Fri, Feb 16, 2024 at 11:10:04PM +1300, Barry Song wrote:
> > On Fri, Feb 16, 2024 at 9:30 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Feb 16, 2024 at 05:08:14PM +1300, Barry Song wrote:
> > > > From: Barry Song <v-songbaohua@xxxxxxxx>
> > > >
> > > > Most compressors are actually CPU-based and won't sleep during
> > > > compression and decompression. We should remove the redundant
> > > > memcpy for them.
> > > >
> > > > Signed-off-by: Barry Song <v-songbaohua@xxxxxxxx>
> > > > Tested-by: Chengming Zhou <zhouchengming@xxxxxxxxxxxxx>
> > > > Reviewed-by: Nhat Pham <nphamcs@xxxxxxxxx>
> > > > ---
> > > >  mm/zswap.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > > index 350dd2fc8159..6319d2281020 100644
> > > > --- a/mm/zswap.c
> > > > +++ b/mm/zswap.c
> > > > @@ -168,6 +168,7 @@ struct crypto_acomp_ctx {
> > > >       struct crypto_wait wait;
> > > >       u8 *buffer;
> > > >       struct mutex mutex;
> > > > +     bool is_sleepable;
> > > >  };
> > > >
> > > >  /*
> > > > @@ -716,6 +717,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > > >               goto acomp_fail;
> > > >       }
> > > >       acomp_ctx->acomp = acomp;
> > > > +     acomp_ctx->is_sleepable = acomp_is_sleepable(acomp);
> > >
> > > Just one question here. In patch 1, sleepable seems to mean "not async".
> > > IIUC, even a synchronous algorithm may sleep (e.g. if there is a
> > > cond_resched or waiting for a mutex). Does sleepable in acomp terms the
> > > same as "atomic" in scheduling/preemption terms?
> >
> > I think the answer is yes though async and sleepable are slightly
> > different semantically
> > generally speaking. but for comp cases, they are equal.
> >
> > We have two backends for compression/ decompression - scomp and acomp. if comp
> > is using scomp backend, we can safely think they are not sleepable at
> > least from the
> > below three facts.
> >
> > 1. in zRAM, we are using scomp APIs only - crypto_comp_decompress()/
> > crypto_comp_compress(),  which are definitely scomp, we have never considered
> > sleeping problem in zram drivers:
> > static int zram_read_from_zspool(struct zram *zram, struct page *page,
> >                                  u32 index)
> > {
> >         struct zcomp_strm *zstrm;
> >         unsigned long handle;
> >         unsigned int size;
> >         void *src, *dst;
> >         u32 prio;
> >         int ret;
> >
> >         handle = zram_get_handle(zram, index);
> >         ...
> >         src = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
> >         if (size == PAGE_SIZE) {
> >                 dst = kmap_local_page(page);
> >                 memcpy(dst, src, PAGE_SIZE);
> >                 kunmap_local(dst);
> >                 ret = 0;
> >         } else {
> >                 dst = kmap_local_page(page);
> >                 ret = zcomp_decompress(zstrm, src, size, dst);
> >                 kunmap_local(dst);
> >                 zcomp_stream_put(zram->comps[prio]);
> >         }
> >         zs_unmap_object(zram->mem_pool, handle);
> >         return ret;
> > }
> >
> > 2. zswap used to only support scomp before we moved to use
> > crypto_acomp_compress()
> > and crypto_acomp_decompress() APIs whose backends can be either scomp
> > or acomp, thus new hardware-based compression drivers can be used in zswap.
> >
> > But before we moved to these new APIs in commit  1ec3b5fe6eec782 ("mm/zswap:
> > move to use crypto_acomp API for hardware acceleration") , zswap had
> > never considered
> > sleeping problems just like zRAM.
> >
> > 3. There is no sleeping in drivers using scomp backend.
> >
> > $ git grep crypto_register_scomp
> > crypto/842.c:   ret = crypto_register_scomp(&scomp);
> > crypto/deflate.c:       ret = crypto_register_scomp(&scomp);
> > crypto/lz4.c:   ret = crypto_register_scomp(&scomp);
> > crypto/lz4hc.c: ret = crypto_register_scomp(&scomp);
> > crypto/lzo-rle.c:       ret = crypto_register_scomp(&scomp);
> > crypto/lzo.c:   ret = crypto_register_scomp(&scomp);
> > crypto/zstd.c:  ret = crypto_register_scomp(&scomp);
> > drivers/crypto/cavium/zip/zip_main.c:   ret =
> > crypto_register_scomp(&zip_scomp_deflate);
> > drivers/crypto/cavium/zip/zip_main.c:   ret =
> > crypto_register_scomp(&zip_scomp_lzs);
> >
> > which are the most common cases.
>
> Thanks for explaining. Ideally we should be able to catch any violations
> with proper debug options as you mentioned. Please include more info the
> commit message about sleepability, a summarized version of what you
> described above.

ok. I will enhance the commit message of patch 1/3 with the summary.

Thanks
Barry