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.
> ---
> 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
--
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/
