linux-kernel - Re: [PATCH v2] zswap: allow setting default status, compressor and allocator in Kconfig

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eb65a87b-5bfe-345e-dd83-39243db717a9@suse.cz>
Date:   Thu, 24 Oct 2019 13:04:07 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>,
        Seth Jennings <sjenning@...hat.com>,
        Dan Streetman <ddstreet@...e.org>
Cc:     Vitaly Wool <vitalywool@...il.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] zswap: allow setting default status, compressor and
 allocator in Kconfig

On 10/24/19 1:22 AM, Maciej S. Szmigiero wrote:
> The compressed cache for swap pages (zswap) currently needs from 1 to 3
> extra kernel command line parameters in order to make it work: it has to be
> enabled by adding a "zswap.enabled=1" command line parameter and if one
> wants a different compressor or pool allocator than the default lzo / zbud
> combination then these choices also need to be specified on the kernel
> command line in additional parameters.
> 
> Using a different compressor and allocator for zswap is actually pretty
> common as guides often recommend using the lz4 / z3fold pair instead of
> the default one.
> In such case it is also necessary to remember to enable the appropriate
> compression algorithm and pool allocator in the kernel config manually.
> 
> Let's avoid the need for adding these kernel command line parameters and
> automatically pull in the dependencies for the selected compressor
> algorithm and pool allocator by adding an appropriate default switches to
> Kconfig.
> 
> The default values for these options match what the code was using
> previously as its defaults.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@...iej.szmigiero.name>
> ---
> Changes from v1:
> Rename CONFIG_ZSWAP_DEFAULT_COMP_* to CONFIG_ZSWAP_COMPRESSOR_DEFAULT_*
> and CONFIG_ZSWAP_DEFAULT_ZPOOL_* to CONFIG_ZSWAP_ZPOOL_DEFAULT_* while
> dropping the "_NAME" suffix from the final string option in both cases.
> 
>  mm/Kconfig | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  mm/zswap.c |  26 ++++++++------
>  2 files changed, 117 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/Kconfig b/mm/Kconfig
> index a5dae9a7eb51..267316941324 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -525,7 +525,6 @@ config MEM_SOFT_DIRTY
>  config ZSWAP
>  	bool "Compressed cache for swap pages (EXPERIMENTAL)"
>  	depends on FRONTSWAP && CRYPTO=y
> -	select CRYPTO_LZO
>  	select ZPOOL
>  	help
>  	  A lightweight compressed cache for swap pages.  It takes
> @@ -541,6 +540,108 @@ config ZSWAP
>  	  they have not be fully explored on the large set of potential
>  	  configurations and workloads that exist.
>  
> +choice
> +	prompt "Compressed cache for swap pages default compressor"
> +	depends on ZSWAP
> +	default ZSWAP_COMPRESSOR_DEFAULT_LZO
> +	help
> +	  Selects the default compression algorithm for the compressed cache
> +	  for swap pages.
> +	  If in doubt, select 'LZO'.

Could it e.g. suggest which one is the fastest and which most space
efficient?
Also does this cover all compression algorithms? Are we going to add new
options if there are new ones? Wouldn't a string instead of choice be
sufficient here?

> +
> +choice
> +	prompt "Compressed cache for swap pages default allocator"
> +	depends on ZSWAP
> +	default ZSWAP_ZPOOL_DEFAULT_ZBUD
> +	help
> +	  Selects the default allocator for the compressed cache for
> +	  swap pages.
> +	  The default is 'zbud' for compatibility, however please do
> +	  read the description of each of the allocators below before
> +	  making a right choice.

It's somewhat unfortunate that the choice options don't include the
description and one has to go look for it elsewhere.

Also, shouldn't the list include zsmalloc?

> +	  The selection made here can be overridden by using the kernel
> +	  command line 'zswap.zpool=' option.
> +
> +config ZSWAP_ZPOOL_DEFAULT_ZBUD
> +	bool "zbud"
> +	select ZBUD
> +	help
> +	  Use the zbud allocator as the default allocator.
> +
> +config ZSWAP_ZPOOL_DEFAULT_Z3FOLD
> +	bool "z3fold"
> +	select Z3FOLD
> +	help
> +	  Use the z3fold allocator as the default allocator.
> +endchoice
> +
> +config ZSWAP_ZPOOL_DEFAULT
> +       string
> +       default "zbud" if ZSWAP_ZPOOL_DEFAULT_ZBUD
> +       default "z3fold" if ZSWAP_ZPOOL_DEFAULT_Z3FOLD
> +       default ""
> +
> +config ZSWAP_DEFAULT_ON
> +	bool "Enable the compressed cache for swap pages by default"
> +	depends on ZSWAP
> +	help
> +	  If selected, the compressed cache for swap pages will be enabled
> +	  at boot, otherwise it will be disabled.
> +
> +	  The selection made here can be overridden by using the kernel
> +	  command line 'zswap.enabled=' option.
> +
>  config ZPOOL
>  	tristate "Common API for compressed memory storage"
>  	help
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 46a322316e52..71795b6f5b71 100644
> --- a/mm/zswap.c