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

[Minchan Kim]

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