RE: [PATCH v4] mm/zswap: move to use crypto_acomp API for hardware acceleration

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: owner-linux-mm@xxxxxxxxx [mailto:owner-linux-mm@xxxxxxxxx] On
> Behalf Of Sebastian Andrzej Siewior
> Sent: Thursday, July 9, 2020 8:41 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>
> Cc: akpm@xxxxxxxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx; linux-crypto@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; Luis Claudio
> R . Goncalves <lgoncalv@xxxxxxxxxx>; Mahipal Challa
> <mahipalreddy2006@xxxxxxxxx>; Seth Jennings <sjenning@xxxxxxxxxx>;
> Dan Streetman <ddstreet@xxxxxxxx>; Vitaly Wool
> <vitaly.wool@xxxxxxxxxxxx>; Wangzhou (B) <wangzhou1@xxxxxxxxxxxxx>;
> Colin Ian King <colin.king@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v4] mm/zswap: move to use crypto_acomp API for
> hardware acceleration
> 
> On 2020-07-09 07:55:22 [+0000], Song Bao Hua (Barry Song) wrote:
> > Hello Sebastian, thanks for your reply and careful review.
> Hi,
> 
> > I don't think we can simply "forward the result to the caller and let him
> decide".
> > Would you like to present some pseudo code?
> 
> I provided just some pseudo code to illustrate an example how the async
> interface should look like (more or less). The essential part is where
> you allow to feed multiple requests without blocking.

Sebastian, Do you mean the below code?

@@ -252,12 +252,15 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
                unlock_page(page);
                goto out;
        }
-       if (frontswap_store(page) == 0) {
+       ret = frontswap_store(page);
+       if (ret == 0) {
                set_page_writeback(page);
                unlock_page(page);
                end_page_writeback(page);
                goto out;
        }
+       if (ret = -EINPROGRESS)
+               goto out;
        ret = __swap_writepage(page, wbc, end_swap_bio_write);
 out:
        return ret;

I think this won' work. -EINPROGRESS won't be able to decide if we should goto out. We can only goto out if the compression
has done without any error. The error might be because of HW like EIO or because the data is not suitable to compress. We can
only know the result after the compression is really done and the completion callback is called by ZIP drivers.

If the compression is still INPROGRESS, we don't know what will happen.

> I went up the call-chain and found one potential user which seem to have
> a list of pages which are processed. This looked like a nice example. I
> haven't looked at the details.
> 
> I have no opinion whether or not it makes sense to switch to the async
> interface in a sync way.

I always appreciate your comment and your opinion.

The real problem here is that all of those new zip drivers are adapted to async interface. There is no old interface support
for those new drivers mainlined these years. zswap doesnât work on those new drivers as they totally don't support
crypto_comp_compress()
crypto_comp_decompress()
...

So the initial goal of this patch is fixing the disconnected bridge between new zip drivers and zswap.

Making frontswap async can probably happen if we see performance improvement. But it seems it is a big project, not
that simple. On the other hand, it seems hisi_zip in drivers/crypto is the only async driver till now. Sorry if I am missing
any one. other drivers are adapted to acomp APIs by scomp APIs. For example:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/crypto/lz4.c?id=8cd9330e0a
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/crypto/lzo.c?id=ac9d2c4b3

So even we make frontswap totally async, most zip drivers are still sync and we don't get the benefit. From my prospective,
I am glad to try the possibility of making frontswap async to leverage the power of ZIP hardware. This would probably and
only happen after we have a base to support acomp APIs.

Thanks
Barry