* [PATCHv7 0/7] add compressing abstraction and multi stream support
@ 2014-02-25 11:34 Sergey Senozhatsky
2014-02-25 11:34 ` [PATCHv7 1/7] zram: introduce compressing backend abstraction Sergey Senozhatsky
` (6 more replies)
0 siblings, 7 replies; 13+ messages in thread
From: Sergey Senozhatsky @ 2014-02-25 11:34 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, Jerome Marchand, Nitin Gupta, linux-kernel,
Sergey Senozhatsky
This patchset introduces zcomp compression backend abstraction
adding ability to support compression algorithms other than LZO;
support for multi compression streams, making parallel compressions
possible.
v6->v7 (reviewed by Minchan Kim):
-- enable multi and single stream support out of the box (drop
ZRAM_MULTI_STREAM config option)
-- add set_max_stream knob, so we can adjust max number of compression
streams in runtime (for multi stream backend at the moment)
-- minor code `nitpicks'
v5->v6 (reviewed by Minchan Kim):
-- handle single compression stream case separately, using mutex locking,
to address perfomance regression
-- handle multi compression stream using spin lock and wait_event()/wake_up()
-- make multi compression stream support configurable (ZRAM_MULTI_STREAM
config option)
v4->v5 (reviewed by Minchan Kim):
-- renamed zcomp buffer_lock; removed src len and dst len from
compress() and decompress(); not using term `buffer' and
`workmem' in code and documentation; define compress() and
decompress() functions for LZO backend; not using goto's;
do not put idle zcomp_strm to idle list tail.
v3->v4 (reviewed by Minchan Kim):
-- renamed compression backend and working memory structs as requested
by Minchan Kim; fixed several issues noted by Minchan Kim.
Sergey Senozhatsky (7):
zram: introduce compressing backend abstraction
zram: use zcomp compressing backends
zram: factor out single stream compression
zram: add multi stream functionality
zram: enable multi stream compression support in zram
zram: add set_max_streams knob
zram: document max_comp_streams
Documentation/ABI/testing/sysfs-block-zram | 9 +-
Documentation/blockdev/zram.txt | 31 ++-
drivers/block/zram/Makefile | 2 +-
drivers/block/zram/zcomp.c | 324 +++++++++++++++++++++++++++++
drivers/block/zram/zcomp.h | 59 ++++++
drivers/block/zram/zcomp_lzo.c | 48 +++++
drivers/block/zram/zram_drv.c | 96 +++++----
drivers/block/zram/zram_drv.h | 10 +-
8 files changed, 532 insertions(+), 47 deletions(-)
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
--
1.9.0.291.g027825b
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv7 1/7] zram: introduce compressing backend abstraction
2014-02-25 11:34 [PATCHv7 0/7] add compressing abstraction and multi stream support Sergey Senozhatsky
@ 2014-02-25 11:34 ` Sergey Senozhatsky
2014-02-25 11:34 ` [PATCHv7 2/7] zram: use zcomp compressing backends Sergey Senozhatsky
` (5 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Sergey Senozhatsky @ 2014-02-25 11:34 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, 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)
Acked-by: Minchan Kim <minchan@kernel.org>
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..947efe3
--- /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) {
+ kfree(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] 13+ messages in thread
* [PATCHv7 2/7] zram: use zcomp compressing backends
2014-02-25 11:34 [PATCHv7 0/7] add compressing abstraction and multi stream support Sergey Senozhatsky
2014-02-25 11:34 ` [PATCHv7 1/7] zram: introduce compressing backend abstraction Sergey Senozhatsky
@ 2014-02-25 11:34 ` Sergey Senozhatsky
2014-02-25 11:34 ` [PATCHv7 3/7] zram: factor out single stream compression Sergey Senozhatsky
` (4 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Sergey Senozhatsky @ 2014-02-25 11:34 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, 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.
Acked-by: Minchan Kim <minchan@kernel.org>
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/block/zram/Makefile | 2 +-
drivers/block/zram/zram_drv.c | 59 ++++++++++++++++++-------------------------
drivers/block/zram/zram_drv.h | 8 +++---
3 files changed, 29 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..569ff58 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -29,7 +29,6 @@
#include <linux/genhd.h>
#include <linux/highmem.h>
#include <linux/slab.h>
-#include <linux/lzo.h>
#include <linux/string.h>
#include <linux/vmalloc.h>
@@ -38,6 +37,7 @@
/* 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 +160,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 +171,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 +185,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 +262,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 +282,12 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
if (size == PAGE_SIZE)
copy_page(mem, cmem);
else
- ret = lzo1x_decompress_safe(cmem, size, mem, &clen);
+ ret = zcomp_decompress(zram->comp, cmem, size, mem);
zs_unmap_object(meta->mem_pool, handle);
read_unlock(&meta->tb_lock);
/* Should NEVER happen. Return bio error if it does. */
- if (unlikely(ret != LZO_E_OK)) {
+ if (unlikely(ret)) {
pr_err("Decompression failed! err=%d, page=%u\n", ret, index);
atomic64_inc(&zram->stats.failed_reads);
return ret;
@@ -349,7 +330,7 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
ret = zram_decompress_page(zram, uncmem, index);
/* Should NEVER happen. Return bio error if it does. */
- if (unlikely(ret != LZO_E_OK))
+ if (unlikely(ret))
goto out_cleanup;
if (is_partial_io(bvec))
@@ -374,11 +355,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
struct page *page;
unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
struct zram_meta *meta = zram->meta;
+ struct zcomp_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 +374,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,22 +400,20 @@ 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;
if (is_partial_io(bvec))
src = uncmem;
}
@@ -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,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
zs_free(meta->mem_pool, handle);
}
+ zcomp_destroy(zram->comp);
zram_meta_free(zram->meta);
zram->meta = NULL;
/* Reset stats */
@@ -550,9 +530,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 %s compressing backend\n",
+ default_compressor);
+ return -EINVAL;
+ }
+
disksize = PAGE_ALIGN(disksize);
zram->meta = zram_meta_alloc(disksize);
if (!zram->meta) {
+ zcomp_destroy(zram->comp);
up_write(&zram->init_lock);
return -ENOMEM;
}
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] 13+ messages in thread
* [PATCHv7 3/7] zram: factor out single stream compression
2014-02-25 11:34 [PATCHv7 0/7] add compressing abstraction and multi stream support Sergey Senozhatsky
2014-02-25 11:34 ` [PATCHv7 1/7] zram: introduce compressing backend abstraction Sergey Senozhatsky
2014-02-25 11:34 ` [PATCHv7 2/7] zram: use zcomp compressing backends Sergey Senozhatsky
@ 2014-02-25 11:34 ` Sergey Senozhatsky
2014-02-26 4:34 ` Minchan Kim
2014-02-25 11:34 ` [PATCHv7 4/7] zram: add multi stream functionality Sergey Senozhatsky
` (3 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Sergey Senozhatsky @ 2014-02-25 11:34 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, 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 | 65 +++++++++++++++++++++++++++++++++++++++-------
drivers/block/zram/zcomp.h | 7 +++--
2 files changed, 61 insertions(+), 11 deletions(-)
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 947efe3..e20054b 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -15,6 +15,14 @@
#include "zcomp.h"
+/*
+ * single zcomp_strm backend
+ */
+struct zcomp_strm_single {
+ struct mutex strm_lock;
+ struct zcomp_strm *zstrm;
+};
+
extern struct zcomp_backend zcomp_lzo;
static struct zcomp_backend *find_backend(const char *compress)
@@ -55,17 +63,58 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
return zstrm;
}
+static struct zcomp_strm *zcomp_strm_single_get(struct zcomp *comp)
+{
+ struct zcomp_strm_single *zs = comp->stream;
+ mutex_lock(&zs->strm_lock);
+ return zs->zstrm;
+}
+
+static void zcomp_strm_single_put(struct zcomp *comp, struct zcomp_strm *zstrm)
+{
+ struct zcomp_strm_single *zs = comp->stream;
+ mutex_unlock(&zs->strm_lock);
+}
+
+static void zcomp_strm_single_destroy(struct zcomp *comp)
+{
+ struct zcomp_strm_single *zs = comp->stream;
+ zcomp_strm_free(comp, zs->zstrm);
+ kfree(zs);
+}
+
+static int zcomp_strm_single_create(struct zcomp *comp)
+{
+ struct zcomp_strm_single *zs;
+
+ comp->destroy = zcomp_strm_single_destroy;
+ comp->strm_get = zcomp_strm_single_get;
+ comp->strm_put = zcomp_strm_single_put;
+ zs = kmalloc(sizeof(struct zcomp_strm_single), GFP_KERNEL);
+ comp->stream = zs;
+ if (!zs)
+ return -ENOMEM;
+
+ mutex_init(&zs->strm_lock);
+ zs->zstrm = zcomp_strm_alloc(comp);
+ if (!zs->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,11 +155,8 @@ struct zcomp *zcomp_create(const char *compress)
return NULL;
comp->backend = backend;
- mutex_init(&comp->strm_lock);
-
- comp->zstrm = zcomp_strm_alloc(comp);
- if (!comp->zstrm) {
- kfree(comp);
+ if (zcomp_strm_single_create(comp) != 0) {
+ zcomp_destroy(comp);
return NULL;
}
return comp;
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 5106f8e..861e04d 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -34,9 +34,12 @@ struct zcomp_backend {
/* dynamic per-device compression frontend */
struct zcomp {
- struct mutex strm_lock;
- struct zcomp_strm *zstrm;
+ void *stream;
struct zcomp_backend *backend;
+
+ struct zcomp_strm *(*strm_get)(struct zcomp *comp);
+ void (*strm_put)(struct zcomp *comp, struct zcomp_strm *zstrm);
+ void (*destroy)(struct zcomp *comp);
};
struct zcomp *zcomp_create(const char *comp);
--
1.9.0.291.g027825b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv7 4/7] zram: add multi stream functionality
2014-02-25 11:34 [PATCHv7 0/7] add compressing abstraction and multi stream support Sergey Senozhatsky
` (2 preceding siblings ...)
2014-02-25 11:34 ` [PATCHv7 3/7] zram: factor out single stream compression Sergey Senozhatsky
@ 2014-02-25 11:34 ` Sergey Senozhatsky
2014-02-26 4:35 ` Minchan Kim
2014-02-25 11:34 ` [PATCHv7 5/7] zram: enable multi stream compression support in zram Sergey Senozhatsky
` (2 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Sergey Senozhatsky @ 2014-02-25 11:34 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, 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.
This is preparation patch, later patches will use this code.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/block/zram/zcomp.c | 119 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 118 insertions(+), 1 deletion(-)
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index e20054b..6f238f5 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -15,6 +15,8 @@
#include "zcomp.h"
+extern struct zcomp_backend zcomp_lzo;
+
/*
* single zcomp_strm backend
*/
@@ -23,7 +25,20 @@ struct zcomp_strm_single {
struct zcomp_strm *zstrm;
};
-extern struct zcomp_backend zcomp_lzo;
+/*
+ * multi zcomp_strm backend
+ */
+struct zcomp_strm_multi {
+ /* protect strm list */
+ spinlock_t strm_lock;
+ /* max possible number of zstrm streams */
+ int max_strm;
+ /* number of available zstrm streams */
+ atomic_t avail_strm;
+ /* list of available strms */
+ struct list_head idle_strm;
+ wait_queue_head_t strm_wait;
+};
static struct zcomp_backend *find_backend(const char *compress)
{
@@ -63,6 +78,108 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
return zstrm;
}
+/*
+ * get 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 *zs = comp->stream;
+ struct zcomp_strm *zstrm;
+
+ while (1) {
+ spin_lock(&zs->strm_lock);
+ if (!list_empty(&zs->idle_strm)) {
+ zstrm = list_entry(zs->idle_strm.next,
+ struct zcomp_strm, list);
+ list_del(&zstrm->list);
+ spin_unlock(&zs->strm_lock);
+ return zstrm;
+ }
+ /* zstrm streams limit reached, wait for idle stream */
+ if (atomic_read(&zs->avail_strm) >= zs->max_strm) {
+ spin_unlock(&zs->strm_lock);
+ wait_event(zs->strm_wait, !list_empty(&zs->idle_strm));
+ continue;
+ }
+ /* allocate new zstrm stream */
+ atomic_inc(&zs->avail_strm);
+ spin_unlock(&zs->strm_lock);
+
+ zstrm = zcomp_strm_alloc(comp);
+ if (!zstrm) {
+ atomic_dec(&zs->avail_strm);
+ wait_event(zs->strm_wait, !list_empty(&zs->idle_strm));
+ continue;
+ }
+ break;
+ }
+ return zstrm;
+}
+
+/* add stream back to idle list and wake up waiter or free the stream */
+static void zcomp_strm_multi_put(struct zcomp *comp, struct zcomp_strm *zstrm)
+{
+ struct zcomp_strm_multi *zs = comp->stream;
+
+ spin_lock(&zs->strm_lock);
+ if (atomic_read(&zs->avail_strm) <= zs->max_strm) {
+ list_add(&zstrm->list, &zs->idle_strm);
+ spin_unlock(&zs->strm_lock);
+ } else {
+ atomic_dec(&zs->avail_strm);
+ spin_unlock(&zs->strm_lock);
+ zcomp_strm_free(comp, zstrm);
+ }
+
+ wake_up(&zs->strm_wait);
+}
+
+static void zcomp_strm_multi_destroy(struct zcomp *comp)
+{
+ struct zcomp_strm_multi *zs = comp->stream;
+ struct zcomp_strm *zstrm;
+
+ while (!list_empty(&zs->idle_strm)) {
+ zstrm = list_entry(zs->idle_strm.next,
+ struct zcomp_strm, list);
+ list_del(&zstrm->list);
+ zcomp_strm_free(comp, zstrm);
+ }
+ kfree(zs);
+}
+
+static int zcomp_strm_multi_create(struct zcomp *comp, int num_strm)
+{
+ struct zcomp_strm *zstrm;
+ struct zcomp_strm_multi *zs;
+ int i;
+
+ comp->destroy = zcomp_strm_multi_destroy;
+ comp->strm_get = zcomp_strm_multi_get;
+ comp->strm_put = zcomp_strm_multi_put;
+ zs = kmalloc(sizeof(struct zcomp_strm_multi), GFP_KERNEL);
+ comp->stream = zs;
+ if (!zs)
+ return -ENOMEM;
+
+ spin_lock_init(&zs->strm_lock);
+ INIT_LIST_HEAD(&zs->idle_strm);
+ init_waitqueue_head(&zs->strm_wait);
+ zs->max_strm = num_strm;
+ atomic_set(&zs->avail_strm, num_strm);
+
+ 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, &zs->idle_strm);
+ }
+ return 0;
+}
+
static struct zcomp_strm *zcomp_strm_single_get(struct zcomp *comp)
{
struct zcomp_strm_single *zs = comp->stream;
--
1.9.0.291.g027825b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv7 5/7] zram: enable multi stream compression support in zram
2014-02-25 11:34 [PATCHv7 0/7] add compressing abstraction and multi stream support Sergey Senozhatsky
` (3 preceding siblings ...)
2014-02-25 11:34 ` [PATCHv7 4/7] zram: add multi stream functionality Sergey Senozhatsky
@ 2014-02-25 11:34 ` Sergey Senozhatsky
2014-02-26 4:35 ` Minchan Kim
2014-02-25 11:34 ` [PATCHv7 6/7] zram: add set_max_streams knob Sergey Senozhatsky
2014-02-25 11:34 ` [PATCHv7 7/7] zram: document max_comp_streams Sergey Senozhatsky
6 siblings, 1 reply; 13+ messages in thread
From: Sergey Senozhatsky @ 2014-02-25 11:34 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, Jerome Marchand, Nitin Gupta, linux-kernel,
Sergey Senozhatsky
1) Introduce zram device attribute max_comp_streams to show and store
current zcomp's max number of zcomp streams (num_strm).
2) 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).
max_comp_streams used during initialisation as follows:
-- 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).
default max_comp_streams value is 1, meaning that zram with single stream
will be initialised.
Later patch will introduce configuration knob to change max_comp_streams
on already initialised and used zcomp.
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/zcomp.c | 9 +++++++--
drivers/block/zram/zcomp.h | 2 +-
drivers/block/zram/zram_drv.c | 42 +++++++++++++++++++++++++++++++++++++++++-
drivers/block/zram/zram_drv.h | 2 +-
4 files changed, 50 insertions(+), 5 deletions(-)
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 6f238f5..1bcb70e 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -258,8 +258,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;
@@ -272,7 +273,11 @@ struct zcomp *zcomp_create(const char *compress)
return NULL;
comp->backend = backend;
- if (zcomp_strm_single_create(comp) != 0) {
+ if (num_strm > 1)
+ ret = zcomp_strm_multi_create(comp, num_strm);
+ else
+ ret = zcomp_strm_single_create(comp);
+ if (ret != 0) {
zcomp_destroy(comp);
return NULL;
}
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 861e04d..5514509 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 569ff58..42b9c7f 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -108,6 +108,40 @@ 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)
+{
+ int 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, "%d\n", val);
+}
+
+static ssize_t max_comp_streams_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ int num;
+ struct zram *zram = dev_to_zram(dev);
+
+ if (kstrtoint(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);
+ return len;
+}
+
/* flag operations needs meta->tb_lock */
static int zram_test_flag(struct zram_meta *meta, u32 index,
enum zram_pageflags flag)
@@ -502,6 +536,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
}
zcomp_destroy(zram->comp);
+ zram->max_comp_streams = 1;
+
zram_meta_free(zram->meta);
zram->meta = NULL;
/* Reset stats */
@@ -530,7 +566,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 %s compressing backend\n",
@@ -693,6 +729,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);
@@ -717,6 +755,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,
};
@@ -779,6 +818,7 @@ static int create_device(struct zram *zram, int device_id)
}
zram->meta = 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..ccf36d1 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 */
-
+ int max_comp_streams;
struct zram_stats stats;
};
#endif
--
1.9.0.291.g027825b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv7 6/7] zram: add set_max_streams knob
2014-02-25 11:34 [PATCHv7 0/7] add compressing abstraction and multi stream support Sergey Senozhatsky
` (4 preceding siblings ...)
2014-02-25 11:34 ` [PATCHv7 5/7] zram: enable multi stream compression support in zram Sergey Senozhatsky
@ 2014-02-25 11:34 ` Sergey Senozhatsky
2014-02-26 4:35 ` Minchan Kim
2014-02-25 11:34 ` [PATCHv7 7/7] zram: document max_comp_streams Sergey Senozhatsky
6 siblings, 1 reply; 13+ messages in thread
From: Sergey Senozhatsky @ 2014-02-25 11:34 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, Jerome Marchand, Nitin Gupta, linux-kernel,
Sergey Senozhatsky
This patch allows to change max_comp_streams on initialised zcomp.
Introduce zcomp set_max_streams() knob, zcomp_strm_multi_set_max_streams()
and zcomp_strm_single_set_max_streams() callbacks to change streams limit
for zcomp_strm_multi and zcomp_strm_single, accordingly. set_max_streams
for single steam zcomp does nothing.
If user has lowered the limit, then zcomp_strm_multi_set_max_streams()
attempts to immediately free extra streams (as much as it can, depending
on idle streams availability).
Note, this patch does not allow to change stream 'policy' from single to
multi stream (or vice versa) on already initialised compression backend,
this will be introduced later.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/block/zram/zcomp.c | 39 +++++++++++++++++++++++++++++++++++++++
drivers/block/zram/zcomp.h | 3 +++
drivers/block/zram/zram_drv.c | 7 ++-----
3 files changed, 44 insertions(+), 5 deletions(-)
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 1bcb70e..adfbfee 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -135,6 +135,32 @@ static void zcomp_strm_multi_put(struct zcomp *comp, struct zcomp_strm *zstrm)
wake_up(&zs->strm_wait);
}
+/* change max_strm limit */
+static void zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm)
+{
+ struct zcomp_strm_multi *zs = comp->stream;
+
+ /* single stream handled as a special case by zcomp_strm_single */
+ if (num_strm < 2)
+ return;
+
+ spin_lock(&zs->strm_lock);
+ zs->max_strm = num_strm;
+ /*
+ * if user has lowered the limit and there are idle streams,
+ * immediately free as much streams (and memory) as we can.
+ */
+ while (atomic_read(&zs->avail_strm) > num_strm &&
+ !list_empty(&zs->idle_strm)) {
+ struct zcomp_strm *zstrm = list_entry(zs->idle_strm.next,
+ struct zcomp_strm, list);
+ list_del(&zstrm->list);
+ zcomp_strm_free(comp, zstrm);
+ atomic_dec(&zs->avail_strm);
+ }
+ spin_unlock(&zs->strm_lock);
+}
+
static void zcomp_strm_multi_destroy(struct zcomp *comp)
{
struct zcomp_strm_multi *zs = comp->stream;
@@ -158,6 +184,7 @@ static int zcomp_strm_multi_create(struct zcomp *comp, int num_strm)
comp->destroy = zcomp_strm_multi_destroy;
comp->strm_get = zcomp_strm_multi_get;
comp->strm_put = zcomp_strm_multi_put;
+ comp->set_max_streams = zcomp_strm_multi_set_max_streams;
zs = kmalloc(sizeof(struct zcomp_strm_multi), GFP_KERNEL);
comp->stream = zs;
if (!zs)
@@ -193,6 +220,12 @@ static void zcomp_strm_single_put(struct zcomp *comp, struct zcomp_strm *zstrm)
mutex_unlock(&zs->strm_lock);
}
+static void zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm)
+{
+ /* zcomp_strm_single support only max_comp_streams == 1 */
+ return;
+}
+
static void zcomp_strm_single_destroy(struct zcomp *comp)
{
struct zcomp_strm_single *zs = comp->stream;
@@ -207,6 +240,7 @@ static int zcomp_strm_single_create(struct zcomp *comp)
comp->destroy = zcomp_strm_single_destroy;
comp->strm_get = zcomp_strm_single_get;
comp->strm_put = zcomp_strm_single_put;
+ comp->set_max_streams = zcomp_strm_single_set_max_streams;
zs = kmalloc(sizeof(struct zcomp_strm_single), GFP_KERNEL);
comp->stream = zs;
if (!zs)
@@ -221,6 +255,11 @@ static int zcomp_strm_single_create(struct zcomp *comp)
return 0;
}
+void zcomp_set_max_streams(struct zcomp *comp, int num_strm)
+{
+ comp->set_max_streams(comp, num_strm);
+}
+
struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
{
return comp->strm_get(comp);
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 5514509..9645e16 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -39,6 +39,7 @@ struct zcomp {
struct zcomp_strm *(*strm_get)(struct zcomp *comp);
void (*strm_put)(struct zcomp *comp, struct zcomp_strm *zstrm);
+ void (*set_max_streams)(struct zcomp *comp, int num_strm);
void (*destroy)(struct zcomp *comp);
};
@@ -53,4 +54,6 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
size_t src_len, unsigned char *dst);
+
+void zcomp_set_max_streams(struct zcomp *comp, int num_strm);
#endif /* _ZCOMP_H_ */
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 42b9c7f..34a0921 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -132,12 +132,9 @@ static ssize_t max_comp_streams_store(struct device *dev,
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;
+ if (init_done(zram))
+ zcomp_set_max_streams(zram->comp, num);
up_write(&zram->init_lock);
return len;
}
--
1.9.0.291.g027825b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv7 7/7] zram: document max_comp_streams
2014-02-25 11:34 [PATCHv7 0/7] add compressing abstraction and multi stream support Sergey Senozhatsky
` (5 preceding siblings ...)
2014-02-25 11:34 ` [PATCHv7 6/7] zram: add set_max_streams knob Sergey Senozhatsky
@ 2014-02-25 11:34 ` Sergey Senozhatsky
6 siblings, 0 replies; 13+ messages in thread
From: Sergey Senozhatsky @ 2014-02-25 11:34 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, 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 | 31 +++++++++++++++++++++++++-----
2 files changed, 34 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..d7f2f8f 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -21,7 +21,28 @@ 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
+ Compression backend may use up to max_comp_streams compression streams,
+ thus allowing up to max_comp_streams concurrent compression operations.
+ By default, compression backend uses single compression stream.
+
+ 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
+
+Note:
+In order to enable compression backend's multi stream support max_comp_streams must
+be initially set to desired concurrency level before ZRAM device initialisation.
+Once the device initialised as a single stream compression backend (max_comp_streams
+equals to 0) changing the value of max_comp_streams will not take any effect, because
+single stream compression backend implemented as a special case and does not support
+dynamic max_comp_streams. Multi stream backend supports dynamic max_comp_streams
+adjustment in range [2, N].
+
+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 +59,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 +81,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] 13+ messages in thread
* Re: [PATCHv7 3/7] zram: factor out single stream compression
2014-02-25 11:34 ` [PATCHv7 3/7] zram: factor out single stream compression Sergey Senozhatsky
@ 2014-02-26 4:34 ` Minchan Kim
2014-02-26 11:32 ` Sergey Senozhatsky
0 siblings, 1 reply; 13+ messages in thread
From: Minchan Kim @ 2014-02-26 4:34 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Andrew Morton, Jerome Marchand, Nitin Gupta, linux-kernel
Hello Sergey,
On Tue, Feb 25, 2014 at 02:34:29PM +0300, Sergey Senozhatsky wrote:
> This is preparation patch to add multi stream support to zcomp.
>
> Introduce struct zcomp_strm_single and a set of functions to manage zcomp_strm
> stream access. zcomp_strm_single implements single compession stream, same way
> as current zcomp implementation. This moves zcomp_strm stream control and
> locking from zcomp, so compressing backend zcomp is not aware of required
> locking (single and multi streams require different locking schemes).
Please, add why we need different locking scheme in here so that
some people understand why we need this via git log in future.
>
> The following set of functions added:
> - zcomp_strm_single_get()/zcomp_strm_single_put()
> get and put compression stream, implement required locking
> - zcomp_strm_single_create()/zcomp_strm_single_destroy()
> create and destroy zcomp_strm_single
>
> New ->strm_get() and ->strm_put() callbacks added to zcomp, which are set to
> zcomp_strm_single_get() and zcomp_strm_single_put() during initialisation.
> Instead of direct locking and zcomp_strm access from zcomp_strm_get() and
> zcomp_strm_put(), zcomp now calls ->strm_get() and ->strm_put()
> correspondingly.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> drivers/block/zram/zcomp.c | 65 +++++++++++++++++++++++++++++++++++++++-------
> drivers/block/zram/zcomp.h | 7 +++--
> 2 files changed, 61 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 947efe3..e20054b 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -15,6 +15,14 @@
>
> #include "zcomp.h"
>
> +/*
> + * single zcomp_strm backend
> + */
> +struct zcomp_strm_single {
> + struct mutex strm_lock;
> + struct zcomp_strm *zstrm;
> +};
> +
> extern struct zcomp_backend zcomp_lzo;
>
> static struct zcomp_backend *find_backend(const char *compress)
> @@ -55,17 +63,58 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
> return zstrm;
> }
>
> +static struct zcomp_strm *zcomp_strm_single_get(struct zcomp *comp)
> +{
> + struct zcomp_strm_single *zs = comp->stream;
> + mutex_lock(&zs->strm_lock);
> + return zs->zstrm;
> +}
> +
> +static void zcomp_strm_single_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> +{
> + struct zcomp_strm_single *zs = comp->stream;
> + mutex_unlock(&zs->strm_lock);
> +}
> +
> +static void zcomp_strm_single_destroy(struct zcomp *comp)
> +{
> + struct zcomp_strm_single *zs = comp->stream;
> + zcomp_strm_free(comp, zs->zstrm);
> + kfree(zs);
> +}
> +
> +static int zcomp_strm_single_create(struct zcomp *comp)
> +{
> + struct zcomp_strm_single *zs;
> +
> + comp->destroy = zcomp_strm_single_destroy;
> + comp->strm_get = zcomp_strm_single_get;
> + comp->strm_put = zcomp_strm_single_put;
> + zs = kmalloc(sizeof(struct zcomp_strm_single), GFP_KERNEL);
> + comp->stream = zs;
> + if (!zs)
> + return -ENOMEM;
Firstly check zs nullness and then assign zs to the comp->stream.
Yeb. your code doesn't have any problem but let's follow normal
convention.
> +
> + mutex_init(&zs->strm_lock);
> + zs->zstrm = zcomp_strm_alloc(comp);
> + if (!zs->zstrm) {
> + zcomp_strm_single_destroy(comp);
Let's just call kfree in here instead of xxx_destroy.
such pair function call is more clear to me rather than using wrapping
function.
> + return -ENOMEM;
> + }
> + return 0;
> +}
> +
> struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
> {
> - mutex_lock(&comp->strm_lock);
> - return comp->zstrm;
> + return comp->strm_get(comp);
> }
>
> void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> {
> - mutex_unlock(&comp->strm_lock);
> + comp->strm_put(comp, zstrm);
> }
>
> +/* compress page */
Function name is clear so I think we don't need a comment.
If we need such comment, it should introduce previous patches, not this one.
> int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> const unsigned char *src, size_t *dst_len)
> {
> @@ -73,6 +122,7 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> zstrm->private);
> }
>
> +/* decompress page */
> int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> size_t src_len, unsigned char *dst)
> {
> @@ -81,7 +131,7 @@ int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
>
> void zcomp_destroy(struct zcomp *comp)
> {
> - zcomp_strm_free(comp, comp->zstrm);
> + comp->destroy(comp);
> kfree(comp);
> }
>
> @@ -105,11 +155,8 @@ struct zcomp *zcomp_create(const char *compress)
> return NULL;
>
> comp->backend = backend;
> - mutex_init(&comp->strm_lock);
> -
> - comp->zstrm = zcomp_strm_alloc(comp);
> - if (!comp->zstrm) {
> - kfree(comp);
> + if (zcomp_strm_single_create(comp) != 0) {
> + zcomp_destroy(comp);
Let's use just kfree.
If zcomp_strm_single_create fails, it should tidy up all memory
allocated by itself so caller should just free thing allocate by
itself.
> return NULL;
> }
> return comp;
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index 5106f8e..861e04d 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -34,9 +34,12 @@ struct zcomp_backend {
>
> /* dynamic per-device compression frontend */
> struct zcomp {
> - struct mutex strm_lock;
> - struct zcomp_strm *zstrm;
> + void *stream;
> struct zcomp_backend *backend;
> +
> + struct zcomp_strm *(*strm_get)(struct zcomp *comp);
> + void (*strm_put)(struct zcomp *comp, struct zcomp_strm *zstrm);
> + void (*destroy)(struct zcomp *comp);
> };
>
> struct zcomp *zcomp_create(const char *comp);
> --
> 1.9.0.291.g027825b
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv7 4/7] zram: add multi stream functionality
2014-02-25 11:34 ` [PATCHv7 4/7] zram: add multi stream functionality Sergey Senozhatsky
@ 2014-02-26 4:35 ` Minchan Kim
0 siblings, 0 replies; 13+ messages in thread
From: Minchan Kim @ 2014-02-26 4:35 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Andrew Morton, Jerome Marchand, Nitin Gupta, linux-kernel
On Tue, Feb 25, 2014 at 02:34:30PM +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.
>
> This is preparation patch, later patches will use this code.
I'm not sure it's good to introduce preparation patch without code using
that but builded together because it could make build warning.
It surely made code review easy so I'm not sure what's the best method
to make handle this case.
When I see next patch, it's not complex so I think we could merge this
and next patchset.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> drivers/block/zram/zcomp.c | 119 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index e20054b..6f238f5 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -15,6 +15,8 @@
>
> #include "zcomp.h"
>
> +extern struct zcomp_backend zcomp_lzo;
> +
> /*
> * single zcomp_strm backend
> */
> @@ -23,7 +25,20 @@ struct zcomp_strm_single {
> struct zcomp_strm *zstrm;
> };
>
> -extern struct zcomp_backend zcomp_lzo;
> +/*
> + * multi zcomp_strm backend
> + */
> +struct zcomp_strm_multi {
> + /* protect strm list */
> + spinlock_t strm_lock;
> + /* max possible number of zstrm streams */
> + int max_strm;
> + /* number of available zstrm streams */
> + atomic_t avail_strm;
> + /* list of available strms */
> + struct list_head idle_strm;
> + wait_queue_head_t strm_wait;
> +};
>
> static struct zcomp_backend *find_backend(const char *compress)
> {
> @@ -63,6 +78,108 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
> return zstrm;
> }
>
> +/*
> + * get 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 *zs = comp->stream;
> + struct zcomp_strm *zstrm;
> +
> + while (1) {
> + spin_lock(&zs->strm_lock);
> + if (!list_empty(&zs->idle_strm)) {
> + zstrm = list_entry(zs->idle_strm.next,
> + struct zcomp_strm, list);
> + list_del(&zstrm->list);
> + spin_unlock(&zs->strm_lock);
> + return zstrm;
> + }
> + /* zstrm streams limit reached, wait for idle stream */
> + if (atomic_read(&zs->avail_strm) >= zs->max_strm) {
We need to declare it as atomic?
It seems you already protect it with spin_lock.
> + spin_unlock(&zs->strm_lock);
> + wait_event(zs->strm_wait, !list_empty(&zs->idle_strm));
> + continue;
> + }
> + /* allocate new zstrm stream */
> + atomic_inc(&zs->avail_strm);
> + spin_unlock(&zs->strm_lock);
> +
> + zstrm = zcomp_strm_alloc(comp);
> + if (!zstrm) {
> + atomic_dec(&zs->avail_strm);
If you use atomic due to this, it's not likely I guess so let's
use spin_lock in here, too then we could avoid atomic usage.
> + wait_event(zs->strm_wait, !list_empty(&zs->idle_strm));
> + continue;
> + }
> + break;
> + }
> + return zstrm;
> +}
> +
> +/* add stream back to idle list and wake up waiter or free the stream */
> +static void zcomp_strm_multi_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> +{
> + struct zcomp_strm_multi *zs = comp->stream;
> +
> + spin_lock(&zs->strm_lock);
> + if (atomic_read(&zs->avail_strm) <= zs->max_strm) {
> + list_add(&zstrm->list, &zs->idle_strm);
> + spin_unlock(&zs->strm_lock);
> + } else {
> + atomic_dec(&zs->avail_strm);
> + spin_unlock(&zs->strm_lock);
> + zcomp_strm_free(comp, zstrm);
> + }
> +
> + wake_up(&zs->strm_wait);
Nitpick:
Just wakeup only if we can return stream to idle list.
> +}
> +
> +static void zcomp_strm_multi_destroy(struct zcomp *comp)
> +{
> + struct zcomp_strm_multi *zs = comp->stream;
> + struct zcomp_strm *zstrm;
> +
> + while (!list_empty(&zs->idle_strm)) {
> + zstrm = list_entry(zs->idle_strm.next,
> + struct zcomp_strm, list);
> + list_del(&zstrm->list);
> + zcomp_strm_free(comp, zstrm);
> + }
> + kfree(zs);
> +}
> +
> +static int zcomp_strm_multi_create(struct zcomp *comp, int num_strm)
Let's use max_strm instead of num_strm.
> +{
> + struct zcomp_strm *zstrm;
> + struct zcomp_strm_multi *zs;
> + int i;
> +
> + comp->destroy = zcomp_strm_multi_destroy;
> + comp->strm_get = zcomp_strm_multi_get;
> + comp->strm_put = zcomp_strm_multi_put;
> + zs = kmalloc(sizeof(struct zcomp_strm_multi), GFP_KERNEL);
> + comp->stream = zs;
> + if (!zs)
> + return -ENOMEM;
> +
> + spin_lock_init(&zs->strm_lock);
> + INIT_LIST_HEAD(&zs->idle_strm);
> + init_waitqueue_head(&zs->strm_wait);
> + zs->max_strm = num_strm;
> + atomic_set(&zs->avail_strm, num_strm);
How about starting 1 stream rather than allocating max stream
in the first place?
> +
> + 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, &zs->idle_strm);
> + }
> + return 0;
> +}
> +
> static struct zcomp_strm *zcomp_strm_single_get(struct zcomp *comp)
> {
> struct zcomp_strm_single *zs = comp->stream;
> --
> 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] 13+ messages in thread
* Re: [PATCHv7 5/7] zram: enable multi stream compression support in zram
2014-02-25 11:34 ` [PATCHv7 5/7] zram: enable multi stream compression support in zram Sergey Senozhatsky
@ 2014-02-26 4:35 ` Minchan Kim
0 siblings, 0 replies; 13+ messages in thread
From: Minchan Kim @ 2014-02-26 4:35 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Andrew Morton, Jerome Marchand, Nitin Gupta, linux-kernel
On Tue, Feb 25, 2014 at 02:34:31PM +0300, Sergey Senozhatsky wrote:
> 1) Introduce zram device attribute max_comp_streams to show and store
> current zcomp's max number of zcomp streams (num_strm).
>
> 2) 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).
>
> max_comp_streams used during initialisation as follows:
> -- 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).
>
> default max_comp_streams value is 1, meaning that zram with single stream
> will be initialised.
>
> Later patch will introduce configuration knob to change max_comp_streams
> on already initialised and used zcomp.
>
> 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/zcomp.c | 9 +++++++--
> drivers/block/zram/zcomp.h | 2 +-
> drivers/block/zram/zram_drv.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> drivers/block/zram/zram_drv.h | 2 +-
> 4 files changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 6f238f5..1bcb70e 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -258,8 +258,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)
Let's use max_strm.
> {
> + int ret;
No need.
> struct zcomp *comp;
> struct zcomp_backend *backend;
>
> @@ -272,7 +273,11 @@ struct zcomp *zcomp_create(const char *compress)
> return NULL;
>
> comp->backend = backend;
> - if (zcomp_strm_single_create(comp) != 0) {
> + if (num_strm > 1)
> + ret = zcomp_strm_multi_create(comp, num_strm);
> + else
> + ret = zcomp_strm_single_create(comp);
> + if (ret != 0) {
> zcomp_destroy(comp);
> return NULL;
> }
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index 861e04d..5514509 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 569ff58..42b9c7f 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -108,6 +108,40 @@ 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)
> +{
> + int 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, "%d\n", val);
> +}
> +
> +static ssize_t max_comp_streams_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + int num;
> + struct zram *zram = dev_to_zram(dev);
> +
> + if (kstrtoint(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);
> + return len;
> +}
> +
> /* flag operations needs meta->tb_lock */
> static int zram_test_flag(struct zram_meta *meta, u32 index,
> enum zram_pageflags flag)
> @@ -502,6 +536,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> }
>
> zcomp_destroy(zram->comp);
> + zram->max_comp_streams = 1;
> +
> zram_meta_free(zram->meta);
> zram->meta = NULL;
> /* Reset stats */
> @@ -530,7 +566,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 %s compressing backend\n",
> @@ -693,6 +729,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);
> @@ -717,6 +755,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,
> };
>
> @@ -779,6 +818,7 @@ static int create_device(struct zram *zram, int device_id)
> }
>
> zram->meta = 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..ccf36d1 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 */
> -
> + int 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] 13+ messages in thread
* Re: [PATCHv7 6/7] zram: add set_max_streams knob
2014-02-25 11:34 ` [PATCHv7 6/7] zram: add set_max_streams knob Sergey Senozhatsky
@ 2014-02-26 4:35 ` Minchan Kim
0 siblings, 0 replies; 13+ messages in thread
From: Minchan Kim @ 2014-02-26 4:35 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Andrew Morton, Jerome Marchand, Nitin Gupta, linux-kernel
On Tue, Feb 25, 2014 at 02:34:32PM +0300, Sergey Senozhatsky wrote:
> This patch allows to change max_comp_streams on initialised zcomp.
>
> Introduce zcomp set_max_streams() knob, zcomp_strm_multi_set_max_streams()
> and zcomp_strm_single_set_max_streams() callbacks to change streams limit
> for zcomp_strm_multi and zcomp_strm_single, accordingly. set_max_streams
> for single steam zcomp does nothing.
>
> If user has lowered the limit, then zcomp_strm_multi_set_max_streams()
> attempts to immediately free extra streams (as much as it can, depending
> on idle streams availability).
>
> Note, this patch does not allow to change stream 'policy' from single to
> multi stream (or vice versa) on already initialised compression backend,
> this will be introduced later.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> drivers/block/zram/zcomp.c | 39 +++++++++++++++++++++++++++++++++++++++
> drivers/block/zram/zcomp.h | 3 +++
> drivers/block/zram/zram_drv.c | 7 ++-----
> 3 files changed, 44 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 1bcb70e..adfbfee 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -135,6 +135,32 @@ static void zcomp_strm_multi_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> wake_up(&zs->strm_wait);
> }
>
> +/* change max_strm limit */
> +static void zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm)
> +{
> + struct zcomp_strm_multi *zs = comp->stream;
> +
> + /* single stream handled as a special case by zcomp_strm_single */
> + if (num_strm < 2)
> + return;
I doubt it's good idea. Just leave it as you want although it makes
performance hurt because user want to get more free memory and it
doesn't make any regression for single stream side.
Additionally, if we enhance it with adaptive lock scheme,
performance problem would be gone. :)
But if you want to limit it really, at least, need to notice to
user. Maybe pr_info?
> +
> + spin_lock(&zs->strm_lock);
> + zs->max_strm = num_strm;
> + /*
> + * if user has lowered the limit and there are idle streams,
> + * immediately free as much streams (and memory) as we can.
> + */
> + while (atomic_read(&zs->avail_strm) > num_strm &&
> + !list_empty(&zs->idle_strm)) {
> + struct zcomp_strm *zstrm = list_entry(zs->idle_strm.next,
> + struct zcomp_strm, list);
> + list_del(&zstrm->list);
> + zcomp_strm_free(comp, zstrm);
> + atomic_dec(&zs->avail_strm);
> + }
> + spin_unlock(&zs->strm_lock);
> +}
> +
> static void zcomp_strm_multi_destroy(struct zcomp *comp)
> {
> struct zcomp_strm_multi *zs = comp->stream;
> @@ -158,6 +184,7 @@ static int zcomp_strm_multi_create(struct zcomp *comp, int num_strm)
> comp->destroy = zcomp_strm_multi_destroy;
> comp->strm_get = zcomp_strm_multi_get;
> comp->strm_put = zcomp_strm_multi_put;
> + comp->set_max_streams = zcomp_strm_multi_set_max_streams;
> zs = kmalloc(sizeof(struct zcomp_strm_multi), GFP_KERNEL);
> comp->stream = zs;
> if (!zs)
> @@ -193,6 +220,12 @@ static void zcomp_strm_single_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> mutex_unlock(&zs->strm_lock);
> }
>
> +static void zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm)
> +{
> + /* zcomp_strm_single support only max_comp_streams == 1 */
I hope we should notice to user.
Maybe this function returns true or false and caller should emit some message
with pr_info?
> + return;
> +}
> +
> static void zcomp_strm_single_destroy(struct zcomp *comp)
> {
> struct zcomp_strm_single *zs = comp->stream;
> @@ -207,6 +240,7 @@ static int zcomp_strm_single_create(struct zcomp *comp)
> comp->destroy = zcomp_strm_single_destroy;
> comp->strm_get = zcomp_strm_single_get;
> comp->strm_put = zcomp_strm_single_put;
> + comp->set_max_streams = zcomp_strm_single_set_max_streams;
> zs = kmalloc(sizeof(struct zcomp_strm_single), GFP_KERNEL);
> comp->stream = zs;
> if (!zs)
> @@ -221,6 +255,11 @@ static int zcomp_strm_single_create(struct zcomp *comp)
> return 0;
> }
>
> +void zcomp_set_max_streams(struct zcomp *comp, int num_strm)
> +{
> + comp->set_max_streams(comp, num_strm);
> +}
> +
> struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
> {
> return comp->strm_get(comp);
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index 5514509..9645e16 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -39,6 +39,7 @@ struct zcomp {
>
> struct zcomp_strm *(*strm_get)(struct zcomp *comp);
> void (*strm_put)(struct zcomp *comp, struct zcomp_strm *zstrm);
> + void (*set_max_streams)(struct zcomp *comp, int num_strm);
> void (*destroy)(struct zcomp *comp);
> };
>
> @@ -53,4 +54,6 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
>
> int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> size_t src_len, unsigned char *dst);
> +
> +void zcomp_set_max_streams(struct zcomp *comp, int num_strm);
> #endif /* _ZCOMP_H_ */
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 42b9c7f..34a0921 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -132,12 +132,9 @@ static ssize_t max_comp_streams_store(struct device *dev,
> 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;
> + if (init_done(zram))
> + zcomp_set_max_streams(zram->comp, num);
> up_write(&zram->init_lock);
> return len;
> }
> --
> 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] 13+ messages in thread
* Re: [PATCHv7 3/7] zram: factor out single stream compression
2014-02-26 4:34 ` Minchan Kim
@ 2014-02-26 11:32 ` Sergey Senozhatsky
0 siblings, 0 replies; 13+ messages in thread
From: Sergey Senozhatsky @ 2014-02-26 11:32 UTC (permalink / raw)
To: Minchan Kim
Cc: Sergey Senozhatsky, Andrew Morton, Jerome Marchand, Nitin Gupta,
linux-kernel
Hello Minchan,
On (02/26/14 13:34), Minchan Kim wrote:
> Hello Sergey,
>
> On Tue, Feb 25, 2014 at 02:34:29PM +0300, Sergey Senozhatsky wrote:
> > This is preparation patch to add multi stream support to zcomp.
> >
> > Introduce struct zcomp_strm_single and a set of functions to manage zcomp_strm
> > stream access. zcomp_strm_single implements single compession stream, same way
> > as current zcomp implementation. This moves zcomp_strm stream control and
> > locking from zcomp, so compressing backend zcomp is not aware of required
> > locking (single and multi streams require different locking schemes).
>
> Please, add why we need different locking scheme in here so that
> some people understand why we need this via git log in future.
>
addressed all of your notes, nitpicks and concerns in v8. thanks.
-ss
> >
> > The following set of functions added:
> > - zcomp_strm_single_get()/zcomp_strm_single_put()
> > get and put compression stream, implement required locking
> > - zcomp_strm_single_create()/zcomp_strm_single_destroy()
> > create and destroy zcomp_strm_single
> >
> > New ->strm_get() and ->strm_put() callbacks added to zcomp, which are set to
> > zcomp_strm_single_get() and zcomp_strm_single_put() during initialisation.
> > Instead of direct locking and zcomp_strm access from zcomp_strm_get() and
> > zcomp_strm_put(), zcomp now calls ->strm_get() and ->strm_put()
> > correspondingly.
> >
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> > drivers/block/zram/zcomp.c | 65 +++++++++++++++++++++++++++++++++++++++-------
> > drivers/block/zram/zcomp.h | 7 +++--
> > 2 files changed, 61 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> > index 947efe3..e20054b 100644
> > --- a/drivers/block/zram/zcomp.c
> > +++ b/drivers/block/zram/zcomp.c
> > @@ -15,6 +15,14 @@
> >
> > #include "zcomp.h"
> >
> > +/*
> > + * single zcomp_strm backend
> > + */
> > +struct zcomp_strm_single {
> > + struct mutex strm_lock;
> > + struct zcomp_strm *zstrm;
> > +};
> > +
> > extern struct zcomp_backend zcomp_lzo;
> >
> > static struct zcomp_backend *find_backend(const char *compress)
> > @@ -55,17 +63,58 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
> > return zstrm;
> > }
> >
> > +static struct zcomp_strm *zcomp_strm_single_get(struct zcomp *comp)
> > +{
> > + struct zcomp_strm_single *zs = comp->stream;
> > + mutex_lock(&zs->strm_lock);
> > + return zs->zstrm;
> > +}
> > +
> > +static void zcomp_strm_single_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> > +{
> > + struct zcomp_strm_single *zs = comp->stream;
> > + mutex_unlock(&zs->strm_lock);
> > +}
> > +
> > +static void zcomp_strm_single_destroy(struct zcomp *comp)
> > +{
> > + struct zcomp_strm_single *zs = comp->stream;
> > + zcomp_strm_free(comp, zs->zstrm);
> > + kfree(zs);
> > +}
> > +
> > +static int zcomp_strm_single_create(struct zcomp *comp)
> > +{
> > + struct zcomp_strm_single *zs;
> > +
> > + comp->destroy = zcomp_strm_single_destroy;
> > + comp->strm_get = zcomp_strm_single_get;
> > + comp->strm_put = zcomp_strm_single_put;
> > + zs = kmalloc(sizeof(struct zcomp_strm_single), GFP_KERNEL);
> > + comp->stream = zs;
> > + if (!zs)
> > + return -ENOMEM;
>
> Firstly check zs nullness and then assign zs to the comp->stream.
> Yeb. your code doesn't have any problem but let's follow normal
> convention.
>
> > +
> > + mutex_init(&zs->strm_lock);
> > + zs->zstrm = zcomp_strm_alloc(comp);
> > + if (!zs->zstrm) {
> > + zcomp_strm_single_destroy(comp);
>
> Let's just call kfree in here instead of xxx_destroy.
> such pair function call is more clear to me rather than using wrapping
> function.
>
>
> > + return -ENOMEM;
> > + }
> > + return 0;
> > +}
> > +
> > struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
> > {
> > - mutex_lock(&comp->strm_lock);
> > - return comp->zstrm;
> > + return comp->strm_get(comp);
> > }
> >
> > void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> > {
> > - mutex_unlock(&comp->strm_lock);
> > + comp->strm_put(comp, zstrm);
> > }
> >
> > +/* compress page */
>
> Function name is clear so I think we don't need a comment.
> If we need such comment, it should introduce previous patches, not this one.
>
> > int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > const unsigned char *src, size_t *dst_len)
> > {
> > @@ -73,6 +122,7 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > zstrm->private);
> > }
> >
> > +/* decompress page */
> > int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> > size_t src_len, unsigned char *dst)
> > {
> > @@ -81,7 +131,7 @@ int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> >
> > void zcomp_destroy(struct zcomp *comp)
> > {
> > - zcomp_strm_free(comp, comp->zstrm);
> > + comp->destroy(comp);
> > kfree(comp);
> > }
> >
> > @@ -105,11 +155,8 @@ struct zcomp *zcomp_create(const char *compress)
> > return NULL;
> >
> > comp->backend = backend;
> > - mutex_init(&comp->strm_lock);
> > -
> > - comp->zstrm = zcomp_strm_alloc(comp);
> > - if (!comp->zstrm) {
> > - kfree(comp);
> > + if (zcomp_strm_single_create(comp) != 0) {
> > + zcomp_destroy(comp);
>
> Let's use just kfree.
> If zcomp_strm_single_create fails, it should tidy up all memory
> allocated by itself so caller should just free thing allocate by
> itself.
>
> > return NULL;
> > }
> > return comp;
> > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > index 5106f8e..861e04d 100644
> > --- a/drivers/block/zram/zcomp.h
> > +++ b/drivers/block/zram/zcomp.h
> > @@ -34,9 +34,12 @@ struct zcomp_backend {
> >
> > /* dynamic per-device compression frontend */
> > struct zcomp {
> > - struct mutex strm_lock;
> > - struct zcomp_strm *zstrm;
> > + void *stream;
> > struct zcomp_backend *backend;
> > +
> > + struct zcomp_strm *(*strm_get)(struct zcomp *comp);
> > + void (*strm_put)(struct zcomp *comp, struct zcomp_strm *zstrm);
> > + void (*destroy)(struct zcomp *comp);
> > };
> >
> > struct zcomp *zcomp_create(const char *comp);
> > --
> > 1.9.0.291.g027825b
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
> --
> Kind regards,
> Minchan Kim
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-02-26 11:36 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-25 11:34 [PATCHv7 0/7] add compressing abstraction and multi stream support Sergey Senozhatsky
2014-02-25 11:34 ` [PATCHv7 1/7] zram: introduce compressing backend abstraction Sergey Senozhatsky
2014-02-25 11:34 ` [PATCHv7 2/7] zram: use zcomp compressing backends Sergey Senozhatsky
2014-02-25 11:34 ` [PATCHv7 3/7] zram: factor out single stream compression Sergey Senozhatsky
2014-02-26 4:34 ` Minchan Kim
2014-02-26 11:32 ` Sergey Senozhatsky
2014-02-25 11:34 ` [PATCHv7 4/7] zram: add multi stream functionality Sergey Senozhatsky
2014-02-26 4:35 ` Minchan Kim
2014-02-25 11:34 ` [PATCHv7 5/7] zram: enable multi stream compression support in zram Sergey Senozhatsky
2014-02-26 4:35 ` Minchan Kim
2014-02-25 11:34 ` [PATCHv7 6/7] zram: add set_max_streams knob Sergey Senozhatsky
2014-02-26 4:35 ` Minchan Kim
2014-02-25 11:34 ` [PATCHv7 7/7] zram: document max_comp_streams Sergey Senozhatsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.