[PATCH v2] mm/vmscan: count layzfree pages and fix nr_isolated_* mismatch

[PATCH v2] mm/vmscan: count layzfree pages and fix nr_isolated_* mismatch

[Jaewon Kim]

This patch fix nr_isolate_* mismatch problem between cma and dirty
lazyfree page.

If try_to_unmap_one is used for reclaim and it detects a dirty lazyfree
page, then the lazyfree page is changed to a normal anon page having
SwapBacked by commit 802a3a92ad7a ("mm: reclaim MADV_FREE pages"). Even
with the change, reclaim context correctly counts isolated files because
it uses is_file_lru to distinguish file. And the change to anon is not
happened if try_to_unmap_one is used for migration. So migration context
like compaction also correctly counts isolated files even though it uses
page_is_file_lru insted of is_file_lru. Recently page_is_file_cache was
renamed to page_is_file_lru by commit 9de4f22a60f7 ("mm: code cleanup for
MADV_FREE").

But the nr_isolate_* mismatch problem happens on cma alloc. There is
reclaim_clean_pages_from_list which is being used only by cma. It was
introduced by commit 02c6de8d757c ("mm: cma: discard clean pages during
contiguous allocation instead of migration") to reclaim clean file pages
without migration. The cma alloc uses both reclaim_clean_pages_from_list
and migrate_pages, and it uses page_is_file_lru to count isolated
files. If there are dirty lazyfree pages allocated from cma memory
region, the pages are counted as isolated file at the beginging but are
counted as isolated anon after finished.

Mem-Info:
Node 0 active_anon:3045904kB inactive_anon:611448kB active_file:14892kB inactive_file:205636kB unevictable:10416kB isolated(anon):0kB isolated(file):37664kB mapped:630216kB dirty:384kB writeback:0kB shmem:42576kB writeback_tmp:0kB unstable:0kB all_unreclaimable? no

Like log above, there was too much isolated file, 37664kB, which
triggers too_many_isolated in reclaim when there is no isolated file in
system wide. It could be reproducible by running two programs, doing
MADV_FREE, writing and doing cma alloc, respectively. Although isolated
anon is 0, I found that the internal value of isolated anon was the
negative value of isolated file.

Fix this by compensating the isolated count for both LRU lists. Count
non-discarded lazyfree pages in shrink_page_list, then compensate the
counted number in reclaim_clean_pages_from_list.

Reported-by: Yong-Taek Lee <ytk.lee@samsung.com>
Signed-off-by: Jaewon Kim <jaewon31.kim@samsung.com>
Suggested-by: and Acked-by: Minchan Kim <minchan@kernel.org>
---
v2: do not change logic regarding lazyfree
    just count dirty lazyfree pages and fix the count issue
v1: skip dirty lazyfree pages in reclaim_clean_pages_from_list
---
 include/linux/vmstat.h |  1 +
 mm/vmscan.c            | 26 ++++++++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
index 292485f3d24d..10cc932e209a 100644
--- a/include/linux/vmstat.h
+++ b/include/linux/vmstat.h
@@ -29,6 +29,7 @@ struct reclaim_stat {
 	unsigned nr_activate[2];
 	unsigned nr_ref_keep;
 	unsigned nr_unmap_fail;
+	unsigned nr_lazyfree_fail;
 };
 
 enum writeback_stat_item {
diff --git a/mm/vmscan.c b/mm/vmscan.c
index b06868fc4926..cd6881810ee9 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1295,11 +1295,15 @@ static unsigned long shrink_page_list(struct list_head *page_list,
 		 */
 		if (page_mapped(page)) {
 			enum ttu_flags flags = ttu_flags | TTU_BATCH_FLUSH;
+			bool lazyfree = PageAnon(page) && !PageSwapBacked(page);
 
 			if (unlikely(PageTransHuge(page)))
 				flags |= TTU_SPLIT_HUGE_PMD;
+
 			if (!try_to_unmap(page, flags)) {
 				stat->nr_unmap_fail += nr_pages;
+				if (lazyfree && PageSwapBacked(page))
+					stat->nr_lazyfree_fail += nr_pages;
 				goto activate_locked;
 			}
 		}
@@ -1491,8 +1495,8 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone,
 		.priority = DEF_PRIORITY,
 		.may_unmap = 1,
 	};
-	struct reclaim_stat dummy_stat;
-	unsigned long ret;
+	struct reclaim_stat stat;
+	unsigned long reclaimed;
 	struct page *page, *next;
 	LIST_HEAD(clean_pages);
 
@@ -1504,11 +1508,21 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone,
 		}
 	}
 
-	ret = shrink_page_list(&clean_pages, zone->zone_pgdat, &sc,
-			TTU_IGNORE_ACCESS, &dummy_stat, true);
+	reclaimed = shrink_page_list(&clean_pages, zone->zone_pgdat, &sc,
+			TTU_IGNORE_ACCESS, &stat, true);
 	list_splice(&clean_pages, page_list);
-	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -ret);
-	return ret;
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -reclaimed);
+	/*
+	 * Since lazyfree pages are isolated from file LRU from the beginning,
+	 * they will rotate back to anonymous LRU in the end if it failed to
+	 * discard so isolated count will be mismatched.
+	 * Compensate the isolated count for both LRU lists.
+	 */
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON,
+					stat.nr_lazyfree_fail);
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE,
+					-stat.nr_lazyfree_fail);
+	return reclaimed;
 }
 
 /*
-- 
2.13.7

Re: [PATCH v2] mm/vmscan: count layzfree pages and fix nr_isolated_* mismatch

[Johannes Weiner]

On Wed, Apr 22, 2020 at 05:48:15PM +0900, Jaewon Kim wrote:
> @@ -1295,11 +1295,15 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>  		 */
>  		if (page_mapped(page)) {
>  			enum ttu_flags flags = ttu_flags | TTU_BATCH_FLUSH;
> +			bool lazyfree = PageAnon(page) && !PageSwapBacked(page);
>  
>  			if (unlikely(PageTransHuge(page)))
>  				flags |= TTU_SPLIT_HUGE_PMD;
> +
>  			if (!try_to_unmap(page, flags)) {
>  				stat->nr_unmap_fail += nr_pages;
> +				if (lazyfree && PageSwapBacked(page))

This looks pretty strange, until you remember that try_to_unmap()
could SetPageSwapbacked again.

This might be more obvious?

			was_swapbacked = PageSwapBacked(page);
			if (!try_to_unmap(page, flags)) {
				stat->nr_unmap_fail += nr_pages;
				if (!was_swapbacked && PageSwapBacked(page))
> +					stat->nr_lazyfree_fail += nr_pages;
>  				goto activate_locked;

Or at least was_lazyfree.

> @@ -1491,8 +1495,8 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone,
>  		.priority = DEF_PRIORITY,
>  		.may_unmap = 1,
>  	};
> -	struct reclaim_stat dummy_stat;
> -	unsigned long ret;
> +	struct reclaim_stat stat;
> +	unsigned long reclaimed;

nr_reclaimed would be better.

I also prefer keeping dummy_stat, since that's still what it is.

Re: [PATCH v2] mm/vmscan: count layzfree pages and fix nr_isolated_* mismatch

[Jaewon Kim]

On 2020년 04월 22일 22:07, Johannes Weiner wrote:
> On Wed, Apr 22, 2020 at 05:48:15PM +0900, Jaewon Kim wrote:
>> @@ -1295,11 +1295,15 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>>  		 */
>>  		if (page_mapped(page)) {
>>  			enum ttu_flags flags = ttu_flags | TTU_BATCH_FLUSH;
>> +			bool lazyfree = PageAnon(page) && !PageSwapBacked(page);
>>  
>>  			if (unlikely(PageTransHuge(page)))
>>  				flags |= TTU_SPLIT_HUGE_PMD;
>> +
>>  			if (!try_to_unmap(page, flags)) {
>>  				stat->nr_unmap_fail += nr_pages;
>> +				if (lazyfree && PageSwapBacked(page))
> This looks pretty strange, until you remember that try_to_unmap()
> could SetPageSwapbacked again.
>
> This might be more obvious?
>
> 			was_swapbacked = PageSwapBacked(page);
> 			if (!try_to_unmap(page, flags)) {
> 				stat->nr_unmap_fail += nr_pages;
> 				if (!was_swapbacked && PageSwapBacked(page))
Hello Johannes, thank you for your comment.

The name can changed from layzyfree to was_swapbacked.
By the way, did you mean removing PageAnon(page), too? It seems to be OK, though.
>> +					stat->nr_lazyfree_fail += nr_pages;
>>  				goto activate_locked;
> Or at least was_lazyfree.
Sorry but I'm confused.
I think you meant additional comment to previous your comment
rather than you wanted to rename stat->nr_lazyfree_fail to stat->was_lazyfree.
>
>> @@ -1491,8 +1495,8 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone,
>>  		.priority = DEF_PRIORITY,
>>  		.may_unmap = 1,
>>  	};
>> -	struct reclaim_stat dummy_stat;
>> -	unsigned long ret;
>> +	struct reclaim_stat stat;
>> +	unsigned long reclaimed;
> nr_reclaimed would be better.
I will add nr_ prefix on next patch.
>
> I also prefer keeping dummy_stat, since that's still what it is.
This patch uses stat.nr_lazyfree_fail, I do not understand why it is still dummy_stat.
If you want, I will keep dummy_stat, though.

Thank you
Jaewon Kim
>
>

Re: [PATCH v2] mm/vmscan: count layzfree pages and fix nr_isolated_* mismatch

[Johannes Weiner]

On Thu, Apr 23, 2020 at 12:16:02PM +0900, Jaewon Kim wrote:
> 
> 
> On 2020년 04월 22일 22:07, Johannes Weiner wrote:
> > On Wed, Apr 22, 2020 at 05:48:15PM +0900, Jaewon Kim wrote:
> >> @@ -1295,11 +1295,15 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> >>  		 */
> >>  		if (page_mapped(page)) {
> >>  			enum ttu_flags flags = ttu_flags | TTU_BATCH_FLUSH;
> >> +			bool lazyfree = PageAnon(page) && !PageSwapBacked(page);
> >>  
> >>  			if (unlikely(PageTransHuge(page)))
> >>  				flags |= TTU_SPLIT_HUGE_PMD;
> >> +
> >>  			if (!try_to_unmap(page, flags)) {
> >>  				stat->nr_unmap_fail += nr_pages;
> >> +				if (lazyfree && PageSwapBacked(page))
> > This looks pretty strange, until you remember that try_to_unmap()
> > could SetPageSwapbacked again.
> >
> > This might be more obvious?
> >
> > 			was_swapbacked = PageSwapBacked(page);
> > 			if (!try_to_unmap(page, flags)) {
> > 				stat->nr_unmap_fail += nr_pages;
> > 				if (!was_swapbacked && PageSwapBacked(page))
> Hello Johannes, thank you for your comment.
> 
> The name can changed from layzyfree to was_swapbacked.
> By the way, did you mean removing PageAnon(page), too? It seems to be OK, though.

I can't decide whether PageAnon() makes it clearer or not. But it's
not really needed for correctness. So feel free to keep what you had.

I would really just at least change bool lazyfree to was_lazyfree,
otherwise it seems a bit confusing. was_lazyfree makes it a bit
clearer that we expect try_to_unmap() might change the state.

> >> +					stat->nr_lazyfree_fail += nr_pages;
> >>  				goto activate_locked;
> > Or at least was_lazyfree.
> Sorry but I'm confused.
> I think you meant additional comment to previous your comment
> rather than you wanted to rename stat->nr_lazyfree_fail to stat->was_lazyfree.

No just the bool variable, the stat one seems fine to me.

> >> @@ -1491,8 +1495,8 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone,
> >>  		.priority = DEF_PRIORITY,
> >>  		.may_unmap = 1,
> >>  	};
> >> -	struct reclaim_stat dummy_stat;
> >> -	unsigned long ret;
> >> +	struct reclaim_stat stat;
> >> +	unsigned long reclaimed;
> > nr_reclaimed would be better.
> I will add nr_ prefix on next patch.

Thanks!

> > I also prefer keeping dummy_stat, since that's still what it is.
> This patch uses stat.nr_lazyfree_fail, I do not understand why it is still dummy_stat.
> If you want, I will keep dummy_stat, though.

My bad, I just misread this. 'stat' makes more sense then.

Re: [PATCH v2] mm/vmscan: count layzfree pages and fix nr_isolated_* mismatch

[Minchan Kim]

On Thu, Apr 23, 2020 at 12:05:46PM -0400, Johannes Weiner wrote:
> On Thu, Apr 23, 2020 at 12:16:02PM +0900, Jaewon Kim wrote:
> > 
> > 
> > On 2020년 04월 22일 22:07, Johannes Weiner wrote:
> > > On Wed, Apr 22, 2020 at 05:48:15PM +0900, Jaewon Kim wrote:
> > >> @@ -1295,11 +1295,15 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> > >>  		 */
> > >>  		if (page_mapped(page)) {
> > >>  			enum ttu_flags flags = ttu_flags | TTU_BATCH_FLUSH;
> > >> +			bool lazyfree = PageAnon(page) && !PageSwapBacked(page);
> > >>  
> > >>  			if (unlikely(PageTransHuge(page)))
> > >>  				flags |= TTU_SPLIT_HUGE_PMD;
> > >> +
> > >>  			if (!try_to_unmap(page, flags)) {
> > >>  				stat->nr_unmap_fail += nr_pages;
> > >> +				if (lazyfree && PageSwapBacked(page))
> > > This looks pretty strange, until you remember that try_to_unmap()
> > > could SetPageSwapbacked again.
> > >
> > > This might be more obvious?
> > >
> > > 			was_swapbacked = PageSwapBacked(page);
> > > 			if (!try_to_unmap(page, flags)) {
> > > 				stat->nr_unmap_fail += nr_pages;
> > > 				if (!was_swapbacked && PageSwapBacked(page))
> > Hello Johannes, thank you for your comment.
> > 
> > The name can changed from layzyfree to was_swapbacked.
> > By the way, did you mean removing PageAnon(page), too? It seems to be OK, though.
> 
> I can't decide whether PageAnon() makes it clearer or not. But it's
> not really needed for correctness. So feel free to keep what you had.

Yub, PageAnon is redundant.

> I would really just at least change bool lazyfree to was_lazyfree,

It's better.

Re: [PATCH v2] mm/vmscan: count layzfree pages and fix nr_isolated_* mismatch

[Jaewon Kim]

On 2020년 04월 24일 05:00, Minchan Kim wrote:
> On Thu, Apr 23, 2020 at 12:05:46PM -0400, Johannes Weiner wrote:
>> On Thu, Apr 23, 2020 at 12:16:02PM +0900, Jaewon Kim wrote:
>>>
>>> On 2020년 04월 22일 22:07, Johannes Weiner wrote:
>>>> On Wed, Apr 22, 2020 at 05:48:15PM +0900, Jaewon Kim wrote:
>>>>> @@ -1295,11 +1295,15 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>>>>>  		 */
>>>>>  		if (page_mapped(page)) {
>>>>>  			enum ttu_flags flags = ttu_flags | TTU_BATCH_FLUSH;
>>>>> +			bool lazyfree = PageAnon(page) && !PageSwapBacked(page);
>>>>>  
>>>>>  			if (unlikely(PageTransHuge(page)))
>>>>>  				flags |= TTU_SPLIT_HUGE_PMD;
>>>>> +
>>>>>  			if (!try_to_unmap(page, flags)) {
>>>>>  				stat->nr_unmap_fail += nr_pages;
>>>>> +				if (lazyfree && PageSwapBacked(page))
>>>> This looks pretty strange, until you remember that try_to_unmap()
>>>> could SetPageSwapbacked again.
>>>>
>>>> This might be more obvious?
>>>>
>>>> 			was_swapbacked = PageSwapBacked(page);
>>>> 			if (!try_to_unmap(page, flags)) {
>>>> 				stat->nr_unmap_fail += nr_pages;
>>>> 				if (!was_swapbacked && PageSwapBacked(page))
>>> Hello Johannes, thank you for your comment.
>>>
>>> The name can changed from layzyfree to was_swapbacked.
>>> By the way, did you mean removing PageAnon(page), too? It seems to be OK, though.
>> I can't decide whether PageAnon() makes it clearer or not. But it's
>> not really needed for correctness. So feel free to keep what you had.
> Yub, PageAnon is redundant.
>
>> I would really just at least change bool lazyfree to was_lazyfree,
> It's better.
Thank you both.

Sorry but let me ask again.

Because PageAnon is redundant, let me remove it on checking lazyfree.
Then, I think, was_swapbacked (first suggestion) is better then was_lazyfree (second suggestion),
because it just checks PageSwapBacked(page). And we knows was_swapbacked is used for lazyfree on stat->nr_lazyfree_fail.

If you don't mind let me pick was_swapbacked instead of was_lazyfree
bool was_lazyfree = PageSwapBacked(page);

Thank you

>
>

Re: [PATCH v2] mm/vmscan: count layzfree pages and fix nr_isolated_* mismatch

[Johannes Weiner]

On Fri, Apr 24, 2020 at 01:16:45PM +0900, Jaewon Kim wrote:
> 
> 
> On 2020년 04월 24일 05:00, Minchan Kim wrote:
> > On Thu, Apr 23, 2020 at 12:05:46PM -0400, Johannes Weiner wrote:
> >> On Thu, Apr 23, 2020 at 12:16:02PM +0900, Jaewon Kim wrote:
> >>>
> >>> On 2020년 04월 22일 22:07, Johannes Weiner wrote:
> >>>> On Wed, Apr 22, 2020 at 05:48:15PM +0900, Jaewon Kim wrote:
> >>>>> @@ -1295,11 +1295,15 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> >>>>>  		 */
> >>>>>  		if (page_mapped(page)) {
> >>>>>  			enum ttu_flags flags = ttu_flags | TTU_BATCH_FLUSH;
> >>>>> +			bool lazyfree = PageAnon(page) && !PageSwapBacked(page);
> >>>>>  
> >>>>>  			if (unlikely(PageTransHuge(page)))
> >>>>>  				flags |= TTU_SPLIT_HUGE_PMD;
> >>>>> +
> >>>>>  			if (!try_to_unmap(page, flags)) {
> >>>>>  				stat->nr_unmap_fail += nr_pages;
> >>>>> +				if (lazyfree && PageSwapBacked(page))
> >>>> This looks pretty strange, until you remember that try_to_unmap()
> >>>> could SetPageSwapbacked again.
> >>>>
> >>>> This might be more obvious?
> >>>>
> >>>> 			was_swapbacked = PageSwapBacked(page);
> >>>> 			if (!try_to_unmap(page, flags)) {
> >>>> 				stat->nr_unmap_fail += nr_pages;
> >>>> 				if (!was_swapbacked && PageSwapBacked(page))
> >>> Hello Johannes, thank you for your comment.
> >>>
> >>> The name can changed from layzyfree to was_swapbacked.
> >>> By the way, did you mean removing PageAnon(page), too? It seems to be OK, though.
> >> I can't decide whether PageAnon() makes it clearer or not. But it's
> >> not really needed for correctness. So feel free to keep what you had.
> > Yub, PageAnon is redundant.
> >
> >> I would really just at least change bool lazyfree to was_lazyfree,
> > It's better.
> Thank you both.
> 
> Sorry but let me ask again.
> 
> Because PageAnon is redundant, let me remove it on checking lazyfree.
> Then, I think, was_swapbacked (first suggestion) is better then was_lazyfree (second suggestion),
> because it just checks PageSwapBacked(page). And we knows was_swapbacked is used for lazyfree on stat->nr_lazyfree_fail.
> 
> If you don't mind let me pick was_swapbacked instead of was_lazyfree

That sounds good to me. Thanks Jaewon!