All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHv4 0/4] add compressing abstraction and multi stream support
@ 2014-02-12 19:39 Sergey Senozhatsky
  2014-02-12 19:39 ` Sergey Senozhatsky
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2014-02-12 19:39 UTC (permalink / raw)
  To: Minchan Kim
  Cc: 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.

Diff from v3:
-- renamed compression backend and working memory structs as requested
   by Minchan Kim; fixed several issues noted by Minchan Kim.

Sergey Senozhatsky (4):
  zram: introduce compressing backend abstraction
  zram: use zcomp compressing backends
  zram: support multi compressing buffers
  zram: document max_comp_streams

 Documentation/ABI/testing/sysfs-block-zram |   9 +-
 Documentation/blockdev/zram.txt            |  21 +++-
 drivers/block/zram/Makefile                |   2 +-
 drivers/block/zram/zcomp.c                 | 156 +++++++++++++++++++++++++++++
 drivers/block/zram/zcomp.h                 |  58 +++++++++++
 drivers/block/zram/zcomp_lzo.c             |  33 ++++++
 drivers/block/zram/zram_drv.c              | 102 ++++++++++++-------
 drivers/block/zram/zram_drv.h              |  10 +-
 8 files changed, 345 insertions(+), 46 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.rc3.256.g4af3ebd


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCHv4 0/4] add compressing abstraction and multi stream support
  2014-02-12 19:39 [PATCHv4 0/4] add compressing abstraction and multi stream support Sergey Senozhatsky
@ 2014-02-12 19:39 ` Sergey Senozhatsky
  2014-02-12 19:39 ` [PATCHv4 1/4] zram: introduce compressing backend abstraction Sergey Senozhatsky
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2014-02-12 19:39 UTC (permalink / raw)
  To: Minchan Kim
  Cc: 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.

Diff from v3:
-- renamed compression backend and working memory structs as requested
   by Minchan Kim; fixed several issues noted by Minchan Kim.

Sergey Senozhatsky (4):
  zram: introduce compressing backend abstraction
  zram: use zcomp compressing backends
  zram: support multi compressing buffers
  zram: document max_comp_streams

 Documentation/ABI/testing/sysfs-block-zram |   9 +-
 Documentation/blockdev/zram.txt            |  21 +++-
 drivers/block/zram/Makefile                |   2 +-
 drivers/block/zram/zcomp.c                 | 156 +++++++++++++++++++++++++++++
 drivers/block/zram/zcomp.h                 |  58 +++++++++++
 drivers/block/zram/zcomp_lzo.c             |  33 ++++++
 drivers/block/zram/zram_drv.c              | 102 ++++++++++++-------
 drivers/block/zram/zram_drv.h              |  10 +-
 8 files changed, 345 insertions(+), 46 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.rc3.256.g4af3ebd


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCHv4 1/4] zram: introduce compressing backend abstraction
  2014-02-12 19:39 [PATCHv4 0/4] add compressing abstraction and multi stream support Sergey Senozhatsky
  2014-02-12 19:39 ` Sergey Senozhatsky
@ 2014-02-12 19:39 ` Sergey Senozhatsky
  2014-02-13  4:53   ` Minchan Kim
  2014-02-12 19:39 ` [PATCHv4 2/4] zram: use zcomp compressing backends Sergey Senozhatsky
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Sergey Senozhatsky @ 2014-02-12 19:39 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. Besides, zram holds buffer_lock almost through the whole write()
function, making parallel compression impossible. To remove requirement
a) new struct zcomp_strm introduced, which contain buffers required by
compression algorithm. 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' buffer.

Every zcomp has a list of idle zcomp_strm buffers, spinlock to protect idle
list and wait queue, making it possible to perform parallel compressions.
Each time zram issues a zcomp_strm_get() call, the following set of operations
performed:
- spin lock buffer_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_put() caller.

zcomp_strm_put():
- spin lock buffer_lock
- add zcomp stream to idle list
- spin unlock, wake up sleeper

In other words, zcomp_strm_get() turns caller into exclusive user of a stream
and zcomp_strm_put() makes a particular zcomp stream available.

Usage examples.

To initialize compressing backend:
	comp = zcomp_create(NAME) /* NAME e.g. "lzo" */

which initialises compressing backend if requested algorithm is supported.

Compress:
	zstrm = zcomp_strm_get(comp)
	zcomp_compress(comp, zstrm, src, src_len, &dst_len)
	[..] /* copy compressed data */
	zcomp_strm_put(comp, zstrm)

Decompress:
	zcomp_decompress(comp, src, src_len, dst, &dst_len);

Free compessing backend and its zcomp stream:
	zcomp_destroy(comp)

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zcomp.c     | 153 +++++++++++++++++++++++++++++++++++++++++
 drivers/block/zram/zcomp.h     |  57 +++++++++++++++
 drivers/block/zram/zcomp_lzo.c |  33 +++++++++
 3 files changed, 243 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..3b4920f
--- /dev/null
+++ b/drivers/block/zram/zcomp.c
@@ -0,0 +1,153 @@
+/*
+ * Copyright (C) 2014 Sergey Senozhatsky.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+#include <linux/wait.h>
+#include <linux/sched.h>
+
+#include "zcomp.h"
+
+extern struct zcomp_backend zcomp_lzo;
+
+static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm)
+{
+	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;
+
+	INIT_LIST_HEAD(&zstrm->list);
+	/* algorithm specific working memory buffer */
+	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)
+		goto fail;
+
+	return zstrm;
+fail:
+	zcomp_strm_free(comp, zstrm);
+	return NULL;
+}
+
+/*
+ * get existing idle zcomp_strm or wait until other process release
+ * (zcomp_strm_put()) one for us
+ */
+struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
+{
+	struct zcomp_strm *zstrm;
+retry:
+	spin_lock(&comp->buffer_lock);
+	if (list_empty(&comp->idle_strm)) {
+		spin_unlock(&comp->buffer_lock);
+		wait_event(comp->strm_wait,
+				!list_empty(&comp->idle_strm));
+		goto retry;
+	}
+
+	zstrm = list_entry(comp->idle_strm.next,
+			struct zcomp_strm, list);
+	list_del(&zstrm->list);
+	spin_unlock(&comp->buffer_lock);
+	return zstrm;
+}
+
+/* add zcomp_strm back to idle list and wake up waiter (if any) */
+void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm)
+{
+	spin_lock(&comp->buffer_lock);
+	list_add_tail(&zstrm->list, &comp->idle_strm);
+	spin_unlock(&comp->buffer_lock);
+
+	wake_up(&comp->strm_wait);
+}
+
+int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
+		const unsigned char *src, size_t src_len, size_t *dst_len)
+{
+	return comp->backend->compress(src, src_len, zstrm->buffer,
+			dst_len, zstrm->private);
+}
+
+int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
+		size_t src_len, unsigned char *dst, size_t *dst_len)
+{
+	return comp->backend->decompress(src, src_len, dst, dst_len);
+}
+
+/* free allocated zcomp_strm buffers and zcomp */
+void zcomp_destroy(struct zcomp *comp)
+{
+	struct zcomp_strm *zstrm;
+	while (!list_empty(&comp->idle_strm)) {
+		zstrm = list_entry(comp->idle_strm.next,
+				struct zcomp_strm, list);
+		list_del(&zstrm->list);
+		zcomp_strm_free(comp, zstrm);
+	}
+	kfree(comp);
+}
+
+static struct zcomp_backend *find_backend(const char *compress)
+{
+	if (sysfs_streq(compress, "lzo"))
+		return &zcomp_lzo;
+	return NULL;
+}
+
+/*
+ * 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;
+	struct zcomp_strm *zstrm;
+
+	backend = find_backend(compress);
+	if (!backend)
+		return NULL;
+
+	comp = kmalloc(sizeof(struct zcomp), GFP_KERNEL);
+	if (!comp)
+		return NULL;
+
+	comp->backend = backend;
+	spin_lock_init(&comp->buffer_lock);
+	INIT_LIST_HEAD(&comp->idle_strm);
+	init_waitqueue_head(&comp->strm_wait);
+
+	zstrm = zcomp_strm_alloc(comp);
+	if (!zstrm) {
+		zcomp_destroy(comp);
+		return NULL;
+	}
+	list_add_tail(&zstrm->list, &comp->idle_strm);
+	return comp;
+}
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
new file mode 100644
index 0000000..213e503
--- /dev/null
+++ b/drivers/block/zram/zcomp.h
@@ -0,0 +1,57 @@
+/*
+ * 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/types.h>
+#include <linux/spinlock.h>
+
+struct zcomp_strm {
+	void *buffer;     /* compression/decompression buffer */
+	void *private;      /* algorithm workmem */
+	struct list_head list;
+};
+
+/* static compression backend */
+struct zcomp_backend {
+	int (*compress)(const unsigned char *src, size_t src_len,
+			unsigned char *dst, size_t *dst_len, void *wrkmem);
+
+	int (*decompress)(const unsigned char *src, size_t src_len,
+			unsigned char *dst, size_t *dst_len);
+
+	void * (*create)(void);
+	void (*destroy)(void *private);
+
+	const char *name;
+};
+
+/* dynamic per-device compression frontend */
+struct zcomp {
+	/* protect strm list */
+	spinlock_t buffer_lock;
+	/* list of available strms */
+	struct list_head idle_strm;
+	wait_queue_head_t strm_wait;
+	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 *zsrtm);
+
+int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
+		const unsigned char *src, size_t src_len, size_t *dst_len);
+
+int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
+		size_t src_len, unsigned char *dst, size_t *dst_len);
+#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..2b23771
--- /dev/null
+++ b/drivers/block/zram/zcomp_lzo.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2014 Sergey Senozhatsky.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/lzo.h>
+
+#include "zcomp.h"
+
+static void * lzo_create(void)
+{
+	return kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
+}
+
+static void lzo_destroy(void *private)
+{
+	kfree(private);
+}
+
+extern struct zcomp_backend zcomp_lzo;
+struct zcomp_backend zcomp_lzo = {
+	.compress = lzo1x_1_compress,
+	.decompress = lzo1x_decompress_safe,
+	.create = lzo_create,
+	.destroy = lzo_destroy,
+	.name = "lzo",
+};
-- 
1.9.0.rc3.256.g4af3ebd


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCHv4 2/4] zram: use zcomp compressing backends
  2014-02-12 19:39 [PATCHv4 0/4] add compressing abstraction and multi stream support Sergey Senozhatsky
  2014-02-12 19:39 ` Sergey Senozhatsky
  2014-02-12 19:39 ` [PATCHv4 1/4] zram: introduce compressing backend abstraction Sergey Senozhatsky
@ 2014-02-12 19:39 ` Sergey Senozhatsky
  2014-02-13  5:04   ` Minchan Kim
  2014-02-12 19:39 ` [PATCHv4 3/4] zram: support multi compressing buffers Sergey Senozhatsky
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Sergey Senozhatsky @ 2014-02-12 19:39 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 instead.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/Makefile   |  2 +-
 drivers/block/zram/zram_drv.c | 62 +++++++++++++++++++------------------------
 drivers/block/zram/zram_drv.h |  8 +++---
 3 files changed, 33 insertions(+), 39 deletions(-)

diff --git a/drivers/block/zram/Makefile b/drivers/block/zram/Makefile
index cb0f9ce..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..d0d7254 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -29,15 +29,14 @@
 #include <linux/genhd.h>
 #include <linux/highmem.h>
 #include <linux/slab.h>
-#include <linux/lzo.h>
 #include <linux/string.h>
-#include <linux/vmalloc.h>
 
 #include "zram_drv.h"
 
 /* Globals */
 static int zram_major;
 static struct zram *zram_devices;
+static const char *default_compressor = "lzo";
 
 /* Module params (documentation at end) */
 static unsigned int num_devices = 1;
@@ -160,8 +159,6 @@ static inline int valid_io_request(struct zram *zram, struct bio *bio)
 static void zram_meta_free(struct zram_meta *meta)
 {
 	zs_destroy_pool(meta->mem_pool);
-	kfree(meta->compress_workmem);
-	free_pages((unsigned long)meta->compress_buffer, 1);
 	vfree(meta->table);
 	kfree(meta);
 }
@@ -173,22 +170,11 @@ static struct zram_meta *zram_meta_alloc(u64 disksize)
 	if (!meta)
 		goto out;
 
-	meta->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
-	if (!meta->compress_workmem)
-		goto free_meta;
-
-	meta->compress_buffer =
-		(void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
-	if (!meta->compress_buffer) {
-		pr_err("Error allocating compressor buffer space\n");
-		goto free_workmem;
-	}
-
 	num_pages = disksize >> PAGE_SHIFT;
 	meta->table = vzalloc(num_pages * sizeof(*meta->table));
 	if (!meta->table) {
 		pr_err("Error allocating zram address table\n");
-		goto free_buffer;
+		goto free_meta;
 	}
 
 	meta->mem_pool = zs_create_pool(GFP_NOIO | __GFP_HIGHMEM);
@@ -198,15 +184,10 @@ static struct zram_meta *zram_meta_alloc(u64 disksize)
 	}
 
 	rwlock_init(&meta->tb_lock);
-	mutex_init(&meta->buffer_lock);
 	return meta;
 
 free_table:
 	vfree(meta->table);
-free_buffer:
-	free_pages((unsigned long)meta->compress_buffer, 1);
-free_workmem:
-	kfree(meta->compress_workmem);
 free_meta:
 	kfree(meta);
 	meta = NULL;
@@ -280,7 +261,7 @@ static void zram_free_page(struct zram *zram, size_t index)
 
 static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 {
-	int ret = LZO_E_OK;
+	int ret = 0;
 	size_t clen = PAGE_SIZE;
 	unsigned char *cmem;
 	struct zram_meta *meta = zram->meta;
@@ -301,12 +282,12 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 	if (size == PAGE_SIZE)
 		copy_page(mem, cmem);
 	else
-		ret = lzo1x_decompress_safe(cmem, size,	mem, &clen);
+		ret = zcomp_decompress(zram->comp, cmem, size, mem, &clen);
 	zs_unmap_object(meta->mem_pool, handle);
 	read_unlock(&meta->tb_lock);
 
 	/* Should NEVER happen. Return bio error if it does. */
-	if (unlikely(ret != LZO_E_OK)) {
+	if (unlikely(ret)) {
 		pr_err("Decompression failed! err=%d, page=%u\n", ret, index);
 		atomic64_inc(&zram->stats.failed_reads);
 		return ret;
@@ -349,7 +330,7 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
 
 	ret = zram_decompress_page(zram, uncmem, index);
 	/* Should NEVER happen. Return bio error if it does. */
-	if (unlikely(ret != LZO_E_OK))
+	if (unlikely(ret))
 		goto out_cleanup;
 
 	if (is_partial_io(bvec))
@@ -374,11 +355,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	struct page *page;
 	unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
 	struct zram_meta *meta = zram->meta;
+	struct zcomp_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,19 +400,18 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		goto out;
 	}
 
-	ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
-			       meta->compress_workmem);
+	ret = zcomp_compress(zram->comp, zstrm, uncmem, bvec->bv_len, &clen);
 	if (!is_partial_io(bvec)) {
 		kunmap_atomic(user_mem);
 		user_mem = NULL;
 		uncmem = NULL;
 	}
 
-	if (unlikely(ret != LZO_E_OK)) {
+	if (unlikely(ret)) {
 		pr_err("Compression failed! err=%d\n", ret);
 		goto out;
 	}
-
+	src = zstrm->buffer;
 	if (unlikely(clen > max_zpage_size)) {
 		clen = PAGE_SIZE;
 		src = NULL;
@@ -457,6 +436,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		memcpy(cmem, src, clen);
 	}
 
+	zcomp_strm_put(zram->comp, zstrm);
+	locked = false;
 	zs_unmap_object(meta->mem_pool, handle);
 
 	/*
@@ -475,10 +456,9 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	atomic64_inc(&zram->stats.pages_stored);
 out:
 	if (locked)
-		mutex_unlock(&meta->buffer_lock);
+		zcomp_strm_put(zram->comp, zstrm);
 	if (is_partial_io(bvec))
 		kfree(uncmem);
-
 	if (ret)
 		atomic64_inc(&zram->stats.failed_writes);
 	return ret;
@@ -522,6 +502,10 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
 		zs_free(meta->mem_pool, handle);
 	}
 
+	if (zram->comp)
+		zcomp_destroy(zram->comp);
+	zram->comp = NULL;
+
 	zram_meta_free(zram->meta);
 	zram->meta = NULL;
 	/* Reset stats */
@@ -550,9 +534,18 @@ static ssize_t disksize_store(struct device *dev,
 		return -EBUSY;
 	}
 
+	zram->comp = zcomp_create(default_compressor);
+	if (!zram->comp) {
+		up_write(&zram->init_lock);
+		pr_info("Cannot initialise compressing backend\n");
+		return -EINVAL;
+	}
+
 	disksize = PAGE_ALIGN(disksize);
 	zram->meta = zram_meta_alloc(disksize);
 	if (!zram->meta) {
+		zcomp_destroy(zram->comp);
+		zram->comp = NULL;
 		up_write(&zram->init_lock);
 		return -ENOMEM;
 	}
@@ -790,6 +783,7 @@ static int create_device(struct zram *zram, int device_id)
 	}
 
 	zram->meta = NULL;
+	zram->comp = NULL;
 	return 0;
 
 out_free_disk:
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 1d5b1f5..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.rc3.256.g4af3ebd


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCHv4 3/4] zram: support multi compressing buffers
  2014-02-12 19:39 [PATCHv4 0/4] add compressing abstraction and multi stream support Sergey Senozhatsky
                   ` (2 preceding siblings ...)
  2014-02-12 19:39 ` [PATCHv4 2/4] zram: use zcomp compressing backends Sergey Senozhatsky
@ 2014-02-12 19:39 ` Sergey Senozhatsky
  2014-02-12 19:39 ` [PATCHv4 4/4] zram: document max_comp_streams Sergey Senozhatsky
  2014-02-17  4:56 ` [PATCHv4 0/4] add compressing abstraction and multi stream support Minchan Kim
  5 siblings, 0 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2014-02-12 19:39 UTC (permalink / raw)
  To: Minchan Kim
  Cc: Jerome Marchand, Nitin Gupta, linux-kernel, Sergey Senozhatsky

1) Extend zcomp zcomp_create() with `num_strm' parameter.
`num_strm' limits the number of zcomp_strm structs in compression
backend's idle list (max buffers).

2) Introduce zram device attribute max_comp_streams to show and
store current device's zcomp max number of zcomp streams (num_strm).

Usage example:
        echo 4 > /sys/block/zram0/max_comp_streams

set max_comp_streams to 4

        cat /sys/block/zram0/max_comp_streams

show current max_comp_streams (default value is 1).

iozone -t -T -R -l 3 -u 3 -r 16K -s 60M -I +Z
(write tests. ext4, lzo, each test performed on newly created
 zram0 device)

        test        max_comp_streams 1    max_comp_streams 3
-------------------------------------------------------------
"  Initial write "      631583.39     |      727834.88
"        Rewrite "      456248.23     |     2105054.97
" Mixed workload "     3370016.43     |     3509382.86
"   Random write "      467703.56     |     1953464.78
"         Pwrite "      629146.88     |      775372.46
"         Fwrite "     2573117.39     |     2529637.62

        test        max_comp_streams 1    max_comp_streams 3
-------------------------------------------------------------
"  Initial write "      601112.91     |      765866.05
"        Rewrite "      447834.33     |     2163449.58
" Mixed workload "     2575128.08     |     3257711.77
"   Random write "      461327.45     |     1974731.12
"         Pwrite "      567038.36     |      740242.52
"         Fwrite "     2527107.44     |     2754336.64

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zcomp.c    | 15 +++++++++------
 drivers/block/zram/zcomp.h    |  2 +-
 drivers/block/zram/zram_drv.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 drivers/block/zram/zram_drv.h |  2 +-
 4 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 3b4920f..45d906a 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -124,11 +124,12 @@ static struct zcomp_backend *find_backend(const char *compress)
  * 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)
 {
 	struct zcomp *comp;
 	struct zcomp_backend *backend;
 	struct zcomp_strm *zstrm;
+	int i;
 
 	backend = find_backend(compress);
 	if (!backend)
@@ -143,11 +144,13 @@ struct zcomp *zcomp_create(const char *compress)
 	INIT_LIST_HEAD(&comp->idle_strm);
 	init_waitqueue_head(&comp->strm_wait);
 
-	zstrm = zcomp_strm_alloc(comp);
-	if (!zstrm) {
-		zcomp_destroy(comp);
-		return NULL;
+	for (i = 0; i < num_strm; i++) {
+		zstrm = zcomp_strm_alloc(comp);
+		if (!zstrm) {
+			zcomp_destroy(comp);
+			return NULL;
+		}
+		list_add_tail(&zstrm->list, &comp->idle_strm);
 	}
-	list_add_tail(&zstrm->list, &comp->idle_strm);
 	return comp;
 }
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 213e503..9b34f48 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -43,7 +43,7 @@ struct zcomp {
 	struct zcomp_backend *backend;
 };
 
-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 d0d7254..551202b 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -107,6 +107,41 @@ static ssize_t mem_used_total_show(struct device *dev,
 	return sprintf(buf, "%llu\n", val);
 }
 
+static ssize_t max_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)
+{
+	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("Cannot set max buffers 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)
@@ -505,6 +540,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
 	if (zram->comp)
 		zcomp_destroy(zram->comp);
 	zram->comp = NULL;
+	zram->max_comp_streams = 1;
 
 	zram_meta_free(zram->meta);
 	zram->meta = NULL;
@@ -534,7 +570,7 @@ static ssize_t disksize_store(struct device *dev,
 		return -EBUSY;
 	}
 
-	zram->comp = zcomp_create(default_compressor);
+	zram->comp = zcomp_create(default_compressor, zram->max_comp_streams);
 	if (!zram->comp) {
 		up_write(&zram->init_lock);
 		pr_info("Cannot initialise compressing backend\n");
@@ -697,6 +733,8 @@ static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
 static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
 static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
 static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
+static DEVICE_ATTR(max_comp_streams, S_IRUGO | S_IWUSR,
+		max_comp_streams_show, max_comp_streams_store);
 
 ZRAM_ATTR_RO(num_reads);
 ZRAM_ATTR_RO(num_writes);
@@ -721,6 +759,7 @@ static struct attribute *zram_disk_attrs[] = {
 	&dev_attr_orig_data_size.attr,
 	&dev_attr_compr_data_size.attr,
 	&dev_attr_mem_used_total.attr,
+	&dev_attr_max_comp_streams.attr,
 	NULL,
 };
 
@@ -784,6 +823,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.rc3.256.g4af3ebd


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCHv4 4/4] zram: document max_comp_streams
  2014-02-12 19:39 [PATCHv4 0/4] add compressing abstraction and multi stream support Sergey Senozhatsky
                   ` (3 preceding siblings ...)
  2014-02-12 19:39 ` [PATCHv4 3/4] zram: support multi compressing buffers Sergey Senozhatsky
@ 2014-02-12 19:39 ` Sergey Senozhatsky
  2014-02-13  5:09   ` Minchan Kim
  2014-02-17  4:56 ` [PATCHv4 0/4] add compressing abstraction and multi stream support Minchan Kim
  5 siblings, 1 reply; 11+ messages in thread
From: Sergey Senozhatsky @ 2014-02-12 19:39 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            | 20 +++++++++++++++-----
 2 files changed, 23 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..c6a55fb 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -21,7 +21,17 @@ 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.
+	Examples:
+	#set max buffers to 3
+	echo 3 > /sys/block/zram0/max_comp_streams
+
+	#show max buffers
+	cat /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 +48,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 +70,11 @@ size of the disk when not in use so a huge zram is wasteful.
 		compr_data_size
 		mem_used_total
 
-5) Deactivate:
+6) Deactivate:
 	swapoff /dev/zram0
 	umount /dev/zram1
 
-6) Reset:
+7) Reset:
 	Write any positive value to 'reset' sysfs node
 	echo 1 > /sys/block/zram0/reset
 	echo 1 > /sys/block/zram1/reset
-- 
1.9.0.rc3.256.g4af3ebd


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCHv4 1/4] zram: introduce compressing backend abstraction
  2014-02-12 19:39 ` [PATCHv4 1/4] zram: introduce compressing backend abstraction Sergey Senozhatsky
@ 2014-02-13  4:53   ` Minchan Kim
  2014-02-13 17:52     ` Sergey Senozhatsky
  0 siblings, 1 reply; 11+ messages in thread
From: Minchan Kim @ 2014-02-13  4:53 UTC (permalink / raw)
  To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel

Hello Sergey,

On Wed, Feb 12, 2014 at 10:39:33PM +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. Besides, zram holds buffer_lock almost through the whole write()
> function, making parallel compression impossible. To remove requirement
> a) new struct zcomp_strm introduced, which contain buffers required by
> compression algorithm. 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' buffer.
> 
> Every zcomp has a list of idle zcomp_strm buffers, spinlock to protect idle
> list and wait queue, making it possible to perform parallel compressions.
> Each time zram issues a zcomp_strm_get() call, the following set of operations
> performed:
> - spin lock buffer_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_put() caller.
> 
> zcomp_strm_put():
> - spin lock buffer_lock
> - add zcomp stream to idle list
> - spin unlock, wake up sleeper
> 
> In other words, zcomp_strm_get() turns caller into exclusive user of a stream
> and zcomp_strm_put() makes a particular zcomp stream available.
> 
> Usage examples.
> 
> To initialize compressing backend:
> 	comp = zcomp_create(NAME) /* NAME e.g. "lzo" */
> 
> which initialises compressing backend if requested algorithm is supported.
> 
> Compress:
> 	zstrm = zcomp_strm_get(comp)
> 	zcomp_compress(comp, zstrm, src, src_len, &dst_len)
> 	[..] /* copy compressed data */
> 	zcomp_strm_put(comp, zstrm)
> 
> Decompress:
> 	zcomp_decompress(comp, src, src_len, dst, &dst_len);
> 
> Free compessing backend and its zcomp stream:
> 	zcomp_destroy(comp)
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/zcomp.c     | 153 +++++++++++++++++++++++++++++++++++++++++
>  drivers/block/zram/zcomp.h     |  57 +++++++++++++++
>  drivers/block/zram/zcomp_lzo.c |  33 +++++++++
>  3 files changed, 243 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..3b4920f
> --- /dev/null
> +++ b/drivers/block/zram/zcomp.c
> @@ -0,0 +1,153 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/wait.h>
> +#include <linux/sched.h>
> +
> +#include "zcomp.h"
> +
> +extern struct zcomp_backend zcomp_lzo;
> +
> +static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm)
> +{
> +	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;
> +
> +	INIT_LIST_HEAD(&zstrm->list);
> +	/* algorithm specific working memory buffer */

Please don't use "buffer". As I said, it would have more other than buffer.

> +	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)
> +		goto fail;

I'm not fond of "goto" such simple error handling.

if (!zram->private || !zstrm->buffer) {
    zcmp_strm_free(comp, zstrm);
    zstrm = NULL;
}

return zstrm;

> +
> +	return zstrm;
> +fail:
> +	zcomp_strm_free(comp, zstrm);
> +	return NULL;
> +}
> +
> +/*
> + * get existing idle zcomp_strm or wait until other process release
> + * (zcomp_strm_put()) one for us
> + */
> +struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
> +{
> +	struct zcomp_strm *zstrm;
> +retry:
> +	spin_lock(&comp->buffer_lock);

Pz, rename buffer_lock with comp->strm_lock.

> +	if (list_empty(&comp->idle_strm)) {
> +		spin_unlock(&comp->buffer_lock);
> +		wait_event(comp->strm_wait,
> +				!list_empty(&comp->idle_strm));
> +		goto retry;
> +	}

Same reason: I don't like "goto"

while(1) {
    spin_lock
    if (list_empty()) {
        spin_unlock
        wait_event()
        continue;
    }
    
    zstrm = list_entry()
    list_del
    spin_unlock
    break;
}

return zstrm;

> +
> +	zstrm = list_entry(comp->idle_strm.next,
> +			struct zcomp_strm, list);
> +	list_del(&zstrm->list);
> +	spin_unlock(&comp->buffer_lock);
> +	return zstrm;
> +}
> +
> +/* add zcomp_strm back to idle list and wake up waiter (if any) */
> +void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> +{
> +	spin_lock(&comp->buffer_lock);
> +	list_add_tail(&zstrm->list, &comp->idle_strm);

What I said "queue" in previous review is that I thougt stack might
be more cache-freindly than queue so list_add is proper than list_add_tail.
But it seems you have a reason to list_add_tail. If so, could you explain
what you expect?

> +	spin_unlock(&comp->buffer_lock);
> +
> +	wake_up(&comp->strm_wait);
> +}
> +
> +int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> +		const unsigned char *src, size_t src_len, size_t *dst_len)
> +{
> +	return comp->backend->compress(src, src_len, zstrm->buffer,
> +			dst_len, zstrm->private);
> +}

We need src_len? it's always PAGE_SIZE.

> +
> +int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> +		size_t src_len, unsigned char *dst, size_t *dst_len)
> +{
> +	return comp->backend->decompress(src, src_len, dst, dst_len);

We need dst_len? It's always PAGE_SIZE, too.

> +}
> +
> +/* free allocated zcomp_strm buffers and zcomp */
> +void zcomp_destroy(struct zcomp *comp)
> +{
> +	struct zcomp_strm *zstrm;
> +	while (!list_empty(&comp->idle_strm)) {
> +		zstrm = list_entry(comp->idle_strm.next,
> +				struct zcomp_strm, list);
> +		list_del(&zstrm->list);
> +		zcomp_strm_free(comp, zstrm);
> +	}
> +	kfree(comp);
> +}
> +
> +static struct zcomp_backend *find_backend(const char *compress)
> +{
> +	if (sysfs_streq(compress, "lzo"))
> +		return &zcomp_lzo;
> +	return NULL;
> +}
> +
> +/*
> + * 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;
> +	struct zcomp_strm *zstrm;
> +
> +	backend = find_backend(compress);
> +	if (!backend)
> +		return NULL;
> +
> +	comp = kmalloc(sizeof(struct zcomp), GFP_KERNEL);
> +	if (!comp)
> +		return NULL;
> +
> +	comp->backend = backend;
> +	spin_lock_init(&comp->buffer_lock);
> +	INIT_LIST_HEAD(&comp->idle_strm);
> +	init_waitqueue_head(&comp->strm_wait);
> +
> +	zstrm = zcomp_strm_alloc(comp);
> +	if (!zstrm) {
> +		zcomp_destroy(comp);
> +		return NULL;
> +	}
> +	list_add_tail(&zstrm->list, &comp->idle_strm);

It's first adding to empty list so there is no difference
between list_add and list_add_tail then, let's use list_add.

> +	return comp;
> +}
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> new file mode 100644
> index 0000000..213e503
> --- /dev/null
> +++ b/drivers/block/zram/zcomp.h
> @@ -0,0 +1,57 @@
> +/*
> + * 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/types.h>
> +#include <linux/spinlock.h>
> +
> +struct zcomp_strm {
> +	void *buffer;     /* compression/decompression buffer */
> +	void *private;      /* algorithm workmem */

Remove workmem.

> +	struct list_head list;
> +};
> +
> +/* static compression backend */
> +struct zcomp_backend {
> +	int (*compress)(const unsigned char *src, size_t src_len,
> +			unsigned char *dst, size_t *dst_len, void *wrkmem);

Rename wrkmem.

> +
> +	int (*decompress)(const unsigned char *src, size_t src_len,
> +			unsigned char *dst, size_t *dst_len);
> +
> +	void * (*create)(void);
> +	void (*destroy)(void *private);
> +
> +	const char *name;
> +};
> +
> +/* dynamic per-device compression frontend */
> +struct zcomp {
> +	/* protect strm list */
> +	spinlock_t buffer_lock;

Pz, rename.

> +	/* list of available strms */
> +	struct list_head idle_strm;
> +	wait_queue_head_t strm_wait;
> +	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 *zsrtm);
                                                              zstrm
> +
> +int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> +		const unsigned char *src, size_t src_len, size_t *dst_len);
> +
> +int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> +		size_t src_len, unsigned char *dst, size_t *dst_len);
> +#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..2b23771
> --- /dev/null
> +++ b/drivers/block/zram/zcomp_lzo.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/lzo.h>
> +
> +#include "zcomp.h"
> +
> +static void * lzo_create(void)
> +{
> +	return kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> +}
> +
> +static void lzo_destroy(void *private)
> +{
> +	kfree(private);
> +}
> +
> +extern struct zcomp_backend zcomp_lzo;
> +struct zcomp_backend zcomp_lzo = {
> +	.compress = lzo1x_1_compress,
> +	.decompress = lzo1x_decompress_safe,
> +	.create = lzo_create,
> +	.destroy = lzo_destroy,
> +	.name = "lzo",
> +};
> -- 
> 1.9.0.rc3.256.g4af3ebd
> 
> --
> 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] 11+ messages in thread

* Re: [PATCHv4 2/4] zram: use zcomp compressing backends
  2014-02-12 19:39 ` [PATCHv4 2/4] zram: use zcomp compressing backends Sergey Senozhatsky
@ 2014-02-13  5:04   ` Minchan Kim
  0 siblings, 0 replies; 11+ messages in thread
From: Minchan Kim @ 2014-02-13  5:04 UTC (permalink / raw)
  To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel

On Wed, Feb 12, 2014 at 10:39:34PM +0300, Sergey Senozhatsky wrote:
> Do not perform direct LZO compress/decompress calls,
> initialise and use zcomp LZO backend instead.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/Makefile   |  2 +-
>  drivers/block/zram/zram_drv.c | 62 +++++++++++++++++++------------------------
>  drivers/block/zram/zram_drv.h |  8 +++---
>  3 files changed, 33 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/block/zram/Makefile b/drivers/block/zram/Makefile
> index cb0f9ce..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..d0d7254 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -29,15 +29,14 @@
>  #include <linux/genhd.h>
>  #include <linux/highmem.h>
>  #include <linux/slab.h>
> -#include <linux/lzo.h>
>  #include <linux/string.h>
> -#include <linux/vmalloc.h>
>  
>  #include "zram_drv.h"
>  
>  /* Globals */
>  static int zram_major;
>  static struct zram *zram_devices;
> +static const char *default_compressor = "lzo";
>  
>  /* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
> @@ -160,8 +159,6 @@ static inline int valid_io_request(struct zram *zram, struct bio *bio)
>  static void zram_meta_free(struct zram_meta *meta)
>  {
>  	zs_destroy_pool(meta->mem_pool);
> -	kfree(meta->compress_workmem);
> -	free_pages((unsigned long)meta->compress_buffer, 1);
>  	vfree(meta->table);
>  	kfree(meta);
>  }
> @@ -173,22 +170,11 @@ static struct zram_meta *zram_meta_alloc(u64 disksize)
>  	if (!meta)
>  		goto out;
>  
> -	meta->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> -	if (!meta->compress_workmem)
> -		goto free_meta;
> -
> -	meta->compress_buffer =
> -		(void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
> -	if (!meta->compress_buffer) {
> -		pr_err("Error allocating compressor buffer space\n");
> -		goto free_workmem;
> -	}
> -
>  	num_pages = disksize >> PAGE_SHIFT;
>  	meta->table = vzalloc(num_pages * sizeof(*meta->table));
>  	if (!meta->table) {
>  		pr_err("Error allocating zram address table\n");
> -		goto free_buffer;
> +		goto free_meta;
>  	}
>  
>  	meta->mem_pool = zs_create_pool(GFP_NOIO | __GFP_HIGHMEM);
> @@ -198,15 +184,10 @@ static struct zram_meta *zram_meta_alloc(u64 disksize)
>  	}
>  
>  	rwlock_init(&meta->tb_lock);
> -	mutex_init(&meta->buffer_lock);
>  	return meta;
>  
>  free_table:
>  	vfree(meta->table);
> -free_buffer:
> -	free_pages((unsigned long)meta->compress_buffer, 1);
> -free_workmem:
> -	kfree(meta->compress_workmem);
>  free_meta:
>  	kfree(meta);
>  	meta = NULL;
> @@ -280,7 +261,7 @@ static void zram_free_page(struct zram *zram, size_t index)
>  
>  static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>  {
> -	int ret = LZO_E_OK;
> +	int ret = 0;
>  	size_t clen = PAGE_SIZE;
>  	unsigned char *cmem;
>  	struct zram_meta *meta = zram->meta;
> @@ -301,12 +282,12 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>  	if (size == PAGE_SIZE)
>  		copy_page(mem, cmem);
>  	else
> -		ret = lzo1x_decompress_safe(cmem, size,	mem, &clen);
> +		ret = zcomp_decompress(zram->comp, cmem, size, mem, &clen);
>  	zs_unmap_object(meta->mem_pool, handle);
>  	read_unlock(&meta->tb_lock);
>  
>  	/* Should NEVER happen. Return bio error if it does. */
> -	if (unlikely(ret != LZO_E_OK)) {

Hmm this code depends on that "LZO_E_OK 0" but it could be changed in future
so it ends up break. Perhaps, we should handle it in backend.

zcomp_lzo.c:

int lzo_decompress()
{
        int ret = lzo1x_decompress_safe(xxx);
        if (ret != LZO_E_OK)
                return ret;
}


> +	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,19 +400,18 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		goto out;
>  	}
>  
> -	ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
> -			       meta->compress_workmem);
> +	ret = zcomp_compress(zram->comp, zstrm, uncmem, bvec->bv_len, &clen);
>  	if (!is_partial_io(bvec)) {
>  		kunmap_atomic(user_mem);
>  		user_mem = NULL;
>  		uncmem = NULL;
>  	}
>  
> -	if (unlikely(ret != LZO_E_OK)) {
> +	if (unlikely(ret)) {
>  		pr_err("Compression failed! err=%d\n", ret);
>  		goto out;
>  	}
> -
> +	src = zstrm->buffer;
>  	if (unlikely(clen > max_zpage_size)) {
>  		clen = PAGE_SIZE;
>  		src = NULL;
> @@ -457,6 +436,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		memcpy(cmem, src, clen);
>  	}
>  
> +	zcomp_strm_put(zram->comp, zstrm);
> +	locked = false;
>  	zs_unmap_object(meta->mem_pool, handle);
>  
>  	/*
> @@ -475,10 +456,9 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  	atomic64_inc(&zram->stats.pages_stored);
>  out:
>  	if (locked)
> -		mutex_unlock(&meta->buffer_lock);
> +		zcomp_strm_put(zram->comp, zstrm);
>  	if (is_partial_io(bvec))
>  		kfree(uncmem);
> -
>  	if (ret)
>  		atomic64_inc(&zram->stats.failed_writes);
>  	return ret;
> @@ -522,6 +502,10 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>  		zs_free(meta->mem_pool, handle);
>  	}
>  
> +	if (zram->comp)
> +		zcomp_destroy(zram->comp);
> +	zram->comp = NULL;

Why we need to reset NULL?

> +
>  	zram_meta_free(zram->meta);
>  	zram->meta = NULL;
>  	/* Reset stats */
> @@ -550,9 +534,18 @@ static ssize_t disksize_store(struct device *dev,
>  		return -EBUSY;
>  	}
>  
> +	zram->comp = zcomp_create(default_compressor);
> +	if (!zram->comp) {
> +		up_write(&zram->init_lock);
> +		pr_info("Cannot initialise compressing backend\n");
> +		return -EINVAL;
> +	}
> +
>  	disksize = PAGE_ALIGN(disksize);
>  	zram->meta = zram_meta_alloc(disksize);
>  	if (!zram->meta) {
> +		zcomp_destroy(zram->comp);
> +		zram->comp = NULL;
>  		up_write(&zram->init_lock);
>  		return -ENOMEM;
>  	}
> @@ -790,6 +783,7 @@ static int create_device(struct zram *zram, int device_id)
>  	}
>  
>  	zram->meta = NULL;
> +	zram->comp = NULL;
>  	return 0;
>  
>  out_free_disk:
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 1d5b1f5..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.rc3.256.g4af3ebd
> 
> --
> 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] 11+ messages in thread

* Re: [PATCHv4 4/4] zram: document max_comp_streams
  2014-02-12 19:39 ` [PATCHv4 4/4] zram: document max_comp_streams Sergey Senozhatsky
@ 2014-02-13  5:09   ` Minchan Kim
  0 siblings, 0 replies; 11+ messages in thread
From: Minchan Kim @ 2014-02-13  5:09 UTC (permalink / raw)
  To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel

On Wed, Feb 12, 2014 at 10:39:36PM +0300, Sergey Senozhatsky wrote:
> 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            | 20 +++++++++++++++-----
>  2 files changed, 23 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..c6a55fb 100644
> --- a/Documentation/blockdev/zram.txt
> +++ b/Documentation/blockdev/zram.txt
> @@ -21,7 +21,17 @@ 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.
> +	Examples:
> +	#set max buffers to 3

Pz, don't use buffers.

> +	echo 3 > /sys/block/zram0/max_comp_streams
> +
> +	#show max buffers

Ditto.

> +	cat /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 +48,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 +70,11 @@ size of the disk when not in use so a huge zram is wasteful.
>  		compr_data_size
>  		mem_used_total
>  
> -5) Deactivate:
> +6) Deactivate:
>  	swapoff /dev/zram0
>  	umount /dev/zram1
>  
> -6) Reset:
> +7) Reset:
>  	Write any positive value to 'reset' sysfs node
>  	echo 1 > /sys/block/zram0/reset
>  	echo 1 > /sys/block/zram1/reset
> -- 
> 1.9.0.rc3.256.g4af3ebd
> 
> --
> 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] 11+ messages in thread

* Re: [PATCHv4 1/4] zram: introduce compressing backend abstraction
  2014-02-13  4:53   ` Minchan Kim
@ 2014-02-13 17:52     ` Sergey Senozhatsky
  0 siblings, 0 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2014-02-13 17:52 UTC (permalink / raw)
  To: Minchan Kim; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel

Hello Minchan,

thank you for review.

On (02/13/14 13:53), Minchan Kim wrote:
> 
> Hello Sergey,
> 
[..]
> > +			struct zcomp_strm, list);
> > +	list_del(&zstrm->list);
> > +	spin_unlock(&comp->buffer_lock);
> > +	return zstrm;
> > +}
> > +
> > +/* add zcomp_strm back to idle list and wake up waiter (if any) */
> > +void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> > +{
> > +	spin_lock(&comp->buffer_lock);
> > +	list_add_tail(&zstrm->list, &comp->idle_strm);
> 
> What I said "queue" in previous review is that I thougt stack might
> be more cache-freindly than queue so list_add is proper than list_add_tail.
> But it seems you have a reason to list_add_tail. If so, could you explain
> what you expect?

sure, 100% my mistake. agreed, using the most recently used zstrm is the
right thing to do. I was not attentive, sorry.

> > +	spin_unlock(&comp->buffer_lock);
> > +
> > +	wake_up(&comp->strm_wait);
> > +}
> > +
> > +int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > +		const unsigned char *src, size_t src_len, size_t *dst_len)
> > +{
> > +	return comp->backend->compress(src, src_len, zstrm->buffer,
> > +			dst_len, zstrm->private);
> > +}
> 
> We need src_len? it's always PAGE_SIZE.
> 
> > +
> > +int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> > +		size_t src_len, unsigned char *dst, size_t *dst_len)
> > +{
> > +	return comp->backend->decompress(src, src_len, dst, dst_len);
> 
> We need dst_len? It's always PAGE_SIZE, too.
> 

the only reason why I left src_len is partial IO, where
`bv_len (src_len) != PAGE_SIZE'. no reason to keep dst_len.



I've addressed your concerns and notes from this email, emails
"zram: document max_comp_streams" and "zram: use zcomp compressing backends"
in PATCH v5.

thank you once again.

	-ss

> > +}
> > +
> > +/* free allocated zcomp_strm buffers and zcomp */
> > +void zcomp_destroy(struct zcomp *comp)
> > +{
> > +	struct zcomp_strm *zstrm;
> > +	while (!list_empty(&comp->idle_strm)) {
> > +		zstrm = list_entry(comp->idle_strm.next,
> > +				struct zcomp_strm, list);
> > +		list_del(&zstrm->list);
> > +		zcomp_strm_free(comp, zstrm);
> > +	}
> > +	kfree(comp);
> > +}
> > +
> > +static struct zcomp_backend *find_backend(const char *compress)
> > +{
> > +	if (sysfs_streq(compress, "lzo"))
> > +		return &zcomp_lzo;
> > +	return NULL;
> > +}
> > +
> > +/*
> > + * 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;
> > +	struct zcomp_strm *zstrm;
> > +
> > +	backend = find_backend(compress);
> > +	if (!backend)
> > +		return NULL;
> > +
> > +	comp = kmalloc(sizeof(struct zcomp), GFP_KERNEL);
> > +	if (!comp)
> > +		return NULL;
> > +
> > +	comp->backend = backend;
> > +	spin_lock_init(&comp->buffer_lock);
> > +	INIT_LIST_HEAD(&comp->idle_strm);
> > +	init_waitqueue_head(&comp->strm_wait);
> > +
> > +	zstrm = zcomp_strm_alloc(comp);
> > +	if (!zstrm) {
> > +		zcomp_destroy(comp);
> > +		return NULL;
> > +	}
> > +	list_add_tail(&zstrm->list, &comp->idle_strm);
> 
> It's first adding to empty list so there is no difference
> between list_add and list_add_tail then, let's use list_add.
> 
> > +	return comp;
> > +}
> > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > new file mode 100644
> > index 0000000..213e503
> > --- /dev/null
> > +++ b/drivers/block/zram/zcomp.h
> > @@ -0,0 +1,57 @@
> > +/*
> > + * 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/types.h>
> > +#include <linux/spinlock.h>
> > +
> > +struct zcomp_strm {
> > +	void *buffer;     /* compression/decompression buffer */
> > +	void *private;      /* algorithm workmem */
> 
> Remove workmem.
> 
> > +	struct list_head list;
> > +};
> > +
> > +/* static compression backend */
> > +struct zcomp_backend {
> > +	int (*compress)(const unsigned char *src, size_t src_len,
> > +			unsigned char *dst, size_t *dst_len, void *wrkmem);
> 
> Rename wrkmem.
> 
> > +
> > +	int (*decompress)(const unsigned char *src, size_t src_len,
> > +			unsigned char *dst, size_t *dst_len);
> > +
> > +	void * (*create)(void);
> > +	void (*destroy)(void *private);
> > +
> > +	const char *name;
> > +};
> > +
> > +/* dynamic per-device compression frontend */
> > +struct zcomp {
> > +	/* protect strm list */
> > +	spinlock_t buffer_lock;
> 
> Pz, rename.
> 
> > +	/* list of available strms */
> > +	struct list_head idle_strm;
> > +	wait_queue_head_t strm_wait;
> > +	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 *zsrtm);
>                                                               zstrm
> > +
> > +int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > +		const unsigned char *src, size_t src_len, size_t *dst_len);
> > +
> > +int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> > +		size_t src_len, unsigned char *dst, size_t *dst_len);
> > +#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..2b23771
> > --- /dev/null
> > +++ b/drivers/block/zram/zcomp_lzo.c
> > @@ -0,0 +1,33 @@
> > +/*
> > + * Copyright (C) 2014 Sergey Senozhatsky.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/lzo.h>
> > +
> > +#include "zcomp.h"
> > +
> > +static void * lzo_create(void)
> > +{
> > +	return kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> > +}
> > +
> > +static void lzo_destroy(void *private)
> > +{
> > +	kfree(private);
> > +}
> > +
> > +extern struct zcomp_backend zcomp_lzo;
> > +struct zcomp_backend zcomp_lzo = {
> > +	.compress = lzo1x_1_compress,
> > +	.decompress = lzo1x_decompress_safe,
> > +	.create = lzo_create,
> > +	.destroy = lzo_destroy,
> > +	.name = "lzo",
> > +};
> > -- 
> > 1.9.0.rc3.256.g4af3ebd
> > 
> > --
> > 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] 11+ messages in thread

* Re: [PATCHv4 0/4] add compressing abstraction and multi stream support
  2014-02-12 19:39 [PATCHv4 0/4] add compressing abstraction and multi stream support Sergey Senozhatsky
                   ` (4 preceding siblings ...)
  2014-02-12 19:39 ` [PATCHv4 4/4] zram: document max_comp_streams Sergey Senozhatsky
@ 2014-02-17  4:56 ` Minchan Kim
  5 siblings, 0 replies; 11+ messages in thread
From: Minchan Kim @ 2014-02-17  4:56 UTC (permalink / raw)
  To: Sergey Senozhatsky; +Cc: Jerome Marchand, Nitin Gupta, linux-kernel

Hello Sergey,

I don't have a time to look at it now
Sorry for that. I will hold the time soonish.

Thanks.

On Wed, Feb 12, 2014 at 10:39:31PM +0300, Sergey Senozhatsky wrote:
> 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.
> 
> Diff from v3:
> -- renamed compression backend and working memory structs as requested
>    by Minchan Kim; fixed several issues noted by Minchan Kim.
> 
> Sergey Senozhatsky (4):
>   zram: introduce compressing backend abstraction
>   zram: use zcomp compressing backends
>   zram: support multi compressing buffers
>   zram: document max_comp_streams
> 
>  Documentation/ABI/testing/sysfs-block-zram |   9 +-
>  Documentation/blockdev/zram.txt            |  21 +++-
>  drivers/block/zram/Makefile                |   2 +-
>  drivers/block/zram/zcomp.c                 | 156 +++++++++++++++++++++++++++++
>  drivers/block/zram/zcomp.h                 |  58 +++++++++++
>  drivers/block/zram/zcomp_lzo.c             |  33 ++++++
>  drivers/block/zram/zram_drv.c              | 102 ++++++++++++-------
>  drivers/block/zram/zram_drv.h              |  10 +-
>  8 files changed, 345 insertions(+), 46 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.rc3.256.g4af3ebd
> 
> --
> 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] 11+ messages in thread

end of thread, other threads:[~2014-02-17  4:56 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-12 19:39 [PATCHv4 0/4] add compressing abstraction and multi stream support Sergey Senozhatsky
2014-02-12 19:39 ` Sergey Senozhatsky
2014-02-12 19:39 ` [PATCHv4 1/4] zram: introduce compressing backend abstraction Sergey Senozhatsky
2014-02-13  4:53   ` Minchan Kim
2014-02-13 17:52     ` Sergey Senozhatsky
2014-02-12 19:39 ` [PATCHv4 2/4] zram: use zcomp compressing backends Sergey Senozhatsky
2014-02-13  5:04   ` Minchan Kim
2014-02-12 19:39 ` [PATCHv4 3/4] zram: support multi compressing buffers Sergey Senozhatsky
2014-02-12 19:39 ` [PATCHv4 4/4] zram: document max_comp_streams Sergey Senozhatsky
2014-02-13  5:09   ` Minchan Kim
2014-02-17  4:56 ` [PATCHv4 0/4] add compressing abstraction and multi stream support Minchan Kim

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.