Received-SPF: pass (google.com: domain of linux-kernel+bounces-50667-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240201-b4-zswap-invalidate-entry-v1-0-56ed496b6e55@bytedance.com>
 <20240201-b4-zswap-invalidate-entry-v1-4-56ed496b6e55@bytedance.com>
In-Reply-To: <20240201-b4-zswap-invalidate-entry-v1-4-56ed496b6e55@bytedance.com>
From: Nhat Pham <nphamcs@gmail.com>
Date: Fri, 2 Feb 2024 14:28:54 -0800
Message-ID: <CAKEwX=Pj9ms56zPaFznXrY0mF7E_q1hMXUkV-Fhnx5V17OY6og@mail.gmail.com>
Subject: Re: [PATCH 4/6] mm/zswap: remove duplicate_entry debug value
To: Chengming Zhou <zhouchengming@bytedance.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>, Andrew Morton <akpm@linux-foundation.org>, 
	Yosry Ahmed <yosryahmed@google.com>, linux-kernel@vger.kernel.org, linux-mm@kvack.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 1, 2024 at 7:50=E2=80=AFAM Chengming Zhou
<zhouchengming@bytedance.com> wrote:
>
> cat /sys/kernel/debug/zswap/duplicate_entry
> 2086447
>
> When testing, the duplicate_entry value is very high, but no warning
> message in the kernel log. From the comment of duplicate_entry
> "Duplicate store was encountered (rare)", it seems something goes wrong.
>
> Actually it's incremented in the beginning of zswap_store(), which found
> its zswap entry has already on the tree. And this is a normal case,
> since the folio could leave zswap entry on the tree after swapin,
> later it's dirtied and swapout/zswap_store again, found its original
> zswap entry. (Maybe we can reuse it instead of invalidating it?)

Interesting. So if we make invalidate load the only mode, this oddity
is gone as well?

>
> So duplicate_entry should be only incremented in the real bug case,
> which already have "WARN_ON(1)", it looks redundant to count bug case,
> so this patch just remove it.

But yeah, I have literally never checked this value (maybe I should
ha). I'm fine with removing it, unless someone has a strong case for
this counter?

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

>
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
>  mm/zswap.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 4381b7a2d4d6..3fbb7e2c8b8d 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -71,8 +71,6 @@ static u64 zswap_reject_compress_poor;
>  static u64 zswap_reject_alloc_fail;
>  /* Store failed because the entry metadata could not be allocated (rare)=
 */
>  static u64 zswap_reject_kmemcache_fail;
> -/* Duplicate store was encountered (rare) */
> -static u64 zswap_duplicate_entry;
>
>  /* Shrinker work queue */
>  static struct workqueue_struct *shrink_wq;
> @@ -1571,10 +1569,8 @@ bool zswap_store(struct folio *folio)
>          */
>         spin_lock(&tree->lock);
>         entry =3D zswap_rb_search(&tree->rbroot, offset);
> -       if (entry) {
> +       if (entry)
>                 zswap_invalidate_entry(tree, entry);
> -               zswap_duplicate_entry++;
> -       }
>         spin_unlock(&tree->lock);
>         objcg =3D get_obj_cgroup_from_folio(folio);
>         if (objcg && !obj_cgroup_may_zswap(objcg)) {
> @@ -1661,7 +1657,6 @@ bool zswap_store(struct folio *folio)
>          */
>         while (zswap_rb_insert(&tree->rbroot, entry, &dupentry) =3D=3D -E=
EXIST) {
>                 WARN_ON(1);
> -               zswap_duplicate_entry++;
>                 zswap_invalidate_entry(tree, dupentry);
>         }
>         if (entry->length) {
> @@ -1822,8 +1817,6 @@ static int zswap_debugfs_init(void)
>                            zswap_debugfs_root, &zswap_reject_compress_poo=
r);
>         debugfs_create_u64("written_back_pages", 0444,
>                            zswap_debugfs_root, &zswap_written_back_pages)=
;
> -       debugfs_create_u64("duplicate_entry", 0444,
> -                          zswap_debugfs_root, &zswap_duplicate_entry);
>         debugfs_create_u64("pool_total_size", 0444,
>                            zswap_debugfs_root, &zswap_pool_total_size);
>         debugfs_create_atomic_t("stored_pages", 0444,
>
> --
> b4 0.10.1