Received-SPF: pass (google.com: domain of linux-kernel+bounces-25584-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: "Huang, Ying" <ying.huang@intel.com>
To: Kairui Song <ryncsn@gmail.com>
Cc: linux-mm@kvack.org,  Andrew Morton <akpm@linux-foundation.org>,  Chris
 Li <chrisl@kernel.org>,  Hugh Dickins <hughd@google.com>,  Johannes Weiner
 <hannes@cmpxchg.org>,  Matthew Wilcox <willy@infradead.org>,  Michal Hocko
 <mhocko@suse.com>,  Yosry Ahmed <yosryahmed@google.com>,  David
 Hildenbrand <david@redhat.com>,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 6/9] mm/swap: handle swapcache lookup in swapin_entry
In-Reply-To: <CAMgjq7Ce6sCSTRd==N3ihrAcvVd2ggszdZuTwxDbFYcBWzcE_g@mail.gmail.com>
	(Kairui Song's message of "Wed, 10 Jan 2024 10:53:42 +0800")
References: <20240102175338.62012-1-ryncsn@gmail.com>
	<20240102175338.62012-7-ryncsn@gmail.com>
	<87a5pg9qno.fsf@yhuang6-desk2.ccr.corp.intel.com>
	<CAMgjq7Ce6sCSTRd==N3ihrAcvVd2ggszdZuTwxDbFYcBWzcE_g@mail.gmail.com>
Date: Mon, 15 Jan 2024 09:45:19 +0800
Message-ID: <87wmsb1ia8.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Kairui Song <ryncsn@gmail.com> writes:

> Huang, Ying <ying.huang@intel.com> =E4=BA=8E2024=E5=B9=B41=E6=9C=888=E6=
=97=A5=E5=91=A8=E4=B8=80 16:28=E5=86=99=E9=81=93=EF=BC=9A
>>
>> Kairui Song <ryncsn@gmail.com> writes:
>>
>> > From: Kairui Song <kasong@tencent.com>
>> >
>> > Since all callers of swapin_entry need to check the swap cache first, =
we
>> > can merge this common routine into swapin_entry, so it can be shared a=
nd
>> > optimized later.
>> >
>> > Also introduce a enum to better represent possible swap cache usage, a=
nd
>> > add some comments about it, make the usage of swap cache easier to
>> > understand.
>>
>> I don't find any benefit to do this.  The code line number isn't
>> reduced.  The concept of swap cache isn't hided either.
>
> Hi Ying
>
> Thanks for the comments.
>
> Maybe I should squash this with the following commit? The following
> commit want to do cache lookup in swapin_entry to avoid a redundant
> shadow lookup, it can help to improve the performance by about 4% for
> swapin path.

It's good to improve performance.  But I don't think we must put
swap_cache_get_folio() in swapin_entry() to do that.  We can just get
"shadow" from swap_cache_get_folio() and pass it to swapin_entry().

> So it need to return a enum to represent cache status.

I don't think we are talking about the same thing here.

> Further more, note the comments added here:
>
> +/*
> + * Caller of swapin_entry may need to know the cache lookup result:
> + *
> + * SWAP_CACHE_HIT: cache hit, cached folio is retured.
> + * SWAP_CACHE_MISS: cache miss, folio is allocated, read from swap device
> + *                  and adde to swap cache, but still may return a cached
> + *                  folio if raced (check __read_swap_cache_async).
> + * SWAP_CACHE_BYPASS: cache miss, folio is new allocated and read
> + *                    from swap device bypassing the cache.
> + */
>
> SWAP_CACHE_MISS might be inaccurate, this is not an issue introduced
> by this commit, but better exposed. May worth a fix later. So far I
> can see two benefits fixing it:
> - More accurate maj/min page fault count.
> - Note the PageHWPoison check in do_swap_page, it ignored the race
> case, if a page getting poisoned is raced with swapcache then it may
> not work as expected.
>
> These are all minor issue indeed, some other optimization might also
> be doable, but at least a comment might be helpful.
>

--
Best Regards,
Huang, Ying