Received-SPF: pass (google.com: domain of linux-kernel+bounces-45330-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240130014208.565554-1-hannes@cmpxchg.org> <20240130014208.565554-14-hannes@cmpxchg.org>
In-Reply-To: <20240130014208.565554-14-hannes@cmpxchg.org>
From: Nhat Pham <nphamcs@gmail.com>
Date: Tue, 30 Jan 2024 13:22:15 -0800
Message-ID: <CAKEwX=NgCmqpxQOENfv+8CPzQLnwv=1A38GJROSXFdC3CEOPFw@mail.gmail.com>
Subject: Re: [PATCH 13/20] mm: zswap: function ordering: pool params
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Andrew Morton <akpm@linux-foundation.org>, Yosry Ahmed <yosryahmed@google.com>, 
	Chengming Zhou <zhouchengming@bytedance.com>, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 29, 2024 at 5:42=E2=80=AFPM Johannes Weiner <hannes@cmpxchg.org=
> wrote:
>
> The parameters primarily control pool attributes. Move those
> operations up to the pool section.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  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 =3D NULL;
> +       char *s =3D strstrip((char *)val);
> +       int ret =3D 0;
> +       bool new_pool =3D 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 =3D param_set_charp(s, kp);
> +               break;
> +       case ZSWAP_INIT_SUCCEED:
> +               new_pool =3D zswap_pool_changed(s, kp);
> +               break;
> +       case ZSWAP_INIT_FAILED:
> +               pr_err("can't set param, initialization failed\n");
> +               ret =3D -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 =3D s;
> +       } else if (!compressor) {
> +               if (!crypto_has_acomp(s, 0, 0)) {
> +                       pr_err("compressor %s not available\n", s);
> +                       return -ENOENT;
> +               }
> +               compressor =3D s;
> +       } else {
> +               WARN_ON(1);
> +               return -EINVAL;
> +       }
> +
> +       spin_lock(&zswap_pools_lock);
> +
> +       pool =3D zswap_pool_find_get(type, compressor);
> +       if (pool) {
> +               zswap_pool_debug("using existing", pool);
> +               WARN_ON(pool =3D=3D zswap_pool_current());
> +               list_del_rcu(&pool->list);
> +       }
> +
> +       spin_unlock(&zswap_pools_lock);
> +
> +       if (!pool)
> +               pool =3D zswap_pool_create(type, compressor);
> +
> +       if (pool)
> +               ret =3D param_set_charp(s, kp);
> +       else
> +               ret =3D -EINVAL;
> +
> +       spin_lock(&zswap_pools_lock);
> +
> +       if (!ret) {
> +               put_pool =3D zswap_pool_current();
> +               list_add_rcu(&pool->list, &zswap_pools);
> +               zswap_has_pool =3D true;
> +       } else if (pool) {
> +               /* add the possibly pre-existing pool to the end of the p=
ools
> +                * list; if it's new (and empty) then it'll be removed an=
d
> +                * destroyed by the put after we drop the lock
> +                */
> +               list_add_tail_rcu(&pool->list, &zswap_pools);
> +               put_pool =3D pool;
> +       }
> +
> +       spin_unlock(&zswap_pools_lock);
> +
> +       if (!zswap_has_pool && !pool) {
> +               /* if initial pool creation failed, and this pool creatio=
n also
> +                * failed, maybe both compressor and zpool params were ba=
d.
> +                * Allow changing this param, so pool creation will succe=
ed
> +                * when the other param is changed. We already verified t=
his
> +                * param is ok in the zpool_has_pool() or crypto_has_acom=
p()
> +                * checks above.
> +                */
> +               ret =3D 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 =3D -ENODEV;
> +
> +       /* if this is load-time (pre-init) param setting, only set param.=
 */
> +       if (system_state !=3D 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 =3D 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_entr=
y *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 =3D NULL;
> -       char *s =3D strstrip((char *)val);
> -       int ret =3D 0;
> -       bool new_pool =3D 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 =3D param_set_charp(s, kp);
> -               break;
> -       case ZSWAP_INIT_SUCCEED:
> -               new_pool =3D zswap_pool_changed(s, kp);
> -               break;
> -       case ZSWAP_INIT_FAILED:
> -               pr_err("can't set param, initialization failed\n");
> -               ret =3D -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 =3D s;
> -       } else if (!compressor) {
> -               if (!crypto_has_acomp(s, 0, 0)) {
> -                       pr_err("compressor %s not available\n", s);
> -                       return -ENOENT;
> -               }
> -               compressor =3D s;
> -       } else {
> -               WARN_ON(1);
> -               return -EINVAL;
> -       }
> -
> -       spin_lock(&zswap_pools_lock);
> -
> -       pool =3D zswap_pool_find_get(type, compressor);
> -       if (pool) {
> -               zswap_pool_debug("using existing", pool);
> -               WARN_ON(pool =3D=3D zswap_pool_current());
> -               list_del_rcu(&pool->list);
> -       }
> -
> -       spin_unlock(&zswap_pools_lock);
> -
> -       if (!pool)
> -               pool =3D zswap_pool_create(type, compressor);
> -
> -       if (pool)
> -               ret =3D param_set_charp(s, kp);
> -       else
> -               ret =3D -EINVAL;
> -
> -       spin_lock(&zswap_pools_lock);
> -
> -       if (!ret) {
> -               put_pool =3D zswap_pool_current();
> -               list_add_rcu(&pool->list, &zswap_pools);
> -               zswap_has_pool =3D true;
> -       } else if (pool) {
> -               /* add the possibly pre-existing pool to the end of the p=
ools
> -                * list; if it's new (and empty) then it'll be removed an=
d
> -                * destroyed by the put after we drop the lock
> -                */
> -               list_add_tail_rcu(&pool->list, &zswap_pools);
> -               put_pool =3D pool;
> -       }
> -
> -       spin_unlock(&zswap_pools_lock);
> -
> -       if (!zswap_has_pool && !pool) {
> -               /* if initial pool creation failed, and this pool creatio=
n also
> -                * failed, maybe both compressor and zpool params were ba=
d.
> -                * Allow changing this param, so pool creation will succe=
ed
> -                * when the other param is changed. We already verified t=
his
> -                * param is ok in the zpool_has_pool() or crypto_has_acom=
p()
> -                * checks above.
> -                */
> -               ret =3D 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 =3D -ENODEV;
> -
> -       /* if this is load-time (pre-init) param setting, only set param.=
 */
> -       if (system_state !=3D 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 =3D 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 *entr=
y)
>  {
>         struct crypto_acomp_ctx *acomp_ctx;
> --
> 2.43.0
>

Reviewed-by: Nhat Pham <nphamcs@gmail.com>