Re: zswap: use PAGE_SIZE * 2 for compression dst buffer size when calling crypto compression API

["Um, Taeil" <taeilum@amazon.com>]

   > do you have a specific example of how this causes any actual problem?
Yes, I have a H/W accelerator that tries to finish compression even if compressed data size is greater than source data size.

   > personally, i'd prefer reducing zswap_dstmem down to 1 page to save
   > memory, since there is no case where zswap would ever want to use a
   > compressed page larger than that.
I don't think "reducing zswap_dstmem down to 1 page" works in today's lzo and lz4 kernel implementation.
If I read kernel's lzo, lz4 code correctly, it does not stop compression when compressed data size is greater than source data size. 

On 9/19/18, 8:47 AM, "Dan Streetman" <ddstreet@ieee.org> wrote:

    On Tue, Sep 18, 2018 at 7:48 PM Um, Taeil <taeilum@amazon.com> wrote:
    >
    > We can tell whether compressed size is greater than PAGE_SIZE by looking at the returned *dlen value from crypto_comp_compress. This should be fairly easy.
    > This is actually what zram is doing today. zram looks for *dlen and not take the compressed result if *dlen is greater than certain size.
    > I think errors from crypto_comp_compress should be real errors.
    >
    > Today in kernel compression drivers such as lzo and lz4, they do not stop just because compression result size is greater than source size.
    > Also, some H/W accelerators would not have the option of stopping compression just because of the result size is greater than source size.
    
    do you have a specific example of how this causes any actual problem?
    
    personally, i'd prefer reducing zswap_dstmem down to 1 page to save
    memory, since there is no case where zswap would ever want to use a
    compressed page larger than that.
    
    >
    > Thank you,
    > Taeil
    >
    > On 9/18/18, 2:44 PM, "Dan Streetman" <ddstreet@ieee.org> wrote:
    >
    >     On Tue, Sep 18, 2018 at 2:52 PM Um, Taeil <taeilum@amazon.com> wrote:
    >     >
    >     > Problem statement:
    >     > "compressed data are not fully copied to destination buffer when compressed data size is greater than source data"
    >     >
    >     > Why:
    >     > 5th argument of crypto_comp_compress function is *dlen, which tell the compression driver how many bytes the destination buffer space is allocated (allowed to write data).
    >     > This *dlen is important especially for H/W accelerator based compression driver because it is dangerous if we allow the H/W accelerator to access memory beyond *dst + *dlen.
    >     > Note that buffer location would be passed as physical address.
    >     > Due to the above reason, H/W accelerator based compression driver need to honor *dlen value when it serves crypto_comp_compress API.
    >
    >     and that's exactly what zswap wants to happen - any compressor (hw or
    >     sw) should fail with an error code (ENOSPC makes the most sense, but
    >     zswap doesn't actually care) if the compressed data size is larger
    >     than the provided data buffer.
    >
    >     > Today, we pass slen = PAGE_SIZE and *dlen=PAGE_SIZE to crypto_comp_compress in zswap.c.
    >     > If compressed data size is greater than source (uncompressed) data size,  H/W accelerator cannot copy (deliver) the entire compressed data.
    >
    >     If the "compressed" data is larger than 1 page, then there is no point
    >     in storing the page in zswap.
    >
    >     remember that zswap is different than zram; in zram, there's no other
    >     place to store the data.  However, with zswap, if compression fails or
    >     isn't good, we can just pass the uncompressed page down to the swap
    >     device.
    >
    >     >
    >     > Thank you,
    >     > Taeil
    >     >
    >     > On 9/18/18, 7:15 AM, "Dan Streetman" <ddstreet@ieee.org> wrote:
    >     >
    >     >     On Mon, Sep 17, 2018 at 7:10 PM Um, Taeil <taeilum@amazon.com> wrote:
    >     >     >
    >     >     > Currently, we allocate PAGE_SIZE * 2 for zswap_dstmem which is used as compression destination buffer.
    >     >     >
    >     >     > However, we pass only half of the size (PAGE_SIZE) to crypto_comp_compress.
    >     >     >
    >     >     > This might not be a problem for CPU based existing lzo, lz4 crypto compression driver implantation.
    >     >     >
    >     >     > However, this could be a problem for some H/W acceleration compression drivers, which honor destination buffer size when it prepares H/W resources.
    >     >
    >     >     How exactly could it be a problem?
    >     >
    >     >     >
    >     >     > Actually, this patch is aligned with what zram is passing when it calls crypto_comp_compress.
    >     >     >
    >     >     > The following simple patch will solve this problem. I tested it with existing crypto/lzo.c and crypto/lz4.c compression driver and it works fine.
    >     >     >
    >     >     >
    >     >     >
    >     >     >
    >     >     >
    >     >     > --- mm/zswap.c.orig       2018-09-14 14:36:37.984199232 -0700
    >     >     >
    >     >     > +++ mm/zswap.c             2018-09-14 14:36:53.340189681 -0700
    >     >     >
    >     >     > @@ -1001,7 +1001,7 @@ static int zswap_frontswap_store(unsigne
    >     >     >
    >     >     >                 struct zswap_entry *entry, *dupentry;
    >     >     >
    >     >     >                 struct crypto_comp *tfm;
    >     >     >
    >     >     >                 int ret;
    >     >     >
    >     >     > -              unsigned int hlen, dlen = PAGE_SIZE;
    >     >     >
    >     >     > +             unsigned int hlen, dlen = PAGE_SIZE * 2;
    >     >     >
    >     >     >                 unsigned long handle, value;
    >     >     >
    >     >     >                 char *buf;
    >     >     >
    >     >     >                 u8 *src, *dst;
    >     >     >
    >     >     >
    >     >     >
    >     >     >
    >     >     >
    >     >     >
    >     >     >
    >     >     > Thank you,
    >     >     >
    >     >     > Taeil
    >     >     >
    >     >     >
    >     >
    >     >
    >     >
    >
    >
    >