Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191117000544.1019556-1-mail@maciej.szmigiero.name>
In-Reply-To: <20191117000544.1019556-1-mail@maciej.szmigiero.name>
From:   Vitaly Wool <vitalywool@gmail.com>
Date:   Mon, 18 Nov 2019 13:43:58 +0100
Message-ID: <CAMJBoFNja-7sY=mU4hWDOqvOhZjvSNqyL1hYqJB+Hm1dygxpCA@mail.gmail.com>
Subject: Re: [PATCH v4] zswap: allow setting default status, compressor and
 allocator in Kconfig
To:     "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>
Cc:     Seth Jennings <sjenning@redhat.com>,
        Dan Streetman <ddstreet@ieee.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        Linux-MM <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Maciej,

sorry for jumping in late into this discussion.

On Sun, Nov 17, 2019 at 1:05 AM Maciej S. Szmigiero
<mail@maciej.szmigiero.name> wrote:
<snip>
> +config ZSWAP_COMPRESSOR_DEFAULT
> +       string
> +       default "deflate" if ZSWAP_COMPRESSOR_DEFAULT_DEFLATE
> +       default "lzo" if ZSWAP_COMPRESSOR_DEFAULT_LZO
> +       default "842" if ZSWAP_COMPRESSOR_DEFAULT_842
> +       default "lz4" if ZSWAP_COMPRESSOR_DEFAULT_LZ4
> +       default "lz4hc" if ZSWAP_COMPRESSOR_DEFAULT_LZ4HC
> +       default "zstd" if ZSWAP_COMPRESSOR_DEFAULT_ZSTD
> +       default ""

I guess this should also depend on ZSWAP. It looks a bit awkward when
this option is present in dotconfig while ZSWAP is not.

> +
> +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.
> +
> +         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.
> +
> +config ZSWAP_ZPOOL_DEFAULT_ZSMALLOC
> +       bool "zsmalloc"
> +       select ZSMALLOC
> +       help
> +         Use the zsmalloc 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 "zsmalloc" if ZSWAP_ZPOOL_DEFAULT_ZSMALLOC
> +       default ""

Same here.

> +
> +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
> +++ b/mm/zswap.c
> @@ -71,8 +71,12 @@ static u64 zswap_duplicate_entry;
>
>  #define ZSWAP_PARAM_UNSET ""
>
> -/* Enable/disable zswap (disabled by default) */
> +/* Enable/disable zswap */
> +#ifdef CONFIG_ZSWAP_DEFAULT_ON
> +static bool zswap_enabled = true;
> +#else
>  static bool zswap_enabled;
> +#endif

Wouldn't it be better to have 'static bool zswap_enabled =
IS_ENABLED(CONFIG_ZSWAP_DEFAULT_ON);' instead?

>  static int zswap_enabled_param_set(const char *,
>                                    const struct kernel_param *);
>  static struct kernel_param_ops zswap_enabled_param_ops = {
> @@ -82,8 +86,7 @@ static struct kernel_param_ops zswap_enabled_param_ops = {
>  module_param_cb(enabled, &zswap_enabled_param_ops, &zswap_enabled, 0644);
>
>  /* Crypto compressor to use */
> -#define ZSWAP_COMPRESSOR_DEFAULT "lzo"
> -static char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
> +static char *zswap_compressor = CONFIG_ZSWAP_COMPRESSOR_DEFAULT;
>  static int zswap_compressor_param_set(const char *,
>                                       const struct kernel_param *);
>  static struct kernel_param_ops zswap_compressor_param_ops = {
> @@ -95,8 +98,7 @@ module_param_cb(compressor, &zswap_compressor_param_ops,
>                 &zswap_compressor, 0644);
>
>  /* Compressed storage zpool to use */
> -#define ZSWAP_ZPOOL_DEFAULT "zbud"
> -static char *zswap_zpool_type = ZSWAP_ZPOOL_DEFAULT;
> +static char *zswap_zpool_type = CONFIG_ZSWAP_ZPOOL_DEFAULT;
>  static int zswap_zpool_param_set(const char *, const struct kernel_param *);
>  static struct kernel_param_ops zswap_zpool_param_ops = {
>         .set =          zswap_zpool_param_set,
> @@ -569,11 +571,12 @@ static __init struct zswap_pool *__zswap_pool_create_fallback(void)
>         bool has_comp, has_zpool;
>
>         has_comp = crypto_has_comp(zswap_compressor, 0, 0);
> -       if (!has_comp && strcmp(zswap_compressor, ZSWAP_COMPRESSOR_DEFAULT)) {
> +       if (!has_comp && strcmp(zswap_compressor,
> +                               CONFIG_ZSWAP_COMPRESSOR_DEFAULT)) {
>                 pr_err("compressor %s not available, using default %s\n",
> -                      zswap_compressor, ZSWAP_COMPRESSOR_DEFAULT);
> +                      zswap_compressor, CONFIG_ZSWAP_COMPRESSOR_DEFAULT);
>                 param_free_charp(&zswap_compressor);
> -               zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
> +               zswap_compressor = CONFIG_ZSWAP_COMPRESSOR_DEFAULT;
>                 has_comp = crypto_has_comp(zswap_compressor, 0, 0);
>         }
>         if (!has_comp) {
> @@ -584,11 +587,12 @@ static __init struct zswap_pool *__zswap_pool_create_fallback(void)
>         }
>
>         has_zpool = zpool_has_pool(zswap_zpool_type);
> -       if (!has_zpool && strcmp(zswap_zpool_type, ZSWAP_ZPOOL_DEFAULT)) {
> +       if (!has_zpool && strcmp(zswap_zpool_type,
> +                                CONFIG_ZSWAP_ZPOOL_DEFAULT)) {
>                 pr_err("zpool %s not available, using default %s\n",
> -                      zswap_zpool_type, ZSWAP_ZPOOL_DEFAULT);
> +                      zswap_zpool_type, CONFIG_ZSWAP_ZPOOL_DEFAULT);
>                 param_free_charp(&zswap_zpool_type);
> -               zswap_zpool_type = ZSWAP_ZPOOL_DEFAULT;
> +               zswap_zpool_type = CONFIG_ZSWAP_ZPOOL_DEFAULT;
>                 has_zpool = zpool_has_pool(zswap_zpool_type);
>         }
>         if (!has_zpool) {