* [PATCHv6 1/6] zram: introduce compressing backend abstraction
2014-02-21 11:50 [PATCHv6 0/6] add compressing abstraction and multi stream support Sergey Senozhatsky
@ 2014-02-21 11:50 ` Sergey Senozhatsky
2014-02-24 0:37 ` Minchan Kim
2014-02-21 11:50 ` [PATCHv6 2/6] zram: use zcomp compressing backends Sergey Senozhatsky
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-21 11:50 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 compressing backend abstraction zcomp in order to
support multiple compression algorithms with the following set of 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. To remove requirement
a) new struct zcomp_strm introduced, which contains a compress/decompress
`buffer' and compression algorithm `private' part. While struct zcomp implements
zcomp_strm stream handling and locking by means of get() and put() semantics and
removes requirement b) from zram meta. zcomp ->create() and ->destroy(),
respectively, allocate and deallocate algorithm specific zcomp_strm `private'
part.
Every zcomp has zcomp stream and mutex to protect its compression stream. Stream
usage semantics remains the same -- only one write can hold stream lock and use
its buffers. zcomp_strm_get() turns caller into exclusive user of a stream
(holding stream mutex until zram put stream), and zcomp_strm_put() makes zcomp
stream available (unlock the stream mutex). Hence no concurrent write
(compression) operations possible at the moment.
iozone -t 3 -R -r 16K -s 60M -I +Z
test base patched
--------------------------------------------------
Initial write 597992.91 591660.58
Rewrite 609674.34 616054.97
Read 2404771.75 2452909.12
Re-read 2459216.81 2470074.44
Reverse Read 1652769.66 1589128.66
Stride read 2202441.81 2202173.31
Random read 2236311.47 2276565.31
Mixed workload 1423760.41 1709760.06
Random write 579584.08 615933.86
Pwrite 597550.02 594933.70
Pread 1703672.53 1718126.72
Fwrite 1330497.06 1461054.00
Fread 3922851.00 3957242.62
Usage examples:
comp = zcomp_create(NAME) /* NAME e.g. "lzo" */
which initialises compressing backend if requested algorithm is supported.
Compress:
zstrm = zcomp_strm_get(comp)
zcomp_compress(comp, zstrm, src, &dst_len)
[..] /* copy compressed data */
zcomp_strm_put(comp, zstrm)
Decompress:
zcomp_decompress(comp, src, src_len, dst);
Free compessing backend and its zcomp stream:
zcomp_destroy(comp)
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/block/zram/zcomp.c | 116 +++++++++++++++++++++++++++++++++++++++++
drivers/block/zram/zcomp.h | 53 +++++++++++++++++++
drivers/block/zram/zcomp_lzo.c | 48 +++++++++++++++++
3 files changed, 217 insertions(+)
create mode 100644 drivers/block/zram/zcomp.c
create mode 100644 drivers/block/zram/zcomp.h
create mode 100644 drivers/block/zram/zcomp_lzo.c
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
new file mode 100644
index 0000000..db72f3d
--- /dev/null
+++ b/drivers/block/zram/zcomp.c
@@ -0,0 +1,116 @@
+/*
+ * 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/wait.h>
+#include <linux/sched.h>
+
+#include "zcomp.h"
+
+extern struct zcomp_backend zcomp_lzo;
+
+static struct zcomp_backend *find_backend(const char *compress)
+{
+ if (strncmp(compress, "lzo", 3) == 0)
+ return &zcomp_lzo;
+ return NULL;
+}
+
+static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm)
+{
+ if (zstrm->private)
+ comp->backend->destroy(zstrm->private);
+ free_pages((unsigned long)zstrm->buffer, 1);
+ kfree(zstrm);
+}
+
+/*
+ * allocate new zcomp_strm structure with ->private initialized by
+ * backend, return NULL on error
+ */
+static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
+{
+ struct zcomp_strm *zstrm = kmalloc(sizeof(*zstrm), GFP_KERNEL);
+ if (!zstrm)
+ return NULL;
+
+ zstrm->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
+ */
+ zstrm->buffer = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
+ if (!zstrm->private || !zstrm->buffer) {
+ zcomp_strm_free(comp, zstrm);
+ zstrm = NULL;
+ }
+ return zstrm;
+}
+
+struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
+{
+ mutex_lock(&comp->strm_lock);
+ return comp->zstrm;
+}
+
+void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm)
+{
+ mutex_unlock(&comp->strm_lock);
+}
+
+int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
+ const unsigned char *src, size_t *dst_len)
+{
+ return comp->backend->compress(src, zstrm->buffer, dst_len,
+ zstrm->private);
+}
+
+int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
+ size_t src_len, unsigned char *dst)
+{
+ return comp->backend->decompress(src, src_len, dst);
+}
+
+void zcomp_destroy(struct zcomp *comp)
+{
+ zcomp_strm_free(comp, comp->zstrm);
+ kfree(comp);
+}
+
+/*
+ * search available compressors for requested algorithm.
+ * allocate new zcomp and initialize it. return NULL
+ * if requested algorithm is not supported or in case
+ * of init error
+ */
+struct zcomp *zcomp_create(const char *compress)
+{
+ struct zcomp *comp;
+ struct zcomp_backend *backend;
+
+ backend = find_backend(compress);
+ if (!backend)
+ return NULL;
+
+ comp = kmalloc(sizeof(struct zcomp), GFP_KERNEL);
+ if (!comp)
+ return NULL;
+
+ comp->backend = backend;
+ mutex_init(&comp->strm_lock);
+
+ comp->zstrm = zcomp_strm_alloc(comp);
+ if (!comp->zstrm) {
+ zcomp_destroy(comp);
+ return NULL;
+ }
+ return comp;
+}
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
new file mode 100644
index 0000000..5106f8e
--- /dev/null
+++ b/drivers/block/zram/zcomp.h
@@ -0,0 +1,53 @@
+/*
+ * 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 _ZCOMP_H_
+#define _ZCOMP_H_
+
+#include <linux/mutex.h>
+
+struct zcomp_strm {
+ void *buffer; /* compression/decompression buffer */
+ void *private;
+ struct list_head list;
+};
+
+/* static compression backend */
+struct zcomp_backend {
+ int (*compress)(const unsigned char *src, unsigned char *dst,
+ size_t *dst_len, void *private);
+
+ int (*decompress)(const unsigned char *src, size_t src_len,
+ unsigned char *dst);
+
+ void *(*create)(void);
+ void (*destroy)(void *private);
+
+ const char *name;
+};
+
+/* dynamic per-device compression frontend */
+struct zcomp {
+ struct mutex strm_lock;
+ struct zcomp_strm *zstrm;
+ struct zcomp_backend *backend;
+};
+
+struct zcomp *zcomp_create(const char *comp);
+void zcomp_destroy(struct zcomp *comp);
+
+struct zcomp_strm *zcomp_strm_get(struct zcomp *comp);
+void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm);
+
+int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
+ const unsigned char *src, size_t *dst_len);
+
+int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
+ size_t src_len, unsigned char *dst);
+#endif /* _ZCOMP_H_ */
diff --git a/drivers/block/zram/zcomp_lzo.c b/drivers/block/zram/zcomp_lzo.c
new file mode 100644
index 0000000..b7722a2
--- /dev/null
+++ b/drivers/block/zram/zcomp_lzo.c
@@ -0,0 +1,48 @@
+/*
+ * 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 "zcomp.h"
+
+static void *lzo_create(void)
+{
+ return kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
+}
+
+static void lzo_destroy(void *private)
+{
+ kfree(private);
+}
+
+static int lzo_compress(const unsigned char *src, unsigned char *dst,
+ size_t *dst_len, void *private)
+{
+ int ret = lzo1x_1_compress(src, PAGE_SIZE, dst, dst_len, private);
+ return ret == LZO_E_OK ? 0 : ret;
+}
+
+static int lzo_decompress(const unsigned char *src, size_t src_len,
+ unsigned char *dst)
+{
+ size_t dst_len = PAGE_SIZE;
+ int ret = lzo1x_decompress_safe(src, src_len, dst, &dst_len);
+ return ret == LZO_E_OK ? 0 : ret;
+}
+
+extern struct zcomp_backend zcomp_lzo;
+struct zcomp_backend zcomp_lzo = {
+ .compress = lzo_compress,
+ .decompress = lzo_decompress,
+ .create = lzo_create,
+ .destroy = lzo_destroy,
+ .name = "lzo",
+};
--
1.9.0.291.g027825b
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCHv6 1/6] zram: introduce compressing backend abstraction
2014-02-21 11:50 ` [PATCHv6 1/6] zram: introduce compressing backend abstraction Sergey Senozhatsky
@ 2014-02-24 0:37 ` Minchan Kim
0 siblings, 0 replies; 23+ messages in thread
From: Minchan Kim @ 2014-02-24 0:37 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Jerome Marchand, Nitin Gupta, linux-kernel, Andrew Morton
Hello Sergey,
Hmm, I seem to know why you took a mistake about adding Ccing Andrew
because ./get_maintainer.pl doesn't say about Andrew. So it's totally
not your fault. But I don't have public git tree for zram and Andrew
usually pick zram patches with my Acked-by into mmotm tree.
So if we follow such model, we need something for ./get_maintainer.pl
to say about Andrew. Otherwise, I need a public tree and pull request
periodically. I will discuss with Andrew what's perferred/best way.
Anyway, until done, pz, add Andrew.
On Fri, Feb 21, 2014 at 02:50:38PM +0300, Sergey Senozhatsky wrote:
> ZRAM performs direct LZO compression algorithm calls, making it the one and
> only option. Introduce compressing backend abstraction zcomp in order to
> support multiple compression algorithms with the following set of 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. To remove requirement
> a) new struct zcomp_strm introduced, which contains a compress/decompress
> `buffer' and compression algorithm `private' part. While struct zcomp implements
> zcomp_strm stream handling and locking by means of get() and put() semantics and
> removes requirement b) from zram meta. zcomp ->create() and ->destroy(),
> respectively, allocate and deallocate algorithm specific zcomp_strm `private'
> part.
>
> Every zcomp has zcomp stream and mutex to protect its compression stream. Stream
> usage semantics remains the same -- only one write can hold stream lock and use
> its buffers. zcomp_strm_get() turns caller into exclusive user of a stream
> (holding stream mutex until zram put stream), and zcomp_strm_put() makes zcomp
> stream available (unlock the stream mutex). Hence no concurrent write
> (compression) operations possible at the moment.
>
> iozone -t 3 -R -r 16K -s 60M -I +Z
>
> test base patched
> --------------------------------------------------
> Initial write 597992.91 591660.58
> Rewrite 609674.34 616054.97
> Read 2404771.75 2452909.12
> Re-read 2459216.81 2470074.44
> Reverse Read 1652769.66 1589128.66
> Stride read 2202441.81 2202173.31
> Random read 2236311.47 2276565.31
> Mixed workload 1423760.41 1709760.06
> Random write 579584.08 615933.86
> Pwrite 597550.02 594933.70
> Pread 1703672.53 1718126.72
> Fwrite 1330497.06 1461054.00
> Fread 3922851.00 3957242.62
>
> Usage examples:
>
> comp = zcomp_create(NAME) /* NAME e.g. "lzo" */
>
> which initialises compressing backend if requested algorithm is supported.
>
> Compress:
> zstrm = zcomp_strm_get(comp)
> zcomp_compress(comp, zstrm, src, &dst_len)
> [..] /* copy compressed data */
> zcomp_strm_put(comp, zstrm)
>
> Decompress:
> zcomp_decompress(comp, src, src_len, dst);
>
> Free compessing backend and its zcomp stream:
> zcomp_destroy(comp)
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> drivers/block/zram/zcomp.c | 116 +++++++++++++++++++++++++++++++++++++++++
> drivers/block/zram/zcomp.h | 53 +++++++++++++++++++
> drivers/block/zram/zcomp_lzo.c | 48 +++++++++++++++++
> 3 files changed, 217 insertions(+)
> create mode 100644 drivers/block/zram/zcomp.c
> create mode 100644 drivers/block/zram/zcomp.h
> create mode 100644 drivers/block/zram/zcomp_lzo.c
>
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> new file mode 100644
> index 0000000..db72f3d
> --- /dev/null
> +++ b/drivers/block/zram/zcomp.c
> @@ -0,0 +1,116 @@
> +/*
> + * 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/wait.h>
> +#include <linux/sched.h>
> +
> +#include "zcomp.h"
> +
> +extern struct zcomp_backend zcomp_lzo;
> +
> +static struct zcomp_backend *find_backend(const char *compress)
> +{
> + if (strncmp(compress, "lzo", 3) == 0)
> + return &zcomp_lzo;
> + return NULL;
> +}
> +
> +static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm)
> +{
> + if (zstrm->private)
> + comp->backend->destroy(zstrm->private);
> + free_pages((unsigned long)zstrm->buffer, 1);
> + kfree(zstrm);
> +}
> +
> +/*
> + * allocate new zcomp_strm structure with ->private initialized by
> + * backend, return NULL on error
> + */
> +static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
> +{
> + struct zcomp_strm *zstrm = kmalloc(sizeof(*zstrm), GFP_KERNEL);
> + if (!zstrm)
> + return NULL;
> +
> + zstrm->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
> + */
> + zstrm->buffer = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
> + if (!zstrm->private || !zstrm->buffer) {
> + zcomp_strm_free(comp, zstrm);
> + zstrm = NULL;
> + }
> + return zstrm;
> +}
> +
> +struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
> +{
> + mutex_lock(&comp->strm_lock);
> + return comp->zstrm;
> +}
> +
> +void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> +{
> + mutex_unlock(&comp->strm_lock);
> +}
> +
> +int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> + const unsigned char *src, size_t *dst_len)
> +{
> + return comp->backend->compress(src, zstrm->buffer, dst_len,
> + zstrm->private);
> +}
> +
> +int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> + size_t src_len, unsigned char *dst)
> +{
> + return comp->backend->decompress(src, src_len, dst);
> +}
> +
> +void zcomp_destroy(struct zcomp *comp)
> +{
> + zcomp_strm_free(comp, comp->zstrm);
> + kfree(comp);
> +}
> +
> +/*
> + * search available compressors for requested algorithm.
> + * allocate new zcomp and initialize it. return NULL
> + * if requested algorithm is not supported or in case
> + * of init error
> + */
> +struct zcomp *zcomp_create(const char *compress)
> +{
> + struct zcomp *comp;
> + struct zcomp_backend *backend;
> +
> + backend = find_backend(compress);
> + if (!backend)
> + return NULL;
> +
> + comp = kmalloc(sizeof(struct zcomp), GFP_KERNEL);
> + if (!comp)
> + return NULL;
> +
> + comp->backend = backend;
> + mutex_init(&comp->strm_lock);
> +
> + comp->zstrm = zcomp_strm_alloc(comp);
> + if (!comp->zstrm) {
> + zcomp_destroy(comp);
Just use kfree instead of zcomp_destroy.
Pz, add my Acked-by in later version if you fix it.
Acked-by: Minchan Kim <minchan@kernel.org>
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCHv6 2/6] zram: use zcomp compressing backends
2014-02-21 11:50 [PATCHv6 0/6] add compressing abstraction and multi stream support Sergey Senozhatsky
2014-02-21 11:50 ` [PATCHv6 1/6] zram: introduce compressing backend abstraction Sergey Senozhatsky
@ 2014-02-21 11:50 ` Sergey Senozhatsky
2014-02-24 1:01 ` Minchan Kim
2014-02-21 11:50 ` [PATCHv6 3/6] zram: factor out single stream compression Sergey Senozhatsky
` (3 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-21 11:50 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 zcomp LZO backend (single compression stream) 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, 32 insertions(+), 40 deletions(-)
diff --git a/drivers/block/zram/Makefile b/drivers/block/zram/Makefile
index cb0f9ce..757c6a5 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 zcomp.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..200c12a 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,8 +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;
- size_t clen = PAGE_SIZE;
+ int ret = 0;
unsigned char *cmem;
struct zram_meta *meta = zram->meta;
unsigned long handle;
@@ -301,12 +281,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);
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 +329,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 +354,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_strm *zstrm;
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 +373,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
goto out;
}
- mutex_lock(&meta->buffer_lock);
+ zstrm = zcomp_strm_get(zram->comp);
locked = true;
user_mem = kmap_atomic(page);
@@ -420,19 +399,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, zstrm, uncmem, &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 = zstrm->buffer;
if (unlikely(clen > max_zpage_size)) {
clen = PAGE_SIZE;
src = NULL;
@@ -457,6 +435,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
memcpy(cmem, src, clen);
}
+ zcomp_strm_put(zram->comp, zstrm);
+ locked = false;
zs_unmap_object(meta->mem_pool, handle);
/*
@@ -475,10 +455,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_strm_put(zram->comp, zstrm);
if (is_partial_io(bvec))
kfree(uncmem);
-
if (ret)
atomic64_inc(&zram->stats.failed_writes);
return ret;
@@ -522,6 +501,9 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
zs_free(meta->mem_pool, handle);
}
+ zcomp_destroy(zram->comp);
+ zram->comp = NULL;
+
zram_meta_free(zram->meta);
zram->meta = NULL;
/* Reset stats */
@@ -550,9 +532,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 +781,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..45e04f7 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 "zcomp.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 zcomp *comp;
+
/* Prevent concurrent execution of device init, reset and R/W request */
struct rw_semaphore init_lock;
/*
--
1.9.0.291.g027825b
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCHv6 2/6] zram: use zcomp compressing backends
2014-02-21 11:50 ` [PATCHv6 2/6] zram: use zcomp compressing backends Sergey Senozhatsky
@ 2014-02-24 1:01 ` Minchan Kim
2014-02-24 8:39 ` Sergey Senozhatsky
0 siblings, 1 reply; 23+ messages in thread
From: Minchan Kim @ 2014-02-24 1:01 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On Fri, Feb 21, 2014 at 02:50:39PM +0300, Sergey Senozhatsky wrote:
> Do not perform direct LZO compress/decompress calls, initialise
> and use zcomp LZO backend (single compression stream) 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, 32 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/block/zram/Makefile b/drivers/block/zram/Makefile
> index cb0f9ce..757c6a5 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 zcomp.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..200c12a 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>
In ARM,
drivers/block/zram/zram_drv.c:161:2: error: implicit declaration of function 'vfree'
drivers/block/zram/zram_drv.c:173:2: error: implicit declaration of function 'vzalloc'
>
> #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,8 +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;
> - size_t clen = PAGE_SIZE;
> + int ret = 0;
> unsigned char *cmem;
> struct zram_meta *meta = zram->meta;
> unsigned long handle;
> @@ -301,12 +281,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);
> 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 +329,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 +354,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_strm *zstrm;
> 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 +373,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> goto out;
> }
>
> - mutex_lock(&meta->buffer_lock);
> + zstrm = zcomp_strm_get(zram->comp);
> locked = true;
> user_mem = kmap_atomic(page);
>
> @@ -420,19 +399,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, zstrm, uncmem, &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 = zstrm->buffer;
> if (unlikely(clen > max_zpage_size)) {
> clen = PAGE_SIZE;
> src = NULL;
Do we need this line? src = NULL?
> @@ -457,6 +435,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> memcpy(cmem, src, clen);
> }
>
> + zcomp_strm_put(zram->comp, zstrm);
> + locked = false;
> zs_unmap_object(meta->mem_pool, handle);
>
> /*
> @@ -475,10 +455,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_strm_put(zram->comp, zstrm);
> if (is_partial_io(bvec))
> kfree(uncmem);
> -
Unnecessary new line
> if (ret)
> atomic64_inc(&zram->stats.failed_writes);
> return ret;
> @@ -522,6 +501,9 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> zs_free(meta->mem_pool, handle);
> }
>
> + zcomp_destroy(zram->comp);
> + zram->comp = NULL;
Just a nitpick:
Do we need to reset zram->comp to NULL?
> +
> zram_meta_free(zram->meta);
> zram->meta = NULL;
> /* Reset stats */
> @@ -550,9 +532,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");
Pz, add compressor name in info.
Okay, Now we have only lzo so anyone doesn't confuse but I'm afraid
that we forget to fix this when we add new compressor. :(
> + return -EINVAL;
> + }
> +
> disksize = PAGE_ALIGN(disksize);
> zram->meta = zram_meta_alloc(disksize);
> if (!zram->meta) {
> + zcomp_destroy(zram->comp);
> + zram->comp = NULL;
I'd like to know why we need to reset zram->comp to NULL.
Do you have in a mind?
> up_write(&zram->init_lock);
> return -ENOMEM;
> }
> @@ -790,6 +781,7 @@ static int create_device(struct zram *zram, int device_id)
> }
>
> zram->meta = NULL;
> + zram->comp = NULL;
Ditto.
> return 0;
>
> out_free_disk:
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 1d5b1f5..45e04f7 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 "zcomp.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 zcomp *comp;
> +
> /* Prevent concurrent execution of device init, reset and R/W request */
> struct rw_semaphore init_lock;
> /*
> --
Other than that,
Acked-by: Minchan Kim <minchan@kernel.org>
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCHv6 2/6] zram: use zcomp compressing backends
2014-02-24 1:01 ` Minchan Kim
@ 2014-02-24 8:39 ` Sergey Senozhatsky
2014-02-24 23:14 ` Minchan Kim
0 siblings, 1 reply; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-24 8:39 UTC (permalink / raw)
To: Minchan Kim; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On (02/24/14 10:01), Minchan Kim wrote:
> On Fri, Feb 21, 2014 at 02:50:39PM +0300, Sergey Senozhatsky wrote:
> > Do not perform direct LZO compress/decompress calls, initialise
> > and use zcomp LZO backend (single compression stream) 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, 32 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/block/zram/Makefile b/drivers/block/zram/Makefile
> > index cb0f9ce..757c6a5 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 zcomp.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..200c12a 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>
>
> In ARM,
>
> drivers/block/zram/zram_drv.c:161:2: error: implicit declaration of function 'vfree'
> drivers/block/zram/zram_drv.c:173:2: error: implicit declaration of function 'vzalloc'
>
will address, thanks.
> >
> > #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,8 +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;
> > - size_t clen = PAGE_SIZE;
> > + int ret = 0;
> > unsigned char *cmem;
> > struct zram_meta *meta = zram->meta;
> > unsigned long handle;
> > @@ -301,12 +281,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);
> > 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 +329,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 +354,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_strm *zstrm;
> > 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 +373,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> > goto out;
> > }
> >
> > - mutex_lock(&meta->buffer_lock);
> > + zstrm = zcomp_strm_get(zram->comp);
> > locked = true;
> > user_mem = kmap_atomic(page);
> >
> > @@ -420,19 +399,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, zstrm, uncmem, &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 = zstrm->buffer;
> > if (unlikely(clen > max_zpage_size)) {
> > clen = PAGE_SIZE;
> > src = NULL;
>
> Do we need this line? src = NULL?
not really.
> > @@ -457,6 +435,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> > memcpy(cmem, src, clen);
> > }
> >
> > + zcomp_strm_put(zram->comp, zstrm);
> > + locked = false;
> > zs_unmap_object(meta->mem_pool, handle);
> >
> > /*
> > @@ -475,10 +455,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_strm_put(zram->comp, zstrm);
> > if (is_partial_io(bvec))
> > kfree(uncmem);
> > -
>
> Unnecessary new line
>
ok.
> > if (ret)
> > atomic64_inc(&zram->stats.failed_writes);
> > return ret;
> > @@ -522,6 +501,9 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> > zs_free(meta->mem_pool, handle);
> > }
> >
> > + zcomp_destroy(zram->comp);
> > + zram->comp = NULL;
>
> Just a nitpick:
> Do we need to reset zram->comp to NULL?
>
well. not at this time, makes sense with when multi backend support.
initialise zcomp to default compressor if it still un-initialised via
compressor device attribute.
> > +
> > zram_meta_free(zram->meta);
> > zram->meta = NULL;
> > /* Reset stats */
> > @@ -550,9 +532,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");
>
> Pz, add compressor name in info.
>
> Okay, Now we have only lzo so anyone doesn't confuse but I'm afraid
> that we forget to fix this when we add new compressor. :(
>
ok.
> > + return -EINVAL;
> > + }
> > +
> > disksize = PAGE_ALIGN(disksize);
> > zram->meta = zram_meta_alloc(disksize);
> > if (!zram->meta) {
> > + zcomp_destroy(zram->comp);
> > + zram->comp = NULL;
>
> I'd like to know why we need to reset zram->comp to NULL.
> Do you have in a mind?
>
same as above. makes sense with multi backend support.
> > up_write(&zram->init_lock);
> > return -ENOMEM;
> > }
> > @@ -790,6 +781,7 @@ static int create_device(struct zram *zram, int device_id)
> > }
> >
> > zram->meta = NULL;
> > + zram->comp = NULL;
>
> Ditto.
>
> > return 0;
> >
> > out_free_disk:
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index 1d5b1f5..45e04f7 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 "zcomp.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 zcomp *comp;
> > +
> > /* Prevent concurrent execution of device init, reset and R/W request */
> > struct rw_semaphore init_lock;
> > /*
> > --
>
> Other than that,
> Acked-by: Minchan Kim <minchan@kernel.org>
>
> --
> Kind regards,
> Minchan Kim
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCHv6 2/6] zram: use zcomp compressing backends
2014-02-24 8:39 ` Sergey Senozhatsky
@ 2014-02-24 23:14 ` Minchan Kim
0 siblings, 0 replies; 23+ messages in thread
From: Minchan Kim @ 2014-02-24 23:14 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On Mon, Feb 24, 2014 at 11:39:50AM +0300, Sergey Senozhatsky wrote:
> On (02/24/14 10:01), Minchan Kim wrote:
> > On Fri, Feb 21, 2014 at 02:50:39PM +0300, Sergey Senozhatsky wrote:
> > > Do not perform direct LZO compress/decompress calls, initialise
> > > and use zcomp LZO backend (single compression stream) 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, 32 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/drivers/block/zram/Makefile b/drivers/block/zram/Makefile
> > > index cb0f9ce..757c6a5 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 zcomp.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..200c12a 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>
> >
> > In ARM,
> >
> > drivers/block/zram/zram_drv.c:161:2: error: implicit declaration of function 'vfree'
> > drivers/block/zram/zram_drv.c:173:2: error: implicit declaration of function 'vzalloc'
> >
>
> will address, thanks.
>
> > >
> > > #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,8 +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;
> > > - size_t clen = PAGE_SIZE;
> > > + int ret = 0;
> > > unsigned char *cmem;
> > > struct zram_meta *meta = zram->meta;
> > > unsigned long handle;
> > > @@ -301,12 +281,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);
> > > 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 +329,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 +354,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_strm *zstrm;
> > > 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 +373,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> > > goto out;
> > > }
> > >
> > > - mutex_lock(&meta->buffer_lock);
> > > + zstrm = zcomp_strm_get(zram->comp);
> > > locked = true;
> > > user_mem = kmap_atomic(page);
> > >
> > > @@ -420,19 +399,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, zstrm, uncmem, &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 = zstrm->buffer;
> > > if (unlikely(clen > max_zpage_size)) {
> > > clen = PAGE_SIZE;
> > > src = NULL;
> >
> > Do we need this line? src = NULL?
>
> not really.
>
> > > @@ -457,6 +435,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> > > memcpy(cmem, src, clen);
> > > }
> > >
> > > + zcomp_strm_put(zram->comp, zstrm);
> > > + locked = false;
> > > zs_unmap_object(meta->mem_pool, handle);
> > >
> > > /*
> > > @@ -475,10 +455,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_strm_put(zram->comp, zstrm);
> > > if (is_partial_io(bvec))
> > > kfree(uncmem);
> > > -
> >
> > Unnecessary new line
> >
>
> ok.
>
> > > if (ret)
> > > atomic64_inc(&zram->stats.failed_writes);
> > > return ret;
> > > @@ -522,6 +501,9 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> > > zs_free(meta->mem_pool, handle);
> > > }
> > >
> > > + zcomp_destroy(zram->comp);
> > > + zram->comp = NULL;
> >
> > Just a nitpick:
> > Do we need to reset zram->comp to NULL?
> >
>
> well. not at this time, makes sense with when multi backend support.
> initialise zcomp to default compressor if it still un-initialised via
> compressor device attribute.
Okay, then let's fix this and adding compressor name I suggested in
below error message when we start to support multi backend.
Let's keep in mind. :)
>
> > > +
> > > zram_meta_free(zram->meta);
> > > zram->meta = NULL;
> > > /* Reset stats */
> > > @@ -550,9 +532,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");
> >
> > Pz, add compressor name in info.
> >
> > Okay, Now we have only lzo so anyone doesn't confuse but I'm afraid
> > that we forget to fix this when we add new compressor. :(
> >
>
> ok.
>
> > > + return -EINVAL;
> > > + }
> > > +
> > > disksize = PAGE_ALIGN(disksize);
> > > zram->meta = zram_meta_alloc(disksize);
> > > if (!zram->meta) {
> > > + zcomp_destroy(zram->comp);
> > > + zram->comp = NULL;
> >
> > I'd like to know why we need to reset zram->comp to NULL.
> > Do you have in a mind?
> >
>
> same as above. makes sense with multi backend support.
>
> > > up_write(&zram->init_lock);
> > > return -ENOMEM;
> > > }
> > > @@ -790,6 +781,7 @@ static int create_device(struct zram *zram, int device_id)
> > > }
> > >
> > > zram->meta = NULL;
> > > + zram->comp = NULL;
> >
> > Ditto.
> >
> > > return 0;
> > >
> > > out_free_disk:
> > > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > > index 1d5b1f5..45e04f7 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 "zcomp.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 zcomp *comp;
> > > +
> > > /* Prevent concurrent execution of device init, reset and R/W request */
> > > struct rw_semaphore init_lock;
> > > /*
> > > --
> >
> > Other than that,
> > Acked-by: Minchan Kim <minchan@kernel.org>
> >
> > --
> > Kind regards,
> > Minchan Kim
> >
> --
> 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] 23+ messages in thread
* [PATCHv6 3/6] zram: factor out single stream compression
2014-02-21 11:50 [PATCHv6 0/6] add compressing abstraction and multi stream support Sergey Senozhatsky
2014-02-21 11:50 ` [PATCHv6 1/6] zram: introduce compressing backend abstraction Sergey Senozhatsky
2014-02-21 11:50 ` [PATCHv6 2/6] zram: use zcomp compressing backends Sergey Senozhatsky
@ 2014-02-21 11:50 ` Sergey Senozhatsky
2014-02-24 2:31 ` Minchan Kim
2014-02-21 11:50 ` [PATCHv6 4/6] zram: add multi stream functionality Sergey Senozhatsky
` (2 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-21 11:50 UTC (permalink / raw)
To: Minchan Kim
Cc: Jerome Marchand, Nitin Gupta, linux-kernel, Sergey Senozhatsky
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>
---
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);
};
struct zcomp *zcomp_create(const char *comp);
--
1.9.0.291.g027825b
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCHv6 3/6] zram: factor out single stream compression
2014-02-21 11:50 ` [PATCHv6 3/6] zram: factor out single stream compression Sergey Senozhatsky
@ 2014-02-24 2:31 ` Minchan Kim
2014-02-24 8:31 ` Sergey Senozhatsky
0 siblings, 1 reply; 23+ messages in thread
From: Minchan Kim @ 2014-02-24 2:31 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCHv6 3/6] zram: factor out single stream compression
2014-02-24 2:31 ` Minchan Kim
@ 2014-02-24 8:31 ` Sergey Senozhatsky
2014-02-24 23:07 ` Minchan Kim
0 siblings, 1 reply; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-24 8:31 UTC (permalink / raw)
To: Minchan Kim; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
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.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.
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
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCHv6 3/6] zram: factor out single stream compression
2014-02-24 8:31 ` Sergey Senozhatsky
@ 2014-02-24 23:07 ` Minchan Kim
2014-02-24 23:14 ` Sergey Senozhatsky
0 siblings, 1 reply; 23+ messages in thread
From: Minchan Kim @ 2014-02-24 23:07 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
Hello Sergey,
On Mon, Feb 24, 2014 at 11:31:52AM +0300, Sergey Senozhatsky wrote:
> 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.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.
>
> 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?
Sounds good to me. That was from my laziness that I just didn't read your
entire patchset. Then, I think we don't need to separate it with new CONFIG
option(and I know you wanted ;-) ) because it just increase small memory
footprint but it could remove maintainace headache and confusing from zram
users.
add/remove: 4/0 grow/shrink: 2/0 up/down: 772/0 (772)
function old new delta
zcomp_strm_multi_get - 189 +189
max_comp_streams_store 14 155 +141
zcomp_strm_alloc - 127 +127
zcomp_create 313 439 +126
zcomp_strm_multi_put - 95 +95
zcomp_strm_multi_destroy - 94 +94
So, let's remove new CONFIG and go with only option but it would work
with mutex if stream is only one so there would be no regression but
a little code size overhead but it's good deal, IMO.
I will review other patches in v6.
Thanks.
>
> >
> > > ---
> > > 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 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] 23+ messages in thread* Re: [PATCHv6 3/6] zram: factor out single stream compression
2014-02-24 23:07 ` Minchan Kim
@ 2014-02-24 23:14 ` Sergey Senozhatsky
0 siblings, 0 replies; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-24 23:14 UTC (permalink / raw)
To: Minchan Kim; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
Good morning Minchan,
On (02/25/14 08:07), Minchan Kim wrote:
> Hello Sergey,
>
> On Mon, Feb 24, 2014 at 11:31:52AM +0300, Sergey Senozhatsky wrote:
> > 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.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.
> >
> > 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?
>
> Sounds good to me. That was from my laziness that I just didn't read your
> entire patchset. Then, I think we don't need to separate it with new CONFIG
> option(and I know you wanted ;-) ) because it just increase small memory
> footprint but it could remove maintainace headache and confusing from zram
> users.
>
> add/remove: 4/0 grow/shrink: 2/0 up/down: 772/0 (772)
> function old new delta
> zcomp_strm_multi_get - 189 +189
> max_comp_streams_store 14 155 +141
> zcomp_strm_alloc - 127 +127
> zcomp_create 313 439 +126
> zcomp_strm_multi_put - 95 +95
> zcomp_strm_multi_destroy - 94 +94
>
> So, let's remove new CONFIG and go with only option but it would work
> with mutex if stream is only one so there would be no regression but
> a little code size overhead but it's good deal, IMO.
ok, sounds good. will remove CONFIG option and make zram both single and
multi stream ready out of the box (max_comp_streams == 1 -- single stream and
mutex-based locking, max_comp_streams > 1 -- multi stream, wait queue and spin
lock based locking). will publish patches as soon as possible.
> I will review other patches in v6.
>
> Thanks.
>
thank you.
-ss
> >
> > >
> > > > ---
> > > > 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 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] 23+ messages in thread
* [PATCHv6 4/6] zram: add multi stream functionality
2014-02-21 11:50 [PATCHv6 0/6] add compressing abstraction and multi stream support Sergey Senozhatsky
` (2 preceding siblings ...)
2014-02-21 11:50 ` [PATCHv6 3/6] zram: factor out single stream compression Sergey Senozhatsky
@ 2014-02-21 11:50 ` Sergey Senozhatsky
2014-02-24 23:43 ` Minchan Kim
2014-02-21 11:50 ` [PATCHv6 5/6] zram: enalbe multi stream compression support in zram Sergey Senozhatsky
2014-02-21 11:50 ` [PATCHv6 6/6] zram: document max_comp_streams Sergey Senozhatsky
5 siblings, 1 reply; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-21 11:50 UTC (permalink / raw)
To: Minchan Kim
Cc: Jerome Marchand, Nitin Gupta, linux-kernel, Sergey Senozhatsky
This patch implements multi stream compression support.
Introduce struct zcomp_strm_multi and a set of functions to manage
zcomp_strm stream access. zcomp_strm_multi has a list of idle zcomp_strm
structs, spinlock to protect idle list and wait queue, making it possible
to perform parallel compressions.
The following set of functions added:
- zcomp_strm_multi_get()/zcomp_strm_multi_put()
get and put compression stream, implement required locking
- zcomp_strm_multi_create()/zcomp_strm_multi_destroy()
create and destroy zcomp_strm_multi
zcomp ->strm_get() and ->strm_put() callbacks are set during initialisation
to zcomp_strm_multi_get()/zcomp_strm_multi_put() correspondingly.
Each time zcomp issues a zcomp_strm_multi_get() call, the following set of
operations performed:
- spin lock strm_lock
- if idle list is not empty, remove zcomp_strm from idle list, spin
unlock and return zcomp stream pointer to caller
- if idle list is empty, current adds itself to wait queue. it will be
awaken by zcomp_strm_multi_put() caller.
zcomp_strm_multi_put():
- spin lock strm_lock
- add zcomp stream to idle list
- spin unlock, wake up sleeper
Minchan Kim reported that spinlock-based locking scheme has demonstrated a
severe perfomance regression for single compression stream case, comparing
to mutex-based (https://lkml.org/lkml/2014/2/18/16)
base spinlock mutex
==Initial write ==Initial write ==Initial write
records: 5 records: 5 records: 5
avg: 1642424.35 avg: 699610.40 avg: 1655583.71
std: 39890.95(2.43%) std: 232014.19(33.16%) std: 52293.96
max: 1690170.94 max: 1163473.45 max: 1697164.75
min: 1568669.52 min: 573429.88 min: 1553410.23
==Rewrite ==Rewrite ==Rewrite
records: 5 records: 5 records: 5
avg: 1611775.39 avg: 501406.64 avg: 1684419.11
std: 17144.58(1.06%) std: 15354.41(3.06%) std: 18367.42
max: 1641800.95 max: 531356.78 max: 1706445.84
min: 1593515.27 min: 488817.78 min: 1655335.73
==Random write ==Random write ==Random write
records: 5 records: 5 records: 5
avg: 1626318.29 avg: 497250.78 avg: 1695582.06
std: 38550.23(2.37%) std: 1405.42(0.28%) std: 9211.98
max: 1665839.62 max: 498585.88 max: 1703808.22
min: 1562141.21 min: 494526.45 min: 1677664.94
==Pwrite ==Pwrite ==Pwrite
records: 5 records: 5 records: 5
avg: 1654641.25 avg: 581709.22 avg: 1641452.34
std: 47202.59(2.85%) std: 9670.46(1.66%) std: 38963.62
max: 1740682.36 max: 591300.09 max: 1687387.69
min: 1611436.34 min: 564324.38 min: 1570496.11
When only one compression stream available, mutex with spin on owner tends
to perform much better than frequent wait_event()/wake_up(). This is why
single stream implemented as a special case with mutex locking.
In order to compile this functionality ZRAM_MULTI_STREAM config option
required to be set, which will be introduced later.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/block/zram/zcomp.c | 101 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 100 insertions(+), 1 deletion(-)
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 9661226..41325ed 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -12,9 +12,14 @@
#include <linux/slab.h>
#include <linux/wait.h>
#include <linux/sched.h>
+#ifdef CONFIG_ZRAM_MULTI_STREAM
+#include <linux/spinlock.h>
+#endif
#include "zcomp.h"
+extern struct zcomp_backend zcomp_lzo;
+
/*
* single zcomp_strm backend private part
*/
@@ -23,7 +28,18 @@ struct zcomp_strm_single {
struct zcomp_strm *zstrm;
};
-extern struct zcomp_backend zcomp_lzo;
+#ifdef CONFIG_ZRAM_MULTI_STREAM
+/*
+ * multi zcomp_strm backend private part
+ */
+struct zcomp_strm_multi {
+ /* protect strm list */
+ spinlock_t strm_lock;
+ /* list of available strms */
+ struct list_head idle_strm;
+ wait_queue_head_t strm_wait;
+};
+#endif
static struct zcomp_backend *find_backend(const char *compress)
{
@@ -63,6 +79,89 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
return zstrm;
}
+#ifdef CONFIG_ZRAM_MULTI_STREAM
+/*
+ * get existing idle zcomp_strm or wait until other process release
+ * (zcomp_strm_put()) one for us
+ */
+static struct zcomp_strm *zcomp_strm_multi_get(struct zcomp *comp)
+{
+ struct zcomp_strm_multi *zp = comp->private;
+ struct zcomp_strm *zstrm;
+
+ while (1) {
+ spin_lock(&zp->strm_lock);
+ if (list_empty(&zp->idle_strm)) {
+ spin_unlock(&zp->strm_lock);
+ wait_event(zp->strm_wait,
+ !list_empty(&zp->idle_strm));
+ continue;
+ }
+
+ zstrm = list_entry(zp->idle_strm.next,
+ struct zcomp_strm, list);
+ list_del(&zstrm->list);
+ spin_unlock(&zp->strm_lock);
+ break;
+ }
+ return zstrm;
+}
+
+/* add zcomp_strm back to idle list and wake up waiter */
+static void zcomp_strm_multi_put(struct zcomp *comp, struct zcomp_strm *zstrm)
+{
+ struct zcomp_strm_multi *zp = comp->private;
+
+ spin_lock(&zp->strm_lock);
+ list_add(&zstrm->list, &zp->idle_strm);
+ spin_unlock(&zp->strm_lock);
+
+ wake_up(&zp->strm_wait);
+}
+
+static void zcomp_strm_multi_destroy(struct zcomp *comp)
+{
+ struct zcomp_strm_multi *zp = comp->private;
+ struct zcomp_strm *zstrm;
+ while (!list_empty(&zp->idle_strm)) {
+ zstrm = list_entry(zp->idle_strm.next,
+ struct zcomp_strm, list);
+ list_del(&zstrm->list);
+ zcomp_strm_free(comp, zstrm);
+ }
+ kfree(zp);
+}
+
+static int zcomp_strm_multi_create(struct zcomp *comp, int num_strm)
+{
+ int i;
+ struct zcomp_strm *zstrm;
+ struct zcomp_strm_multi *zp;
+
+ comp->destroy = zcomp_strm_multi_destroy;
+ comp->strm_get = zcomp_strm_multi_get;
+ comp->strm_put = zcomp_strm_multi_put;
+ zp = kmalloc(sizeof(struct zcomp_strm_multi), GFP_KERNEL);
+ comp->private = zp;
+ if (!zp)
+ return -ENOMEM;
+
+ spin_lock_init(&zp->strm_lock);
+ INIT_LIST_HEAD(&zp->idle_strm);
+ init_waitqueue_head(&zp->strm_wait);
+
+ for (i = 0; i < num_strm; i++) {
+ zstrm = zcomp_strm_alloc(comp);
+ if (!zstrm) {
+ zcomp_strm_multi_destroy(comp);
+ return -ENOMEM;
+ }
+ list_add(&zstrm->list, &zp->idle_strm);
+ }
+ return 0;
+}
+#endif
+
static struct zcomp_strm *zcomp_strm_single_get(struct zcomp *comp)
{
struct zcomp_strm_single *zp = comp->private;
--
1.9.0.291.g027825b
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCHv6 4/6] zram: add multi stream functionality
2014-02-21 11:50 ` [PATCHv6 4/6] zram: add multi stream functionality Sergey Senozhatsky
@ 2014-02-24 23:43 ` Minchan Kim
2014-02-24 23:48 ` Sergey Senozhatsky
2014-02-24 23:51 ` Minchan Kim
0 siblings, 2 replies; 23+ messages in thread
From: Minchan Kim @ 2014-02-24 23:43 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On Fri, Feb 21, 2014 at 02:50:41PM +0300, Sergey Senozhatsky wrote:
> This patch implements multi stream compression support.
>
> Introduce struct zcomp_strm_multi and a set of functions to manage
> zcomp_strm stream access. zcomp_strm_multi has a list of idle zcomp_strm
> structs, spinlock to protect idle list and wait queue, making it possible
> to perform parallel compressions.
>
> The following set of functions added:
> - zcomp_strm_multi_get()/zcomp_strm_multi_put()
> get and put compression stream, implement required locking
> - zcomp_strm_multi_create()/zcomp_strm_multi_destroy()
> create and destroy zcomp_strm_multi
>
> zcomp ->strm_get() and ->strm_put() callbacks are set during initialisation
> to zcomp_strm_multi_get()/zcomp_strm_multi_put() correspondingly.
>
> Each time zcomp issues a zcomp_strm_multi_get() call, the following set of
> operations performed:
> - spin lock strm_lock
> - if idle list is not empty, remove zcomp_strm from idle list, spin
> unlock and return zcomp stream pointer to caller
> - if idle list is empty, current adds itself to wait queue. it will be
> awaken by zcomp_strm_multi_put() caller.
>
> zcomp_strm_multi_put():
> - spin lock strm_lock
> - add zcomp stream to idle list
> - spin unlock, wake up sleeper
>
> Minchan Kim reported that spinlock-based locking scheme has demonstrated a
> severe perfomance regression for single compression stream case, comparing
> to mutex-based (https://lkml.org/lkml/2014/2/18/16)
>
> base spinlock mutex
>
> ==Initial write ==Initial write ==Initial write
> records: 5 records: 5 records: 5
> avg: 1642424.35 avg: 699610.40 avg: 1655583.71
> std: 39890.95(2.43%) std: 232014.19(33.16%) std: 52293.96
> max: 1690170.94 max: 1163473.45 max: 1697164.75
> min: 1568669.52 min: 573429.88 min: 1553410.23
> ==Rewrite ==Rewrite ==Rewrite
> records: 5 records: 5 records: 5
> avg: 1611775.39 avg: 501406.64 avg: 1684419.11
> std: 17144.58(1.06%) std: 15354.41(3.06%) std: 18367.42
> max: 1641800.95 max: 531356.78 max: 1706445.84
> min: 1593515.27 min: 488817.78 min: 1655335.73
> ==Random write ==Random write ==Random write
> records: 5 records: 5 records: 5
> avg: 1626318.29 avg: 497250.78 avg: 1695582.06
> std: 38550.23(2.37%) std: 1405.42(0.28%) std: 9211.98
> max: 1665839.62 max: 498585.88 max: 1703808.22
> min: 1562141.21 min: 494526.45 min: 1677664.94
> ==Pwrite ==Pwrite ==Pwrite
> records: 5 records: 5 records: 5
> avg: 1654641.25 avg: 581709.22 avg: 1641452.34
> std: 47202.59(2.85%) std: 9670.46(1.66%) std: 38963.62
> max: 1740682.36 max: 591300.09 max: 1687387.69
> min: 1611436.34 min: 564324.38 min: 1570496.11
>
> When only one compression stream available, mutex with spin on owner tends
> to perform much better than frequent wait_event()/wake_up(). This is why
> single stream implemented as a special case with mutex locking.
Side note:
It's true for x86 but it could be changed in embedded system like ARM
which is lower power compared to x86 and it also could depend on
compression algorithm speed so I am thinking we need adaptive locking
which sometime work with mutex sometime work with spinlock dynamically.
Anyway, it is further work and I should investigate more.
Most important thing is that this patchset wouldn't make any regression
if we use mutex for single stream as default so I'm okay now.
>
> In order to compile this functionality ZRAM_MULTI_STREAM config option
> required to be set, which will be introduced later.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> drivers/block/zram/zcomp.c | 101 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 100 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 9661226..41325ed 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -12,9 +12,14 @@
> #include <linux/slab.h>
> #include <linux/wait.h>
> #include <linux/sched.h>
> +#ifdef CONFIG_ZRAM_MULTI_STREAM
> +#include <linux/spinlock.h>
> +#endif
Do we really need to include spinlock.h?
>
> #include "zcomp.h"
>
> +extern struct zcomp_backend zcomp_lzo;
> +
> /*
> * single zcomp_strm backend private part
> */
> @@ -23,7 +28,18 @@ struct zcomp_strm_single {
> struct zcomp_strm *zstrm;
> };
>
> -extern struct zcomp_backend zcomp_lzo;
> +#ifdef CONFIG_ZRAM_MULTI_STREAM
> +/*
> + * multi zcomp_strm backend private part
> + */
> +struct zcomp_strm_multi {
> + /* protect strm list */
> + spinlock_t strm_lock;
> + /* list of available strms */
> + struct list_head idle_strm;
> + wait_queue_head_t strm_wait;
> +};
> +#endif
>
> static struct zcomp_backend *find_backend(const char *compress)
> {
> @@ -63,6 +79,89 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
> return zstrm;
> }
>
> +#ifdef CONFIG_ZRAM_MULTI_STREAM
> +/*
> + * get existing idle zcomp_strm or wait until other process release
> + * (zcomp_strm_put()) one for us
> + */
> +static struct zcomp_strm *zcomp_strm_multi_get(struct zcomp *comp)
> +{
> + struct zcomp_strm_multi *zp = comp->private;
How about comp->stream instead of private?
Other than that, Looks good to me.
> + struct zcomp_strm *zstrm;
> +
> + while (1) {
> + spin_lock(&zp->strm_lock);
> + if (list_empty(&zp->idle_strm)) {
> + spin_unlock(&zp->strm_lock);
> + wait_event(zp->strm_wait,
> + !list_empty(&zp->idle_strm));
> + continue;
> + }
> +
> + zstrm = list_entry(zp->idle_strm.next,
> + struct zcomp_strm, list);
> + list_del(&zstrm->list);
> + spin_unlock(&zp->strm_lock);
> + break;
> + }
> + return zstrm;
> +}
> +
> +/* add zcomp_strm back to idle list and wake up waiter */
> +static void zcomp_strm_multi_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> +{
> + struct zcomp_strm_multi *zp = comp->private;
> +
> + spin_lock(&zp->strm_lock);
> + list_add(&zstrm->list, &zp->idle_strm);
> + spin_unlock(&zp->strm_lock);
> +
> + wake_up(&zp->strm_wait);
> +}
> +
> +static void zcomp_strm_multi_destroy(struct zcomp *comp)
> +{
> + struct zcomp_strm_multi *zp = comp->private;
> + struct zcomp_strm *zstrm;
> + while (!list_empty(&zp->idle_strm)) {
> + zstrm = list_entry(zp->idle_strm.next,
> + struct zcomp_strm, list);
> + list_del(&zstrm->list);
> + zcomp_strm_free(comp, zstrm);
> + }
> + kfree(zp);
> +}
> +
> +static int zcomp_strm_multi_create(struct zcomp *comp, int num_strm)
> +{
> + int i;
> + struct zcomp_strm *zstrm;
> + struct zcomp_strm_multi *zp;
> +
> + comp->destroy = zcomp_strm_multi_destroy;
> + comp->strm_get = zcomp_strm_multi_get;
> + comp->strm_put = zcomp_strm_multi_put;
> + zp = kmalloc(sizeof(struct zcomp_strm_multi), GFP_KERNEL);
> + comp->private = zp;
> + if (!zp)
> + return -ENOMEM;
> +
> + spin_lock_init(&zp->strm_lock);
> + INIT_LIST_HEAD(&zp->idle_strm);
> + init_waitqueue_head(&zp->strm_wait);
> +
> + for (i = 0; i < num_strm; i++) {
> + zstrm = zcomp_strm_alloc(comp);
> + if (!zstrm) {
> + zcomp_strm_multi_destroy(comp);
> + return -ENOMEM;
> + }
> + list_add(&zstrm->list, &zp->idle_strm);
> + }
> + return 0;
> +}
> +#endif
> +
> static struct zcomp_strm *zcomp_strm_single_get(struct zcomp *comp)
> {
> struct zcomp_strm_single *zp = comp->private;
> --
> 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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCHv6 4/6] zram: add multi stream functionality
2014-02-24 23:43 ` Minchan Kim
@ 2014-02-24 23:48 ` Sergey Senozhatsky
2014-02-24 23:51 ` Minchan Kim
1 sibling, 0 replies; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-24 23:48 UTC (permalink / raw)
To: Minchan Kim; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On (02/25/14 08:43), Minchan Kim wrote:
> On Fri, Feb 21, 2014 at 02:50:41PM +0300, Sergey Senozhatsky wrote:
> > This patch implements multi stream compression support.
> >
> > Introduce struct zcomp_strm_multi and a set of functions to manage
> > zcomp_strm stream access. zcomp_strm_multi has a list of idle zcomp_strm
> > structs, spinlock to protect idle list and wait queue, making it possible
> > to perform parallel compressions.
> >
> > The following set of functions added:
> > - zcomp_strm_multi_get()/zcomp_strm_multi_put()
> > get and put compression stream, implement required locking
> > - zcomp_strm_multi_create()/zcomp_strm_multi_destroy()
> > create and destroy zcomp_strm_multi
> >
> > zcomp ->strm_get() and ->strm_put() callbacks are set during initialisation
> > to zcomp_strm_multi_get()/zcomp_strm_multi_put() correspondingly.
> >
> > Each time zcomp issues a zcomp_strm_multi_get() call, the following set of
> > operations performed:
> > - spin lock strm_lock
> > - if idle list is not empty, remove zcomp_strm from idle list, spin
> > unlock and return zcomp stream pointer to caller
> > - if idle list is empty, current adds itself to wait queue. it will be
> > awaken by zcomp_strm_multi_put() caller.
> >
> > zcomp_strm_multi_put():
> > - spin lock strm_lock
> > - add zcomp stream to idle list
> > - spin unlock, wake up sleeper
> >
> > Minchan Kim reported that spinlock-based locking scheme has demonstrated a
> > severe perfomance regression for single compression stream case, comparing
> > to mutex-based (https://lkml.org/lkml/2014/2/18/16)
> >
> > base spinlock mutex
> >
> > ==Initial write ==Initial write ==Initial write
> > records: 5 records: 5 records: 5
> > avg: 1642424.35 avg: 699610.40 avg: 1655583.71
> > std: 39890.95(2.43%) std: 232014.19(33.16%) std: 52293.96
> > max: 1690170.94 max: 1163473.45 max: 1697164.75
> > min: 1568669.52 min: 573429.88 min: 1553410.23
> > ==Rewrite ==Rewrite ==Rewrite
> > records: 5 records: 5 records: 5
> > avg: 1611775.39 avg: 501406.64 avg: 1684419.11
> > std: 17144.58(1.06%) std: 15354.41(3.06%) std: 18367.42
> > max: 1641800.95 max: 531356.78 max: 1706445.84
> > min: 1593515.27 min: 488817.78 min: 1655335.73
> > ==Random write ==Random write ==Random write
> > records: 5 records: 5 records: 5
> > avg: 1626318.29 avg: 497250.78 avg: 1695582.06
> > std: 38550.23(2.37%) std: 1405.42(0.28%) std: 9211.98
> > max: 1665839.62 max: 498585.88 max: 1703808.22
> > min: 1562141.21 min: 494526.45 min: 1677664.94
> > ==Pwrite ==Pwrite ==Pwrite
> > records: 5 records: 5 records: 5
> > avg: 1654641.25 avg: 581709.22 avg: 1641452.34
> > std: 47202.59(2.85%) std: 9670.46(1.66%) std: 38963.62
> > max: 1740682.36 max: 591300.09 max: 1687387.69
> > min: 1611436.34 min: 564324.38 min: 1570496.11
> >
> > When only one compression stream available, mutex with spin on owner tends
> > to perform much better than frequent wait_event()/wake_up(). This is why
> > single stream implemented as a special case with mutex locking.
>
> Side note:
>
> It's true for x86 but it could be changed in embedded system like ARM
> which is lower power compared to x86 and it also could depend on
> compression algorithm speed so I am thinking we need adaptive locking
> which sometime work with mutex sometime work with spinlock dynamically.
> Anyway, it is further work and I should investigate more.
>
> Most important thing is that this patchset wouldn't make any regression
> if we use mutex for single stream as default so I'm okay now.
>
>
good note.
> >
> > In order to compile this functionality ZRAM_MULTI_STREAM config option
> > required to be set, which will be introduced later.
> >
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> > drivers/block/zram/zcomp.c | 101 ++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 100 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> > index 9661226..41325ed 100644
> > --- a/drivers/block/zram/zcomp.c
> > +++ b/drivers/block/zram/zcomp.c
> > @@ -12,9 +12,14 @@
> > #include <linux/slab.h>
> > #include <linux/wait.h>
> > #include <linux/sched.h>
> > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > +#include <linux/spinlock.h>
> > +#endif
>
> Do we really need to include spinlock.h?
not really. and I'm also droping CONFIG_ZRAM_MULTI_STREAM
ifdefs.
> >
> > #include "zcomp.h"
> >
> > +extern struct zcomp_backend zcomp_lzo;
> > +
> > /*
> > * single zcomp_strm backend private part
> > */
> > @@ -23,7 +28,18 @@ struct zcomp_strm_single {
> > struct zcomp_strm *zstrm;
> > };
> >
> > -extern struct zcomp_backend zcomp_lzo;
> > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > +/*
> > + * multi zcomp_strm backend private part
> > + */
> > +struct zcomp_strm_multi {
> > + /* protect strm list */
> > + spinlock_t strm_lock;
> > + /* list of available strms */
> > + struct list_head idle_strm;
> > + wait_queue_head_t strm_wait;
> > +};
> > +#endif
> >
> > static struct zcomp_backend *find_backend(const char *compress)
> > {
> > @@ -63,6 +79,89 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
> > return zstrm;
> > }
> >
> > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > +/*
> > + * get existing idle zcomp_strm or wait until other process release
> > + * (zcomp_strm_put()) one for us
> > + */
> > +static struct zcomp_strm *zcomp_strm_multi_get(struct zcomp *comp)
> > +{
> > + struct zcomp_strm_multi *zp = comp->private;
>
> How about comp->stream instead of private?
ok. so I'll rename zcomp private in this and previous (factor out single
stream) patch
> Other than that, Looks good to me.
>
> > + struct zcomp_strm *zstrm;
> > +
> > + while (1) {
> > + spin_lock(&zp->strm_lock);
> > + if (list_empty(&zp->idle_strm)) {
> > + spin_unlock(&zp->strm_lock);
> > + wait_event(zp->strm_wait,
> > + !list_empty(&zp->idle_strm));
> > + continue;
> > + }
> > +
> > + zstrm = list_entry(zp->idle_strm.next,
> > + struct zcomp_strm, list);
> > + list_del(&zstrm->list);
> > + spin_unlock(&zp->strm_lock);
> > + break;
> > + }
> > + return zstrm;
> > +}
> > +
> > +/* add zcomp_strm back to idle list and wake up waiter */
> > +static void zcomp_strm_multi_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> > +{
> > + struct zcomp_strm_multi *zp = comp->private;
> > +
> > + spin_lock(&zp->strm_lock);
> > + list_add(&zstrm->list, &zp->idle_strm);
> > + spin_unlock(&zp->strm_lock);
> > +
> > + wake_up(&zp->strm_wait);
> > +}
> > +
> > +static void zcomp_strm_multi_destroy(struct zcomp *comp)
> > +{
> > + struct zcomp_strm_multi *zp = comp->private;
> > + struct zcomp_strm *zstrm;
> > + while (!list_empty(&zp->idle_strm)) {
> > + zstrm = list_entry(zp->idle_strm.next,
> > + struct zcomp_strm, list);
> > + list_del(&zstrm->list);
> > + zcomp_strm_free(comp, zstrm);
> > + }
> > + kfree(zp);
> > +}
> > +
> > +static int zcomp_strm_multi_create(struct zcomp *comp, int num_strm)
> > +{
> > + int i;
> > + struct zcomp_strm *zstrm;
> > + struct zcomp_strm_multi *zp;
> > +
> > + comp->destroy = zcomp_strm_multi_destroy;
> > + comp->strm_get = zcomp_strm_multi_get;
> > + comp->strm_put = zcomp_strm_multi_put;
> > + zp = kmalloc(sizeof(struct zcomp_strm_multi), GFP_KERNEL);
> > + comp->private = zp;
> > + if (!zp)
> > + return -ENOMEM;
> > +
> > + spin_lock_init(&zp->strm_lock);
> > + INIT_LIST_HEAD(&zp->idle_strm);
> > + init_waitqueue_head(&zp->strm_wait);
> > +
> > + for (i = 0; i < num_strm; i++) {
> > + zstrm = zcomp_strm_alloc(comp);
> > + if (!zstrm) {
> > + zcomp_strm_multi_destroy(comp);
> > + return -ENOMEM;
> > + }
> > + list_add(&zstrm->list, &zp->idle_strm);
> > + }
> > + return 0;
> > +}
> > +#endif
> > +
> > static struct zcomp_strm *zcomp_strm_single_get(struct zcomp *comp)
> > {
> > struct zcomp_strm_single *zp = comp->private;
> > --
> > 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
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCHv6 4/6] zram: add multi stream functionality
2014-02-24 23:43 ` Minchan Kim
2014-02-24 23:48 ` Sergey Senozhatsky
@ 2014-02-24 23:51 ` Minchan Kim
1 sibling, 0 replies; 23+ messages in thread
From: Minchan Kim @ 2014-02-24 23:51 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On Tue, Feb 25, 2014 at 08:43:13AM +0900, Minchan Kim wrote:
> On Fri, Feb 21, 2014 at 02:50:41PM +0300, Sergey Senozhatsky wrote:
> > This patch implements multi stream compression support.
> >
> > Introduce struct zcomp_strm_multi and a set of functions to manage
> > zcomp_strm stream access. zcomp_strm_multi has a list of idle zcomp_strm
> > structs, spinlock to protect idle list and wait queue, making it possible
> > to perform parallel compressions.
> >
> > The following set of functions added:
> > - zcomp_strm_multi_get()/zcomp_strm_multi_put()
> > get and put compression stream, implement required locking
> > - zcomp_strm_multi_create()/zcomp_strm_multi_destroy()
> > create and destroy zcomp_strm_multi
> >
> > zcomp ->strm_get() and ->strm_put() callbacks are set during initialisation
> > to zcomp_strm_multi_get()/zcomp_strm_multi_put() correspondingly.
> >
> > Each time zcomp issues a zcomp_strm_multi_get() call, the following set of
> > operations performed:
> > - spin lock strm_lock
> > - if idle list is not empty, remove zcomp_strm from idle list, spin
> > unlock and return zcomp stream pointer to caller
> > - if idle list is empty, current adds itself to wait queue. it will be
> > awaken by zcomp_strm_multi_put() caller.
> >
> > zcomp_strm_multi_put():
> > - spin lock strm_lock
> > - add zcomp stream to idle list
> > - spin unlock, wake up sleeper
> >
> > Minchan Kim reported that spinlock-based locking scheme has demonstrated a
> > severe perfomance regression for single compression stream case, comparing
> > to mutex-based (https://lkml.org/lkml/2014/2/18/16)
> >
> > base spinlock mutex
> >
> > ==Initial write ==Initial write ==Initial write
> > records: 5 records: 5 records: 5
> > avg: 1642424.35 avg: 699610.40 avg: 1655583.71
> > std: 39890.95(2.43%) std: 232014.19(33.16%) std: 52293.96
> > max: 1690170.94 max: 1163473.45 max: 1697164.75
> > min: 1568669.52 min: 573429.88 min: 1553410.23
> > ==Rewrite ==Rewrite ==Rewrite
> > records: 5 records: 5 records: 5
> > avg: 1611775.39 avg: 501406.64 avg: 1684419.11
> > std: 17144.58(1.06%) std: 15354.41(3.06%) std: 18367.42
> > max: 1641800.95 max: 531356.78 max: 1706445.84
> > min: 1593515.27 min: 488817.78 min: 1655335.73
> > ==Random write ==Random write ==Random write
> > records: 5 records: 5 records: 5
> > avg: 1626318.29 avg: 497250.78 avg: 1695582.06
> > std: 38550.23(2.37%) std: 1405.42(0.28%) std: 9211.98
> > max: 1665839.62 max: 498585.88 max: 1703808.22
> > min: 1562141.21 min: 494526.45 min: 1677664.94
> > ==Pwrite ==Pwrite ==Pwrite
> > records: 5 records: 5 records: 5
> > avg: 1654641.25 avg: 581709.22 avg: 1641452.34
> > std: 47202.59(2.85%) std: 9670.46(1.66%) std: 38963.62
> > max: 1740682.36 max: 591300.09 max: 1687387.69
> > min: 1611436.34 min: 564324.38 min: 1570496.11
> >
> > When only one compression stream available, mutex with spin on owner tends
> > to perform much better than frequent wait_event()/wake_up(). This is why
> > single stream implemented as a special case with mutex locking.
>
> Side note:
>
> It's true for x86 but it could be changed in embedded system like ARM
> which is lower power compared to x86 and it also could depend on
> compression algorithm speed so I am thinking we need adaptive locking
> which sometime work with mutex sometime work with spinlock dynamically.
> Anyway, it is further work and I should investigate more.
>
> Most important thing is that this patchset wouldn't make any regression
> if we use mutex for single stream as default so I'm okay now.
>
> >
> > In order to compile this functionality ZRAM_MULTI_STREAM config option
> > required to be set, which will be introduced later.
> >
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> > drivers/block/zram/zcomp.c | 101 ++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 100 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> > index 9661226..41325ed 100644
> > --- a/drivers/block/zram/zcomp.c
> > +++ b/drivers/block/zram/zcomp.c
> > @@ -12,9 +12,14 @@
> > #include <linux/slab.h>
> > #include <linux/wait.h>
> > #include <linux/sched.h>
> > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > +#include <linux/spinlock.h>
> > +#endif
>
> Do we really need to include spinlock.h?
>
> >
> > #include "zcomp.h"
> >
> > +extern struct zcomp_backend zcomp_lzo;
> > +
> > /*
> > * single zcomp_strm backend private part
> > */
> > @@ -23,7 +28,18 @@ struct zcomp_strm_single {
> > struct zcomp_strm *zstrm;
> > };
> >
> > -extern struct zcomp_backend zcomp_lzo;
> > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > +/*
> > + * multi zcomp_strm backend private part
> > + */
> > +struct zcomp_strm_multi {
> > + /* protect strm list */
> > + spinlock_t strm_lock;
> > + /* list of available strms */
> > + struct list_head idle_strm;
> > + wait_queue_head_t strm_wait;
> > +};
> > +#endif
> >
> > static struct zcomp_backend *find_backend(const char *compress)
> > {
> > @@ -63,6 +79,89 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
> > return zstrm;
> > }
> >
> > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > +/*
> > + * get existing idle zcomp_strm or wait until other process release
> > + * (zcomp_strm_put()) one for us
> > + */
> > +static struct zcomp_strm *zcomp_strm_multi_get(struct zcomp *comp)
> > +{
> > + struct zcomp_strm_multi *zp = comp->private;
>
> How about comp->stream instead of private?
>
> Other than that, Looks good to me.
>
> > + struct zcomp_strm *zstrm;
> > +
> > + while (1) {
> > + spin_lock(&zp->strm_lock);
> > + if (list_empty(&zp->idle_strm)) {
> > + spin_unlock(&zp->strm_lock);
> > + wait_event(zp->strm_wait,
> > + !list_empty(&zp->idle_strm));
> > + continue;
> > + }
> > +
> > + zstrm = list_entry(zp->idle_strm.next,
> > + struct zcomp_strm, list);
> > + list_del(&zstrm->list);
> > + spin_unlock(&zp->strm_lock);
> > + break;
> > + }
> > + return zstrm;
> > +}
> > +
> > +/* add zcomp_strm back to idle list and wake up waiter */
> > +static void zcomp_strm_multi_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> > +{
> > + struct zcomp_strm_multi *zp = comp->private;
> > +
> > + spin_lock(&zp->strm_lock);
> > + list_add(&zstrm->list, &zp->idle_strm);
> > + spin_unlock(&zp->strm_lock);
> > +
> > + wake_up(&zp->strm_wait);
> > +}
> > +
> > +static void zcomp_strm_multi_destroy(struct zcomp *comp)
> > +{
> > + struct zcomp_strm_multi *zp = comp->private;
> > + struct zcomp_strm *zstrm;
> > + while (!list_empty(&zp->idle_strm)) {
> > + zstrm = list_entry(zp->idle_strm.next,
> > + struct zcomp_strm, list);
> > + list_del(&zstrm->list);
> > + zcomp_strm_free(comp, zstrm);
> > + }
> > + kfree(zp);
> > +}
> > +
> > +static int zcomp_strm_multi_create(struct zcomp *comp, int num_strm)
> > +{
> > + int i;
> > + struct zcomp_strm *zstrm;
> > + struct zcomp_strm_multi *zp;
> > +
> > + comp->destroy = zcomp_strm_multi_destroy;
> > + comp->strm_get = zcomp_strm_multi_get;
> > + comp->strm_put = zcomp_strm_multi_put;
> > + zp = kmalloc(sizeof(struct zcomp_strm_multi), GFP_KERNEL);
> > + comp->private = zp;
> > + if (!zp)
> > + return -ENOMEM;
> > +
> > + spin_lock_init(&zp->strm_lock);
> > + INIT_LIST_HEAD(&zp->idle_strm);
> > + init_waitqueue_head(&zp->strm_wait);
> > +
> > + for (i = 0; i < num_strm; i++) {
> > + zstrm = zcomp_strm_alloc(comp);
> > + if (!zstrm) {
> > + zcomp_strm_multi_destroy(comp);
> > + return -ENOMEM;
> > + }
> > + list_add(&zstrm->list, &zp->idle_strm);
> > + }
> > + return 0;
One thing I missed.
How about adding new comp stream dynamically rather than creating
all device is initialized?
> > +}
> > +#endif
> > +
> > static struct zcomp_strm *zcomp_strm_single_get(struct zcomp *comp)
> > {
> > struct zcomp_strm_single *zp = comp->private;
> > --
> > 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
> --
> 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] 23+ messages in thread
* [PATCHv6 5/6] zram: enalbe multi stream compression support in zram
2014-02-21 11:50 [PATCHv6 0/6] add compressing abstraction and multi stream support Sergey Senozhatsky
` (3 preceding siblings ...)
2014-02-21 11:50 ` [PATCHv6 4/6] zram: add multi stream functionality Sergey Senozhatsky
@ 2014-02-21 11:50 ` Sergey Senozhatsky
2014-02-24 23:53 ` Minchan Kim
2014-02-21 11:50 ` [PATCHv6 6/6] zram: document max_comp_streams Sergey Senozhatsky
5 siblings, 1 reply; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-21 11:50 UTC (permalink / raw)
To: Minchan Kim
Cc: Jerome Marchand, Nitin Gupta, linux-kernel, Sergey Senozhatsky
1) Add ZRAM_MULTI_STREAM configuration option and make multi stream
zcomp support available.
2) Introduce zram device attribute max_comp_streams to show and store
current zcomp's max number of zcomp streams (num_strm).
3) Extend zcomp zcomp_create() with `num_strm' parameter. `num_strm'
limits the number of zcomp_strm structs in compression backend's idle
list (max_comp_streams).
If ZRAM_MULTI_STREAM option selected:
-- user can change max number of compression streams (max_comp_streams),
otherwise, attempt to change its default value (1) will be ignored.
-- passing to zcomp_create() num_strm equals to 1 will initialise zcomp
using single compression stream zcomp_strm_single (mutex-based locking)
-- passing to zcomp_create() num_strm greater than 1 will initialise zcomp
using multi compression stream zcomp_strm_multi (spinlock-based locking)
If ZRAM_MULTI_STREAM has not been selected, only single compression stream
support available.
iozone -t 3 -R -r 16K -s 60M -I +Z
test base 1 strm (mutex) 3 strm (spinlock)
-----------------------------------------------------------------------
Initial write 589286.78 583518.39 718011.05
Rewrite 604837.97 596776.38 1515125.72
Random write 584120.11 595714.58 1388850.25
Pwrite 535731.17 541117.38 739295.27
Fwrite 1418083.88 1478612.72 1484927.06
Usage example:
set max_comp_streams to 4
echo 4 > /sys/block/zram0/max_comp_streams
show current max_comp_streams (default value is 1).
cat /sys/block/zram0/max_comp_streams
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/block/zram/Kconfig | 10 ++++++++++
drivers/block/zram/zcomp.c | 13 +++++++++++--
drivers/block/zram/zcomp.h | 2 +-
drivers/block/zram/zram_drv.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
drivers/block/zram/zram_drv.h | 2 +-
5 files changed, 65 insertions(+), 5 deletions(-)
diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
index 3450be8..a2ba9a9 100644
--- a/drivers/block/zram/Kconfig
+++ b/drivers/block/zram/Kconfig
@@ -15,6 +15,16 @@ config ZRAM
See zram.txt for more information.
+config ZRAM_MULTI_STREAM
+ bool "Multi stream compression support"
+ depends on ZRAM
+ default n
+ help
+ This option adds additional functionality to ZRAM, that enables
+ multi compression stream support and provides ability to control
+ maximum number of parallel compressions using max_comp_streams
+ device attribute.
+
config ZRAM_DEBUG
bool "Compressed RAM block device debug support"
depends on ZRAM
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 41325ed..7faa6bd 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -240,8 +240,9 @@ void zcomp_destroy(struct zcomp *comp)
* if requested algorithm is not supported or in case
* of init error
*/
-struct zcomp *zcomp_create(const char *compress)
+struct zcomp *zcomp_create(const char *compress, int num_strm)
{
+ int ret;
struct zcomp *comp;
struct zcomp_backend *backend;
@@ -254,7 +255,15 @@ struct zcomp *zcomp_create(const char *compress)
return NULL;
comp->backend = backend;
- if (zcomp_strm_single_create(comp) != 0) {
+#ifdef CONFIG_ZRAM_MULTI_STREAM
+ if (num_strm > 1)
+ ret = zcomp_strm_multi_create(comp, num_strm);
+ else
+ ret = zcomp_strm_single_create(comp);
+#else
+ ret = zcomp_strm_single_create(comp);
+#endif
+ if (ret != 0) {
zcomp_destroy(comp);
return NULL;
}
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 8dc1d7f..d7e1229 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -42,7 +42,7 @@ struct zcomp {
void (*destroy)(struct zcomp *comp);
};
-struct zcomp *zcomp_create(const char *comp);
+struct zcomp *zcomp_create(const char *comp, int num_strm);
void zcomp_destroy(struct zcomp *comp);
struct zcomp_strm *zcomp_strm_get(struct zcomp *comp);
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 200c12a..c147e6f 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -107,6 +107,42 @@ static ssize_t mem_used_total_show(struct device *dev,
return sprintf(buf, "%llu\n", val);
}
+static ssize_t max_comp_streams_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_comp_streams;
+ up_read(&zram->init_lock);
+
+ return sprintf(buf, "%ld\n", val);
+}
+
+static ssize_t max_comp_streams_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+#ifdef CONFIG_ZRAM_MULTI_STREAM
+ long num;
+ struct zram *zram = dev_to_zram(dev);
+
+ if (kstrtol(buf, 0, &num))
+ return -EINVAL;
+ if (num < 1)
+ return -EINVAL;
+ down_write(&zram->init_lock);
+ if (init_done(zram)) {
+ up_write(&zram->init_lock);
+ pr_info("Can't set max_comp_streams for initialized device\n");
+ return -EBUSY;
+ }
+ zram->max_comp_streams = num;
+ up_write(&zram->init_lock);
+#endif
+ return len;
+}
+
/* flag operations needs meta->tb_lock */
static int zram_test_flag(struct zram_meta *meta, u32 index,
enum zram_pageflags flag)
@@ -503,6 +539,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
zcomp_destroy(zram->comp);
zram->comp = NULL;
+ zram->max_comp_streams = 1;
zram_meta_free(zram->meta);
zram->meta = NULL;
@@ -532,7 +569,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_comp_streams);
if (!zram->comp) {
up_write(&zram->init_lock);
pr_info("Cannot initialise compressing backend\n");
@@ -695,6 +732,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_comp_streams, S_IRUGO | S_IWUSR,
+ max_comp_streams_show, max_comp_streams_store);
ZRAM_ATTR_RO(num_reads);
ZRAM_ATTR_RO(num_writes);
@@ -719,6 +758,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_comp_streams.attr,
NULL,
};
@@ -782,6 +822,7 @@ static int create_device(struct zram *zram, int device_id)
zram->meta = NULL;
zram->comp = NULL;
+ zram->max_comp_streams = 1;
return 0;
out_free_disk:
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 45e04f7..8dccb22 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_comp_streams;
struct zram_stats stats;
};
#endif
--
1.9.0.291.g027825b
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCHv6 5/6] zram: enalbe multi stream compression support in zram
2014-02-21 11:50 ` [PATCHv6 5/6] zram: enalbe multi stream compression support in zram Sergey Senozhatsky
@ 2014-02-24 23:53 ` Minchan Kim
2014-02-24 23:58 ` Sergey Senozhatsky
0 siblings, 1 reply; 23+ messages in thread
From: Minchan Kim @ 2014-02-24 23:53 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On Fri, Feb 21, 2014 at 02:50:42PM +0300, Sergey Senozhatsky wrote:
> 1) Add ZRAM_MULTI_STREAM configuration option and make multi stream
> zcomp support available.
>
> 2) Introduce zram device attribute max_comp_streams to show and store
> current zcomp's max number of zcomp streams (num_strm).
>
> 3) Extend zcomp zcomp_create() with `num_strm' parameter. `num_strm'
> limits the number of zcomp_strm structs in compression backend's idle
> list (max_comp_streams).
>
> If ZRAM_MULTI_STREAM option selected:
> -- user can change max number of compression streams (max_comp_streams),
> otherwise, attempt to change its default value (1) will be ignored.
> -- passing to zcomp_create() num_strm equals to 1 will initialise zcomp
> using single compression stream zcomp_strm_single (mutex-based locking)
> -- passing to zcomp_create() num_strm greater than 1 will initialise zcomp
> using multi compression stream zcomp_strm_multi (spinlock-based locking)
>
> If ZRAM_MULTI_STREAM has not been selected, only single compression stream
> support available.
>
> iozone -t 3 -R -r 16K -s 60M -I +Z
>
> test base 1 strm (mutex) 3 strm (spinlock)
> -----------------------------------------------------------------------
> Initial write 589286.78 583518.39 718011.05
> Rewrite 604837.97 596776.38 1515125.72
> Random write 584120.11 595714.58 1388850.25
> Pwrite 535731.17 541117.38 739295.27
> Fwrite 1418083.88 1478612.72 1484927.06
>
> Usage example:
> set max_comp_streams to 4
> echo 4 > /sys/block/zram0/max_comp_streams
>
> show current max_comp_streams (default value is 1).
> cat /sys/block/zram0/max_comp_streams
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> drivers/block/zram/Kconfig | 10 ++++++++++
> drivers/block/zram/zcomp.c | 13 +++++++++++--
> drivers/block/zram/zcomp.h | 2 +-
> drivers/block/zram/zram_drv.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> drivers/block/zram/zram_drv.h | 2 +-
> 5 files changed, 65 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> index 3450be8..a2ba9a9 100644
> --- a/drivers/block/zram/Kconfig
> +++ b/drivers/block/zram/Kconfig
> @@ -15,6 +15,16 @@ config ZRAM
>
> See zram.txt for more information.
>
> +config ZRAM_MULTI_STREAM
> + bool "Multi stream compression support"
> + depends on ZRAM
> + default n
> + help
> + This option adds additional functionality to ZRAM, that enables
> + multi compression stream support and provides ability to control
> + maximum number of parallel compressions using max_comp_streams
> + device attribute.
> +
> config ZRAM_DEBUG
> bool "Compressed RAM block device debug support"
> depends on ZRAM
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 41325ed..7faa6bd 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -240,8 +240,9 @@ void zcomp_destroy(struct zcomp *comp)
> * if requested algorithm is not supported or in case
> * of init error
> */
> -struct zcomp *zcomp_create(const char *compress)
> +struct zcomp *zcomp_create(const char *compress, int num_strm)
> {
> + int ret;
> struct zcomp *comp;
> struct zcomp_backend *backend;
>
> @@ -254,7 +255,15 @@ struct zcomp *zcomp_create(const char *compress)
> return NULL;
>
> comp->backend = backend;
> - if (zcomp_strm_single_create(comp) != 0) {
> +#ifdef CONFIG_ZRAM_MULTI_STREAM
> + if (num_strm > 1)
> + ret = zcomp_strm_multi_create(comp, num_strm);
> + else
> + ret = zcomp_strm_single_create(comp);
> +#else
> + ret = zcomp_strm_single_create(comp);
> +#endif
> + if (ret != 0) {
> zcomp_destroy(comp);
> return NULL;
> }
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index 8dc1d7f..d7e1229 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -42,7 +42,7 @@ struct zcomp {
> void (*destroy)(struct zcomp *comp);
> };
>
> -struct zcomp *zcomp_create(const char *comp);
> +struct zcomp *zcomp_create(const char *comp, int num_strm);
> void zcomp_destroy(struct zcomp *comp);
>
> struct zcomp_strm *zcomp_strm_get(struct zcomp *comp);
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 200c12a..c147e6f 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -107,6 +107,42 @@ static ssize_t mem_used_total_show(struct device *dev,
> return sprintf(buf, "%llu\n", val);
> }
>
> +static ssize_t max_comp_streams_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + long val;
Why 'long'?
> + struct zram *zram = dev_to_zram(dev);
> +
> + down_read(&zram->init_lock);
> + val = zram->max_comp_streams;
> + up_read(&zram->init_lock);
> +
> + return sprintf(buf, "%ld\n", val);
> +}
> +
> +static ssize_t max_comp_streams_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> +#ifdef CONFIG_ZRAM_MULTI_STREAM
> + long num;
> + struct zram *zram = dev_to_zram(dev);
> +
> + if (kstrtol(buf, 0, &num))
> + return -EINVAL;
> + if (num < 1)
> + return -EINVAL;
> + down_write(&zram->init_lock);
> + if (init_done(zram)) {
> + up_write(&zram->init_lock);
> + pr_info("Can't set max_comp_streams for initialized device\n");
> + return -EBUSY;
> + }
> + zram->max_comp_streams = num;
> + up_write(&zram->init_lock);
If user decrease max stream in runtime, who care of freeing streams?
> +#endif
> + return len;
> +}
> +
> /* flag operations needs meta->tb_lock */
> static int zram_test_flag(struct zram_meta *meta, u32 index,
> enum zram_pageflags flag)
> @@ -503,6 +539,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>
> zcomp_destroy(zram->comp);
> zram->comp = NULL;
> + zram->max_comp_streams = 1;
>
> zram_meta_free(zram->meta);
> zram->meta = NULL;
> @@ -532,7 +569,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_comp_streams);
> if (!zram->comp) {
> up_write(&zram->init_lock);
> pr_info("Cannot initialise compressing backend\n");
> @@ -695,6 +732,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_comp_streams, S_IRUGO | S_IWUSR,
> + max_comp_streams_show, max_comp_streams_store);
>
> ZRAM_ATTR_RO(num_reads);
> ZRAM_ATTR_RO(num_writes);
> @@ -719,6 +758,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_comp_streams.attr,
> NULL,
> };
>
> @@ -782,6 +822,7 @@ static int create_device(struct zram *zram, int device_id)
>
> zram->meta = NULL;
> zram->comp = NULL;
> + zram->max_comp_streams = 1;
> return 0;
>
> out_free_disk:
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 45e04f7..8dccb22 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_comp_streams;
> struct zram_stats stats;
> };
> #endif
> --
> 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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCHv6 5/6] zram: enalbe multi stream compression support in zram
2014-02-24 23:53 ` Minchan Kim
@ 2014-02-24 23:58 ` Sergey Senozhatsky
2014-02-25 0:12 ` Minchan Kim
0 siblings, 1 reply; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-24 23:58 UTC (permalink / raw)
To: Minchan Kim; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On (02/25/14 08:53), Minchan Kim wrote:
> On Fri, Feb 21, 2014 at 02:50:42PM +0300, Sergey Senozhatsky wrote:
> > 1) Add ZRAM_MULTI_STREAM configuration option and make multi stream
> > zcomp support available.
> >
> > 2) Introduce zram device attribute max_comp_streams to show and store
> > current zcomp's max number of zcomp streams (num_strm).
> >
> > 3) Extend zcomp zcomp_create() with `num_strm' parameter. `num_strm'
> > limits the number of zcomp_strm structs in compression backend's idle
> > list (max_comp_streams).
> >
> > If ZRAM_MULTI_STREAM option selected:
> > -- user can change max number of compression streams (max_comp_streams),
> > otherwise, attempt to change its default value (1) will be ignored.
> > -- passing to zcomp_create() num_strm equals to 1 will initialise zcomp
> > using single compression stream zcomp_strm_single (mutex-based locking)
> > -- passing to zcomp_create() num_strm greater than 1 will initialise zcomp
> > using multi compression stream zcomp_strm_multi (spinlock-based locking)
> >
> > If ZRAM_MULTI_STREAM has not been selected, only single compression stream
> > support available.
> >
> > iozone -t 3 -R -r 16K -s 60M -I +Z
> >
> > test base 1 strm (mutex) 3 strm (spinlock)
> > -----------------------------------------------------------------------
> > Initial write 589286.78 583518.39 718011.05
> > Rewrite 604837.97 596776.38 1515125.72
> > Random write 584120.11 595714.58 1388850.25
> > Pwrite 535731.17 541117.38 739295.27
> > Fwrite 1418083.88 1478612.72 1484927.06
> >
> > Usage example:
> > set max_comp_streams to 4
> > echo 4 > /sys/block/zram0/max_comp_streams
> >
> > show current max_comp_streams (default value is 1).
> > cat /sys/block/zram0/max_comp_streams
> >
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> > drivers/block/zram/Kconfig | 10 ++++++++++
> > drivers/block/zram/zcomp.c | 13 +++++++++++--
> > drivers/block/zram/zcomp.h | 2 +-
> > drivers/block/zram/zram_drv.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> > drivers/block/zram/zram_drv.h | 2 +-
> > 5 files changed, 65 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> > index 3450be8..a2ba9a9 100644
> > --- a/drivers/block/zram/Kconfig
> > +++ b/drivers/block/zram/Kconfig
> > @@ -15,6 +15,16 @@ config ZRAM
> >
> > See zram.txt for more information.
> >
> > +config ZRAM_MULTI_STREAM
> > + bool "Multi stream compression support"
> > + depends on ZRAM
> > + default n
> > + help
> > + This option adds additional functionality to ZRAM, that enables
> > + multi compression stream support and provides ability to control
> > + maximum number of parallel compressions using max_comp_streams
> > + device attribute.
> > +
> > config ZRAM_DEBUG
> > bool "Compressed RAM block device debug support"
> > depends on ZRAM
> > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> > index 41325ed..7faa6bd 100644
> > --- a/drivers/block/zram/zcomp.c
> > +++ b/drivers/block/zram/zcomp.c
> > @@ -240,8 +240,9 @@ void zcomp_destroy(struct zcomp *comp)
> > * if requested algorithm is not supported or in case
> > * of init error
> > */
> > -struct zcomp *zcomp_create(const char *compress)
> > +struct zcomp *zcomp_create(const char *compress, int num_strm)
> > {
> > + int ret;
> > struct zcomp *comp;
> > struct zcomp_backend *backend;
> >
> > @@ -254,7 +255,15 @@ struct zcomp *zcomp_create(const char *compress)
> > return NULL;
> >
> > comp->backend = backend;
> > - if (zcomp_strm_single_create(comp) != 0) {
> > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > + if (num_strm > 1)
> > + ret = zcomp_strm_multi_create(comp, num_strm);
> > + else
> > + ret = zcomp_strm_single_create(comp);
> > +#else
> > + ret = zcomp_strm_single_create(comp);
> > +#endif
> > + if (ret != 0) {
> > zcomp_destroy(comp);
> > return NULL;
> > }
> > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > index 8dc1d7f..d7e1229 100644
> > --- a/drivers/block/zram/zcomp.h
> > +++ b/drivers/block/zram/zcomp.h
> > @@ -42,7 +42,7 @@ struct zcomp {
> > void (*destroy)(struct zcomp *comp);
> > };
> >
> > -struct zcomp *zcomp_create(const char *comp);
> > +struct zcomp *zcomp_create(const char *comp, int num_strm);
> > void zcomp_destroy(struct zcomp *comp);
> >
> > struct zcomp_strm *zcomp_strm_get(struct zcomp *comp);
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 200c12a..c147e6f 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -107,6 +107,42 @@ static ssize_t mem_used_total_show(struct device *dev,
> > return sprintf(buf, "%llu\n", val);
> > }
> >
> > +static ssize_t max_comp_streams_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + long val;
>
> Why 'long'?
>
> > + struct zram *zram = dev_to_zram(dev);
> > +
> > + down_read(&zram->init_lock);
> > + val = zram->max_comp_streams;
> > + up_read(&zram->init_lock);
> > +
> > + return sprintf(buf, "%ld\n", val);
> > +}
> > +
> > +static ssize_t max_comp_streams_store(struct device *dev,
> > + struct device_attribute *attr, const char *buf, size_t len)
> > +{
> > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > + long num;
> > + struct zram *zram = dev_to_zram(dev);
> > +
> > + if (kstrtol(buf, 0, &num))
> > + return -EINVAL;
> > + if (num < 1)
> > + return -EINVAL;
> > + down_write(&zram->init_lock);
> > + if (init_done(zram)) {
> > + up_write(&zram->init_lock);
> > + pr_info("Can't set max_comp_streams for initialized device\n");
> > + return -EBUSY;
> > + }
> > + zram->max_comp_streams = num;
> > + up_write(&zram->init_lock);
>
> If user decrease max stream in runtime, who care of freeing streams?
>
at the moment (to make it simpler), user can set max stream number
only for un-initialised device. once zcomp created and initialised
user cannot change max streams
pr_info("Can't set max_comp_streams for initialized device\n");
> > +#endif
> > + return len;
> > +}
> > +
> > /* flag operations needs meta->tb_lock */
> > static int zram_test_flag(struct zram_meta *meta, u32 index,
> > enum zram_pageflags flag)
> > @@ -503,6 +539,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> >
> > zcomp_destroy(zram->comp);
> > zram->comp = NULL;
> > + zram->max_comp_streams = 1;
> >
> > zram_meta_free(zram->meta);
> > zram->meta = NULL;
> > @@ -532,7 +569,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_comp_streams);
> > if (!zram->comp) {
> > up_write(&zram->init_lock);
> > pr_info("Cannot initialise compressing backend\n");
> > @@ -695,6 +732,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_comp_streams, S_IRUGO | S_IWUSR,
> > + max_comp_streams_show, max_comp_streams_store);
> >
> > ZRAM_ATTR_RO(num_reads);
> > ZRAM_ATTR_RO(num_writes);
> > @@ -719,6 +758,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_comp_streams.attr,
> > NULL,
> > };
> >
> > @@ -782,6 +822,7 @@ static int create_device(struct zram *zram, int device_id)
> >
> > zram->meta = NULL;
> > zram->comp = NULL;
> > + zram->max_comp_streams = 1;
> > return 0;
> >
> > out_free_disk:
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index 45e04f7..8dccb22 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_comp_streams;
> > struct zram_stats stats;
> > };
> > #endif
> > --
> > 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
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCHv6 5/6] zram: enalbe multi stream compression support in zram
2014-02-24 23:58 ` Sergey Senozhatsky
@ 2014-02-25 0:12 ` Minchan Kim
2014-02-25 0:25 ` Sergey Senozhatsky
0 siblings, 1 reply; 23+ messages in thread
From: Minchan Kim @ 2014-02-25 0:12 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On Tue, Feb 25, 2014 at 02:58:24AM +0300, Sergey Senozhatsky wrote:
> On (02/25/14 08:53), Minchan Kim wrote:
> > On Fri, Feb 21, 2014 at 02:50:42PM +0300, Sergey Senozhatsky wrote:
> > > 1) Add ZRAM_MULTI_STREAM configuration option and make multi stream
> > > zcomp support available.
> > >
> > > 2) Introduce zram device attribute max_comp_streams to show and store
> > > current zcomp's max number of zcomp streams (num_strm).
> > >
> > > 3) Extend zcomp zcomp_create() with `num_strm' parameter. `num_strm'
> > > limits the number of zcomp_strm structs in compression backend's idle
> > > list (max_comp_streams).
> > >
> > > If ZRAM_MULTI_STREAM option selected:
> > > -- user can change max number of compression streams (max_comp_streams),
> > > otherwise, attempt to change its default value (1) will be ignored.
> > > -- passing to zcomp_create() num_strm equals to 1 will initialise zcomp
> > > using single compression stream zcomp_strm_single (mutex-based locking)
> > > -- passing to zcomp_create() num_strm greater than 1 will initialise zcomp
> > > using multi compression stream zcomp_strm_multi (spinlock-based locking)
> > >
> > > If ZRAM_MULTI_STREAM has not been selected, only single compression stream
> > > support available.
> > >
> > > iozone -t 3 -R -r 16K -s 60M -I +Z
> > >
> > > test base 1 strm (mutex) 3 strm (spinlock)
> > > -----------------------------------------------------------------------
> > > Initial write 589286.78 583518.39 718011.05
> > > Rewrite 604837.97 596776.38 1515125.72
> > > Random write 584120.11 595714.58 1388850.25
> > > Pwrite 535731.17 541117.38 739295.27
> > > Fwrite 1418083.88 1478612.72 1484927.06
> > >
> > > Usage example:
> > > set max_comp_streams to 4
> > > echo 4 > /sys/block/zram0/max_comp_streams
> > >
> > > show current max_comp_streams (default value is 1).
> > > cat /sys/block/zram0/max_comp_streams
> > >
> > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > ---
> > > drivers/block/zram/Kconfig | 10 ++++++++++
> > > drivers/block/zram/zcomp.c | 13 +++++++++++--
> > > drivers/block/zram/zcomp.h | 2 +-
> > > drivers/block/zram/zram_drv.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> > > drivers/block/zram/zram_drv.h | 2 +-
> > > 5 files changed, 65 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> > > index 3450be8..a2ba9a9 100644
> > > --- a/drivers/block/zram/Kconfig
> > > +++ b/drivers/block/zram/Kconfig
> > > @@ -15,6 +15,16 @@ config ZRAM
> > >
> > > See zram.txt for more information.
> > >
> > > +config ZRAM_MULTI_STREAM
> > > + bool "Multi stream compression support"
> > > + depends on ZRAM
> > > + default n
> > > + help
> > > + This option adds additional functionality to ZRAM, that enables
> > > + multi compression stream support and provides ability to control
> > > + maximum number of parallel compressions using max_comp_streams
> > > + device attribute.
> > > +
> > > config ZRAM_DEBUG
> > > bool "Compressed RAM block device debug support"
> > > depends on ZRAM
> > > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> > > index 41325ed..7faa6bd 100644
> > > --- a/drivers/block/zram/zcomp.c
> > > +++ b/drivers/block/zram/zcomp.c
> > > @@ -240,8 +240,9 @@ void zcomp_destroy(struct zcomp *comp)
> > > * if requested algorithm is not supported or in case
> > > * of init error
> > > */
> > > -struct zcomp *zcomp_create(const char *compress)
> > > +struct zcomp *zcomp_create(const char *compress, int num_strm)
> > > {
> > > + int ret;
> > > struct zcomp *comp;
> > > struct zcomp_backend *backend;
> > >
> > > @@ -254,7 +255,15 @@ struct zcomp *zcomp_create(const char *compress)
> > > return NULL;
> > >
> > > comp->backend = backend;
> > > - if (zcomp_strm_single_create(comp) != 0) {
> > > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > > + if (num_strm > 1)
> > > + ret = zcomp_strm_multi_create(comp, num_strm);
> > > + else
> > > + ret = zcomp_strm_single_create(comp);
> > > +#else
> > > + ret = zcomp_strm_single_create(comp);
> > > +#endif
> > > + if (ret != 0) {
> > > zcomp_destroy(comp);
> > > return NULL;
> > > }
> > > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > > index 8dc1d7f..d7e1229 100644
> > > --- a/drivers/block/zram/zcomp.h
> > > +++ b/drivers/block/zram/zcomp.h
> > > @@ -42,7 +42,7 @@ struct zcomp {
> > > void (*destroy)(struct zcomp *comp);
> > > };
> > >
> > > -struct zcomp *zcomp_create(const char *comp);
> > > +struct zcomp *zcomp_create(const char *comp, int num_strm);
> > > void zcomp_destroy(struct zcomp *comp);
> > >
> > > struct zcomp_strm *zcomp_strm_get(struct zcomp *comp);
> > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > > index 200c12a..c147e6f 100644
> > > --- a/drivers/block/zram/zram_drv.c
> > > +++ b/drivers/block/zram/zram_drv.c
> > > @@ -107,6 +107,42 @@ static ssize_t mem_used_total_show(struct device *dev,
> > > return sprintf(buf, "%llu\n", val);
> > > }
> > >
> > > +static ssize_t max_comp_streams_show(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + long val;
> >
> > Why 'long'?
> >
> > > + struct zram *zram = dev_to_zram(dev);
> > > +
> > > + down_read(&zram->init_lock);
> > > + val = zram->max_comp_streams;
> > > + up_read(&zram->init_lock);
> > > +
> > > + return sprintf(buf, "%ld\n", val);
> > > +}
> > > +
> > > +static ssize_t max_comp_streams_store(struct device *dev,
> > > + struct device_attribute *attr, const char *buf, size_t len)
> > > +{
> > > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > > + long num;
> > > + struct zram *zram = dev_to_zram(dev);
> > > +
> > > + if (kstrtol(buf, 0, &num))
> > > + return -EINVAL;
> > > + if (num < 1)
> > > + return -EINVAL;
> > > + down_write(&zram->init_lock);
> > > + if (init_done(zram)) {
> > > + up_write(&zram->init_lock);
> > > + pr_info("Can't set max_comp_streams for initialized device\n");
> > > + return -EBUSY;
> > > + }
> > > + zram->max_comp_streams = num;
> > > + up_write(&zram->init_lock);
> >
> > If user decrease max stream in runtime, who care of freeing streams?
> >
>
> at the moment (to make it simpler), user can set max stream number
> only for un-initialised device. once zcomp created and initialised
> user cannot change max streams
Hmm, I hope we have the abilitiy in the first place because I think
it's not complicate and make test very handy. Please.
>
> pr_info("Can't set max_comp_streams for initialized device\n");
>
> > > +#endif
> > > + return len;
> > > +}
> > > +
> > > /* flag operations needs meta->tb_lock */
> > > static int zram_test_flag(struct zram_meta *meta, u32 index,
> > > enum zram_pageflags flag)
> > > @@ -503,6 +539,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> > >
> > > zcomp_destroy(zram->comp);
> > > zram->comp = NULL;
> > > + zram->max_comp_streams = 1;
> > >
> > > zram_meta_free(zram->meta);
> > > zram->meta = NULL;
> > > @@ -532,7 +569,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_comp_streams);
> > > if (!zram->comp) {
> > > up_write(&zram->init_lock);
> > > pr_info("Cannot initialise compressing backend\n");
> > > @@ -695,6 +732,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_comp_streams, S_IRUGO | S_IWUSR,
> > > + max_comp_streams_show, max_comp_streams_store);
> > >
> > > ZRAM_ATTR_RO(num_reads);
> > > ZRAM_ATTR_RO(num_writes);
> > > @@ -719,6 +758,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_comp_streams.attr,
> > > NULL,
> > > };
> > >
> > > @@ -782,6 +822,7 @@ static int create_device(struct zram *zram, int device_id)
> > >
> > > zram->meta = NULL;
> > > zram->comp = NULL;
> > > + zram->max_comp_streams = 1;
> > > return 0;
> > >
> > > out_free_disk:
> > > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > > index 45e04f7..8dccb22 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_comp_streams;
> > > struct zram_stats stats;
> > > };
> > > #endif
> > > --
> > > 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
> >
> --
> 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] 23+ messages in thread* Re: [PATCHv6 5/6] zram: enalbe multi stream compression support in zram
2014-02-25 0:12 ` Minchan Kim
@ 2014-02-25 0:25 ` Sergey Senozhatsky
2014-02-25 0:40 ` Minchan Kim
0 siblings, 1 reply; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-25 0:25 UTC (permalink / raw)
To: Minchan Kim; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On (02/25/14 09:12), Minchan Kim wrote:
> On Tue, Feb 25, 2014 at 02:58:24AM +0300, Sergey Senozhatsky wrote:
> > On (02/25/14 08:53), Minchan Kim wrote:
> > > On Fri, Feb 21, 2014 at 02:50:42PM +0300, Sergey Senozhatsky wrote:
> > > > 1) Add ZRAM_MULTI_STREAM configuration option and make multi stream
> > > > zcomp support available.
> > > >
> > > > 2) Introduce zram device attribute max_comp_streams to show and store
> > > > current zcomp's max number of zcomp streams (num_strm).
> > > >
> > > > 3) Extend zcomp zcomp_create() with `num_strm' parameter. `num_strm'
> > > > limits the number of zcomp_strm structs in compression backend's idle
> > > > list (max_comp_streams).
> > > >
> > > > If ZRAM_MULTI_STREAM option selected:
> > > > -- user can change max number of compression streams (max_comp_streams),
> > > > otherwise, attempt to change its default value (1) will be ignored.
> > > > -- passing to zcomp_create() num_strm equals to 1 will initialise zcomp
> > > > using single compression stream zcomp_strm_single (mutex-based locking)
> > > > -- passing to zcomp_create() num_strm greater than 1 will initialise zcomp
> > > > using multi compression stream zcomp_strm_multi (spinlock-based locking)
> > > >
> > > > If ZRAM_MULTI_STREAM has not been selected, only single compression stream
> > > > support available.
> > > >
> > > > iozone -t 3 -R -r 16K -s 60M -I +Z
> > > >
> > > > test base 1 strm (mutex) 3 strm (spinlock)
> > > > -----------------------------------------------------------------------
> > > > Initial write 589286.78 583518.39 718011.05
> > > > Rewrite 604837.97 596776.38 1515125.72
> > > > Random write 584120.11 595714.58 1388850.25
> > > > Pwrite 535731.17 541117.38 739295.27
> > > > Fwrite 1418083.88 1478612.72 1484927.06
> > > >
> > > > Usage example:
> > > > set max_comp_streams to 4
> > > > echo 4 > /sys/block/zram0/max_comp_streams
> > > >
> > > > show current max_comp_streams (default value is 1).
> > > > cat /sys/block/zram0/max_comp_streams
> > > >
> > > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > > ---
> > > > drivers/block/zram/Kconfig | 10 ++++++++++
> > > > drivers/block/zram/zcomp.c | 13 +++++++++++--
> > > > drivers/block/zram/zcomp.h | 2 +-
> > > > drivers/block/zram/zram_drv.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> > > > drivers/block/zram/zram_drv.h | 2 +-
> > > > 5 files changed, 65 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> > > > index 3450be8..a2ba9a9 100644
> > > > --- a/drivers/block/zram/Kconfig
> > > > +++ b/drivers/block/zram/Kconfig
> > > > @@ -15,6 +15,16 @@ config ZRAM
> > > >
> > > > See zram.txt for more information.
> > > >
> > > > +config ZRAM_MULTI_STREAM
> > > > + bool "Multi stream compression support"
> > > > + depends on ZRAM
> > > > + default n
> > > > + help
> > > > + This option adds additional functionality to ZRAM, that enables
> > > > + multi compression stream support and provides ability to control
> > > > + maximum number of parallel compressions using max_comp_streams
> > > > + device attribute.
> > > > +
> > > > config ZRAM_DEBUG
> > > > bool "Compressed RAM block device debug support"
> > > > depends on ZRAM
> > > > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> > > > index 41325ed..7faa6bd 100644
> > > > --- a/drivers/block/zram/zcomp.c
> > > > +++ b/drivers/block/zram/zcomp.c
> > > > @@ -240,8 +240,9 @@ void zcomp_destroy(struct zcomp *comp)
> > > > * if requested algorithm is not supported or in case
> > > > * of init error
> > > > */
> > > > -struct zcomp *zcomp_create(const char *compress)
> > > > +struct zcomp *zcomp_create(const char *compress, int num_strm)
> > > > {
> > > > + int ret;
> > > > struct zcomp *comp;
> > > > struct zcomp_backend *backend;
> > > >
> > > > @@ -254,7 +255,15 @@ struct zcomp *zcomp_create(const char *compress)
> > > > return NULL;
> > > >
> > > > comp->backend = backend;
> > > > - if (zcomp_strm_single_create(comp) != 0) {
> > > > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > > > + if (num_strm > 1)
> > > > + ret = zcomp_strm_multi_create(comp, num_strm);
> > > > + else
> > > > + ret = zcomp_strm_single_create(comp);
> > > > +#else
> > > > + ret = zcomp_strm_single_create(comp);
> > > > +#endif
> > > > + if (ret != 0) {
> > > > zcomp_destroy(comp);
> > > > return NULL;
> > > > }
> > > > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > > > index 8dc1d7f..d7e1229 100644
> > > > --- a/drivers/block/zram/zcomp.h
> > > > +++ b/drivers/block/zram/zcomp.h
> > > > @@ -42,7 +42,7 @@ struct zcomp {
> > > > void (*destroy)(struct zcomp *comp);
> > > > };
> > > >
> > > > -struct zcomp *zcomp_create(const char *comp);
> > > > +struct zcomp *zcomp_create(const char *comp, int num_strm);
> > > > void zcomp_destroy(struct zcomp *comp);
> > > >
> > > > struct zcomp_strm *zcomp_strm_get(struct zcomp *comp);
> > > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > > > index 200c12a..c147e6f 100644
> > > > --- a/drivers/block/zram/zram_drv.c
> > > > +++ b/drivers/block/zram/zram_drv.c
> > > > @@ -107,6 +107,42 @@ static ssize_t mem_used_total_show(struct device *dev,
> > > > return sprintf(buf, "%llu\n", val);
> > > > }
> > > >
> > > > +static ssize_t max_comp_streams_show(struct device *dev,
> > > > + struct device_attribute *attr, char *buf)
> > > > +{
> > > > + long val;
> > >
> > > Why 'long'?
> > >
> > > > + struct zram *zram = dev_to_zram(dev);
> > > > +
> > > > + down_read(&zram->init_lock);
> > > > + val = zram->max_comp_streams;
> > > > + up_read(&zram->init_lock);
> > > > +
> > > > + return sprintf(buf, "%ld\n", val);
> > > > +}
> > > > +
> > > > +static ssize_t max_comp_streams_store(struct device *dev,
> > > > + struct device_attribute *attr, const char *buf, size_t len)
> > > > +{
> > > > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > > > + long num;
> > > > + struct zram *zram = dev_to_zram(dev);
> > > > +
> > > > + if (kstrtol(buf, 0, &num))
> > > > + return -EINVAL;
> > > > + if (num < 1)
> > > > + return -EINVAL;
> > > > + down_write(&zram->init_lock);
> > > > + if (init_done(zram)) {
> > > > + up_write(&zram->init_lock);
> > > > + pr_info("Can't set max_comp_streams for initialized device\n");
> > > > + return -EBUSY;
> > > > + }
> > > > + zram->max_comp_streams = num;
> > > > + up_write(&zram->init_lock);
> > >
> > > If user decrease max stream in runtime, who care of freeing streams?
> > >
> >
> > at the moment (to make it simpler), user can set max stream number
> > only for un-initialised device. once zcomp created and initialised
> > user cannot change max streams
>
> Hmm, I hope we have the abilitiy in the first place because I think
> it's not complicate and make test very handy. Please.
>
ok, I'll take a look. I'm not sure that I'll finish it tonight.
I need to think about it for a moment. probably it will require
several additional counters in zcomp_strm_multi -- max_strms
and num_strms (num_strms equals to number of currently allocated
streams). user may decrease (for example) max streams while every
available stream is busy (idle list is empty), so we need to check
max_strms and avail_strms while we _put() stream. and we definitely
don't want max_strms for zcomp_strm_multi to be less than 2 :-)
so far I've address your notes and concerns -- no CONFIG option,
both multi and single streams options, etc.
-ss
> >
> > pr_info("Can't set max_comp_streams for initialized device\n");
> >
> > > > +#endif
> > > > + return len;
> > > > +}
> > > > +
> > > > /* flag operations needs meta->tb_lock */
> > > > static int zram_test_flag(struct zram_meta *meta, u32 index,
> > > > enum zram_pageflags flag)
> > > > @@ -503,6 +539,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> > > >
> > > > zcomp_destroy(zram->comp);
> > > > zram->comp = NULL;
> > > > + zram->max_comp_streams = 1;
> > > >
> > > > zram_meta_free(zram->meta);
> > > > zram->meta = NULL;
> > > > @@ -532,7 +569,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_comp_streams);
> > > > if (!zram->comp) {
> > > > up_write(&zram->init_lock);
> > > > pr_info("Cannot initialise compressing backend\n");
> > > > @@ -695,6 +732,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_comp_streams, S_IRUGO | S_IWUSR,
> > > > + max_comp_streams_show, max_comp_streams_store);
> > > >
> > > > ZRAM_ATTR_RO(num_reads);
> > > > ZRAM_ATTR_RO(num_writes);
> > > > @@ -719,6 +758,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_comp_streams.attr,
> > > > NULL,
> > > > };
> > > >
> > > > @@ -782,6 +822,7 @@ static int create_device(struct zram *zram, int device_id)
> > > >
> > > > zram->meta = NULL;
> > > > zram->comp = NULL;
> > > > + zram->max_comp_streams = 1;
> > > > return 0;
> > > >
> > > > out_free_disk:
> > > > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > > > index 45e04f7..8dccb22 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_comp_streams;
> > > > struct zram_stats stats;
> > > > };
> > > > #endif
> > > > --
> > > > 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
> > >
> > --
> > 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] 23+ messages in thread* Re: [PATCHv6 5/6] zram: enalbe multi stream compression support in zram
2014-02-25 0:25 ` Sergey Senozhatsky
@ 2014-02-25 0:40 ` Minchan Kim
0 siblings, 0 replies; 23+ messages in thread
From: Minchan Kim @ 2014-02-25 0:40 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel
On Tue, Feb 25, 2014 at 03:25:22AM +0300, Sergey Senozhatsky wrote:
> On (02/25/14 09:12), Minchan Kim wrote:
> > On Tue, Feb 25, 2014 at 02:58:24AM +0300, Sergey Senozhatsky wrote:
> > > On (02/25/14 08:53), Minchan Kim wrote:
> > > > On Fri, Feb 21, 2014 at 02:50:42PM +0300, Sergey Senozhatsky wrote:
> > > > > 1) Add ZRAM_MULTI_STREAM configuration option and make multi stream
> > > > > zcomp support available.
> > > > >
> > > > > 2) Introduce zram device attribute max_comp_streams to show and store
> > > > > current zcomp's max number of zcomp streams (num_strm).
> > > > >
> > > > > 3) Extend zcomp zcomp_create() with `num_strm' parameter. `num_strm'
> > > > > limits the number of zcomp_strm structs in compression backend's idle
> > > > > list (max_comp_streams).
> > > > >
> > > > > If ZRAM_MULTI_STREAM option selected:
> > > > > -- user can change max number of compression streams (max_comp_streams),
> > > > > otherwise, attempt to change its default value (1) will be ignored.
> > > > > -- passing to zcomp_create() num_strm equals to 1 will initialise zcomp
> > > > > using single compression stream zcomp_strm_single (mutex-based locking)
> > > > > -- passing to zcomp_create() num_strm greater than 1 will initialise zcomp
> > > > > using multi compression stream zcomp_strm_multi (spinlock-based locking)
> > > > >
> > > > > If ZRAM_MULTI_STREAM has not been selected, only single compression stream
> > > > > support available.
> > > > >
> > > > > iozone -t 3 -R -r 16K -s 60M -I +Z
> > > > >
> > > > > test base 1 strm (mutex) 3 strm (spinlock)
> > > > > -----------------------------------------------------------------------
> > > > > Initial write 589286.78 583518.39 718011.05
> > > > > Rewrite 604837.97 596776.38 1515125.72
> > > > > Random write 584120.11 595714.58 1388850.25
> > > > > Pwrite 535731.17 541117.38 739295.27
> > > > > Fwrite 1418083.88 1478612.72 1484927.06
> > > > >
> > > > > Usage example:
> > > > > set max_comp_streams to 4
> > > > > echo 4 > /sys/block/zram0/max_comp_streams
> > > > >
> > > > > show current max_comp_streams (default value is 1).
> > > > > cat /sys/block/zram0/max_comp_streams
> > > > >
> > > > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > > > ---
> > > > > drivers/block/zram/Kconfig | 10 ++++++++++
> > > > > drivers/block/zram/zcomp.c | 13 +++++++++++--
> > > > > drivers/block/zram/zcomp.h | 2 +-
> > > > > drivers/block/zram/zram_drv.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> > > > > drivers/block/zram/zram_drv.h | 2 +-
> > > > > 5 files changed, 65 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> > > > > index 3450be8..a2ba9a9 100644
> > > > > --- a/drivers/block/zram/Kconfig
> > > > > +++ b/drivers/block/zram/Kconfig
> > > > > @@ -15,6 +15,16 @@ config ZRAM
> > > > >
> > > > > See zram.txt for more information.
> > > > >
> > > > > +config ZRAM_MULTI_STREAM
> > > > > + bool "Multi stream compression support"
> > > > > + depends on ZRAM
> > > > > + default n
> > > > > + help
> > > > > + This option adds additional functionality to ZRAM, that enables
> > > > > + multi compression stream support and provides ability to control
> > > > > + maximum number of parallel compressions using max_comp_streams
> > > > > + device attribute.
> > > > > +
> > > > > config ZRAM_DEBUG
> > > > > bool "Compressed RAM block device debug support"
> > > > > depends on ZRAM
> > > > > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> > > > > index 41325ed..7faa6bd 100644
> > > > > --- a/drivers/block/zram/zcomp.c
> > > > > +++ b/drivers/block/zram/zcomp.c
> > > > > @@ -240,8 +240,9 @@ void zcomp_destroy(struct zcomp *comp)
> > > > > * if requested algorithm is not supported or in case
> > > > > * of init error
> > > > > */
> > > > > -struct zcomp *zcomp_create(const char *compress)
> > > > > +struct zcomp *zcomp_create(const char *compress, int num_strm)
> > > > > {
> > > > > + int ret;
> > > > > struct zcomp *comp;
> > > > > struct zcomp_backend *backend;
> > > > >
> > > > > @@ -254,7 +255,15 @@ struct zcomp *zcomp_create(const char *compress)
> > > > > return NULL;
> > > > >
> > > > > comp->backend = backend;
> > > > > - if (zcomp_strm_single_create(comp) != 0) {
> > > > > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > > > > + if (num_strm > 1)
> > > > > + ret = zcomp_strm_multi_create(comp, num_strm);
> > > > > + else
> > > > > + ret = zcomp_strm_single_create(comp);
> > > > > +#else
> > > > > + ret = zcomp_strm_single_create(comp);
> > > > > +#endif
> > > > > + if (ret != 0) {
> > > > > zcomp_destroy(comp);
> > > > > return NULL;
> > > > > }
> > > > > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > > > > index 8dc1d7f..d7e1229 100644
> > > > > --- a/drivers/block/zram/zcomp.h
> > > > > +++ b/drivers/block/zram/zcomp.h
> > > > > @@ -42,7 +42,7 @@ struct zcomp {
> > > > > void (*destroy)(struct zcomp *comp);
> > > > > };
> > > > >
> > > > > -struct zcomp *zcomp_create(const char *comp);
> > > > > +struct zcomp *zcomp_create(const char *comp, int num_strm);
> > > > > void zcomp_destroy(struct zcomp *comp);
> > > > >
> > > > > struct zcomp_strm *zcomp_strm_get(struct zcomp *comp);
> > > > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > > > > index 200c12a..c147e6f 100644
> > > > > --- a/drivers/block/zram/zram_drv.c
> > > > > +++ b/drivers/block/zram/zram_drv.c
> > > > > @@ -107,6 +107,42 @@ static ssize_t mem_used_total_show(struct device *dev,
> > > > > return sprintf(buf, "%llu\n", val);
> > > > > }
> > > > >
> > > > > +static ssize_t max_comp_streams_show(struct device *dev,
> > > > > + struct device_attribute *attr, char *buf)
> > > > > +{
> > > > > + long val;
> > > >
> > > > Why 'long'?
> > > >
> > > > > + struct zram *zram = dev_to_zram(dev);
> > > > > +
> > > > > + down_read(&zram->init_lock);
> > > > > + val = zram->max_comp_streams;
> > > > > + up_read(&zram->init_lock);
> > > > > +
> > > > > + return sprintf(buf, "%ld\n", val);
> > > > > +}
> > > > > +
> > > > > +static ssize_t max_comp_streams_store(struct device *dev,
> > > > > + struct device_attribute *attr, const char *buf, size_t len)
> > > > > +{
> > > > > +#ifdef CONFIG_ZRAM_MULTI_STREAM
> > > > > + long num;
> > > > > + struct zram *zram = dev_to_zram(dev);
> > > > > +
> > > > > + if (kstrtol(buf, 0, &num))
> > > > > + return -EINVAL;
> > > > > + if (num < 1)
> > > > > + return -EINVAL;
> > > > > + down_write(&zram->init_lock);
> > > > > + if (init_done(zram)) {
> > > > > + up_write(&zram->init_lock);
> > > > > + pr_info("Can't set max_comp_streams for initialized device\n");
> > > > > + return -EBUSY;
> > > > > + }
> > > > > + zram->max_comp_streams = num;
> > > > > + up_write(&zram->init_lock);
> > > >
> > > > If user decrease max stream in runtime, who care of freeing streams?
> > > >
> > >
> > > at the moment (to make it simpler), user can set max stream number
> > > only for un-initialised device. once zcomp created and initialised
> > > user cannot change max streams
> >
> > Hmm, I hope we have the abilitiy in the first place because I think
> > it's not complicate and make test very handy. Please.
> >
>
> ok, I'll take a look. I'm not sure that I'll finish it tonight.
> I need to think about it for a moment. probably it will require
> several additional counters in zcomp_strm_multi -- max_strms
> and num_strms (num_strms equals to number of currently allocated
> streams). user may decrease (for example) max streams while every
> available stream is busy (idle list is empty), so we need to check
> max_strms and avail_strms while we _put() stream. and we definitely
> don't want max_strms for zcomp_strm_multi to be less than 2 :-)
>
> so far I've address your notes and concerns -- no CONFIG option,
> both multi and single streams options, etc.
Thanks!
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCHv6 6/6] zram: document max_comp_streams
2014-02-21 11:50 [PATCHv6 0/6] add compressing abstraction and multi stream support Sergey Senozhatsky
` (4 preceding siblings ...)
2014-02-21 11:50 ` [PATCHv6 5/6] zram: enalbe multi stream compression support in zram Sergey Senozhatsky
@ 2014-02-21 11:50 ` Sergey Senozhatsky
5 siblings, 0 replies; 23+ messages in thread
From: Sergey Senozhatsky @ 2014-02-21 11:50 UTC (permalink / raw)
To: Minchan Kim
Cc: Jerome Marchand, Nitin Gupta, linux-kernel, Sergey Senozhatsky
Add max_comp_streams device attribute documentation.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
Documentation/ABI/testing/sysfs-block-zram | 9 ++++++++-
Documentation/blockdev/zram.txt | 24 +++++++++++++++++++-----
2 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-block-zram b/Documentation/ABI/testing/sysfs-block-zram
index 8aa0468..0da9ed6 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_comp_streams
+Date: February 2014
+Contact: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
+Description:
+ The max_comp_streams file is read-write and specifies the
+ number of backend's zcomp_strm compression streams (number of
+ concurrent compress 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..89b36d7 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -21,7 +21,21 @@ 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 streams
+ If ZRAM compiled with multi stream compression support, compression
+ backend may use up to max_comp_streams compression streams, thus
+ allowing up to max_comp_streams concurrent compression operations.
+ Otherwise, compression backend uses single compression stream and does
+ not change max_comp_streams value.
+
+ Examples:
+ #show max compression streams number
+ cat /sys/block/zram0/max_comp_streams
+
+ #set max compression streams number to 3
+ echo 3 > /sys/block/zram0/max_comp_streams
+
+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 +52,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 +74,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.291.g027825b
^ permalink raw reply related [flat|nested] 23+ messages in thread