Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
MIME-Version: 1.0
References: <20231113130601.3350915-1-hezhongkun.hzk@bytedance.com>
 <CAKEwX=NhiGyoqR3HaDCuarqGmM5CCMYDatvVEEtVKzrL8DdDbw@mail.gmail.com>
 <CACSyD1NyynLrrkrp-A8tGW5gUJeFQiYk_E_SO14m_7wqrddtiQ@mail.gmail.com> <CAKEwX=MinEqt_gR+hvyYc1dgU68B1Auw8fypRo7SKFjFK7VHaA@mail.gmail.com>
In-Reply-To: <CAKEwX=MinEqt_gR+hvyYc1dgU68B1Auw8fypRo7SKFjFK7VHaA@mail.gmail.com>
From:   =?UTF-8?B?6LS65Lit5Z2k?= <hezhongkun.hzk@bytedance.com>
Date:   Wed, 15 Nov 2023 20:12:01 +0800
Message-ID: <CACSyD1Ou-CuqXfpNtjj=oypYP47natsJtYd8iZ38hVuncfuT_g@mail.gmail.com>
Subject: Re: [External] Re: [PATCH] mm:zswap: fix zswap entry reclamation
 failure in two scenarios
To:     Nhat Pham <nphamcs@gmail.com>
Cc:     akpm@linux-foundation.org, hannes@cmpxchg.org,
        yosryahmed@google.com, sjenning@redhat.com, ddstreet@ieee.org,
        vitaly.wool@konsulko.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

>
> Ah my bad, I should have been clearer.
>
> I was looking at the zswap shrinker patch series (specifically the
> cgroup-aware LRU patch), which moves the counter update out of
> zswap_writeback_entry. If we apply that patch on top of that series, we will
> screw up the counter. Should be easily fixable anyway though.

Got it.

>
> Ah I think I understand the point of the patch a bit better now.
>
> Essentially, we're invalidating these entries, which does reclaim the
> memory used for these compressed objects, but there is no IO involved.
> Writeback-less shrinking, if you will.
>
> This will still screw up one of the heuristics I'm using for the zswap
> shrinker a bit, but that should be easily fixable with some plumbing.
> Same goes for the writeback counter - but depending on the order in
> which Andrew apply the patches, you might have to resolve the conflicts
> there :)

OK,  I will fix it.

>
> Other than this objection, I think this optimization makes sense to me:
>
> In the first case, we already freed the swap entry. Might as well also
> dropped the zswap entry.
>
> In the second case, we already have another copy in memory, so
> dropping the compressed copy to make space for warmer objects
> coming into zswap makes sense to me. Might be worth doing a
> micro-benchmark to verify this intuition, but I agree that it's more
> important to maintain the LRU ordering than any CPU saving from
> skipping re-compression.
>
> I would suggest that you should expand on this on the commit log
> to make clearer the motivation behind this optimization, if you were
> to re-submit this patch for some reason (for e.g to resolve the
> aforementioned conflicts with the zswap shrinker series).
>
> But otherwise, LGTM!
>
> Feel free to add the following tag:
> Reviewed-by: Nhat Pham <nphamcs@gmail.com>

Thanks, there are still some commits from Yosry,
after that, I will send it again.