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.senozhat...@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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/