On Wed, Jul 13, 2016 at 09:55:16AM +0100, Mel Gorman wrote:
> On Tue, Jul 12, 2016 at 10:58:01AM -0400, Johannes Weiner wrote:
> > On Fri, Jul 08, 2016 at 10:34:54AM +0100, Mel Gorman wrote:
> > > NR_FILE_PAGES is the number of file pages.
> > > NR_FILE_MAPPED is the number of mapped file pages.
> > > NR_ANON_PAGES is the number of mapped anon pages.
> > >
> > > This is unhelpful naming as it's easy to confuse NR_FILE_MAPPED and
> > > NR_ANON_PAGES for mapped pages. This patch renames NR_ANON_PAGES so we
> > > have
> > >
> > > NR_FILE_PAGES is the number of file pages.
> > > NR_FILE_MAPPED is the number of mapped file pages.
> > > NR_ANON_MAPPED is the number of mapped anon pages.
> >
> > That looks wrong to me. The symmetry is between NR_FILE_PAGES and
> > NR_ANON_PAGES. NR_FILE_MAPPED is merely elaborating on the mapped
> > subset of NR_FILE_PAGES, something which isn't necessary for anon
> > pages as they're always mapped.
>
> How strongly do you feel about reverting it as later patches would cause
> lots of conflicts.
>
> Obviously I found the new names clearer but I was thinking a lot at the
> time about mapped vs unmapped due to looking closely at both reclaim and
> [f|m]advise functions at the time. I found it mildly irksome to switch
> between the semantics of file/anon when looking at the vmstat updates.
I can see that. It all depends on whether you consider mapping state
or page type the more fundamental attribute, and coming from the
mapping perspective those new names make sense as well.
However, that leaves the disconnect between the enum name and what we
print to userspace. I find myself having to associate those quite a
lot to find all the sites that modify a given /proc/vmstat item, and
that's a bit of a pain if the names don't match.
I don't care strongly enough to cause a respin of half the series, and
it's not your problem that I waited until the last revision went into
mmots to review and comment. But if you agreed to a revert, would you
consider tacking on a revert patch at the end of the series?
Something like this?
>From de22dd5dee337db8590f46919616dd7ef2cfd002 Mon Sep 17 00:00:00 2001
From: Johannes Weiner <hannes@xxxxxxxxxxx>
Date: Wed, 13 Jul 2016 08:50:24 -0400
Subject: [PATCH] mm: revert NR_ANON_MAPPED to NR_ANON_PAGES
This reverts 'mm: rename NR_ANON_PAGES to NR_ANON_MAPPED', which had
the following rationale:
> NR_FILE_PAGES is the number of file pages.
> NR_FILE_MAPPED is the number of mapped file pages.
> NR_ANON_PAGES is the number of mapped anon pages.
>
> This is unhelpful naming as it's easy to confuse NR_FILE_MAPPED and
> NR_ANON_PAGES for mapped pages. This patch renames NR_ANON_PAGES so we
> have
>
> NR_FILE_PAGES is the number of file pages.
> NR_FILE_MAPPED is the number of mapped file pages.
> NR_ANON_MAPPED is the number of mapped anon pages.
Arguably, the symmetry is either between mapped and unmapped, or anon
and file, so both namings work. However, this change disconnected the
internal enum name from the name exported to userspace, which makes it
painful to trace back an observed statistic to its sources in the VM.
Revert back, such that NR_ANON_PAGES and NR_FILE_PAGES go together,
and NR_FILE_MAPPED is an elaboration on the latter. To make this even
clearer, reorder the statistics so that NR_FILE_MAPPED goes with the
other file page specifics, NR_FILE_DIRTY, NR_FILE_WRITEBACK, NR_SHMEM.
Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
---
drivers/base/node.c | 2 +-
fs/proc/meminfo.c | 2 +-
include/linux/mmzone.h | 4 ++--
mm/migrate.c | 2 +-
mm/rmap.c | 8 ++++----
mm/vmstat.c | 2 +-
6 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/base/node.c b/drivers/base/node.c
index 89e4f96..50d4004 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -122,7 +122,7 @@ static ssize_t node_read_meminfo(struct device *dev,
nid, K(node_page_state(pgdat, NR_WRITEBACK)),
nid, K(node_page_state(pgdat, NR_FILE_PAGES)),
nid, K(node_page_state(pgdat, NR_FILE_MAPPED)),
- nid, K(node_page_state(pgdat, NR_ANON_MAPPED)),
+ nid, K(node_page_state(pgdat, NR_ANON_PAGES)),
nid, K(i.sharedram),
nid, sum_zone_node_page_state(nid, NR_KERNEL_STACK) *
THREAD_SIZE / 1024,
diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
index c1fdcc1..f7b4bbd 100644
--- a/fs/proc/meminfo.c
+++ b/fs/proc/meminfo.c
@@ -140,7 +140,7 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
K(i.freeswap),
K(global_node_page_state(NR_FILE_DIRTY)),
K(global_node_page_state(NR_WRITEBACK)),
- K(global_node_page_state(NR_ANON_MAPPED)),
+ K(global_node_page_state(NR_ANON_PAGES)),
K(global_node_page_state(NR_FILE_MAPPED)),
K(i.sharedram),
K(global_page_state(NR_SLAB_RECLAIMABLE) +
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index a3b7f45..fd16082 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -144,10 +144,10 @@ enum node_stat_item {
WORKINGSET_REFAULT,
WORKINGSET_ACTIVATE,
WORKINGSET_NODERECLAIM,
- NR_ANON_MAPPED, /* Mapped anonymous pages */
+ NR_ANON_PAGES, /* Mapped anonymous pages */
+ NR_FILE_PAGES,
NR_FILE_MAPPED, /* pagecache pages mapped into pagetables.
only modified from process context */
- NR_FILE_PAGES,
NR_FILE_DIRTY,
NR_WRITEBACK,
NR_WRITEBACK_TEMP, /* Writeback using temporary buffers */
diff --git a/mm/migrate.c b/mm/migrate.c
index ed2f85e..525679a 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -501,7 +501,7 @@ int migrate_page_move_mapping(struct address_space *mapping,
* new page and drop references to the old page.
*
* Note that anonymous pages are accounted for
- * via NR_FILE_PAGES and NR_ANON_MAPPED if they
+ * via NR_FILE_PAGES and NR_ANON_PAGES if they
* are mapped to swap space.
*/
if (newzone != oldzone) {
diff --git a/mm/rmap.c b/mm/rmap.c
index 414688c..203ba16 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1217,7 +1217,7 @@ void do_page_add_anon_rmap(struct page *page,
*/
if (compound)
__inc_node_page_state(page, NR_ANON_THPS);
- __mod_node_page_state(page_pgdat(page), NR_ANON_MAPPED, nr);
+ __mod_node_page_state(page_pgdat(page), NR_ANON_PAGES, nr);
}
if (unlikely(PageKsm(page)))
return;
@@ -1261,7 +1261,7 @@ void page_add_new_anon_rmap(struct page *page,
/* increment count (starts at -1) */
atomic_set(&page->_mapcount, 0);
}
- __mod_node_page_state(page_pgdat(page), NR_ANON_MAPPED, nr);
+ __mod_node_page_state(page_pgdat(page), NR_ANON_PAGES, nr);
__page_set_anon_rmap(page, vma, address, 1);
}
@@ -1378,7 +1378,7 @@ static void page_remove_anon_compound_rmap(struct page *page)
clear_page_mlock(page);
if (nr) {
- __mod_node_page_state(page_pgdat(page), NR_ANON_MAPPED, -nr);
+ __mod_node_page_state(page_pgdat(page), NR_ANON_PAGES, -nr);
deferred_split_huge_page(page);
}
}
@@ -1407,7 +1407,7 @@ void page_remove_rmap(struct page *page, bool compound)
* these counters are not modified in interrupt context, and
* pte lock(a spinlock) is held, which implies preemption disabled.
*/
- __dec_node_page_state(page, NR_ANON_MAPPED);
+ __dec_node_page_state(page, NR_ANON_PAGES);
if (unlikely(PageMlocked(page)))
clear_page_mlock(page);
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 7415775..1d5de5d 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -953,8 +953,8 @@ const char * const vmstat_text[] = {
"workingset_activate",
"workingset_nodereclaim",
"nr_anon_pages",
- "nr_mapped",
"nr_file_pages",
+ "nr_mapped",
"nr_dirty",
"nr_writeback",
"nr_writeback_temp",
--
2.8.2
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>