Received-SPF: pass (google.com: domain of linux-kernel+bounces-68416-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240216040815.114202-1-21cnbao@gmail.com> <20240216040815.114202-3-21cnbao@gmail.com>
 <Zc8dEn7eqFmC_Kcd@google.com>
In-Reply-To: <Zc8dEn7eqFmC_Kcd@google.com>
From: Barry Song <21cnbao@gmail.com>
Date: Fri, 16 Feb 2024 23:10:04 +1300
Message-ID: <CAGsJ_4x6z48N9Sq1V8Bn16eSdRAjBcy3=O_a2iizg=D-tPng=Q@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] mm/zswap: remove the memcpy if acomp is not sleepable
To: Yosry Ahmed <yosryahmed@google.com>
Cc: akpm@linux-foundation.org, davem@davemloft.net, hannes@cmpxchg.org, 
	herbert@gondor.apana.org.au, linux-crypto@vger.kernel.org, linux-mm@kvack.org, 
	nphamcs@gmail.com, zhouchengming@bytedance.com, chriscli@google.com, 
	chrisl@kernel.org, ddstreet@ieee.org, linux-kernel@vger.kernel.org, 
	sjenning@redhat.com, vitaly.wool@konsulko.com, 
	Barry Song <v-songbaohua@oppo.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 16, 2024 at 9:30=E2=80=AFPM Yosry Ahmed <yosryahmed@google.com>=
 wrote:
>
> On Fri, Feb 16, 2024 at 05:08:14PM +1300, Barry Song wrote:
> > From: Barry Song <v-songbaohua@oppo.com>
> >
> > 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@oppo.com>
> > Tested-by: Chengming Zhou <zhouchengming@bytedance.com>
> > Reviewed-by: Nhat Pham <nphamcs@gmail.com>
> > ---
> >  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 =3D acomp;
> > +     acomp_ctx->is_sleepable =3D 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 c=
omp
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 consider=
ed
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 =3D zram_get_handle(zram, index);
        ...
        src =3D zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
        if (size =3D=3D PAGE_SIZE) {
                dst =3D kmap_local_page(page);
                memcpy(dst, src, PAGE_SIZE);
                kunmap_local(dst);
                ret =3D 0;
        } else {
                dst =3D kmap_local_page(page);
                ret =3D 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 =3D crypto_register_scomp(&scomp);
crypto/deflate.c:       ret =3D crypto_register_scomp(&scomp);
crypto/lz4.c:   ret =3D crypto_register_scomp(&scomp);
crypto/lz4hc.c: ret =3D crypto_register_scomp(&scomp);
crypto/lzo-rle.c:       ret =3D crypto_register_scomp(&scomp);
crypto/lzo.c:   ret =3D crypto_register_scomp(&scomp);
crypto/zstd.c:  ret =3D crypto_register_scomp(&scomp);
drivers/crypto/cavium/zip/zip_main.c:   ret =3D
crypto_register_scomp(&zip_scomp_deflate);
drivers/crypto/cavium/zip/zip_main.c:   ret =3D
crypto_register_scomp(&zip_scomp_lzs);

which are the most common cases.

>
> Also, was this tested with debug options to catch any possible sleeps in
> atomic context?

yes. i have enabled CONFIG_DEBUG_ATOMIC_SLEEP=3Dy.

>
> If the answer to both questions is yes, the change otherwise LGTM. Feel
> free to add:
> Acked-by: Yosry Ahmed <yosryahmed@google.com>

Thanks!

>
> Thanks!
>
> >
> >       req =3D acomp_request_alloc(acomp_ctx->acomp);
> >       if (!req) {
> > @@ -1368,7 +1370,7 @@ static void __zswap_load(struct zswap_entry *entr=
y, struct page *page)
> >       mutex_lock(&acomp_ctx->mutex);
> >
> >       src =3D zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
> > -     if (!zpool_can_sleep_mapped(zpool)) {
> > +     if (acomp_ctx->is_sleepable && !zpool_can_sleep_mapped(zpool)) {
> >               memcpy(acomp_ctx->buffer, src, entry->length);
> >               src =3D acomp_ctx->buffer;
> >               zpool_unmap_handle(zpool, entry->handle);
> > @@ -1382,7 +1384,7 @@ static void __zswap_load(struct zswap_entry *entr=
y, struct page *page)
> >       BUG_ON(acomp_ctx->req->dlen !=3D PAGE_SIZE);
> >       mutex_unlock(&acomp_ctx->mutex);
> >
> > -     if (zpool_can_sleep_mapped(zpool))
> > +     if (!acomp_ctx->is_sleepable || zpool_can_sleep_mapped(zpool))
> >               zpool_unmap_handle(zpool, entry->handle);
> >  }
> >
> > --
> > 2.34.1
> >

Thanks
Barry