On Fri, Oct 18, 2019 at 6:16 PM Maciej S. Szmigiero
<mail@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On 18.10.2019 13:19, Dan Streetman wrote:
> > On Fri, Oct 11, 2019 at 7:40 PM Maciej S. Szmigiero
> > <mail@xxxxxxxxxxxxxxxxxxxxx> 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.
> >
> > Who is the target for using these kernel build-time defaults? I don't
> > think any distribution would be defaulting zswap to enabled, and if
> > the config defaults are intended for personal kernel builds, it is
> > really so much harder to just configure it on the boot cmdline?
>
> Appropriate kernel config defaults are normally provided for kernel
> parameters that are reasonably expected to be configured for normal
> operation (and sometimes for debugging parameters, too), so one does not
> need to boot kernel with a lot of command line parameters to get the
> desired behavior.
>
> A quick, limited grep of Documentation/admin-guide/kernel-parameters.txt
> gives me the following config options that control the default values of
> command line parameters:
> CONFIG_HPET_MMAP_DEFAULT
> CONFIG_BOOTPARAM_HUNG_TASK_PANIC
> CONFIG_INIT_ON_ALLOC_DEFAULT_ON
> CONFIG_INIT_ON_FREE_DEFAULT_ON
> CONFIG_IOMMU_DEFAULT_PASSTHROUGH
> CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
> CONFIG_LSM
> CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
> CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT
> CONFIG_RANDOM_TRUST_CPU
> CONFIG_SLUB_MEMCG_SYSFS_ON
> CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC
> CONFIG_SYSFS_DEPRECATED_V2
> CONFIG_COMPAT_VDSO
> CONFIG_WQ_POWER_EFFICIENT_DEFAULT
> CONFIG_XEN_SCRUB_PAGES_DEFAULT
>
> There are countless other ones that set the default values for module
> parameters, for example in the "sound" directory alone there are four:
> CONFIG_SND_PS3_DEFAULT_START_DELAY
> CONFIG_SND_HDA_POWER_SAVE_DEFAULT
> CONFIG_SND_AC97_POWER_SAVE_DEFAULT
> CONFIG_SND_SEQ_HRTIMER_DEFAULT
>
> Providing such selectable default also means that the dependencies for
> the selected compressor algorithm and pool allocator are pulled in
> automatically - the old way pulled in the LZO algorithm (and only the
> LZO algorithm) into the kernel unconditionally, even if the user was
> ultimately going to use another algorithm.
>
> >>
> >> The default values for these options match what the code was using
> >> previously as its defaults.
> >>
> >> Signed-off-by: Maciej S. Szmigiero <mail@xxxxxxxxxxxxxxxxxxxxx>
> >> ---
> >> mm/Kconfig | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >> mm/zswap.c | 26 ++++++++------
> >> 2 files changed, 117 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/mm/Kconfig b/mm/Kconfig
> >> index a5dae9a7eb51..4309bcaaa29d 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
> >
> > Using choice becomes a bit of a maintenence issue...if we add this,
> > wouldn't it be better to use string input so new compression algs can
> > be added without having to update this Kconfig?
>
> If this is changed to a string prompt we'll lose automatic pulling in of
> an appropriate CONFIG_CRYPTO_* dependency.
>
> Other similar options are presented as a choice, too, see for example
> CONFIG_KERNEL_{GZIP,BZIP2,LZMA,XZ,LZO,LZ4,UNCOMPRESSED} and
> CONFIG_INITRAMFS_COMPRESSION_{NONE,GZIP,BZIP2,LZMA,XZ,LZO,LZ4} (that
> maps to an extension string table called CONFIG_INITRAMFS_COMPRESSION).
>
> BTW: New compression algorithm being added to the kernel is a rare
> event, I also envision that not every new algorithm will need to be
> presented as a choice for zswap default.
>
> >> + prompt "Compressed cache for swap pages default compressor"
> >> + depends on ZSWAP
> >> + default ZSWAP_DEFAULT_COMP_LZO
> >> + help
> >> + Selects the default compression algorithm for the compressed cache
> >> + for swap pages.
> >> + If in doubt, select 'LZO'.
> >> +
> >> + The selection made here can be overridden by using the kernel
> >> + command line 'zswap.compressor=' option.
> >> +
> >> +config ZSWAP_DEFAULT_COMP_DEFLATE
> >> + bool "Deflate"
> >> + select CRYPTO_DEFLATE
> >> + help
> >> + Use the Deflate algorithm as the default compression algorithm.
> >> +
> >> +config ZSWAP_DEFAULT_COMP_LZO
> >> + bool "LZO"
> >> + select CRYPTO_LZO
> >> + help
> >> + Use the LZO algorithm as the default compression algorithm.
> >> +
> >> +config ZSWAP_DEFAULT_COMP_842
> >> + bool "842"
> >> + select CRYPTO_842
> >> + help
> >> + Use the 842 algorithm as the default compression algorithm.
> >> +
> >> +config ZSWAP_DEFAULT_COMP_LZ4
> >> + bool "LZ4"
> >> + select CRYPTO_LZ4
> >> + help
> >> + Use the LZ4 algorithm as the default compression algorithm.
> >> +
> >> +config ZSWAP_DEFAULT_COMP_LZ4HC
> >> + bool "LZ4HC"
> >> + select CRYPTO_LZ4HC
> >> + help
> >> + Use the LZ4HC algorithm as the default compression algorithm.
> >> +
> >> +config ZSWAP_DEFAULT_COMP_ZSTD
> >> + bool "zstd"
> >> + select CRYPTO_ZSTD
> >> + help
> >> + Use the zstd algorithm as the default compression algorithm.
> >> +endchoice
> >> +
> >> +config ZSWAP_DEFAULT_COMP_NAME
> >> + string
> >> + default "deflate" if ZSWAP_DEFAULT_COMP_DEFLATE
> >> + default "lzo" if ZSWAP_DEFAULT_COMP_LZO
> >> + default "842" if ZSWAP_DEFAULT_COMP_842
> >> + default "lz4" if ZSWAP_DEFAULT_COMP_LZ4
> >> + default "lz4hc" if ZSWAP_DEFAULT_COMP_LZ4HC
> >> + default "zstd" if ZSWAP_DEFAULT_COMP_ZSTD
> >> + default ""
> >> +
> >> +choice
> >> + prompt "Compressed cache for swap pages default allocator"
> >> + depends on ZSWAP
> >> + default ZSWAP_DEFAULT_ZPOOL_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_DEFAULT_ZPOOL_ZBUD
> >> + bool "zbud"
> >> + select ZBUD
> >> + help
> >> + Use the zbud allocator as the default allocator.
> >> +
> >> +config ZSWAP_DEFAULT_ZPOOL_Z3FOLD
> >> + bool "z3fold"
> >> + select Z3FOLD
> >> + help
> >> + Use the z3fold allocator as the default allocator.
> >> +endchoice
> >> +
> >> +config ZSWAP_DEFAULT_ZPOOL_NAME
> >> + string
> >> + default "zbud" if ZSWAP_DEFAULT_ZPOOL_ZBUD
> >> + default "z3fold" if ZSWAP_DEFAULT_ZPOOL_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..59231f6fb2ca 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
> >> 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_DEFAULT_COMP_NAME;
> >> 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_DEFAULT_ZPOOL_NAME;
> >> 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_DEFAULT_COMP_NAME)) {
> >
> > bit of bikeshedding, wouldn't CONFIG_ZSWAP_COMPRESSOR_DEFAULT be
> > clearer than CONFIG_ZSWAP_DEFAULT_COMP_NAME?
>
> I'm fine either way (used "COMP" instead of "COMPRESSOR" to shorten these
> identifiers a bit), also CONFIG_ZSWAP_DEFAULT_ZPOOL_NAME then probably
> would need renaming to CONFIG_ZSWAP_ZPOOL_DEFAULT for consistency.
The "NAME" part doesn't add any clarity, while shortening COMPRESSOR
to COMP removes clarity.
Other than that I'm ok with all this. Thanks!
>
> Thanks,
> Maciej
