[RFC PATCH 0/3] add LZ4 compression support

[RFC PATCH 0/3] add LZ4 compression support

[Sergey Senozhatsky]

This patchset introduces support for LZ4 compression
(along with LZO) and adds functionality to list and change
compression algorithms, using new `compressor' device attribute.

Sergey Senozhatsky (3):
  zram: delete zram_init_device() function
  zram: introduce zram compressor operations struct
  zram: list and select compression algorithms

 Documentation/blockdev/zram.txt | 23 +++++++++---
 drivers/block/zram/Kconfig      |  2 +
 drivers/block/zram/zram_drv.c   | 83 +++++++++++++++++++++++++++++------------
 drivers/block/zram/zram_drv.h   | 10 +++++
 4 files changed, 89 insertions(+), 29 deletions(-)

-- 
1.8.5.3.493.gb139ac2

[RFC PATCH 1/3] zram: delete zram_init_device() function

[Sergey Senozhatsky]

allocate new `zram_meta' in disksize_store() only for uninitialised
zram device, saving a number of allocations and deallocations in case
if disksize_store() was called on currently used device. at the same
time zram_meta stack variable is not necessary, because we can set
->meta directly. there is also no need in setting QUEUE_FLAG_NONROT
queue on every disksize_store(), set it once during device creation.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 5ec61be..7124042 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -533,38 +533,28 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
 	up_write(&zram->init_lock);
 }
 
-static void zram_init_device(struct zram *zram, struct zram_meta *meta)
-{
-	/* zram devices sort of resembles non-rotational disks */
-	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
-	zram->meta = meta;
-	pr_debug("Initialization done!\n");
-}
-
 static ssize_t disksize_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t len)
 {
 	u64 disksize;
-	struct zram_meta *meta;
 	struct zram *zram = dev_to_zram(dev);
 
 	disksize = memparse(buf, NULL);
 	if (!disksize)
 		return -EINVAL;
 
-	disksize = PAGE_ALIGN(disksize);
-	meta = zram_meta_alloc(disksize);
 	down_write(&zram->init_lock);
 	if (init_done(zram)) {
 		up_write(&zram->init_lock);
-		zram_meta_free(meta);
 		pr_info("Cannot change disksize for initialized device\n");
 		return -EBUSY;
 	}
 
+	disksize = PAGE_ALIGN(disksize);
+	zram->meta = zram_meta_alloc(disksize);
+
 	zram->disksize = disksize;
 	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
-	zram_init_device(zram, meta);
 	up_write(&zram->init_lock);
 
 	return len;
@@ -774,7 +764,8 @@ static int create_device(struct zram *zram, int device_id)
 
 	/* Actual capacity set using syfs (/sys/block/zram<id>/disksize */
 	set_capacity(zram->disk, 0);
-
+	/* zram devices sort of resembles non-rotational disks */
+	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
 	/*
 	 * To ensure that we always get PAGE_SIZE aligned
 	 * and n*PAGE_SIZED sized I/O requests.
-- 
1.8.5.3.493.gb139ac2

[RFC PATCH 2/3] zram: introduce zram compressor operations struct

[Sergey Senozhatsky]

This is preparation patch to add LZ4 compression support.

struct zram_compress_ops defines common compress and decompress
prototypes. Use these ops->compress and ops->decompress callbacks
instead of direct LZO lzo1x_1_compress() and lzo1x_decompress_safe()
calls.

Compressor ops should be defined before zram meta allocation,
because the size of meta->compress_workmem depends on selected
compression algorithm.

Define LZO compressor ops and set it as the only one available
zram compressor at the moment.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 20 +++++++++++++-------
 drivers/block/zram/zram_drv.h | 10 ++++++++++
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 7124042..4f2c748 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -28,10 +28,9 @@
 #include <linux/device.h>
 #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 <linux/lzo.h>
 
 #include "zram_drv.h"
 
@@ -42,6 +41,12 @@ static struct zram *zram_devices;
 /* Module params (documentation at end) */
 static unsigned int num_devices = 1;
 
+static struct zram_compress_ops lzo_compressor = {
+	.workmem_sz = LZO1X_MEM_COMPRESS,
+	.compress = lzo1x_1_compress,
+	.decompress = lzo1x_decompress_safe,
+};
+
 #define ZRAM_ATTR_RO(name)						\
 static ssize_t zram_attr_##name##_show(struct device *d,		\
 				struct device_attribute *attr, char *b)	\
@@ -166,14 +171,14 @@ static void zram_meta_free(struct zram_meta *meta)
 	kfree(meta);
 }
 
-static struct zram_meta *zram_meta_alloc(u64 disksize)
+static struct zram_meta *zram_meta_alloc(int workmem_sz, u64 disksize)
 {
 	size_t num_pages;
 	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
 	if (!meta)
 		goto out;
 
-	meta->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
+	meta->compress_workmem = kzalloc(workmem_sz, GFP_KERNEL);
 	if (!meta->compress_workmem)
 		goto free_meta;
 
@@ -301,7 +306,7 @@ 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 = zram->ops->decompress(cmem, size, mem, &clen);
 	zs_unmap_object(meta->mem_pool, handle);
 	read_unlock(&meta->tb_lock);
 
@@ -420,7 +425,7 @@ 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,
+	ret = zram->ops->compress(uncmem, PAGE_SIZE, src, &clen,
 			       meta->compress_workmem);
 	if (!is_partial_io(bvec)) {
 		kunmap_atomic(user_mem);
@@ -551,7 +556,7 @@ static ssize_t disksize_store(struct device *dev,
 	}
 
 	disksize = PAGE_ALIGN(disksize);
-	zram->meta = zram_meta_alloc(disksize);
+	zram->meta = zram_meta_alloc(zram->ops->workmem_sz, disksize);
 
 	zram->disksize = disksize;
 	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
@@ -785,6 +790,7 @@ static int create_device(struct zram *zram, int device_id)
 		goto out_free_disk;
 	}
 
+	zram->ops = &lzo_compressor;
 	zram->meta = NULL;
 	return 0;
 
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 1d5b1f5..5488682 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -88,8 +88,18 @@ struct zram_meta {
 	struct mutex buffer_lock; /* protect compress buffers */
 };
 
+/* compression/decompression functions and algorithm-specific workmem size */
+struct zram_compress_ops {
+	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);
+	long workmem_sz;
+};
+
 struct zram {
 	struct zram_meta *meta;
+	struct zram_compress_ops *ops;
 	struct request_queue *queue;
 	struct gendisk *disk;
 	/* Prevent concurrent execution of device init, reset and R/W request */
-- 
1.8.5.3.493.gb139ac2

[RFC PATCH 3/3] zram: list and select compression algorithms

[Sergey Senozhatsky]

Add compressor device attr that allows to list and select
compression algorithms.

Define and make available for selection LZ4 compressor ops.

usage example:
List available compression algorithms (currently selected
one is LZO):
  cat /sys/block/zram0/compressor
  <lzo> lz4

Change compression algorithm to LZ4:
  echo lz4 > /sys/block/zram0/compressor
  cat /sys/block/zram0/compressor
  lzo <lz4>

Update documentation with "Select compression algorithm" section

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 Documentation/blockdev/zram.txt | 23 ++++++++++++++++-----
 drivers/block/zram/Kconfig      |  2 ++
 drivers/block/zram/zram_drv.c   | 46 +++++++++++++++++++++++++++++++++++++----
 3 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index 393541be..af90d29 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -21,7 +21,20 @@ 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) Select compression algorithm
+'compressor' sysfs node allows to see available, currently used
+and change compression algorithms.
+In order to list available compression algorithms (currently selected
+one is LZO):
+	cat /sys/block/zram0/compressor
+	<lzo> lz4
+
+Change compression algorithm to LZ4:
+	echo lz4 > /sys/block/zram0/compressor
+	cat /sys/block/zram0/compressor
+	lzo <lz4>
+
+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 +51,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
@@ -59,11 +72,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
diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
index 3450be8..09dacd6 100644
--- a/drivers/block/zram/Kconfig
+++ b/drivers/block/zram/Kconfig
@@ -3,6 +3,8 @@ config ZRAM
 	depends on BLOCK && SYSFS && ZSMALLOC
 	select LZO_COMPRESS
 	select LZO_DECOMPRESS
+	select LZ4_COMPRESS
+	select LZ4_DECOMPRESS
 	default n
 	help
 	  Creates virtual block devices called /dev/zramX (X = 0, 1, ...).
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 4f2c748..f1434f8 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -31,6 +31,7 @@
 #include <linux/string.h>
 #include <linux/vmalloc.h>
 #include <linux/lzo.h>
+#include <linux/lz4.h>
 
 #include "zram_drv.h"
 
@@ -47,6 +48,12 @@ static struct zram_compress_ops lzo_compressor = {
 	.decompress = lzo1x_decompress_safe,
 };
 
+static struct zram_compress_ops lz4_compressor = {
+	.workmem_sz = LZ4_MEM_COMPRESS,
+	.compress = lz4_compress,
+	.decompress = lz4_decompress_unknownoutputsize,
+};
+
 #define ZRAM_ATTR_RO(name)						\
 static ssize_t zram_attr_##name##_show(struct device *d,		\
 				struct device_attribute *attr, char *b)	\
@@ -113,6 +120,34 @@ static ssize_t mem_used_total_show(struct device *dev,
 	return sprintf(buf, "%llu\n", val);
 }
 
+static ssize_t compressor_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct zram *zram = dev_to_zram(dev);
+	if (zram->ops == &lzo_compressor)
+		return sprintf(buf, "<lzo> lz4\n");
+	return sprintf(buf, "lzo <lz4>\n");
+}
+
+static ssize_t compressor_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct zram *zram = dev_to_zram(dev);
+
+	down_write(&zram->init_lock);
+	if (init_done(zram)) {
+		up_write(&zram->init_lock);
+		pr_info("Cannot change compressor for initialized device\n");
+		return -EBUSY;
+	}
+	if (strncmp(buf, "lzo", 3) == 0)
+		zram->ops = &lzo_compressor;
+	else
+		zram->ops = &lz4_compressor;
+	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)
@@ -285,7 +320,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;
@@ -311,7 +346,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 	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;
@@ -354,7 +389,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))
@@ -433,7 +468,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		uncmem = NULL;
 	}
 
-	if (unlikely(ret != LZO_E_OK)) {
+	if (unlikely(ret)) {
 		pr_err("Compression failed! err=%d\n", ret);
 		goto out;
 	}
@@ -705,6 +740,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(compressor, S_IRUGO | S_IWUSR,
+		compressor_show, compressor_store);
 
 ZRAM_ATTR_RO(num_reads);
 ZRAM_ATTR_RO(num_writes);
@@ -719,6 +756,7 @@ static struct attribute *zram_disk_attrs[] = {
 	&dev_attr_disksize.attr,
 	&dev_attr_initstate.attr,
 	&dev_attr_reset.attr,
+	&dev_attr_compressor.attr,
 	&dev_attr_num_reads.attr,
 	&dev_attr_num_writes.attr,
 	&dev_attr_failed_reads.attr,
-- 
1.8.5.3.493.gb139ac2

Re: [RFC PATCH 1/3] zram: delete zram_init_device() function

[Minchan Kim]

Hello Sergey,

Looks good to me and I found a bug which had been in there so I rebased
this patch on the top.
https://git.kernel.org/cgit/linux/kernel/git/minchan/linux.git/commit/?h=zram-next&id=241e34fc6c3c1a41575fbe6383436be70df300d1

On Fri, Jan 17, 2014 at 02:04:15PM +0300, Sergey Senozhatsky wrote:
> allocate new `zram_meta' in disksize_store() only for uninitialised
> zram device, saving a number of allocations and deallocations in case
> if disksize_store() was called on currently used device. at the same
> time zram_meta stack variable is not necessary, because we can set
> ->meta directly. there is also no need in setting QUEUE_FLAG_NONROT
> queue on every disksize_store(), set it once during device creation.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Otherwise,

Acked-by: Minchan Kim <minchan@kernel.org>

-- 
Kind regards,
Minchan Kim

Re: [RFC PATCH 2/3] zram: introduce zram compressor operations struct

[Minchan Kim]

Hello Sergey,

I reviewed this patchset and I suggest somethings.
Please have a look and feedback to me. :)

1. Let's define new file zram_comp.c
2. zram_comp includes following field
   .create
   .compress
   .decompress.
   .destroy
   .name

1) create/destroy
Will set up necessary things like allocating buffer, lock init or other things
might happen in future when initialization time.
I have a plan to support multiple buffer to do compression/decompression in
parallel so we could optimize write VS write path, too.

2) compress/decompress

It's very clear.

3) name field will be used for tell to user what's kinds of compression
algorithm zram support.

On Fri, Jan 17, 2014 at 02:04:16PM +0300, Sergey Senozhatsky wrote:
> This is preparation patch to add LZ4 compression support.
> 
> struct zram_compress_ops defines common compress and decompress
> prototypes. Use these ops->compress and ops->decompress callbacks
> instead of direct LZO lzo1x_1_compress() and lzo1x_decompress_safe()
> calls.
> 
> Compressor ops should be defined before zram meta allocation,
> because the size of meta->compress_workmem depends on selected
> compression algorithm.
> 
> Define LZO compressor ops and set it as the only one available
> zram compressor at the moment.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/zram_drv.c | 20 +++++++++++++-------
>  drivers/block/zram/zram_drv.h | 10 ++++++++++
>  2 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 7124042..4f2c748 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -28,10 +28,9 @@
>  #include <linux/device.h>
>  #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 <linux/lzo.h>

Let's include zram_comps.h only and zram_comps.h includes other compression
alrogithms header. If user don't want to support some compression
alrogithm, it shouldn't be included.
Of course, user can select kinds of compressor in configuration step.

>  
>  #include "zram_drv.h"
>  
> @@ -42,6 +41,12 @@ static struct zram *zram_devices;
>  /* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
>  
> +static struct zram_compress_ops lzo_compressor = {
> +	.workmem_sz = LZO1X_MEM_COMPRESS,

workmem_sz should be part of compressor internal.

> +	.compress = lzo1x_1_compress,
> +	.decompress = lzo1x_decompress_safe,
> +};
> +
>  #define ZRAM_ATTR_RO(name)						\
>  static ssize_t zram_attr_##name##_show(struct device *d,		\
>  				struct device_attribute *attr, char *b)	\
> @@ -166,14 +171,14 @@ static void zram_meta_free(struct zram_meta *meta)
>  	kfree(meta);
>  }
>  
> -static struct zram_meta *zram_meta_alloc(u64 disksize)
> +static struct zram_meta *zram_meta_alloc(int workmem_sz, u64 disksize)
>  {
>  	size_t num_pages;
>  	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
>  	if (!meta)
>  		goto out;
>  
> -	meta->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> +	meta->compress_workmem = kzalloc(workmem_sz, GFP_KERNEL);

Instead of exposing compression internal stuff, let's call comp->create.

>  	if (!meta->compress_workmem)
>  		goto free_meta;
>  
> @@ -301,7 +306,7 @@ 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 = zram->ops->decompress(cmem, size, mem, &clen);
>  	zs_unmap_object(meta->mem_pool, handle);
>  	read_unlock(&meta->tb_lock);
>  
> @@ -420,7 +425,7 @@ 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,
> +	ret = zram->ops->compress(uncmem, PAGE_SIZE, src, &clen,
>  			       meta->compress_workmem);

Ditto. compress_workmem should be part of compressor itself.


>  	if (!is_partial_io(bvec)) {
>  		kunmap_atomic(user_mem);
> @@ -551,7 +556,7 @@ static ssize_t disksize_store(struct device *dev,
>  	}
>  
>  	disksize = PAGE_ALIGN(disksize);
> -	zram->meta = zram_meta_alloc(disksize);
> +	zram->meta = zram_meta_alloc(zram->ops->workmem_sz, disksize);

So, we don't need zram->ops->workmem_sz.

>  
>  	zram->disksize = disksize;
>  	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
> @@ -785,6 +790,7 @@ static int create_device(struct zram *zram, int device_id)
>  		goto out_free_disk;
>  	}
>  
> +	zram->ops = &lzo_compressor;

Let's define choose_compressor function with some argument
so the result is following as

zram->comp = choose_compressor("lzo");

>  	zram->meta = NULL;
>  	return 0;
>  
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 1d5b1f5..5488682 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -88,8 +88,18 @@ struct zram_meta {
>  	struct mutex buffer_lock; /* protect compress buffers */
>  };
>  
> +/* compression/decompression functions and algorithm-specific workmem size */
> +struct zram_compress_ops {
> +	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);
> +	long workmem_sz;
> +};
> +
>  struct zram {
>  	struct zram_meta *meta;
> +	struct zram_compress_ops *ops;
>  	struct request_queue *queue;
>  	struct gendisk *disk;
>  	/* Prevent concurrent execution of device init, reset and R/W request */
> -- 
> 1.8.5.3.493.gb139ac2
> 
> --
> 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: [RFC PATCH 3/3] zram: list and select compression algorithms

[Minchan Kim]

On Fri, Jan 17, 2014 at 02:04:17PM +0300, Sergey Senozhatsky wrote:
> Add compressor device attr that allows to list and select
> compression algorithms.
> 
> Define and make available for selection LZ4 compressor ops.
> 
> usage example:
> List available compression algorithms (currently selected
> one is LZO):
>   cat /sys/block/zram0/compressor
>   <lzo> lz4
> 
> Change compression algorithm to LZ4:
>   echo lz4 > /sys/block/zram0/compressor
>   cat /sys/block/zram0/compressor
>   lzo <lz4>

Interface looks good to me.

> 
> Update documentation with "Select compression algorithm" section
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  Documentation/blockdev/zram.txt | 23 ++++++++++++++++-----
>  drivers/block/zram/Kconfig      |  2 ++
>  drivers/block/zram/zram_drv.c   | 46 +++++++++++++++++++++++++++++++++++++----
>  3 files changed, 62 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> index 393541be..af90d29 100644
> --- a/Documentation/blockdev/zram.txt
> +++ b/Documentation/blockdev/zram.txt
> @@ -21,7 +21,20 @@ 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) Select compression algorithm
> +'compressor' sysfs node allows to see available, currently used
> +and change compression algorithms.
> +In order to list available compression algorithms (currently selected
> +one is LZO):
> +	cat /sys/block/zram0/compressor
> +	<lzo> lz4
> +
> +Change compression algorithm to LZ4:
> +	echo lz4 > /sys/block/zram0/compressor
> +	cat /sys/block/zram0/compressor
> +	lzo <lz4>
> +
> +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 +51,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
> @@ -59,11 +72,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
> diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> index 3450be8..09dacd6 100644
> --- a/drivers/block/zram/Kconfig
> +++ b/drivers/block/zram/Kconfig
> @@ -3,6 +3,8 @@ config ZRAM
>  	depends on BLOCK && SYSFS && ZSMALLOC
>  	select LZO_COMPRESS
>  	select LZO_DECOMPRESS
> +	select LZ4_COMPRESS
> +	select LZ4_DECOMPRESS

But user should select what kinds of compressor then want
some user want only lzo while others want lzo, lz2 and zlib all.


>  	default n
>  	help
>  	  Creates virtual block devices called /dev/zramX (X = 0, 1, ...).
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 4f2c748..f1434f8 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -31,6 +31,7 @@
>  #include <linux/string.h>
>  #include <linux/vmalloc.h>
>  #include <linux/lzo.h>
> +#include <linux/lz4.h>
>  
>  #include "zram_drv.h"
>  
> @@ -47,6 +48,12 @@ static struct zram_compress_ops lzo_compressor = {
>  	.decompress = lzo1x_decompress_safe,
>  };
>  
> +static struct zram_compress_ops lz4_compressor = {
> +	.workmem_sz = LZ4_MEM_COMPRESS,
> +	.compress = lz4_compress,
> +	.decompress = lz4_decompress_unknownoutputsize,
> +};
> +
>  #define ZRAM_ATTR_RO(name)						\
>  static ssize_t zram_attr_##name##_show(struct device *d,		\
>  				struct device_attribute *attr, char *b)	\
> @@ -113,6 +120,34 @@ static ssize_t mem_used_total_show(struct device *dev,
>  	return sprintf(buf, "%llu\n", val);
>  }
>  
> +static ssize_t compressor_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct zram *zram = dev_to_zram(dev);
> +	if (zram->ops == &lzo_compressor)
> +		return sprintf(buf, "<lzo> lz4\n");
> +	return sprintf(buf, "lzo <lz4>\n");
> +}
> +
> +static ssize_t compressor_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t len)
> +{
> +	struct zram *zram = dev_to_zram(dev);
> +
> +	down_write(&zram->init_lock);
> +	if (init_done(zram)) {
> +		up_write(&zram->init_lock);
> +		pr_info("Cannot change compressor for initialized device\n");
> +		return -EBUSY;
> +	}
> +	if (strncmp(buf, "lzo", 3) == 0)
> +		zram->ops = &lzo_compressor;
> +	else
> +		zram->ops = &lz4_compressor;
> +	up_write(&zram->init_lock);
> +	return len;
> +}

Changing compressow should be allowed only when zram was created or
reset. IOW, there should be no datacompressed by old compressor in memory.

> +
>  /* flag operations needs meta->tb_lock */
>  static int zram_test_flag(struct zram_meta *meta, u32 index,
>  			enum zram_pageflags flag)
> @@ -285,7 +320,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;
> @@ -311,7 +346,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>  	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;
> @@ -354,7 +389,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))
> @@ -433,7 +468,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		uncmem = NULL;
>  	}
>  
> -	if (unlikely(ret != LZO_E_OK)) {
> +	if (unlikely(ret)) {
>  		pr_err("Compression failed! err=%d\n", ret);
>  		goto out;
>  	}
> @@ -705,6 +740,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(compressor, S_IRUGO | S_IWUSR,
> +		compressor_show, compressor_store);
>  
>  ZRAM_ATTR_RO(num_reads);
>  ZRAM_ATTR_RO(num_writes);
> @@ -719,6 +756,7 @@ static struct attribute *zram_disk_attrs[] = {
>  	&dev_attr_disksize.attr,
>  	&dev_attr_initstate.attr,
>  	&dev_attr_reset.attr,
> +	&dev_attr_compressor.attr,
>  	&dev_attr_num_reads.attr,
>  	&dev_attr_num_writes.attr,
>  	&dev_attr_failed_reads.attr,
> -- 
> 1.8.5.3.493.gb139ac2
> 
> --
> 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: [RFC PATCH 2/3] zram: introduce zram compressor operations struct

[Sergey Senozhatsky]

On (01/20/14 14:12), Minchan Kim wrote:
> Date: Mon, 20 Jan 2014 14:12:33 +0900
> From: Minchan Kim <minchan@kernel.org>
> To: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Jerome Marchand <jmarchan@redhat.com>, Nitin Gupta <ngupta@vflare.org>,
>  linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH 2/3] zram: introduce zram compressor operations
>  struct
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> Hello Sergey,
> 

Hello Minchan,

> I reviewed this patchset and I suggest somethings.
> Please have a look and feedback to me. :)
> 
> 1. Let's define new file zram_comp.c
> 2. zram_comp includes following field
>    .create
>    .compress
>    .decompress.
>    .destroy
>    .name
> 
> 1) create/destroy
> Will set up necessary things like allocating buffer, lock init or other things
> might happen in future when initialization time.
> I have a plan to support multiple buffer to do compression/decompression in
> parallel so we could optimize write VS write path, too.
> 

I have a patch for this

> 2) compress/decompress
> 
> It's very clear.
> 
> 3) name field will be used for tell to user what's kinds of compression
> algorithm zram support.
> 

looks good. will start addressing


	-ss

> On Fri, Jan 17, 2014 at 02:04:16PM +0300, Sergey Senozhatsky wrote:
> > This is preparation patch to add LZ4 compression support.
> > 
> > struct zram_compress_ops defines common compress and decompress
> > prototypes. Use these ops->compress and ops->decompress callbacks
> > instead of direct LZO lzo1x_1_compress() and lzo1x_decompress_safe()
> > calls.
> > 
> > Compressor ops should be defined before zram meta allocation,
> > because the size of meta->compress_workmem depends on selected
> > compression algorithm.
> > 
> > Define LZO compressor ops and set it as the only one available
> > zram compressor at the moment.
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  drivers/block/zram/zram_drv.c | 20 +++++++++++++-------
> >  drivers/block/zram/zram_drv.h | 10 ++++++++++
> >  2 files changed, 23 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 7124042..4f2c748 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -28,10 +28,9 @@
> >  #include <linux/device.h>
> >  #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 <linux/lzo.h>
> 
> Let's include zram_comps.h only and zram_comps.h includes other compression
> alrogithms header. If user don't want to support some compression
> alrogithm, it shouldn't be included.
> Of course, user can select kinds of compressor in configuration step.
> 
> >  
> >  #include "zram_drv.h"
> >  
> > @@ -42,6 +41,12 @@ static struct zram *zram_devices;
> >  /* Module params (documentation at end) */
> >  static unsigned int num_devices = 1;
> >  
> > +static struct zram_compress_ops lzo_compressor = {
> > +	.workmem_sz = LZO1X_MEM_COMPRESS,
> 
> workmem_sz should be part of compressor internal.
> 
> > +	.compress = lzo1x_1_compress,
> > +	.decompress = lzo1x_decompress_safe,
> > +};
> > +
> >  #define ZRAM_ATTR_RO(name)						\
> >  static ssize_t zram_attr_##name##_show(struct device *d,		\
> >  				struct device_attribute *attr, char *b)	\
> > @@ -166,14 +171,14 @@ static void zram_meta_free(struct zram_meta *meta)
> >  	kfree(meta);
> >  }
> >  
> > -static struct zram_meta *zram_meta_alloc(u64 disksize)
> > +static struct zram_meta *zram_meta_alloc(int workmem_sz, u64 disksize)
> >  {
> >  	size_t num_pages;
> >  	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
> >  	if (!meta)
> >  		goto out;
> >  
> > -	meta->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> > +	meta->compress_workmem = kzalloc(workmem_sz, GFP_KERNEL);
> 
> Instead of exposing compression internal stuff, let's call comp->create.
> 
> >  	if (!meta->compress_workmem)
> >  		goto free_meta;
> >  
> > @@ -301,7 +306,7 @@ 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 = zram->ops->decompress(cmem, size, mem, &clen);
> >  	zs_unmap_object(meta->mem_pool, handle);
> >  	read_unlock(&meta->tb_lock);
> >  
> > @@ -420,7 +425,7 @@ 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,
> > +	ret = zram->ops->compress(uncmem, PAGE_SIZE, src, &clen,
> >  			       meta->compress_workmem);
> 
> Ditto. compress_workmem should be part of compressor itself.
> 
> 
> >  	if (!is_partial_io(bvec)) {
> >  		kunmap_atomic(user_mem);
> > @@ -551,7 +556,7 @@ static ssize_t disksize_store(struct device *dev,
> >  	}
> >  
> >  	disksize = PAGE_ALIGN(disksize);
> > -	zram->meta = zram_meta_alloc(disksize);
> > +	zram->meta = zram_meta_alloc(zram->ops->workmem_sz, disksize);
> 
> So, we don't need zram->ops->workmem_sz.
> 
> >  
> >  	zram->disksize = disksize;
> >  	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
> > @@ -785,6 +790,7 @@ static int create_device(struct zram *zram, int device_id)
> >  		goto out_free_disk;
> >  	}
> >  
> > +	zram->ops = &lzo_compressor;
> 
> Let's define choose_compressor function with some argument
> so the result is following as
> 
> zram->comp = choose_compressor("lzo");
> 
> >  	zram->meta = NULL;
> >  	return 0;
> >  
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index 1d5b1f5..5488682 100644
> > --- a/drivers/block/zram/zram_drv.h
> > +++ b/drivers/block/zram/zram_drv.h
> > @@ -88,8 +88,18 @@ struct zram_meta {
> >  	struct mutex buffer_lock; /* protect compress buffers */
> >  };
> >  
> > +/* compression/decompression functions and algorithm-specific workmem size */
> > +struct zram_compress_ops {
> > +	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);
> > +	long workmem_sz;
> > +};
> > +
> >  struct zram {
> >  	struct zram_meta *meta;
> > +	struct zram_compress_ops *ops;
> >  	struct request_queue *queue;
> >  	struct gendisk *disk;
> >  	/* Prevent concurrent execution of device init, reset and R/W request */
> > -- 
> > 1.8.5.3.493.gb139ac2
> > 
> > --
> > 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: [RFC PATCH 2/3] zram: introduce zram compressor operations struct

[Sergey Senozhatsky]

On (01/20/14 14:12), Minchan Kim wrote:
> Hello Sergey,
> 
> I reviewed this patchset and I suggest somethings.
> Please have a look and feedback to me. :)
> 
> 1. Let's define new file zram_comp.c
> 2. zram_comp includes following field
>    .create
>    .compress
>    .decompress.
>    .destroy
>    .name
> 

alternatively, we can use crypto api, the same way as zswap does (that
will require handling of cpu hotplug).

	-ss

> 1) create/destroy
> Will set up necessary things like allocating buffer, lock init or other things
> might happen in future when initialization time.
> I have a plan to support multiple buffer to do compression/decompression in
> parallel so we could optimize write VS write path, too.
> 
> 2) compress/decompress
> 
> It's very clear.
> 
> 3) name field will be used for tell to user what's kinds of compression
> algorithm zram support.
> 
> On Fri, Jan 17, 2014 at 02:04:16PM +0300, Sergey Senozhatsky wrote:
> > This is preparation patch to add LZ4 compression support.
> > 
> > struct zram_compress_ops defines common compress and decompress
> > prototypes. Use these ops->compress and ops->decompress callbacks
> > instead of direct LZO lzo1x_1_compress() and lzo1x_decompress_safe()
> > calls.
> > 
> > Compressor ops should be defined before zram meta allocation,
> > because the size of meta->compress_workmem depends on selected
> > compression algorithm.
> > 
> > Define LZO compressor ops and set it as the only one available
> > zram compressor at the moment.
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  drivers/block/zram/zram_drv.c | 20 +++++++++++++-------
> >  drivers/block/zram/zram_drv.h | 10 ++++++++++
> >  2 files changed, 23 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 7124042..4f2c748 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -28,10 +28,9 @@
> >  #include <linux/device.h>
> >  #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 <linux/lzo.h>
> 
> Let's include zram_comps.h only and zram_comps.h includes other compression
> alrogithms header. If user don't want to support some compression
> alrogithm, it shouldn't be included.
> Of course, user can select kinds of compressor in configuration step.
> 
> >  
> >  #include "zram_drv.h"
> >  
> > @@ -42,6 +41,12 @@ static struct zram *zram_devices;
> >  /* Module params (documentation at end) */
> >  static unsigned int num_devices = 1;
> >  
> > +static struct zram_compress_ops lzo_compressor = {
> > +	.workmem_sz = LZO1X_MEM_COMPRESS,
> 
> workmem_sz should be part of compressor internal.
> 
> > +	.compress = lzo1x_1_compress,
> > +	.decompress = lzo1x_decompress_safe,
> > +};
> > +
> >  #define ZRAM_ATTR_RO(name)						\
> >  static ssize_t zram_attr_##name##_show(struct device *d,		\
> >  				struct device_attribute *attr, char *b)	\
> > @@ -166,14 +171,14 @@ static void zram_meta_free(struct zram_meta *meta)
> >  	kfree(meta);
> >  }
> >  
> > -static struct zram_meta *zram_meta_alloc(u64 disksize)
> > +static struct zram_meta *zram_meta_alloc(int workmem_sz, u64 disksize)
> >  {
> >  	size_t num_pages;
> >  	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
> >  	if (!meta)
> >  		goto out;
> >  
> > -	meta->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> > +	meta->compress_workmem = kzalloc(workmem_sz, GFP_KERNEL);
> 
> Instead of exposing compression internal stuff, let's call comp->create.
> 
> >  	if (!meta->compress_workmem)
> >  		goto free_meta;
> >  
> > @@ -301,7 +306,7 @@ 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 = zram->ops->decompress(cmem, size, mem, &clen);
> >  	zs_unmap_object(meta->mem_pool, handle);
> >  	read_unlock(&meta->tb_lock);
> >  
> > @@ -420,7 +425,7 @@ 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,
> > +	ret = zram->ops->compress(uncmem, PAGE_SIZE, src, &clen,
> >  			       meta->compress_workmem);
> 
> Ditto. compress_workmem should be part of compressor itself.
> 
> 
> >  	if (!is_partial_io(bvec)) {
> >  		kunmap_atomic(user_mem);
> > @@ -551,7 +556,7 @@ static ssize_t disksize_store(struct device *dev,
> >  	}
> >  
> >  	disksize = PAGE_ALIGN(disksize);
> > -	zram->meta = zram_meta_alloc(disksize);
> > +	zram->meta = zram_meta_alloc(zram->ops->workmem_sz, disksize);
> 
> So, we don't need zram->ops->workmem_sz.
> 
> >  
> >  	zram->disksize = disksize;
> >  	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
> > @@ -785,6 +790,7 @@ static int create_device(struct zram *zram, int device_id)
> >  		goto out_free_disk;
> >  	}
> >  
> > +	zram->ops = &lzo_compressor;
> 
> Let's define choose_compressor function with some argument
> so the result is following as
> 
> zram->comp = choose_compressor("lzo");
> 
> >  	zram->meta = NULL;
> >  	return 0;
> >  
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index 1d5b1f5..5488682 100644
> > --- a/drivers/block/zram/zram_drv.h
> > +++ b/drivers/block/zram/zram_drv.h
> > @@ -88,8 +88,18 @@ struct zram_meta {
> >  	struct mutex buffer_lock; /* protect compress buffers */
> >  };
> >  
> > +/* compression/decompression functions and algorithm-specific workmem size */
> > +struct zram_compress_ops {
> > +	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);
> > +	long workmem_sz;
> > +};
> > +
> >  struct zram {
> >  	struct zram_meta *meta;
> > +	struct zram_compress_ops *ops;
> >  	struct request_queue *queue;
> >  	struct gendisk *disk;
> >  	/* Prevent concurrent execution of device init, reset and R/W request */
> > -- 
> > 1.8.5.3.493.gb139ac2
> > 
> > --
> > 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: [RFC PATCH 2/3] zram: introduce zram compressor operations struct

[Minchan Kim]

On Mon, Jan 20, 2014 at 01:03:48PM +0300, Sergey Senozhatsky wrote:
> On (01/20/14 14:12), Minchan Kim wrote:
> > Hello Sergey,
> > 
> > I reviewed this patchset and I suggest somethings.
> > Please have a look and feedback to me. :)
> > 
> > 1. Let's define new file zram_comp.c
> > 2. zram_comp includes following field
> >    .create
> >    .compress
> >    .decompress.
> >    .destroy
> >    .name
> > 
> 
> alternatively, we can use crypto api, the same way as zswap does (that
> will require handling of cpu hotplug).
> 
> 	-ss

I really doubt what's the benefit from crypto API for zram.
It's maybe since I'm not familiar with it so I should ask a silly
question.

1. What's the runtime overhead for using such frontend?

   As you know, zram is in-memory block device so I don't want to
   add unnecessary overhead to optimize.

2. What's the memory footprint for using such frontend?

   As you know, zram is very popular for small-memory embedded device
   so I don't want to consume more runtime memory and static memory
   due to CONFIG_CRYPTO friend.

3. Is it a flexible to alloc/handle multiple compressor buffer for
   the our purpose? zswap and zcache have been used it with per-cpu
   buffer but it would a problem for write scalabitliy if we uses zlib
   which takes long time to compress.
   When I read code, maybe we can allocate multiple buffers through
   cryptop_alloc_compo several time but it would cause 1) and 2) problem
   again.
   
So, what's the attractive point for using crypto?
One of thing I could imagine is that it could make zram H/W compressor
but I don't have heard about it so if we don't have any special reason,
I'd like to go with raw compressor so we can get a *base* number. Then,
if we really need crypto API, we can change it easily and benchmark.
Finally, we could get a comparision number in future and it would make
the decision easily.

Thanks.

-- 
Kind regards,
Minchan Kim

Re: [RFC PATCH 2/3] zram: introduce zram compressor operations struct

[Sergey Senozhatsky]

On (01/21/14 09:09), Minchan Kim wrote:
> On Mon, Jan 20, 2014 at 01:03:48PM +0300, Sergey Senozhatsky wrote:
> > On (01/20/14 14:12), Minchan Kim wrote:
> > > Hello Sergey,
> > > 
> > > I reviewed this patchset and I suggest somethings.
> > > Please have a look and feedback to me. :)
> > > 
> > > 1. Let's define new file zram_comp.c
> > > 2. zram_comp includes following field
> > >    .create
> > >    .compress
> > >    .decompress.
> > >    .destroy
> > >    .name
> > > 
> > 
> > alternatively, we can use crypto api, the same way as zswap does (that
> > will require handling of cpu hotplug).
> > 
> > 	-ss
> 
> I really doubt what's the benefit from crypto API for zram.
> It's maybe since I'm not familiar with it so I should ask a silly
> question.
> 
> 1. What's the runtime overhead for using such frontend?
> 
>    As you know, zram is in-memory block device so I don't want to
>    add unnecessary overhead to optimize.
> 
> 2. What's the memory footprint for using such frontend?
> 
>    As you know, zram is very popular for small-memory embedded device
>    so I don't want to consume more runtime memory and static memory
>    due to CONFIG_CRYPTO friend.
> 

schematically:

char *compressor = "lzo"; /* or supplied by user via COMPRESSOR_store() */

if (crypto_has_comp(compressor, 0, 0)) {
	tfms = alloc_percpu(struct crypto_comp *);
	tfm = crypto_alloc_comp(compressor, 0, 0);
	*per_cpu_ptr(tfms, cpu) = tfm;
}

	ret = crypto_comp_compress(tfm, src, slen, dst, dlen);
and
	ret = crypto_comp_decompress(tfm, src, slen, dst, dlen);


memory overhead looks like a bunch of structures (transformation
contexts -- tfm). they touch tfm within compress/decompress, so crypto
users usually allocate per cpu tfm or somehow protect parallel tfm
usage, which is a big disadvantage to my opinion. otoh, crypto provides
HC compressors (LZ4HC at the moment).

my choice is to use raw compressor (as I did in initial patchset).

	-ss

> 3. Is it a flexible to alloc/handle multiple compressor buffer for
>    the our purpose? zswap and zcache have been used it with per-cpu
>    buffer but it would a problem for write scalabitliy if we uses zlib
>    which takes long time to compress.
>    When I read code, maybe we can allocate multiple buffers through
>    cryptop_alloc_compo several time but it would cause 1) and 2) problem
>    again.
>    
> So, what's the attractive point for using crypto?
> One of thing I could imagine is that it could make zram H/W compressor
> but I don't have heard about it so if we don't have any special reason,
> I'd like to go with raw compressor so we can get a *base* number. Then,
> if we really need crypto API, we can change it easily and benchmark.
> Finally, we could get a comparision number in future and it would make
> the decision easily.
> 
> Thanks.
> 
> -- 
> Kind regards,
> Minchan Kim
>