Messages in this thread | | | Date | Wed, 26 Feb 2014 14:32:41 +0300 | From | Sergey Senozhatsky <> | Subject | Re: [PATCHv7 3/7] zram: factor out single stream compression |
| |
Hello Minchan,
On (02/26/14 13:34), Minchan Kim wrote: > Hello Sergey, > > On Tue, Feb 25, 2014 at 02:34:29PM +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). > > Please, add why we need different locking scheme in here so that > some people understand why we need this via git log in future. >
addressed all of your notes, nitpicks and concerns in v8. thanks.
-ss
> > > > 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> > > --- > > drivers/block/zram/zcomp.c | 65 +++++++++++++++++++++++++++++++++++++++------- > > drivers/block/zram/zcomp.h | 7 +++-- > > 2 files changed, 61 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c > > index 947efe3..e20054b 100644 > > --- a/drivers/block/zram/zcomp.c > > +++ b/drivers/block/zram/zcomp.c > > @@ -15,6 +15,14 @@ > > > > #include "zcomp.h" > > > > +/* > > + * single zcomp_strm backend > > + */ > > +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 *zs = comp->stream; > > + mutex_lock(&zs->strm_lock); > > + return zs->zstrm; > > +} > > + > > +static void zcomp_strm_single_put(struct zcomp *comp, struct zcomp_strm *zstrm) > > +{ > > + struct zcomp_strm_single *zs = comp->stream; > > + mutex_unlock(&zs->strm_lock); > > +} > > + > > +static void zcomp_strm_single_destroy(struct zcomp *comp) > > +{ > > + struct zcomp_strm_single *zs = comp->stream; > > + zcomp_strm_free(comp, zs->zstrm); > > + kfree(zs); > > +} > > + > > +static int zcomp_strm_single_create(struct zcomp *comp) > > +{ > > + struct zcomp_strm_single *zs; > > + > > + comp->destroy = zcomp_strm_single_destroy; > > + comp->strm_get = zcomp_strm_single_get; > > + comp->strm_put = zcomp_strm_single_put; > > + zs = kmalloc(sizeof(struct zcomp_strm_single), GFP_KERNEL); > > + comp->stream = zs; > > + if (!zs) > > + return -ENOMEM; > > Firstly check zs nullness and then assign zs to the comp->stream. > Yeb. your code doesn't have any problem but let's follow normal > convention. > > > + > > + mutex_init(&zs->strm_lock); > > + zs->zstrm = zcomp_strm_alloc(comp); > > + if (!zs->zstrm) { > > + zcomp_strm_single_destroy(comp); > > Let's just call kfree in here instead of xxx_destroy. > such pair function call is more clear to me rather than using wrapping > function. > > > > + 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 */ > > Function name is clear so I think we don't need a comment. > If we need such comment, it should introduce previous patches, not this one. > > > 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,11 +155,8 @@ 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) { > > - kfree(comp); > > + if (zcomp_strm_single_create(comp) != 0) { > > + zcomp_destroy(comp); > > Let's use just kfree. > If zcomp_strm_single_create fails, it should tidy up all memory > allocated by itself so caller should just free thing allocate by > itself. > > > return NULL; > > } > > return comp; > > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h > > index 5106f8e..861e04d 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 *stream; > > 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); > > }; > > > > struct zcomp *zcomp_create(const char *comp); > > -- > > 1.9.0.291.g027825b > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > -- > Kind regards, > Minchan Kim >
| |