Re: [RFC PATCH 2/3] zram: introduce zram compressor operations struct

[Sergey Senozhatsky <sergey.senozhatsky@gmail.com>]

On (01/21/14 09:09), Minchan Kim wrote:
> On Mon, Jan 20, 2014 at 01:03:48PM +0300, Sergey Senozhatsky wrote:
> > On (01/20/14 14:12), Minchan Kim wrote:
> > > Hello Sergey,
> > > 
> > > I reviewed this patchset and I suggest somethings.
> > > Please have a look and feedback to me. :)
> > > 
> > > 1. Let's define new file zram_comp.c
> > > 2. zram_comp includes following field
> > >    .create
> > >    .compress
> > >    .decompress.
> > >    .destroy
> > >    .name
> > > 
> > 
> > alternatively, we can use crypto api, the same way as zswap does (that
> > will require handling of cpu hotplug).
> > 
> > 	-ss
> 
> I really doubt what's the benefit from crypto API for zram.
> It's maybe since I'm not familiar with it so I should ask a silly
> question.
> 
> 1. What's the runtime overhead for using such frontend?
> 
>    As you know, zram is in-memory block device so I don't want to
>    add unnecessary overhead to optimize.
> 
> 2. What's the memory footprint for using such frontend?
> 
>    As you know, zram is very popular for small-memory embedded device
>    so I don't want to consume more runtime memory and static memory
>    due to CONFIG_CRYPTO friend.
> 

schematically:

char *compressor = "lzo"; /* or supplied by user via COMPRESSOR_store() */

if (crypto_has_comp(compressor, 0, 0)) {
	tfms = alloc_percpu(struct crypto_comp *);
	tfm = crypto_alloc_comp(compressor, 0, 0);
	*per_cpu_ptr(tfms, cpu) = tfm;
}

	ret = crypto_comp_compress(tfm, src, slen, dst, dlen);
and
	ret = crypto_comp_decompress(tfm, src, slen, dst, dlen);


memory overhead looks like a bunch of structures (transformation
contexts -- tfm). they touch tfm within compress/decompress, so crypto
users usually allocate per cpu tfm or somehow protect parallel tfm
usage, which is a big disadvantage to my opinion. otoh, crypto provides
HC compressors (LZ4HC at the moment).

my choice is to use raw compressor (as I did in initial patchset).

	-ss

> 3. Is it a flexible to alloc/handle multiple compressor buffer for
>    the our purpose? zswap and zcache have been used it with per-cpu
>    buffer but it would a problem for write scalabitliy if we uses zlib
>    which takes long time to compress.
>    When I read code, maybe we can allocate multiple buffers through
>    cryptop_alloc_compo several time but it would cause 1) and 2) problem
>    again.
>    
> So, what's the attractive point for using crypto?
> One of thing I could imagine is that it could make zram H/W compressor
> but I don't have heard about it so if we don't have any special reason,
> I'd like to go with raw compressor so we can get a *base* number. Then,
> if we really need crypto API, we can change it easily and benchmark.
> Finally, we could get a comparision number in future and it would make
> the decision easily.
> 
> Thanks.
> 
> -- 
> Kind regards,
> Minchan Kim
>