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
