Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220827104600.1813214-1-liushixin2@huawei.com> <20220827104600.1813214-4-liushixin2@huawei.com>
In-Reply-To: <20220827104600.1813214-4-liushixin2@huawei.com>
From:   Vitaly Wool <vitaly.wool@konsulko.com>
Date:   Sun, 28 Aug 2022 22:46:02 +0200
Message-ID: <CAM4kBBKYRkr6OSQ3dHcAKTSdVgsx4GKFanyBpr1jX4Yg1objBw@mail.gmail.com>
Subject: Re: [PATCH -next v3 3/5] mm/zswap: replace zswap_init_{started/failed}
 with zswap_init_state
To:     Liu Shixin <liushixin2@huawei.com>
Cc:     Seth Jennings <sjenning@redhat.com>,
        Dan Streetman <ddstreet@ieee.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Christoph Hellwig <hch@lst.de>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Kefeng Wang <wangkefeng.wang@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Aug 27, 2022 at 12:12 PM Liu Shixin <liushixin2@huawei.com> wrote:
>
> zswap_init_started indicates that the initialization is started. And
> zswap_init_failed indicates that the initialization is failed. As we will
> support to init zswap after system startup, it's necessary to add a state
> to indicate the initialization is complete and succeed to avoid
> concurrency issues. Since we don't care about the difference between
> init started with init completion. We only need three states:
> uninitialized, initial failed, initial succeed.
>
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>

Reviewed-by: Vitaly Wool <vitaly.wool@konulko.com>

> ---
>  mm/zswap.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 2d48fd59cc7a..84e38300f571 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -214,11 +214,12 @@ static DEFINE_SPINLOCK(zswap_pools_lock);
>  /* pool counter to provide unique names to zpool */
>  static atomic_t zswap_pools_count = ATOMIC_INIT(0);
>
> -/* used by param callback function */
> -static bool zswap_init_started;
> +#define ZSWAP_UNINIT           0
> +#define ZSWAP_INIT_SUCCEED     1
> +#define ZSWAP_INIT_FAILED      2
>
> -/* fatal error during init */
> -static bool zswap_init_failed;
> +/* init state */
> +static int zswap_init_state;
>
>  /* init completed, but couldn't create the initial pool */
>  static bool zswap_has_pool;
> @@ -772,7 +773,7 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
>         char *s = strstrip((char *)val);
>         int ret;
>
> -       if (zswap_init_failed) {
> +       if (zswap_init_state == ZSWAP_INIT_FAILED) {
>                 pr_err("can't set param, initialization failed\n");
>                 return -ENODEV;
>         }
> @@ -784,7 +785,7 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
>         /* if this is load-time (pre-init) param setting,
>          * don't create a pool; that's done during init.
>          */
> -       if (!zswap_init_started)
> +       if (zswap_init_state == ZSWAP_UNINIT)
>                 return param_set_charp(s, kp);
>
>         if (!type) {
> @@ -875,11 +876,11 @@ static int zswap_zpool_param_set(const char *val,
>  static int zswap_enabled_param_set(const char *val,
>                                    const struct kernel_param *kp)
>  {
> -       if (zswap_init_failed) {
> +       if (zswap_init_state == ZSWAP_INIT_FAILED) {
>                 pr_err("can't enable, initialization failed\n");
>                 return -ENODEV;
>         }
> -       if (!zswap_has_pool && zswap_init_started) {
> +       if (!zswap_has_pool && zswap_init_state == ZSWAP_INIT_SUCCEED) {
>                 pr_err("can't enable, no pool configured\n");
>                 return -ENODEV;
>         }
> @@ -1476,8 +1477,6 @@ static int __init init_zswap(void)
>         struct zswap_pool *pool;
>         int ret;
>
> -       zswap_init_started = true;
> -
>         if (zswap_entry_cache_create()) {
>                 pr_err("entry cache creation failed\n");
>                 goto cache_fail;
> @@ -1517,6 +1516,7 @@ static int __init init_zswap(void)
>                 goto destroy_wq;
>         if (zswap_debugfs_init())
>                 pr_warn("debugfs initialization failed\n");
> +       zswap_init_state = ZSWAP_INIT_SUCCEED;
>         return 0;
>
>  destroy_wq:
> @@ -1530,7 +1530,7 @@ static int __init init_zswap(void)
>         zswap_entry_cache_destroy();
>  cache_fail:
>         /* if built-in, we aren't unloaded on failure; don't allow use */
> -       zswap_init_failed = true;
> +       zswap_init_state = ZSWAP_INIT_FAILED;
>         zswap_enabled = false;
>         return -ENOMEM;
>  }
> --
> 2.25.1
>