Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

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

35 views
Skip to first unread message

Sergey Senozhatsky

unread,
Jan 17, 2014, 6:10:02 AM1/17/14
to
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.se...@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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Sergey Senozhatsky

unread,
Jan 17, 2014, 6:10:02 AM1/17/14
to
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.se...@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.

Sergey Senozhatsky

unread,
Jan 17, 2014, 6:10:03 AM1/17/14
to
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(-)

Sergey Senozhatsky

unread,
Jan 17, 2014, 6:10:03 AM1/17/14
to
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.se...@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 */

Minchan Kim

unread,
Jan 19, 2014, 11:50:01 PM1/19/14
to
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.se...@gmail.com>

Otherwise,

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

--
Kind regards,
Minchan Kim

Minchan Kim

unread,
Jan 20, 2014, 12:20:01 AM1/20/14
to
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.
But user should select what kinds of compressor then want
some user want only lzo while others want lzo, lz2 and zlib all.
Changing compressow should be allowed only when zram was created or
reset. IOW, there should be no datacompressed by old compressor in memory.
--
Kind regards,
Minchan Kim

Minchan Kim

unread,
Jan 20, 2014, 12:20:02 AM1/20/14
to
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.
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");
--
Kind regards,
Minchan Kim

Sergey Senozhatsky

unread,
Jan 20, 2014, 3:50:02 AM1/20/14
to
On (01/20/14 14:12), Minchan Kim wrote:
> Date: Mon, 20 Jan 2014 14:12:33 +0900
> From: Minchan Kim <min...@kernel.org>
> To: Sergey Senozhatsky <sergey.se...@gmail.com>
> Cc: Jerome Marchand <jmar...@redhat.com>, Nitin Gupta <ngu...@vflare.org>,
> linux-...@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

Sergey Senozhatsky

unread,
Jan 20, 2014, 5:10:03 AM1/20/14
to
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

Minchan Kim

unread,
Jan 20, 2014, 7:10:03 PM1/20/14
to
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.

Sergey Senozhatsky

unread,
Jan 21, 2014, 4:00:02 AM1/21/14
to
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
0 new messages