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

[Minchan Kim <minchan@kernel.org>]

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.

> ---
>  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;
}

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!

-- 
Kind regards,
Minchan Kim