Re: [PATCHv6 3/6] zram: factor out single stream compression

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

Hello Minchan,

thanks for your review.

On (02/24/14 11:31), Minchan Kim wrote:
> Hello Sergey,
> 
> On Fri, Feb 21, 2014 at 02:50:40PM +0300, Sergey Senozhatsky wrote:
> > This is preparation patch to add multi stream support to zcomp.
> > 
> > Introduce struct zcomp_strm_single and a set of functions to manage zcomp_strm
> > stream access. zcomp_strm_single implements single compession stream, same way
> > as current zcomp implementation. This moves zcomp_strm stream control and
> > locking from zcomp, so compressing backend zcomp is not aware of required
> > locking (single and multi streams require different locking schemes).
> > 
> > The following set of functions added:
> > - zcomp_strm_single_get()/zcomp_strm_single_put()
> >   get and put compression stream, implement required locking
> > - zcomp_strm_single_create()/zcomp_strm_single_destroy()
> >   create and destroy zcomp_strm_single
> > 
> > New ->strm_get() and ->strm_put() callbacks added to zcomp, which are set to
> > zcomp_strm_single_get() and zcomp_strm_single_put() during initialisation.
> > Instead of direct locking and zcomp_strm access from zcomp_strm_get() and
> > zcomp_strm_put(), zcomp now calls ->strm_get() and ->strm_put()
> > correspondingly.
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> It's actually not what I expect.
> What I want was to separate implementation to different files
> whether it enalbles CONFIG_ZRAM_ZCOMP_MULTI or not so that
> popular users who want to use zram as only swap with small
> memory system have little side effect about performance and
> code size.

am I right to guess that you multi stream implementation replaces single
stream. in other words, CONFIG_ZRAM_ZCOMP_MULTI turns zcomp into just a
multi stream backend?

the reasoning behind this indirection is that it allows us to have
CONFIG_ZRAM_ZCOMP_MULTI as additional functionality. if user selects
CONFIG_ZRAM_ZCOMP_MULTI then there is a possibility for user to have both
single (e.g. if he uses zram as a swap device) and multi implemetation
(e.g. if he also uses it as a compressed block device with fs) on his
system. in other words, user may create N zram devices: one swap device
(with single stream inplementation) and N-1 multi stream.

so CONFIG_ZRAM_ZCOMP_MULTI is additional functionality, not the replacing
one. otherwise, there is a small foot print (IMHO. just several function
pointers, other than that it's just a single stream mutex-based implementation).
sounds sane?

> 
> > ---
> >  drivers/block/zram/zcomp.c | 63 ++++++++++++++++++++++++++++++++++++++++------
> >  drivers/block/zram/zcomp.h |  7 ++++--
> >  2 files changed, 60 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> > index db72f3d..9661226 100644
> > --- a/drivers/block/zram/zcomp.c
> > +++ b/drivers/block/zram/zcomp.c
> > @@ -15,6 +15,14 @@
> >  
> >  #include "zcomp.h"
> >  
> > +/*
> > + * single zcomp_strm backend private part
> > + */
> > +struct zcomp_strm_single {
> > +	struct mutex strm_lock;
> > +	struct zcomp_strm *zstrm;
> > +};
> > +
> >  extern struct zcomp_backend zcomp_lzo;
> >  
> >  static struct zcomp_backend *find_backend(const char *compress)
> > @@ -55,17 +63,58 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
> >  	return zstrm;
> >  }
> >  
> > +static struct zcomp_strm *zcomp_strm_single_get(struct zcomp *comp)
> > +{
> > +	struct zcomp_strm_single *zp = comp->private;
> > +	mutex_lock(&zp->strm_lock);
> > +	return zp->zstrm;
> > +}
> > +
> > +static void zcomp_strm_single_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> > +{
> > +	struct zcomp_strm_single *zp = comp->private;
> > +	mutex_unlock(&zp->strm_lock);
> > +}
> > +
> > +static void zcomp_strm_single_destroy(struct zcomp *comp)
> > +{
> > +	struct zcomp_strm_single *zp = comp->private;
> > +	zcomp_strm_free(comp, zp->zstrm);
> > +	kfree(zp);
> > +}
> > +
> > +static int zcomp_strm_single_create(struct zcomp *comp)
> > +{
> > +	struct zcomp_strm_single *zp;
> > +
> > +	comp->destroy = zcomp_strm_single_destroy;
> > +	comp->strm_get = zcomp_strm_single_get;
> > +	comp->strm_put = zcomp_strm_single_put;
> > +	zp = kmalloc(sizeof(struct zcomp_strm_single), GFP_KERNEL);
> > +	comp->private = zp;
> > +	if (!zp)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&zp->strm_lock);
> > +	zp->zstrm = zcomp_strm_alloc(comp);
> > +	if (!zp->zstrm) {
> > +		zcomp_strm_single_destroy(comp);
> > +		return -ENOMEM;
> > +	}
> > +	return 0;
> > +}
> > +
> >  struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
> >  {
> > -	mutex_lock(&comp->strm_lock);
> > -	return comp->zstrm;
> > +	return comp->strm_get(comp);
> >  }
> >  
> >  void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> >  {
> > -	mutex_unlock(&comp->strm_lock);
> > +	comp->strm_put(comp, zstrm);
> >  }
> >  
> > +/* compress page */
> >  int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> >  		const unsigned char *src, size_t *dst_len)
> >  {
> > @@ -73,6 +122,7 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> >  			zstrm->private);
> >  }
> >  
> > +/* decompress page */
> >  int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> >  		size_t src_len, unsigned char *dst)
> >  {
> > @@ -81,7 +131,7 @@ int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> >  
> >  void zcomp_destroy(struct zcomp *comp)
> >  {
> > -	zcomp_strm_free(comp, comp->zstrm);
> > +	comp->destroy(comp);
> >  	kfree(comp);
> >  }
> >  
> > @@ -105,10 +155,7 @@ struct zcomp *zcomp_create(const char *compress)
> >  		return NULL;
> >  
> >  	comp->backend = backend;
> > -	mutex_init(&comp->strm_lock);
> > -
> > -	comp->zstrm = zcomp_strm_alloc(comp);
> > -	if (!comp->zstrm) {
> > +	if (zcomp_strm_single_create(comp) != 0) {
> >  		zcomp_destroy(comp);
> >  		return NULL;
> >  	}
> > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > index 5106f8e..8dc1d7f 100644
> > --- a/drivers/block/zram/zcomp.h
> > +++ b/drivers/block/zram/zcomp.h
> > @@ -34,9 +34,12 @@ struct zcomp_backend {
> >  
> >  /* dynamic per-device compression frontend */
> >  struct zcomp {
> > -	struct mutex strm_lock;
> > -	struct zcomp_strm *zstrm;
> > +	void *private;
> >  	struct zcomp_backend *backend;
> > +
> > +	struct zcomp_strm *(*strm_get)(struct zcomp *comp);
> > +	void (*strm_put)(struct zcomp *comp, struct zcomp_strm *zstrm);
> > +	void (*destroy)(struct zcomp *comp);
> 
> I don't think we need indirection for get/put/destroy.
> zram_drv.c just calls zcomp_strm_get and zcomp.c could implement it
> 
> zcomp_strm_get()
> {
>         mutex_lock
>         return strm;
> }
> 
> and zcomp_multi.c can do it
> 
> zcomp_strm_get()
> {
>         spin_lock
>         spin_unlock
>         wait_event
>         return strm;
> }

so we have only one option -- it either only single stream based zram or
only multi stream based zram. I can move in this direction.

my implemtation allowed two options:

	-- single stream zram
or
	-- (CONFIG_ZRAM_ZCOMP_MULTI selected) single stream and multi stream,
	depending of user set max_comp_streams.

> It seems that you live in my opposite country(ie, you start to dump patches
> when I am about leaving office so ping-pong gap of patch is at least
> one day round. It makes us collaboration very hard so eaieist method I can
> think is just I can implement my thought by myself but I don't want it.
> You thought this idea firstly and I want that you have all credit although
> it waste our time)
> 
> If I made you annoying, I'm really sorry to you.
> Again, thanks for looking at this, Sergey!
> 

I really appreciate and value all your input and review. Thank you. And
sorry if it consumes a lot of your time.

	-ss

> -- 
> Kind regards,
> Minchan Kim
>