diff mbox series

[03/14] mm/vmscan: fix NR_ISOLATED_FILE corruption on 64-bit

Message ID 20201114065146.3H6OX7gIF%akpm@linux-foundation.org (mailing list archive)
State New
Headers show
Series [01/14] mm/compaction: count pages and stop correctly during page isolation | expand

Commit Message

Andrew Morton Nov. 14, 2020, 6:51 a.m. UTC
From: Nicholas Piggin <npiggin@gmail.com>
Subject: mm/vmscan: fix NR_ISOLATED_FILE corruption on 64-bit

Previously the negated unsigned long would be cast back to signed long
which would have the correct negative value.  After commit 730ec8c01a2b
("mm/vmscan.c: change prototype for shrink_page_list"), the large unsigned
int converts to a large positive signed long.

Symptoms include CMA allocations hanging forever holding the cma_mutex due
to alloc_contig_range->...->isolate_migratepages_block waiting forever in
"while (unlikely(too_many_isolated(pgdat)))".

[akpm@linux-foundation.org: fix -stat.nr_lazyfree_fail as well, per Michal]
Link: https://lkml.kernel.org/r/20201029032320.1448441-1-npiggin@gmail.com
Fixes: 730ec8c01a2b ("mm/vmscan.c: change prototype for shrink_page_list")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Vaneet Narang <v.narang@samsung.com>
Cc: Maninder Singh <maninder1.s@samsung.com>
Cc: Amit Sahrawat <a.sahrawat@samsung.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/vmscan.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Linus Torvalds Nov. 14, 2020, 9:39 p.m. UTC | #1
I've applied this patch, but I have to say that I absolutely detest it.

This is very fragile, and I think the problem is the nasty interface.

Why don't we have simple wrappers that internally do that "mod", but
actually expose "inc" and "dec" instead, and make it much harder to
have these kinds of problems?

The function name is horrible too: "mod_node_page_state()"?  It's not
like that really even describes what it does. It doesn't modify any
page state what-so-ever, what it does is to update some node counters.

Did somebody mis-understand what "stat" stands for? It's not some odd
shortening of "state" (like "creat"). It's shorthand for "statistics".
Because that completely nonsensical "state" naming shows up in all of
those helper functions too.

And the "page" part of the name is misleading too, since it's not even
about page statistics, and the people who have a page need to
literally do "page_pgdat()" on that page in order to pass in the
"struct pglist_data" that the function wants.

To make matters worse, we have the double underscore versions too,
which have that magical "we know interrupts are disabled"
optimization, so we duplicate all of this horror.

So I really think this whole area would be ripe for some major
interface cleanups, both in naming and in interfaces.

A _small_ step would have been to avoid the "mod" thing.

            Linus

On Fri, Nov 13, 2020 at 10:51 PM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> From: Nicholas Piggin <npiggin@gmail.com>
> Subject: mm/vmscan: fix NR_ISOLATED_FILE corruption on 64-bit
>
> -       mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, -nr_reclaimed);
> +       mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE,
> +                           -(long)nr_reclaimed);
...
>         mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE,
> -                           -stat.nr_lazyfree_fail);
> +                           -(long)stat.nr_lazyfree_fail);
Matthew Wilcox Nov. 14, 2020, 10:14 p.m. UTC | #2
On Sat, Nov 14, 2020 at 01:39:47PM -0800, Linus Torvalds wrote:
> I've applied this patch, but I have to say that I absolutely detest it.
> 
> This is very fragile, and I think the problem is the nasty interface.

I agree, it's nast.

> Why don't we have simple wrappers that internally do that "mod", but
> actually expose "inc" and "dec" instead, and make it much harder to
> have these kinds of problems?

We actually need 'add' and 'sub', not 'inc' and 'dec'.  Sometimes we inc,
but often we need to add.  But, yes, 'mod' is a bad name.
diff mbox series

Patch

--- a/mm/vmscan.c~mm-vmscan-fix-nr_isolated_file-corruption-on-64-bit
+++ a/mm/vmscan.c
@@ -1516,7 +1516,8 @@  unsigned int reclaim_clean_pages_from_li
 	nr_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, -nr_reclaimed);
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE,
+			    -(long)nr_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
@@ -1526,7 +1527,7 @@  unsigned int reclaim_clean_pages_from_li
 	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);
+			    -(long)stat.nr_lazyfree_fail);
 	return nr_reclaimed;
 }