linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: a.sahrawat@samsung.com, Linux-MM <linux-mm@kvack.org>,
	maninder1.s@samsung.com,  Mel Gorman <mgorman@suse.de>,
	Michal Hocko <mhocko@suse.com>,
	mm-commits@vger.kernel.org,  Nick Piggin <npiggin@gmail.com>,
	stable <stable@vger.kernel.org>,
	v.narang@samsung.com,  Vlastimil Babka <vbabka@suse.cz>
Subject: Re: [patch 03/14] mm/vmscan: fix NR_ISOLATED_FILE corruption on 64-bit
Date: Sat, 14 Nov 2020 13:39:47 -0800	[thread overview]
Message-ID: <CAHk-=wj847SudR-kt+46fT3+xFFgiwpgThvm7DJWGdi4cVrbnQ@mail.gmail.com> (raw)
In-Reply-To: <20201114065146.3H6OX7gIF%akpm@linux-foundation.org>

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);


  reply	other threads:[~2020-11-14 21:40 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-14  6:51 incoming Andrew Morton
2020-11-14  6:51 ` [patch 01/14] mm/compaction: count pages and stop correctly during page isolation Andrew Morton
2020-11-14  6:51 ` [patch 02/14] mm/compaction: stop isolation if too many pages are isolated and we have pages to migrate Andrew Morton
2020-11-14  6:51 ` [patch 03/14] mm/vmscan: fix NR_ISOLATED_FILE corruption on 64-bit Andrew Morton
2020-11-14 21:39   ` Linus Torvalds [this message]
2020-11-14 22:14     ` Matthew Wilcox
2020-11-14  6:51 ` [patch 04/14] mailmap: fix entry for Dmitry Baryshkov/Eremin-Solenikov Andrew Morton
2020-11-14  6:51 ` [patch 05/14] mm/slub: fix panic in slab_alloc_node() Andrew Morton
2020-11-14  6:51 ` [patch 06/14] mm/gup: use unpin_user_pages() in __gup_longterm_locked() Andrew Morton
2020-11-14  6:51 ` [patch 07/14] compiler.h: fix barrier_data() on clang Andrew Morton
2020-11-14  6:52 ` [patch 08/14] Revert "kernel/reboot.c: convert simple_strtoul to kstrtoint" Andrew Morton
2020-11-14  6:52 ` [patch 09/14] reboot: fix overflow parsing reboot cpu number Andrew Morton
2020-11-14  6:52 ` [patch 10/14] kernel/watchdog: fix watchdog_allowed_mask not used warning Andrew Morton
2020-11-14  6:52 ` [patch 11/14] mm: memcontrol: fix missing wakeup polling thread Andrew Morton
2020-11-14  6:52 ` [patch 12/14] hugetlbfs: fix anon huge page migration race Andrew Morton
2020-11-14  6:52 ` [patch 13/14] panic: don't dump stack twice on warn Andrew Morton
2020-11-14  6:52 ` [patch 14/14] ocfs2: initialize ip_next_orphan Andrew Morton

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to='CAHk-=wj847SudR-kt+46fT3+xFFgiwpgThvm7DJWGdi4cVrbnQ@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=a.sahrawat@samsung.com \
    --cc=akpm@linux-foundation.org \
    --cc=linux-mm@kvack.org \
    --cc=maninder1.s@samsung.com \
    --cc=mgorman@suse.de \
    --cc=mhocko@suse.com \
    --cc=mm-commits@vger.kernel.org \
    --cc=npiggin@gmail.com \
    --cc=stable@vger.kernel.org \
    --cc=v.narang@samsung.com \
    --cc=vbabka@suse.cz \
    /path/to/YOUR_REPLY

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

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