| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140120051233.GC8155@bbox>
Date: Mon, 20 Jan 2014 14:12:33 +0900
From: Minchan Kim <minchan@...nel.org>
To: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc: Jerome Marchand <jmarchan@...hat.com>,
Nitin Gupta <ngupta@...are.org>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 2/3] zram: introduce zram compressor operations struct
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@...il.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@...r.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists