Re: [PATCH 13/20] mm: zswap: function ordering: pool params

[Nhat Pham]

On Mon, Jan 29, 2024 at 5:42 PM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> The parameters primarily control pool attributes. Move those
> operations up to the pool section.
>
> Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> ---
>  mm/zswap.c | 312 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 156 insertions(+), 156 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 168afd6767b3..e650fc587116 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -590,6 +590,162 @@ static struct zswap_pool *zswap_pool_find_get(char *type, char *compressor)
>         return NULL;
>  }
>
> +/*********************************
> +* param callbacks
> +**********************************/
> +
> +static bool zswap_pool_changed(const char *s, const struct kernel_param *kp)
> +{
> +       /* no change required */
> +       if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
> +               return false;
> +       return true;
> +}
> +
> +/* val must be a null-terminated string */
> +static int __zswap_param_set(const char *val, const struct kernel_param *kp,
> +                            char *type, char *compressor)
> +{
> +       struct zswap_pool *pool, *put_pool = NULL;
> +       char *s = strstrip((char *)val);
> +       int ret = 0;
> +       bool new_pool = false;
> +
> +       mutex_lock(&zswap_init_lock);
> +       switch (zswap_init_state) {
> +       case ZSWAP_UNINIT:
> +               /* if this is load-time (pre-init) param setting,
> +                * don't create a pool; that's done during init.
> +                */
> +               ret = param_set_charp(s, kp);
> +               break;
> +       case ZSWAP_INIT_SUCCEED:
> +               new_pool = zswap_pool_changed(s, kp);
> +               break;
> +       case ZSWAP_INIT_FAILED:
> +               pr_err("can't set param, initialization failed\n");
> +               ret = -ENODEV;
> +       }
> +       mutex_unlock(&zswap_init_lock);
> +
> +       /* no need to create a new pool, return directly */
> +       if (!new_pool)
> +               return ret;
> +
> +       if (!type) {
> +               if (!zpool_has_pool(s)) {
> +                       pr_err("zpool %s not available\n", s);
> +                       return -ENOENT;
> +               }
> +               type = s;
> +       } else if (!compressor) {
> +               if (!crypto_has_acomp(s, 0, 0)) {
> +                       pr_err("compressor %s not available\n", s);
> +                       return -ENOENT;
> +               }
> +               compressor = s;
> +       } else {
> +               WARN_ON(1);
> +               return -EINVAL;
> +       }
> +
> +       spin_lock(&zswap_pools_lock);
> +
> +       pool = zswap_pool_find_get(type, compressor);
> +       if (pool) {
> +               zswap_pool_debug("using existing", pool);
> +               WARN_ON(pool == zswap_pool_current());
> +               list_del_rcu(&pool->list);
> +       }
> +
> +       spin_unlock(&zswap_pools_lock);
> +
> +       if (!pool)
> +               pool = zswap_pool_create(type, compressor);
> +
> +       if (pool)
> +               ret = param_set_charp(s, kp);
> +       else
> +               ret = -EINVAL;
> +
> +       spin_lock(&zswap_pools_lock);
> +
> +       if (!ret) {
> +               put_pool = zswap_pool_current();
> +               list_add_rcu(&pool->list, &zswap_pools);
> +               zswap_has_pool = true;
> +       } else if (pool) {
> +               /* add the possibly pre-existing pool to the end of the pools
> +                * list; if it's new (and empty) then it'll be removed and
> +                * destroyed by the put after we drop the lock
> +                */
> +               list_add_tail_rcu(&pool->list, &zswap_pools);
> +               put_pool = pool;
> +       }
> +
> +       spin_unlock(&zswap_pools_lock);
> +
> +       if (!zswap_has_pool && !pool) {
> +               /* if initial pool creation failed, and this pool creation also
> +                * failed, maybe both compressor and zpool params were bad.
> +                * Allow changing this param, so pool creation will succeed
> +                * when the other param is changed. We already verified this
> +                * param is ok in the zpool_has_pool() or crypto_has_acomp()
> +                * checks above.
> +                */
> +               ret = param_set_charp(s, kp);
> +       }
> +
> +       /* drop the ref from either the old current pool,
> +        * or the new pool we failed to add
> +        */
> +       if (put_pool)
> +               zswap_pool_put(put_pool);
> +
> +       return ret;
> +}
> +
> +static int zswap_compressor_param_set(const char *val,
> +                                     const struct kernel_param *kp)
> +{
> +       return __zswap_param_set(val, kp, zswap_zpool_type, NULL);
> +}
> +
> +static int zswap_zpool_param_set(const char *val,
> +                                const struct kernel_param *kp)
> +{
> +       return __zswap_param_set(val, kp, NULL, zswap_compressor);
> +}
> +
> +static int zswap_enabled_param_set(const char *val,
> +                                  const struct kernel_param *kp)
> +{
> +       int ret = -ENODEV;
> +
> +       /* if this is load-time (pre-init) param setting, only set param. */
> +       if (system_state != SYSTEM_RUNNING)
> +               return param_set_bool(val, kp);
> +
> +       mutex_lock(&zswap_init_lock);
> +       switch (zswap_init_state) {
> +       case ZSWAP_UNINIT:
> +               if (zswap_setup())
> +                       break;
> +               fallthrough;
> +       case ZSWAP_INIT_SUCCEED:
> +               if (!zswap_has_pool)
> +                       pr_err("can't enable, no pool configured\n");
> +               else
> +                       ret = param_set_bool(val, kp);
> +               break;
> +       case ZSWAP_INIT_FAILED:
> +               pr_err("can't enable, initialization failed\n");
> +       }
> +       mutex_unlock(&zswap_init_lock);
> +
> +       return ret;
> +}
> +
>  /* should be called under RCU */
>  #ifdef CONFIG_MEMCG
>  static inline struct mem_cgroup *mem_cgroup_from_entry(struct zswap_entry *entry)
> @@ -1163,162 +1319,6 @@ static void shrink_worker(struct work_struct *w)
>         zswap_pool_put(pool);
>  }
>
> -/*********************************
> -* param callbacks
> -**********************************/
> -
> -static bool zswap_pool_changed(const char *s, const struct kernel_param *kp)
> -{
> -       /* no change required */
> -       if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
> -               return false;
> -       return true;
> -}
> -
> -/* val must be a null-terminated string */
> -static int __zswap_param_set(const char *val, const struct kernel_param *kp,
> -                            char *type, char *compressor)
> -{
> -       struct zswap_pool *pool, *put_pool = NULL;
> -       char *s = strstrip((char *)val);
> -       int ret = 0;
> -       bool new_pool = false;
> -
> -       mutex_lock(&zswap_init_lock);
> -       switch (zswap_init_state) {
> -       case ZSWAP_UNINIT:
> -               /* if this is load-time (pre-init) param setting,
> -                * don't create a pool; that's done during init.
> -                */
> -               ret = param_set_charp(s, kp);
> -               break;
> -       case ZSWAP_INIT_SUCCEED:
> -               new_pool = zswap_pool_changed(s, kp);
> -               break;
> -       case ZSWAP_INIT_FAILED:
> -               pr_err("can't set param, initialization failed\n");
> -               ret = -ENODEV;
> -       }
> -       mutex_unlock(&zswap_init_lock);
> -
> -       /* no need to create a new pool, return directly */
> -       if (!new_pool)
> -               return ret;
> -
> -       if (!type) {
> -               if (!zpool_has_pool(s)) {
> -                       pr_err("zpool %s not available\n", s);
> -                       return -ENOENT;
> -               }
> -               type = s;
> -       } else if (!compressor) {
> -               if (!crypto_has_acomp(s, 0, 0)) {
> -                       pr_err("compressor %s not available\n", s);
> -                       return -ENOENT;
> -               }
> -               compressor = s;
> -       } else {
> -               WARN_ON(1);
> -               return -EINVAL;
> -       }
> -
> -       spin_lock(&zswap_pools_lock);
> -
> -       pool = zswap_pool_find_get(type, compressor);
> -       if (pool) {
> -               zswap_pool_debug("using existing", pool);
> -               WARN_ON(pool == zswap_pool_current());
> -               list_del_rcu(&pool->list);
> -       }
> -
> -       spin_unlock(&zswap_pools_lock);
> -
> -       if (!pool)
> -               pool = zswap_pool_create(type, compressor);
> -
> -       if (pool)
> -               ret = param_set_charp(s, kp);
> -       else
> -               ret = -EINVAL;
> -
> -       spin_lock(&zswap_pools_lock);
> -
> -       if (!ret) {
> -               put_pool = zswap_pool_current();
> -               list_add_rcu(&pool->list, &zswap_pools);
> -               zswap_has_pool = true;
> -       } else if (pool) {
> -               /* add the possibly pre-existing pool to the end of the pools
> -                * list; if it's new (and empty) then it'll be removed and
> -                * destroyed by the put after we drop the lock
> -                */
> -               list_add_tail_rcu(&pool->list, &zswap_pools);
> -               put_pool = pool;
> -       }
> -
> -       spin_unlock(&zswap_pools_lock);
> -
> -       if (!zswap_has_pool && !pool) {
> -               /* if initial pool creation failed, and this pool creation also
> -                * failed, maybe both compressor and zpool params were bad.
> -                * Allow changing this param, so pool creation will succeed
> -                * when the other param is changed. We already verified this
> -                * param is ok in the zpool_has_pool() or crypto_has_acomp()
> -                * checks above.
> -                */
> -               ret = param_set_charp(s, kp);
> -       }
> -
> -       /* drop the ref from either the old current pool,
> -        * or the new pool we failed to add
> -        */
> -       if (put_pool)
> -               zswap_pool_put(put_pool);
> -
> -       return ret;
> -}
> -
> -static int zswap_compressor_param_set(const char *val,
> -                                     const struct kernel_param *kp)
> -{
> -       return __zswap_param_set(val, kp, zswap_zpool_type, NULL);
> -}
> -
> -static int zswap_zpool_param_set(const char *val,
> -                                const struct kernel_param *kp)
> -{
> -       return __zswap_param_set(val, kp, NULL, zswap_compressor);
> -}
> -
> -static int zswap_enabled_param_set(const char *val,
> -                                  const struct kernel_param *kp)
> -{
> -       int ret = -ENODEV;
> -
> -       /* if this is load-time (pre-init) param setting, only set param. */
> -       if (system_state != SYSTEM_RUNNING)
> -               return param_set_bool(val, kp);
> -
> -       mutex_lock(&zswap_init_lock);
> -       switch (zswap_init_state) {
> -       case ZSWAP_UNINIT:
> -               if (zswap_setup())
> -                       break;
> -               fallthrough;
> -       case ZSWAP_INIT_SUCCEED:
> -               if (!zswap_has_pool)
> -                       pr_err("can't enable, no pool configured\n");
> -               else
> -                       ret = param_set_bool(val, kp);
> -               break;
> -       case ZSWAP_INIT_FAILED:
> -               pr_err("can't enable, initialization failed\n");
> -       }
> -       mutex_unlock(&zswap_init_lock);
> -
> -       return ret;
> -}
> -
>  static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
>  {
>         struct crypto_acomp_ctx *acomp_ctx;
> --
> 2.43.0
>

Reviewed-by: Nhat Pham <nphamcs@xxxxxxxxx>