[PATCHv3 0/4] add compression backend abstraction

[PATCHv3 0/4] add compression backend abstraction

[Sergey Senozhatsky]

This patchset introduces zram_comp compression backend abstraction
adding ability to support compression algorithms other than LZO,
and support for multi buffers, making parallel compressions possible.

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

 Documentation/ABI/testing/sysfs-block-zram |   9 +-
 Documentation/blockdev/zram.txt            |  21 +++-
 drivers/block/zram/Makefile                |   2 +-
 drivers/block/zram/zcomp_lzo.c             |  33 +++++++
 drivers/block/zram/zram_comp.c             | 150 +++++++++++++++++++++++++++++
 drivers/block/zram/zram_comp.h             |  58 +++++++++++
 drivers/block/zram/zram_drv.c              | 102 +++++++++++++-------
 drivers/block/zram/zram_drv.h              |  10 +-
 8 files changed, 339 insertions(+), 46 deletions(-)
 create mode 100644 drivers/block/zram/zcomp_lzo.c
 create mode 100644 drivers/block/zram/zram_comp.c
 create mode 100644 drivers/block/zram/zram_comp.h

-- 
1.9.0.rc3.244.g3497008

[PATCHv3 1/4] zram: introduce compressing backend abstraction

[Sergey Senozhatsky]

ZRAM performs direct LZO compression algorithm calls, making it the one
and only option. Introduce abstract struct zram_comp in order to support
multiple compression algorithms. struct zram_comp defines the following
set of compressing backend operations:
        .create
        .destroy
        .compress
        .decompress

Schematically zram write() usually contains the following steps:
0) preparation (decompression of partioal IO, etc.)
1) lock buffer_lock mutex (protects meta compress buffers)
2) compress (using meta compress buffers)
3) alloc and map zs_pool object
4) copy compressed data (from meta compress buffers) to object allocated by 3)
5) free previous pool page, assign a new one
6) unlock buffer_lock mutex

As we can see, compressing buffers must remain untouched from 1) to 4),
because, otherwise, concurrent write() can overwrite data. At the same
time, zram_meta must be aware of a) specific compression algorithm
memory requirements and b) necessary locking to protect compression
buffers. Besides, zram holds buffer_lock almost through the whole write()
function, making parallel compression impossible. To remove requirement
a) new struct zcomp_workmem introduced, which contain buffers required by
compression algorithm. While struct zram_comp implements workmem handling
and locking by means of get() and put() semantics and removes requirement
b) from zram meta. zram_comp ->create() and ->destroy(), respectively,
allocate and deallocate algorithm specific zcomp_workmem `private' buffer.

Every zram_comp has a list of idle workmem buffers (at least 1 workmem),
spinlock to protect idle list and wait queue, making it possible to perform
parallel compressions. Each time zram issues a zcomp_workmem_get() call, the
following set of operations performed:
- spin lock buffer_lock
- if idle list is not empty, remove workmem from idle list, spin
  unlock and return workmem pointer
- if idle list is empty, current adds itself to wait queue. it will be
  awaken by zcomp_workmem_put() caller.

zcomp_workmem_put():
- spin lock buffer_lock
- add workmem to idle list
- spin unlock, wake up sleeper (if any)

In other words, zcomp_workmem_get() turns caller into exclusive user of workmem
and zcomp_workem_put() makes a particular workmem available.

Usage examples.

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

which initialises compressing backend if requested algorithm is supported.

Compress:
	wm = zcomp_workmem_get(comp)
	zcomp_compress(comp, wm, src, src_len, &dst_len)
	[..] /* copy compressed data */
	zcomp_workmem_put(comp, wm)

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

Free compessing backend and its workmem:
	zcomp_destroy(comp)

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zcomp_lzo.c |  33 +++++++++
 drivers/block/zram/zram_comp.c | 147 +++++++++++++++++++++++++++++++++++++++++
 drivers/block/zram/zram_comp.h |  58 ++++++++++++++++
 3 files changed, 238 insertions(+)
 create mode 100644 drivers/block/zram/zcomp_lzo.c
 create mode 100644 drivers/block/zram/zram_comp.c
 create mode 100644 drivers/block/zram/zram_comp.h

diff --git a/drivers/block/zram/zcomp_lzo.c b/drivers/block/zram/zcomp_lzo.c
new file mode 100644
index 0000000..bbde74e
--- /dev/null
+++ b/drivers/block/zram/zcomp_lzo.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2014 Sergey Senozhatsky.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/lzo.h>
+
+#include "zram_comp.h"
+
+static void * lzo_create(void)
+{
+	return kmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
+}
+
+static void lzo_destroy(void *workmem)
+{
+	kfree(workmem);
+}
+
+extern struct zram_comp_backend zcomp_lzo;
+struct zram_comp_backend zcomp_lzo = {
+	.compress = lzo1x_1_compress,
+	.decompress = lzo1x_decompress_safe,
+	.create = lzo_create,
+	.destroy = lzo_destroy,
+	.name = "lzo",
+};
diff --git a/drivers/block/zram/zram_comp.c b/drivers/block/zram/zram_comp.c
new file mode 100644
index 0000000..139a468
--- /dev/null
+++ b/drivers/block/zram/zram_comp.c
@@ -0,0 +1,147 @@
+/*
+ * Copyright (C) 2014 Sergey Senozhatsky.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+#include <linux/wait.h>
+#include <linux/sched.h>
+
+#include "zram_comp.h"
+
+extern struct zram_comp_backend zcomp_lzo;
+
+static void workmem_free(struct zram_comp *comp, struct zcomp_workmem *workmem)
+{
+	comp->backend->destroy(workmem->private);
+	free_pages((unsigned long)workmem->buffer, 1);
+	kfree(workmem);
+}
+
+/* allocate new workmem structure with ->mem of requested size,
+ * return NULL on error */
+static struct zcomp_workmem *workmem_alloc(struct zram_comp *comp)
+{
+	struct zcomp_workmem *workmem = kmalloc(sizeof(*workmem), GFP_NOFS);
+	if (!workmem)
+		return NULL;
+
+	INIT_LIST_HEAD(&workmem->list);
+	/* algorithm specific working memory buffer */
+	workmem->private = comp->backend->create();
+	/* allocate 2 pages. 1 for compressed data, plus 1 extra for the
+	 * case when compressed size is larger than the original one. */
+	workmem->buffer = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
+	if (!workmem->private || !workmem->buffer)
+		goto fail;
+
+	return workmem;
+fail:
+	workmem_free(comp, workmem);
+	return NULL;
+}
+
+/* get existing idle workmem or wait until other process release
+ * (workmem_put()) one for us */
+struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp)
+{
+	struct zcomp_workmem *wm;
+retry:
+	spin_lock(&comp->buffer_lock);
+	if (list_empty(&comp->idle_workmem)) {
+		spin_unlock(&comp->buffer_lock);
+		wait_event(comp->workmem_wait,
+				!list_empty(&comp->idle_workmem));
+		goto retry;
+	}
+
+	wm = list_entry(comp->idle_workmem.next,
+			struct zcomp_workmem, list);
+	list_del(&wm->list);
+	spin_unlock(&comp->buffer_lock);
+	return wm;
+}
+
+/* add workmem back to idle list and wake up waiter (if any) */
+void zcomp_workmem_put(struct zram_comp *comp,
+		struct zcomp_workmem *workmem)
+{
+	spin_lock(&comp->buffer_lock);
+	list_add_tail(&workmem->list, &comp->idle_workmem);
+	spin_unlock(&comp->buffer_lock);
+
+	if (waitqueue_active(&comp->workmem_wait))
+		wake_up(&comp->workmem_wait);
+}
+
+int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
+		const unsigned char *src, size_t src_len, size_t *dst_len)
+{
+	return comp->backend->compress(src, src_len, workmem->buffer,
+			dst_len, workmem->private);
+}
+
+int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
+		size_t src_len, unsigned char *dst, size_t *dst_len)
+{
+	return comp->backend->decompress(src, src_len, dst, dst_len);
+}
+
+/* free allocated workmem buffers and zram_comp */
+void zcomp_destroy(struct zram_comp *comp)
+{
+	struct zcomp_workmem *wm;
+	while (!list_empty(&comp->idle_workmem)) {
+		wm = list_entry(comp->idle_workmem.next,
+				struct zcomp_workmem, list);
+		list_del(&wm->list);
+		workmem_free(comp, wm);
+	}
+	kfree(comp);
+}
+
+static struct zram_comp_backend *find_backend(const char *compress)
+{
+	if (sysfs_streq(compress, "lzo"))
+		return &zcomp_lzo;
+	return NULL;
+}
+
+/* search available compressors for requested algorithm.
+ * allocate new zram_comp and initialize it. return NULL
+ * if requested algorithm is not supported or in case
+ * of init error */
+struct zram_comp *zcomp_create(const char *compress)
+{
+	struct zram_comp *comp;
+	struct zram_comp_backend *backend;
+	struct zcomp_workmem *wm;
+
+	backend = find_backend(compress);
+	if (!backend)
+		return NULL;
+
+	comp = kzalloc(sizeof(struct zram_comp), GFP_KERNEL);
+	if (!comp)
+		return NULL;
+
+	comp->backend = backend;
+	spin_lock_init(&comp->buffer_lock);
+	INIT_LIST_HEAD(&comp->idle_workmem);
+	init_waitqueue_head(&comp->workmem_wait);
+
+	wm = workmem_alloc(comp);
+	if (!wm) {
+		zcomp_destroy(comp);
+		return NULL;
+	}
+	list_add_tail(&wm->list, &comp->idle_workmem);
+	return comp;
+}
diff --git a/drivers/block/zram/zram_comp.h b/drivers/block/zram/zram_comp.h
new file mode 100644
index 0000000..90355e8
--- /dev/null
+++ b/drivers/block/zram/zram_comp.h
@@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2014 Sergey Senozhatsky.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _ZRAM_COMP_H_
+#define _ZRAM_COMP_H_
+
+#include <linux/types.h>
+#include <linux/spinlock.h>
+
+struct zcomp_workmem {
+	void *buffer;     /* compression/decompression buffer */
+	void *private;      /* algorithm workmem */
+	struct list_head list;
+};
+
+/* static compression backend */
+struct zram_comp_backend {
+	int (*compress)(const unsigned char *src, size_t src_len,
+			unsigned char *dst, size_t *dst_len, void *wrkmem);
+
+	int (*decompress)(const unsigned char *src, size_t src_len,
+			unsigned char *dst, size_t *dst_len);
+
+	void *(*create)(void);
+	void (*destroy)(void *workmem);
+
+	const char *name;
+};
+
+/* dynamic per-device compression frontend */
+struct zram_comp {
+	/* protect workmem list */
+	spinlock_t buffer_lock;
+	/* list of available workmems */
+	struct list_head idle_workmem;
+	wait_queue_head_t workmem_wait;
+	struct zram_comp_backend *backend;
+};
+
+struct zram_comp *zcomp_create(const char *comp);
+void zcomp_destroy(struct zram_comp *comp);
+
+struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp);
+void zcomp_workmem_put(struct zram_comp *comp,
+		struct zcomp_workmem *workmem);
+
+int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
+		const unsigned char *src, size_t src_len, size_t *dst_len);
+
+int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
+		size_t src_len, unsigned char *dst, size_t *dst_len);
+#endif /* _ZRAM_COMP_H_ */
-- 
1.9.0.rc3.244.g3497008

[PATCHv3 2/4] zram: use zram_comp compressing backends

[Sergey Senozhatsky]

Do not perform direct LZO compress/decompress calls,
initialise and use zram_comp LZO backend instead.

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

diff --git a/drivers/block/zram/Makefile b/drivers/block/zram/Makefile
index cb0f9ce..4c2e829 100644
--- a/drivers/block/zram/Makefile
+++ b/drivers/block/zram/Makefile
@@ -1,3 +1,3 @@
-zram-y	:=	zram_drv.o
+zram-y	:=	zcomp_lzo.o zram_comp.o zram_drv.o
 
 obj-$(CONFIG_ZRAM)	+=	zram.o
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 9baac5b..4875a16 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -29,15 +29,14 @@
 #include <linux/genhd.h>
 #include <linux/highmem.h>
 #include <linux/slab.h>
-#include <linux/lzo.h>
 #include <linux/string.h>
-#include <linux/vmalloc.h>
 
 #include "zram_drv.h"
 
 /* Globals */
 static int zram_major;
 static struct zram *zram_devices;
+static const char *default_compressor = "lzo";
 
 /* Module params (documentation at end) */
 static unsigned int num_devices = 1;
@@ -160,8 +159,6 @@ static inline int valid_io_request(struct zram *zram, struct bio *bio)
 static void zram_meta_free(struct zram_meta *meta)
 {
 	zs_destroy_pool(meta->mem_pool);
-	kfree(meta->compress_workmem);
-	free_pages((unsigned long)meta->compress_buffer, 1);
 	vfree(meta->table);
 	kfree(meta);
 }
@@ -173,22 +170,11 @@ static struct zram_meta *zram_meta_alloc(u64 disksize)
 	if (!meta)
 		goto out;
 
-	meta->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
-	if (!meta->compress_workmem)
-		goto free_meta;
-
-	meta->compress_buffer =
-		(void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
-	if (!meta->compress_buffer) {
-		pr_err("Error allocating compressor buffer space\n");
-		goto free_workmem;
-	}
-
 	num_pages = disksize >> PAGE_SHIFT;
 	meta->table = vzalloc(num_pages * sizeof(*meta->table));
 	if (!meta->table) {
 		pr_err("Error allocating zram address table\n");
-		goto free_buffer;
+		goto free_meta;
 	}
 
 	meta->mem_pool = zs_create_pool(GFP_NOIO | __GFP_HIGHMEM);
@@ -198,15 +184,10 @@ static struct zram_meta *zram_meta_alloc(u64 disksize)
 	}
 
 	rwlock_init(&meta->tb_lock);
-	mutex_init(&meta->buffer_lock);
 	return meta;
 
 free_table:
 	vfree(meta->table);
-free_buffer:
-	free_pages((unsigned long)meta->compress_buffer, 1);
-free_workmem:
-	kfree(meta->compress_workmem);
 free_meta:
 	kfree(meta);
 	meta = NULL;
@@ -280,7 +261,7 @@ static void zram_free_page(struct zram *zram, size_t index)
 
 static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 {
-	int ret = LZO_E_OK;
+	int ret = 0;
 	size_t clen = PAGE_SIZE;
 	unsigned char *cmem;
 	struct zram_meta *meta = zram->meta;
@@ -301,12 +282,12 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 	if (size == PAGE_SIZE)
 		copy_page(mem, cmem);
 	else
-		ret = lzo1x_decompress_safe(cmem, size,	mem, &clen);
+		ret = zcomp_decompress(zram->comp, cmem, size, mem, &clen);
 	zs_unmap_object(meta->mem_pool, handle);
 	read_unlock(&meta->tb_lock);
 
 	/* Should NEVER happen. Return bio error if it does. */
-	if (unlikely(ret != LZO_E_OK)) {
+	if (unlikely(ret)) {
 		pr_err("Decompression failed! err=%d, page=%u\n", ret, index);
 		atomic64_inc(&zram->stats.failed_reads);
 		return ret;
@@ -349,7 +330,7 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
 
 	ret = zram_decompress_page(zram, uncmem, index);
 	/* Should NEVER happen. Return bio error if it does. */
-	if (unlikely(ret != LZO_E_OK))
+	if (unlikely(ret))
 		goto out_cleanup;
 
 	if (is_partial_io(bvec))
@@ -374,11 +355,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	struct page *page;
 	unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
 	struct zram_meta *meta = zram->meta;
+	struct zcomp_workmem *wm;
 	bool locked = false;
 
 	page = bvec->bv_page;
-	src = meta->compress_buffer;
-
 	if (is_partial_io(bvec)) {
 		/*
 		 * This is a partial IO. We need to read the full page
@@ -394,7 +374,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 			goto out;
 	}
 
-	mutex_lock(&meta->buffer_lock);
+	wm = zcomp_workmem_get(zram->comp);
 	locked = true;
 	user_mem = kmap_atomic(page);
 
@@ -420,19 +400,18 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		goto out;
 	}
 
-	ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
-			       meta->compress_workmem);
+	ret = zcomp_compress(zram->comp, wm, uncmem, bvec->bv_len, &clen);
 	if (!is_partial_io(bvec)) {
 		kunmap_atomic(user_mem);
 		user_mem = NULL;
 		uncmem = NULL;
 	}
 
-	if (unlikely(ret != LZO_E_OK)) {
+	if (unlikely(ret)) {
 		pr_err("Compression failed! err=%d\n", ret);
 		goto out;
 	}
-
+	src = wm->buffer;
 	if (unlikely(clen > max_zpage_size)) {
 		clen = PAGE_SIZE;
 		src = NULL;
@@ -457,6 +436,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		memcpy(cmem, src, clen);
 	}
 
+	zcomp_workmem_put(zram->comp, wm);
+	locked = false;
 	zs_unmap_object(meta->mem_pool, handle);
 
 	/*
@@ -475,10 +456,9 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	atomic64_inc(&zram->stats.pages_stored);
 out:
 	if (locked)
-		mutex_unlock(&meta->buffer_lock);
+		zcomp_workmem_put(zram->comp, wm);
 	if (is_partial_io(bvec))
 		kfree(uncmem);
-
 	if (ret)
 		atomic64_inc(&zram->stats.failed_writes);
 	return ret;
@@ -522,6 +502,10 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
 		zs_free(meta->mem_pool, handle);
 	}
 
+	if (zram->comp)
+		zcomp_destroy(zram->comp);
+	zram->comp = NULL;
+
 	zram_meta_free(zram->meta);
 	zram->meta = NULL;
 	/* Reset stats */
@@ -550,9 +534,18 @@ static ssize_t disksize_store(struct device *dev,
 		return -EBUSY;
 	}
 
+	zram->comp = zcomp_create(default_compressor);
+	if (!zram->comp) {
+		up_write(&zram->init_lock);
+		pr_info("Cannot initialise compressing backend\n");
+		return -EINVAL;
+	}
+
 	disksize = PAGE_ALIGN(disksize);
 	zram->meta = zram_meta_alloc(disksize);
 	if (!zram->meta) {
+		zcomp_destroy(zram->comp);
+		zram->comp = NULL;
 		up_write(&zram->init_lock);
 		return -ENOMEM;
 	}
@@ -790,6 +783,7 @@ static int create_device(struct zram *zram, int device_id)
 	}
 
 	zram->meta = NULL;
+	zram->comp = NULL;
 	return 0;
 
 out_free_disk:
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 1d5b1f5..a2e17d7 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -16,9 +16,10 @@
 #define _ZRAM_DRV_H_
 
 #include <linux/spinlock.h>
-#include <linux/mutex.h>
 #include <linux/zsmalloc.h>
 
+#include "zram_comp.h"
+
 /*
  * Some arbitrary value. This is just to catch
  * invalid value for num_devices module parameter.
@@ -81,17 +82,16 @@ struct zram_stats {
 
 struct zram_meta {
 	rwlock_t tb_lock;	/* protect table */
-	void *compress_workmem;
-	void *compress_buffer;
 	struct table *table;
 	struct zs_pool *mem_pool;
-	struct mutex buffer_lock; /* protect compress buffers */
 };
 
 struct zram {
 	struct zram_meta *meta;
 	struct request_queue *queue;
 	struct gendisk *disk;
+	struct zram_comp *comp;
+
 	/* Prevent concurrent execution of device init, reset and R/W request */
 	struct rw_semaphore init_lock;
 	/*
-- 
1.9.0.rc3.244.g3497008

[PATCHv3 3/4] zram: support multi compressing buffers

[Sergey Senozhatsky]

1) Extend zram_comp zcomp_create() with `num_wm' parameter.
`num_wm' limits the number of workmem structs in compression
backend's idle list (max buffers).

2) Introduce zram device attribute max_buffers to show and
store current device's zram_comp max number (num_wm) of workmems.

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

set max_buffers to 4

	cat /sys/block/zram0/max_buffers

show current max_buffers (default value is 1).

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

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

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

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

diff --git a/drivers/block/zram/zram_comp.c b/drivers/block/zram/zram_comp.c
index 139a468..d9b34ba 100644
--- a/drivers/block/zram/zram_comp.c
+++ b/drivers/block/zram/zram_comp.c
@@ -118,11 +118,12 @@ static struct zram_comp_backend *find_backend(const char *compress)
  * allocate new zram_comp and initialize it. return NULL
  * if requested algorithm is not supported or in case
  * of init error */
-struct zram_comp *zcomp_create(const char *compress)
+struct zram_comp *zcomp_create(const char *compress, int num_wm)
 {
 	struct zram_comp *comp;
 	struct zram_comp_backend *backend;
 	struct zcomp_workmem *wm;
+	int i;
 
 	backend = find_backend(compress);
 	if (!backend)
@@ -137,11 +138,13 @@ struct zram_comp *zcomp_create(const char *compress)
 	INIT_LIST_HEAD(&comp->idle_workmem);
 	init_waitqueue_head(&comp->workmem_wait);
 
-	wm = workmem_alloc(comp);
-	if (!wm) {
-		zcomp_destroy(comp);
-		return NULL;
+	for (i = 0; i < num_wm; i++) {
+		wm = workmem_alloc(comp);
+		if (!wm) {
+			zcomp_destroy(comp);
+			return NULL;
+		}
+		list_add_tail(&wm->list, &comp->idle_workmem);
 	}
-	list_add_tail(&wm->list, &comp->idle_workmem);
 	return comp;
 }
diff --git a/drivers/block/zram/zram_comp.h b/drivers/block/zram/zram_comp.h
index 90355e8..277afa0 100644
--- a/drivers/block/zram/zram_comp.h
+++ b/drivers/block/zram/zram_comp.h
@@ -43,7 +43,7 @@ struct zram_comp {
 	struct zram_comp_backend *backend;
 };
 
-struct zram_comp *zcomp_create(const char *comp);
+struct zram_comp *zcomp_create(const char *comp, int num_wm);
 void zcomp_destroy(struct zram_comp *comp);
 
 struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp);
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 4875a16..a065c1b 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -107,6 +107,41 @@ static ssize_t mem_used_total_show(struct device *dev,
 	return sprintf(buf, "%llu\n", val);
 }
 
+static ssize_t max_buffers_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	long val;
+	struct zram *zram = dev_to_zram(dev);
+
+	down_read(&zram->init_lock);
+	val = zram->max_buffers;
+	up_read(&zram->init_lock);
+
+	return sprintf(buf, "%ld\n", val);
+}
+
+static ssize_t max_buffers_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	long num;
+	struct zram *zram = dev_to_zram(dev);
+
+	if (kstrtol(buf, 0, &num))
+		return -EINVAL;
+	if (num < 1 || num > 100)
+		return -EINVAL;
+	down_write(&zram->init_lock);
+	if (init_done(zram)) {
+		up_write(&zram->init_lock);
+		pr_info("Cannot set max buffers for initialized device\n");
+		return -EBUSY;
+	}
+	zram->max_buffers = num;
+	up_write(&zram->init_lock);
+
+	return len;
+}
+
 /* flag operations needs meta->tb_lock */
 static int zram_test_flag(struct zram_meta *meta, u32 index,
 			enum zram_pageflags flag)
@@ -505,6 +540,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
 	if (zram->comp)
 		zcomp_destroy(zram->comp);
 	zram->comp = NULL;
+	zram->max_buffers = 1;
 
 	zram_meta_free(zram->meta);
 	zram->meta = NULL;
@@ -534,7 +570,7 @@ static ssize_t disksize_store(struct device *dev,
 		return -EBUSY;
 	}
 
-	zram->comp = zcomp_create(default_compressor);
+	zram->comp = zcomp_create(default_compressor, zram->max_buffers);
 	if (!zram->comp) {
 		up_write(&zram->init_lock);
 		pr_info("Cannot initialise compressing backend\n");
@@ -697,6 +733,8 @@ static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
 static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
 static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
 static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
+static DEVICE_ATTR(max_buffers, S_IRUGO | S_IWUSR,
+		max_buffers_show, max_buffers_store);
 
 ZRAM_ATTR_RO(num_reads);
 ZRAM_ATTR_RO(num_writes);
@@ -721,6 +759,7 @@ static struct attribute *zram_disk_attrs[] = {
 	&dev_attr_orig_data_size.attr,
 	&dev_attr_compr_data_size.attr,
 	&dev_attr_mem_used_total.attr,
+	&dev_attr_max_buffers.attr,
 	NULL,
 };
 
@@ -784,6 +823,7 @@ static int create_device(struct zram *zram, int device_id)
 
 	zram->meta = NULL;
 	zram->comp = NULL;
+	zram->max_buffers = 1;
 	return 0;
 
 out_free_disk:
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index a2e17d7..68a5421 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -99,7 +99,7 @@ struct zram {
 	 * we can store in a disk.
 	 */
 	u64 disksize;	/* bytes */
-
+	long max_buffers;
 	struct zram_stats stats;
 };
 #endif
-- 
1.9.0.rc3.244.g3497008

[PATCHv3 4/4] zram: document max_buffers

[Sergey Senozhatsky]

Add max_buffers device attribute documentation.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 Documentation/ABI/testing/sysfs-block-zram |  9 ++++++++-
 Documentation/blockdev/zram.txt            | 21 ++++++++++++++++-----
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-block-zram b/Documentation/ABI/testing/sysfs-block-zram
index 8aa0468..0e36bf7 100644
--- a/Documentation/ABI/testing/sysfs-block-zram
+++ b/Documentation/ABI/testing/sysfs-block-zram
@@ -50,7 +50,6 @@ Description:
 		The failed_reads file is read-only and specifies the number of
 		failed reads happened on this device.
 
-
 What:		/sys/block/zram<id>/failed_writes
 Date:		February 2014
 Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
@@ -58,6 +57,14 @@ Description:
 		The failed_writes file is read-only and specifies the number of
 		failed writes happened on this device.
 
+What:		/sys/block/zram<id>/max_buffers
+Date:		February 2014
+Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
+Description:
+		The max_buffers file is read-write and specifies the number of
+		compression backend's workmem buffers (number of concurrent
+		compress/decompress operations).
+
 What:		/sys/block/zram<id>/notify_free
 Date:		August 2010
 Contact:	Nitin Gupta <ngupta@vflare.org>
diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index b31ac5e..b525c8e 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -21,7 +21,18 @@ Following shows a typical sequence of steps for using zram.
 	This creates 4 devices: /dev/zram{0,1,2,3}
 	(num_devices parameter is optional. Default: 1)
 
-2) Set Disksize
+2) Set max number of compression buffers
+	Compression backend may use up to max_buffers workmem buffers,
+	thus allowing up to max_buffers concurrent compression/decompression
+	operations.
+	Examples:
+	#set max buffers to 3
+	echo 3 > /sys/block/zram0/max_buffers
+
+	#show max buffers
+	cat /sys/block/zram0/max_buffers
+
+3) Set Disksize
         Set disk size by writing the value to sysfs node 'disksize'.
         The value can be either in bytes or you can use mem suffixes.
         Examples:
@@ -38,14 +49,14 @@ There is little point creating a zram of greater than twice the size of memory
 since we expect a 2:1 compression ratio. Note that zram uses about 0.1% of the
 size of the disk when not in use so a huge zram is wasteful.
 
-3) Activate:
+4) Activate:
 	mkswap /dev/zram0
 	swapon /dev/zram0
 
 	mkfs.ext4 /dev/zram1
 	mount /dev/zram1 /tmp
 
-4) Stats:
+5) Stats:
 	Per-device statistics are exported as various nodes under
 	/sys/block/zram<id>/
 		disksize
@@ -60,11 +71,11 @@ size of the disk when not in use so a huge zram is wasteful.
 		compr_data_size
 		mem_used_total
 
-5) Deactivate:
+6) Deactivate:
 	swapoff /dev/zram0
 	umount /dev/zram1
 
-6) Reset:
+7) Reset:
 	Write any positive value to 'reset' sysfs node
 	echo 1 > /sys/block/zram0/reset
 	echo 1 > /sys/block/zram1/reset
-- 
1.9.0.rc3.244.g3497008

Re: [PATCHv3 1/4] zram: introduce compressing backend abstraction

[Minchan Kim]

Hello Sergey,

On Wed, Feb 12, 2014 at 01:21:03AM +0300, Sergey Senozhatsky wrote:
> ZRAM performs direct LZO compression algorithm calls, making it the one
> and only option. Introduce abstract struct zram_comp in order to support
> multiple compression algorithms. struct zram_comp defines the following
> set of compressing backend operations:
>         .create
>         .destroy
>         .compress
>         .decompress
> 
> Schematically zram write() usually contains the following steps:
> 0) preparation (decompression of partioal IO, etc.)
> 1) lock buffer_lock mutex (protects meta compress buffers)
> 2) compress (using meta compress buffers)
> 3) alloc and map zs_pool object
> 4) copy compressed data (from meta compress buffers) to object allocated by 3)
> 5) free previous pool page, assign a new one
> 6) unlock buffer_lock mutex
> 
> As we can see, compressing buffers must remain untouched from 1) to 4),
> because, otherwise, concurrent write() can overwrite data. At the same
> time, zram_meta must be aware of a) specific compression algorithm
> memory requirements and b) necessary locking to protect compression
> buffers. Besides, zram holds buffer_lock almost through the whole write()
> function, making parallel compression impossible. To remove requirement
> a) new struct zcomp_workmem introduced, which contain buffers required by
> compression algorithm. While struct zram_comp implements workmem handling
> and locking by means of get() and put() semantics and removes requirement
> b) from zram meta. zram_comp ->create() and ->destroy(), respectively,
> allocate and deallocate algorithm specific zcomp_workmem `private' buffer.
> 
> Every zram_comp has a list of idle workmem buffers (at least 1 workmem),
> spinlock to protect idle list and wait queue, making it possible to perform
> parallel compressions. Each time zram issues a zcomp_workmem_get() call, the
> following set of operations performed:
> - spin lock buffer_lock
> - if idle list is not empty, remove workmem from idle list, spin
>   unlock and return workmem pointer
> - if idle list is empty, current adds itself to wait queue. it will be
>   awaken by zcomp_workmem_put() caller.
> 
> zcomp_workmem_put():
> - spin lock buffer_lock
> - add workmem to idle list
> - spin unlock, wake up sleeper (if any)
> 
> In other words, zcomp_workmem_get() turns caller into exclusive user of workmem
> and zcomp_workem_put() makes a particular workmem available.
> 
> Usage examples.
> 
> To initialize compressing backend:
> 	comp = zcomp_create(NAME) /* NAME e.g. lzo */
> 
> which initialises compressing backend if requested algorithm is supported.
> 
> Compress:
> 	wm = zcomp_workmem_get(comp)
> 	zcomp_compress(comp, wm, src, src_len, &dst_len)
> 	[..] /* copy compressed data */
> 	zcomp_workmem_put(comp, wm)
> 
> Decompress:
> 	zcomp_decompress(comp, src, src_len, dst, &dst_len);
> 
> Free compessing backend and its workmem:
> 	zcomp_destroy(comp)
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/zcomp_lzo.c |  33 +++++++++
>  drivers/block/zram/zram_comp.c | 147 +++++++++++++++++++++++++++++++++++++++++
>  drivers/block/zram/zram_comp.h |  58 ++++++++++++++++
>  3 files changed, 238 insertions(+)
>  create mode 100644 drivers/block/zram/zcomp_lzo.c
>  create mode 100644 drivers/block/zram/zram_comp.c
>  create mode 100644 drivers/block/zram/zram_comp.h
> 
> diff --git a/drivers/block/zram/zcomp_lzo.c b/drivers/block/zram/zcomp_lzo.c
> new file mode 100644
> index 0000000..bbde74e
> --- /dev/null
> +++ b/drivers/block/zram/zcomp_lzo.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/lzo.h>
> +
> +#include "zram_comp.h"
> +
> +static void * lzo_create(void)
> +{
> +	return kmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> +}
> +
> +static void lzo_destroy(void *workmem)
> +{
> +	kfree(workmem);
> +}

It's okay to name "workmem" for lzo because lzo needs only work buffer to work
but let's think about zlib. It needs z_streamp which has many feilds
to work as well as work memory. That's why I don't like "workmem" name which
is very specific. Anyway, it's okay for lzo backend now.

> +
> +extern struct zram_comp_backend zcomp_lzo;
> +struct zram_comp_backend zcomp_lzo = {
> +	.compress = lzo1x_1_compress,
> +	.decompress = lzo1x_decompress_safe,
> +	.create = lzo_create,
> +	.destroy = lzo_destroy,
> +	.name = "lzo",
> +};
> diff --git a/drivers/block/zram/zram_comp.c b/drivers/block/zram/zram_comp.c
> new file mode 100644
> index 0000000..139a468
> --- /dev/null
> +++ b/drivers/block/zram/zram_comp.c
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/wait.h>
> +#include <linux/sched.h>
> +
> +#include "zram_comp.h"
> +
> +extern struct zram_comp_backend zcomp_lzo;
> +
> +static void workmem_free(struct zram_comp *comp, struct zcomp_workmem *workmem)
> +{
> +	comp->backend->destroy(workmem->private);
> +	free_pages((unsigned long)workmem->buffer, 1);
> +	kfree(workmem);
> +}
> +
> +/* allocate new workmem structure with ->mem of requested size,
> + * return NULL on error */
> +static struct zcomp_workmem *workmem_alloc(struct zram_comp *comp)
> +{
> +	struct zcomp_workmem *workmem = kmalloc(sizeof(*workmem), GFP_NOFS);
> +	if (!workmem)
> +		return NULL;
> +
> +	INIT_LIST_HEAD(&workmem->list);
> +	/* algorithm specific working memory buffer */
> +	workmem->private = comp->backend->create();
> +	/* allocate 2 pages. 1 for compressed data, plus 1 extra for the
> +	 * case when compressed size is larger than the original one. */
> +	workmem->buffer = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
> +	if (!workmem->private || !workmem->buffer)
> +		goto fail;
> +
> +	return workmem;
> +fail:
> +	workmem_free(comp, workmem);
> +	return NULL;
> +}

Hmm, you introduced workmem again. :(
As I said above, workmem is too specific.
It's only valid for lzo and lz4 so I don't want to expose workmem out of zcomp
and that's why I suggested zcomp_strm or zcomp_stream.
It seems "stream" is more general term for compressor.

> +
> +/* get existing idle workmem or wait until other process release
> + * (workmem_put()) one for us */
> +struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp)
> +{
> +	struct zcomp_workmem *wm;
> +retry:
> +	spin_lock(&comp->buffer_lock);
> +	if (list_empty(&comp->idle_workmem)) {
> +		spin_unlock(&comp->buffer_lock);
> +		wait_event(comp->workmem_wait,
> +				!list_empty(&comp->idle_workmem));
> +		goto retry;
> +	}
> +
> +	wm = list_entry(comp->idle_workmem.next,
> +			struct zcomp_workmem, list);
> +	list_del(&wm->list);
> +	spin_unlock(&comp->buffer_lock);
> +	return wm;
> +}
> +
> +/* add workmem back to idle list and wake up waiter (if any) */
> +void zcomp_workmem_put(struct zram_comp *comp,
> +		struct zcomp_workmem *workmem)
> +{
> +	spin_lock(&comp->buffer_lock);
> +	list_add_tail(&workmem->list, &comp->idle_workmem);

Why do you implement queue instead of stack?

> +	spin_unlock(&comp->buffer_lock);
> +
> +	if (waitqueue_active(&comp->workmem_wait))

Possible losting wakeup?

> +		wake_up(&comp->workmem_wait);
> +}
> +
> +int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
> +		const unsigned char *src, size_t src_len, size_t *dst_len)
> +{

As I said, I don't like zcomp_workmem.
Please use zcomp_strm and let's make naming clear.
Something is zcomp and sometime is zram_comp.
I prefer zcomp but it depends on you.

> +	return comp->backend->compress(src, src_len, workmem->buffer,
> +			dst_len, workmem->private);
> +}
> +
> +int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
> +		size_t src_len, unsigned char *dst, size_t *dst_len)
> +{
> +	return comp->backend->decompress(src, src_len, dst, dst_len);
> +}
> +
> +/* free allocated workmem buffers and zram_comp */
> +void zcomp_destroy(struct zram_comp *comp)
> +{
> +	struct zcomp_workmem *wm;
> +	while (!list_empty(&comp->idle_workmem)) {
> +		wm = list_entry(comp->idle_workmem.next,
> +				struct zcomp_workmem, list);
> +		list_del(&wm->list);
> +		workmem_free(comp, wm);
> +	}
> +	kfree(comp);
> +}
> +
> +static struct zram_comp_backend *find_backend(const char *compress)
> +{
> +	if (sysfs_streq(compress, "lzo"))
> +		return &zcomp_lzo;
> +	return NULL;
> +}
> +
> +/* search available compressors for requested algorithm.

Please correct comment style.

> + * allocate new zram_comp and initialize it. return NULL
> + * if requested algorithm is not supported or in case
> + * of init error */
> +struct zram_comp *zcomp_create(const char *compress)
> +{
> +	struct zram_comp *comp;
> +	struct zram_comp_backend *backend;
> +	struct zcomp_workmem *wm;
> +
> +	backend = find_backend(compress);
> +	if (!backend)
> +		return NULL;
> +
> +	comp = kzalloc(sizeof(struct zram_comp), GFP_KERNEL);
> +	if (!comp)
> +		return NULL;
> +
> +	comp->backend = backend;
> +	spin_lock_init(&comp->buffer_lock);
> +	INIT_LIST_HEAD(&comp->idle_workmem);
> +	init_waitqueue_head(&comp->workmem_wait);
> +
> +	wm = workmem_alloc(comp);
> +	if (!wm) {
> +		zcomp_destroy(comp);
> +		return NULL;
> +	}
> +	list_add_tail(&wm->list, &comp->idle_workmem);
> +	return comp;
> +}
> diff --git a/drivers/block/zram/zram_comp.h b/drivers/block/zram/zram_comp.h
> new file mode 100644
> index 0000000..90355e8
> --- /dev/null
> +++ b/drivers/block/zram/zram_comp.h
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#ifndef _ZRAM_COMP_H_
> +#define _ZRAM_COMP_H_
> +
> +#include <linux/types.h>
> +#include <linux/spinlock.h>
> +
> +struct zcomp_workmem {
> +	void *buffer;     /* compression/decompression buffer */
> +	void *private;      /* algorithm workmem */
> +	struct list_head list;
> +};
> +
> +/* static compression backend */
> +struct zram_comp_backend {
> +	int (*compress)(const unsigned char *src, size_t src_len,
> +			unsigned char *dst, size_t *dst_len, void *wrkmem);

zram works based on PAGE so I like compress_page so maybe we couldn't
reduce a argument "src_len".

> +
> +	int (*decompress)(const unsigned char *src, size_t src_len,
> +			unsigned char *dst, size_t *dst_len);
> +
> +	void *(*create)(void);
> +	void (*destroy)(void *workmem);
> +
> +	const char *name;
> +};
> +
> +/* dynamic per-device compression frontend */
> +struct zram_comp {
> +	/* protect workmem list */
> +	spinlock_t buffer_lock;
> +	/* list of available workmems */
> +	struct list_head idle_workmem;
> +	wait_queue_head_t workmem_wait;
> +	struct zram_comp_backend *backend;
> +};
> +
> +struct zram_comp *zcomp_create(const char *comp);
> +void zcomp_destroy(struct zram_comp *comp);
> +
> +struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp);
> +void zcomp_workmem_put(struct zram_comp *comp,
> +		struct zcomp_workmem *workmem);
> +
> +int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
> +		const unsigned char *src, size_t src_len, size_t *dst_len);
> +
> +int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
> +		size_t src_len, unsigned char *dst, size_t *dst_len);
> +#endif /* _ZRAM_COMP_H_ */
> -- 
> 1.9.0.rc3.244.g3497008
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Kind regards,
Minchan Kim

Re: [PATCHv3 1/4] zram: introduce compressing backend abstraction

[Minchan Kim]

On Wed, Feb 12, 2014 at 01:21:03AM +0300, Sergey Senozhatsky wrote:
> ZRAM performs direct LZO compression algorithm calls, making it the one
> and only option. Introduce abstract struct zram_comp in order to support
> multiple compression algorithms. struct zram_comp defines the following
> set of compressing backend operations:
>         .create
>         .destroy
>         .compress
>         .decompress
> 
> Schematically zram write() usually contains the following steps:
> 0) preparation (decompression of partioal IO, etc.)
> 1) lock buffer_lock mutex (protects meta compress buffers)
> 2) compress (using meta compress buffers)
> 3) alloc and map zs_pool object
> 4) copy compressed data (from meta compress buffers) to object allocated by 3)
> 5) free previous pool page, assign a new one
> 6) unlock buffer_lock mutex
> 
> As we can see, compressing buffers must remain untouched from 1) to 4),
> because, otherwise, concurrent write() can overwrite data. At the same
> time, zram_meta must be aware of a) specific compression algorithm
> memory requirements and b) necessary locking to protect compression
> buffers. Besides, zram holds buffer_lock almost through the whole write()
> function, making parallel compression impossible. To remove requirement
> a) new struct zcomp_workmem introduced, which contain buffers required by
> compression algorithm. While struct zram_comp implements workmem handling
> and locking by means of get() and put() semantics and removes requirement
> b) from zram meta. zram_comp ->create() and ->destroy(), respectively,
> allocate and deallocate algorithm specific zcomp_workmem `private' buffer.
> 
> Every zram_comp has a list of idle workmem buffers (at least 1 workmem),
> spinlock to protect idle list and wait queue, making it possible to perform
> parallel compressions. Each time zram issues a zcomp_workmem_get() call, the
> following set of operations performed:
> - spin lock buffer_lock
> - if idle list is not empty, remove workmem from idle list, spin
>   unlock and return workmem pointer
> - if idle list is empty, current adds itself to wait queue. it will be
>   awaken by zcomp_workmem_put() caller.
> 
> zcomp_workmem_put():
> - spin lock buffer_lock
> - add workmem to idle list
> - spin unlock, wake up sleeper (if any)
> 
> In other words, zcomp_workmem_get() turns caller into exclusive user of workmem
> and zcomp_workem_put() makes a particular workmem available.
> 
> Usage examples.
> 
> To initialize compressing backend:
> 	comp = zcomp_create(NAME) /* NAME e.g. lzo */
> 
> which initialises compressing backend if requested algorithm is supported.
> 
> Compress:
> 	wm = zcomp_workmem_get(comp)
> 	zcomp_compress(comp, wm, src, src_len, &dst_len)
> 	[..] /* copy compressed data */
> 	zcomp_workmem_put(comp, wm)
> 
> Decompress:
> 	zcomp_decompress(comp, src, src_len, dst, &dst_len);
> 
> Free compessing backend and its workmem:
> 	zcomp_destroy(comp)
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/zcomp_lzo.c |  33 +++++++++
>  drivers/block/zram/zram_comp.c | 147 +++++++++++++++++++++++++++++++++++++++++
>  drivers/block/zram/zram_comp.h |  58 ++++++++++++++++
>  3 files changed, 238 insertions(+)
>  create mode 100644 drivers/block/zram/zcomp_lzo.c
>  create mode 100644 drivers/block/zram/zram_comp.c
>  create mode 100644 drivers/block/zram/zram_comp.h
> 
> diff --git a/drivers/block/zram/zcomp_lzo.c b/drivers/block/zram/zcomp_lzo.c
> new file mode 100644
> index 0000000..bbde74e
> --- /dev/null
> +++ b/drivers/block/zram/zcomp_lzo.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/lzo.h>
> +
> +#include "zram_comp.h"
> +
> +static void * lzo_create(void)
> +{
> +	return kmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> +}
> +
> +static void lzo_destroy(void *workmem)
> +{
> +	kfree(workmem);
> +}
> +
> +extern struct zram_comp_backend zcomp_lzo;
> +struct zram_comp_backend zcomp_lzo = {
> +	.compress = lzo1x_1_compress,
> +	.decompress = lzo1x_decompress_safe,
> +	.create = lzo_create,
> +	.destroy = lzo_destroy,
> +	.name = "lzo",
> +};
> diff --git a/drivers/block/zram/zram_comp.c b/drivers/block/zram/zram_comp.c
> new file mode 100644
> index 0000000..139a468
> --- /dev/null
> +++ b/drivers/block/zram/zram_comp.c
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (C) 2014 Sergey Senozhatsky.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/wait.h>
> +#include <linux/sched.h>
> +
> +#include "zram_comp.h"
> +
> +extern struct zram_comp_backend zcomp_lzo;
> +
> +static void workmem_free(struct zram_comp *comp, struct zcomp_workmem *workmem)
> +{
> +	comp->backend->destroy(workmem->private);
> +	free_pages((unsigned long)workmem->buffer, 1);
> +	kfree(workmem);
> +}
> +
> +/* allocate new workmem structure with ->mem of requested size,
> + * return NULL on error */
> +static struct zcomp_workmem *workmem_alloc(struct zram_comp *comp)
> +{
> +	struct zcomp_workmem *workmem = kmalloc(sizeof(*workmem), GFP_NOFS);


One more thing, pz, say why we need GFP_NOFS in here.

-- 
Kind regards,
Minchan Kim

Re: [PATCHv3 1/4] zram: introduce compressing backend abstraction

[Sergey Senozhatsky]

On (02/12/14 16:08), Minchan Kim wrote:
[..]
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <linux/slab.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/wait.h>
> > +#include <linux/sched.h>
> > +
> > +#include "zram_comp.h"
> > +
> > +extern struct zram_comp_backend zcomp_lzo;
> > +
> > +static void workmem_free(struct zram_comp *comp, struct zcomp_workmem *workmem)
> > +{
> > +	comp->backend->destroy(workmem->private);
> > +	free_pages((unsigned long)workmem->buffer, 1);
> > +	kfree(workmem);
> > +}
> > +
> > +/* allocate new workmem structure with ->mem of requested size,
> > + * return NULL on error */
> > +static struct zcomp_workmem *workmem_alloc(struct zram_comp *comp)
> > +{
> > +	struct zcomp_workmem *workmem = kmalloc(sizeof(*workmem), GFP_NOFS);
> 
> 
> One more thing, pz, say why we need GFP_NOFS in here.
>

my bad. it definitely should be GFP_KERNEL.

	-ss

Re: [PATCHv3 1/4] zram: introduce compressing backend abstraction

[Sergey Senozhatsky]

On (02/12/14 16:06), Minchan Kim wrote:
[..]
> > +{
> > +	kfree(workmem);
> > +}
> 
> It's okay to name "workmem" for lzo because lzo needs only work buffer to work
> but let's think about zlib. It needs z_streamp which has many feilds
> to work as well as work memory. That's why I don't like "workmem" name which
> is very specific. Anyway, it's okay for lzo backend now.
> 

ok. will use zcomp_strm

[..]
> > +	return workmem;
> > +fail:
> > +	workmem_free(comp, workmem);
> > +	return NULL;
> > +}
> 
> Hmm, you introduced workmem again. :(
> As I said above, workmem is too specific.
> It's only valid for lzo and lz4 so I don't want to expose workmem out of zcomp
> and that's why I suggested zcomp_strm or zcomp_stream.
> It seems "stream" is more general term for compressor.
> 

> > +/* add workmem back to idle list and wake up waiter (if any) */
> > +void zcomp_workmem_put(struct zram_comp *comp,
> > +		struct zcomp_workmem *workmem)
> > +{
> > +	spin_lock(&comp->buffer_lock);
> > +	list_add_tail(&workmem->list, &comp->idle_workmem);
> 
> Why do you implement queue instead of stack?
> 

zcomp gets the first element from a list and never iterates it,
so it's O(1).
kfifo with copying and write memory barriers... I don't think
it will be a better choice here. and we don't have `first in,
first out' pattern, zcomp does not care about order.

> > +	spin_unlock(&comp->buffer_lock);
> > +
> > +	if (waitqueue_active(&comp->workmem_wait))
> 
> Possible losting wakeup?

hm, could be.

> > +		wake_up(&comp->workmem_wait);
> > +}
> > +
> > +int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
> > +		const unsigned char *src, size_t src_len, size_t *dst_len)
> > +{
> 
> As I said, I don't like zcomp_workmem.
> Please use zcomp_strm and let's make naming clear.
> Something is zcomp and sometime is zram_comp.
> I prefer zcomp but it depends on you.

ok. I drop zram part. zcomp and zcomp_strm

> > +	return comp->backend->compress(src, src_len, workmem->buffer,
> > +			dst_len, workmem->private);
> > +}
> > +
> > +int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
> > +		size_t src_len, unsigned char *dst, size_t *dst_len)
> > +{
> > +	return comp->backend->decompress(src, src_len, dst, dst_len);
> > +}
> > +
> > +/* free allocated workmem buffers and zram_comp */
> > +void zcomp_destroy(struct zram_comp *comp)
> > +{
> > +	struct zcomp_workmem *wm;
> > +	while (!list_empty(&comp->idle_workmem)) {
> > +		wm = list_entry(comp->idle_workmem.next,
> > +				struct zcomp_workmem, list);
> > +		list_del(&wm->list);
> > +		workmem_free(comp, wm);
> > +	}
> > +	kfree(comp);
> > +}
> > +
> > +static struct zram_comp_backend *find_backend(const char *compress)
> > +{
> > +	if (sysfs_streq(compress, "lzo"))
> > +		return &zcomp_lzo;
> > +	return NULL;
> > +}
> > +
> > +/* search available compressors for requested algorithm.
> 
> Please correct comment style.

I'll take a look (checkpatch didn't complain).

> > +/* static compression backend */
> > +struct zram_comp_backend {
> > +	int (*compress)(const unsigned char *src, size_t src_len,
> > +			unsigned char *dst, size_t *dst_len, void *wrkmem);
> 
> zram works based on PAGE so I like compress_page so maybe we couldn't
> reduce a argument "src_len".
> 

yes and no.

zram can see partial IOs

static inline int is_partial_io(struct bio_vec *bvec)
{
	return bvec->bv_len != PAGE_SIZE;
}

so, in general yes -- we can drop src_len and compress PAGE_SIZE bytes,
but at the same time `bvec->bv_len < PAGE_SIZE' is possible (if it's not
then we can greatly simplify zram).

	-ss

> > +
> > +	int (*decompress)(const unsigned char *src, size_t src_len,
> > +			unsigned char *dst, size_t *dst_len);
> > +
> > +	void *(*create)(void);
> > +	void (*destroy)(void *workmem);
> > +
> > +	const char *name;
> > +};
> > +
> > +/* dynamic per-device compression frontend */
> > +struct zram_comp {
> > +	/* protect workmem list */
> > +	spinlock_t buffer_lock;
> > +	/* list of available workmems */
> > +	struct list_head idle_workmem;
> > +	wait_queue_head_t workmem_wait;
> > +	struct zram_comp_backend *backend;
> > +};
> > +
> > +struct zram_comp *zcomp_create(const char *comp);
> > +void zcomp_destroy(struct zram_comp *comp);
> > +
> > +struct zcomp_workmem *zcomp_workmem_get(struct zram_comp *comp);
> > +void zcomp_workmem_put(struct zram_comp *comp,
> > +		struct zcomp_workmem *workmem);
> > +
> > +int zcomp_compress(struct zram_comp *comp, struct zcomp_workmem *workmem,
> > +		const unsigned char *src, size_t src_len, size_t *dst_len);
> > +
> > +int zcomp_decompress(struct zram_comp *comp, const unsigned char *src,
> > +		size_t src_len, unsigned char *dst, size_t *dst_len);
> > +#endif /* _ZRAM_COMP_H_ */
> > -- 
> > 1.9.0.rc3.244.g3497008
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> -- 
> Kind regards,
> Minchan Kim
>