* [PATCHv3 1/4] zram: introduce compressing backend abstraction
2014-02-11 22:21 [PATCHv3 0/4] add compression backend abstraction Sergey Senozhatsky
@ 2014-02-11 22:21 ` Sergey Senozhatsky
2014-02-12 7:06 ` Minchan Kim
2014-02-12 7:08 ` Minchan Kim
2014-02-11 22:21 ` [PATCHv3 2/4] zram: use zram_comp compressing backends Sergey Senozhatsky
` (2 subsequent siblings)
3 siblings, 2 replies; 9+ messages in thread
From: Sergey Senozhatsky @ 2014-02-11 22:21 UTC (permalink / raw)
To: Minchan Kim
Cc: Jerome Marchand, Nitin Gupta, linux-kernel, Sergey Senozhatsky
ZRAM performs direct LZO compression algorithm calls, making it the one
and only option. Introduce abstract struct zram_comp in order to support
multiple compression algorithms. struct zram_comp defines the following
set of compressing backend operations:
.create
.destroy
.compress
.decompress
Schematically zram write() usually contains the following steps:
0) preparation (decompression of partioal IO, etc.)
1) lock buffer_lock mutex (protects meta compress buffers)
2) compress (using meta compress buffers)
3) alloc and map zs_pool object
4) copy compressed data (from meta compress buffers) to object allocated by 3)
5) free previous pool page, assign a new one
6) unlock buffer_lock mutex
As we can see, compressing buffers must remain untouched from 1) to 4),
because, otherwise, concurrent write() can overwrite data. At the same
time, zram_meta must be aware of a) specific compression algorithm
memory requirements and b) necessary locking to protect compression
buffers. Besides, zram holds buffer_lock almost through the whole write()
function, making parallel compression impossible. To remove requirement
a) new struct zcomp_workmem introduced, which contain buffers required by
compression algorithm. While struct zram_comp implements workmem handling
and locking by means of get() and put() semantics and removes requirement
b) from zram meta. zram_comp ->create() and ->destroy(), respectively,
allocate and deallocate algorithm specific zcomp_workmem `private' buffer.
Every zram_comp has a list of idle workmem buffers (at least 1 workmem),
spinlock to protect idle list and wait queue, making it possible to perform
parallel compressions. Each time zram issues a zcomp_workmem_get() call, the
following set of operations performed:
- spin lock buffer_lock
- if idle list is not empty, remove workmem from idle list, spin
unlock and return workmem pointer
- if idle list is empty, current adds itself to wait queue. it will be
awaken by zcomp_workmem_put() caller.
zcomp_workmem_put():
- spin lock buffer_lock
- add workmem to idle list
- spin unlock, wake up sleeper (if any)
In other words, zcomp_workmem_get() turns caller into exclusive user of workmem
and zcomp_workem_put() makes a particular workmem available.
Usage examples.
To initialize compressing backend:
comp = zcomp_create(NAME) /* NAME e.g. lzo */
which initialises compressing backend if requested algorithm is supported.
Compress:
wm = zcomp_workmem_get(comp)
zcomp_compress(comp, wm, src, src_len, &dst_len)
[..] /* copy compressed data */
zcomp_workmem_put(comp, wm)
Decompress:
zcomp_decompress(comp, src, src_len, dst, &dst_len);
Free compessing backend and its workmem:
zcomp_destroy(comp)
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/block/zram/zcomp_lzo.c | 33 +++++++++
drivers/block/zram/zram_comp.c | 147 +++++++++++++++++++++++++++++++++++++++++
drivers/block/zram/zram_comp.h | 58 ++++++++++++++++
3 files changed, 238 insertions(+)
create mode 100644 drivers/block/zram/zcomp_lzo.c
create mode 100644 drivers/block/zram/zram_comp.c
create mode 100644 drivers/block/zram/zram_comp.h
diff --git a/drivers/block/zram/zcomp_lzo.c b/drivers/block/zram/zcomp_lzo.c
new file mode 100644
index 0000000..bbde74e
--- /dev/null
+++ b/drivers/block/zram/zcomp_lzo.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2014 Sergey Senozhatsky.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/lzo.h>
+
+#include "zram_comp.h"
+
+static void * lzo_create(void)
+{
+ return kmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
+}
+
+static void lzo_destroy(void *workmem)
+{
+ kfree(workmem);
+}
+
+extern struct zram_comp_backend zcomp_lzo;
+struct zram_comp_backend zcomp_lzo = {
+ .compress = lzo1x_1_compress,
+ .decompress = lzo1x_decompress_safe,
+ .create = lzo_create,
+ .destroy = lzo_destroy,
+ .name = "lzo",
+};
diff --git a/drivers/block/zram/zram_comp.c b/drivers/block/zram/zram_comp.c
new file mode 100644
index 0000000..139a468
--- /dev/null
+++ b/drivers/block/zram/zram_comp.c
@@ -0,0 +1,147 @@
+/*
+ * Copyright (C) 2014 Sergey Senozhatsky.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+#include <linux/wait.h>
+#include <linux/sched.h>
+
+#include "zram_comp.h"
+
+extern struct zram_comp_backend zcomp_lzo;
+
+static void workmem_free(struct zram_comp *comp, struct zcomp_workmem *workmem)
+{
+ comp->backend->destroy(workmem->private);
+ free_pages((unsigned long)workmem->buffer, 1);
+ kfree(workmem);
+}
+
+/* allocate new workmem structure with ->mem of requested size,
+ * return NULL on error */
+static struct zcomp_workmem *workmem_alloc(struct zram_comp *comp)
+{
+ struct zcomp_workmem *workmem = kmalloc(sizeof(*workmem), GFP_NOFS);
+ if (!workmem)
+ return NULL;
+
+ INIT_LIST_HEAD(&workmem->list);
+ /* algorithm specific working memory buffer */
+ workmem->private = comp->backend->create();
+ /* allocate 2 pages. 1 for compressed data, plus 1 extra for the
+ * case when compressed size is larger than the original one. */
+ workmem->buffer = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
+ if (!workmem->private || !workmem->buffer)
+ goto fail;
+
+ return workmem;
+fail:
+ workmem_free(comp, workmem);
+ return NULL;
+}
+
+/* get existing idle workmem or wait until other process release
+ * (workmem_put()) one for us */
+struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp)
+{
+ struct zcomp_workmem *wm;
+retry:
+ spin_lock(&comp->buffer_lock);
+ if (list_empty(&comp->idle_workmem)) {
+ spin_unlock(&comp->buffer_lock);
+ wait_event(comp->workmem_wait,
+ !list_empty(&comp->idle_workmem));
+ goto retry;
+ }
+
+ wm = list_entry(comp->idle_workmem.next,
+ struct zcomp_workmem, list);
+ list_del(&wm->list);
+ spin_unlock(&comp->buffer_lock);
+ return wm;
+}
+
+/* add workmem back to idle list and wake up waiter (if any) */
+void zcomp_workmem_put(struct zram_comp *comp,
+ struct zcomp_workmem *workmem)
+{
+ spin_lock(&comp->buffer_lock);
+ list_add_tail(&workmem->list, &comp->idle_workmem);
+ spin_unlock(&comp->buffer_lock);
+
+ if (waitqueue_active(&comp->workmem_wait))
+ wake_up(&comp->workmem_wait);
+}
+
+int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
+ const unsigned char *src, size_t src_len, size_t *dst_len)
+{
+ return comp->backend->compress(src, src_len, workmem->buffer,
+ dst_len, workmem->private);
+}
+
+int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
+ size_t src_len, unsigned char *dst, size_t *dst_len)
+{
+ return comp->backend->decompress(src, src_len, dst, dst_len);
+}
+
+/* free allocated workmem buffers and zram_comp */
+void zcomp_destroy(struct zram_comp *comp)
+{
+ struct zcomp_workmem *wm;
+ while (!list_empty(&comp->idle_workmem)) {
+ wm = list_entry(comp->idle_workmem.next,
+ struct zcomp_workmem, list);
+ list_del(&wm->list);
+ workmem_free(comp, wm);
+ }
+ kfree(comp);
+}
+
+static struct zram_comp_backend *find_backend(const char *compress)
+{
+ if (sysfs_streq(compress, "lzo"))
+ return &zcomp_lzo;
+ return NULL;
+}
+
+/* search available compressors for requested algorithm.
+ * allocate new zram_comp and initialize it. return NULL
+ * if requested algorithm is not supported or in case
+ * of init error */
+struct zram_comp *zcomp_create(const char *compress)
+{
+ struct zram_comp *comp;
+ struct zram_comp_backend *backend;
+ struct zcomp_workmem *wm;
+
+ backend = find_backend(compress);
+ if (!backend)
+ return NULL;
+
+ comp = kzalloc(sizeof(struct zram_comp), GFP_KERNEL);
+ if (!comp)
+ return NULL;
+
+ comp->backend = backend;
+ spin_lock_init(&comp->buffer_lock);
+ INIT_LIST_HEAD(&comp->idle_workmem);
+ init_waitqueue_head(&comp->workmem_wait);
+
+ wm = workmem_alloc(comp);
+ if (!wm) {
+ zcomp_destroy(comp);
+ return NULL;
+ }
+ list_add_tail(&wm->list, &comp->idle_workmem);
+ return comp;
+}
diff --git a/drivers/block/zram/zram_comp.h b/drivers/block/zram/zram_comp.h
new file mode 100644
index 0000000..90355e8
--- /dev/null
+++ b/drivers/block/zram/zram_comp.h
@@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2014 Sergey Senozhatsky.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _ZRAM_COMP_H_
+#define _ZRAM_COMP_H_
+
+#include <linux/types.h>
+#include <linux/spinlock.h>
+
+struct zcomp_workmem {
+ void *buffer; /* compression/decompression buffer */
+ void *private; /* algorithm workmem */
+ struct list_head list;
+};
+
+/* static compression backend */
+struct zram_comp_backend {
+ int (*compress)(const unsigned char *src, size_t src_len,
+ unsigned char *dst, size_t *dst_len, void *wrkmem);
+
+ int (*decompress)(const unsigned char *src, size_t src_len,
+ unsigned char *dst, size_t *dst_len);
+
+ void *(*create)(void);
+ void (*destroy)(void *workmem);
+
+ const char *name;
+};
+
+/* dynamic per-device compression frontend */
+struct zram_comp {
+ /* protect workmem list */
+ spinlock_t buffer_lock;
+ /* list of available workmems */
+ struct list_head idle_workmem;
+ wait_queue_head_t workmem_wait;
+ struct zram_comp_backend *backend;
+};
+
+struct zram_comp *zcomp_create(const char *comp);
+void zcomp_destroy(struct zram_comp *comp);
+
+struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp);
+void zcomp_workmem_put(struct zram_comp *comp,
+ struct zcomp_workmem *workmem);
+
+int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
+ const unsigned char *src, size_t src_len, size_t *dst_len);
+
+int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
+ size_t src_len, unsigned char *dst, size_t *dst_len);
+#endif /* _ZRAM_COMP_H_ */
--
1.9.0.rc3.244.g3497008
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCHv3 1/4] zram: introduce compressing backend abstraction
2014-02-11 22:21 ` [PATCHv3 1/4] zram: introduce compressing " Sergey Senozhatsky
@ 2014-02-12 7:06 ` Minchan Kim
2014-02-12 9:02 ` Sergey Senozhatsky
2014-02-12 7:08 ` Minchan Kim
1 sibling, 1 reply; 9+ messages in thread
From: Minchan Kim @ 2014-02-12 7:06 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
Hello Sergey,
On Wed, Feb 12, 2014 at 01:21:03AM +0300, Sergey Senozhatsky wrote:
> ZRAM performs direct LZO compression algorithm calls, making it the one
> and only option. Introduce abstract struct zram_comp in order to support
> multiple compression algorithms. struct zram_comp defines the following
> set of compressing backend operations:
> .create
> .destroy
> .compress
> .decompress
>
> Schematically zram write() usually contains the following steps:
> 0) preparation (decompression of partioal IO, etc.)
> 1) lock buffer_lock mutex (protects meta compress buffers)
> 2) compress (using meta compress buffers)
> 3) alloc and map zs_pool object
> 4) copy compressed data (from meta compress buffers) to object allocated by 3)
> 5) free previous pool page, assign a new one
> 6) unlock buffer_lock mutex
>
> As we can see, compressing buffers must remain untouched from 1) to 4),
> because, otherwise, concurrent write() can overwrite data. At the same
> time, zram_meta must be aware of a) specific compression algorithm
> memory requirements and b) necessary locking to protect compression
> buffers. Besides, zram holds buffer_lock almost through the whole write()
> function, making parallel compression impossible. To remove requirement
> a) new struct zcomp_workmem introduced, which contain buffers required by
> compression algorithm. While struct zram_comp implements workmem handling
> and locking by means of get() and put() semantics and removes requirement
> b) from zram meta. zram_comp ->create() and ->destroy(), respectively,
> allocate and deallocate algorithm specific zcomp_workmem `private' buffer.
>
> Every zram_comp has a list of idle workmem buffers (at least 1 workmem),
> spinlock to protect idle list and wait queue, making it possible to perform
> parallel compressions. Each time zram issues a zcomp_workmem_get() call, the
> following set of operations performed:
> - spin lock buffer_lock
> - if idle list is not empty, remove workmem from idle list, spin
> unlock and return workmem pointer
> - if idle list is empty, current adds itself to wait queue. it will be
> awaken by zcomp_workmem_put() caller.
>
> zcomp_workmem_put():
> - spin lock buffer_lock
> - add workmem to idle list
> - spin unlock, wake up sleeper (if any)
>
> In other words, zcomp_workmem_get() turns caller into exclusive user of workmem
> and zcomp_workem_put() makes a particular workmem available.
>
> Usage examples.
>
> To initialize compressing backend:
> comp = zcomp_create(NAME) /* NAME e.g. lzo */
>
> which initialises compressing backend if requested algorithm is supported.
>
> Compress:
> wm = zcomp_workmem_get(comp)
> zcomp_compress(comp, wm, src, src_len, &dst_len)
> [..] /* copy compressed data */
> zcomp_workmem_put(comp, wm)
>
> Decompress:
> zcomp_decompress(comp, src, src_len, dst, &dst_len);
>
> Free compessing backend and its workmem:
> zcomp_destroy(comp)
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> drivers/block/zram/zcomp_lzo.c | 33 +++++++++
> drivers/block/zram/zram_comp.c | 147 +++++++++++++++++++++++++++++++++++++++++
> drivers/block/zram/zram_comp.h | 58 ++++++++++++++++
> 3 files changed, 238 insertions(+)
> create mode 100644 drivers/block/zram/zcomp_lzo.c
> create mode 100644 drivers/block/zram/zram_comp.c
> create mode 100644 drivers/block/zram/zram_comp.h
>
> diff --git a/drivers/block/zram/zcomp_lzo.c b/drivers/block/zram/zcomp_lzo.c
> new file mode 100644
> index 0000000..bbde74e
> --- /dev/null
> +++ b/drivers/block/zram/zcomp_lzo.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/lzo.h>
> +
> +#include "zram_comp.h"
> +
> +static void * lzo_create(void)
> +{
> + return kmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> +}
> +
> +static void lzo_destroy(void *workmem)
> +{
> + kfree(workmem);
> +}
It's okay to name "workmem" for lzo because lzo needs only work buffer to work
but let's think about zlib. It needs z_streamp which has many feilds
to work as well as work memory. That's why I don't like "workmem" name which
is very specific. Anyway, it's okay for lzo backend now.
> +
> +extern struct zram_comp_backend zcomp_lzo;
> +struct zram_comp_backend zcomp_lzo = {
> + .compress = lzo1x_1_compress,
> + .decompress = lzo1x_decompress_safe,
> + .create = lzo_create,
> + .destroy = lzo_destroy,
> + .name = "lzo",
> +};
> diff --git a/drivers/block/zram/zram_comp.c b/drivers/block/zram/zram_comp.c
> new file mode 100644
> index 0000000..139a468
> --- /dev/null
> +++ b/drivers/block/zram/zram_comp.c
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/wait.h>
> +#include <linux/sched.h>
> +
> +#include "zram_comp.h"
> +
> +extern struct zram_comp_backend zcomp_lzo;
> +
> +static void workmem_free(struct zram_comp *comp, struct zcomp_workmem *workmem)
> +{
> + comp->backend->destroy(workmem->private);
> + free_pages((unsigned long)workmem->buffer, 1);
> + kfree(workmem);
> +}
> +
> +/* allocate new workmem structure with ->mem of requested size,
> + * return NULL on error */
> +static struct zcomp_workmem *workmem_alloc(struct zram_comp *comp)
> +{
> + struct zcomp_workmem *workmem = kmalloc(sizeof(*workmem), GFP_NOFS);
> + if (!workmem)
> + return NULL;
> +
> + INIT_LIST_HEAD(&workmem->list);
> + /* algorithm specific working memory buffer */
> + workmem->private = comp->backend->create();
> + /* allocate 2 pages. 1 for compressed data, plus 1 extra for the
> + * case when compressed size is larger than the original one. */
> + workmem->buffer = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
> + if (!workmem->private || !workmem->buffer)
> + goto fail;
> +
> + return workmem;
> +fail:
> + workmem_free(comp, workmem);
> + return NULL;
> +}
Hmm, you introduced workmem again. :(
As I said above, workmem is too specific.
It's only valid for lzo and lz4 so I don't want to expose workmem out of zcomp
and that's why I suggested zcomp_strm or zcomp_stream.
It seems "stream" is more general term for compressor.
> +
> +/* get existing idle workmem or wait until other process release
> + * (workmem_put()) one for us */
> +struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp)
> +{
> + struct zcomp_workmem *wm;
> +retry:
> + spin_lock(&comp->buffer_lock);
> + if (list_empty(&comp->idle_workmem)) {
> + spin_unlock(&comp->buffer_lock);
> + wait_event(comp->workmem_wait,
> + !list_empty(&comp->idle_workmem));
> + goto retry;
> + }
> +
> + wm = list_entry(comp->idle_workmem.next,
> + struct zcomp_workmem, list);
> + list_del(&wm->list);
> + spin_unlock(&comp->buffer_lock);
> + return wm;
> +}
> +
> +/* add workmem back to idle list and wake up waiter (if any) */
> +void zcomp_workmem_put(struct zram_comp *comp,
> + struct zcomp_workmem *workmem)
> +{
> + spin_lock(&comp->buffer_lock);
> + list_add_tail(&workmem->list, &comp->idle_workmem);
Why do you implement queue instead of stack?
> + spin_unlock(&comp->buffer_lock);
> +
> + if (waitqueue_active(&comp->workmem_wait))
Possible losting wakeup?
> + wake_up(&comp->workmem_wait);
> +}
> +
> +int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
> + const unsigned char *src, size_t src_len, size_t *dst_len)
> +{
As I said, I don't like zcomp_workmem.
Please use zcomp_strm and let's make naming clear.
Something is zcomp and sometime is zram_comp.
I prefer zcomp but it depends on you.
> + return comp->backend->compress(src, src_len, workmem->buffer,
> + dst_len, workmem->private);
> +}
> +
> +int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
> + size_t src_len, unsigned char *dst, size_t *dst_len)
> +{
> + return comp->backend->decompress(src, src_len, dst, dst_len);
> +}
> +
> +/* free allocated workmem buffers and zram_comp */
> +void zcomp_destroy(struct zram_comp *comp)
> +{
> + struct zcomp_workmem *wm;
> + while (!list_empty(&comp->idle_workmem)) {
> + wm = list_entry(comp->idle_workmem.next,
> + struct zcomp_workmem, list);
> + list_del(&wm->list);
> + workmem_free(comp, wm);
> + }
> + kfree(comp);
> +}
> +
> +static struct zram_comp_backend *find_backend(const char *compress)
> +{
> + if (sysfs_streq(compress, "lzo"))
> + return &zcomp_lzo;
> + return NULL;
> +}
> +
> +/* search available compressors for requested algorithm.
Please correct comment style.
> + * allocate new zram_comp and initialize it. return NULL
> + * if requested algorithm is not supported or in case
> + * of init error */
> +struct zram_comp *zcomp_create(const char *compress)
> +{
> + struct zram_comp *comp;
> + struct zram_comp_backend *backend;
> + struct zcomp_workmem *wm;
> +
> + backend = find_backend(compress);
> + if (!backend)
> + return NULL;
> +
> + comp = kzalloc(sizeof(struct zram_comp), GFP_KERNEL);
> + if (!comp)
> + return NULL;
> +
> + comp->backend = backend;
> + spin_lock_init(&comp->buffer_lock);
> + INIT_LIST_HEAD(&comp->idle_workmem);
> + init_waitqueue_head(&comp->workmem_wait);
> +
> + wm = workmem_alloc(comp);
> + if (!wm) {
> + zcomp_destroy(comp);
> + return NULL;
> + }
> + list_add_tail(&wm->list, &comp->idle_workmem);
> + return comp;
> +}
> diff --git a/drivers/block/zram/zram_comp.h b/drivers/block/zram/zram_comp.h
> new file mode 100644
> index 0000000..90355e8
> --- /dev/null
> +++ b/drivers/block/zram/zram_comp.h
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#ifndef _ZRAM_COMP_H_
> +#define _ZRAM_COMP_H_
> +
> +#include <linux/types.h>
> +#include <linux/spinlock.h>
> +
> +struct zcomp_workmem {
> + void *buffer; /* compression/decompression buffer */
> + void *private; /* algorithm workmem */
> + struct list_head list;
> +};
> +
> +/* static compression backend */
> +struct zram_comp_backend {
> + int (*compress)(const unsigned char *src, size_t src_len,
> + unsigned char *dst, size_t *dst_len, void *wrkmem);
zram works based on PAGE so I like compress_page so maybe we couldn't
reduce a argument "src_len".
> +
> + int (*decompress)(const unsigned char *src, size_t src_len,
> + unsigned char *dst, size_t *dst_len);
> +
> + void *(*create)(void);
> + void (*destroy)(void *workmem);
> +
> + const char *name;
> +};
> +
> +/* dynamic per-device compression frontend */
> +struct zram_comp {
> + /* protect workmem list */
> + spinlock_t buffer_lock;
> + /* list of available workmems */
> + struct list_head idle_workmem;
> + wait_queue_head_t workmem_wait;
> + struct zram_comp_backend *backend;
> +};
> +
> +struct zram_comp *zcomp_create(const char *comp);
> +void zcomp_destroy(struct zram_comp *comp);
> +
> +struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp);
> +void zcomp_workmem_put(struct zram_comp *comp,
> + struct zcomp_workmem *workmem);
> +
> +int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
> + const unsigned char *src, size_t src_len, size_t *dst_len);
> +
> +int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
> + size_t src_len, unsigned char *dst, size_t *dst_len);
> +#endif /* _ZRAM_COMP_H_ */
> --
> 1.9.0.rc3.244.g3497008
>
> --
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCHv3 1/4] zram: introduce compressing backend abstraction
2014-02-12 7:06 ` Minchan Kim
@ 2014-02-12 9:02 ` Sergey Senozhatsky
0 siblings, 0 replies; 9+ messages in thread
From: Sergey Senozhatsky @ 2014-02-12 9:02 UTC (permalink / raw)
To: Minchan Kim; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On (02/12/14 16:06), Minchan Kim wrote:
[..]
> > +{
> > + kfree(workmem);
> > +}
>
> It's okay to name "workmem" for lzo because lzo needs only work buffer to work
> but let's think about zlib. It needs z_streamp which has many feilds
> to work as well as work memory. That's why I don't like "workmem" name which
> is very specific. Anyway, it's okay for lzo backend now.
>
ok. will use zcomp_strm
[..]
> > + return workmem;
> > +fail:
> > + workmem_free(comp, workmem);
> > + return NULL;
> > +}
>
> Hmm, you introduced workmem again. :(
> As I said above, workmem is too specific.
> It's only valid for lzo and lz4 so I don't want to expose workmem out of zcomp
> and that's why I suggested zcomp_strm or zcomp_stream.
> It seems "stream" is more general term for compressor.
>
> > +/* add workmem back to idle list and wake up waiter (if any) */
> > +void zcomp_workmem_put(struct zram_comp *comp,
> > + struct zcomp_workmem *workmem)
> > +{
> > + spin_lock(&comp->buffer_lock);
> > + list_add_tail(&workmem->list, &comp->idle_workmem);
>
> Why do you implement queue instead of stack?
>
zcomp gets the first element from a list and never iterates it,
so it's O(1).
kfifo with copying and write memory barriers... I don't think
it will be a better choice here. and we don't have `first in,
first out' pattern, zcomp does not care about order.
> > + spin_unlock(&comp->buffer_lock);
> > +
> > + if (waitqueue_active(&comp->workmem_wait))
>
> Possible losting wakeup?
hm, could be.
> > + wake_up(&comp->workmem_wait);
> > +}
> > +
> > +int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
> > + const unsigned char *src, size_t src_len, size_t *dst_len)
> > +{
>
> As I said, I don't like zcomp_workmem.
> Please use zcomp_strm and let's make naming clear.
> Something is zcomp and sometime is zram_comp.
> I prefer zcomp but it depends on you.
ok. I drop zram part. zcomp and zcomp_strm
> > + return comp->backend->compress(src, src_len, workmem->buffer,
> > + dst_len, workmem->private);
> > +}
> > +
> > +int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
> > + size_t src_len, unsigned char *dst, size_t *dst_len)
> > +{
> > + return comp->backend->decompress(src, src_len, dst, dst_len);
> > +}
> > +
> > +/* free allocated workmem buffers and zram_comp */
> > +void zcomp_destroy(struct zram_comp *comp)
> > +{
> > + struct zcomp_workmem *wm;
> > + while (!list_empty(&comp->idle_workmem)) {
> > + wm = list_entry(comp->idle_workmem.next,
> > + struct zcomp_workmem, list);
> > + list_del(&wm->list);
> > + workmem_free(comp, wm);
> > + }
> > + kfree(comp);
> > +}
> > +
> > +static struct zram_comp_backend *find_backend(const char *compress)
> > +{
> > + if (sysfs_streq(compress, "lzo"))
> > + return &zcomp_lzo;
> > + return NULL;
> > +}
> > +
> > +/* search available compressors for requested algorithm.
>
> Please correct comment style.
I'll take a look (checkpatch didn't complain).
> > +/* static compression backend */
> > +struct zram_comp_backend {
> > + int (*compress)(const unsigned char *src, size_t src_len,
> > + unsigned char *dst, size_t *dst_len, void *wrkmem);
>
> zram works based on PAGE so I like compress_page so maybe we couldn't
> reduce a argument "src_len".
>
yes and no.
zram can see partial IOs
static inline int is_partial_io(struct bio_vec *bvec)
{
return bvec->bv_len != PAGE_SIZE;
}
so, in general yes -- we can drop src_len and compress PAGE_SIZE bytes,
but at the same time `bvec->bv_len < PAGE_SIZE' is possible (if it's not
then we can greatly simplify zram).
-ss
> > +
> > + int (*decompress)(const unsigned char *src, size_t src_len,
> > + unsigned char *dst, size_t *dst_len);
> > +
> > + void *(*create)(void);
> > + void (*destroy)(void *workmem);
> > +
> > + const char *name;
> > +};
> > +
> > +/* dynamic per-device compression frontend */
> > +struct zram_comp {
> > + /* protect workmem list */
> > + spinlock_t buffer_lock;
> > + /* list of available workmems */
> > + struct list_head idle_workmem;
> > + wait_queue_head_t workmem_wait;
> > + struct zram_comp_backend *backend;
> > +};
> > +
> > +struct zram_comp *zcomp_create(const char *comp);
> > +void zcomp_destroy(struct zram_comp *comp);
> > +
> > +struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp);
> > +void zcomp_workmem_put(struct zram_comp *comp,
> > + struct zcomp_workmem *workmem);
> > +
> > +int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
> > + const unsigned char *src, size_t src_len, size_t *dst_len);
> > +
> > +int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
> > + size_t src_len, unsigned char *dst, size_t *dst_len);
> > +#endif /* _ZRAM_COMP_H_ */
> > --
> > 1.9.0.rc3.244.g3497008
> >
> > --
> > 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv3 1/4] zram: introduce compressing backend abstraction
2014-02-11 22:21 ` [PATCHv3 1/4] zram: introduce compressing " Sergey Senozhatsky
2014-02-12 7:06 ` Minchan Kim
@ 2014-02-12 7:08 ` Minchan Kim
2014-02-12 8:38 ` Sergey Senozhatsky
1 sibling, 1 reply; 9+ messages in thread
From: Minchan Kim @ 2014-02-12 7:08 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On Wed, Feb 12, 2014 at 01:21:03AM +0300, Sergey Senozhatsky wrote:
> ZRAM performs direct LZO compression algorithm calls, making it the one
> and only option. Introduce abstract struct zram_comp in order to support
> multiple compression algorithms. struct zram_comp defines the following
> set of compressing backend operations:
> .create
> .destroy
> .compress
> .decompress
>
> Schematically zram write() usually contains the following steps:
> 0) preparation (decompression of partioal IO, etc.)
> 1) lock buffer_lock mutex (protects meta compress buffers)
> 2) compress (using meta compress buffers)
> 3) alloc and map zs_pool object
> 4) copy compressed data (from meta compress buffers) to object allocated by 3)
> 5) free previous pool page, assign a new one
> 6) unlock buffer_lock mutex
>
> As we can see, compressing buffers must remain untouched from 1) to 4),
> because, otherwise, concurrent write() can overwrite data. At the same
> time, zram_meta must be aware of a) specific compression algorithm
> memory requirements and b) necessary locking to protect compression
> buffers. Besides, zram holds buffer_lock almost through the whole write()
> function, making parallel compression impossible. To remove requirement
> a) new struct zcomp_workmem introduced, which contain buffers required by
> compression algorithm. While struct zram_comp implements workmem handling
> and locking by means of get() and put() semantics and removes requirement
> b) from zram meta. zram_comp ->create() and ->destroy(), respectively,
> allocate and deallocate algorithm specific zcomp_workmem `private' buffer.
>
> Every zram_comp has a list of idle workmem buffers (at least 1 workmem),
> spinlock to protect idle list and wait queue, making it possible to perform
> parallel compressions. Each time zram issues a zcomp_workmem_get() call, the
> following set of operations performed:
> - spin lock buffer_lock
> - if idle list is not empty, remove workmem from idle list, spin
> unlock and return workmem pointer
> - if idle list is empty, current adds itself to wait queue. it will be
> awaken by zcomp_workmem_put() caller.
>
> zcomp_workmem_put():
> - spin lock buffer_lock
> - add workmem to idle list
> - spin unlock, wake up sleeper (if any)
>
> In other words, zcomp_workmem_get() turns caller into exclusive user of workmem
> and zcomp_workem_put() makes a particular workmem available.
>
> Usage examples.
>
> To initialize compressing backend:
> comp = zcomp_create(NAME) /* NAME e.g. lzo */
>
> which initialises compressing backend if requested algorithm is supported.
>
> Compress:
> wm = zcomp_workmem_get(comp)
> zcomp_compress(comp, wm, src, src_len, &dst_len)
> [..] /* copy compressed data */
> zcomp_workmem_put(comp, wm)
>
> Decompress:
> zcomp_decompress(comp, src, src_len, dst, &dst_len);
>
> Free compessing backend and its workmem:
> zcomp_destroy(comp)
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> drivers/block/zram/zcomp_lzo.c | 33 +++++++++
> drivers/block/zram/zram_comp.c | 147 +++++++++++++++++++++++++++++++++++++++++
> drivers/block/zram/zram_comp.h | 58 ++++++++++++++++
> 3 files changed, 238 insertions(+)
> create mode 100644 drivers/block/zram/zcomp_lzo.c
> create mode 100644 drivers/block/zram/zram_comp.c
> create mode 100644 drivers/block/zram/zram_comp.h
>
> diff --git a/drivers/block/zram/zcomp_lzo.c b/drivers/block/zram/zcomp_lzo.c
> new file mode 100644
> index 0000000..bbde74e
> --- /dev/null
> +++ b/drivers/block/zram/zcomp_lzo.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/lzo.h>
> +
> +#include "zram_comp.h"
> +
> +static void * lzo_create(void)
> +{
> + return kmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> +}
> +
> +static void lzo_destroy(void *workmem)
> +{
> + kfree(workmem);
> +}
> +
> +extern struct zram_comp_backend zcomp_lzo;
> +struct zram_comp_backend zcomp_lzo = {
> + .compress = lzo1x_1_compress,
> + .decompress = lzo1x_decompress_safe,
> + .create = lzo_create,
> + .destroy = lzo_destroy,
> + .name = "lzo",
> +};
> diff --git a/drivers/block/zram/zram_comp.c b/drivers/block/zram/zram_comp.c
> new file mode 100644
> index 0000000..139a468
> --- /dev/null
> +++ b/drivers/block/zram/zram_comp.c
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/wait.h>
> +#include <linux/sched.h>
> +
> +#include "zram_comp.h"
> +
> +extern struct zram_comp_backend zcomp_lzo;
> +
> +static void workmem_free(struct zram_comp *comp, struct zcomp_workmem *workmem)
> +{
> + comp->backend->destroy(workmem->private);
> + free_pages((unsigned long)workmem->buffer, 1);
> + kfree(workmem);
> +}
> +
> +/* allocate new workmem structure with ->mem of requested size,
> + * return NULL on error */
> +static struct zcomp_workmem *workmem_alloc(struct zram_comp *comp)
> +{
> + struct zcomp_workmem *workmem = kmalloc(sizeof(*workmem), GFP_NOFS);
One more thing, pz, say why we need GFP_NOFS in here.
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCHv3 1/4] zram: introduce compressing backend abstraction
2014-02-12 7:08 ` Minchan Kim
@ 2014-02-12 8:38 ` Sergey Senozhatsky
0 siblings, 0 replies; 9+ messages in thread
From: Sergey Senozhatsky @ 2014-02-12 8:38 UTC (permalink / raw)
To: Minchan Kim; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On (02/12/14 16:08), Minchan Kim wrote:
[..]
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <linux/slab.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/wait.h>
> > +#include <linux/sched.h>
> > +
> > +#include "zram_comp.h"
> > +
> > +extern struct zram_comp_backend zcomp_lzo;
> > +
> > +static void workmem_free(struct zram_comp *comp, struct zcomp_workmem *workmem)
> > +{
> > + comp->backend->destroy(workmem->private);
> > + free_pages((unsigned long)workmem->buffer, 1);
> > + kfree(workmem);
> > +}
> > +
> > +/* allocate new workmem structure with ->mem of requested size,
> > + * return NULL on error */
> > +static struct zcomp_workmem *workmem_alloc(struct zram_comp *comp)
> > +{
> > + struct zcomp_workmem *workmem = kmalloc(sizeof(*workmem), GFP_NOFS);
>
>
> One more thing, pz, say why we need GFP_NOFS in here.
>
my bad. it definitely should be GFP_KERNEL.
-ss
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCHv3 2/4] zram: use zram_comp compressing backends
2014-02-11 22:21 [PATCHv3 0/4] add compression backend abstraction Sergey Senozhatsky
2014-02-11 22:21 ` [PATCHv3 1/4] zram: introduce compressing " Sergey Senozhatsky
@ 2014-02-11 22:21 ` Sergey Senozhatsky
2014-02-11 22:21 ` [PATCHv3 3/4] zram: support multi compressing buffers Sergey Senozhatsky
2014-02-11 22:21 ` [PATCHv3 4/4] zram: document max_buffers Sergey Senozhatsky
3 siblings, 0 replies; 9+ messages in thread
From: Sergey Senozhatsky @ 2014-02-11 22:21 UTC (permalink / raw)
To: Minchan Kim
Cc: Jerome Marchand, Nitin Gupta, linux-kernel, Sergey Senozhatsky
Do not perform direct LZO compress/decompress calls,
initialise and use zram_comp LZO backend instead.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/block/zram/Makefile | 2 +-
drivers/block/zram/zram_drv.c | 62 +++++++++++++++++++------------------------
drivers/block/zram/zram_drv.h | 8 +++---
3 files changed, 33 insertions(+), 39 deletions(-)
diff --git a/drivers/block/zram/Makefile b/drivers/block/zram/Makefile
index cb0f9ce..4c2e829 100644
--- a/drivers/block/zram/Makefile
+++ b/drivers/block/zram/Makefile
@@ -1,3 +1,3 @@
-zram-y := zram_drv.o
+zram-y := zcomp_lzo.o zram_comp.o zram_drv.o
obj-$(CONFIG_ZRAM) += zram.o
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 9baac5b..4875a16 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -29,15 +29,14 @@
#include <linux/genhd.h>
#include <linux/highmem.h>
#include <linux/slab.h>
-#include <linux/lzo.h>
#include <linux/string.h>
-#include <linux/vmalloc.h>
#include "zram_drv.h"
/* Globals */
static int zram_major;
static struct zram *zram_devices;
+static const char *default_compressor = "lzo";
/* Module params (documentation at end) */
static unsigned int num_devices = 1;
@@ -160,8 +159,6 @@ static inline int valid_io_request(struct zram *zram, struct bio *bio)
static void zram_meta_free(struct zram_meta *meta)
{
zs_destroy_pool(meta->mem_pool);
- kfree(meta->compress_workmem);
- free_pages((unsigned long)meta->compress_buffer, 1);
vfree(meta->table);
kfree(meta);
}
@@ -173,22 +170,11 @@ static struct zram_meta *zram_meta_alloc(u64 disksize)
if (!meta)
goto out;
- meta->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
- if (!meta->compress_workmem)
- goto free_meta;
-
- meta->compress_buffer =
- (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
- if (!meta->compress_buffer) {
- pr_err("Error allocating compressor buffer space\n");
- goto free_workmem;
- }
-
num_pages = disksize >> PAGE_SHIFT;
meta->table = vzalloc(num_pages * sizeof(*meta->table));
if (!meta->table) {
pr_err("Error allocating zram address table\n");
- goto free_buffer;
+ goto free_meta;
}
meta->mem_pool = zs_create_pool(GFP_NOIO | __GFP_HIGHMEM);
@@ -198,15 +184,10 @@ static struct zram_meta *zram_meta_alloc(u64 disksize)
}
rwlock_init(&meta->tb_lock);
- mutex_init(&meta->buffer_lock);
return meta;
free_table:
vfree(meta->table);
-free_buffer:
- free_pages((unsigned long)meta->compress_buffer, 1);
-free_workmem:
- kfree(meta->compress_workmem);
free_meta:
kfree(meta);
meta = NULL;
@@ -280,7 +261,7 @@ static void zram_free_page(struct zram *zram, size_t index)
static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
{
- int ret = LZO_E_OK;
+ int ret = 0;
size_t clen = PAGE_SIZE;
unsigned char *cmem;
struct zram_meta *meta = zram->meta;
@@ -301,12 +282,12 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
if (size == PAGE_SIZE)
copy_page(mem, cmem);
else
- ret = lzo1x_decompress_safe(cmem, size, mem, &clen);
+ ret = zcomp_decompress(zram->comp, cmem, size, mem, &clen);
zs_unmap_object(meta->mem_pool, handle);
read_unlock(&meta->tb_lock);
/* Should NEVER happen. Return bio error if it does. */
- if (unlikely(ret != LZO_E_OK)) {
+ if (unlikely(ret)) {
pr_err("Decompression failed! err=%d, page=%u\n", ret, index);
atomic64_inc(&zram->stats.failed_reads);
return ret;
@@ -349,7 +330,7 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
ret = zram_decompress_page(zram, uncmem, index);
/* Should NEVER happen. Return bio error if it does. */
- if (unlikely(ret != LZO_E_OK))
+ if (unlikely(ret))
goto out_cleanup;
if (is_partial_io(bvec))
@@ -374,11 +355,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
struct page *page;
unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
struct zram_meta *meta = zram->meta;
+ struct zcomp_workmem *wm;
bool locked = false;
page = bvec->bv_page;
- src = meta->compress_buffer;
-
if (is_partial_io(bvec)) {
/*
* This is a partial IO. We need to read the full page
@@ -394,7 +374,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
goto out;
}
- mutex_lock(&meta->buffer_lock);
+ wm = zcomp_workmem_get(zram->comp);
locked = true;
user_mem = kmap_atomic(page);
@@ -420,19 +400,18 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
goto out;
}
- ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
- meta->compress_workmem);
+ ret = zcomp_compress(zram->comp, wm, uncmem, bvec->bv_len, &clen);
if (!is_partial_io(bvec)) {
kunmap_atomic(user_mem);
user_mem = NULL;
uncmem = NULL;
}
- if (unlikely(ret != LZO_E_OK)) {
+ if (unlikely(ret)) {
pr_err("Compression failed! err=%d\n", ret);
goto out;
}
-
+ src = wm->buffer;
if (unlikely(clen > max_zpage_size)) {
clen = PAGE_SIZE;
src = NULL;
@@ -457,6 +436,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
memcpy(cmem, src, clen);
}
+ zcomp_workmem_put(zram->comp, wm);
+ locked = false;
zs_unmap_object(meta->mem_pool, handle);
/*
@@ -475,10 +456,9 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
atomic64_inc(&zram->stats.pages_stored);
out:
if (locked)
- mutex_unlock(&meta->buffer_lock);
+ zcomp_workmem_put(zram->comp, wm);
if (is_partial_io(bvec))
kfree(uncmem);
-
if (ret)
atomic64_inc(&zram->stats.failed_writes);
return ret;
@@ -522,6 +502,10 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
zs_free(meta->mem_pool, handle);
}
+ if (zram->comp)
+ zcomp_destroy(zram->comp);
+ zram->comp = NULL;
+
zram_meta_free(zram->meta);
zram->meta = NULL;
/* Reset stats */
@@ -550,9 +534,18 @@ static ssize_t disksize_store(struct device *dev,
return -EBUSY;
}
+ zram->comp = zcomp_create(default_compressor);
+ if (!zram->comp) {
+ up_write(&zram->init_lock);
+ pr_info("Cannot initialise compressing backend\n");
+ return -EINVAL;
+ }
+
disksize = PAGE_ALIGN(disksize);
zram->meta = zram_meta_alloc(disksize);
if (!zram->meta) {
+ zcomp_destroy(zram->comp);
+ zram->comp = NULL;
up_write(&zram->init_lock);
return -ENOMEM;
}
@@ -790,6 +783,7 @@ static int create_device(struct zram *zram, int device_id)
}
zram->meta = NULL;
+ zram->comp = NULL;
return 0;
out_free_disk:
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 1d5b1f5..a2e17d7 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -16,9 +16,10 @@
#define _ZRAM_DRV_H_
#include <linux/spinlock.h>
-#include <linux/mutex.h>
#include <linux/zsmalloc.h>
+#include "zram_comp.h"
+
/*
* Some arbitrary value. This is just to catch
* invalid value for num_devices module parameter.
@@ -81,17 +82,16 @@ struct zram_stats {
struct zram_meta {
rwlock_t tb_lock; /* protect table */
- void *compress_workmem;
- void *compress_buffer;
struct table *table;
struct zs_pool *mem_pool;
- struct mutex buffer_lock; /* protect compress buffers */
};
struct zram {
struct zram_meta *meta;
struct request_queue *queue;
struct gendisk *disk;
+ struct zram_comp *comp;
+
/* Prevent concurrent execution of device init, reset and R/W request */
struct rw_semaphore init_lock;
/*
--
1.9.0.rc3.244.g3497008
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCHv3 3/4] zram: support multi compressing buffers
2014-02-11 22:21 [PATCHv3 0/4] add compression backend abstraction Sergey Senozhatsky
2014-02-11 22:21 ` [PATCHv3 1/4] zram: introduce compressing " Sergey Senozhatsky
2014-02-11 22:21 ` [PATCHv3 2/4] zram: use zram_comp compressing backends Sergey Senozhatsky
@ 2014-02-11 22:21 ` Sergey Senozhatsky
2014-02-11 22:21 ` [PATCHv3 4/4] zram: document max_buffers Sergey Senozhatsky
3 siblings, 0 replies; 9+ messages in thread
From: Sergey Senozhatsky @ 2014-02-11 22:21 UTC (permalink / raw)
To: Minchan Kim
Cc: Jerome Marchand, Nitin Gupta, linux-kernel, Sergey Senozhatsky
1) Extend zram_comp zcomp_create() with `num_wm' parameter.
`num_wm' limits the number of workmem structs in compression
backend's idle list (max buffers).
2) Introduce zram device attribute max_buffers to show and
store current device's zram_comp max number (num_wm) of workmems.
Usage example:
echo 4 > /sys/block/zram0/max_buffers
set max_buffers to 4
cat /sys/block/zram0/max_buffers
show current max_buffers (default value is 1).
iozone -t -T -R -l 3 -u 3 -r 16K -s 60M -I +Z
(write tests. ext4, lzo, each test performed on newly created
zram0 device)
test max_buffers 1 max_buffers 3
---------------------------------------------------
" Initial write " 631583.39 | 727834.88
" Rewrite " 456248.23 | 2105054.97
" Mixed workload " 3370016.43 | 3509382.86
" Random write " 467703.56 | 1953464.78
" Pwrite " 629146.88 | 775372.46
" Fwrite " 2573117.39 | 2529637.62
test max_buffers 1 max_buffers 3
---------------------------------------------------
" Initial write " 601112.91 | 765866.05
" Rewrite " 447834.33 | 2163449.58
" Mixed workload " 2575128.08 | 3257711.77
" Random write " 461327.45 | 1974731.12
" Pwrite " 567038.36 | 740242.52
" Fwrite " 2527107.44 | 2754336.64
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/block/zram/zram_comp.c | 15 +++++++++------
drivers/block/zram/zram_comp.h | 2 +-
drivers/block/zram/zram_drv.c | 42 +++++++++++++++++++++++++++++++++++++++++-
drivers/block/zram/zram_drv.h | 2 +-
4 files changed, 52 insertions(+), 9 deletions(-)
diff --git a/drivers/block/zram/zram_comp.c b/drivers/block/zram/zram_comp.c
index 139a468..d9b34ba 100644
--- a/drivers/block/zram/zram_comp.c
+++ b/drivers/block/zram/zram_comp.c
@@ -118,11 +118,12 @@ static struct zram_comp_backend *find_backend(const char *compress)
* allocate new zram_comp and initialize it. return NULL
* if requested algorithm is not supported or in case
* of init error */
-struct zram_comp *zcomp_create(const char *compress)
+struct zram_comp *zcomp_create(const char *compress, int num_wm)
{
struct zram_comp *comp;
struct zram_comp_backend *backend;
struct zcomp_workmem *wm;
+ int i;
backend = find_backend(compress);
if (!backend)
@@ -137,11 +138,13 @@ struct zram_comp *zcomp_create(const char *compress)
INIT_LIST_HEAD(&comp->idle_workmem);
init_waitqueue_head(&comp->workmem_wait);
- wm = workmem_alloc(comp);
- if (!wm) {
- zcomp_destroy(comp);
- return NULL;
+ for (i = 0; i < num_wm; i++) {
+ wm = workmem_alloc(comp);
+ if (!wm) {
+ zcomp_destroy(comp);
+ return NULL;
+ }
+ list_add_tail(&wm->list, &comp->idle_workmem);
}
- list_add_tail(&wm->list, &comp->idle_workmem);
return comp;
}
diff --git a/drivers/block/zram/zram_comp.h b/drivers/block/zram/zram_comp.h
index 90355e8..277afa0 100644
--- a/drivers/block/zram/zram_comp.h
+++ b/drivers/block/zram/zram_comp.h
@@ -43,7 +43,7 @@ struct zram_comp {
struct zram_comp_backend *backend;
};
-struct zram_comp *zcomp_create(const char *comp);
+struct zram_comp *zcomp_create(const char *comp, int num_wm);
void zcomp_destroy(struct zram_comp *comp);
struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp);
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 4875a16..a065c1b 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -107,6 +107,41 @@ static ssize_t mem_used_total_show(struct device *dev,
return sprintf(buf, "%llu\n", val);
}
+static ssize_t max_buffers_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ long val;
+ struct zram *zram = dev_to_zram(dev);
+
+ down_read(&zram->init_lock);
+ val = zram->max_buffers;
+ up_read(&zram->init_lock);
+
+ return sprintf(buf, "%ld\n", val);
+}
+
+static ssize_t max_buffers_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ long num;
+ struct zram *zram = dev_to_zram(dev);
+
+ if (kstrtol(buf, 0, &num))
+ return -EINVAL;
+ if (num < 1 || num > 100)
+ return -EINVAL;
+ down_write(&zram->init_lock);
+ if (init_done(zram)) {
+ up_write(&zram->init_lock);
+ pr_info("Cannot set max buffers for initialized device\n");
+ return -EBUSY;
+ }
+ zram->max_buffers = num;
+ up_write(&zram->init_lock);
+
+ return len;
+}
+
/* flag operations needs meta->tb_lock */
static int zram_test_flag(struct zram_meta *meta, u32 index,
enum zram_pageflags flag)
@@ -505,6 +540,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
if (zram->comp)
zcomp_destroy(zram->comp);
zram->comp = NULL;
+ zram->max_buffers = 1;
zram_meta_free(zram->meta);
zram->meta = NULL;
@@ -534,7 +570,7 @@ static ssize_t disksize_store(struct device *dev,
return -EBUSY;
}
- zram->comp = zcomp_create(default_compressor);
+ zram->comp = zcomp_create(default_compressor, zram->max_buffers);
if (!zram->comp) {
up_write(&zram->init_lock);
pr_info("Cannot initialise compressing backend\n");
@@ -697,6 +733,8 @@ static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
+static DEVICE_ATTR(max_buffers, S_IRUGO | S_IWUSR,
+ max_buffers_show, max_buffers_store);
ZRAM_ATTR_RO(num_reads);
ZRAM_ATTR_RO(num_writes);
@@ -721,6 +759,7 @@ static struct attribute *zram_disk_attrs[] = {
&dev_attr_orig_data_size.attr,
&dev_attr_compr_data_size.attr,
&dev_attr_mem_used_total.attr,
+ &dev_attr_max_buffers.attr,
NULL,
};
@@ -784,6 +823,7 @@ static int create_device(struct zram *zram, int device_id)
zram->meta = NULL;
zram->comp = NULL;
+ zram->max_buffers = 1;
return 0;
out_free_disk:
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index a2e17d7..68a5421 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -99,7 +99,7 @@ struct zram {
* we can store in a disk.
*/
u64 disksize; /* bytes */
-
+ long max_buffers;
struct zram_stats stats;
};
#endif
--
1.9.0.rc3.244.g3497008
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCHv3 4/4] zram: document max_buffers
2014-02-11 22:21 [PATCHv3 0/4] add compression backend abstraction Sergey Senozhatsky
` (2 preceding siblings ...)
2014-02-11 22:21 ` [PATCHv3 3/4] zram: support multi compressing buffers Sergey Senozhatsky
@ 2014-02-11 22:21 ` Sergey Senozhatsky
3 siblings, 0 replies; 9+ messages in thread
From: Sergey Senozhatsky @ 2014-02-11 22:21 UTC (permalink / raw)
To: Minchan Kim
Cc: Jerome Marchand, Nitin Gupta, linux-kernel, Sergey Senozhatsky
Add max_buffers device attribute documentation.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
Documentation/ABI/testing/sysfs-block-zram | 9 ++++++++-
Documentation/blockdev/zram.txt | 21 ++++++++++++++++-----
2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-block-zram b/Documentation/ABI/testing/sysfs-block-zram
index 8aa0468..0e36bf7 100644
--- a/Documentation/ABI/testing/sysfs-block-zram
+++ b/Documentation/ABI/testing/sysfs-block-zram
@@ -50,7 +50,6 @@ Description:
The failed_reads file is read-only and specifies the number of
failed reads happened on this device.
-
What: /sys/block/zram<id>/failed_writes
Date: February 2014
Contact: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
@@ -58,6 +57,14 @@ Description:
The failed_writes file is read-only and specifies the number of
failed writes happened on this device.
+What: /sys/block/zram<id>/max_buffers
+Date: February 2014
+Contact: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
+Description:
+ The max_buffers file is read-write and specifies the number of
+ compression backend's workmem buffers (number of concurrent
+ compress/decompress operations).
+
What: /sys/block/zram<id>/notify_free
Date: August 2010
Contact: Nitin Gupta <ngupta@vflare.org>
diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index b31ac5e..b525c8e 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -21,7 +21,18 @@ Following shows a typical sequence of steps for using zram.
This creates 4 devices: /dev/zram{0,1,2,3}
(num_devices parameter is optional. Default: 1)
-2) Set Disksize
+2) Set max number of compression buffers
+ Compression backend may use up to max_buffers workmem buffers,
+ thus allowing up to max_buffers concurrent compression/decompression
+ operations.
+ Examples:
+ #set max buffers to 3
+ echo 3 > /sys/block/zram0/max_buffers
+
+ #show max buffers
+ cat /sys/block/zram0/max_buffers
+
+3) Set Disksize
Set disk size by writing the value to sysfs node 'disksize'.
The value can be either in bytes or you can use mem suffixes.
Examples:
@@ -38,14 +49,14 @@ There is little point creating a zram of greater than twice the size of memory
since we expect a 2:1 compression ratio. Note that zram uses about 0.1% of the
size of the disk when not in use so a huge zram is wasteful.
-3) Activate:
+4) Activate:
mkswap /dev/zram0
swapon /dev/zram0
mkfs.ext4 /dev/zram1
mount /dev/zram1 /tmp
-4) Stats:
+5) Stats:
Per-device statistics are exported as various nodes under
/sys/block/zram<id>/
disksize
@@ -60,11 +71,11 @@ size of the disk when not in use so a huge zram is wasteful.
compr_data_size
mem_used_total
-5) Deactivate:
+6) Deactivate:
swapoff /dev/zram0
umount /dev/zram1
-6) Reset:
+7) Reset:
Write any positive value to 'reset' sysfs node
echo 1 > /sys/block/zram0/reset
echo 1 > /sys/block/zram1/reset
--
1.9.0.rc3.244.g3497008
^ permalink raw reply related [flat|nested] 9+ messages in thread