linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Kairui Song <ryncsn@gmail.com>
To: "Huang, Ying" <ying.huang@intel.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
Date: Wed, 10 Jan 2024 10:53:42 +0800	[thread overview]
Message-ID: <CAMgjq7Ce6sCSTRd==N3ihrAcvVd2ggszdZuTwxDbFYcBWzcE_g@mail.gmail.com> (raw)
In-Reply-To: <87a5pg9qno.fsf@yhuang6-desk2.ccr.corp.intel.com>

Huang, Ying <ying.huang@intel.com> 于2024年1月8日周一 16:28写道:
>
> 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 and
> > optimized later.
> >
> > Also introduce a enum to better represent possible swap cache usage, and
> > 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. So it need to return a enum to represent cache status.

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
>


  reply	other threads:[~2024-01-10  2:54 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-02 17:53 [PATCH v2 0/9] swapin refactor for optimization and unified readahead Kairui Song
2024-01-02 17:53 ` [PATCH v2 1/9] mm/swapfile.c: add back some comment Kairui Song
2024-01-02 17:53 ` [PATCH v2 2/9] mm/swap: move no readahead swapin code to a stand-alone helper Kairui Song
2024-01-04  7:28   ` Huang, Ying
2024-01-05  7:43     ` Kairui Song
2024-01-02 17:53 ` [PATCH v2 3/9] mm/swap: avoid doing extra unlock error checks for direct swapin Kairui Song
2024-01-04  8:10   ` Huang, Ying
2024-01-09  9:38     ` Kairui Song
2024-01-02 17:53 ` [PATCH v2 4/9] mm/swap: always account swapped in page into current memcg Kairui Song
2024-01-05  7:14   ` Huang, Ying
2024-01-05  7:33     ` Kairui Song
2024-01-08  7:44       ` Huang, Ying
2024-01-09  9:42         ` Kairui Song
2024-01-02 17:53 ` [PATCH v2 5/9] mm/swap: introduce swapin_entry for unified readahead policy Kairui Song
2024-01-05  7:28   ` Huang, Ying
2024-01-10  2:42     ` Kairui Song
2024-01-02 17:53 ` [PATCH v2 6/9] mm/swap: handle swapcache lookup in swapin_entry Kairui Song
2024-01-08  8:26   ` Huang, Ying
2024-01-10  2:53     ` Kairui Song [this message]
2024-01-15  1:45       ` Huang, Ying
2024-01-15 17:11         ` Kairui Song
2024-01-02 17:53 ` [PATCH v2 7/9] mm/swap: avoid a duplicated swap cache lookup for SWP_SYNCHRONOUS_IO Kairui Song
2024-01-03 12:50   ` kernel test robot
2024-01-02 17:53 ` [PATCH v2 8/9] mm/swap: introduce a helper for swapin without vmfault Kairui Song
2024-01-09  1:08   ` Huang, Ying
2024-01-10  3:32     ` Kairui Song
2024-01-15  1:52       ` Huang, Ying
2024-01-21 18:40         ` Kairui Song
2024-01-22  6:38           ` Huang, Ying
2024-01-22 11:35             ` Kairui Song
2024-01-24  3:31               ` Huang, Ying
2024-01-02 17:53 ` [PATCH v2 9/9] mm/swap, shmem: use new swapin helper to skip readahead conditionally Kairui Song
2024-01-03 11:56   ` kernel test robot
2024-01-03 13:45   ` kernel test robot
2024-01-09  2:03   ` Huang, Ying
2024-01-10  3:35     ` Kairui Song
2024-01-30  0:39       ` Kairui Song
2024-01-30  2:01         ` Huang, Ying

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMgjq7Ce6sCSTRd==N3ihrAcvVd2ggszdZuTwxDbFYcBWzcE_g@mail.gmail.com' \
    --to=ryncsn@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=chrisl@kernel.org \
    --cc=david@redhat.com \
    --cc=hannes@cmpxchg.org \
    --cc=hughd@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=willy@infradead.org \
    --cc=ying.huang@intel.com \
    --cc=yosryahmed@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox