Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1406906 > unrolled thread

[PATCH 0/7] zram: switch to crypto api

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] zram: switch to crypto api Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-05-25 15:40 +0200
    [PATCH 6/7] zram: delete custom lzo/lz4 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-05-25 15:40 +0200
    [PATCH 5/7] zram: use crypto api to check alg availability Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-05-25 15:40 +0200
      Re: [PATCH 5/7] zram: use crypto api to check alg availability Minchan Kim <minchan@kernel.org> - 2016-05-27 07:00 +0200
        Re: [PATCH 5/7] zram: use crypto api to check alg availability Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-05-27 10:00 +0200
          Re: [PATCH 5/7] zram: use crypto api to check alg availability Minchan Kim <minchan@kernel.org> - 2016-05-27 10:30 +0200
            Re: [PATCH 5/7] zram: use crypto api to check alg availability Herbert Xu <herbert@gondor.apana.org.au> - 2016-05-27 10:50 +0200
              Re: [PATCH 5/7] zram: use crypto api to check alg availability Minchan Kim <minchan@kernel.org> - 2016-05-27 11:10 +0200
                Re: [PATCH 5/7] zram: use crypto api to check alg availability Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-05-29 04:30 +0200
                  Re: [PATCH 5/7] zram: use crypto api to check alg availability Minchan Kim <minchan@kernel.org> - 2016-05-30 06:50 +0200
                    Re: [PATCH 5/7] zram: use crypto api to check alg availability Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-05-30 07:00 +0200
    [PATCH 1/7] zram: rename zstrm find-release functions Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-05-25 15:40 +0200
      Re: [PATCH 1/7] zram: rename zstrm find-release functions Minchan Kim <minchan@kernel.org> - 2016-05-26 02:50 +0200
        Re: [PATCH 1/7] zram: rename zstrm find-release functions Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-05-26 03:10 +0200
    Re: [PATCH 0/7] zram: switch to crypto api Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-26 02:50 +0200
      Re: [PATCH 0/7] zram: switch to crypto api Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-05-26 03:20 +0200
        Re: [PATCH 0/7] zram: switch to crypto api Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-26 04:00 +0200
    Re: [PATCH 0/7] zram: switch to crypto api Minchan Kim <minchan@kernel.org> - 2016-05-26 03:00 +0200
      Re: [PATCH 0/7] zram: switch to crypto api Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-05-26 03:10 +0200

#1406906 — [PATCH 0/7] zram: switch to crypto api

Hello,

  This has started as a 'add zlib support' work, but after some
thinking I saw no blockers for a bigger change -- a switch to
crypto API.

  We don't have an idle zstreams list anymore and our write path
now works absolutely differently, preventing preemption during
compression. This removes possibilities of read paths preempting
writes at wrong places and opens the door for a move from custom
LZO/LZ4 compression backends implementation to a more generic one,
using crypto compress API.

  This patch set also eliminates the need of a new context-less
crypto API interface, which was quite hard to sell, so we can
move along faster.


Sergey Senozhatsky (7):
  zram: rename zstrm find-release functions
  zram: switch to crypto compress API
  zram: drop zcomp param from compress/decompress
  zram: align zcomp interface to crypto comp API
  zram: use crypto api to check alg availability
  zram: delete custom lzo/lz4
  zram: add more compression algorithms

 drivers/block/zram/Kconfig     | 15 +------
 drivers/block/zram/Makefile    |  4 +-
 drivers/block/zram/zcomp.c     | 91 +++++++++++++++++++++++++++---------------
 drivers/block/zram/zcomp.h     | 29 ++++----------
 drivers/block/zram/zcomp_lz4.c | 56 --------------------------
 drivers/block/zram/zcomp_lz4.h | 17 --------
 drivers/block/zram/zcomp_lzo.c | 56 --------------------------
 drivers/block/zram/zcomp_lzo.h | 17 --------
 drivers/block/zram/zram_drv.c  | 26 +++++++-----
 9 files changed, 84 insertions(+), 227 deletions(-)
 delete mode 100644 drivers/block/zram/zcomp_lz4.c
 delete mode 100644 drivers/block/zram/zcomp_lz4.h
 delete mode 100644 drivers/block/zram/zcomp_lzo.c
 delete mode 100644 drivers/block/zram/zcomp_lzo.h

-- 
2.8.3.394.g3916adf

[toc] | [next] | [standalone]

#1406907 — [PATCH 6/7] zram: delete custom lzo/lz4

Remove lzo/lz4 backends, we use crypto API now.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 drivers/block/zram/Kconfig     |  9 -------
 drivers/block/zram/Makefile    |  4 +--
 drivers/block/zram/zcomp.c     |  2 --
 drivers/block/zram/zcomp.h     | 15 -----------
 drivers/block/zram/zcomp_lz4.c | 56 ------------------------------------------
 drivers/block/zram/zcomp_lz4.h | 17 -------------
 drivers/block/zram/zcomp_lzo.c | 56 ------------------------------------------
 drivers/block/zram/zcomp_lzo.h | 17 -------------
 8 files changed, 1 insertion(+), 175 deletions(-)
 delete mode 100644 drivers/block/zram/zcomp_lz4.c
 delete mode 100644 drivers/block/zram/zcomp_lz4.h
 delete mode 100644 drivers/block/zram/zcomp_lzo.c
 delete mode 100644 drivers/block/zram/zcomp_lzo.h

diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
index 2252cd7..b8ecba6 100644
--- a/drivers/block/zram/Kconfig
+++ b/drivers/block/zram/Kconfig
@@ -13,12 +13,3 @@ config ZRAM
 	  disks and maybe many more.
 
 	  See zram.txt for more information.
-
-config ZRAM_LZ4_COMPRESS
-	bool "Enable LZ4 algorithm support"
-	depends on ZRAM
-	select CRYPTO_LZ4
-	default n
-	help
-	  This option enables LZ4 compression algorithm support. Compression
-	  algorithm can be changed using `comp_algorithm' device attribute.
diff --git a/drivers/block/zram/Makefile b/drivers/block/zram/Makefile
index be0763f..9e2b79e 100644
--- a/drivers/block/zram/Makefile
+++ b/drivers/block/zram/Makefile
@@ -1,5 +1,3 @@
-zram-y	:=	zcomp_lzo.o zcomp.o zram_drv.o
-
-zram-$(CONFIG_ZRAM_LZ4_COMPRESS) += zcomp_lz4.o
+zram-y	:=	zcomp.o zram_drv.o
 
 obj-$(CONFIG_ZRAM)	+=	zram.o
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index a8593e9..5ec0eb2 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -20,9 +20,7 @@
 
 static const char * const backends[] = {
 	"lzo",
-#ifdef CONFIG_ZRAM_LZ4_COMPRESS
 	"lz4",
-#endif
 	NULL
 };
 
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index dcc0951..3cb13f4 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -21,24 +21,9 @@ struct zcomp_strm {
 	void *private;
 };
 
-/* static compression backend */
-struct zcomp_backend {
-	int (*compress)(const unsigned char *src, unsigned char *dst,
-			size_t *dst_len, void *private);
-
-	int (*decompress)(const unsigned char *src, size_t src_len,
-			unsigned char *dst);
-
-	void *(*create)(gfp_t flags);
-	void (*destroy)(void *private);
-
-	const char *name;
-};
-
 /* dynamic per-device compression frontend */
 struct zcomp {
 	struct zcomp_strm * __percpu *stream;
-	struct zcomp_backend *backend;
 	struct notifier_block notifier;
 
 	const char *name;
diff --git a/drivers/block/zram/zcomp_lz4.c b/drivers/block/zram/zcomp_lz4.c
deleted file mode 100644
index 0110086..0000000
--- a/drivers/block/zram/zcomp_lz4.c
+++ /dev/null
@@ -1,56 +0,0 @@
-/*
- * 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/lz4.h>
-#include <linux/vmalloc.h>
-#include <linux/mm.h>
-
-#include "zcomp_lz4.h"
-
-static void *zcomp_lz4_create(gfp_t flags)
-{
-	void *ret;
-
-	ret = kmalloc(LZ4_MEM_COMPRESS, flags);
-	if (!ret)
-		ret = __vmalloc(LZ4_MEM_COMPRESS,
-				flags | __GFP_HIGHMEM,
-				PAGE_KERNEL);
-	return ret;
-}
-
-static void zcomp_lz4_destroy(void *private)
-{
-	kvfree(private);
-}
-
-static int zcomp_lz4_compress(const unsigned char *src, unsigned char *dst,
-		size_t *dst_len, void *private)
-{
-	/* return  : Success if return 0 */
-	return lz4_compress(src, PAGE_SIZE, dst, dst_len, private);
-}
-
-static int zcomp_lz4_decompress(const unsigned char *src, size_t src_len,
-		unsigned char *dst)
-{
-	size_t dst_len = PAGE_SIZE;
-	/* return  : Success if return 0 */
-	return lz4_decompress_unknownoutputsize(src, src_len, dst, &dst_len);
-}
-
-struct zcomp_backend zcomp_lz4 = {
-	.compress = zcomp_lz4_compress,
-	.decompress = zcomp_lz4_decompress,
-	.create = zcomp_lz4_create,
-	.destroy = zcomp_lz4_destroy,
-	.name = "lz4",
-};
diff --git a/drivers/block/zram/zcomp_lz4.h b/drivers/block/zram/zcomp_lz4.h
deleted file mode 100644
index 60613fb..0000000
--- a/drivers/block/zram/zcomp_lz4.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/*
- * Copyright (C) 2014 Sergey Senozhatsky.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version
- * 2 of the License, or (at your option) any later version.
- */
-
-#ifndef _ZCOMP_LZ4_H_
-#define _ZCOMP_LZ4_H_
-
-#include "zcomp.h"
-
-extern struct zcomp_backend zcomp_lz4;
-
-#endif /* _ZCOMP_LZ4_H_ */
diff --git a/drivers/block/zram/zcomp_lzo.c b/drivers/block/zram/zcomp_lzo.c
deleted file mode 100644
index ed7a1f0..0000000
--- a/drivers/block/zram/zcomp_lzo.c
+++ /dev/null
@@ -1,56 +0,0 @@
-/*
- * 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 <linux/vmalloc.h>
-#include <linux/mm.h>
-
-#include "zcomp_lzo.h"
-
-static void *lzo_create(gfp_t flags)
-{
-	void *ret;
-
-	ret = kmalloc(LZO1X_MEM_COMPRESS, flags);
-	if (!ret)
-		ret = __vmalloc(LZO1X_MEM_COMPRESS,
-				flags | __GFP_HIGHMEM,
-				PAGE_KERNEL);
-	return ret;
-}
-
-static void lzo_destroy(void *private)
-{
-	kvfree(private);
-}
-
-static int lzo_compress(const unsigned char *src, unsigned char *dst,
-		size_t *dst_len, void *private)
-{
-	int ret = lzo1x_1_compress(src, PAGE_SIZE, dst, dst_len, private);
-	return ret == LZO_E_OK ? 0 : ret;
-}
-
-static int lzo_decompress(const unsigned char *src, size_t src_len,
-		unsigned char *dst)
-{
-	size_t dst_len = PAGE_SIZE;
-	int ret = lzo1x_decompress_safe(src, src_len, dst, &dst_len);
-	return ret == LZO_E_OK ? 0 : ret;
-}
-
-struct zcomp_backend zcomp_lzo = {
-	.compress = lzo_compress,
-	.decompress = lzo_decompress,
-	.create = lzo_create,
-	.destroy = lzo_destroy,
-	.name = "lzo",
-};
diff --git a/drivers/block/zram/zcomp_lzo.h b/drivers/block/zram/zcomp_lzo.h
deleted file mode 100644
index 128c580..0000000
--- a/drivers/block/zram/zcomp_lzo.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/*
- * Copyright (C) 2014 Sergey Senozhatsky.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version
- * 2 of the License, or (at your option) any later version.
- */
-
-#ifndef _ZCOMP_LZO_H_
-#define _ZCOMP_LZO_H_
-
-#include "zcomp.h"
-
-extern struct zcomp_backend zcomp_lzo;
-
-#endif /* _ZCOMP_LZO_H_ */
-- 
2.8.3.394.g3916adf

[toc] | [prev] | [next] | [standalone]

#1406908 — [PATCH 5/7] zram: use crypto api to check alg availability

There is no way to get a string with all the crypto comp
algorithms supported by the crypto comp engine, so we need
to maintain our own backends list. At the same time we
additionally need to use crypto_has_comp() to make sure
that the user has requested a compression algorithm that is
recognized by the crypto comp engine. Relying on /proc/crypto
is not an options here, because it does not show not-yet-inserted
compression modules.

Example:

 modprobe zram
 cat /proc/crypto | grep -i lz4
 modprobe lz4
 cat /proc/crypto | grep -i lz4
name         : lz4
driver       : lz4-generic
module       : lz4

So the user can't tell exactly if the lz4 is really supported
from /proc/crypto output, unless someone or something has loaded
it.

This patch also adds crypto_has_comp() to zcomp_available_show().
We store all the compression algorithms names in zcomp's `backends'
array, regardless the CONFIG_CRYPTO_FOO configuration, but show
only those that are also supported by crypto engine. This helps
user to know the exact list of compression algorithms that can be
used.

Example:
  module lz4 is not loaded yet, but is supported by the crypto
  engine. /proc/crypto has no information on this module, while
  zram's `comp_algorithm' lists it:

 cat /proc/crypto | grep -i lz4

 cat /sys/block/zram0/comp_algorithm
[lzo] lz4 deflate lz4hc 842

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 drivers/block/zram/zcomp.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 79b30d7..a8593e9 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -29,12 +29,16 @@ static const char * const backends[] = {
 static const char *find_backend(const char *compress)
 {
 	int i = 0;
+
 	while (backends[i]) {
 		if (sysfs_streq(compress, backends[i]))
 			break;
 		i++;
 	}
-	return backends[i];
+
+	if (backends[i] && crypto_has_comp(backends[i], 0, 0))
+		return backends[i];
+	return NULL;
 }
 
 static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm)
@@ -74,14 +78,16 @@ ssize_t zcomp_available_show(const char *comp, char *buf)
 	ssize_t sz = 0;
 	int i = 0;
 
-	while (backends[i]) {
+	for (; backends[i]; i++) {
+		if (!crypto_has_comp(backends[i], 0, 0))
+			continue;
+
 		if (!strcmp(comp, backends[i]))
 			sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
 					"[%s] ", backends[i]);
 		else
 			sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
 					"%s ", backends[i]);
-		i++;
 	}
 	sz += scnprintf(buf + sz, PAGE_SIZE - sz, "\n");
 	return sz;
-- 
2.8.3.394.g3916adf

[toc] | [prev] | [next] | [standalone]

#1407813 — Re: [PATCH 5/7] zram: use crypto api to check alg availability

Hello Sergey,

I want to know more how it works so below questions goes.

On Wed, May 25, 2016 at 11:30:04PM +0900, Sergey Senozhatsky wrote:
> There is no way to get a string with all the crypto comp
> algorithms supported by the crypto comp engine, so we need
> to maintain our own backends list. At the same time we
> additionally need to use crypto_has_comp() to make sure
> that the user has requested a compression algorithm that is
> recognized by the crypto comp engine. Relying on /proc/crypto
> is not an options here, because it does not show not-yet-inserted
> compression modules.
> 
> Example:
> 
>  modprobe zram
>  cat /proc/crypto | grep -i lz4
>  modprobe lz4
>  cat /proc/crypto | grep -i lz4
> name         : lz4
> driver       : lz4-generic
> module       : lz4
> 
> So the user can't tell exactly if the lz4 is really supported
> from /proc/crypto output, unless someone or something has loaded
> it.
> 
> This patch also adds crypto_has_comp() to zcomp_available_show().

crypto_has_comp works regardless of that whether module is loading or not?
IOW, currently, if lz4 modules is not loading, but crypto_has_comp return
true about lz4 module.
Right?

> We store all the compression algorithms names in zcomp's `backends'
> array, regardless the CONFIG_CRYPTO_FOO configuration, but show

Then, you mean we should add new string into backend array whenever
adding new crypto compatible compression algorithm?

> only those that are also supported by crypto engine. This helps
> user to know the exact list of compression algorithms that can be
> used.
> 
> Example:
>   module lz4 is not loaded yet, but is supported by the crypto
>   engine. /proc/crypto has no information on this module, while
>   zram's `comp_algorithm' lists it:
> 
>  cat /proc/crypto | grep -i lz4
> 
>  cat /sys/block/zram0/comp_algorithm
> [lzo] lz4 deflate lz4hc 842

So, when lzo module is loading?

> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
>  drivers/block/zram/zcomp.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 79b30d7..a8593e9 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -29,12 +29,16 @@ static const char * const backends[] = {
>  static const char *find_backend(const char *compress)
>  {
>  	int i = 0;
> +
>  	while (backends[i]) {
>  		if (sysfs_streq(compress, backends[i]))
>  			break;
>  		i++;
>  	}
> -	return backends[i];
> +
> +	if (backends[i] && crypto_has_comp(backends[i], 0, 0))
> +		return backends[i];
> +	return NULL;
>  }
>  
>  static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm)
> @@ -74,14 +78,16 @@ ssize_t zcomp_available_show(const char *comp, char *buf)
>  	ssize_t sz = 0;
>  	int i = 0;
>  
> -	while (backends[i]) {
> +	for (; backends[i]; i++) {
> +		if (!crypto_has_comp(backends[i], 0, 0))
> +			continue;
> +
>  		if (!strcmp(comp, backends[i]))
>  			sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
>  					"[%s] ", backends[i]);
>  		else
>  			sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
>  					"%s ", backends[i]);
> -		i++;
>  	}
>  	sz += scnprintf(buf + sz, PAGE_SIZE - sz, "\n");
>  	return sz;
> -- 
> 2.8.3.394.g3916adf
> 

[toc] | [prev] | [next] | [standalone]

#1407889 — Re: [PATCH 5/7] zram: use crypto api to check alg availability

On (05/27/16 13:43), Minchan Kim wrote:
[..]
> >  modprobe zram
> >  cat /proc/crypto | grep -i lz4
> >  modprobe lz4
> >  cat /proc/crypto | grep -i lz4
> > name         : lz4
> > driver       : lz4-generic
> > module       : lz4
> > 
> > So the user can't tell exactly if the lz4 is really supported
> > from /proc/crypto output, unless someone or something has loaded
> > it.
> > 
> > This patch also adds crypto_has_comp() to zcomp_available_show().
> 
> crypto_has_comp works regardless of that whether module is loading or not?
> IOW, currently, if lz4 modules is not loading, but crypto_has_comp return
> true about lz4 module.
> Right?

correct. crypto_has_comp() regardless the module being loaded.

# modprobe zram
# cat /proc/crypto | egrep -e "lzo|lz4|deflate"

# echo lzo > /sys/block/zram0/comp_algorithm 
# cat /proc/crypto | egrep -e "lzo|lz4|deflate"
name         : lzo
driver       : lzo-generic
module       : lzo

# echo lz4 > /sys/block/zram0/comp_algorithm 
# cat /proc/crypto | egrep -e "lzo|lz4|deflate"
name         : lz4
driver       : lz4-generic
module       : lz4
name         : lzo
driver       : lzo-generic
module       : lzo

# echo deflate > /sys/block/zram0/comp_algorithm 
# cat /proc/crypto | egrep -e "lzo|lz4|deflate"
name         : deflate
driver       : deflate-generic
module       : deflate
name         : lz4
driver       : lz4-generic
module       : lz4
name         : lzo
driver       : lzo-generic
module       : lzo


what is does, tho, it modprobs() the module upon the first
request:

crypto_has_comp(...)
	crypto_has_alg()
		crypto_alg_mod_lookup()
			crypto_larval_lookup()
				request_module("crypto-%s", name)
					__request_module()
						call_modprobe()

and this is when /proc/crypto is getting updated. otherwise user has
no information (well, unless modules were loaded by something/someome
else, or crypto compressors were built-in into the kernel). I'm not
aware of any other way to achieve this functionality for zram.


> > We store all the compression algorithms names in zcomp's `backends'
> > array, regardless the CONFIG_CRYPTO_FOO configuration, but show
> 
> Then, you mean we should add new string into backend array whenever
> adding new crypto compatible compression algorithm?

yes. which looks quite trivial: adding or removing a string to/from
the array.


> >  cat /proc/crypto | grep -i lz4
> > 
> >  cat /sys/block/zram0/comp_algorithm
> > [lzo] lz4 deflate lz4hc 842
> 
> So, when lzo module is loading?

when we execute crypto_has_comp("lzo") for the first time.


that's why doing just

# modprobe zram

will not cause /proc/crypto update


# modprobe zram
# cat /proc/crypto | egrep -e "lzo|lz4|deflate"


crypto_has_comp() updates it -- when we read or write
from/to comp_algorithm:

# cat /sys/block/zram0/comp_algorithm
[lzo] lz4 deflate lz4hc 842

# cat /proc/crypto | egrep -e "lzo|lz4|deflate"
name         : lzo
driver       : lzo-generic
module       : lzo


but I didn't want zram to depend on this, or to depend on
/proc/crypto content; that's why I did it the way it is.

	-ss

[toc] | [prev] | [next] | [standalone]

#1407916 — Re: [PATCH 5/7] zram: use crypto api to check alg availability

On Fri, May 27, 2016 at 04:50:52PM +0900, Sergey Senozhatsky wrote:
> On (05/27/16 13:43), Minchan Kim wrote:
> [..]
> > >  modprobe zram
> > >  cat /proc/crypto | grep -i lz4
> > >  modprobe lz4
> > >  cat /proc/crypto | grep -i lz4
> > > name         : lz4
> > > driver       : lz4-generic
> > > module       : lz4
> > > 
> > > So the user can't tell exactly if the lz4 is really supported
> > > from /proc/crypto output, unless someone or something has loaded
> > > it.
> > > 
> > > This patch also adds crypto_has_comp() to zcomp_available_show().
> > 
> > crypto_has_comp works regardless of that whether module is loading or not?
> > IOW, currently, if lz4 modules is not loading, but crypto_has_comp return
> > true about lz4 module.
> > Right?
> 
> correct. crypto_has_comp() regardless the module being loaded.
> 
> # modprobe zram
> # cat /proc/crypto | egrep -e "lzo|lz4|deflate"
> 
> # echo lzo > /sys/block/zram0/comp_algorithm 
> # cat /proc/crypto | egrep -e "lzo|lz4|deflate"
> name         : lzo
> driver       : lzo-generic
> module       : lzo
> 
> # echo lz4 > /sys/block/zram0/comp_algorithm 
> # cat /proc/crypto | egrep -e "lzo|lz4|deflate"
> name         : lz4
> driver       : lz4-generic
> module       : lz4
> name         : lzo
> driver       : lzo-generic
> module       : lzo
> 
> # echo deflate > /sys/block/zram0/comp_algorithm 
> # cat /proc/crypto | egrep -e "lzo|lz4|deflate"
> name         : deflate
> driver       : deflate-generic
> module       : deflate
> name         : lz4
> driver       : lz4-generic
> module       : lz4
> name         : lzo
> driver       : lzo-generic
> module       : lzo
> 
> 
> whab it does, tho, it modprobs() the module upon the first
> request:
> 
> crypto_has_comp(...)
> 	crypto_has_alg()
> 		crypto_alg_mod_lookup()
> 			crypto_larval_lookup()
> 				request_module("crypto-%s", name)
> 					__request_module()
> 						call_modprobe()
> 
> and this is when /proc/crypto is getting updated. otherwise user has
> no information (well, unless modules were loaded by something/someome
> else, or crypto compressors were built-in into the kernel). I'm not
> aware of any other way to achieve this functionality for zram.

Now I got it. Thanks for spending time for me.

> 
> 
> > > We store all the compression algorithms names in zcomp's `backends'
> > > array, regardless the CONFIG_CRYPTO_FOO configuration, but show
> > 
> > Then, you mean we should add new string into backend array whenever
> > adding new crypto compatible compression algorithm?
> 
> yes. which looks quite trivial: adding or removing a string to/from
> the array.

Cc'ing Herbert.

Yes, it might be trivial to adding new "string" into the backend array
if we consider frequency of adding new compressoin algorithm in linux
but it would be better if we can get names of supported compression
algorithm name by crypto API.

If it's not good idea or something hard to implement, let's go with
hardcoding.

Herbert, Could you give us thought?

> 
> 
> > >  cat /proc/crypto | grep -i lz4
> > > 
> > >  cat /sys/block/zram0/comp_algorithm
> > > [lzo] lz4 deflate lz4hc 842
> > 
> > So, when lzo module is loading?
> 
> when we execute crypto_has_comp("lzo") for the first time.
> 
> 
> that's why doing just
> 
> # modprobe zram
> 
> will not cause /proc/crypto update
> 
> 
> # modprobe zram
> # cat /proc/crypto | egrep -e "lzo|lz4|deflate"
> 
> 
> crypto_has_comp() updates it -- when we read or write
> from/to comp_algorithm:
> 
> # cat /sys/block/zram0/comp_algorithm
> [lzo] lz4 deflate lz4hc 842
> 
> # cat /proc/crypto | egrep -e "lzo|lz4|deflate"
> name         : lzo
> driver       : lzo-generic
> module       : lzo
> 
> 
> but I didn't want zram to depend on this, or to depend on
> /proc/crypto content; that's why I did it the way it is.
> 
> 	-ss

[toc] | [prev] | [next] | [standalone]

#1407927 — Re: [PATCH 5/7] zram: use crypto api to check alg availability

On Fri, May 27, 2016 at 05:27:44PM +0900, Minchan Kim wrote:
>
> Yes, it might be trivial to adding new "string" into the backend array
> if we consider frequency of adding new compressoin algorithm in linux
> but it would be better if we can get names of supported compression
> algorithm name by crypto API.
> 
> If it's not good idea or something hard to implement, let's go with
> hardcoding.
> 
> Herbert, Could you give us thought?

It is fundamentally impossible to get a list of all *potential*
algorithms if you allow loadable modules.  By definition someone
could load a new module and thus introduce a new algorithm.

Given a specific algorithm name you could determine whether it
is present on the system.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [next] | [standalone]

#1407943 — Re: [PATCH 5/7] zram: use crypto api to check alg availability

On Fri, May 27, 2016 at 04:43:45PM +0800, Herbert Xu wrote:
> On Fri, May 27, 2016 at 05:27:44PM +0900, Minchan Kim wrote:
> >
> > Yes, it might be trivial to adding new "string" into the backend array
> > if we consider frequency of adding new compressoin algorithm in linux
> > but it would be better if we can get names of supported compression
> > algorithm name by crypto API.
> > 
> > If it's not good idea or something hard to implement, let's go with
> > hardcoding.
> > 
> > Herbert, Could you give us thought?
> 
> It is fundamentally impossible to get a list of all *potential*
> algorithms if you allow loadable modules.  By definition someone
> could load a new module and thus introduce a new algorithm.

Thanks for the quick response. I understand now.

Sergey,

Then, shouldn't we add functionality to load new algorithm although
there is no entry in zcomp backend array?

The main reason we changed to crypto is to support various compression
algorithm for zram so we should be able to support anyone who want to
use custom crypto compression module.

A idea is that let's not adding new compression string into backend
array from now on and deprecates /zram/comp_algorithm to show name.
So, from now on, users should be aware of the compression naming and
load module by oneself. It's not kind but I hope admins already knows
what cryto compressor they have.

I don't have better idea.

> 
> Given a specific algorithm name you could determine whether it
> is present on the system.
> 
> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [next] | [standalone]

#1408538 — Re: [PATCH 5/7] zram: use crypto api to check alg availability

Hello,

On (05/27/16 18:04), Minchan Kim wrote:
> > It is fundamentally impossible to get a list of all *potential*
> > algorithms if you allow loadable modules.  By definition someone
> > could load a new module and thus introduce a new algorithm.
> 
> Thanks for the quick response. I understand now.
> 
> Sergey,
> 
> Then, shouldn't we add functionality to load new algorithm although
> there is no entry in zcomp backend array?

can do, sure: make find_backend() depend on crypto_has_comp() only,
*may be* printk a message when we have a mismatch in backends array
and crypto_has_comp(). but not really sure about the latter one.

> The main reason we changed to crypto is to support various compression
> algorithm for zram so we should be able to support anyone who want to
> use custom crypto compression module.

yes.

> A idea is that let's not adding new compression string into backend
> array from now on and deprecates /zram/comp_algorithm to show name.

I'd really prefer not to deprecate /zram/comp_algorithm now.

> So, from now on, users should be aware of the compression naming and
> load module by oneself. It's not kind but I hope admins already knows
> what cryto compressor they have.

um, I'm not in love with this approach.

what admins? zram users may be completely unaware of the existence of
the crypto API and the kernel rebuild process. for some people switching
to new zram will be a matter of "apt-get upgrade/pacman -Syu", not
"git am ...; make menuconfig; make -j...". this looks like a major step
back. keeping this dummy list of compressing backends around does not take
a lot of effort, especially if we will make it unimportant for find_backend()
and only use it for that nice user-friendly `cat /zram/comp_algorithm`
output.

	-ss

[toc] | [prev] | [next] | [standalone]

#1408752 — Re: [PATCH 5/7] zram: use crypto api to check alg availability

Hi Sergey,

On Sun, May 29, 2016 at 12:24:58PM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> On (05/27/16 18:04), Minchan Kim wrote:
> > > It is fundamentally impossible to get a list of all *potential*
> > > algorithms if you allow loadable modules.  By definition someone
> > > could load a new module and thus introduce a new algorithm.
> > 
> > Thanks for the quick response. I understand now.
> > 
> > Sergey,
> > 
> > Then, shouldn't we add functionality to load new algorithm although
> > there is no entry in zcomp backend array?
> 
> can do, sure: make find_backend() depend on crypto_has_comp() only,
> *may be* printk a message when we have a mismatch in backends array
> and crypto_has_comp(). but not really sure about the latter one.

Me, too. We don't need to warn about that. Instead, we should allow loading
other algorithm which doesn't live in backend array more clearly.
We need to update documentation to clear it out.

> 
> > The main reason we changed to crypto is to support various compression
> > algorithm for zram so we should be able to support anyone who want to
> > use custom crypto compression module.
> 
> yes.
> 
> > A idea is that let's not adding new compression string into backend
> > array from now on and deprecates /zram/comp_algorithm to show name.
> 
> I'd really prefer not to deprecate /zram/comp_algorithm now.
> 
> > So, from now on, users should be aware of the compression naming and
> > load module by oneself. It's not kind but I hope admins already knows
> > what cryto compressor they have.
> 
> um, I'm not in love with this approach.
> 
> what admins? zram users may be completely unaware of the existence of
> the crypto API and the kernel rebuild process. for some people switching
> to new zram will be a matter of "apt-get upgrade/pacman -Syu", not
> "git am ...; make menuconfig; make -j...". this looks like a major step
> back. keeping this dummy list of compressing backends around does not take
> a lot of effort, especially if we will make it unimportant for find_backend()
> and only use it for that nice user-friendly `cat /zram/comp_algorithm`
> output.

I am not against with it. Instead, please clear it out to write down docuement
"comp_algorithm is just optional so user can load any new algorithm like this
 blah blah"

Thanks.
> 
> 	-ss

[toc] | [prev] | [next] | [standalone]

#1408755 — Re: [PATCH 5/7] zram: use crypto api to check alg availability

Hello Minchan,

On (05/30/16 13:47), Minchan Kim wrote:
[..]
> > can do, sure: make find_backend() depend on crypto_has_comp() only,
> > *may be* printk a message when we have a mismatch in backends array
> > and crypto_has_comp(). but not really sure about the latter one.
> 
> Me, too. We don't need to warn about that. Instead, we should allow loading
> other algorithm which doesn't live in backend array more clearly.
> We need to update documentation to clear it out.

yes, entirely forgot to update the Documentation.

> > > So, from now on, users should be aware of the compression naming and
> > > load module by oneself. It's not kind but I hope admins already knows
> > > what cryto compressor they have.
> > 
> > um, I'm not in love with this approach.
> > 
> > what admins? zram users may be completely unaware of the existence of
> > the crypto API and the kernel rebuild process. for some people switching
> > to new zram will be a matter of "apt-get upgrade/pacman -Syu", not
> > "git am ...; make menuconfig; make -j...". this looks like a major step
> > back. keeping this dummy list of compressing backends around does not take
> > a lot of effort, especially if we will make it unimportant for find_backend()
> > and only use it for that nice user-friendly `cat /zram/comp_algorithm`
> > output.
> 
> I am not against with it. Instead, please clear it out to write down docuement
> "comp_algorithm is just optional so user can load any new algorithm like this
>  blah blah"

ok, will do.

thanks.

	-ss

[toc] | [prev] | [next] | [standalone]

#1406910 — [PATCH 1/7] zram: rename zstrm find-release functions

We don't perform any zstream idle list lookup anymore, so
zcomp_strm_find()/zcomp_strm_release() names are not
representative.

Rename to zcomp_stream_get()/zcomp_stream_put().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 drivers/block/zram/zcomp.c    | 4 ++--
 drivers/block/zram/zcomp.h    | 4 ++--
 drivers/block/zram/zram_drv.c | 8 ++++----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index b51a816..400f826 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -95,12 +95,12 @@ bool zcomp_available_algorithm(const char *comp)
 	return find_backend(comp) != NULL;
 }
 
-struct zcomp_strm *zcomp_strm_find(struct zcomp *comp)
+struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
 {
 	return *get_cpu_ptr(comp->stream);
 }
 
-void zcomp_strm_release(struct zcomp *comp, struct zcomp_strm *zstrm)
+void zcomp_stream_put(struct zcomp *comp)
 {
 	put_cpu_ptr(comp->stream);
 }
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index ffd88cb..944b8e6 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -48,8 +48,8 @@ bool zcomp_available_algorithm(const char *comp);
 struct zcomp *zcomp_create(const char *comp);
 void zcomp_destroy(struct zcomp *comp);
 
-struct zcomp_strm *zcomp_strm_find(struct zcomp *comp);
-void zcomp_strm_release(struct zcomp *comp, struct zcomp_strm *zstrm);
+struct zcomp_strm *zcomp_stream_get(struct zcomp *comp);
+void zcomp_stream_put(struct zcomp *comp);
 
 int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
 		const unsigned char *src, size_t *dst_len);
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 8fcad8b..9361a5d 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -695,7 +695,7 @@ compress_again:
 		goto out;
 	}
 
-	zstrm = zcomp_strm_find(zram->comp);
+	zstrm = zcomp_stream_get(zram->comp);
 	ret = zcomp_compress(zram->comp, zstrm, uncmem, &clen);
 	if (!is_partial_io(bvec)) {
 		kunmap_atomic(user_mem);
@@ -734,7 +734,7 @@ compress_again:
 				__GFP_NOWARN |
 				__GFP_HIGHMEM);
 	if (!handle) {
-		zcomp_strm_release(zram->comp, zstrm);
+		zcomp_stream_put(zram->comp);
 		zstrm = NULL;
 
 		atomic64_inc(&zram->stats.writestall);
@@ -769,7 +769,7 @@ compress_again:
 		memcpy(cmem, src, clen);
 	}
 
-	zcomp_strm_release(zram->comp, zstrm);
+	zcomp_stream_put(zram->comp);
 	zstrm = NULL;
 	zs_unmap_object(meta->mem_pool, handle);
 
@@ -789,7 +789,7 @@ compress_again:
 	atomic64_inc(&zram->stats.pages_stored);
 out:
 	if (zstrm)
-		zcomp_strm_release(zram->comp, zstrm);
+		zcomp_stream_put(zram->comp);
 	if (is_partial_io(bvec))
 		kfree(uncmem);
 	return ret;
-- 
2.8.3.394.g3916adf

[toc] | [prev] | [next] | [standalone]

#1407316 — Re: [PATCH 1/7] zram: rename zstrm find-release functions

On Wed, May 25, 2016 at 11:30:00PM +0900, Sergey Senozhatsky wrote:
> We don't perform any zstream idle list lookup anymore, so
> zcomp_strm_find()/zcomp_strm_release() names are not
> representative.
> 
> Rename to zcomp_stream_get()/zcomp_stream_put().

Actually, I wanted it when we applied percpu but didn't say to you because

1. It's preference of author.

Frankly speaking, I prefer get to find but you might think different
with me so I want to respect patch author's right if it's not huge pain
to me. :)
Now I realized you were on same page.

2. We might roll back to stream list.

In that case, find is proper word again but it's too trivial.
So, I want to merge this patch regardless of this patchset. :)

> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Minchan Kim <minchan@kernel.org>
Acked-by: Minchan Kim <minchan@kernel.org>
 

[toc] | [prev] | [next] | [standalone]

#1407320 — Re: [PATCH 1/7] zram: rename zstrm find-release functions

On (05/26/16 09:44), Minchan Kim wrote:
> On Wed, May 25, 2016 at 11:30:00PM +0900, Sergey Senozhatsky wrote:
> > We don't perform any zstream idle list lookup anymore, so
> > zcomp_strm_find()/zcomp_strm_release() names are not
> > representative.
> > 
> > Rename to zcomp_stream_get()/zcomp_stream_put().
> 
> Actually, I wanted it when we applied percpu but didn't say to you because
> 
> 1. It's preference of author.
> 
> Frankly speaking, I prefer get to find but you might think different
> with me so I want to respect patch author's right if it's not huge pain
> to me. :)
> Now I realized you were on same page.
> 
> 2. We might roll back to stream list.
> 
> In that case, find is proper word again but it's too trivial.
> So, I want to merge this patch regardless of this patchset. :)
> 
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > Cc: Minchan Kim <minchan@kernel.org>
> Acked-by: Minchan Kim <minchan@kernel.org>

thanks.

	-ss

[toc] | [prev] | [next] | [standalone]

#1407315

On Wed, May 25, 2016 at 11:29:59PM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
>   This has started as a 'add zlib support' work, but after some
> thinking I saw no blockers for a bigger change -- a switch to
> crypto API.
> 
>   We don't have an idle zstreams list anymore and our write path
> now works absolutely differently, preventing preemption during
> compression. This removes possibilities of read paths preempting
> writes at wrong places and opens the door for a move from custom
> LZO/LZ4 compression backends implementation to a more generic one,
> using crypto compress API.

Hello, Sergey.

I don't look at each patches deeply but nice work! I didn't notice that
rececnt zram changes makes thing simpler. :)

Thanks.

[toc] | [prev] | [next] | [standalone]

#1407326

On (05/26/16 09:43), Joonsoo Kim wrote:
[..]
> Hello, Sergey.
> 
> I don't look at each patches deeply but nice work! I didn't notice that
> rececnt zram changes makes thing simpler. :)

Hello Joonsoo,

thanks.

I owe you a drink for pushing it in the context-less crypto
API direction. sorry about that.

	-ss

[toc] | [prev] | [next] | [standalone]

#1407339

On Thu, May 26, 2016 at 10:12:16AM +0900, Sergey Senozhatsky wrote:
> On (05/26/16 09:43), Joonsoo Kim wrote:
> [..]
> > Hello, Sergey.
> > 
> > I don't look at each patches deeply but nice work! I didn't notice that
> > rececnt zram changes makes thing simpler. :)
> 
> Hello Joonsoo,
> 
> thanks.
> 
> I owe you a drink for pushing it in the context-less crypto
> API direction. sorry about that.

Good to hear. I will wait for your invitation. :)

Thanks.

[toc] | [prev] | [next] | [standalone]

#1407317

On Wed, May 25, 2016 at 11:29:59PM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
>   This has started as a 'add zlib support' work, but after some
> thinking I saw no blockers for a bigger change -- a switch to
> crypto API.
> 
>   We don't have an idle zstreams list anymore and our write path
> now works absolutely differently, preventing preemption during
> compression. This removes possibilities of read paths preempting
> writes at wrong places and opens the door for a move from custom
> LZO/LZ4 compression backends implementation to a more generic one,
> using crypto compress API.
> 
>   This patch set also eliminates the need of a new context-less
> crypto API interface, which was quite hard to sell, so we can
> move along faster.

Super fast Sergey.
At a first glance, patchset looks nice.
I will review as soon as possible.

Thanks a lot!

> 
> 
> Sergey Senozhatsky (7):
>   zram: rename zstrm find-release functions
>   zram: switch to crypto compress API
>   zram: drop zcomp param from compress/decompress
>   zram: align zcomp interface to crypto comp API
>   zram: use crypto api to check alg availability
>   zram: delete custom lzo/lz4
>   zram: add more compression algorithms
> 
>  drivers/block/zram/Kconfig     | 15 +------
>  drivers/block/zram/Makefile    |  4 +-
>  drivers/block/zram/zcomp.c     | 91 +++++++++++++++++++++++++++---------------
>  drivers/block/zram/zcomp.h     | 29 ++++----------
>  drivers/block/zram/zcomp_lz4.c | 56 --------------------------
>  drivers/block/zram/zcomp_lz4.h | 17 --------
>  drivers/block/zram/zcomp_lzo.c | 56 --------------------------
>  drivers/block/zram/zcomp_lzo.h | 17 --------
>  drivers/block/zram/zram_drv.c  | 26 +++++++-----
>  9 files changed, 84 insertions(+), 227 deletions(-)
>  delete mode 100644 drivers/block/zram/zcomp_lz4.c
>  delete mode 100644 drivers/block/zram/zcomp_lz4.h
>  delete mode 100644 drivers/block/zram/zcomp_lzo.c
>  delete mode 100644 drivers/block/zram/zcomp_lzo.h
> 
> -- 
> 2.8.3.394.g3916adf
> 

[toc] | [prev] | [next] | [standalone]

#1407324

On (05/26/16 09:52), Minchan Kim wrote:
> On Wed, May 25, 2016 at 11:29:59PM +0900, Sergey Senozhatsky wrote:
> > Hello,
> > 
> >   This has started as a 'add zlib support' work, but after some
> > thinking I saw no blockers for a bigger change -- a switch to
> > crypto API.
> > 
> >   We don't have an idle zstreams list anymore and our write path
> > now works absolutely differently, preventing preemption during
> > compression. This removes possibilities of read paths preempting
> > writes at wrong places and opens the door for a move from custom
> > LZO/LZ4 compression backends implementation to a more generic one,
> > using crypto compress API.
> > 
> >   This patch set also eliminates the need of a new context-less
> > crypto API interface, which was quite hard to sell, so we can
> > move along faster.
> 
> Super fast Sergey.

hahaha :)

> At a first glance, patchset looks nice.
> I will review as soon as possible.
> 
> Thanks a lot!

thanks!

	-ss

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web