Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
From:   "Huang, Ying" <ying.huang@intel.com>
To:     Yosry Ahmed <yosryahmed@google.com>
Cc:     Zhongkun He <hezhongkun.hzk@bytedance.com>,
        akpm@linux-foundation.org, hannes@cmpxchg.org, nphamcs@gmail.com,
        sjenning@redhat.com, ddstreet@ieee.org, vitaly.wool@konsulko.com,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm:zswap: fix zswap entry reclamation failure in two
 scenarios
In-Reply-To: <CAJD7tkY8SwROmNEaBAhkS4OKj33g-6fHsKFeYKW3afT+yAbvxA@mail.gmail.com>
        (Yosry Ahmed's message of "Tue, 14 Nov 2023 09:16:00 -0800")
References: <20231113130601.3350915-1-hezhongkun.hzk@bytedance.com>
        <CAJD7tkY8SwROmNEaBAhkS4OKj33g-6fHsKFeYKW3afT+yAbvxA@mail.gmail.com>
Date:   Thu, 16 Nov 2023 16:31:09 +0800
Message-ID: <87bkbudrqq.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Yosry Ahmed <yosryahmed@google.com> writes:

> +Ying
>
> On Mon, Nov 13, 2023 at 5:06=E2=80=AFAM Zhongkun He
> <hezhongkun.hzk@bytedance.com> wrote:
>>
>> I recently found two scenarios where zswap entry could not be
>> released, which will cause shrink_worker and active recycling
>> to fail.
>> 1)The swap entry has been freed, but cached in swap_slots_cache,
>> no swap cache and swapcount=3D0.
>> 2)When the option zswap_exclusive_loads_enabled disabled and
>> zswap_load completed(page in swap_cache and swapcount =3D 0).
>
> For case (1), I think a cleaner solution would be to move the
> zswap_invalidate() call from swap_range_free() (which is called after
> the cached slots are freed) to __swap_entry_free_locked() if the usage
> goes to 0. I actually think conceptually this makes not just for
> zswap_invalidate(), but also for the arch call, memcg uncharging, etc.
> Slots caching is a swapfile optimization that should be internal to
> swapfile code. Once a swap entry is freed (i.e. swap count is 0 AND
> not in the swap cache), all the hooks should be called (memcg, zswap,
> arch, ..) as the swap entry is effectively freed. The fact that
> swapfile code internally batches and caches slots should be
> transparent to other parts of MM. I am not sure if the calls can just
> be moved or if there are underlying assumptions in the implementation
> that would be broken, but it feels like the right thing to do.

This sounds reasonable for me.  Just don't forget to change other
swap_range_free() callers too.

--
Best Regards,
Huang, Ying