From: Vitaly Wool <vitalywool@gmail.com>
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>
Subject: Re: [PATCH v4] zswap: allow setting default status, compressor and allocator in Kconfig
Date: Mon, 18 Nov 2019 13:43:58 +0100 [thread overview]
Message-ID: <CAMJBoFNja-7sY=mU4hWDOqvOhZjvSNqyL1hYqJB+Hm1dygxpCA@mail.gmail.com> (raw)
In-Reply-To: <20191117000544.1019556-1-mail@maciej.szmigiero.name>
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) {
next prev parent reply other threads:[~2019-11-18 12:44 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-17 0:05 [PATCH v4] zswap: allow setting default status, compressor and allocator in Kconfig Maciej S. Szmigiero
2019-11-18 12:43 ` Vitaly Wool [this message]
2019-11-18 23:03 ` Maciej S. Szmigiero
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMJBoFNja-7sY=mU4hWDOqvOhZjvSNqyL1hYqJB+Hm1dygxpCA@mail.gmail.com' \
--to=vitalywool@gmail.com \
--cc=ddstreet@ieee.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mail@maciej.szmigiero.name \
--cc=sjenning@redhat.com \
--cc=vbabka@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).