Re: [PATCH v5 02/12] crypto: acomp - Define new interfaces for compress/decompress batching.

[Barry Song]

On Fri, Feb 21, 2025 at 6:32 AM Yosry Ahmed <yosry.ahmed@xxxxxxxxx> wrote:
>
> On Sun, Feb 16, 2025 at 01:17:59PM +0800, Herbert Xu wrote:
> > On Mon, Jan 06, 2025 at 07:10:53PM -0800, Yosry Ahmed wrote:
> > >
> > > The main problem is memory usage. Zswap needs a PAGE_SIZE*2-sized
> > > buffer for each request on each CPU. We preallocate these buffers to
> > > avoid trying to allocate this much memory in the reclaim path (i.e.
> > > potentially allocating two pages to reclaim one).
> >
> > Actually this PAGE_SIZE * 2 thing baffles me.  Why would you
> > allocate more memory than the input? The comment says that it's
> > because certain hardware accelerators will disregard the output
> > buffer length, but surely that's just a bug in the driver?
> >
> > Which driver does this? We should fix it or remove it if it's
> > writing output with no regard to the maximum length.
> >
> > You should only ever need PAGE_SIZE for the output buffer, if
> > the output exceeds that then just fail the compression.
>
> I agree this should be fixed if it can be. This was discussed before
> here:
> https://lore.kernel.org/lkml/CAGsJ_4wuTZcGurby9h4PU2DwFaiEKB4bxuycaeyz3bPw3jSX3A@xxxxxxxxxxxxxx/
>
> Barry is the one who brought up why we need PAGE_SIZE*2. Barry, could
> you please chime in here?

I'm not sure if any real hardware driver fails to return -ERRNO, but there could
be another reason why zRAM doesn't want -ERRNO from the previous code
comment:
"When we receive -ERRNO from the compression backend, there's nothing more
we can do":

int zcomp_compress(struct zcomp_strm *zstrm,
                const void *src, unsigned int *dst_len)
{
        /*
         * Our dst memory (zstrm->buffer) is always `2 * PAGE_SIZE' sized
         * because sometimes we can endup having a bigger compressed data
         * due to various reasons: for example compression algorithms tend
         * to add some padding to the compressed buffer. Speaking of padding,
         * comp algorithm `842' pads the compressed length to multiple of 8
         * and returns -ENOSP when the dst memory is not big enough, which
         * is not something that ZRAM wants to see. We can handle the
         * `compressed_size > PAGE_SIZE' case easily in ZRAM, but when we
         * receive -ERRNO from the compressing backend we can't help it
         * anymore. To make `842' happy we need to tell the exact size of
         * the dst buffer, zram_drv will take care of the fact that
         * compressed buffer is too big.
         */
        *dst_len = PAGE_SIZE * 2;

        return crypto_comp_compress(zstrm->tfm,
                        src, PAGE_SIZE,
                        zstrm->buffer, dst_len);
}

After reviewing the zRAM code, I don't see why zram_write_page() needs
to rely on
comp_len to call write_incompressible_page().

zram_write_page()
{
        ret = zcomp_compress(zram->comps[ZRAM_PRIMARY_COMP], zstrm,
                             mem, &comp_len);
        kunmap_local(mem);

        if (unlikely(ret)) {
                zcomp_stream_put(zstrm);
                pr_err("Compression failed! err=%d\n", ret);
                return ret;
        }

        if (comp_len >= huge_class_size) {
                zcomp_stream_put(zstrm);
                return write_incompressible_page(zram, page, index);
        }
}

I mean, why can't we make it as the below:

zram_write_page()
{
        ret = zcomp_compress(zram->comps[ZRAM_PRIMARY_COMP], zstrm,
                             mem, &comp_len);
        kunmap_local(mem);

        if (unlikely(ret && ret != -ENOSP)) {
                zcomp_stream_put(zstrm);
                pr_err("Compression failed! err=%d\n", ret);
                return ret;
        }

        if (comp_len >= huge_class_size || ret) {
                zcomp_stream_put(zstrm);
                return write_incompressible_page(zram, page, index);
        }
}

As long as crypto drivers consistently return -ENOSP or a specific error
code for dst_buf overflow, we should be able to eliminate the
2*PAGE_SIZE buffer.

My point is:
1. All drivers must be capable of handling dst_buf overflow.
2. All drivers must return a consistent and dedicated error code for
dst_buf overflow.

+Minchan, Sergey,
Do you think we can implement this change in zRAM by using PAGE_SIZE instead
of 2 * PAGE_SIZE?

>
> >
> > Cheers,
> > --
> > Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> > Home Page: http://gondor.apana.org.au/~herbert/
> > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> >

Thanks
Barry