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

Groups > linux.kernel > #1410249 > unrolled thread

[PATCH v2 4/8] zram: use crypto api to check alg availability

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 4/8] zram: use crypto api to check alg availability Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-05-31 14:30 +0200
    Re: [PATCH v2 4/8] zram: use crypto api to check alg availability Minchan Kim <minchan@kernel.org> - 2016-06-01 02:10 +0200
      Re: [PATCH v2 4/8] zram: use crypto api to check alg availability Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-06-01 03:10 +0200
        Re: [PATCH v2 4/8] zram: use crypto api to check alg availability Minchan Kim <minchan@kernel.org> - 2016-06-01 04:30 +0200
          Re: [PATCH v2 4/8] zram: use crypto api to check alg availability Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-06-01 05:20 +0200
            Re: [PATCH v2 4/8] zram: use crypto api to check alg availability Minchan Kim <minchan@kernel.org> - 2016-06-01 08:50 +0200
              Re: [PATCH v2 4/8] zram: use crypto api to check alg availability Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-06-01 09:50 +0200
                Re: [PATCH v2 4/8] zram: use crypto api to check alg availability "Austin S. Hemmelgarn" <ahferroin7@gmail.com> - 2016-06-01 17:10 +0200
                Re: [PATCH v2 4/8] zram: use crypto api to check alg availability Minchan Kim <minchan@kernel.org> - 2016-06-02 04:50 +0200

#1410249 — [PATCH v2 4/8] 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

We also now fully rely on crypto_has_comp() when configure a new
device. The existing `backends' array is kept for user's convenience
only -- there is no way to list all of the compression algorithms
supported by crypto -- and is not guaranteed to contain every
compression module name supported by the kernel. Switch to
crypto_has_comp() has an advantage of permitting the usage of
out-of-tree crypto compression modules (implementing S/W or H/W
compression).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 Documentation/blockdev/zram.txt | 11 ++++++++
 drivers/block/zram/zcomp.c      | 58 ++++++++++++++++++++++++-----------------
 drivers/block/zram/zram_drv.c   | 16 +++++++-----
 drivers/block/zram/zram_drv.h   |  5 ++--
 4 files changed, 57 insertions(+), 33 deletions(-)

diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index 13100fb..7c05357 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -83,6 +83,17 @@ pre-created. Default: 1.
 	#select lzo compression algorithm
 	echo lzo > /sys/block/zram0/comp_algorithm
 
+	For the time being, the `comp_algorithm' content does not necessarily
+	show every compression algorithm supported by the kernel. We keep this
+	list primarily to simplify device configuration and one can configure
+	a new device with a compression algorithm that is not listed in
+	`comp_algorithm'. The thing is that, internally, ZRAM uses Crypto API
+	and, if some of the algorithms were built as modules, it's impossible
+	to list all of them using, for instance, /proc/crypto or any other
+	method. This, however, has an advantage of permitting the usage of
+	custom crypto compression modules (implementing S/W or H/W
+	compression).
+
 4) 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.
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index f357268..2381ca9 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -26,17 +26,6 @@ static const char * const backends[] = {
 	NULL
 };
 
-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];
-}
-
 static void zcomp_strm_free(struct zcomp_strm *zstrm)
 {
 	if (!IS_ERR_OR_NULL(zstrm->tfm))
@@ -68,30 +57,53 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp, gfp_t flags)
 	return zstrm;
 }
 
+bool zcomp_available_algorithm(const char *comp)
+{
+	/*
+	 * Crypto does not ignore a trailing new line symbol,
+	 * so make sure you don't supply a string containing
+	 * one.
+	 * This also means that we keep `backends' array for
+	 * zcomp_available_show() only and will init a new zram
+	 * device with any compressing algorithm known to crypto
+	 * api.
+	 */
+	return crypto_has_comp(comp, 0, 0) == 1;
+}
+
 /* show available compressors */
 ssize_t zcomp_available_show(const char *comp, char *buf)
 {
+	bool known_algorithm = false;
 	ssize_t sz = 0;
 	int i = 0;
 
-	while (backends[i]) {
-		if (!strcmp(comp, backends[i]))
+	for (; backends[i]; i++) {
+		if (!zcomp_available_algorithm(backends[i]))
+			continue;
+
+		if (!strcmp(comp, backends[i])) {
+			known_algorithm = true;
 			sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
 					"[%s] ", backends[i]);
-		else
+		} else {
 			sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
 					"%s ", backends[i]);
-		i++;
+		}
 	}
+
+	/*
+	 * Out-of-tree module known to crypto api or a missing
+	 * entry in `backends'.
+	 */
+	if (!known_algorithm && zcomp_available_algorithm(comp))
+		sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
+				"[%s] ", comp);
+
 	sz += scnprintf(buf + sz, PAGE_SIZE - sz, "\n");
 	return sz;
 }
 
-bool zcomp_available_algorithm(const char *comp)
-{
-	return find_backend(comp) != NULL;
-}
-
 struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
 {
 	return *get_cpu_ptr(comp->stream);
@@ -227,18 +239,16 @@ void zcomp_destroy(struct zcomp *comp)
 struct zcomp *zcomp_create(const char *compress)
 {
 	struct zcomp *comp;
-	const char *backend;
 	int error;
 
-	backend = find_backend(compress);
-	if (!backend)
+	if (!zcomp_available_algorithm(compress))
 		return ERR_PTR(-EINVAL);
 
 	comp = kzalloc(sizeof(struct zcomp), GFP_KERNEL);
 	if (!comp)
 		return ERR_PTR(-ENOMEM);
 
-	comp->name = backend;
+	comp->name = compress;
 	error = zcomp_init(comp);
 	if (error) {
 		kfree(comp);
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 65d1403..c2a1d7d 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -342,9 +342,16 @@ static ssize_t comp_algorithm_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t len)
 {
 	struct zram *zram = dev_to_zram(dev);
+	char compressor[CRYPTO_MAX_ALG_NAME];
 	size_t sz;
 
-	if (!zcomp_available_algorithm(buf))
+	strlcpy(compressor, buf, sizeof(compressor));
+	/* ignore trailing newline */
+	sz = strlen(compressor);
+	if (sz > 0 && compressor[sz - 1] == '\n')
+		compressor[sz - 1] = 0x00;
+
+	if (!zcomp_available_algorithm(compressor))
 		return -EINVAL;
 
 	down_write(&zram->init_lock);
@@ -353,13 +360,8 @@ static ssize_t comp_algorithm_store(struct device *dev,
 		pr_info("Can't change algorithm for initialized device\n");
 		return -EBUSY;
 	}
-	strlcpy(zram->compressor, buf, sizeof(zram->compressor));
-
-	/* ignore trailing newline */
-	sz = strlen(zram->compressor);
-	if (sz > 0 && zram->compressor[sz - 1] == '\n')
-		zram->compressor[sz - 1] = 0x00;
 
+	strlcpy(zram->compressor, compressor, sizeof(compressor));
 	up_write(&zram->init_lock);
 	return len;
 }
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 3f5bf66..74fcf10 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -15,8 +15,9 @@
 #ifndef _ZRAM_DRV_H_
 #define _ZRAM_DRV_H_
 
-#include <linux/spinlock.h>
+#include <linux/rwsem.h>
 #include <linux/zsmalloc.h>
+#include <linux/crypto.h>
 
 #include "zcomp.h"
 
@@ -113,7 +114,7 @@ struct zram {
 	 * we can store in a disk.
 	 */
 	u64 disksize;	/* bytes */
-	char compressor[10];
+	char compressor[CRYPTO_MAX_ALG_NAME];
 	/*
 	 * zram is claimed so open request will be failed
 	 */
-- 
2.8.3.394.g3916adf

[toc] | [next] | [standalone]

#1410696

On Tue, May 31, 2016 at 09:20:13PM +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().
> 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.

So, if we do 'cat /sys/block/zram0/comp_algorithm", every crypto modules
in the backend array are loaded in memory and not unloaded until admin
executes rmmod? Right?

> 
> 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
> 
> We also now fully rely on crypto_has_comp() when configure a new
> device. The existing `backends' array is kept for user's convenience
> only -- there is no way to list all of the compression algorithms
> supported by crypto -- and is not guaranteed to contain every
> compression module name supported by the kernel. Switch to
> crypto_has_comp() has an advantage of permitting the usage of
> out-of-tree crypto compression modules (implementing S/W or H/W
> compression).

If user load out-of-tree crypto compression module, what's status of
comp_algorithm?

#> insmod foo_crypto.ko
#> echo foo > /sys/block/zram0/comp_algorithm
#> cat /sys/block/zram0/comp_algorithm
lzo lz4 [foo]
?

> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
>  Documentation/blockdev/zram.txt | 11 ++++++++
>  drivers/block/zram/zcomp.c      | 58 ++++++++++++++++++++++++-----------------
>  drivers/block/zram/zram_drv.c   | 16 +++++++-----
>  drivers/block/zram/zram_drv.h   |  5 ++--
>  4 files changed, 57 insertions(+), 33 deletions(-)
> 
> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> index 13100fb..7c05357 100644
> --- a/Documentation/blockdev/zram.txt
> +++ b/Documentation/blockdev/zram.txt
> @@ -83,6 +83,17 @@ pre-created. Default: 1.
>  	#select lzo compression algorithm
>  	echo lzo > /sys/block/zram0/comp_algorithm
>  
> +	For the time being, the `comp_algorithm' content does not necessarily
> +	show every compression algorithm supported by the kernel. We keep this
> +	list primarily to simplify device configuration and one can configure
> +	a new device with a compression algorithm that is not listed in
> +	`comp_algorithm'. The thing is that, internally, ZRAM uses Crypto API
> +	and, if some of the algorithms were built as modules, it's impossible
> +	to list all of them using, for instance, /proc/crypto or any other
> +	method. This, however, has an advantage of permitting the usage of
> +	custom crypto compression modules (implementing S/W or H/W
> +	compression).
> +
>  4) 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.
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index f357268..2381ca9 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -26,17 +26,6 @@ static const char * const backends[] = {
>  	NULL
>  };
>  
> -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];
> -}
> -
>  static void zcomp_strm_free(struct zcomp_strm *zstrm)
>  {
>  	if (!IS_ERR_OR_NULL(zstrm->tfm))
> @@ -68,30 +57,53 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp, gfp_t flags)
>  	return zstrm;
>  }
>  
> +bool zcomp_available_algorithm(const char *comp)
> +{
> +	/*
> +	 * Crypto does not ignore a trailing new line symbol,
> +	 * so make sure you don't supply a string containing
> +	 * one.
> +	 * This also means that we keep `backends' array for
> +	 * zcomp_available_show() only and will init a new zram
> +	 * device with any compressing algorithm known to crypto
> +	 * api.
> +	 */
> +	return crypto_has_comp(comp, 0, 0) == 1;
> +}
> +
>  /* show available compressors */
>  ssize_t zcomp_available_show(const char *comp, char *buf)
>  {
> +	bool known_algorithm = false;
>  	ssize_t sz = 0;
>  	int i = 0;
>  
> -	while (backends[i]) {
> -		if (!strcmp(comp, backends[i]))
> +	for (; backends[i]; i++) {
> +		if (!zcomp_available_algorithm(backends[i]))
> +			continue;
> +
> +		if (!strcmp(comp, backends[i])) {
> +			known_algorithm = true;
>  			sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
>  					"[%s] ", backends[i]);
> -		else
> +		} else {
>  			sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
>  					"%s ", backends[i]);
> -		i++;
> +		}
>  	}
> +
> +	/*
> +	 * Out-of-tree module known to crypto api or a missing
> +	 * entry in `backends'.
> +	 */
> +	if (!known_algorithm && zcomp_available_algorithm(comp))
> +		sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
> +				"[%s] ", comp);
> +
>  	sz += scnprintf(buf + sz, PAGE_SIZE - sz, "\n");
>  	return sz;
>  }
>  
> -bool zcomp_available_algorithm(const char *comp)
> -{
> -	return find_backend(comp) != NULL;
> -}
> -
>  struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
>  {
>  	return *get_cpu_ptr(comp->stream);
> @@ -227,18 +239,16 @@ void zcomp_destroy(struct zcomp *comp)
>  struct zcomp *zcomp_create(const char *compress)
>  {
>  	struct zcomp *comp;
> -	const char *backend;
>  	int error;
>  
> -	backend = find_backend(compress);
> -	if (!backend)
> +	if (!zcomp_available_algorithm(compress))
>  		return ERR_PTR(-EINVAL);
>  
>  	comp = kzalloc(sizeof(struct zcomp), GFP_KERNEL);
>  	if (!comp)
>  		return ERR_PTR(-ENOMEM);
>  
> -	comp->name = backend;
> +	comp->name = compress;
>  	error = zcomp_init(comp);
>  	if (error) {
>  		kfree(comp);
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 65d1403..c2a1d7d 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -342,9 +342,16 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  		struct device_attribute *attr, const char *buf, size_t len)
>  {
>  	struct zram *zram = dev_to_zram(dev);
> +	char compressor[CRYPTO_MAX_ALG_NAME];
>  	size_t sz;
>  
> -	if (!zcomp_available_algorithm(buf))
> +	strlcpy(compressor, buf, sizeof(compressor));
> +	/* ignore trailing newline */
> +	sz = strlen(compressor);
> +	if (sz > 0 && compressor[sz - 1] == '\n')
> +		compressor[sz - 1] = 0x00;
> +
> +	if (!zcomp_available_algorithm(compressor))
>  		return -EINVAL;
>  
>  	down_write(&zram->init_lock);
> @@ -353,13 +360,8 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  		pr_info("Can't change algorithm for initialized device\n");
>  		return -EBUSY;
>  	}
> -	strlcpy(zram->compressor, buf, sizeof(zram->compressor));
> -
> -	/* ignore trailing newline */
> -	sz = strlen(zram->compressor);
> -	if (sz > 0 && zram->compressor[sz - 1] == '\n')
> -		zram->compressor[sz - 1] = 0x00;
>  
> +	strlcpy(zram->compressor, compressor, sizeof(compressor));
>  	up_write(&zram->init_lock);
>  	return len;
>  }
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 3f5bf66..74fcf10 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -15,8 +15,9 @@
>  #ifndef _ZRAM_DRV_H_
>  #define _ZRAM_DRV_H_
>  
> -#include <linux/spinlock.h>
> +#include <linux/rwsem.h>
>  #include <linux/zsmalloc.h>
> +#include <linux/crypto.h>
>  
>  #include "zcomp.h"
>  
> @@ -113,7 +114,7 @@ struct zram {
>  	 * we can store in a disk.
>  	 */
>  	u64 disksize;	/* bytes */
> -	char compressor[10];
> +	char compressor[CRYPTO_MAX_ALG_NAME];
>  	/*
>  	 * zram is claimed so open request will be failed
>  	 */
> -- 
> 2.8.3.394.g3916adf
> 

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

#1410712

Hello Minchan,

On (06/01/16 09:03), Minchan Kim wrote:
[..]
> So, if we do 'cat /sys/block/zram0/comp_algorithm", every crypto modules
> in the backend array are loaded in memory and not unloaded until admin
> executes rmmod? Right?

yes, I think so.

[..]
> If user load out-of-tree crypto compression module, what's status of
> comp_algorithm?
> 
> #> insmod foo_crypto.ko
> #> echo foo > /sys/block/zram0/comp_algorithm
> #> cat /sys/block/zram0/comp_algorithm
> lzo lz4 [foo]
> ?

yes, "lzo lz4 [out-of-tree-module-name]".

	-ss

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

#1410744

On Wed, Jun 01, 2016 at 10:07:07AM +0900, Sergey Senozhatsky wrote:
> Hello Minchan,
> 
> On (06/01/16 09:03), Minchan Kim wrote:
> [..]
> > So, if we do 'cat /sys/block/zram0/comp_algorithm", every crypto modules
> > in the backend array are loaded in memory and not unloaded until admin
> > executes rmmod? Right?
> 
> yes, I think so.

It scares me. Common case, except one we choosed, every loaded modules
will be not used. I think it's really not good. Although the wastage
might be not big now, it will be heavy as crypto comp modules are
increased.

What do you think about it?

> 
> [..]
> > If user load out-of-tree crypto compression module, what's status of
> > comp_algorithm?
> > 
> > #> insmod foo_crypto.ko
> > #> echo foo > /sys/block/zram0/comp_algorithm
> > #> cat /sys/block/zram0/comp_algorithm
> > lzo lz4 [foo]
> > ?
> 
> yes, "lzo lz4 [out-of-tree-module-name]".

Makes sense!

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

#1410799

On (06/01/16 11:27), Minchan Kim wrote:
[..]
> > > So, if we do 'cat /sys/block/zram0/comp_algorithm", every crypto modules
> > > in the backend array are loaded in memory and not unloaded until admin
> > > executes rmmod? Right?
> > 
> > yes, I think so.
> 
> It scares me. Common case, except one we choosed, every loaded modules
> will be not used. I think it's really not good. Although the wastage
> might be not big now, it will be heavy as crypto comp modules are
> increased.

well... if you have those modules enabled then you somehow expect
them to be loaded at some point, if not by zram, then by something
else (networking, etc.). /* not speaking of the systems that have
those modules built-in */ I'm not saying that what we have is
optimal, of course, but it's not so senseless at the same time.


> What do you think about it?

I can do something like this:

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 1a4bd20..9b704cc 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -20,10 +20,18 @@
 
 static const char * const backends[] = {
        "lzo",
+#if IS_ENABLED(CONFIG_CRYPTO_LZ4)
        "lz4",
+#endif
+#if IS_ENABLED(CONFIG_CRYPTO_DEFLATE)
        "deflate",
+#endif
+#if IS_ENABLED(CONFIG_CRYPTO_LZ4HC)
        "lz4hc",
+#endif
+#if IS_ENABLED(CONFIG_CRYPTO_842)
        "842",
+#endif
        NULL
 };


so both BUILTIN and BUILT-AS-A-MODULE cases are handled at compile
time now and we can avoid crypto_has_comp() checks for most of the
comp_algorithm calls, except for the case when someone requests an
out-of-tree module.

	-ss

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

#1410858

On Wed, Jun 01, 2016 at 12:17:35PM +0900, Sergey Senozhatsky wrote:
> On (06/01/16 11:27), Minchan Kim wrote:
> [..]
> > > > So, if we do 'cat /sys/block/zram0/comp_algorithm", every crypto modules
> > > > in the backend array are loaded in memory and not unloaded until admin
> > > > executes rmmod? Right?
> > > 
> > > yes, I think so.
> > 
> > It scares me. Common case, except one we choosed, every loaded modules
> > will be not used. I think it's really not good. Although the wastage
> > might be not big now, it will be heavy as crypto comp modules are
> > increased.
> 
> well... if you have those modules enabled then you somehow expect
> them to be loaded at some point, if not by zram, then by something
> else (networking, etc.). /* not speaking of the systems that have
> those modules built-in */ I'm not saying that what we have is
> optimal, of course, but it's not so senseless at the same time.

In my local system, there are a lot of modules I am not using but
distro installed for some point but I believe that some point will
not come. IMO, they shouldn't be loaded by the reason they will be
used some point, potentially.

> 
> 
> > What do you think about it?
> 
> I can do something like this:
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 1a4bd20..9b704cc 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -20,10 +20,18 @@
>  
>  static const char * const backends[] = {
>         "lzo",
> +#if IS_ENABLED(CONFIG_CRYPTO_LZ4)
>         "lz4",
> +#endif
> +#if IS_ENABLED(CONFIG_CRYPTO_DEFLATE)
>         "deflate",
> +#endif
> +#if IS_ENABLED(CONFIG_CRYPTO_LZ4HC)
>         "lz4hc",
> +#endif
> +#if IS_ENABLED(CONFIG_CRYPTO_842)
>         "842",
> +#endif
>         NULL
>  };
> 
> 
> so both BUILTIN and BUILT-AS-A-MODULE cases are handled at compile
> time now and we can avoid crypto_has_comp() checks for most of the
> comp_algorithm calls, except for the case when someone requests an
> out-of-tree module.

Hmm, isn't it problem, either?

That module was built but not installed. In that case, setting the
algorithm will be failed. IOW, we are lying to user.
For solving the problem, if we check it with crypto_has_comp, again,
it will load module into memory. :(

1) Use IS_BUILTIN, not IS_ENABLED for backend

For module-crypto, user should set directly without supporting from
backend.

2) Deprecated /sys/block/zram0/comp_alrogithm totally

Admin should set algorithm by himself like zswap and other cyrpto users.

3) Supporting from zramctl.

Maybe, we can teach zramctl so zramctl can find /lib/modules/`uname-r`/
into not-yet-known-modules and use lsmod to find loaded-known-module for
zram to use compression algorithm.

So, 1,3 combination can be or 2,3 combination can be.

Welcome other suggestion.

> 
> 	-ss

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

#1410895

On (06/01/16 15:47), Minchan Kim wrote:
[..]
> > so both BUILTIN and BUILT-AS-A-MODULE cases are handled at compile
> > time now and we can avoid crypto_has_comp() checks for most of the
> > comp_algorithm calls, except for the case when someone requests an
> > out-of-tree module.
> 
> Hmm, isn't it problem, either?
> 
> That module was built but not installed. In that case, setting the
> algorithm will be failed. IOW, we are lying to user.

have you ever seen this? really, why should we even bother?
if there is no requested algorithm we will fallback to LZO.

and how is that different from: user enabled LZO in .config (because it's
a prerequisite for zram) but forgot to install the module? do we have to
"fix" this as well?... implement our own LZO compression in zram?
or `cp lib/lzo/* drivers/block/zram/'?

> For solving the problem, if we check it with crypto_has_comp, again,
> it will load module into memory. :(

this will require a *VERY* non-standard behaviour from user

	cat /sys/block/zram0/comp_algorithm
	[lzo] lz4
	# um...
	echo 842 > /sys/block/zram0/comp_algorithm

and I'm quite confident that anyone who does this actually want
to init the device with the requested out-of-tree module right
after `echo FOO > comp_algorithm', rather than anything else.

	-ss

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

#1411302

On 2016-06-01 03:48, Sergey Senozhatsky wrote:
> On (06/01/16 15:47), Minchan Kim wrote:
> [..]
>>> so both BUILTIN and BUILT-AS-A-MODULE cases are handled at compile
>>> time now and we can avoid crypto_has_comp() checks for most of the
>>> comp_algorithm calls, except for the case when someone requests an
>>> out-of-tree module.
>>
>> Hmm, isn't it problem, either?
>>
>> That module was built but not installed. In that case, setting the
>> algorithm will be failed. IOW, we are lying to user.
>
> have you ever seen this? really, why should we even bother?
> if there is no requested algorithm we will fallback to LZO.
>
> and how is that different from: user enabled LZO in .config (because it's
> a prerequisite for zram) but forgot to install the module? do we have to
> "fix" this as well?... implement our own LZO compression in zram?
> or `cp lib/lzo/* drivers/block/zram/'?
Ideally, it should fall back to whatever algorithm it can find that is 
supported, preferably in the following order:
lzo lz4 lz4hc deflate 842
LZO first will keep backwards compatibility, while the rest is roughly 
in decreasing order of performance on most hardware (except on PPC 
systems which have hardware support for 842 compression).

Handling not being able to find any algorithm gets trickier, and the 
choices are pretty much use a null algorithm and just store flat data, 
or refuse to store anything.
>
>> For solving the problem, if we check it with crypto_has_comp, again,
>> it will load module into memory. :(
>
> this will require a *VERY* non-standard behaviour from user
>
> 	cat /sys/block/zram0/comp_algorithm
> 	[lzo] lz4
> 	# um...
> 	echo 842 > /sys/block/zram0/comp_algorithm
>
> and I'm quite confident that anyone who does this actually want
> to init the device with the requested out-of-tree module right
> after `echo FOO > comp_algorithm', rather than anything else.
Just from the perspective of a system administrator, most people 
probably aren't going to be directly touching the sysfs entries 
themselves except possibly for testing, and I don't think I've ever seen 
anything that actually reads zram/comp_algorithm except to verify that 
it's set to the requested algorithm.  Given that, the behavior I'd 
expect from zram/comp_algorithm as an administrator would be:
1. List the currently used algorithm together with all algorithm's 
supported in the mainline kernel.
2. If somebody writes an algorithm we don't know about, check it with 
crypto_has_comp, and switch to it if successful.
3. Cache positive lookups of unknown algorithms so that you don't have 
to check again on other devices, and list those algorithms in 
zram/comp_algorithm even if they're not being used.
This would provide relative compatibility with the current behavior, 
while still allowing people using unknown compression modules to use them.

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

#1411785

On Wed, Jun 01, 2016 at 04:48:45PM +0900, Sergey Senozhatsky wrote:
> On (06/01/16 15:47), Minchan Kim wrote:
> [..]
> > > so both BUILTIN and BUILT-AS-A-MODULE cases are handled at compile
> > > time now and we can avoid crypto_has_comp() checks for most of the
> > > comp_algorithm calls, except for the case when someone requests an
> > > out-of-tree module.
> > 
> > Hmm, isn't it problem, either?
> > 
> > That module was built but not installed. In that case, setting the
> > algorithm will be failed. IOW, we are lying to user.
> 
> have you ever seen this? really, why should we even bother?
> if there is no requested algorithm we will fallback to LZO.

Yeb, it seems I am too paranoid. Let's not take care about the case.
We can simple return -EINVAL and fallback lzo.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web