[PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Liu Shixin]

When spaces of swap devices are exhausted, only file pages can be
reclaimed.  But there are still some swapcache pages in anon lru list.
This can lead to a premature out-of-memory.

The problem is found with such step:

 Firstly, set a 9MB disk swap space, then create a cgroup with 10MB
 memory limit, then runs an program to allocates about 15MB memory.

The problem occurs occasionally, which may need about 100 times [1].

Fix it by checking number of swapcache pages in can_reclaim_anon_pages().
If the number is not zero, return true and set swapcache_only to 1.
When scan anon lru list in swapcache_only mode, non-swapcache pages will
be skipped to isolate in order to accelerate reclaim efficiency.

However, in swapcache_only mode, the scan count still increased when scan
non-swapcache pages because there are large number of non-swapcache pages
and rare swapcache pages in swapcache_only mode, and if the non-swapcache
is skipped and do not count, the scan of pages in isolate_lru_folios() can
eventually lead to hung task, just as Sachin reported [2].

By the way, since there are enough times of memory reclaim before OOM, it
is not need to isolate too much swapcache pages in one times.

[1]. https://lore.kernel.org/lkml/CAJD7tkZAfgncV+KbKr36=eDzMnT=9dZOT0dpMWcurHLr6Do+GA@mail.gmail.com/
[2]. https://lore.kernel.org/linux-mm/CAJD7tkafz_2XAuqE8tGLPEcpLngewhUo=5US14PAtSM9tLBUQg@mail.gmail.com/

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
Tested-by: Yosry Ahmed <yosryahmed@google.com>
Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
---
v9->v10: Use per-node swapcache suggested by Yu Zhao.
v8->v9: Move the swapcache check after can_demote() and refector 
	can_reclaim_anon_pages() a bit.
v7->v8: Reset swapcache_only at the beginning of can_reclaim_anon_pages().
v6->v7: Reset swapcache_only to zero after there are swap spaces.
v5->v6: Fix NULL pointing derefence and hung task problem reported by Sachin.

 mm/vmscan.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 506f8220c5fe..1fcc94717370 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -136,6 +136,9 @@ struct scan_control {
 	/* Always discard instead of demoting to lower tier memory */
 	unsigned int no_demotion:1;
 
+	/* Swap space is exhausted, only reclaim swapcache for anon LRU */
+	unsigned int swapcache_only:1;
+
 	/* Allocation order */
 	s8 order;
 
@@ -308,10 +311,36 @@ static bool can_demote(int nid, struct scan_control *sc)
 	return true;
 }
 
+#ifdef CONFIG_SWAP
+static bool can_reclaim_swapcache(struct mem_cgroup *memcg, int nid)
+{
+	struct pglist_data *pgdat = NODE_DATA(nid);
+	unsigned long nr_swapcache;
+
+	if (!memcg) {
+		nr_swapcache = node_page_state(pgdat, NR_SWAPCACHE);
+	} else {
+		struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
+
+		nr_swapcache = lruvec_page_state_local(lruvec, NR_SWAPCACHE);
+	}
+
+	return nr_swapcache > 0;
+}
+#else
+static bool can_reclaim_swapcache(struct mem_cgroup *memcg, int nid)
+{
+	return false;
+}
+#endif
+
 static inline bool can_reclaim_anon_pages(struct mem_cgroup *memcg,
 					  int nid,
 					  struct scan_control *sc)
 {
+	if (sc)
+		sc->swapcache_only = 0;
+
 	if (memcg == NULL) {
 		/*
 		 * For non-memcg reclaim, is there
@@ -330,7 +359,17 @@ static inline bool can_reclaim_anon_pages(struct mem_cgroup *memcg,
 	 *
 	 * Can it be reclaimed from this node via demotion?
 	 */
-	return can_demote(nid, sc);
+	if (can_demote(nid, sc))
+		return true;
+
+	/* Is there any swapcache pages to reclaim in this node? */
+	if (can_reclaim_swapcache(memcg, nid)) {
+		if (sc)
+			sc->swapcache_only = 1;
+		return true;
+	}
+
+	return false;
 }
 
 /*
@@ -1642,6 +1681,15 @@ static unsigned long isolate_lru_folios(unsigned long nr_to_scan,
 		 */
 		scan += nr_pages;
 
+		/*
+		 * Count non-swapcache too because the swapcache pages may
+		 * be rare and it takes too much times here if not count
+		 * the non-swapcache pages.
+		 */
+		if (unlikely(sc->swapcache_only && !is_file_lru(lru) &&
+		    !folio_test_swapcache(folio)))
+			goto move;
+
 		if (!folio_test_lru(folio))
 			goto move;
 		if (!sc->may_unmap && folio_mapped(folio))
-- 
2.25.1

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Michal Hocko]

On Tue 21-11-23 17:06:24, Liu Shixin wrote:
> When spaces of swap devices are exhausted, only file pages can be
> reclaimed.  But there are still some swapcache pages in anon lru list.
> This can lead to a premature out-of-memory.
> 
> The problem is found with such step:
> 
>  Firstly, set a 9MB disk swap space, then create a cgroup with 10MB
>  memory limit, then runs an program to allocates about 15MB memory.
> 
> The problem occurs occasionally, which may need about 100 times [1].
> 
> Fix it by checking number of swapcache pages in can_reclaim_anon_pages().
> If the number is not zero, return true and set swapcache_only to 1.
> When scan anon lru list in swapcache_only mode, non-swapcache pages will
> be skipped to isolate in order to accelerate reclaim efficiency.
> 
> However, in swapcache_only mode, the scan count still increased when scan
> non-swapcache pages because there are large number of non-swapcache pages
> and rare swapcache pages in swapcache_only mode, and if the non-swapcache
> is skipped and do not count, the scan of pages in isolate_lru_folios() can
> eventually lead to hung task, just as Sachin reported [2].

I find this paragraph really confusing! I guess what you meant to say is
that a real swapcache_only is problematic because it can end up not
making any progress, correct? 

AFAIU you have addressed that problem by making swapcache_only anon LRU
specific, right? That would be certainly more robust as you can still
reclaim from file LRUs. I cannot say I like that because swapcache_only
is a bit confusing and I do not think we want to grow more special
purpose reclaim types. Would it be possible/reasonable to instead put
swapcache pages on the file LRU instead?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Liu Shixin]

On 2023/11/21 21:00, Michal Hocko wrote:
> On Tue 21-11-23 17:06:24, Liu Shixin wrote:
>
> However, in swapcache_only mode, the scan count still increased when scan
> non-swapcache pages because there are large number of non-swapcache pages
> and rare swapcache pages in swapcache_only mode, and if the non-swapcache
> is skipped and do not count, the scan of pages in isolate_lru_folios() can
> eventually lead to hung task, just as Sachin reported [2].
> I find this paragraph really confusing! I guess what you meant to say is
> that a real swapcache_only is problematic because it can end up not
> making any progress, correct? 
This paragraph is going to explain why checking swapcache_only after scan += nr_pages;
>
> AFAIU you have addressed that problem by making swapcache_only anon LRU
> specific, right? That would be certainly more robust as you can still
> reclaim from file LRUs. I cannot say I like that because swapcache_only
> is a bit confusing and I do not think we want to grow more special
> purpose reclaim types. Would it be possible/reasonable to instead put
> swapcache pages on the file LRU instead?
It looks like a good idea, but I'm not sure if it's possible. I can try it, is there anything to
pay attention to?

Thanks,

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Yosry Ahmed]

On Tue, Nov 21, 2023 at 10:41 PM Liu Shixin <liushixin2@huawei.com> wrote:
>
>
> On 2023/11/21 21:00, Michal Hocko wrote:
> > On Tue 21-11-23 17:06:24, Liu Shixin wrote:
> >
> > However, in swapcache_only mode, the scan count still increased when scan
> > non-swapcache pages because there are large number of non-swapcache pages
> > and rare swapcache pages in swapcache_only mode, and if the non-swapcache
> > is skipped and do not count, the scan of pages in isolate_lru_folios() can
> > eventually lead to hung task, just as Sachin reported [2].
> > I find this paragraph really confusing! I guess what you meant to say is
> > that a real swapcache_only is problematic because it can end up not
> > making any progress, correct?
> This paragraph is going to explain why checking swapcache_only after scan += nr_pages;
> >
> > AFAIU you have addressed that problem by making swapcache_only anon LRU
> > specific, right? That would be certainly more robust as you can still
> > reclaim from file LRUs. I cannot say I like that because swapcache_only
> > is a bit confusing and I do not think we want to grow more special
> > purpose reclaim types. Would it be possible/reasonable to instead put
> > swapcache pages on the file LRU instead?
> It looks like a good idea, but I'm not sure if it's possible. I can try it, is there anything to
> pay attention to?

I think this might be more intrusive than we think. Every time a page
is added to or removed from the swap cache, we will need to move it
between LRUs. All pages on the anon LRU will need to go through the
file LRU before being reclaimed. I think this might be too big of a
change to achieve this patch's goal.

>
> Thanks,
>

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Yosry Ahmed <yosryahmed@google.com> writes:

> On Tue, Nov 21, 2023 at 10:41 PM Liu Shixin <liushixin2@huawei.com> wrote:
>>
>>
>> On 2023/11/21 21:00, Michal Hocko wrote:
>> > On Tue 21-11-23 17:06:24, Liu Shixin wrote:
>> >
>> > However, in swapcache_only mode, the scan count still increased when scan
>> > non-swapcache pages because there are large number of non-swapcache pages
>> > and rare swapcache pages in swapcache_only mode, and if the non-swapcache
>> > is skipped and do not count, the scan of pages in isolate_lru_folios() can
>> > eventually lead to hung task, just as Sachin reported [2].
>> > I find this paragraph really confusing! I guess what you meant to say is
>> > that a real swapcache_only is problematic because it can end up not
>> > making any progress, correct?
>> This paragraph is going to explain why checking swapcache_only after scan += nr_pages;
>> >
>> > AFAIU you have addressed that problem by making swapcache_only anon LRU
>> > specific, right? That would be certainly more robust as you can still
>> > reclaim from file LRUs. I cannot say I like that because swapcache_only
>> > is a bit confusing and I do not think we want to grow more special
>> > purpose reclaim types. Would it be possible/reasonable to instead put
>> > swapcache pages on the file LRU instead?
>> It looks like a good idea, but I'm not sure if it's possible. I can try it, is there anything to
>> pay attention to?
>
> I think this might be more intrusive than we think. Every time a page
> is added to or removed from the swap cache, we will need to move it
> between LRUs. All pages on the anon LRU will need to go through the
> file LRU before being reclaimed. I think this might be too big of a
> change to achieve this patch's goal.

We need to identify swap cache pages on file LRU firstly.  It appears
hard from the current definition of page flags.

  /* Filesystems */
  PG_checked = PG_owner_priv_1,

  /* SwapBacked */
  PG_swapcache = PG_owner_priv_1,       /* Swap page: swp_entry_t in private */

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Michal Hocko]

On Wed 22-11-23 14:57:24, Huang, Ying wrote:
> Yosry Ahmed <yosryahmed@google.com> writes:
> 
> > On Tue, Nov 21, 2023 at 10:41 PM Liu Shixin <liushixin2@huawei.com> wrote:
> >>
> >>
> >> On 2023/11/21 21:00, Michal Hocko wrote:
> >> > On Tue 21-11-23 17:06:24, Liu Shixin wrote:
> >> >
> >> > However, in swapcache_only mode, the scan count still increased when scan
> >> > non-swapcache pages because there are large number of non-swapcache pages
> >> > and rare swapcache pages in swapcache_only mode, and if the non-swapcache
> >> > is skipped and do not count, the scan of pages in isolate_lru_folios() can
> >> > eventually lead to hung task, just as Sachin reported [2].
> >> > I find this paragraph really confusing! I guess what you meant to say is
> >> > that a real swapcache_only is problematic because it can end up not
> >> > making any progress, correct?
> >> This paragraph is going to explain why checking swapcache_only after scan += nr_pages;
> >> >
> >> > AFAIU you have addressed that problem by making swapcache_only anon LRU
> >> > specific, right? That would be certainly more robust as you can still
> >> > reclaim from file LRUs. I cannot say I like that because swapcache_only
> >> > is a bit confusing and I do not think we want to grow more special
> >> > purpose reclaim types. Would it be possible/reasonable to instead put
> >> > swapcache pages on the file LRU instead?
> >> It looks like a good idea, but I'm not sure if it's possible. I can try it, is there anything to
> >> pay attention to?
> >
> > I think this might be more intrusive than we think. Every time a page
> > is added to or removed from the swap cache, we will need to move it
> > between LRUs. All pages on the anon LRU will need to go through the
> > file LRU before being reclaimed. I think this might be too big of a
> > change to achieve this patch's goal.
> 
> We need to identify swap cache pages on file LRU firstly.  It appears
> hard from the current definition of page flags.
> 
>   /* Filesystems */
>   PG_checked = PG_owner_priv_1,
> 
>   /* SwapBacked */
>   PG_swapcache = PG_owner_priv_1,       /* Swap page: swp_entry_t in private */

Checking along with folio_test_swapbacked would do the trick, right?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Michal Hocko]

On Tue 21-11-23 22:44:32, Yosry Ahmed wrote:
> On Tue, Nov 21, 2023 at 10:41 PM Liu Shixin <liushixin2@huawei.com> wrote:
> >
> >
> > On 2023/11/21 21:00, Michal Hocko wrote:
> > > On Tue 21-11-23 17:06:24, Liu Shixin wrote:
> > >
> > > However, in swapcache_only mode, the scan count still increased when scan
> > > non-swapcache pages because there are large number of non-swapcache pages
> > > and rare swapcache pages in swapcache_only mode, and if the non-swapcache
> > > is skipped and do not count, the scan of pages in isolate_lru_folios() can
> > > eventually lead to hung task, just as Sachin reported [2].
> > > I find this paragraph really confusing! I guess what you meant to say is
> > > that a real swapcache_only is problematic because it can end up not
> > > making any progress, correct?
> > This paragraph is going to explain why checking swapcache_only after scan += nr_pages;
> > >
> > > AFAIU you have addressed that problem by making swapcache_only anon LRU
> > > specific, right? That would be certainly more robust as you can still
> > > reclaim from file LRUs. I cannot say I like that because swapcache_only
> > > is a bit confusing and I do not think we want to grow more special
> > > purpose reclaim types. Would it be possible/reasonable to instead put
> > > swapcache pages on the file LRU instead?
> > It looks like a good idea, but I'm not sure if it's possible. I can try it, is there anything to
> > pay attention to?
> 
> I think this might be more intrusive than we think. Every time a page
> is added to or removed from the swap cache, we will need to move it
> between LRUs. All pages on the anon LRU will need to go through the
> file LRU before being reclaimed. I think this might be too big of a
> change to achieve this patch's goal.

TBH I am not really sure how complex that might turn out to be.
Swapcache tends to be full of subtle issues. So you might be right but
it would be better to know _why_ this is not possible before we end up
phising for couple of swapcache pages on potentially huge anon LRU to
isolate them. Think of TB sized machines in this context.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Michal Hocko]

On Wed 22-11-23 09:52:42, Michal Hocko wrote:
> On Tue 21-11-23 22:44:32, Yosry Ahmed wrote:
> > On Tue, Nov 21, 2023 at 10:41 PM Liu Shixin <liushixin2@huawei.com> wrote:
> > >
> > >
> > > On 2023/11/21 21:00, Michal Hocko wrote:
> > > > On Tue 21-11-23 17:06:24, Liu Shixin wrote:
> > > >
> > > > However, in swapcache_only mode, the scan count still increased when scan
> > > > non-swapcache pages because there are large number of non-swapcache pages
> > > > and rare swapcache pages in swapcache_only mode, and if the non-swapcache
> > > > is skipped and do not count, the scan of pages in isolate_lru_folios() can
> > > > eventually lead to hung task, just as Sachin reported [2].
> > > > I find this paragraph really confusing! I guess what you meant to say is
> > > > that a real swapcache_only is problematic because it can end up not
> > > > making any progress, correct?
> > > This paragraph is going to explain why checking swapcache_only after scan += nr_pages;
> > > >
> > > > AFAIU you have addressed that problem by making swapcache_only anon LRU
> > > > specific, right? That would be certainly more robust as you can still
> > > > reclaim from file LRUs. I cannot say I like that because swapcache_only
> > > > is a bit confusing and I do not think we want to grow more special
> > > > purpose reclaim types. Would it be possible/reasonable to instead put
> > > > swapcache pages on the file LRU instead?
> > > It looks like a good idea, but I'm not sure if it's possible. I can try it, is there anything to
> > > pay attention to?
> > 
> > I think this might be more intrusive than we think. Every time a page
> > is added to or removed from the swap cache, we will need to move it
> > between LRUs. All pages on the anon LRU will need to go through the
> > file LRU before being reclaimed. I think this might be too big of a
> > change to achieve this patch's goal.
> 
> TBH I am not really sure how complex that might turn out to be.
> Swapcache tends to be full of subtle issues. So you might be right but
> it would be better to know _why_ this is not possible before we end up
> phising for couple of swapcache pages on potentially huge anon LRU to
> isolate them. Think of TB sized machines in this context.

Forgot to mention that it is not really far fetched from comparing this
to MADV_FREE pages. Those are anonymous but we do not want to keep them
on anon LRU because we want to age them indepdendent on the swap
availability as they are just dropped during reclaim. Not too much
different from swapcache pages. There are more constrains on those but
fundamentally this is the same problem, no?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Yosry Ahmed]

On Wed, Nov 22, 2023 at 2:09 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Wed 22-11-23 09:52:42, Michal Hocko wrote:
> > On Tue 21-11-23 22:44:32, Yosry Ahmed wrote:
> > > On Tue, Nov 21, 2023 at 10:41 PM Liu Shixin <liushixin2@huawei.com> wrote:
> > > >
> > > >
> > > > On 2023/11/21 21:00, Michal Hocko wrote:
> > > > > On Tue 21-11-23 17:06:24, Liu Shixin wrote:
> > > > >
> > > > > However, in swapcache_only mode, the scan count still increased when scan
> > > > > non-swapcache pages because there are large number of non-swapcache pages
> > > > > and rare swapcache pages in swapcache_only mode, and if the non-swapcache
> > > > > is skipped and do not count, the scan of pages in isolate_lru_folios() can
> > > > > eventually lead to hung task, just as Sachin reported [2].
> > > > > I find this paragraph really confusing! I guess what you meant to say is
> > > > > that a real swapcache_only is problematic because it can end up not
> > > > > making any progress, correct?
> > > > This paragraph is going to explain why checking swapcache_only after scan += nr_pages;
> > > > >
> > > > > AFAIU you have addressed that problem by making swapcache_only anon LRU
> > > > > specific, right? That would be certainly more robust as you can still
> > > > > reclaim from file LRUs. I cannot say I like that because swapcache_only
> > > > > is a bit confusing and I do not think we want to grow more special
> > > > > purpose reclaim types. Would it be possible/reasonable to instead put
> > > > > swapcache pages on the file LRU instead?
> > > > It looks like a good idea, but I'm not sure if it's possible. I can try it, is there anything to
> > > > pay attention to?
> > >
> > > I think this might be more intrusive than we think. Every time a page
> > > is added to or removed from the swap cache, we will need to move it
> > > between LRUs. All pages on the anon LRU will need to go through the
> > > file LRU before being reclaimed. I think this might be too big of a
> > > change to achieve this patch's goal.
> >
> > TBH I am not really sure how complex that might turn out to be.
> > Swapcache tends to be full of subtle issues. So you might be right but
> > it would be better to know _why_ this is not possible before we end up
> > phising for couple of swapcache pages on potentially huge anon LRU to
> > isolate them. Think of TB sized machines in this context.
>
> Forgot to mention that it is not really far fetched from comparing this
> to MADV_FREE pages. Those are anonymous but we do not want to keep them
> on anon LRU because we want to age them indepdendent on the swap
> availability as they are just dropped during reclaim. Not too much
> different from swapcache pages. There are more constrains on those but
> fundamentally this is the same problem, no?

I agree it's not a first, but swap cache pages are more complicated
because they can go back and forth, unlike MADV_FREE pages which
usually go on a one way ticket AFAICT. Also pages going into the swap
cache can be much more common that MADV_FREE pages for a lot of
workloads. I am not sure how different reclaim heuristics will react
to such mobility between the LRUs, and the fact that all pages will
now only get evicted through the file LRU. The anon LRU will
essentially become an LRU that feeds the file LRU. Also, the more
pages we move between LRUs, the more ordering violations we introduce,
as we may put colder pages in front of hotter pages or vice versa.

All in all, I am not saying it's a bad idea or not possible, I am just
saying it's probably more complicated than MADV_FREE, and adding more
cases where pages move between LRUs could introduce problems (or make
existing problems more visible).

> --
> Michal Hocko
> SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Michal Hocko]

On Wed 22-11-23 02:39:15, Yosry Ahmed wrote:
> On Wed, Nov 22, 2023 at 2:09 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Wed 22-11-23 09:52:42, Michal Hocko wrote:
> > > On Tue 21-11-23 22:44:32, Yosry Ahmed wrote:
> > > > On Tue, Nov 21, 2023 at 10:41 PM Liu Shixin <liushixin2@huawei.com> wrote:
> > > > >
> > > > >
> > > > > On 2023/11/21 21:00, Michal Hocko wrote:
> > > > > > On Tue 21-11-23 17:06:24, Liu Shixin wrote:
> > > > > >
> > > > > > However, in swapcache_only mode, the scan count still increased when scan
> > > > > > non-swapcache pages because there are large number of non-swapcache pages
> > > > > > and rare swapcache pages in swapcache_only mode, and if the non-swapcache
> > > > > > is skipped and do not count, the scan of pages in isolate_lru_folios() can
> > > > > > eventually lead to hung task, just as Sachin reported [2].
> > > > > > I find this paragraph really confusing! I guess what you meant to say is
> > > > > > that a real swapcache_only is problematic because it can end up not
> > > > > > making any progress, correct?
> > > > > This paragraph is going to explain why checking swapcache_only after scan += nr_pages;
> > > > > >
> > > > > > AFAIU you have addressed that problem by making swapcache_only anon LRU
> > > > > > specific, right? That would be certainly more robust as you can still
> > > > > > reclaim from file LRUs. I cannot say I like that because swapcache_only
> > > > > > is a bit confusing and I do not think we want to grow more special
> > > > > > purpose reclaim types. Would it be possible/reasonable to instead put
> > > > > > swapcache pages on the file LRU instead?
> > > > > It looks like a good idea, but I'm not sure if it's possible. I can try it, is there anything to
> > > > > pay attention to?
> > > >
> > > > I think this might be more intrusive than we think. Every time a page
> > > > is added to or removed from the swap cache, we will need to move it
> > > > between LRUs. All pages on the anon LRU will need to go through the
> > > > file LRU before being reclaimed. I think this might be too big of a
> > > > change to achieve this patch's goal.
> > >
> > > TBH I am not really sure how complex that might turn out to be.
> > > Swapcache tends to be full of subtle issues. So you might be right but
> > > it would be better to know _why_ this is not possible before we end up
> > > phising for couple of swapcache pages on potentially huge anon LRU to
> > > isolate them. Think of TB sized machines in this context.
> >
> > Forgot to mention that it is not really far fetched from comparing this
> > to MADV_FREE pages. Those are anonymous but we do not want to keep them
> > on anon LRU because we want to age them indepdendent on the swap
> > availability as they are just dropped during reclaim. Not too much
> > different from swapcache pages. There are more constrains on those but
> > fundamentally this is the same problem, no?
> 
> I agree it's not a first, but swap cache pages are more complicated
> because they can go back and forth, unlike MADV_FREE pages which
> usually go on a one way ticket AFAICT.

Yes swapcache pages are indeed more complicated but most of the time
they just go away as well, no? MADV_FREE can be reinitiated if they are
written as well. So fundamentally they are not that different.

> Also pages going into the swap
> cache can be much more common that MADV_FREE pages for a lot of
> workloads. I am not sure how different reclaim heuristics will react
> to such mobility between the LRUs, and the fact that all pages will
> now only get evicted through the file LRU. The anon LRU will
> essentially become an LRU that feeds the file LRU. Also, the more
> pages we move between LRUs, the more ordering violations we introduce,
> as we may put colder pages in front of hotter pages or vice versa.

Well, traditionally the file LRU has been maintaining page cache or
easily disposable pages like MADV_FREE (which can be considered a cache
as well). Swapcache is a form of a page cache as well.

> All in all, I am not saying it's a bad idea or not possible, I am just
> saying it's probably more complicated than MADV_FREE, and adding more
> cases where pages move between LRUs could introduce problems (or make
> existing problems more visible).

Do we want to start adding filtered anon scan for a certain type of
pages? Because this is the question here AFAICS. This might seem an
easier solution but I would argue that it is less predictable one. 
It is not unusual that a huge anon LRU would contain only very few LRU
pages.

That being said, I might be missing some obvious or less obvious reasons
why this is completely bad idea. Swapcache is indeed subtle.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Yosry Ahmed]

On Wed, Nov 22, 2023 at 5:19 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Wed 22-11-23 02:39:15, Yosry Ahmed wrote:
> > On Wed, Nov 22, 2023 at 2:09 AM Michal Hocko <mhocko@suse.com> wrote:
> > >
> > > On Wed 22-11-23 09:52:42, Michal Hocko wrote:
> > > > On Tue 21-11-23 22:44:32, Yosry Ahmed wrote:
> > > > > On Tue, Nov 21, 2023 at 10:41 PM Liu Shixin <liushixin2@huawei.com> wrote:
> > > > > >
> > > > > >
> > > > > > On 2023/11/21 21:00, Michal Hocko wrote:
> > > > > > > On Tue 21-11-23 17:06:24, Liu Shixin wrote:
> > > > > > >
> > > > > > > However, in swapcache_only mode, the scan count still increased when scan
> > > > > > > non-swapcache pages because there are large number of non-swapcache pages
> > > > > > > and rare swapcache pages in swapcache_only mode, and if the non-swapcache
> > > > > > > is skipped and do not count, the scan of pages in isolate_lru_folios() can
> > > > > > > eventually lead to hung task, just as Sachin reported [2].
> > > > > > > I find this paragraph really confusing! I guess what you meant to say is
> > > > > > > that a real swapcache_only is problematic because it can end up not
> > > > > > > making any progress, correct?
> > > > > > This paragraph is going to explain why checking swapcache_only after scan += nr_pages;
> > > > > > >
> > > > > > > AFAIU you have addressed that problem by making swapcache_only anon LRU
> > > > > > > specific, right? That would be certainly more robust as you can still
> > > > > > > reclaim from file LRUs. I cannot say I like that because swapcache_only
> > > > > > > is a bit confusing and I do not think we want to grow more special
> > > > > > > purpose reclaim types. Would it be possible/reasonable to instead put
> > > > > > > swapcache pages on the file LRU instead?
> > > > > > It looks like a good idea, but I'm not sure if it's possible. I can try it, is there anything to
> > > > > > pay attention to?
> > > > >
> > > > > I think this might be more intrusive than we think. Every time a page
> > > > > is added to or removed from the swap cache, we will need to move it
> > > > > between LRUs. All pages on the anon LRU will need to go through the
> > > > > file LRU before being reclaimed. I think this might be too big of a
> > > > > change to achieve this patch's goal.
> > > >
> > > > TBH I am not really sure how complex that might turn out to be.
> > > > Swapcache tends to be full of subtle issues. So you might be right but
> > > > it would be better to know _why_ this is not possible before we end up
> > > > phising for couple of swapcache pages on potentially huge anon LRU to
> > > > isolate them. Think of TB sized machines in this context.
> > >
> > > Forgot to mention that it is not really far fetched from comparing this
> > > to MADV_FREE pages. Those are anonymous but we do not want to keep them
> > > on anon LRU because we want to age them indepdendent on the swap
> > > availability as they are just dropped during reclaim. Not too much
> > > different from swapcache pages. There are more constrains on those but
> > > fundamentally this is the same problem, no?
> >
> > I agree it's not a first, but swap cache pages are more complicated
> > because they can go back and forth, unlike MADV_FREE pages which
> > usually go on a one way ticket AFAICT.
>
> Yes swapcache pages are indeed more complicated but most of the time
> they just go away as well, no? MADV_FREE can be reinitiated if they are
> written as well. So fundamentally they are not that different.
>
> > Also pages going into the swap
> > cache can be much more common that MADV_FREE pages for a lot of
> > workloads. I am not sure how different reclaim heuristics will react
> > to such mobility between the LRUs, and the fact that all pages will
> > now only get evicted through the file LRU. The anon LRU will
> > essentially become an LRU that feeds the file LRU. Also, the more
> > pages we move between LRUs, the more ordering violations we introduce,
> > as we may put colder pages in front of hotter pages or vice versa.
>
> Well, traditionally the file LRU has been maintaining page cache or
> easily disposable pages like MADV_FREE (which can be considered a cache
> as well). Swapcache is a form of a page cache as well.

If I understand correctly, when we move the MADV_FREE pages to the
file LRU, we don't have correct information about their relative
ordering compared to the pages that are already in the inactive file
LRU. IOW, we mess up the ordering of the inactive file LRU a little.
If we add more cases of moving pages to the file LRU (for the swap
cache), we may make it worse. I am also not sure how this works with
MGLRU generations.

Keep in mind that when a page is affected with MADV_FREE, it's always
called. On the other hand, when a page is added to the swap cache, it
could be because it's under reclaim (cold), or it was just swapped in
(hot). I am not sure this practically matters, just something to think
about.

It also seems like all evictions will now be done from the file LRU,
so some heuristics (like workingset) may need to be updated
accordingly.

>
> > All in all, I am not saying it's a bad idea or not possible, I am just
> > saying it's probably more complicated than MADV_FREE, and adding more
> > cases where pages move between LRUs could introduce problems (or make
> > existing problems more visible).
>
> Do we want to start adding filtered anon scan for a certain type of
> pages? Because this is the question here AFAICS. This might seem an
> easier solution but I would argue that it is less predictable one.
> It is not unusual that a huge anon LRU would contain only very few LRU
> pages.

I agree that it may be a problem in some situations.

>
> That being said, I might be missing some obvious or less obvious reasons
> why this is completely bad idea. Swapcache is indeed subtle.
> --
> Michal Hocko
> SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Michal Hocko <mhocko@suse.com> writes:

> On Wed 22-11-23 02:39:15, Yosry Ahmed wrote:
>> On Wed, Nov 22, 2023 at 2:09 AM Michal Hocko <mhocko@suse.com> wrote:
>> >
>> > On Wed 22-11-23 09:52:42, Michal Hocko wrote:
>> > > On Tue 21-11-23 22:44:32, Yosry Ahmed wrote:
>> > > > On Tue, Nov 21, 2023 at 10:41 PM Liu Shixin <liushixin2@huawei.com> wrote:
>> > > > >
>> > > > >
>> > > > > On 2023/11/21 21:00, Michal Hocko wrote:
>> > > > > > On Tue 21-11-23 17:06:24, Liu Shixin wrote:
>> > > > > >
>> > > > > > However, in swapcache_only mode, the scan count still increased when scan
>> > > > > > non-swapcache pages because there are large number of non-swapcache pages
>> > > > > > and rare swapcache pages in swapcache_only mode, and if the non-swapcache
>> > > > > > is skipped and do not count, the scan of pages in isolate_lru_folios() can
>> > > > > > eventually lead to hung task, just as Sachin reported [2].
>> > > > > > I find this paragraph really confusing! I guess what you meant to say is
>> > > > > > that a real swapcache_only is problematic because it can end up not
>> > > > > > making any progress, correct?
>> > > > > This paragraph is going to explain why checking swapcache_only after scan += nr_pages;
>> > > > > >
>> > > > > > AFAIU you have addressed that problem by making swapcache_only anon LRU
>> > > > > > specific, right? That would be certainly more robust as you can still
>> > > > > > reclaim from file LRUs. I cannot say I like that because swapcache_only
>> > > > > > is a bit confusing and I do not think we want to grow more special
>> > > > > > purpose reclaim types. Would it be possible/reasonable to instead put
>> > > > > > swapcache pages on the file LRU instead?
>> > > > > It looks like a good idea, but I'm not sure if it's possible. I can try it, is there anything to
>> > > > > pay attention to?
>> > > >
>> > > > I think this might be more intrusive than we think. Every time a page
>> > > > is added to or removed from the swap cache, we will need to move it
>> > > > between LRUs. All pages on the anon LRU will need to go through the
>> > > > file LRU before being reclaimed. I think this might be too big of a
>> > > > change to achieve this patch's goal.
>> > >
>> > > TBH I am not really sure how complex that might turn out to be.
>> > > Swapcache tends to be full of subtle issues. So you might be right but
>> > > it would be better to know _why_ this is not possible before we end up
>> > > phising for couple of swapcache pages on potentially huge anon LRU to
>> > > isolate them. Think of TB sized machines in this context.
>> >
>> > Forgot to mention that it is not really far fetched from comparing this
>> > to MADV_FREE pages. Those are anonymous but we do not want to keep them
>> > on anon LRU because we want to age them indepdendent on the swap
>> > availability as they are just dropped during reclaim. Not too much
>> > different from swapcache pages. There are more constrains on those but
>> > fundamentally this is the same problem, no?
>> 
>> I agree it's not a first, but swap cache pages are more complicated
>> because they can go back and forth, unlike MADV_FREE pages which
>> usually go on a one way ticket AFAICT.
>
> Yes swapcache pages are indeed more complicated but most of the time
> they just go away as well, no?

When we swapin a page, we will put it in swapcache too.  And the page
can be in that state for long time if there is more than 50% free space
in the swap device.

> MADV_FREE can be reinitiated if they are
> written as well. So fundamentally they are not that different.
>

[snip]

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Michal Hocko]

On Thu 23-11-23 14:15:59, Huang, Ying wrote:
> Michal Hocko <mhocko@suse.com> writes:
[...]
> > Yes swapcache pages are indeed more complicated but most of the time
> > they just go away as well, no?
> 
> When we swapin a page, we will put it in swapcache too.  And the page
> can be in that state for long time if there is more than 50% free space
> in the swap device.

True, but why is that a problem? If you encounter such a swapped in page
on the file LRU then the page should be referened and as such should be
activated, no?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Michal Hocko <mhocko@suse.com> writes:

> On Thu 23-11-23 14:15:59, Huang, Ying wrote:
>> Michal Hocko <mhocko@suse.com> writes:
> [...]
>> > Yes swapcache pages are indeed more complicated but most of the time
>> > they just go away as well, no?
>> 
>> When we swapin a page, we will put it in swapcache too.  And the page
>> can be in that state for long time if there is more than 50% free space
>> in the swap device.
>
> True, but why is that a problem? If you encounter such a swapped in page
> on the file LRU then the page should be referened and as such should be
> activated, no?

This just means that anonymous pages in file LRU aren't temporary or
short-term.  So we need to consider that.  For example, the original
method to balance between anonymous pages and file pages need to be
re-designed.  The anonymous pages are considered hotter than file pages
in general.

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Chris Li]

Hi Ying,

On Sun, Nov 26, 2023 at 6:36 PM Huang, Ying <ying.huang@intel.com> wrote:
>
> Michal Hocko <mhocko@suse.com> writes:
>
> > On Thu 23-11-23 14:15:59, Huang, Ying wrote:
> >> Michal Hocko <mhocko@suse.com> writes:
> > [...]
> >> > Yes swapcache pages are indeed more complicated but most of the time
> >> > they just go away as well, no?
> >>
> >> When we swapin a page, we will put it in swapcache too.  And the page
> >> can be in that state for long time if there is more than 50% free space
> >> in the swap device.
> >
> > True, but why is that a problem? If you encounter such a swapped in page
> > on the file LRU then the page should be referened and as such should be
> > activated, no?
>
> This just means that anonymous pages in file LRU aren't temporary or
> short-term.  So we need to consider that.  For example, the original
> method to balance between anonymous pages and file pages need to be
> re-designed.  The anonymous pages are considered hotter than file pages
> in general.

 I agree with Ying that anonymous pages typically have different page
access patterns than file pages, so we might want to treat them
differently to reclaim them effectively.
One random idea:
How about we put the anonymous page in a swap cache in a different LRU
than the rest of the anonymous pages. Then shrinking against those
pages in the swap cache would be more effective.Instead of having
[anon, file] LRU, now we have [anon not in swap cache, anon in swap
cache, file] LRU

Chris

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Chris Li <chrisl@kernel.org> writes:

> Hi Ying,
>
> On Sun, Nov 26, 2023 at 6:36 PM Huang, Ying <ying.huang@intel.com> wrote:
>>
>> Michal Hocko <mhocko@suse.com> writes:
>>
>> > On Thu 23-11-23 14:15:59, Huang, Ying wrote:
>> >> Michal Hocko <mhocko@suse.com> writes:
>> > [...]
>> >> > Yes swapcache pages are indeed more complicated but most of the time
>> >> > they just go away as well, no?
>> >>
>> >> When we swapin a page, we will put it in swapcache too.  And the page
>> >> can be in that state for long time if there is more than 50% free space
>> >> in the swap device.
>> >
>> > True, but why is that a problem? If you encounter such a swapped in page
>> > on the file LRU then the page should be referened and as such should be
>> > activated, no?
>>
>> This just means that anonymous pages in file LRU aren't temporary or
>> short-term.  So we need to consider that.  For example, the original
>> method to balance between anonymous pages and file pages need to be
>> re-designed.  The anonymous pages are considered hotter than file pages
>> in general.
>
>  I agree with Ying that anonymous pages typically have different page
> access patterns than file pages, so we might want to treat them
> differently to reclaim them effectively.
> One random idea:
> How about we put the anonymous page in a swap cache in a different LRU
> than the rest of the anonymous pages. Then shrinking against those
> pages in the swap cache would be more effective.Instead of having
> [anon, file] LRU, now we have [anon not in swap cache, anon in swap
> cache, file] LRU

I don't think that it is necessary.  The patch is only for a special use
case.  Where the swap device is used up while some pages are in swap
cache.  The patch will kill performance, but it is used to avoid OOM
only, not to improve performance.  Per my understanding, we will not use
up swap device space in most cases.  This may be true for ZRAM, but will
we keep pages in swap cache for long when we use ZRAM?

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Chris Li]

On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
> >  I agree with Ying that anonymous pages typically have different page
> > access patterns than file pages, so we might want to treat them
> > differently to reclaim them effectively.
> > One random idea:
> > How about we put the anonymous page in a swap cache in a different LRU
> > than the rest of the anonymous pages. Then shrinking against those
> > pages in the swap cache would be more effective.Instead of having
> > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
> > cache, file] LRU
>
> I don't think that it is necessary.  The patch is only for a special use
> case.  Where the swap device is used up while some pages are in swap
> cache.  The patch will kill performance, but it is used to avoid OOM
> only, not to improve performance.  Per my understanding, we will not use
> up swap device space in most cases.  This may be true for ZRAM, but will
> we keep pages in swap cache for long when we use ZRAM?

I ask the question regarding how many pages can be freed by this patch
in this email thread as well, but haven't got the answer from the
author yet. That is one important aspect to evaluate how valuable is
that patch.
Regarding running out of swap space. That is a good point, in server
workload we don't typically run out of swap device space anyway.

Android uses ZRAM, the story might be different. Adding Minchan here.

Chris

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Minchan Kim]

On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
> On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
> > >  I agree with Ying that anonymous pages typically have different page
> > > access patterns than file pages, so we might want to treat them
> > > differently to reclaim them effectively.
> > > One random idea:
> > > How about we put the anonymous page in a swap cache in a different LRU
> > > than the rest of the anonymous pages. Then shrinking against those
> > > pages in the swap cache would be more effective.Instead of having
> > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
> > > cache, file] LRU
> >
> > I don't think that it is necessary.  The patch is only for a special use
> > case.  Where the swap device is used up while some pages are in swap
> > cache.  The patch will kill performance, but it is used to avoid OOM
> > only, not to improve performance.  Per my understanding, we will not use
> > up swap device space in most cases.  This may be true for ZRAM, but will
> > we keep pages in swap cache for long when we use ZRAM?
> 
> I ask the question regarding how many pages can be freed by this patch
> in this email thread as well, but haven't got the answer from the
> author yet. That is one important aspect to evaluate how valuable is
> that patch.

Exactly. Since swap cache has different life time with page cache, they
would be usually dropped when pages are unmapped(unless they are shared
with others but anon is usually exclusive private) so I wonder how much
memory we can save.

> Regarding running out of swap space. That is a good point, in server
> workload we don't typically run out of swap device space anyway.
> 
> Android uses ZRAM, the story might be different. Adding Minchan here.

Swap is usually almost full in Android since it compacts(i.e., swapout)
background apps aggressively.

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Yosry Ahmed]

On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
>
> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
> > > >  I agree with Ying that anonymous pages typically have different page
> > > > access patterns than file pages, so we might want to treat them
> > > > differently to reclaim them effectively.
> > > > One random idea:
> > > > How about we put the anonymous page in a swap cache in a different LRU
> > > > than the rest of the anonymous pages. Then shrinking against those
> > > > pages in the swap cache would be more effective.Instead of having
> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
> > > > cache, file] LRU
> > >
> > > I don't think that it is necessary.  The patch is only for a special use
> > > case.  Where the swap device is used up while some pages are in swap
> > > cache.  The patch will kill performance, but it is used to avoid OOM
> > > only, not to improve performance.  Per my understanding, we will not use
> > > up swap device space in most cases.  This may be true for ZRAM, but will
> > > we keep pages in swap cache for long when we use ZRAM?
> >
> > I ask the question regarding how many pages can be freed by this patch
> > in this email thread as well, but haven't got the answer from the
> > author yet. That is one important aspect to evaluate how valuable is
> > that patch.
>
> Exactly. Since swap cache has different life time with page cache, they
> would be usually dropped when pages are unmapped(unless they are shared
> with others but anon is usually exclusive private) so I wonder how much
> memory we can save.

I think the point of this patch is not saving memory, but rather
avoiding an OOM condition that will happen if we have no swap space
left, but some pages left in the swap cache. Of course, the OOM
avoidance will come at the cost of extra work in reclaim to swap those
pages out.

The only case where I think this might be harmful is if there's plenty
of pages to reclaim on the file LRU, and instead we opt to chase down
the few swap cache pages. So perhaps we can add a check to only set
sc->swapcache_only if the number of pages in the swap cache is more
than the number of pages on the file LRU or similar? Just make sure we
don't chase the swapcache pages down if there's plenty to scan on the
file LRU?

>
> > Regarding running out of swap space. That is a good point, in server
> > workload we don't typically run out of swap device space anyway.
> >
> > Android uses ZRAM, the story might be different. Adding Minchan here.
>
> Swap is usually almost full in Android since it compacts(i.e., swapout)
> background apps aggressively.

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Yosry Ahmed <yosryahmed@google.com> writes:

> On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
>>
>> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
>> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
>> > > >  I agree with Ying that anonymous pages typically have different page
>> > > > access patterns than file pages, so we might want to treat them
>> > > > differently to reclaim them effectively.
>> > > > One random idea:
>> > > > How about we put the anonymous page in a swap cache in a different LRU
>> > > > than the rest of the anonymous pages. Then shrinking against those
>> > > > pages in the swap cache would be more effective.Instead of having
>> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
>> > > > cache, file] LRU
>> > >
>> > > I don't think that it is necessary.  The patch is only for a special use
>> > > case.  Where the swap device is used up while some pages are in swap
>> > > cache.  The patch will kill performance, but it is used to avoid OOM
>> > > only, not to improve performance.  Per my understanding, we will not use
>> > > up swap device space in most cases.  This may be true for ZRAM, but will
>> > > we keep pages in swap cache for long when we use ZRAM?
>> >
>> > I ask the question regarding how many pages can be freed by this patch
>> > in this email thread as well, but haven't got the answer from the
>> > author yet. That is one important aspect to evaluate how valuable is
>> > that patch.
>>
>> Exactly. Since swap cache has different life time with page cache, they
>> would be usually dropped when pages are unmapped(unless they are shared
>> with others but anon is usually exclusive private) so I wonder how much
>> memory we can save.
>
> I think the point of this patch is not saving memory, but rather
> avoiding an OOM condition that will happen if we have no swap space
> left, but some pages left in the swap cache. Of course, the OOM
> avoidance will come at the cost of extra work in reclaim to swap those
> pages out.
>
> The only case where I think this might be harmful is if there's plenty
> of pages to reclaim on the file LRU, and instead we opt to chase down
> the few swap cache pages. So perhaps we can add a check to only set
> sc->swapcache_only if the number of pages in the swap cache is more
> than the number of pages on the file LRU or similar? Just make sure we
> don't chase the swapcache pages down if there's plenty to scan on the
> file LRU?

The swap cache pages can be divided to 3 groups.

- group 1: pages have been written out, at the tail of inactive LRU, but
  not reclaimed yet.

- group 2: pages have been written out, but were failed to be reclaimed
  (e.g., were accessed before reclaiming)

- group 3: pages have been swapped in, but were kept in swap cache.  The
  pages may be in active LRU.

The main target of the original patch should be group 1.  And the pages
may be cheaper to reclaim than file pages.

Group 2 are hard to be reclaimed if swap_count() isn't 0.

Group 3 should be reclaimed in theory, but the overhead may be high.
And we may need to reclaim the swap entries instead of pages if the pages
are hot.  But we can start to reclaim the swap entries before the swap
space is run out.

So, if we can count group 1, we may use that as indicator to scan anon
pages.  And we may add code to reclaim group 3 earlier.

>> > Regarding running out of swap space. That is a good point, in server
>> > workload we don't typically run out of swap device space anyway.

Think about this again.  In server workload, if we set some swap usage
limit for a memcg, we may run out of the limit.  Is it common for a
server workload run out of the swap usage limit of the memcg?

>> > Android uses ZRAM, the story might be different. Adding Minchan here.
>>
>> Swap is usually almost full in Android since it compacts(i.e., swapout)
>> background apps aggressively.

If my understanding were correct, because ZRAM has SWP_SYNCHRONOUS_IO
set, the anonymous pages will only be put in swap cache temporarily
during swap out.  So, the remaining swap cache pages in anon LRU should
not be a problem for ZRAM.

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Yosry Ahmed]

On Mon, Nov 27, 2023 at 7:21 PM Huang, Ying <ying.huang@intel.com> wrote:
>
> Yosry Ahmed <yosryahmed@google.com> writes:
>
> > On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
> >>
> >> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
> >> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
> >> > > >  I agree with Ying that anonymous pages typically have different page
> >> > > > access patterns than file pages, so we might want to treat them
> >> > > > differently to reclaim them effectively.
> >> > > > One random idea:
> >> > > > How about we put the anonymous page in a swap cache in a different LRU
> >> > > > than the rest of the anonymous pages. Then shrinking against those
> >> > > > pages in the swap cache would be more effective.Instead of having
> >> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
> >> > > > cache, file] LRU
> >> > >
> >> > > I don't think that it is necessary.  The patch is only for a special use
> >> > > case.  Where the swap device is used up while some pages are in swap
> >> > > cache.  The patch will kill performance, but it is used to avoid OOM
> >> > > only, not to improve performance.  Per my understanding, we will not use
> >> > > up swap device space in most cases.  This may be true for ZRAM, but will
> >> > > we keep pages in swap cache for long when we use ZRAM?
> >> >
> >> > I ask the question regarding how many pages can be freed by this patch
> >> > in this email thread as well, but haven't got the answer from the
> >> > author yet. That is one important aspect to evaluate how valuable is
> >> > that patch.
> >>
> >> Exactly. Since swap cache has different life time with page cache, they
> >> would be usually dropped when pages are unmapped(unless they are shared
> >> with others but anon is usually exclusive private) so I wonder how much
> >> memory we can save.
> >
> > I think the point of this patch is not saving memory, but rather
> > avoiding an OOM condition that will happen if we have no swap space
> > left, but some pages left in the swap cache. Of course, the OOM
> > avoidance will come at the cost of extra work in reclaim to swap those
> > pages out.
> >
> > The only case where I think this might be harmful is if there's plenty
> > of pages to reclaim on the file LRU, and instead we opt to chase down
> > the few swap cache pages. So perhaps we can add a check to only set
> > sc->swapcache_only if the number of pages in the swap cache is more
> > than the number of pages on the file LRU or similar? Just make sure we
> > don't chase the swapcache pages down if there's plenty to scan on the
> > file LRU?
>
> The swap cache pages can be divided to 3 groups.
>
> - group 1: pages have been written out, at the tail of inactive LRU, but
>   not reclaimed yet.
>
> - group 2: pages have been written out, but were failed to be reclaimed
>   (e.g., were accessed before reclaiming)
>
> - group 3: pages have been swapped in, but were kept in swap cache.  The
>   pages may be in active LRU.
>
> The main target of the original patch should be group 1.  And the pages
> may be cheaper to reclaim than file pages.
>
> Group 2 are hard to be reclaimed if swap_count() isn't 0.
>
> Group 3 should be reclaimed in theory, but the overhead may be high.
> And we may need to reclaim the swap entries instead of pages if the pages
> are hot.  But we can start to reclaim the swap entries before the swap
> space is run out.
>
> So, if we can count group 1, we may use that as indicator to scan anon
> pages.  And we may add code to reclaim group 3 earlier.
>

My point was not that reclaiming the pages in the swap cache is more
expensive that reclaiming the pages in the file LRU. In a lot of
cases, as you point out, the pages in the swap cache can just be
dropped, so they may be as cheap or cheaper to reclaim than the pages
in the file LRU.

My point was that scanning the anon LRU when swap space is exhausted
to get to the pages in the swap cache may be much more expensive,
because there may be a lot of pages on the anon LRU that are not in
the swap cache, and hence are not reclaimable, unlike pages in the
file LRU, which should mostly be reclaimable.

So what I am saying is that maybe we should not do the effort of
scanning the anon LRU in the swapcache_only case unless there aren't a
lot of pages to reclaim on the file LRU (relatively). For example, if
we have a 100 pages in the swap cache out of 10000 pages in the anon
LRU, and there are 10000 pages in the file LRU, it's probably not
worth scanning the anon LRU.

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Yosry Ahmed <yosryahmed@google.com> writes:

> On Mon, Nov 27, 2023 at 7:21 PM Huang, Ying <ying.huang@intel.com> wrote:
>>
>> Yosry Ahmed <yosryahmed@google.com> writes:
>>
>> > On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
>> >>
>> >> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
>> >> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
>> >> > > >  I agree with Ying that anonymous pages typically have different page
>> >> > > > access patterns than file pages, so we might want to treat them
>> >> > > > differently to reclaim them effectively.
>> >> > > > One random idea:
>> >> > > > How about we put the anonymous page in a swap cache in a different LRU
>> >> > > > than the rest of the anonymous pages. Then shrinking against those
>> >> > > > pages in the swap cache would be more effective.Instead of having
>> >> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
>> >> > > > cache, file] LRU
>> >> > >
>> >> > > I don't think that it is necessary.  The patch is only for a special use
>> >> > > case.  Where the swap device is used up while some pages are in swap
>> >> > > cache.  The patch will kill performance, but it is used to avoid OOM
>> >> > > only, not to improve performance.  Per my understanding, we will not use
>> >> > > up swap device space in most cases.  This may be true for ZRAM, but will
>> >> > > we keep pages in swap cache for long when we use ZRAM?
>> >> >
>> >> > I ask the question regarding how many pages can be freed by this patch
>> >> > in this email thread as well, but haven't got the answer from the
>> >> > author yet. That is one important aspect to evaluate how valuable is
>> >> > that patch.
>> >>
>> >> Exactly. Since swap cache has different life time with page cache, they
>> >> would be usually dropped when pages are unmapped(unless they are shared
>> >> with others but anon is usually exclusive private) so I wonder how much
>> >> memory we can save.
>> >
>> > I think the point of this patch is not saving memory, but rather
>> > avoiding an OOM condition that will happen if we have no swap space
>> > left, but some pages left in the swap cache. Of course, the OOM
>> > avoidance will come at the cost of extra work in reclaim to swap those
>> > pages out.
>> >
>> > The only case where I think this might be harmful is if there's plenty
>> > of pages to reclaim on the file LRU, and instead we opt to chase down
>> > the few swap cache pages. So perhaps we can add a check to only set
>> > sc->swapcache_only if the number of pages in the swap cache is more
>> > than the number of pages on the file LRU or similar? Just make sure we
>> > don't chase the swapcache pages down if there's plenty to scan on the
>> > file LRU?
>>
>> The swap cache pages can be divided to 3 groups.
>>
>> - group 1: pages have been written out, at the tail of inactive LRU, but
>>   not reclaimed yet.
>>
>> - group 2: pages have been written out, but were failed to be reclaimed
>>   (e.g., were accessed before reclaiming)
>>
>> - group 3: pages have been swapped in, but were kept in swap cache.  The
>>   pages may be in active LRU.
>>
>> The main target of the original patch should be group 1.  And the pages
>> may be cheaper to reclaim than file pages.
>>
>> Group 2 are hard to be reclaimed if swap_count() isn't 0.
>>
>> Group 3 should be reclaimed in theory, but the overhead may be high.
>> And we may need to reclaim the swap entries instead of pages if the pages
>> are hot.  But we can start to reclaim the swap entries before the swap
>> space is run out.
>>
>> So, if we can count group 1, we may use that as indicator to scan anon
>> pages.  And we may add code to reclaim group 3 earlier.
>>
>
> My point was not that reclaiming the pages in the swap cache is more
> expensive that reclaiming the pages in the file LRU. In a lot of
> cases, as you point out, the pages in the swap cache can just be
> dropped, so they may be as cheap or cheaper to reclaim than the pages
> in the file LRU.
>
> My point was that scanning the anon LRU when swap space is exhausted
> to get to the pages in the swap cache may be much more expensive,
> because there may be a lot of pages on the anon LRU that are not in
> the swap cache, and hence are not reclaimable, unlike pages in the
> file LRU, which should mostly be reclaimable.
>
> So what I am saying is that maybe we should not do the effort of
> scanning the anon LRU in the swapcache_only case unless there aren't a
> lot of pages to reclaim on the file LRU (relatively). For example, if
> we have a 100 pages in the swap cache out of 10000 pages in the anon
> LRU, and there are 10000 pages in the file LRU, it's probably not
> worth scanning the anon LRU.

For group 1 pages, they are at the tail of the anon inactive LRU, so the
scan overhead is low too.  For example, if number of group 1 pages is
100, we just need to scan 100 pages to reclaim them.  We can choose to
stop scanning when the number of the non-group-1 pages reached some
threshold.

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Yosry Ahmed]

On Mon, Nov 27, 2023 at 8:05 PM Huang, Ying <ying.huang@intel.com> wrote:
>
> Yosry Ahmed <yosryahmed@google.com> writes:
>
> > On Mon, Nov 27, 2023 at 7:21 PM Huang, Ying <ying.huang@intel.com> wrote:
> >>
> >> Yosry Ahmed <yosryahmed@google.com> writes:
> >>
> >> > On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
> >> >>
> >> >> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
> >> >> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
> >> >> > > >  I agree with Ying that anonymous pages typically have different page
> >> >> > > > access patterns than file pages, so we might want to treat them
> >> >> > > > differently to reclaim them effectively.
> >> >> > > > One random idea:
> >> >> > > > How about we put the anonymous page in a swap cache in a different LRU
> >> >> > > > than the rest of the anonymous pages. Then shrinking against those
> >> >> > > > pages in the swap cache would be more effective.Instead of having
> >> >> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
> >> >> > > > cache, file] LRU
> >> >> > >
> >> >> > > I don't think that it is necessary.  The patch is only for a special use
> >> >> > > case.  Where the swap device is used up while some pages are in swap
> >> >> > > cache.  The patch will kill performance, but it is used to avoid OOM
> >> >> > > only, not to improve performance.  Per my understanding, we will not use
> >> >> > > up swap device space in most cases.  This may be true for ZRAM, but will
> >> >> > > we keep pages in swap cache for long when we use ZRAM?
> >> >> >
> >> >> > I ask the question regarding how many pages can be freed by this patch
> >> >> > in this email thread as well, but haven't got the answer from the
> >> >> > author yet. That is one important aspect to evaluate how valuable is
> >> >> > that patch.
> >> >>
> >> >> Exactly. Since swap cache has different life time with page cache, they
> >> >> would be usually dropped when pages are unmapped(unless they are shared
> >> >> with others but anon is usually exclusive private) so I wonder how much
> >> >> memory we can save.
> >> >
> >> > I think the point of this patch is not saving memory, but rather
> >> > avoiding an OOM condition that will happen if we have no swap space
> >> > left, but some pages left in the swap cache. Of course, the OOM
> >> > avoidance will come at the cost of extra work in reclaim to swap those
> >> > pages out.
> >> >
> >> > The only case where I think this might be harmful is if there's plenty
> >> > of pages to reclaim on the file LRU, and instead we opt to chase down
> >> > the few swap cache pages. So perhaps we can add a check to only set
> >> > sc->swapcache_only if the number of pages in the swap cache is more
> >> > than the number of pages on the file LRU or similar? Just make sure we
> >> > don't chase the swapcache pages down if there's plenty to scan on the
> >> > file LRU?
> >>
> >> The swap cache pages can be divided to 3 groups.
> >>
> >> - group 1: pages have been written out, at the tail of inactive LRU, but
> >>   not reclaimed yet.
> >>
> >> - group 2: pages have been written out, but were failed to be reclaimed
> >>   (e.g., were accessed before reclaiming)
> >>
> >> - group 3: pages have been swapped in, but were kept in swap cache.  The
> >>   pages may be in active LRU.
> >>
> >> The main target of the original patch should be group 1.  And the pages
> >> may be cheaper to reclaim than file pages.
> >>
> >> Group 2 are hard to be reclaimed if swap_count() isn't 0.
> >>
> >> Group 3 should be reclaimed in theory, but the overhead may be high.
> >> And we may need to reclaim the swap entries instead of pages if the pages
> >> are hot.  But we can start to reclaim the swap entries before the swap
> >> space is run out.
> >>
> >> So, if we can count group 1, we may use that as indicator to scan anon
> >> pages.  And we may add code to reclaim group 3 earlier.
> >>
> >
> > My point was not that reclaiming the pages in the swap cache is more
> > expensive that reclaiming the pages in the file LRU. In a lot of
> > cases, as you point out, the pages in the swap cache can just be
> > dropped, so they may be as cheap or cheaper to reclaim than the pages
> > in the file LRU.
> >
> > My point was that scanning the anon LRU when swap space is exhausted
> > to get to the pages in the swap cache may be much more expensive,
> > because there may be a lot of pages on the anon LRU that are not in
> > the swap cache, and hence are not reclaimable, unlike pages in the
> > file LRU, which should mostly be reclaimable.
> >
> > So what I am saying is that maybe we should not do the effort of
> > scanning the anon LRU in the swapcache_only case unless there aren't a
> > lot of pages to reclaim on the file LRU (relatively). For example, if
> > we have a 100 pages in the swap cache out of 10000 pages in the anon
> > LRU, and there are 10000 pages in the file LRU, it's probably not
> > worth scanning the anon LRU.
>
> For group 1 pages, they are at the tail of the anon inactive LRU, so the
> scan overhead is low too.  For example, if number of group 1 pages is
> 100, we just need to scan 100 pages to reclaim them.  We can choose to
> stop scanning when the number of the non-group-1 pages reached some
> threshold.
>

We should still try to reclaim pages in groups 2 & 3 before OOMing
though. Maybe the motivation for this patch is group 1, but I don't
see why we should special case them. Pages in groups 2 & 3 should be
roughly equally cheap to reclaim. They may have higher refault cost,
but IIUC we should still try to reclaim them before OOMing.

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Yosry Ahmed <yosryahmed@google.com> writes:

> On Mon, Nov 27, 2023 at 8:05 PM Huang, Ying <ying.huang@intel.com> wrote:
>>
>> Yosry Ahmed <yosryahmed@google.com> writes:
>>
>> > On Mon, Nov 27, 2023 at 7:21 PM Huang, Ying <ying.huang@intel.com> wrote:
>> >>
>> >> Yosry Ahmed <yosryahmed@google.com> writes:
>> >>
>> >> > On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
>> >> >>
>> >> >> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
>> >> >> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
>> >> >> > > >  I agree with Ying that anonymous pages typically have different page
>> >> >> > > > access patterns than file pages, so we might want to treat them
>> >> >> > > > differently to reclaim them effectively.
>> >> >> > > > One random idea:
>> >> >> > > > How about we put the anonymous page in a swap cache in a different LRU
>> >> >> > > > than the rest of the anonymous pages. Then shrinking against those
>> >> >> > > > pages in the swap cache would be more effective.Instead of having
>> >> >> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
>> >> >> > > > cache, file] LRU
>> >> >> > >
>> >> >> > > I don't think that it is necessary.  The patch is only for a special use
>> >> >> > > case.  Where the swap device is used up while some pages are in swap
>> >> >> > > cache.  The patch will kill performance, but it is used to avoid OOM
>> >> >> > > only, not to improve performance.  Per my understanding, we will not use
>> >> >> > > up swap device space in most cases.  This may be true for ZRAM, but will
>> >> >> > > we keep pages in swap cache for long when we use ZRAM?
>> >> >> >
>> >> >> > I ask the question regarding how many pages can be freed by this patch
>> >> >> > in this email thread as well, but haven't got the answer from the
>> >> >> > author yet. That is one important aspect to evaluate how valuable is
>> >> >> > that patch.
>> >> >>
>> >> >> Exactly. Since swap cache has different life time with page cache, they
>> >> >> would be usually dropped when pages are unmapped(unless they are shared
>> >> >> with others but anon is usually exclusive private) so I wonder how much
>> >> >> memory we can save.
>> >> >
>> >> > I think the point of this patch is not saving memory, but rather
>> >> > avoiding an OOM condition that will happen if we have no swap space
>> >> > left, but some pages left in the swap cache. Of course, the OOM
>> >> > avoidance will come at the cost of extra work in reclaim to swap those
>> >> > pages out.
>> >> >
>> >> > The only case where I think this might be harmful is if there's plenty
>> >> > of pages to reclaim on the file LRU, and instead we opt to chase down
>> >> > the few swap cache pages. So perhaps we can add a check to only set
>> >> > sc->swapcache_only if the number of pages in the swap cache is more
>> >> > than the number of pages on the file LRU or similar? Just make sure we
>> >> > don't chase the swapcache pages down if there's plenty to scan on the
>> >> > file LRU?
>> >>
>> >> The swap cache pages can be divided to 3 groups.
>> >>
>> >> - group 1: pages have been written out, at the tail of inactive LRU, but
>> >>   not reclaimed yet.
>> >>
>> >> - group 2: pages have been written out, but were failed to be reclaimed
>> >>   (e.g., were accessed before reclaiming)
>> >>
>> >> - group 3: pages have been swapped in, but were kept in swap cache.  The
>> >>   pages may be in active LRU.
>> >>
>> >> The main target of the original patch should be group 1.  And the pages
>> >> may be cheaper to reclaim than file pages.
>> >>
>> >> Group 2 are hard to be reclaimed if swap_count() isn't 0.
>> >>
>> >> Group 3 should be reclaimed in theory, but the overhead may be high.
>> >> And we may need to reclaim the swap entries instead of pages if the pages
>> >> are hot.  But we can start to reclaim the swap entries before the swap
>> >> space is run out.
>> >>
>> >> So, if we can count group 1, we may use that as indicator to scan anon
>> >> pages.  And we may add code to reclaim group 3 earlier.
>> >>
>> >
>> > My point was not that reclaiming the pages in the swap cache is more
>> > expensive that reclaiming the pages in the file LRU. In a lot of
>> > cases, as you point out, the pages in the swap cache can just be
>> > dropped, so they may be as cheap or cheaper to reclaim than the pages
>> > in the file LRU.
>> >
>> > My point was that scanning the anon LRU when swap space is exhausted
>> > to get to the pages in the swap cache may be much more expensive,
>> > because there may be a lot of pages on the anon LRU that are not in
>> > the swap cache, and hence are not reclaimable, unlike pages in the
>> > file LRU, which should mostly be reclaimable.
>> >
>> > So what I am saying is that maybe we should not do the effort of
>> > scanning the anon LRU in the swapcache_only case unless there aren't a
>> > lot of pages to reclaim on the file LRU (relatively). For example, if
>> > we have a 100 pages in the swap cache out of 10000 pages in the anon
>> > LRU, and there are 10000 pages in the file LRU, it's probably not
>> > worth scanning the anon LRU.
>>
>> For group 1 pages, they are at the tail of the anon inactive LRU, so the
>> scan overhead is low too.  For example, if number of group 1 pages is
>> 100, we just need to scan 100 pages to reclaim them.  We can choose to
>> stop scanning when the number of the non-group-1 pages reached some
>> threshold.
>>
>
> We should still try to reclaim pages in groups 2 & 3 before OOMing
> though. Maybe the motivation for this patch is group 1, but I don't
> see why we should special case them. Pages in groups 2 & 3 should be
> roughly equally cheap to reclaim. They may have higher refault cost,
> but IIUC we should still try to reclaim them before OOMing.

The scan cost of group 3 may be high, you may need to scan all anonymous
pages to identify them.  The reclaim cost of group 2 may be high, it may
just cause trashing (shared pages that are accessed by just one
process).  So I think that we can allow reclaim group 1 in all cases.
Try to reclaim swap entries for group 3 during normal LRU scanning after
more than half of swap space of limit is used.  As a last resort before
OOM, try to reclaim group 2 and group 3.  Or, limit scan count for group
2 and group 3.

BTW, in some situation, OOM is not the worst situation.  For example,
trashing may kill interaction latency, while killing the memory hog (may
be caused by memory leak) saves system response time.

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Yosry Ahmed]

On Mon, Nov 27, 2023 at 9:39 PM Huang, Ying <ying.huang@intel.com> wrote:
>
> Yosry Ahmed <yosryahmed@google.com> writes:
>
> > On Mon, Nov 27, 2023 at 8:05 PM Huang, Ying <ying.huang@intel.com> wrote:
> >>
> >> Yosry Ahmed <yosryahmed@google.com> writes:
> >>
> >> > On Mon, Nov 27, 2023 at 7:21 PM Huang, Ying <ying.huang@intel.com> wrote:
> >> >>
> >> >> Yosry Ahmed <yosryahmed@google.com> writes:
> >> >>
> >> >> > On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
> >> >> >>
> >> >> >> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
> >> >> >> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
> >> >> >> > > >  I agree with Ying that anonymous pages typically have different page
> >> >> >> > > > access patterns than file pages, so we might want to treat them
> >> >> >> > > > differently to reclaim them effectively.
> >> >> >> > > > One random idea:
> >> >> >> > > > How about we put the anonymous page in a swap cache in a different LRU
> >> >> >> > > > than the rest of the anonymous pages. Then shrinking against those
> >> >> >> > > > pages in the swap cache would be more effective.Instead of having
> >> >> >> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
> >> >> >> > > > cache, file] LRU
> >> >> >> > >
> >> >> >> > > I don't think that it is necessary.  The patch is only for a special use
> >> >> >> > > case.  Where the swap device is used up while some pages are in swap
> >> >> >> > > cache.  The patch will kill performance, but it is used to avoid OOM
> >> >> >> > > only, not to improve performance.  Per my understanding, we will not use
> >> >> >> > > up swap device space in most cases.  This may be true for ZRAM, but will
> >> >> >> > > we keep pages in swap cache for long when we use ZRAM?
> >> >> >> >
> >> >> >> > I ask the question regarding how many pages can be freed by this patch
> >> >> >> > in this email thread as well, but haven't got the answer from the
> >> >> >> > author yet. That is one important aspect to evaluate how valuable is
> >> >> >> > that patch.
> >> >> >>
> >> >> >> Exactly. Since swap cache has different life time with page cache, they
> >> >> >> would be usually dropped when pages are unmapped(unless they are shared
> >> >> >> with others but anon is usually exclusive private) so I wonder how much
> >> >> >> memory we can save.
> >> >> >
> >> >> > I think the point of this patch is not saving memory, but rather
> >> >> > avoiding an OOM condition that will happen if we have no swap space
> >> >> > left, but some pages left in the swap cache. Of course, the OOM
> >> >> > avoidance will come at the cost of extra work in reclaim to swap those
> >> >> > pages out.
> >> >> >
> >> >> > The only case where I think this might be harmful is if there's plenty
> >> >> > of pages to reclaim on the file LRU, and instead we opt to chase down
> >> >> > the few swap cache pages. So perhaps we can add a check to only set
> >> >> > sc->swapcache_only if the number of pages in the swap cache is more
> >> >> > than the number of pages on the file LRU or similar? Just make sure we
> >> >> > don't chase the swapcache pages down if there's plenty to scan on the
> >> >> > file LRU?
> >> >>
> >> >> The swap cache pages can be divided to 3 groups.
> >> >>
> >> >> - group 1: pages have been written out, at the tail of inactive LRU, but
> >> >>   not reclaimed yet.
> >> >>
> >> >> - group 2: pages have been written out, but were failed to be reclaimed
> >> >>   (e.g., were accessed before reclaiming)
> >> >>
> >> >> - group 3: pages have been swapped in, but were kept in swap cache.  The
> >> >>   pages may be in active LRU.
> >> >>
> >> >> The main target of the original patch should be group 1.  And the pages
> >> >> may be cheaper to reclaim than file pages.
> >> >>
> >> >> Group 2 are hard to be reclaimed if swap_count() isn't 0.
> >> >>
> >> >> Group 3 should be reclaimed in theory, but the overhead may be high.
> >> >> And we may need to reclaim the swap entries instead of pages if the pages
> >> >> are hot.  But we can start to reclaim the swap entries before the swap
> >> >> space is run out.
> >> >>
> >> >> So, if we can count group 1, we may use that as indicator to scan anon
> >> >> pages.  And we may add code to reclaim group 3 earlier.
> >> >>
> >> >
> >> > My point was not that reclaiming the pages in the swap cache is more
> >> > expensive that reclaiming the pages in the file LRU. In a lot of
> >> > cases, as you point out, the pages in the swap cache can just be
> >> > dropped, so they may be as cheap or cheaper to reclaim than the pages
> >> > in the file LRU.
> >> >
> >> > My point was that scanning the anon LRU when swap space is exhausted
> >> > to get to the pages in the swap cache may be much more expensive,
> >> > because there may be a lot of pages on the anon LRU that are not in
> >> > the swap cache, and hence are not reclaimable, unlike pages in the
> >> > file LRU, which should mostly be reclaimable.
> >> >
> >> > So what I am saying is that maybe we should not do the effort of
> >> > scanning the anon LRU in the swapcache_only case unless there aren't a
> >> > lot of pages to reclaim on the file LRU (relatively). For example, if
> >> > we have a 100 pages in the swap cache out of 10000 pages in the anon
> >> > LRU, and there are 10000 pages in the file LRU, it's probably not
> >> > worth scanning the anon LRU.
> >>
> >> For group 1 pages, they are at the tail of the anon inactive LRU, so the
> >> scan overhead is low too.  For example, if number of group 1 pages is
> >> 100, we just need to scan 100 pages to reclaim them.  We can choose to
> >> stop scanning when the number of the non-group-1 pages reached some
> >> threshold.
> >>
> >
> > We should still try to reclaim pages in groups 2 & 3 before OOMing
> > though. Maybe the motivation for this patch is group 1, but I don't
> > see why we should special case them. Pages in groups 2 & 3 should be
> > roughly equally cheap to reclaim. They may have higher refault cost,
> > but IIUC we should still try to reclaim them before OOMing.
>
> The scan cost of group 3 may be high, you may need to scan all anonymous
> pages to identify them.  The reclaim cost of group 2 may be high, it may
> just cause trashing (shared pages that are accessed by just one
> process).  So I think that we can allow reclaim group 1 in all cases.
> Try to reclaim swap entries for group 3 during normal LRU scanning after
> more than half of swap space of limit is used.  As a last resort before
> OOM, try to reclaim group 2 and group 3.  Or, limit scan count for group
> 2 and group 3.

It would be nice if this can be done auto-magically without having to
keep track of the groups separately.

>
> BTW, in some situation, OOM is not the worst situation.  For example,
> trashing may kill interaction latency, while killing the memory hog (may
> be caused by memory leak) saves system response time.

I agree that in some situations OOMs are better than thrashing, it's
not an easy problem.

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Yosry Ahmed <yosryahmed@google.com> writes:

> On Mon, Nov 27, 2023 at 9:39 PM Huang, Ying <ying.huang@intel.com> wrote:
>>
>> Yosry Ahmed <yosryahmed@google.com> writes:
>>
>> > On Mon, Nov 27, 2023 at 8:05 PM Huang, Ying <ying.huang@intel.com> wrote:
>> >>
>> >> Yosry Ahmed <yosryahmed@google.com> writes:
>> >>
>> >> > On Mon, Nov 27, 2023 at 7:21 PM Huang, Ying <ying.huang@intel.com> wrote:
>> >> >>
>> >> >> Yosry Ahmed <yosryahmed@google.com> writes:
>> >> >>
>> >> >> > On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
>> >> >> >>
>> >> >> >> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
>> >> >> >> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
>> >> >> >> > > >  I agree with Ying that anonymous pages typically have different page
>> >> >> >> > > > access patterns than file pages, so we might want to treat them
>> >> >> >> > > > differently to reclaim them effectively.
>> >> >> >> > > > One random idea:
>> >> >> >> > > > How about we put the anonymous page in a swap cache in a different LRU
>> >> >> >> > > > than the rest of the anonymous pages. Then shrinking against those
>> >> >> >> > > > pages in the swap cache would be more effective.Instead of having
>> >> >> >> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
>> >> >> >> > > > cache, file] LRU
>> >> >> >> > >
>> >> >> >> > > I don't think that it is necessary.  The patch is only for a special use
>> >> >> >> > > case.  Where the swap device is used up while some pages are in swap
>> >> >> >> > > cache.  The patch will kill performance, but it is used to avoid OOM
>> >> >> >> > > only, not to improve performance.  Per my understanding, we will not use
>> >> >> >> > > up swap device space in most cases.  This may be true for ZRAM, but will
>> >> >> >> > > we keep pages in swap cache for long when we use ZRAM?
>> >> >> >> >
>> >> >> >> > I ask the question regarding how many pages can be freed by this patch
>> >> >> >> > in this email thread as well, but haven't got the answer from the
>> >> >> >> > author yet. That is one important aspect to evaluate how valuable is
>> >> >> >> > that patch.
>> >> >> >>
>> >> >> >> Exactly. Since swap cache has different life time with page cache, they
>> >> >> >> would be usually dropped when pages are unmapped(unless they are shared
>> >> >> >> with others but anon is usually exclusive private) so I wonder how much
>> >> >> >> memory we can save.
>> >> >> >
>> >> >> > I think the point of this patch is not saving memory, but rather
>> >> >> > avoiding an OOM condition that will happen if we have no swap space
>> >> >> > left, but some pages left in the swap cache. Of course, the OOM
>> >> >> > avoidance will come at the cost of extra work in reclaim to swap those
>> >> >> > pages out.
>> >> >> >
>> >> >> > The only case where I think this might be harmful is if there's plenty
>> >> >> > of pages to reclaim on the file LRU, and instead we opt to chase down
>> >> >> > the few swap cache pages. So perhaps we can add a check to only set
>> >> >> > sc->swapcache_only if the number of pages in the swap cache is more
>> >> >> > than the number of pages on the file LRU or similar? Just make sure we
>> >> >> > don't chase the swapcache pages down if there's plenty to scan on the
>> >> >> > file LRU?
>> >> >>
>> >> >> The swap cache pages can be divided to 3 groups.
>> >> >>
>> >> >> - group 1: pages have been written out, at the tail of inactive LRU, but
>> >> >>   not reclaimed yet.
>> >> >>
>> >> >> - group 2: pages have been written out, but were failed to be reclaimed
>> >> >>   (e.g., were accessed before reclaiming)
>> >> >>
>> >> >> - group 3: pages have been swapped in, but were kept in swap cache.  The
>> >> >>   pages may be in active LRU.
>> >> >>
>> >> >> The main target of the original patch should be group 1.  And the pages
>> >> >> may be cheaper to reclaim than file pages.
>> >> >>
>> >> >> Group 2 are hard to be reclaimed if swap_count() isn't 0.
>> >> >>
>> >> >> Group 3 should be reclaimed in theory, but the overhead may be high.
>> >> >> And we may need to reclaim the swap entries instead of pages if the pages
>> >> >> are hot.  But we can start to reclaim the swap entries before the swap
>> >> >> space is run out.
>> >> >>
>> >> >> So, if we can count group 1, we may use that as indicator to scan anon
>> >> >> pages.  And we may add code to reclaim group 3 earlier.
>> >> >>
>> >> >
>> >> > My point was not that reclaiming the pages in the swap cache is more
>> >> > expensive that reclaiming the pages in the file LRU. In a lot of
>> >> > cases, as you point out, the pages in the swap cache can just be
>> >> > dropped, so they may be as cheap or cheaper to reclaim than the pages
>> >> > in the file LRU.
>> >> >
>> >> > My point was that scanning the anon LRU when swap space is exhausted
>> >> > to get to the pages in the swap cache may be much more expensive,
>> >> > because there may be a lot of pages on the anon LRU that are not in
>> >> > the swap cache, and hence are not reclaimable, unlike pages in the
>> >> > file LRU, which should mostly be reclaimable.
>> >> >
>> >> > So what I am saying is that maybe we should not do the effort of
>> >> > scanning the anon LRU in the swapcache_only case unless there aren't a
>> >> > lot of pages to reclaim on the file LRU (relatively). For example, if
>> >> > we have a 100 pages in the swap cache out of 10000 pages in the anon
>> >> > LRU, and there are 10000 pages in the file LRU, it's probably not
>> >> > worth scanning the anon LRU.
>> >>
>> >> For group 1 pages, they are at the tail of the anon inactive LRU, so the
>> >> scan overhead is low too.  For example, if number of group 1 pages is
>> >> 100, we just need to scan 100 pages to reclaim them.  We can choose to
>> >> stop scanning when the number of the non-group-1 pages reached some
>> >> threshold.
>> >>
>> >
>> > We should still try to reclaim pages in groups 2 & 3 before OOMing
>> > though. Maybe the motivation for this patch is group 1, but I don't
>> > see why we should special case them. Pages in groups 2 & 3 should be
>> > roughly equally cheap to reclaim. They may have higher refault cost,
>> > but IIUC we should still try to reclaim them before OOMing.
>>
>> The scan cost of group 3 may be high, you may need to scan all anonymous
>> pages to identify them.  The reclaim cost of group 2 may be high, it may
>> just cause trashing (shared pages that are accessed by just one
>> process).  So I think that we can allow reclaim group 1 in all cases.
>> Try to reclaim swap entries for group 3 during normal LRU scanning after
>> more than half of swap space of limit is used.  As a last resort before
>> OOM, try to reclaim group 2 and group 3.  Or, limit scan count for group
>> 2 and group 3.
>
> It would be nice if this can be done auto-magically without having to
> keep track of the groups separately.

Some rough idea may be

- trying to scan anon LRU if there are swap cache pages.

- if some number of pages other than group 1 encountered, stop scanning
  anon LRU list.

- the threshold to stopping can be tuned according to whether we are
  going to OOM.

We can try to reclaim swap entries for group 3 when we haven't run out
of swap space yet.

>>
>> BTW, in some situation, OOM is not the worst situation.  For example,
>> trashing may kill interaction latency, while killing the memory hog (may
>> be caused by memory leak) saves system response time.
>
> I agree that in some situations OOMs are better than thrashing, it's
> not an easy problem.

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Minchan Kim]

On Tue, Nov 28, 2023 at 11:19:20AM +0800, Huang, Ying wrote:
> Yosry Ahmed <yosryahmed@google.com> writes:
> 
> > On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
> >>
> >> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
> >> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
> >> > > >  I agree with Ying that anonymous pages typically have different page
> >> > > > access patterns than file pages, so we might want to treat them
> >> > > > differently to reclaim them effectively.
> >> > > > One random idea:
> >> > > > How about we put the anonymous page in a swap cache in a different LRU
> >> > > > than the rest of the anonymous pages. Then shrinking against those
> >> > > > pages in the swap cache would be more effective.Instead of having
> >> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
> >> > > > cache, file] LRU
> >> > >
> >> > > I don't think that it is necessary.  The patch is only for a special use
> >> > > case.  Where the swap device is used up while some pages are in swap
> >> > > cache.  The patch will kill performance, but it is used to avoid OOM
> >> > > only, not to improve performance.  Per my understanding, we will not use
> >> > > up swap device space in most cases.  This may be true for ZRAM, but will
> >> > > we keep pages in swap cache for long when we use ZRAM?
> >> >
> >> > I ask the question regarding how many pages can be freed by this patch
> >> > in this email thread as well, but haven't got the answer from the
> >> > author yet. That is one important aspect to evaluate how valuable is
> >> > that patch.
> >>
> >> Exactly. Since swap cache has different life time with page cache, they
> >> would be usually dropped when pages are unmapped(unless they are shared
> >> with others but anon is usually exclusive private) so I wonder how much
> >> memory we can save.
> >
> > I think the point of this patch is not saving memory, but rather
> > avoiding an OOM condition that will happen if we have no swap space
> > left, but some pages left in the swap cache. Of course, the OOM
> > avoidance will come at the cost of extra work in reclaim to swap those
> > pages out.
> >
> > The only case where I think this might be harmful is if there's plenty
> > of pages to reclaim on the file LRU, and instead we opt to chase down
> > the few swap cache pages. So perhaps we can add a check to only set
> > sc->swapcache_only if the number of pages in the swap cache is more
> > than the number of pages on the file LRU or similar? Just make sure we
> > don't chase the swapcache pages down if there's plenty to scan on the
> > file LRU?
> 
> The swap cache pages can be divided to 3 groups.
> 
> - group 1: pages have been written out, at the tail of inactive LRU, but
>   not reclaimed yet.
> 
> - group 2: pages have been written out, but were failed to be reclaimed
>   (e.g., were accessed before reclaiming)
> 
> - group 3: pages have been swapped in, but were kept in swap cache.  The
>   pages may be in active LRU.
> 
> The main target of the original patch should be group 1.  And the pages
> may be cheaper to reclaim than file pages.

Yeah, that's common for asynchronous swap devices and that's popular. Then,
How about freeing those memory as soon as the writeback is done instead of
keep adding more tricks to solve the issue?

https://lkml.kernel.org/linux-mm/1368411048-3753-1-git-send-email-minchan@kernel.org/

I remember it's under softIRQ context so there were some issues to change
locking rules for memcg and swap. And there was some concern to increase
softirq latency due to page freeing but both were not the main obstacle to
be fixed.

> 
> Group 2 are hard to be reclaimed if swap_count() isn't 0.

"were accessed before reclaiming" would be rare.

> 
> Group 3 should be reclaimed in theory, but the overhead may be high.
> And we may need to reclaim the swap entries instead of pages if the pages
> are hot.  But we can start to reclaim the swap entries before the swap
> space is run out.

I thought the swap-in path will reclaim the swap slots once it detects
swapspace wasn't enough(e.g., vm_swap_full or mem_cgroup_swap-full)?

> 
> So, if we can count group 1, we may use that as indicator to scan anon
> pages.  And we may add code to reclaim group 3 earlier.

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Minchan Kim <minchan@kernel.org> writes:

> On Tue, Nov 28, 2023 at 11:19:20AM +0800, Huang, Ying wrote:
>> Yosry Ahmed <yosryahmed@google.com> writes:
>> 
>> > On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
>> >>
>> >> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
>> >> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
>> >> > > >  I agree with Ying that anonymous pages typically have different page
>> >> > > > access patterns than file pages, so we might want to treat them
>> >> > > > differently to reclaim them effectively.
>> >> > > > One random idea:
>> >> > > > How about we put the anonymous page in a swap cache in a different LRU
>> >> > > > than the rest of the anonymous pages. Then shrinking against those
>> >> > > > pages in the swap cache would be more effective.Instead of having
>> >> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
>> >> > > > cache, file] LRU
>> >> > >
>> >> > > I don't think that it is necessary.  The patch is only for a special use
>> >> > > case.  Where the swap device is used up while some pages are in swap
>> >> > > cache.  The patch will kill performance, but it is used to avoid OOM
>> >> > > only, not to improve performance.  Per my understanding, we will not use
>> >> > > up swap device space in most cases.  This may be true for ZRAM, but will
>> >> > > we keep pages in swap cache for long when we use ZRAM?
>> >> >
>> >> > I ask the question regarding how many pages can be freed by this patch
>> >> > in this email thread as well, but haven't got the answer from the
>> >> > author yet. That is one important aspect to evaluate how valuable is
>> >> > that patch.
>> >>
>> >> Exactly. Since swap cache has different life time with page cache, they
>> >> would be usually dropped when pages are unmapped(unless they are shared
>> >> with others but anon is usually exclusive private) so I wonder how much
>> >> memory we can save.
>> >
>> > I think the point of this patch is not saving memory, but rather
>> > avoiding an OOM condition that will happen if we have no swap space
>> > left, but some pages left in the swap cache. Of course, the OOM
>> > avoidance will come at the cost of extra work in reclaim to swap those
>> > pages out.
>> >
>> > The only case where I think this might be harmful is if there's plenty
>> > of pages to reclaim on the file LRU, and instead we opt to chase down
>> > the few swap cache pages. So perhaps we can add a check to only set
>> > sc->swapcache_only if the number of pages in the swap cache is more
>> > than the number of pages on the file LRU or similar? Just make sure we
>> > don't chase the swapcache pages down if there's plenty to scan on the
>> > file LRU?
>> 
>> The swap cache pages can be divided to 3 groups.
>> 
>> - group 1: pages have been written out, at the tail of inactive LRU, but
>>   not reclaimed yet.
>> 
>> - group 2: pages have been written out, but were failed to be reclaimed
>>   (e.g., were accessed before reclaiming)
>> 
>> - group 3: pages have been swapped in, but were kept in swap cache.  The
>>   pages may be in active LRU.
>> 
>> The main target of the original patch should be group 1.  And the pages
>> may be cheaper to reclaim than file pages.
>
> Yeah, that's common for asynchronous swap devices and that's popular. Then,
> How about freeing those memory as soon as the writeback is done instead of
> keep adding more tricks to solve the issue?
>
> https://lkml.kernel.org/linux-mm/1368411048-3753-1-git-send-email-minchan@kernel.org/
>
> I remember it's under softIRQ context so there were some issues to change
> locking rules for memcg and swap. And there was some concern to increase
> softirq latency due to page freeing but both were not the main obstacle to
> be fixed.

Thanks for sharing.  It's good to avoid to add the pages back to LRU,
then isolate them from LRU.  I have concerns that is it possible that
too many pages are reclaimed?  For example, to reclaim a small number of
pages, too many pages were written to disk because the performance
difference between CPU and storage.  Originally, we still only reclaim
requested number of pages although much more were written.  But with the
change, we may reclaim them all.

>> 
>> Group 2 are hard to be reclaimed if swap_count() isn't 0.
>
> "were accessed before reclaiming" would be rare.

If page reclaiming algorithm works well enough, that should be true.

>> 
>> Group 3 should be reclaimed in theory, but the overhead may be high.
>> And we may need to reclaim the swap entries instead of pages if the pages
>> are hot.  But we can start to reclaim the swap entries before the swap
>> space is run out.
>
> I thought the swap-in path will reclaim the swap slots once it detects
> swapspace wasn't enough(e.g., vm_swap_full or mem_cgroup_swap-full)?

Yes.  You are right.  But before swap space wasn't enough, we may keep
quite some pages in swap cache.  But these pages may becomes hot later.
Then we have no opportunity to reclaim these swap space.  So, we may
need to add some code to check this situation at appropriate places.
For example, when we scan pages in active list, or activate pages in
inactive list.

>> 
>> So, if we can count group 1, we may use that as indicator to scan anon
>> pages.  And we may add code to reclaim group 3 earlier.

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Michal Hocko]

On Tue 28-11-23 11:19:20, Huang, Ying wrote:
> Yosry Ahmed <yosryahmed@google.com> writes:
> 
> > On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
> >>
> >> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
> >> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
> >> > > >  I agree with Ying that anonymous pages typically have different page
> >> > > > access patterns than file pages, so we might want to treat them
> >> > > > differently to reclaim them effectively.
> >> > > > One random idea:
> >> > > > How about we put the anonymous page in a swap cache in a different LRU
> >> > > > than the rest of the anonymous pages. Then shrinking against those
> >> > > > pages in the swap cache would be more effective.Instead of having
> >> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
> >> > > > cache, file] LRU
> >> > >
> >> > > I don't think that it is necessary.  The patch is only for a special use
> >> > > case.  Where the swap device is used up while some pages are in swap
> >> > > cache.  The patch will kill performance, but it is used to avoid OOM
> >> > > only, not to improve performance.  Per my understanding, we will not use
> >> > > up swap device space in most cases.  This may be true for ZRAM, but will
> >> > > we keep pages in swap cache for long when we use ZRAM?
> >> >
> >> > I ask the question regarding how many pages can be freed by this patch
> >> > in this email thread as well, but haven't got the answer from the
> >> > author yet. That is one important aspect to evaluate how valuable is
> >> > that patch.
> >>
> >> Exactly. Since swap cache has different life time with page cache, they
> >> would be usually dropped when pages are unmapped(unless they are shared
> >> with others but anon is usually exclusive private) so I wonder how much
> >> memory we can save.
> >
> > I think the point of this patch is not saving memory, but rather
> > avoiding an OOM condition that will happen if we have no swap space
> > left, but some pages left in the swap cache. Of course, the OOM
> > avoidance will come at the cost of extra work in reclaim to swap those
> > pages out.
> >
> > The only case where I think this might be harmful is if there's plenty
> > of pages to reclaim on the file LRU, and instead we opt to chase down
> > the few swap cache pages. So perhaps we can add a check to only set
> > sc->swapcache_only if the number of pages in the swap cache is more
> > than the number of pages on the file LRU or similar? Just make sure we
> > don't chase the swapcache pages down if there's plenty to scan on the
> > file LRU?
> 
> The swap cache pages can be divided to 3 groups.
> 
> - group 1: pages have been written out, at the tail of inactive LRU, but
>   not reclaimed yet.
> 
> - group 2: pages have been written out, but were failed to be reclaimed
>   (e.g., were accessed before reclaiming)
> 
> - group 3: pages have been swapped in, but were kept in swap cache.  The
>   pages may be in active LRU.
> 
> The main target of the original patch should be group 1.  And the pages
> may be cheaper to reclaim than file pages.

Thanks this is really useful summary. And it begs question. How are we
telling those different types from each other? vmstat counter is
certainly not sufficient and that means we might be scanning a lot
without actually making any progress. And doing that repeatedly.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Michal Hocko <mhocko@suse.com> writes:

> On Tue 28-11-23 11:19:20, Huang, Ying wrote:
>> Yosry Ahmed <yosryahmed@google.com> writes:
>> 
>> > On Mon, Nov 27, 2023 at 1:32 PM Minchan Kim <minchan@kernel.org> wrote:
>> >>
>> >> On Mon, Nov 27, 2023 at 12:22:59AM -0800, Chris Li wrote:
>> >> > On Mon, Nov 27, 2023 at 12:14 AM Huang, Ying <ying.huang@intel.com> wrote:
>> >> > > >  I agree with Ying that anonymous pages typically have different page
>> >> > > > access patterns than file pages, so we might want to treat them
>> >> > > > differently to reclaim them effectively.
>> >> > > > One random idea:
>> >> > > > How about we put the anonymous page in a swap cache in a different LRU
>> >> > > > than the rest of the anonymous pages. Then shrinking against those
>> >> > > > pages in the swap cache would be more effective.Instead of having
>> >> > > > [anon, file] LRU, now we have [anon not in swap cache, anon in swap
>> >> > > > cache, file] LRU
>> >> > >
>> >> > > I don't think that it is necessary.  The patch is only for a special use
>> >> > > case.  Where the swap device is used up while some pages are in swap
>> >> > > cache.  The patch will kill performance, but it is used to avoid OOM
>> >> > > only, not to improve performance.  Per my understanding, we will not use
>> >> > > up swap device space in most cases.  This may be true for ZRAM, but will
>> >> > > we keep pages in swap cache for long when we use ZRAM?
>> >> >
>> >> > I ask the question regarding how many pages can be freed by this patch
>> >> > in this email thread as well, but haven't got the answer from the
>> >> > author yet. That is one important aspect to evaluate how valuable is
>> >> > that patch.
>> >>
>> >> Exactly. Since swap cache has different life time with page cache, they
>> >> would be usually dropped when pages are unmapped(unless they are shared
>> >> with others but anon is usually exclusive private) so I wonder how much
>> >> memory we can save.
>> >
>> > I think the point of this patch is not saving memory, but rather
>> > avoiding an OOM condition that will happen if we have no swap space
>> > left, but some pages left in the swap cache. Of course, the OOM
>> > avoidance will come at the cost of extra work in reclaim to swap those
>> > pages out.
>> >
>> > The only case where I think this might be harmful is if there's plenty
>> > of pages to reclaim on the file LRU, and instead we opt to chase down
>> > the few swap cache pages. So perhaps we can add a check to only set
>> > sc->swapcache_only if the number of pages in the swap cache is more
>> > than the number of pages on the file LRU or similar? Just make sure we
>> > don't chase the swapcache pages down if there's plenty to scan on the
>> > file LRU?
>> 
>> The swap cache pages can be divided to 3 groups.
>> 
>> - group 1: pages have been written out, at the tail of inactive LRU, but
>>   not reclaimed yet.
>> 
>> - group 2: pages have been written out, but were failed to be reclaimed
>>   (e.g., were accessed before reclaiming)
>> 
>> - group 3: pages have been swapped in, but were kept in swap cache.  The
>>   pages may be in active LRU.
>> 
>> The main target of the original patch should be group 1.  And the pages
>> may be cheaper to reclaim than file pages.
>
> Thanks this is really useful summary. And it begs question. How are we
> telling those different types from each other? vmstat counter is
> certainly not sufficient and that means we might be scanning a lot
> without actually making any progress. And doing that repeatedly.

We don't have counters for pages in individual groups.  Pages in group 1
and some pages in group 2 are always at the tail of inactive LRU.  So,
we can identify them relatively easily.  So a simple method could be, if
there are swap cache, try scan the tail of inactive LRU to free pages in
group 1 and move pages in group 2.  If we found some pages aren't in
swap cache, there may be no pages in group 1.  Then, we may give up
scanning if the memory pressure isn't too large.

One possible issue is that some pages (map count == 1) may be swapped
out, swapped in, then deleted from swap cache.  So, some pages not in
swap cache may be at the end of inactive LRU too.

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Chris Li]

On Mon, Nov 27, 2023 at 1:57 PM Yosry Ahmed <yosryahmed@google.com> wrote:

> >
> > Exactly. Since swap cache has different life time with page cache, they
> > would be usually dropped when pages are unmapped(unless they are shared
> > with others but anon is usually exclusive private) so I wonder how much
> > memory we can save.
>
> I think the point of this patch is not saving memory, but rather
> avoiding an OOM condition that will happen if we have no swap space
> left, but some pages left in the swap cache. Of course, the OOM
> avoidance will come at the cost of extra work in reclaim to swap those
> pages out.

You are discussing how to use the memory that got freed. e.g. using
other apps so avoid OOM.
I am asking how much memory can be freed by this patch. That number is
still useful to understand how effective the patch is.  Does it
justify the additional complexity?

> The only case where I think this might be harmful is if there's plenty
> of pages to reclaim on the file LRU, and instead we opt to chase down
> the few swap cache pages. So perhaps we can add a check to only set
> sc->swapcache_only if the number of pages in the swap cache is more
> than the number of pages on the file LRU or similar? Just make sure we
> don't chase the swapcache pages down if there's plenty to scan on the
> file LRU?

One idea is that we need to measure how effective the reclaim was
toward the swap cache. If we do a lot of work but are not able to
reclaim much from the swap cache, then additional reclaim work on swap
cache is not going to be effective.
MGLRU has a PID controller to adjust the reclaim ratio between the
file and anonymous LRU. That is one way of measuring the effectiveness
of the reclaim. Use the feedback to adjust the follow up reclaim work.
I kind of wish we could have some feedback machine like this for the
swap cache reclaim as well. Not sure how complicated it is to
implement one.

Chris

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Michal Hocko]

On Mon 27-11-23 10:34:46, Huang, Ying wrote:
> Michal Hocko <mhocko@suse.com> writes:
> 
> > On Thu 23-11-23 14:15:59, Huang, Ying wrote:
> >> Michal Hocko <mhocko@suse.com> writes:
> > [...]
> >> > Yes swapcache pages are indeed more complicated but most of the time
> >> > they just go away as well, no?
> >> 
> >> When we swapin a page, we will put it in swapcache too.  And the page
> >> can be in that state for long time if there is more than 50% free space
> >> in the swap device.
> >
> > True, but why is that a problem? If you encounter such a swapped in page
> > on the file LRU then the page should be referened and as such should be
> > activated, no?
> 
> This just means that anonymous pages in file LRU aren't temporary or
> short-term.  So we need to consider that.

Right. On the other hand we could be more aggressive when dropping the
swapcache. Is there any actual reason why we cannot try to folio_free_swap
even when mem_cgroup_swap_full == F?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Michal Hocko <mhocko@suse.com> writes:

> On Mon 27-11-23 10:34:46, Huang, Ying wrote:
>> Michal Hocko <mhocko@suse.com> writes:
>> 
>> > On Thu 23-11-23 14:15:59, Huang, Ying wrote:
>> >> Michal Hocko <mhocko@suse.com> writes:
>> > [...]
>> >> > Yes swapcache pages are indeed more complicated but most of the time
>> >> > they just go away as well, no?
>> >> 
>> >> When we swapin a page, we will put it in swapcache too.  And the page
>> >> can be in that state for long time if there is more than 50% free space
>> >> in the swap device.
>> >
>> > True, but why is that a problem? If you encounter such a swapped in page
>> > on the file LRU then the page should be referened and as such should be
>> > activated, no?
>> 
>> This just means that anonymous pages in file LRU aren't temporary or
>> short-term.  So we need to consider that.
>
> Right. On the other hand we could be more aggressive when dropping the
> swapcache. Is there any actual reason why we cannot try to folio_free_swap
> even when mem_cgroup_swap_full == F?

If there are plenty free space in swap device, why not take advantage of
it?  If the page is in swap cache and clean, we can discard it when it
is reclaimed, or avoid to allocate swap entry for it.

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Michal Hocko]

On Tue 28-11-23 09:31:06, Huang, Ying wrote:
> Michal Hocko <mhocko@suse.com> writes:
[...]
> > Right. On the other hand we could be more aggressive when dropping the
> > swapcache. Is there any actual reason why we cannot try to folio_free_swap
> > even when mem_cgroup_swap_full == F?
> 
> If there are plenty free space in swap device, why not take advantage of
> it?

Maybe a stupid question but what is the advantage of keeping around in
the swap cache?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Minchan Kim]

On Tue, Nov 28, 2023 at 11:16:04AM +0100, Michal Hocko wrote:
> On Tue 28-11-23 09:31:06, Huang, Ying wrote:
> > Michal Hocko <mhocko@suse.com> writes:
> [...]
> > > Right. On the other hand we could be more aggressive when dropping the
> > > swapcache. Is there any actual reason why we cannot try to folio_free_swap
> > > even when mem_cgroup_swap_full == F?
> > 
> > If there are plenty free space in swap device, why not take advantage of
> > it?
> 
> Maybe a stupid question but what is the advantage of keeping around in
> the swap cache?

If the page is shared, we avoids addtional IO to bring them back so
swap cache.

If the page is still clean on reclaim moment since swap-in, VM doesn't
need to write them out to the swap disk since original data is already
there.

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Yosry Ahmed]

On Tue, Nov 28, 2023 at 2:45 PM Minchan Kim <minchan@kernel.org> wrote:
>
> On Tue, Nov 28, 2023 at 11:16:04AM +0100, Michal Hocko wrote:
> > On Tue 28-11-23 09:31:06, Huang, Ying wrote:
> > > Michal Hocko <mhocko@suse.com> writes:
> > [...]
> > > > Right. On the other hand we could be more aggressive when dropping the
> > > > swapcache. Is there any actual reason why we cannot try to folio_free_swap
> > > > even when mem_cgroup_swap_full == F?
> > >
> > > If there are plenty free space in swap device, why not take advantage of
> > > it?
> >
> > Maybe a stupid question but what is the advantage of keeping around in
> > the swap cache?
>
> If the page is shared, we avoids addtional IO to bring them back so
> swap cache.

I think this case is actually necessary for correctness, not just to
avoid additional IO. Otherwise subsequent swapins will create new
copies of the page, right?

>
> If the page is still clean on reclaim moment since swap-in, VM doesn't
> need to write them out to the swap disk since original data is already
> there.

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Minchan Kim]

On Tue, Nov 28, 2023 at 03:05:29PM -0800, Yosry Ahmed wrote:
> On Tue, Nov 28, 2023 at 2:45 PM Minchan Kim <minchan@kernel.org> wrote:
> >
> > On Tue, Nov 28, 2023 at 11:16:04AM +0100, Michal Hocko wrote:
> > > On Tue 28-11-23 09:31:06, Huang, Ying wrote:
> > > > Michal Hocko <mhocko@suse.com> writes:
> > > [...]
> > > > > Right. On the other hand we could be more aggressive when dropping the
> > > > > swapcache. Is there any actual reason why we cannot try to folio_free_swap
> > > > > even when mem_cgroup_swap_full == F?
> > > >
> > > > If there are plenty free space in swap device, why not take advantage of
> > > > it?
> > >
> > > Maybe a stupid question but what is the advantage of keeping around in
> > > the swap cache?
> >
> > If the page is shared, we avoids addtional IO to bring them back so
> > swap cache.
> 
> I think this case is actually necessary for correctness, not just to
> avoid additional IO. Otherwise subsequent swapins will create new
> copies of the page, right?

I think if the page was shared by MAP_SHARED, then, yes.
I think if the page was shared by MAP_PRIVATE but CoW(e.g., fork), then, no.

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Michal Hocko]

On Tue 28-11-23 15:15:08, Minchan Kim wrote:
> On Tue, Nov 28, 2023 at 03:05:29PM -0800, Yosry Ahmed wrote:
> > On Tue, Nov 28, 2023 at 2:45 PM Minchan Kim <minchan@kernel.org> wrote:
> > >
> > > On Tue, Nov 28, 2023 at 11:16:04AM +0100, Michal Hocko wrote:
> > > > On Tue 28-11-23 09:31:06, Huang, Ying wrote:
> > > > > Michal Hocko <mhocko@suse.com> writes:
> > > > [...]
> > > > > > Right. On the other hand we could be more aggressive when dropping the
> > > > > > swapcache. Is there any actual reason why we cannot try to folio_free_swap
> > > > > > even when mem_cgroup_swap_full == F?
> > > > >
> > > > > If there are plenty free space in swap device, why not take advantage of
> > > > > it?
> > > >
> > > > Maybe a stupid question but what is the advantage of keeping around in
> > > > the swap cache?
> > >
> > > If the page is shared, we avoids addtional IO to bring them back so
> > > swap cache.
> > 
> > I think this case is actually necessary for correctness, not just to
> > avoid additional IO. Otherwise subsequent swapins will create new
> > copies of the page, right?
> 
> I think if the page was shared by MAP_SHARED, then, yes.

In that case deleting from the swap cache would fail because there are
still swapped out ptes, no?

> I think if the page was shared by MAP_PRIVATE but CoW(e.g., fork), then, no.

OK, but we are talking about a memory pressure here and evicting
available memory. So what is the actual cost benefit model here?
Is it really better to keep swapcache around even under memory pressure
if the CoWed page could never be faulted in?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Andrew Morton]

tl;dr, what's the viewpoint on this patch now?

I have a note here that we're hoping input from Johannes, Vlastimil and
Mel (please).

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Huang, Ying]

Andrew Morton <akpm@linux-foundation.org> writes:

> tl;dr, what's the viewpoint on this patch now?
>
> I have a note here that we're hoping input from Johannes, Vlastimil and
> Mel (please).

Michal has performance concerns.  So, I think that we need to stop
scanning anon LRU if don't find enough page in swap cache at the tail of
the inactive list.

--
Best Regards,
Huang, Ying

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Andrew Morton]

On Fri, 15 Dec 2023 13:05:09 +0800 "Huang, Ying" <ying.huang@intel.com> wrote:

> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > tl;dr, what's the viewpoint on this patch now?
> >
> > I have a note here that we're hoping input from Johannes, Vlastimil and
> > Mel (please).
> 
> Michal has performance concerns.  So, I think that we need to stop
> scanning anon LRU if don't find enough page in swap cache at the tail of
> the inactive list.

OK, thanks, I'll make the v10 patch go away for now.

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Chris Li]

On Tue, Nov 21, 2023 at 5:00 AM Michal Hocko <mhocko@suse.com> wrote:
> > However, in swapcache_only mode, the scan count still increased when scan
> > non-swapcache pages because there are large number of non-swapcache pages
> > and rare swapcache pages in swapcache_only mode, and if the non-swapcache
> > is skipped and do not count, the scan of pages in isolate_lru_folios() can
> > eventually lead to hung task, just as Sachin reported [2].
>
> I find this paragraph really confusing! I guess what you meant to say is
> that a real swapcache_only is problematic because it can end up not
> making any progress, correct?
>
> AFAIU you have addressed that problem by making swapcache_only anon LRU
> specific, right? That would be certainly more robust as you can still
> reclaim from file LRUs. I cannot say I like that because swapcache_only
> is a bit confusing and I do not think we want to grow more special

That is my feeling as well. I don't like to have too many special
purposes modes either. It makes the whole process much harder to
reason. The comment seems to suggest it is not effective in some
situations. I am wondering if we can address that situation more
directly without the special mode. At the same time I am not very
familiar with the reclaim code path yet. I need to learn more about
this problem space to articulate my thoughts better . I can dig in
more, I might ask a lot of silly questions.

Chris

> purpose reclaim types. Would it be possible/reasonable to instead put
> swapcache pages on the file LRU instead?
> --
> Michal Hocko
> SUSE Labs
>

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Chris Li]

Hi Shixin,

On Tue, Nov 21, 2023 at 12:08 AM Liu Shixin <liushixin2@huawei.com> wrote:
>
> When spaces of swap devices are exhausted, only file pages can be
> reclaimed.  But there are still some swapcache pages in anon lru list.
> This can lead to a premature out-of-memory.
>
> The problem is found with such step:
>
>  Firstly, set a 9MB disk swap space, then create a cgroup with 10MB
>  memory limit, then runs an program to allocates about 15MB memory.
>
> The problem occurs occasionally, which may need about 100 times [1].

Just out of my curiosity, in your usage case, how much additional
memory in terms of pages or MB can be freed by this patch, using
current code as base line?
Does the swap cache page reclaimed in swapcache_only mode, all swap
count drop to zero, and the only reason to stay in swap cache is to
void page IO write if we need to swap that page out again?

> Fix it by checking number of swapcache pages in can_reclaim_anon_pages().
> If the number is not zero, return true and set swapcache_only to 1.
> When scan anon lru list in swapcache_only mode, non-swapcache pages will
> be skipped to isolate in order to accelerate reclaim efficiency.

Here you said non-swapcache will be skipped if swapcache_only == 1

>
> However, in swapcache_only mode, the scan count still increased when scan
> non-swapcache pages because there are large number of non-swapcache pages
> and rare swapcache pages in swapcache_only mode, and if the non-swapcache

Here you suggest non-swapcache pages will also be scanned even when
swapcache_only == 1. It seems to contradict what you said above. I
feel that I am missing something here.

> is skipped and do not count, the scan of pages in isolate_lru_folios() can

Can you clarify which "scan of pages", are those pages swapcache pages
or non-swapcache pages?

> eventually lead to hung task, just as Sachin reported [2].
>
> By the way, since there are enough times of memory reclaim before OOM, it
> is not need to isolate too much swapcache pages in one times.
>
> [1]. https://lore.kernel.org/lkml/CAJD7tkZAfgncV+KbKr36=eDzMnT=9dZOT0dpMWcurHLr6Do+GA@mail.gmail.com/
> [2]. https://lore.kernel.org/linux-mm/CAJD7tkafz_2XAuqE8tGLPEcpLngewhUo=5US14PAtSM9tLBUQg@mail.gmail.com/
>
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
> Tested-by: Yosry Ahmed <yosryahmed@google.com>
> Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
> Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
> ---
> v9->v10: Use per-node swapcache suggested by Yu Zhao.
> v8->v9: Move the swapcache check after can_demote() and refector
>         can_reclaim_anon_pages() a bit.
> v7->v8: Reset swapcache_only at the beginning of can_reclaim_anon_pages().
> v6->v7: Reset swapcache_only to zero after there are swap spaces.
> v5->v6: Fix NULL pointing derefence and hung task problem reported by Sachin.
>
>  mm/vmscan.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 506f8220c5fe..1fcc94717370 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -136,6 +136,9 @@ struct scan_control {
>         /* Always discard instead of demoting to lower tier memory */
>         unsigned int no_demotion:1;
>
> +       /* Swap space is exhausted, only reclaim swapcache for anon LRU */
> +       unsigned int swapcache_only:1;
> +
>         /* Allocation order */
>         s8 order;
>
> @@ -308,10 +311,36 @@ static bool can_demote(int nid, struct scan_control *sc)
>         return true;
>  }
>
> +#ifdef CONFIG_SWAP
> +static bool can_reclaim_swapcache(struct mem_cgroup *memcg, int nid)
> +{
> +       struct pglist_data *pgdat = NODE_DATA(nid);
> +       unsigned long nr_swapcache;
> +
> +       if (!memcg) {
> +               nr_swapcache = node_page_state(pgdat, NR_SWAPCACHE);
> +       } else {
> +               struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
> +
> +               nr_swapcache = lruvec_page_state_local(lruvec, NR_SWAPCACHE);
> +       }
> +
> +       return nr_swapcache > 0;
> +}
> +#else
> +static bool can_reclaim_swapcache(struct mem_cgroup *memcg, int nid)
> +{
> +       return false;
> +}
> +#endif
> +
>  static inline bool can_reclaim_anon_pages(struct mem_cgroup *memcg,
>                                           int nid,
>                                           struct scan_control *sc)
>  {
> +       if (sc)
> +               sc->swapcache_only = 0;
> +

Minor nitpick. The sc->swapcache_only is first set to 0 then later set
to 1. Better use a local variable then write to sc->swapcache_only in
one go. If the scan_control has more than one thread accessing it, the
threads can see the flicker of 0->1 change.  I don't think that is the
case in our current code, sc is created on stack. There are other
minor benefits as The "if (sc) test" only needs to be done once, one
store instruction.

Chris

>         if (memcg == NULL) {
>                 /*
>                  * For non-memcg reclaim, is there
> @@ -330,7 +359,17 @@ static inline bool can_reclaim_anon_pages(struct mem_cgroup *memcg,
>          *
>          * Can it be reclaimed from this node via demotion?
>          */
> -       return can_demote(nid, sc);
> +       if (can_demote(nid, sc))
> +               return true;
> +
> +       /* Is there any swapcache pages to reclaim in this node? */
> +       if (can_reclaim_swapcache(memcg, nid)) {
> +               if (sc)
> +                       sc->swapcache_only = 1;
> +               return true;
> +       }
> +
> +       return false;
>  }
>
>  /*
> @@ -1642,6 +1681,15 @@ static unsigned long isolate_lru_folios(unsigned long nr_to_scan,
>                  */
>                 scan += nr_pages;
>
> +               /*
> +                * Count non-swapcache too because the swapcache pages may
> +                * be rare and it takes too much times here if not count
> +                * the non-swapcache pages.
> +                */
> +               if (unlikely(sc->swapcache_only && !is_file_lru(lru) &&
> +                   !folio_test_swapcache(folio)))
> +                       goto move;
> +
>                 if (!folio_test_lru(folio))
>                         goto move;
>                 if (!sc->may_unmap && folio_mapped(folio))
> --
> 2.25.1
>
>

Re: [PATCH v10] mm: vmscan: try to reclaim swapcache pages if no swap space

[Liu Shixin]

On 2023/11/24 1:19, Chris Li wrote:
> Hi Shixin,
>
> On Tue, Nov 21, 2023 at 12:08 AM Liu Shixin <liushixin2@huawei.com> wrote:
>> When spaces of swap devices are exhausted, only file pages can be
>> reclaimed.  But there are still some swapcache pages in anon lru list.
>> This can lead to a premature out-of-memory.
>>
>> The problem is found with such step:
>>
>>  Firstly, set a 9MB disk swap space, then create a cgroup with 10MB
>>  memory limit, then runs an program to allocates about 15MB memory.
>>
>> The problem occurs occasionally, which may need about 100 times [1].
> Just out of my curiosity, in your usage case, how much additional
> memory in terms of pages or MB can be freed by this patch, using
> current code as base line?
My testcase is in a memory cgroup with memory limit of 10MB, the memory can
be freed is only about 5MB.
> Does the swap cache page reclaimed in swapcache_only mode, all swap
> count drop to zero, and the only reason to stay in swap cache is to
> void page IO write if we need to swap that page out again?
Yes.
>
>> Fix it by checking number of swapcache pages in can_reclaim_anon_pages().
>> If the number is not zero, return true and set swapcache_only to 1.
>> When scan anon lru list in swapcache_only mode, non-swapcache pages will
>> be skipped to isolate in order to accelerate reclaim efficiency.
> Here you said non-swapcache will be skipped if swapcache_only == 1
>
>> However, in swapcache_only mode, the scan count still increased when scan
>> non-swapcache pages because there are large number of non-swapcache pages
>> and rare swapcache pages in swapcache_only mode, and if the non-swapcache
> Here you suggest non-swapcache pages will also be scanned even when
> swapcache_only == 1. It seems to contradict what you said above. I
> feel that I am missing something here.
The swapcache pages and non-swapcache pages are both in anon lru. So when scan
anon pages, then non-swapcache pages will also be scanned. In isolate_lru_folios(),
if we select to put non-swapcache pages in folios_skipped list, the scan of anon list
will running until finding enough swapcache pages, this will waste too much time.
To avoid such problem, after we scan enough anon pages, even if we don't isolate
enough swapcache pages, we have to stop.
>
>> is skipped and do not count, the scan of pages in isolate_lru_folios() can
> Can you clarify which "scan of pages", are those pages swapcache pages
> or non-swapcache pages?
I mean scan of anon pages, include both swapcache pages and non-swpacache pages.
>
>> eventually lead to hung task, just as Sachin reported [2].
>>
>> By the way, since there are enough times of memory reclaim before OOM, it
>> is not need to isolate too much swapcache pages in one times.
>>
>> [1]. https://lore.kernel.org/lkml/CAJD7tkZAfgncV+KbKr36=eDzMnT=9dZOT0dpMWcurHLr6Do+GA@mail.gmail.com/
>> [2]. https://lore.kernel.org/linux-mm/CAJD7tkafz_2XAuqE8tGLPEcpLngewhUo=5US14PAtSM9tLBUQg@mail.gmail.com/
>>
>> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
>> Tested-by: Yosry Ahmed <yosryahmed@google.com>
>> Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
>> Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
>> ---
>> v9->v10: Use per-node swapcache suggested by Yu Zhao.
>> v8->v9: Move the swapcache check after can_demote() and refector
>>         can_reclaim_anon_pages() a bit.
>> v7->v8: Reset swapcache_only at the beginning of can_reclaim_anon_pages().
>> v6->v7: Reset swapcache_only to zero after there are swap spaces.
>> v5->v6: Fix NULL pointing derefence and hung task problem reported by Sachin.
>>
>>  mm/vmscan.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 506f8220c5fe..1fcc94717370 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -136,6 +136,9 @@ struct scan_control {
>>         /* Always discard instead of demoting to lower tier memory */
>>         unsigned int no_demotion:1;
>>
>> +       /* Swap space is exhausted, only reclaim swapcache for anon LRU */
>> +       unsigned int swapcache_only:1;
>> +
>>         /* Allocation order */
>>         s8 order;
>>
>> @@ -308,10 +311,36 @@ static bool can_demote(int nid, struct scan_control *sc)
>>         return true;
>>  }
>>
>> +#ifdef CONFIG_SWAP
>> +static bool can_reclaim_swapcache(struct mem_cgroup *memcg, int nid)
>> +{
>> +       struct pglist_data *pgdat = NODE_DATA(nid);
>> +       unsigned long nr_swapcache;
>> +
>> +       if (!memcg) {
>> +               nr_swapcache = node_page_state(pgdat, NR_SWAPCACHE);
>> +       } else {
>> +               struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
>> +
>> +               nr_swapcache = lruvec_page_state_local(lruvec, NR_SWAPCACHE);
>> +       }
>> +
>> +       return nr_swapcache > 0;
>> +}
>> +#else
>> +static bool can_reclaim_swapcache(struct mem_cgroup *memcg, int nid)
>> +{
>> +       return false;
>> +}
>> +#endif
>> +
>>  static inline bool can_reclaim_anon_pages(struct mem_cgroup *memcg,
>>                                           int nid,
>>                                           struct scan_control *sc)
>>  {
>> +       if (sc)
>> +               sc->swapcache_only = 0;
>> +
> Minor nitpick. The sc->swapcache_only is first set to 0 then later set
> to 1. Better use a local variable then write to sc->swapcache_only in
> one go. If the scan_control has more than one thread accessing it, the
> threads can see the flicker of 0->1 change.  I don't think that is the
> case in our current code, sc is created on stack. There are other
> minor benefits as The "if (sc) test" only needs to be done once, one
> store instruction.
>
> Chris
Thanks for your advice. If finally decide to use this patch, I will revise it.
>>         if (memcg == NULL) {
>>                 /*
>>                  * For non-memcg reclaim, is there
>> @@ -330,7 +359,17 @@ static inline bool can_reclaim_anon_pages(struct mem_cgroup *memcg,
>>          *
>>          * Can it be reclaimed from this node via demotion?
>>          */
>> -       return can_demote(nid, sc);
>> +       if (can_demote(nid, sc))
>> +               return true;
>> +
>> +       /* Is there any swapcache pages to reclaim in this node? */
>> +       if (can_reclaim_swapcache(memcg, nid)) {
>> +               if (sc)
>> +                       sc->swapcache_only = 1;
>> +               return true;
>> +       }
>> +
>> +       return false;
>>  }
>>
>>  /*
>> @@ -1642,6 +1681,15 @@ static unsigned long isolate_lru_folios(unsigned long nr_to_scan,
>>                  */
>>                 scan += nr_pages;
>>
>> +               /*
>> +                * Count non-swapcache too because the swapcache pages may
>> +                * be rare and it takes too much times here if not count
>> +                * the non-swapcache pages.
>> +                */
>> +               if (unlikely(sc->swapcache_only && !is_file_lru(lru) &&
>> +                   !folio_test_swapcache(folio)))
>> +                       goto move;
>> +
>>                 if (!folio_test_lru(folio))
>>                         goto move;
>>                 if (!sc->may_unmap && folio_mapped(folio))
>> --
>> 2.25.1
>>
>>
> .
>