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