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

[Sergey Senozhatsky]

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

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

[Sergey Senozhatsky]

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

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

[Minchan Kim]

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

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

[Minchan Kim]

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

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

[Sergey Senozhatsky]

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

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

[Sergey Senozhatsky]

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

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

[Minchan Kim]

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 
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Minchan Kim]

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 
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

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 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

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 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Minchan Kim]

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

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

[Minchan Kim]

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

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

[Sergey Senozhatsky]

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

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

[Sergey Senozhatsky]

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

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

[Minchan Kim]

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 
> Cc: Minchan Kim 
> Cc: Joonsoo Kim 
> ---
>  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
> 

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

[Minchan Kim]

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 
> Cc: Minchan Kim 
> Cc: Joonsoo Kim 
> ---
>  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
>