Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1441470 > unrolled thread

Re: [PATCH 18/34] mm: rename NR_ANON_PAGES to NR_ANON_MAPPED

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 18/34] mm: rename NR_ANON_PAGES to NR_ANON_MAPPED Johannes Weiner <hannes@cmpxchg.org> - 2016-07-12 17:00 +0200
    Re: [PATCH 18/34] mm: rename NR_ANON_PAGES to NR_ANON_MAPPED Mel Gorman <mgorman@techsingularity.net> - 2016-07-13 11:00 +0200
      Re: [PATCH 18/34] mm: rename NR_ANON_PAGES to NR_ANON_MAPPED Johannes Weiner <hannes@cmpxchg.org> - 2016-07-13 15:10 +0200
        Re: [PATCH 18/34] mm: rename NR_ANON_PAGES to NR_ANON_MAPPED Mel Gorman <mgorman@techsingularity.net> - 2016-07-13 15:50 +0200
          Re: [PATCH 18/34] mm: rename NR_ANON_PAGES to NR_ANON_MAPPED Andrew Morton <akpm@linux-foundation.org> - 2016-07-13 23:20 +0200
            Re: [PATCH 18/34] mm: rename NR_ANON_PAGES to NR_ANON_MAPPED Mel Gorman <mgorman@techsingularity.net> - 2016-07-15 12:50 +0200
              Re: [PATCH 18/34] mm: rename NR_ANON_PAGES to NR_ANON_MAPPED Andrew Morton <akpm@linux-foundation.org> - 2016-07-16 00:40 +0200
                Re: [PATCH 18/34] mm: rename NR_ANON_PAGES to NR_ANON_MAPPED Johannes Weiner <hannes@cmpxchg.org> - 2016-07-18 15:40 +0200
          Re: [PATCH 18/34] mm: rename NR_ANON_PAGES to NR_ANON_MAPPED Minchan Kim <minchan@kernel.org> - 2016-07-14 03:30 +0200

#1441470 — Re: [PATCH 18/34] mm: rename NR_ANON_PAGES to NR_ANON_MAPPED

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.

[toc] | [next] | [standalone]

#1442226

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.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1442438

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@cmpxchg.org>
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@cmpxchg.org>
---
 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

[toc] | [prev] | [next] | [standalone]

#1442470

On Wed, Jul 13, 2016 at 09:04:15AM -0400, Johannes Weiner wrote:
> > 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.
> 

From a reclaim perspective, I consider the mapped state to be more
important. This is particularly true when the advise calls are taken
into account. For example, madvise unmaps the pages without affecting
memory residency (distinct from RSS) without aging. fadvise ignores mapped
pages so the mapped state is very important for advise hints.  Similarly,
the mapped state can affect how the pages are aged as mapped pages affect
slab scan rates and incur TLB flushes on unmap. I guess I've been thinking
about mapped/unmapped a lot recently which pushed me towards distinct naming.

> 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 was tempted to rename userspace what is printed to vmstat as well but
worried about breaking tools that parse it.

> 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?
> 

In this case, I'm going to ask the other people on the cc for a
tie-breaker. If someone else prefers the old names then I'm happy for
your patch to be applied on top with my ack instead of respinning the
whole series.

Anyone for a tie breaker?

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1442867

On Wed, 13 Jul 2016 14:37:01 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:

> > 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?
> > 
> 
> In this case, I'm going to ask the other people on the cc for a
> tie-breaker. If someone else prefers the old names then I'm happy for
> your patch to be applied on top with my ack instead of respinning the
> whole series.
> 
> Anyone for a tie breaker?

I am aggressively undecided.  I guess as it's a bit of a 51/49
situation, the "stay with what people are familiar with" benefit tips the
balance toward the legacy names?

[toc] | [prev] | [next] | [standalone]

#1444177

On Wed, Jul 13, 2016 at 02:13:43PM -0700, Andrew Morton wrote:
> On Wed, 13 Jul 2016 14:37:01 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:
> 
> > > 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?
> > > 
> > 
> > In this case, I'm going to ask the other people on the cc for a
> > tie-breaker. If someone else prefers the old names then I'm happy for
> > your patch to be applied on top with my ack instead of respinning the
> > whole series.
> > 
> > Anyone for a tie breaker?
> 
> I am aggressively undecided.  I guess as it's a bit of a 51/49
> situation, the "stay with what people are familiar with" benefit tips the
> balance toward the legacy names?
> 

I still can't decide. It's currently still a draw in terms of naming. If
you're worried, use the old naming. It wouldn't be the first time I
thought a name was odd.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1444653

On Fri, 15 Jul 2016 11:46:05 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:

> On Wed, Jul 13, 2016 at 02:13:43PM -0700, Andrew Morton wrote:
> > On Wed, 13 Jul 2016 14:37:01 +0100 Mel Gorman <mgorman@techsingularity.net> wrote:
> > 
> > > > 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?
> > > > 
> > > 
> > > In this case, I'm going to ask the other people on the cc for a
> > > tie-breaker. If someone else prefers the old names then I'm happy for
> > > your patch to be applied on top with my ack instead of respinning the
> > > whole series.
> > > 
> > > Anyone for a tie breaker?
> > 
> > I am aggressively undecided.  I guess as it's a bit of a 51/49
> > situation, the "stay with what people are familiar with" benefit tips the
> > balance toward the legacy names?
> > 
> 
> I still can't decide. It's currently still a draw in terms of naming. If
> you're worried, use the old naming. It wouldn't be the first time I
> thought a name was odd.

Well I dunno.  We can leave the series as-is for now and we can merge
the rename-it-back patch sometime during the next -rc cycle if we find
that people are running around in confusion and tumbling out of high
windows.

[toc] | [prev] | [next] | [standalone]

#1445490

On Fri, Jul 15, 2016 at 03:35:54PM -0700, Andrew Morton wrote:
> Well I dunno.  We can leave the series as-is for now and we can merge
> the rename-it-back patch sometime during the next -rc cycle if we find
> that people are running around in confusion and tumbling out of high
> windows.

Sounds good to me.

[toc] | [prev] | [next] | [standalone]

#1442983

On Wed, Jul 13, 2016 at 02:37:01PM +0100, Mel Gorman wrote:
> On Wed, Jul 13, 2016 at 09:04:15AM -0400, Johannes Weiner wrote:
> > > 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.
> > 
> 
> From a reclaim perspective, I consider the mapped state to be more
> important. This is particularly true when the advise calls are taken
> into account. For example, madvise unmaps the pages without affecting
> memory residency (distinct from RSS) without aging. fadvise ignores mapped
> pages so the mapped state is very important for advise hints.  Similarly,
> the mapped state can affect how the pages are aged as mapped pages affect
> slab scan rates and incur TLB flushes on unmap. I guess I've been thinking
> about mapped/unmapped a lot recently which pushed me towards distinct naming.
> 
> > 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 was tempted to rename userspace what is printed to vmstat as well but
> worried about breaking tools that parse it.
> 
> > 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?
> > 
> 
> In this case, I'm going to ask the other people on the cc for a
> tie-breaker. If someone else prefers the old names then I'm happy for
> your patch to be applied on top with my ack instead of respinning the
> whole series.
> 
> Anyone for a tie breaker?

I have thought it from reclaim perspective for a long time so I tempted to
change the naming like new one but there is no big justification for that.
In this chance, I vote new name.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web