[PATCH 0/6] Follow up work on NUMA Balancing

[PATCH 0/6] Follow up work on NUMA Balancing

[Mel Gorman]

The following series is a few follow-up patches left over from NUMA
balancing. The three three patches are tiny fixes. Patches 4 and 5 fold
page->_last_nid into page->flags and is entirely based on work from Peter
Zijlstra. The final patch is a cleanup by Hugh Dickins that he had marked
as a prototype but on examination and testing I could not find any problems
with it (famous last words).

 include/linux/mm.h                |   73 ++++++++++++---------------
 include/linux/mm_types.h          |    9 ++--
 include/linux/mmzone.h            |   22 +--------
 include/linux/page-flags-layout.h |   88 +++++++++++++++++++++++++++++++++
 include/linux/vmstat.h            |    2 +-
 mm/huge_memory.c                  |   28 ++++-------
 mm/memory.c                       |    4 ++
 mm/migrate.c                      |   99 +++++++++++++++++--------------------
 8 files changed, 186 insertions(+), 139 deletions(-)
 create mode 100644 include/linux/page-flags-layout.h

-- 
1.7.9.2

[PATCH 0/6] Follow up work on NUMA Balancing

[Mel Gorman]

The following series is a few follow-up patches left over from NUMA
balancing. The three three patches are tiny fixes. Patches 4 and 5 fold
page->_last_nid into page->flags and is entirely based on work from Peter
Zijlstra. The final patch is a cleanup by Hugh Dickins that he had marked
as a prototype but on examination and testing I could not find any problems
with it (famous last words).

 include/linux/mm.h                |   73 ++++++++++++---------------
 include/linux/mm_types.h          |    9 ++--
 include/linux/mmzone.h            |   22 +--------
 include/linux/page-flags-layout.h |   88 +++++++++++++++++++++++++++++++++
 include/linux/vmstat.h            |    2 +-
 mm/huge_memory.c                  |   28 ++++-------
 mm/memory.c                       |    4 ++
 mm/migrate.c                      |   99 +++++++++++++++++--------------------
 8 files changed, 186 insertions(+), 139 deletions(-)
 create mode 100644 include/linux/page-flags-layout.h

-- 
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/6] mm: numa: Fix minor typo in numa_next_scan

[Mel Gorman]

s/me/be/ and clarify the comment a bit when we're changing it anyway.

Suggested-by: Simon Jeons <simon.jeons@gmail.com>
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/mm_types.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index f8f5162a..47047cb 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -414,9 +414,9 @@ struct mm_struct {
 #endif
 #ifdef CONFIG_NUMA_BALANCING
 	/*
-	 * numa_next_scan is the next time when the PTEs will me marked
-	 * pte_numa to gather statistics and migrate pages to new nodes
-	 * if necessary
+	 * numa_next_scan is the next time that the PTEs will be marked
+	 * pte_numa. NUMA hinting faults will gather statistics and migrate
+	 * pages to new nodes if necessary.
 	 */
 	unsigned long numa_next_scan;
 
-- 
1.7.9.2

[PATCH 1/6] mm: numa: Fix minor typo in numa_next_scan

[Mel Gorman]

s/me/be/ and clarify the comment a bit when we're changing it anyway.

Suggested-by: Simon Jeons <simon.jeons@gmail.com>
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/mm_types.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index f8f5162a..47047cb 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -414,9 +414,9 @@ struct mm_struct {
 #endif
 #ifdef CONFIG_NUMA_BALANCING
 	/*
-	 * numa_next_scan is the next time when the PTEs will me marked
-	 * pte_numa to gather statistics and migrate pages to new nodes
-	 * if necessary
+	 * numa_next_scan is the next time that the PTEs will be marked
+	 * pte_numa. NUMA hinting faults will gather statistics and migrate
+	 * pages to new nodes if necessary.
 	 */
 	unsigned long numa_next_scan;
 
-- 
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/6] mm: numa: Take THP into account when migrating pages for NUMA balancing

[Mel Gorman]

Wanpeng Li pointed out that numamigrate_isolate_page() assumes that only one
base page is being migrated when in fact it can also be checking THP. The
consequences are that a migration will be attempted when a target node
is nearly full and fail later. It's unlikely to be user-visible but it
should be fixed. While we are there, migrate_balanced_pgdat() should treat
nr_migrate_pages as an unsigned long as it is treated as a watermark.

Suggested-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 mm/migrate.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index c387786..73e432d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1459,7 +1459,7 @@ int migrate_vmas(struct mm_struct *mm, const nodemask_t *to,
  * pages. Currently it only checks the watermarks which crude
  */
 static bool migrate_balanced_pgdat(struct pglist_data *pgdat,
-				   int nr_migrate_pages)
+				   unsigned long nr_migrate_pages)
 {
 	int z;
 	for (z = pgdat->nr_zones - 1; z >= 0; z--) {
@@ -1557,8 +1557,10 @@ int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 {
 	int ret = 0;
 
+	VM_BUG_ON(compound_order(page) && !PageTransHuge(page));
+
 	/* Avoid migrating to a node that is nearly full */
-	if (migrate_balanced_pgdat(pgdat, 1)) {
+	if (migrate_balanced_pgdat(pgdat, 1UL << compound_order(page))) {
 		int page_lru;
 
 		if (isolate_lru_page(page)) {
-- 
1.7.9.2

[PATCH 2/6] mm: numa: Take THP into account when migrating pages for NUMA balancing

[Mel Gorman]

Wanpeng Li pointed out that numamigrate_isolate_page() assumes that only one
base page is being migrated when in fact it can also be checking THP. The
consequences are that a migration will be attempted when a target node
is nearly full and fail later. It's unlikely to be user-visible but it
should be fixed. While we are there, migrate_balanced_pgdat() should treat
nr_migrate_pages as an unsigned long as it is treated as a watermark.

Suggested-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 mm/migrate.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index c387786..73e432d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1459,7 +1459,7 @@ int migrate_vmas(struct mm_struct *mm, const nodemask_t *to,
  * pages. Currently it only checks the watermarks which crude
  */
 static bool migrate_balanced_pgdat(struct pglist_data *pgdat,
-				   int nr_migrate_pages)
+				   unsigned long nr_migrate_pages)
 {
 	int z;
 	for (z = pgdat->nr_zones - 1; z >= 0; z--) {
@@ -1557,8 +1557,10 @@ int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 {
 	int ret = 0;
 
+	VM_BUG_ON(compound_order(page) && !PageTransHuge(page));
+
 	/* Avoid migrating to a node that is nearly full */
-	if (migrate_balanced_pgdat(pgdat, 1)) {
+	if (migrate_balanced_pgdat(pgdat, 1UL << compound_order(page))) {
 		int page_lru;
 
 		if (isolate_lru_page(page)) {
-- 
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/6] mm: numa: Handle side-effects in count_vm_numa_events() for !CONFIG_NUMA_BALANCING

[Mel Gorman]

The current definitions for count_vm_numa_events() is wrong for
!CONFIG_NUMA_BALANCING as the following would miss the side-effect.

	count_vm_numa_events(NUMA_FOO, bar++);

There are no such users of count_vm_numa_events() but it is a potential
pitfall. This patch fixes it and converts count_vm_numa_event() so that
the definitions look similar.

Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/vmstat.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
index a13291f..5fd71a7 100644
--- a/include/linux/vmstat.h
+++ b/include/linux/vmstat.h
@@ -85,7 +85,7 @@ static inline void vm_events_fold_cpu(int cpu)
 #define count_vm_numa_events(x, y) count_vm_events(x, y)
 #else
 #define count_vm_numa_event(x) do {} while (0)
-#define count_vm_numa_events(x, y) do {} while (0)
+#define count_vm_numa_events(x, y) do { (void)(y); } while (0)
 #endif /* CONFIG_NUMA_BALANCING */
 
 #define __count_zone_vm_events(item, zone, delta) \
-- 
1.7.9.2

[PATCH 3/6] mm: numa: Handle side-effects in count_vm_numa_events() for !CONFIG_NUMA_BALANCING

[Mel Gorman]

The current definitions for count_vm_numa_events() is wrong for
!CONFIG_NUMA_BALANCING as the following would miss the side-effect.

	count_vm_numa_events(NUMA_FOO, bar++);

There are no such users of count_vm_numa_events() but it is a potential
pitfall. This patch fixes it and converts count_vm_numa_event() so that
the definitions look similar.

Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/vmstat.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
index a13291f..5fd71a7 100644
--- a/include/linux/vmstat.h
+++ b/include/linux/vmstat.h
@@ -85,7 +85,7 @@ static inline void vm_events_fold_cpu(int cpu)
 #define count_vm_numa_events(x, y) count_vm_events(x, y)
 #else
 #define count_vm_numa_event(x) do {} while (0)
-#define count_vm_numa_events(x, y) do {} while (0)
+#define count_vm_numa_events(x, y) do { (void)(y); } while (0)
 #endif /* CONFIG_NUMA_BALANCING */
 
 #define __count_zone_vm_events(item, zone, delta) \
-- 
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/6] mm: numa: Handle side-effects in count_vm_numa_events() for !CONFIG_NUMA_BALANCING

[Andrew Morton]

On Tue, 22 Jan 2013 17:12:39 +0000
Mel Gorman <mgorman@suse.de> wrote:

> The current definitions for count_vm_numa_events() is wrong for
> !CONFIG_NUMA_BALANCING as the following would miss the side-effect.
> 
> 	count_vm_numa_events(NUMA_FOO, bar++);

Stupid macros.

> There are no such users of count_vm_numa_events() but it is a potential
> pitfall. This patch fixes it and converts count_vm_numa_event() so that
> the definitions look similar.

Confused.  The patch doesn't alter count_vm_numa_event().  No matter.

> --- a/include/linux/vmstat.h
> +++ b/include/linux/vmstat.h
> @@ -85,7 +85,7 @@ static inline void vm_events_fold_cpu(int cpu)
>  #define count_vm_numa_events(x, y) count_vm_events(x, y)
>  #else
>  #define count_vm_numa_event(x) do {} while (0)
> -#define count_vm_numa_events(x, y) do {} while (0)
> +#define count_vm_numa_events(x, y) do { (void)(y); } while (0)
>  #endif /* CONFIG_NUMA_BALANCING */
>  
>  #define __count_zone_vm_events(item, zone, delta) \

Re: [PATCH 3/6] mm: numa: Handle side-effects in count_vm_numa_events() for !CONFIG_NUMA_BALANCING

[Andrew Morton]

On Tue, 22 Jan 2013 17:12:39 +0000
Mel Gorman <mgorman@suse.de> wrote:

> The current definitions for count_vm_numa_events() is wrong for
> !CONFIG_NUMA_BALANCING as the following would miss the side-effect.
> 
> 	count_vm_numa_events(NUMA_FOO, bar++);

Stupid macros.

> There are no such users of count_vm_numa_events() but it is a potential
> pitfall. This patch fixes it and converts count_vm_numa_event() so that
> the definitions look similar.

Confused.  The patch doesn't alter count_vm_numa_event().  No matter.

> --- a/include/linux/vmstat.h
> +++ b/include/linux/vmstat.h
> @@ -85,7 +85,7 @@ static inline void vm_events_fold_cpu(int cpu)
>  #define count_vm_numa_events(x, y) count_vm_events(x, y)
>  #else
>  #define count_vm_numa_event(x) do {} while (0)
> -#define count_vm_numa_events(x, y) do {} while (0)
> +#define count_vm_numa_events(x, y) do { (void)(y); } while (0)
>  #endif /* CONFIG_NUMA_BALANCING */
>  
>  #define __count_zone_vm_events(item, zone, delta) \

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/6] mm: numa: Handle side-effects in count_vm_numa_events() for !CONFIG_NUMA_BALANCING

[Mel Gorman]

On Tue, Jan 22, 2013 at 02:40:24PM -0800, Andrew Morton wrote:
> On Tue, 22 Jan 2013 17:12:39 +0000
> Mel Gorman <mgorman@suse.de> wrote:
> 
> > The current definitions for count_vm_numa_events() is wrong for
> > !CONFIG_NUMA_BALANCING as the following would miss the side-effect.
> > 
> > 	count_vm_numa_events(NUMA_FOO, bar++);
> 
> Stupid macros.
> 

I know but static inlines are unsuitable in this case.

> > There are no such users of count_vm_numa_events() but it is a potential
> > pitfall. This patch fixes it and converts count_vm_numa_event() so that
> > the definitions look similar.
> 
> Confused.  The patch doesn't alter count_vm_numa_event().  No matter.
> 

Nuts. When I wrote that line in the changelog, it
was because I had converted both to a static inline but that fails to
compile if !CONFIG_NUMA_BALANCING because NUMA_PTE_UPDATES is not
defined.

===
There are no such users of count_vm_numa_events() but this patch fixes it as
it is a potential pitfall. Ideally both would be converted to static inline
but NUMA_PTE_UPDATES is not defined if !CONFIG_NUMA_BALANCING and creating
dummy constants just to have a static inline would be similarly clumsy.
====

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 3/6] mm: numa: Handle side-effects in count_vm_numa_events() for !CONFIG_NUMA_BALANCING

[Mel Gorman]

On Tue, Jan 22, 2013 at 02:40:24PM -0800, Andrew Morton wrote:
> On Tue, 22 Jan 2013 17:12:39 +0000
> Mel Gorman <mgorman@suse.de> wrote:
> 
> > The current definitions for count_vm_numa_events() is wrong for
> > !CONFIG_NUMA_BALANCING as the following would miss the side-effect.
> > 
> > 	count_vm_numa_events(NUMA_FOO, bar++);
> 
> Stupid macros.
> 

I know but static inlines are unsuitable in this case.

> > There are no such users of count_vm_numa_events() but it is a potential
> > pitfall. This patch fixes it and converts count_vm_numa_event() so that
> > the definitions look similar.
> 
> Confused.  The patch doesn't alter count_vm_numa_event().  No matter.
> 

Nuts. When I wrote that line in the changelog, it
was because I had converted both to a static inline but that fails to
compile if !CONFIG_NUMA_BALANCING because NUMA_PTE_UPDATES is not
defined.

===
There are no such users of count_vm_numa_events() but this patch fixes it as
it is a potential pitfall. Ideally both would be converted to static inline
but NUMA_PTE_UPDATES is not defined if !CONFIG_NUMA_BALANCING and creating
dummy constants just to have a static inline would be similarly clumsy.
====

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/6] mm: Move page flags layout to separate header

[Mel Gorman]

From: Peter Zijlstra <a.p.zijlstra@chello.nl>

This is a preparation patch for moving page->_last_nid into page->flags
that moves page flag layout information to a separate header. This patch
is necessary because otherwise there would be a circular dependency
between mm_types.h and mm.h.

Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/mm.h                |   40 ---------------------
 include/linux/mm_types.h          |    1 +
 include/linux/mmzone.h            |   22 +-----------
 include/linux/page-flags-layout.h |   71 +++++++++++++++++++++++++++++++++++++
 4 files changed, 73 insertions(+), 61 deletions(-)
 create mode 100644 include/linux/page-flags-layout.h

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 66e2f7c..87420e6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -580,52 +580,12 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
  * sets it, so none of the operations on it need to be atomic.
  */
 
-
-/*
- * page->flags layout:
- *
- * There are three possibilities for how page->flags get
- * laid out.  The first is for the normal case, without
- * sparsemem.  The second is for sparsemem when there is
- * plenty of space for node and section.  The last is when
- * we have run out of space and have to fall back to an
- * alternate (slower) way of determining the node.
- *
- * No sparsemem or sparsemem vmemmap: |       NODE     | ZONE | ... | FLAGS |
- * classic sparse with space for node:| SECTION | NODE | ZONE | ... | FLAGS |
- * classic sparse no space for node:  | SECTION |     ZONE    | ... | FLAGS |
- */
-#if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
-#define SECTIONS_WIDTH		SECTIONS_SHIFT
-#else
-#define SECTIONS_WIDTH		0
-#endif
-
-#define ZONES_WIDTH		ZONES_SHIFT
-
-#if SECTIONS_WIDTH+ZONES_WIDTH+NODES_SHIFT <= BITS_PER_LONG - NR_PAGEFLAGS
-#define NODES_WIDTH		NODES_SHIFT
-#else
-#ifdef CONFIG_SPARSEMEM_VMEMMAP
-#error "Vmemmap: No space for nodes field in page flags"
-#endif
-#define NODES_WIDTH		0
-#endif
-
 /* Page flags: | [SECTION] | [NODE] | ZONE | ... | FLAGS | */
 #define SECTIONS_PGOFF		((sizeof(unsigned long)*8) - SECTIONS_WIDTH)
 #define NODES_PGOFF		(SECTIONS_PGOFF - NODES_WIDTH)
 #define ZONES_PGOFF		(NODES_PGOFF - ZONES_WIDTH)
 
 /*
- * We are going to use the flags for the page to node mapping if its in
- * there.  This includes the case where there is no node, so it is implicit.
- */
-#if !(NODES_WIDTH > 0 || NODES_SHIFT == 0)
-#define NODE_NOT_IN_PAGE_FLAGS
-#endif
-
-/*
  * Define the bit shifts to access each section.  For non-existent
  * sections we define the shift as 0; that plus a 0 mask ensures
  * the compiler will optimise away reference to them.
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 47047cb..d05d632 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -12,6 +12,7 @@
 #include <linux/cpumask.h>
 #include <linux/page-debug-flags.h>
 #include <linux/uprobes.h>
+#include <linux/page-flags-layout.h>
 #include <asm/page.h>
 #include <asm/mmu.h>
 
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 73b64a3..5e6a814 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -15,7 +15,7 @@
 #include <linux/seqlock.h>
 #include <linux/nodemask.h>
 #include <linux/pageblock-flags.h>
-#include <generated/bounds.h>
+#include <linux/page-flags-layout.h>
 #include <linux/atomic.h>
 #include <asm/page.h>
 
@@ -308,24 +308,6 @@ enum zone_type {
 
 #ifndef __GENERATING_BOUNDS_H
 
-/*
- * When a memory allocation must conform to specific limitations (such
- * as being suitable for DMA) the caller will pass in hints to the
- * allocator in the gfp_mask, in the zone modifier bits.  These bits
- * are used to select a priority ordered list of memory zones which
- * match the requested limits. See gfp_zone() in include/linux/gfp.h
- */
-
-#if MAX_NR_ZONES < 2
-#define ZONES_SHIFT 0
-#elif MAX_NR_ZONES <= 2
-#define ZONES_SHIFT 1
-#elif MAX_NR_ZONES <= 4
-#define ZONES_SHIFT 2
-#else
-#error ZONES_SHIFT -- too many zones configured adjust calculation
-#endif
-
 struct zone {
 	/* Fields commonly accessed by the page allocator */
 
@@ -1053,8 +1035,6 @@ static inline unsigned long early_pfn_to_nid(unsigned long pfn)
  * PA_SECTION_SHIFT		physical address to/from section number
  * PFN_SECTION_SHIFT		pfn to/from section number
  */
-#define SECTIONS_SHIFT		(MAX_PHYSMEM_BITS - SECTION_SIZE_BITS)
-
 #define PA_SECTION_SHIFT	(SECTION_SIZE_BITS)
 #define PFN_SECTION_SHIFT	(SECTION_SIZE_BITS - PAGE_SHIFT)
 
diff --git a/include/linux/page-flags-layout.h b/include/linux/page-flags-layout.h
new file mode 100644
index 0000000..316805d
--- /dev/null
+++ b/include/linux/page-flags-layout.h
@@ -0,0 +1,71 @@
+#ifndef PAGE_FLAGS_LAYOUT_H
+#define PAGE_FLAGS_LAYOUT_H
+
+#include <linux/numa.h>
+#include <generated/bounds.h>
+
+/*
+ * When a memory allocation must conform to specific limitations (such
+ * as being suitable for DMA) the caller will pass in hints to the
+ * allocator in the gfp_mask, in the zone modifier bits.  These bits
+ * are used to select a priority ordered list of memory zones which
+ * match the requested limits. See gfp_zone() in include/linux/gfp.h
+ */
+#if MAX_NR_ZONES < 2
+#define ZONES_SHIFT 0
+#elif MAX_NR_ZONES <= 2
+#define ZONES_SHIFT 1
+#elif MAX_NR_ZONES <= 4
+#define ZONES_SHIFT 2
+#else
+#error ZONES_SHIFT -- too many zones configured adjust calculation
+#endif
+
+#ifdef CONFIG_SPARSEMEM
+#include <asm/sparsemem.h>
+
+/* SECTION_SHIFT	#bits space required to store a section # */
+#define SECTIONS_SHIFT	(MAX_PHYSMEM_BITS - SECTION_SIZE_BITS)
+
+#endif /* CONFIG_SPARSEMEM */
+
+/*
+ * page->flags layout:
+ *
+ * There are three possibilities for how page->flags get
+ * laid out.  The first is for the normal case, without
+ * sparsemem.  The second is for sparsemem when there is
+ * plenty of space for node and section.  The last is when
+ * we have run out of space and have to fall back to an
+ * alternate (slower) way of determining the node.
+ *
+ * No sparsemem or sparsemem vmemmap: |       NODE     | ZONE | ... | FLAGS |
+ * classic sparse with space for node:| SECTION | NODE | ZONE | ... | FLAGS |
+ * classic sparse no space for node:  | SECTION |     ZONE    | ... | FLAGS |
+ */
+#if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
+#define SECTIONS_WIDTH		SECTIONS_SHIFT
+#else
+#define SECTIONS_WIDTH		0
+#endif
+
+#define ZONES_WIDTH		ZONES_SHIFT
+
+#if SECTIONS_WIDTH+ZONES_WIDTH+NODES_SHIFT <= BITS_PER_LONG - NR_PAGEFLAGS
+#define NODES_WIDTH		NODES_SHIFT
+#else
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
+#error "Vmemmap: No space for nodes field in page flags"
+#endif
+#define NODES_WIDTH		0
+#endif
+
+/*
+ * We are going to use the flags for the page to node mapping if its in
+ * there.  This includes the case where there is no node, so it is implicit.
+ */
+#if !(NODES_WIDTH > 0 || NODES_SHIFT == 0)
+#define NODE_NOT_IN_PAGE_FLAGS
+#endif
+
+#endif /* _LINUX_PAGE_FLAGS_LAYOUT */
-- 
1.7.9.2

[PATCH 4/6] mm: Move page flags layout to separate header

[Mel Gorman]

From: Peter Zijlstra <a.p.zijlstra@chello.nl>

This is a preparation patch for moving page->_last_nid into page->flags
that moves page flag layout information to a separate header. This patch
is necessary because otherwise there would be a circular dependency
between mm_types.h and mm.h.

Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/mm.h                |   40 ---------------------
 include/linux/mm_types.h          |    1 +
 include/linux/mmzone.h            |   22 +-----------
 include/linux/page-flags-layout.h |   71 +++++++++++++++++++++++++++++++++++++
 4 files changed, 73 insertions(+), 61 deletions(-)
 create mode 100644 include/linux/page-flags-layout.h

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 66e2f7c..87420e6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -580,52 +580,12 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
  * sets it, so none of the operations on it need to be atomic.
  */
 
-
-/*
- * page->flags layout:
- *
- * There are three possibilities for how page->flags get
- * laid out.  The first is for the normal case, without
- * sparsemem.  The second is for sparsemem when there is
- * plenty of space for node and section.  The last is when
- * we have run out of space and have to fall back to an
- * alternate (slower) way of determining the node.
- *
- * No sparsemem or sparsemem vmemmap: |       NODE     | ZONE | ... | FLAGS |
- * classic sparse with space for node:| SECTION | NODE | ZONE | ... | FLAGS |
- * classic sparse no space for node:  | SECTION |     ZONE    | ... | FLAGS |
- */
-#if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
-#define SECTIONS_WIDTH		SECTIONS_SHIFT
-#else
-#define SECTIONS_WIDTH		0
-#endif
-
-#define ZONES_WIDTH		ZONES_SHIFT
-
-#if SECTIONS_WIDTH+ZONES_WIDTH+NODES_SHIFT <= BITS_PER_LONG - NR_PAGEFLAGS
-#define NODES_WIDTH		NODES_SHIFT
-#else
-#ifdef CONFIG_SPARSEMEM_VMEMMAP
-#error "Vmemmap: No space for nodes field in page flags"
-#endif
-#define NODES_WIDTH		0
-#endif
-
 /* Page flags: | [SECTION] | [NODE] | ZONE | ... | FLAGS | */
 #define SECTIONS_PGOFF		((sizeof(unsigned long)*8) - SECTIONS_WIDTH)
 #define NODES_PGOFF		(SECTIONS_PGOFF - NODES_WIDTH)
 #define ZONES_PGOFF		(NODES_PGOFF - ZONES_WIDTH)
 
 /*
- * We are going to use the flags for the page to node mapping if its in
- * there.  This includes the case where there is no node, so it is implicit.
- */
-#if !(NODES_WIDTH > 0 || NODES_SHIFT == 0)
-#define NODE_NOT_IN_PAGE_FLAGS
-#endif
-
-/*
  * Define the bit shifts to access each section.  For non-existent
  * sections we define the shift as 0; that plus a 0 mask ensures
  * the compiler will optimise away reference to them.
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 47047cb..d05d632 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -12,6 +12,7 @@
 #include <linux/cpumask.h>
 #include <linux/page-debug-flags.h>
 #include <linux/uprobes.h>
+#include <linux/page-flags-layout.h>
 #include <asm/page.h>
 #include <asm/mmu.h>
 
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 73b64a3..5e6a814 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -15,7 +15,7 @@
 #include <linux/seqlock.h>
 #include <linux/nodemask.h>
 #include <linux/pageblock-flags.h>
-#include <generated/bounds.h>
+#include <linux/page-flags-layout.h>
 #include <linux/atomic.h>
 #include <asm/page.h>
 
@@ -308,24 +308,6 @@ enum zone_type {
 
 #ifndef __GENERATING_BOUNDS_H
 
-/*
- * When a memory allocation must conform to specific limitations (such
- * as being suitable for DMA) the caller will pass in hints to the
- * allocator in the gfp_mask, in the zone modifier bits.  These bits
- * are used to select a priority ordered list of memory zones which
- * match the requested limits. See gfp_zone() in include/linux/gfp.h
- */
-
-#if MAX_NR_ZONES < 2
-#define ZONES_SHIFT 0
-#elif MAX_NR_ZONES <= 2
-#define ZONES_SHIFT 1
-#elif MAX_NR_ZONES <= 4
-#define ZONES_SHIFT 2
-#else
-#error ZONES_SHIFT -- too many zones configured adjust calculation
-#endif
-
 struct zone {
 	/* Fields commonly accessed by the page allocator */
 
@@ -1053,8 +1035,6 @@ static inline unsigned long early_pfn_to_nid(unsigned long pfn)
  * PA_SECTION_SHIFT		physical address to/from section number
  * PFN_SECTION_SHIFT		pfn to/from section number
  */
-#define SECTIONS_SHIFT		(MAX_PHYSMEM_BITS - SECTION_SIZE_BITS)
-
 #define PA_SECTION_SHIFT	(SECTION_SIZE_BITS)
 #define PFN_SECTION_SHIFT	(SECTION_SIZE_BITS - PAGE_SHIFT)
 
diff --git a/include/linux/page-flags-layout.h b/include/linux/page-flags-layout.h
new file mode 100644
index 0000000..316805d
--- /dev/null
+++ b/include/linux/page-flags-layout.h
@@ -0,0 +1,71 @@
+#ifndef PAGE_FLAGS_LAYOUT_H
+#define PAGE_FLAGS_LAYOUT_H
+
+#include <linux/numa.h>
+#include <generated/bounds.h>
+
+/*
+ * When a memory allocation must conform to specific limitations (such
+ * as being suitable for DMA) the caller will pass in hints to the
+ * allocator in the gfp_mask, in the zone modifier bits.  These bits
+ * are used to select a priority ordered list of memory zones which
+ * match the requested limits. See gfp_zone() in include/linux/gfp.h
+ */
+#if MAX_NR_ZONES < 2
+#define ZONES_SHIFT 0
+#elif MAX_NR_ZONES <= 2
+#define ZONES_SHIFT 1
+#elif MAX_NR_ZONES <= 4
+#define ZONES_SHIFT 2
+#else
+#error ZONES_SHIFT -- too many zones configured adjust calculation
+#endif
+
+#ifdef CONFIG_SPARSEMEM
+#include <asm/sparsemem.h>
+
+/* SECTION_SHIFT	#bits space required to store a section # */
+#define SECTIONS_SHIFT	(MAX_PHYSMEM_BITS - SECTION_SIZE_BITS)
+
+#endif /* CONFIG_SPARSEMEM */
+
+/*
+ * page->flags layout:
+ *
+ * There are three possibilities for how page->flags get
+ * laid out.  The first is for the normal case, without
+ * sparsemem.  The second is for sparsemem when there is
+ * plenty of space for node and section.  The last is when
+ * we have run out of space and have to fall back to an
+ * alternate (slower) way of determining the node.
+ *
+ * No sparsemem or sparsemem vmemmap: |       NODE     | ZONE | ... | FLAGS |
+ * classic sparse with space for node:| SECTION | NODE | ZONE | ... | FLAGS |
+ * classic sparse no space for node:  | SECTION |     ZONE    | ... | FLAGS |
+ */
+#if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
+#define SECTIONS_WIDTH		SECTIONS_SHIFT
+#else
+#define SECTIONS_WIDTH		0
+#endif
+
+#define ZONES_WIDTH		ZONES_SHIFT
+
+#if SECTIONS_WIDTH+ZONES_WIDTH+NODES_SHIFT <= BITS_PER_LONG - NR_PAGEFLAGS
+#define NODES_WIDTH		NODES_SHIFT
+#else
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
+#error "Vmemmap: No space for nodes field in page flags"
+#endif
+#define NODES_WIDTH		0
+#endif
+
+/*
+ * We are going to use the flags for the page to node mapping if its in
+ * there.  This includes the case where there is no node, so it is implicit.
+ */
+#if !(NODES_WIDTH > 0 || NODES_SHIFT == 0)
+#define NODE_NOT_IN_PAGE_FLAGS
+#endif
+
+#endif /* _LINUX_PAGE_FLAGS_LAYOUT */
-- 
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[Mel Gorman]

From: Peter Zijlstra <a.p.zijlstra@chello.nl>

page->_last_nid fits into page->flags on 64-bit. The unlikely 32-bit NUMA
configuration with NUMA Balancing will still need an extra page field.
As Peter notes "Completely dropping 32bit support for CONFIG_NUMA_BALANCING
would simplify things, but it would also remove the warning if we grow
enough 64bit only page-flags to push the last-cpu out."

[mgorman@suse.de: Minor modifications]
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/mm.h                |   33 ++++++++++++++++++++++++++++++++-
 include/linux/mm_types.h          |    2 +-
 include/linux/page-flags-layout.h |   33 +++++++++++++++++++++++++--------
 mm/memory.c                       |    4 ++++
 4 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 87420e6..e25d47f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -580,10 +580,11 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
  * sets it, so none of the operations on it need to be atomic.
  */
 
-/* Page flags: | [SECTION] | [NODE] | ZONE | ... | FLAGS | */
+/* Page flags: | [SECTION] | [NODE] | ZONE | [LAST_NID] | ... | FLAGS | */
 #define SECTIONS_PGOFF		((sizeof(unsigned long)*8) - SECTIONS_WIDTH)
 #define NODES_PGOFF		(SECTIONS_PGOFF - NODES_WIDTH)
 #define ZONES_PGOFF		(NODES_PGOFF - ZONES_WIDTH)
+#define LAST_NID_PGOFF		(ZONES_PGOFF - LAST_NID_WIDTH)
 
 /*
  * Define the bit shifts to access each section.  For non-existent
@@ -593,6 +594,7 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
 #define SECTIONS_PGSHIFT	(SECTIONS_PGOFF * (SECTIONS_WIDTH != 0))
 #define NODES_PGSHIFT		(NODES_PGOFF * (NODES_WIDTH != 0))
 #define ZONES_PGSHIFT		(ZONES_PGOFF * (ZONES_WIDTH != 0))
+#define LAST_NID_PGSHIFT	(LAST_NID_PGOFF * (LAST_NID_WIDTH != 0))
 
 /* NODE:ZONE or SECTION:ZONE is used to ID a zone for the buddy allocator */
 #ifdef NODE_NOT_IN_PAGE_FLAGS
@@ -614,6 +616,7 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
 #define ZONES_MASK		((1UL << ZONES_WIDTH) - 1)
 #define NODES_MASK		((1UL << NODES_WIDTH) - 1)
 #define SECTIONS_MASK		((1UL << SECTIONS_WIDTH) - 1)
+#define LAST_NID_MASK		((1UL << LAST_NID_WIDTH) - 1)
 #define ZONEID_MASK		((1UL << ZONEID_SHIFT) - 1)
 
 static inline enum zone_type page_zonenum(const struct page *page)
@@ -653,6 +656,7 @@ static inline int page_to_nid(const struct page *page)
 #endif
 
 #ifdef CONFIG_NUMA_BALANCING
+#ifdef LAST_NID_NOT_IN_PAGE_FLAGS
 static inline int page_xchg_last_nid(struct page *page, int nid)
 {
 	return xchg(&page->_last_nid, nid);
@@ -667,6 +671,33 @@ static inline void reset_page_last_nid(struct page *page)
 	page->_last_nid = -1;
 }
 #else
+static inline int page_last_nid(struct page *page)
+{
+	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
+}
+
+static inline int page_xchg_last_nid(struct page *page, int nid)
+{
+	unsigned long old_flags, flags;
+	int last_nid;
+
+	do {
+		old_flags = flags = page->flags;
+		last_nid = page_last_nid(page);
+
+		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
+		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
+	} while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags));
+
+	return last_nid;
+}
+
+static inline void reset_page_last_nid(struct page *page)
+{
+	page_xchg_last_nid(page, (1 << LAST_NID_SHIFT) - 1);
+}
+#endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
+#else
 static inline int page_xchg_last_nid(struct page *page, int nid)
 {
 	return page_to_nid(page);
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index d05d632..ace9a5f 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -174,7 +174,7 @@ struct page {
 	void *shadow;
 #endif
 
-#ifdef CONFIG_NUMA_BALANCING
+#ifdef LAST_NID_NOT_IN_PAGE_FLAGS
 	int _last_nid;
 #endif
 }
diff --git a/include/linux/page-flags-layout.h b/include/linux/page-flags-layout.h
index 316805d..93506a1 100644
--- a/include/linux/page-flags-layout.h
+++ b/include/linux/page-flags-layout.h
@@ -32,15 +32,16 @@
 /*
  * page->flags layout:
  *
- * There are three possibilities for how page->flags get
- * laid out.  The first is for the normal case, without
- * sparsemem.  The second is for sparsemem when there is
- * plenty of space for node and section.  The last is when
- * we have run out of space and have to fall back to an
- * alternate (slower) way of determining the node.
+ * There are five possibilities for how page->flags get laid out.  The first
+ * pair is for the normal case without sparsemem. The second pair is for
+ * sparsemem when there is plenty of space for node and section information.
+ * The last is when there is insufficient space in page->flags and a separate
+ * lookup is necessary.
  *
- * No sparsemem or sparsemem vmemmap: |       NODE     | ZONE | ... | FLAGS |
- * classic sparse with space for node:| SECTION | NODE | ZONE | ... | FLAGS |
+ * No sparsemem or sparsemem vmemmap: |       NODE     | ZONE |          ... | FLAGS |
+ *         " plus space for last_nid: |       NODE     | ZONE | LAST_NID ... | FLAGS |
+ * classic sparse with space for node:| SECTION | NODE | ZONE |          ... | FLAGS |
+ *         " plus space for last_nid: | SECTION | NODE | ZONE | LAST_NID ... | FLAGS |
  * classic sparse no space for node:  | SECTION |     ZONE    | ... | FLAGS |
  */
 #if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
@@ -60,6 +61,18 @@
 #define NODES_WIDTH		0
 #endif
 
+#ifdef CONFIG_NUMA_BALANCING
+#define LAST_NID_SHIFT NODES_SHIFT
+#else
+#define LAST_NID_SHIFT 0
+#endif
+
+#if SECTIONS_WIDTH+ZONES_WIDTH+NODES_SHIFT+LAST_NID_SHIFT <= BITS_PER_LONG - NR_PAGEFLAGS
+#define LAST_NID_WIDTH LAST_NID_SHIFT
+#else
+#define LAST_NID_WIDTH 0
+#endif
+
 /*
  * We are going to use the flags for the page to node mapping if its in
  * there.  This includes the case where there is no node, so it is implicit.
@@ -68,4 +81,8 @@
 #define NODE_NOT_IN_PAGE_FLAGS
 #endif
 
+#if defined(CONFIG_NUMA_BALANCING) && LAST_NID_WIDTH == 0
+#define LAST_NID_NOT_IN_PAGE_FLAGS
+#endif
+
 #endif /* _LINUX_PAGE_FLAGS_LAYOUT */
diff --git a/mm/memory.c b/mm/memory.c
index bb1369f..16e697c 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -69,6 +69,10 @@
 
 #include "internal.h"
 
+#ifdef LAST_NID_NOT_IN_PAGE_FLAGS
+#warning Unfortunate NUMA and NUMA Balancing config, growing page-frame for last_nid.
+#endif
+
 #ifndef CONFIG_NEED_MULTIPLE_NODES
 /* use the per-pgdat data instead for discontigmem - mbligh */
 unsigned long max_mapnr;
-- 
1.7.9.2

[PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[Mel Gorman]

From: Peter Zijlstra <a.p.zijlstra@chello.nl>

page->_last_nid fits into page->flags on 64-bit. The unlikely 32-bit NUMA
configuration with NUMA Balancing will still need an extra page field.
As Peter notes "Completely dropping 32bit support for CONFIG_NUMA_BALANCING
would simplify things, but it would also remove the warning if we grow
enough 64bit only page-flags to push the last-cpu out."

[mgorman@suse.de: Minor modifications]
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/mm.h                |   33 ++++++++++++++++++++++++++++++++-
 include/linux/mm_types.h          |    2 +-
 include/linux/page-flags-layout.h |   33 +++++++++++++++++++++++++--------
 mm/memory.c                       |    4 ++++
 4 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 87420e6..e25d47f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -580,10 +580,11 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
  * sets it, so none of the operations on it need to be atomic.
  */
 
-/* Page flags: | [SECTION] | [NODE] | ZONE | ... | FLAGS | */
+/* Page flags: | [SECTION] | [NODE] | ZONE | [LAST_NID] | ... | FLAGS | */
 #define SECTIONS_PGOFF		((sizeof(unsigned long)*8) - SECTIONS_WIDTH)
 #define NODES_PGOFF		(SECTIONS_PGOFF - NODES_WIDTH)
 #define ZONES_PGOFF		(NODES_PGOFF - ZONES_WIDTH)
+#define LAST_NID_PGOFF		(ZONES_PGOFF - LAST_NID_WIDTH)
 
 /*
  * Define the bit shifts to access each section.  For non-existent
@@ -593,6 +594,7 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
 #define SECTIONS_PGSHIFT	(SECTIONS_PGOFF * (SECTIONS_WIDTH != 0))
 #define NODES_PGSHIFT		(NODES_PGOFF * (NODES_WIDTH != 0))
 #define ZONES_PGSHIFT		(ZONES_PGOFF * (ZONES_WIDTH != 0))
+#define LAST_NID_PGSHIFT	(LAST_NID_PGOFF * (LAST_NID_WIDTH != 0))
 
 /* NODE:ZONE or SECTION:ZONE is used to ID a zone for the buddy allocator */
 #ifdef NODE_NOT_IN_PAGE_FLAGS
@@ -614,6 +616,7 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
 #define ZONES_MASK		((1UL << ZONES_WIDTH) - 1)
 #define NODES_MASK		((1UL << NODES_WIDTH) - 1)
 #define SECTIONS_MASK		((1UL << SECTIONS_WIDTH) - 1)
+#define LAST_NID_MASK		((1UL << LAST_NID_WIDTH) - 1)
 #define ZONEID_MASK		((1UL << ZONEID_SHIFT) - 1)
 
 static inline enum zone_type page_zonenum(const struct page *page)
@@ -653,6 +656,7 @@ static inline int page_to_nid(const struct page *page)
 #endif
 
 #ifdef CONFIG_NUMA_BALANCING
+#ifdef LAST_NID_NOT_IN_PAGE_FLAGS
 static inline int page_xchg_last_nid(struct page *page, int nid)
 {
 	return xchg(&page->_last_nid, nid);
@@ -667,6 +671,33 @@ static inline void reset_page_last_nid(struct page *page)
 	page->_last_nid = -1;
 }
 #else
+static inline int page_last_nid(struct page *page)
+{
+	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
+}
+
+static inline int page_xchg_last_nid(struct page *page, int nid)
+{
+	unsigned long old_flags, flags;
+	int last_nid;
+
+	do {
+		old_flags = flags = page->flags;
+		last_nid = page_last_nid(page);
+
+		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
+		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
+	} while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags));
+
+	return last_nid;
+}
+
+static inline void reset_page_last_nid(struct page *page)
+{
+	page_xchg_last_nid(page, (1 << LAST_NID_SHIFT) - 1);
+}
+#endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
+#else
 static inline int page_xchg_last_nid(struct page *page, int nid)
 {
 	return page_to_nid(page);
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index d05d632..ace9a5f 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -174,7 +174,7 @@ struct page {
 	void *shadow;
 #endif
 
-#ifdef CONFIG_NUMA_BALANCING
+#ifdef LAST_NID_NOT_IN_PAGE_FLAGS
 	int _last_nid;
 #endif
 }
diff --git a/include/linux/page-flags-layout.h b/include/linux/page-flags-layout.h
index 316805d..93506a1 100644
--- a/include/linux/page-flags-layout.h
+++ b/include/linux/page-flags-layout.h
@@ -32,15 +32,16 @@
 /*
  * page->flags layout:
  *
- * There are three possibilities for how page->flags get
- * laid out.  The first is for the normal case, without
- * sparsemem.  The second is for sparsemem when there is
- * plenty of space for node and section.  The last is when
- * we have run out of space and have to fall back to an
- * alternate (slower) way of determining the node.
+ * There are five possibilities for how page->flags get laid out.  The first
+ * pair is for the normal case without sparsemem. The second pair is for
+ * sparsemem when there is plenty of space for node and section information.
+ * The last is when there is insufficient space in page->flags and a separate
+ * lookup is necessary.
  *
- * No sparsemem or sparsemem vmemmap: |       NODE     | ZONE | ... | FLAGS |
- * classic sparse with space for node:| SECTION | NODE | ZONE | ... | FLAGS |
+ * No sparsemem or sparsemem vmemmap: |       NODE     | ZONE |          ... | FLAGS |
+ *         " plus space for last_nid: |       NODE     | ZONE | LAST_NID ... | FLAGS |
+ * classic sparse with space for node:| SECTION | NODE | ZONE |          ... | FLAGS |
+ *         " plus space for last_nid: | SECTION | NODE | ZONE | LAST_NID ... | FLAGS |
  * classic sparse no space for node:  | SECTION |     ZONE    | ... | FLAGS |
  */
 #if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
@@ -60,6 +61,18 @@
 #define NODES_WIDTH		0
 #endif
 
+#ifdef CONFIG_NUMA_BALANCING
+#define LAST_NID_SHIFT NODES_SHIFT
+#else
+#define LAST_NID_SHIFT 0
+#endif
+
+#if SECTIONS_WIDTH+ZONES_WIDTH+NODES_SHIFT+LAST_NID_SHIFT <= BITS_PER_LONG - NR_PAGEFLAGS
+#define LAST_NID_WIDTH LAST_NID_SHIFT
+#else
+#define LAST_NID_WIDTH 0
+#endif
+
 /*
  * We are going to use the flags for the page to node mapping if its in
  * there.  This includes the case where there is no node, so it is implicit.
@@ -68,4 +81,8 @@
 #define NODE_NOT_IN_PAGE_FLAGS
 #endif
 
+#if defined(CONFIG_NUMA_BALANCING) && LAST_NID_WIDTH == 0
+#define LAST_NID_NOT_IN_PAGE_FLAGS
+#endif
+
 #endif /* _LINUX_PAGE_FLAGS_LAYOUT */
diff --git a/mm/memory.c b/mm/memory.c
index bb1369f..16e697c 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -69,6 +69,10 @@
 
 #include "internal.h"
 
+#ifdef LAST_NID_NOT_IN_PAGE_FLAGS
+#warning Unfortunate NUMA and NUMA Balancing config, growing page-frame for last_nid.
+#endif
+
 #ifndef CONFIG_NEED_MULTIPLE_NODES
 /* use the per-pgdat data instead for discontigmem - mbligh */
 unsigned long max_mapnr;
-- 
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[Andrew Morton]

On Tue, 22 Jan 2013 17:12:41 +0000
Mel Gorman <mgorman@suse.de> wrote:

> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> page->_last_nid fits into page->flags on 64-bit. The unlikely 32-bit NUMA
> configuration with NUMA Balancing will still need an extra page field.
> As Peter notes "Completely dropping 32bit support for CONFIG_NUMA_BALANCING
> would simplify things, but it would also remove the warning if we grow
> enough 64bit only page-flags to push the last-cpu out."

How much space remains in the 64-bit page->flags?

Was this the best possible use of the remaining space?

It's good that we can undo this later by flipping
LAST_NID_NOT_IN_PAGE_FLAGS.

> [mgorman@suse.de: Minor modifications]
> Signed-off-by: Mel Gorman <mgorman@suse.de>

Several of these patches are missing signoffs (Peter and Hugh).

>
> ...
>
> +static inline int page_last_nid(struct page *page)
> +{
> +	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
> +}
> +
> +static inline int page_xchg_last_nid(struct page *page, int nid)
> +{
> +	unsigned long old_flags, flags;
> +	int last_nid;
> +
> +	do {
> +		old_flags = flags = page->flags;
> +		last_nid = page_last_nid(page);
> +
> +		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
> +		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
> +	} while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags));
> +
> +	return last_nid;
> +}
> +
> +static inline void reset_page_last_nid(struct page *page)
> +{
> +	page_xchg_last_nid(page, (1 << LAST_NID_SHIFT) - 1);
> +}

page_xchg_last_nid() and reset_page_last_nid() are getting nuttily
large.  Please investigate uninlining them?

reset_page_last_nid() is poorly named.  page_reset_last_nid() would be
better, and consistent.

Re: [PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[Andrew Morton]

On Tue, 22 Jan 2013 17:12:41 +0000
Mel Gorman <mgorman@suse.de> wrote:

> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> page->_last_nid fits into page->flags on 64-bit. The unlikely 32-bit NUMA
> configuration with NUMA Balancing will still need an extra page field.
> As Peter notes "Completely dropping 32bit support for CONFIG_NUMA_BALANCING
> would simplify things, but it would also remove the warning if we grow
> enough 64bit only page-flags to push the last-cpu out."

How much space remains in the 64-bit page->flags?

Was this the best possible use of the remaining space?

It's good that we can undo this later by flipping
LAST_NID_NOT_IN_PAGE_FLAGS.

> [mgorman@suse.de: Minor modifications]
> Signed-off-by: Mel Gorman <mgorman@suse.de>

Several of these patches are missing signoffs (Peter and Hugh).

>
> ...
>
> +static inline int page_last_nid(struct page *page)
> +{
> +	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
> +}
> +
> +static inline int page_xchg_last_nid(struct page *page, int nid)
> +{
> +	unsigned long old_flags, flags;
> +	int last_nid;
> +
> +	do {
> +		old_flags = flags = page->flags;
> +		last_nid = page_last_nid(page);
> +
> +		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
> +		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
> +	} while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags));
> +
> +	return last_nid;
> +}
> +
> +static inline void reset_page_last_nid(struct page *page)
> +{
> +	page_xchg_last_nid(page, (1 << LAST_NID_SHIFT) - 1);
> +}

page_xchg_last_nid() and reset_page_last_nid() are getting nuttily
large.  Please investigate uninlining them?

reset_page_last_nid() is poorly named.  page_reset_last_nid() would be
better, and consistent.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[Mel Gorman]

On Tue, Jan 22, 2013 at 02:46:59PM -0800, Andrew Morton wrote:
> On Tue, 22 Jan 2013 17:12:41 +0000
> Mel Gorman <mgorman@suse.de> wrote:
> 
> > From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > 
> > page->_last_nid fits into page->flags on 64-bit. The unlikely 32-bit NUMA
> > configuration with NUMA Balancing will still need an extra page field.
> > As Peter notes "Completely dropping 32bit support for CONFIG_NUMA_BALANCING
> > would simplify things, but it would also remove the warning if we grow
> > enough 64bit only page-flags to push the last-cpu out."
> 
> How much space remains in the 64-bit page->flags?
> 

Good question.

There are 19 free bits in my configuration but it's related to
CONFIG_NODES_SHIFT which is 9 for me (512 nodes) and very heavily affected
by options such as CONFIG_SPARSEMEM_VMEMMAP. Memory hot-remove does not work
with CONFIG_SPARSEMEM_VMEMMAP and enterprise distribution configs may be
taking the performance hit to enable memory hot-remove. If I disable this
option to enable memory hot-remove then there are 0 free bits in page->flags.

Your milage will vary *considerably*.

In answering this question I remembered that mminit_loglevel is able to
answer these sort of questions but only if it's updated properly. I'll
post a follow-up patch.

> Was this the best possible use of the remaining space?
> 

Another good question and I do not have a good answer. There is a definite
cost to having a larger struct page on large memory systems. The benefit
to saving flags on 64-bit page->flags for potential future use is more
intangiable.

> It's good that we can undo this later by flipping
> LAST_NID_NOT_IN_PAGE_FLAGS.
> 

Yes and it generates a dirty warning if it's forced to use
LAST_NID_NOT_IN_PAGE_FLAGS.

> > [mgorman@suse.de: Minor modifications]
> > Signed-off-by: Mel Gorman <mgorman@suse.de>
> 
> Several of these patches are missing signoffs (Peter and Hugh).
> 

In the case of Peter's patches, they changed enough that I couldn't preserve
the signed-off-by. This happened for the NUMA balancing patches too. I
preserved the "From" and I'm hoping he'll respond to add his Signed-off-by
to these patches if he's ok with them.

In Hugh's case he did not add his signed-off-by because he was not sure
whether there was a gremlin hidden in there. If there is, I was not able
to find it. It's up to him whether he wants to put his signed-off-by on
it but I preserved the "From:".

> >
> > ...
> >
> > +static inline int page_last_nid(struct page *page)
> > +{
> > +	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
> > +}
> > +
> > +static inline int page_xchg_last_nid(struct page *page, int nid)
> > +{
> > +	unsigned long old_flags, flags;
> > +	int last_nid;
> > +
> > +	do {
> > +		old_flags = flags = page->flags;
> > +		last_nid = page_last_nid(page);
> > +
> > +		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
> > +		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
> > +	} while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags));
> > +
> > +	return last_nid;
> > +}
> > +
> > +static inline void reset_page_last_nid(struct page *page)
> > +{
> > +	page_xchg_last_nid(page, (1 << LAST_NID_SHIFT) - 1);
> > +}
> 
> page_xchg_last_nid() and reset_page_last_nid() are getting nuttily
> large.  Please investigate uninlining them?
> 

Will do.

> reset_page_last_nid() is poorly named.  page_reset_last_nid() would be
> better, and consistent.
> 

Will fix.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[Mel Gorman]

On Tue, Jan 22, 2013 at 02:46:59PM -0800, Andrew Morton wrote:
> On Tue, 22 Jan 2013 17:12:41 +0000
> Mel Gorman <mgorman@suse.de> wrote:
> 
> > From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > 
> > page->_last_nid fits into page->flags on 64-bit. The unlikely 32-bit NUMA
> > configuration with NUMA Balancing will still need an extra page field.
> > As Peter notes "Completely dropping 32bit support for CONFIG_NUMA_BALANCING
> > would simplify things, but it would also remove the warning if we grow
> > enough 64bit only page-flags to push the last-cpu out."
> 
> How much space remains in the 64-bit page->flags?
> 

Good question.

There are 19 free bits in my configuration but it's related to
CONFIG_NODES_SHIFT which is 9 for me (512 nodes) and very heavily affected
by options such as CONFIG_SPARSEMEM_VMEMMAP. Memory hot-remove does not work
with CONFIG_SPARSEMEM_VMEMMAP and enterprise distribution configs may be
taking the performance hit to enable memory hot-remove. If I disable this
option to enable memory hot-remove then there are 0 free bits in page->flags.

Your milage will vary *considerably*.

In answering this question I remembered that mminit_loglevel is able to
answer these sort of questions but only if it's updated properly. I'll
post a follow-up patch.

> Was this the best possible use of the remaining space?
> 

Another good question and I do not have a good answer. There is a definite
cost to having a larger struct page on large memory systems. The benefit
to saving flags on 64-bit page->flags for potential future use is more
intangiable.

> It's good that we can undo this later by flipping
> LAST_NID_NOT_IN_PAGE_FLAGS.
> 

Yes and it generates a dirty warning if it's forced to use
LAST_NID_NOT_IN_PAGE_FLAGS.

> > [mgorman@suse.de: Minor modifications]
> > Signed-off-by: Mel Gorman <mgorman@suse.de>
> 
> Several of these patches are missing signoffs (Peter and Hugh).
> 

In the case of Peter's patches, they changed enough that I couldn't preserve
the signed-off-by. This happened for the NUMA balancing patches too. I
preserved the "From" and I'm hoping he'll respond to add his Signed-off-by
to these patches if he's ok with them.

In Hugh's case he did not add his signed-off-by because he was not sure
whether there was a gremlin hidden in there. If there is, I was not able
to find it. It's up to him whether he wants to put his signed-off-by on
it but I preserved the "From:".

> >
> > ...
> >
> > +static inline int page_last_nid(struct page *page)
> > +{
> > +	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
> > +}
> > +
> > +static inline int page_xchg_last_nid(struct page *page, int nid)
> > +{
> > +	unsigned long old_flags, flags;
> > +	int last_nid;
> > +
> > +	do {
> > +		old_flags = flags = page->flags;
> > +		last_nid = page_last_nid(page);
> > +
> > +		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
> > +		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
> > +	} while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags));
> > +
> > +	return last_nid;
> > +}
> > +
> > +static inline void reset_page_last_nid(struct page *page)
> > +{
> > +	page_xchg_last_nid(page, (1 << LAST_NID_SHIFT) - 1);
> > +}
> 
> page_xchg_last_nid() and reset_page_last_nid() are getting nuttily
> large.  Please investigate uninlining them?
> 

Will do.

> reset_page_last_nid() is poorly named.  page_reset_last_nid() would be
> better, and consistent.
> 

Will fix.

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[KOSAKI Motohiro]

> Good question.
>
> There are 19 free bits in my configuration but it's related to
> CONFIG_NODES_SHIFT which is 9 for me (512 nodes) and very heavily affected
> by options such as CONFIG_SPARSEMEM_VMEMMAP. Memory hot-remove does not work
> with CONFIG_SPARSEMEM_VMEMMAP and enterprise distribution configs may be
> taking the performance hit to enable memory hot-remove. If I disable this
> option to enable memory hot-remove then there are 0 free bits in page->flags.

FWIW, when using current mmotm, memory hot memory remove does work w/
CONFIG_SPARSEMEM_VMEMMAP. Recently Fujitsu changed.

Re: [PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[KOSAKI Motohiro]

> Good question.
>
> There are 19 free bits in my configuration but it's related to
> CONFIG_NODES_SHIFT which is 9 for me (512 nodes) and very heavily affected
> by options such as CONFIG_SPARSEMEM_VMEMMAP. Memory hot-remove does not work
> with CONFIG_SPARSEMEM_VMEMMAP and enterprise distribution configs may be
> taking the performance hit to enable memory hot-remove. If I disable this
> option to enable memory hot-remove then there are 0 free bits in page->flags.

FWIW, when using current mmotm, memory hot memory remove does work w/
CONFIG_SPARSEMEM_VMEMMAP. Recently Fujitsu changed.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm: init: Report on last-nid information stored in page->flags

[Mel Gorman]

Answering the question "how much space remains in the page->flags" is
time-consuming. mminit_loglevel can help answer the question but it does
not take last_nid information into account. This patch corrects it and
while there it corrects the messages related to page flag usage, pgshifts
and node/zone id. When applied the relevant output looks something like
this but will depend on the kernel configuration.

[    0.000000] mminit::pageflags_layout_widths Section 0 Node 9 Zone 2 Lastnid 9 Flags 25
[    0.000000] mminit::pageflags_layout_shifts Section 19 Node 9 Zone 2 Lastnid 9
[    0.000000] mminit::pageflags_layout_pgshifts Section 0 Node 55 Zone 53 Lastnid 44
[    0.000000] mminit::pageflags_layout_nodezoneid Node/Zone ID: 64 -> 53
[    0.000000] mminit::pageflags_layout_usage location: 64 -> 44 layout 44 -> 25 unused 25 -> 0 page-flags

Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 mm/mm_init.c |   31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/mm/mm_init.c b/mm/mm_init.c
index 1ffd97a..c280a02 100644
--- a/mm/mm_init.c
+++ b/mm/mm_init.c
@@ -69,34 +69,41 @@ void __init mminit_verify_pageflags_layout(void)
 	unsigned long or_mask, add_mask;
 
 	shift = 8 * sizeof(unsigned long);
-	width = shift - SECTIONS_WIDTH - NODES_WIDTH - ZONES_WIDTH;
+	width = shift - SECTIONS_WIDTH - NODES_WIDTH - ZONES_WIDTH - LAST_NID_SHIFT;
 	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_widths",
-		"Section %d Node %d Zone %d Flags %d\n",
+		"Section %d Node %d Zone %d Lastnid %d Flags %d\n",
 		SECTIONS_WIDTH,
 		NODES_WIDTH,
 		ZONES_WIDTH,
+		LAST_NID_WIDTH,
 		NR_PAGEFLAGS);
 	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_shifts",
-		"Section %d Node %d Zone %d\n",
+		"Section %d Node %d Zone %d Lastnid %d\n",
 		SECTIONS_SHIFT,
 		NODES_SHIFT,
-		ZONES_SHIFT);
-	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_offsets",
-		"Section %lu Node %lu Zone %lu\n",
+		ZONES_SHIFT,
+		LAST_NID_SHIFT);
+	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_pgshifts",
+		"Section %lu Node %lu Zone %lu Lastnid %lu\n",
 		(unsigned long)SECTIONS_PGSHIFT,
 		(unsigned long)NODES_PGSHIFT,
-		(unsigned long)ZONES_PGSHIFT);
-	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_zoneid",
-		"Zone ID: %lu -> %lu\n",
-		(unsigned long)ZONEID_PGOFF,
-		(unsigned long)(ZONEID_PGOFF + ZONEID_SHIFT));
+		(unsigned long)ZONES_PGSHIFT,
+		(unsigned long)LAST_NID_PGSHIFT);
+	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_nodezoneid",
+		"Node/Zone ID: %lu -> %lu\n",
+		(unsigned long)(ZONEID_PGOFF + ZONEID_SHIFT),
+		(unsigned long)ZONEID_PGOFF);
 	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_usage",
-		"location: %d -> %d unused %d -> %d flags %d -> %d\n",
+		"location: %d -> %d layout %d -> %d unused %d -> %d page-flags\n",
 		shift, width, width, NR_PAGEFLAGS, NR_PAGEFLAGS, 0);
 #ifdef NODE_NOT_IN_PAGE_FLAGS
 	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_nodeflags",
 		"Node not in page flags");
 #endif
+#ifdef LAST_NID_NOT_IN_PAGE_FLAGS
+	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_nodeflags",
+		"Last nid not in page flags");
+#endif
 
 	if (SECTIONS_WIDTH) {
 		shift -= SECTIONS_WIDTH;

[PATCH] mm: init: Report on last-nid information stored in page->flags

[Mel Gorman]

Answering the question "how much space remains in the page->flags" is
time-consuming. mminit_loglevel can help answer the question but it does
not take last_nid information into account. This patch corrects it and
while there it corrects the messages related to page flag usage, pgshifts
and node/zone id. When applied the relevant output looks something like
this but will depend on the kernel configuration.

[    0.000000] mminit::pageflags_layout_widths Section 0 Node 9 Zone 2 Lastnid 9 Flags 25
[    0.000000] mminit::pageflags_layout_shifts Section 19 Node 9 Zone 2 Lastnid 9
[    0.000000] mminit::pageflags_layout_pgshifts Section 0 Node 55 Zone 53 Lastnid 44
[    0.000000] mminit::pageflags_layout_nodezoneid Node/Zone ID: 64 -> 53
[    0.000000] mminit::pageflags_layout_usage location: 64 -> 44 layout 44 -> 25 unused 25 -> 0 page-flags

Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 mm/mm_init.c |   31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/mm/mm_init.c b/mm/mm_init.c
index 1ffd97a..c280a02 100644
--- a/mm/mm_init.c
+++ b/mm/mm_init.c
@@ -69,34 +69,41 @@ void __init mminit_verify_pageflags_layout(void)
 	unsigned long or_mask, add_mask;
 
 	shift = 8 * sizeof(unsigned long);
-	width = shift - SECTIONS_WIDTH - NODES_WIDTH - ZONES_WIDTH;
+	width = shift - SECTIONS_WIDTH - NODES_WIDTH - ZONES_WIDTH - LAST_NID_SHIFT;
 	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_widths",
-		"Section %d Node %d Zone %d Flags %d\n",
+		"Section %d Node %d Zone %d Lastnid %d Flags %d\n",
 		SECTIONS_WIDTH,
 		NODES_WIDTH,
 		ZONES_WIDTH,
+		LAST_NID_WIDTH,
 		NR_PAGEFLAGS);
 	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_shifts",
-		"Section %d Node %d Zone %d\n",
+		"Section %d Node %d Zone %d Lastnid %d\n",
 		SECTIONS_SHIFT,
 		NODES_SHIFT,
-		ZONES_SHIFT);
-	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_offsets",
-		"Section %lu Node %lu Zone %lu\n",
+		ZONES_SHIFT,
+		LAST_NID_SHIFT);
+	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_pgshifts",
+		"Section %lu Node %lu Zone %lu Lastnid %lu\n",
 		(unsigned long)SECTIONS_PGSHIFT,
 		(unsigned long)NODES_PGSHIFT,
-		(unsigned long)ZONES_PGSHIFT);
-	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_zoneid",
-		"Zone ID: %lu -> %lu\n",
-		(unsigned long)ZONEID_PGOFF,
-		(unsigned long)(ZONEID_PGOFF + ZONEID_SHIFT));
+		(unsigned long)ZONES_PGSHIFT,
+		(unsigned long)LAST_NID_PGSHIFT);
+	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_nodezoneid",
+		"Node/Zone ID: %lu -> %lu\n",
+		(unsigned long)(ZONEID_PGOFF + ZONEID_SHIFT),
+		(unsigned long)ZONEID_PGOFF);
 	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_usage",
-		"location: %d -> %d unused %d -> %d flags %d -> %d\n",
+		"location: %d -> %d layout %d -> %d unused %d -> %d page-flags\n",
 		shift, width, width, NR_PAGEFLAGS, NR_PAGEFLAGS, 0);
 #ifdef NODE_NOT_IN_PAGE_FLAGS
 	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_nodeflags",
 		"Node not in page flags");
 #endif
+#ifdef LAST_NID_NOT_IN_PAGE_FLAGS
+	mminit_dprintk(MMINIT_TRACE, "pageflags_layout_nodeflags",
+		"Last nid not in page flags");
+#endif
 
 	if (SECTIONS_WIDTH) {
 		shift -= SECTIONS_WIDTH;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[Mel Gorman]

On Tue, Jan 22, 2013 at 02:46:59PM -0800, Andrew Morton wrote:
> 
> reset_page_last_nid() is poorly named.  page_reset_last_nid() would be
> better, and consistent.
> 

Look at this closer, are you sure you want? Why is page_reset_last_nid()
better or more consistent?

The getter functions for page-related fields start with page (page_count,
page_mapcount etc.) but the setters begin with set (set_page_section,
set_page_zone, set_page_links etc.). For mapcount, we also have
reset_page_mapcount() so to me reset_page_last_nid() is already
consistent.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[Mel Gorman]

On Tue, Jan 22, 2013 at 02:46:59PM -0800, Andrew Morton wrote:
> 
> reset_page_last_nid() is poorly named.  page_reset_last_nid() would be
> better, and consistent.
> 

Look at this closer, are you sure you want? Why is page_reset_last_nid()
better or more consistent?

The getter functions for page-related fields start with page (page_count,
page_mapcount etc.) but the setters begin with set (set_page_section,
set_page_zone, set_page_links etc.). For mapcount, we also have
reset_page_mapcount() so to me reset_page_last_nid() is already
consistent.

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[Andrew Morton]

On Wed, 23 Jan 2013 14:25:07 +0000
Mel Gorman <mgorman@suse.de> wrote:

> On Tue, Jan 22, 2013 at 02:46:59PM -0800, Andrew Morton wrote:
> > 
> > reset_page_last_nid() is poorly named.  page_reset_last_nid() would be
> > better, and consistent.
> > 
> 
> Look at this closer, are you sure you want? Why is page_reset_last_nid()
> better or more consistent?

I was looking at this group:

static inline int page_xchg_last_nid(struct page *page, int nid)
static inline int page_last_nid(struct page *page)
static inline void reset_page_last_nid(struct page *page)

IMO the best naming for these would be page_nid_xchg_last(),
page_nid_last() and page_nid_reset_last().

> The getter functions for page-related fields start with page (page_count,
> page_mapcount etc.) but the setters begin with set (set_page_section,
> set_page_zone, set_page_links etc.). For mapcount, we also have
> reset_page_mapcount() so to me reset_page_last_nid() is already
> consistent.

But those schemes make no sense.

I don't see any benefit in being consistent with existing
inconsistency.  It's better to use good naming for new things and to
fix up the old things where practical.

Re: [PATCH 5/6] mm: Fold page->_last_nid into page->flags where possible

[Andrew Morton]

On Wed, 23 Jan 2013 14:25:07 +0000
Mel Gorman <mgorman@suse.de> wrote:

> On Tue, Jan 22, 2013 at 02:46:59PM -0800, Andrew Morton wrote:
> > 
> > reset_page_last_nid() is poorly named.  page_reset_last_nid() would be
> > better, and consistent.
> > 
> 
> Look at this closer, are you sure you want? Why is page_reset_last_nid()
> better or more consistent?

I was looking at this group:

static inline int page_xchg_last_nid(struct page *page, int nid)
static inline int page_last_nid(struct page *page)
static inline void reset_page_last_nid(struct page *page)

IMO the best naming for these would be page_nid_xchg_last(),
page_nid_last() and page_nid_reset_last().

> The getter functions for page-related fields start with page (page_count,
> page_mapcount etc.) but the setters begin with set (set_page_section,
> set_page_zone, set_page_links etc.). For mapcount, we also have
> reset_page_mapcount() so to me reset_page_last_nid() is already
> consistent.

But those schemes make no sense.

I don't see any benefit in being consistent with existing
inconsistency.  It's better to use good naming for new things and to
fix up the old things where practical.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm: Rename page struct field helpers

[Mel Gorman]

The function names page_xchg_last_nid(), page_last_nid() and
reset_page_last_nid() were judged to be inconsistent so rename them
to a struct_field_op style pattern. As it looked jarring to have
reset_page_mapcount() and page_nid_reset_last() beside each other in
memmap_init_zone(), this patch also renames reset_page_mapcount() to
page_mapcount_reset(). There are others like init_page_count() but as it
is used throughout the arch code a rename would likely cause more conflicts
than it is worth.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 drivers/staging/ramster/zbud.c           |    2 +-
 drivers/staging/zsmalloc/zsmalloc-main.c |    2 +-
 include/linux/mm.h                       |   20 ++++++++++----------
 mm/huge_memory.c                         |    2 +-
 mm/mempolicy.c                           |    2 +-
 mm/migrate.c                             |    4 ++--
 mm/mmzone.c                              |    4 ++--
 mm/page_alloc.c                          |   10 +++++-----
 mm/slob.c                                |    2 +-
 mm/slub.c                                |    2 +-
 10 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/ramster/zbud.c b/drivers/staging/ramster/zbud.c
index a7c4361..cc2deff 100644
--- a/drivers/staging/ramster/zbud.c
+++ b/drivers/staging/ramster/zbud.c
@@ -401,7 +401,7 @@ static inline struct page *zbud_unuse_zbudpage(struct zbudpage *zbudpage,
 	else
 		zbud_pers_pageframes--;
 	zbudpage_spin_unlock(zbudpage);
-	reset_page_mapcount(page);
+	page_mapcount_reset(page);
 	init_page_count(page);
 	page->index = 0;
 	return page;
diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
index 09a9d35..c7785f2 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -475,7 +475,7 @@ static void reset_page(struct page *page)
 	set_page_private(page, 0);
 	page->mapping = NULL;
 	page->freelist = NULL;
-	reset_page_mapcount(page);
+	page_mapcount_reset(page);
 }
 
 static void free_zspage(struct page *first_page)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6e4468f..0aa0944 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -366,7 +366,7 @@ static inline struct page *compound_head(struct page *page)
  * both from it and to it can be tracked, using atomic_inc_and_test
  * and atomic_add_negative(-1).
  */
-static inline void reset_page_mapcount(struct page *page)
+static inline void page_mapcount_reset(struct page *page)
 {
 	atomic_set(&(page)->_mapcount, -1);
 }
@@ -657,28 +657,28 @@ static inline int page_to_nid(const struct page *page)
 
 #ifdef CONFIG_NUMA_BALANCING
 #ifdef LAST_NID_NOT_IN_PAGE_FLAGS
-static inline int page_xchg_last_nid(struct page *page, int nid)
+static inline int page_nid_xchg_last(struct page *page, int nid)
 {
 	return xchg(&page->_last_nid, nid);
 }
 
-static inline int page_last_nid(struct page *page)
+static inline int page_nid_last(struct page *page)
 {
 	return page->_last_nid;
 }
-static inline void reset_page_last_nid(struct page *page)
+static inline void page_nid_reset_last(struct page *page)
 {
 	page->_last_nid = -1;
 }
 #else
-static inline int page_last_nid(struct page *page)
+static inline int page_nid_last(struct page *page)
 {
 	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
 }
 
-extern int page_xchg_last_nid(struct page *page, int nid);
+extern int page_nid_xchg_last(struct page *page, int nid);
 
-static inline void reset_page_last_nid(struct page *page)
+static inline void page_nid_reset_last(struct page *page)
 {
 	int nid = (1 << LAST_NID_SHIFT) - 1;
 
@@ -687,17 +687,17 @@ static inline void reset_page_last_nid(struct page *page)
 }
 #endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
 #else
-static inline int page_xchg_last_nid(struct page *page, int nid)
+static inline int page_nid_xchg_last(struct page *page, int nid)
 {
 	return page_to_nid(page);
 }
 
-static inline int page_last_nid(struct page *page)
+static inline int page_nid_last(struct page *page)
 {
 	return page_to_nid(page);
 }
 
-static inline void reset_page_last_nid(struct page *page)
+static inline void page_nid_reset_last(struct page *page)
 {
 }
 #endif
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 648c102..c52311a 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1642,7 +1642,7 @@ static void __split_huge_page_refcount(struct page *page)
 		page_tail->mapping = page->mapping;
 
 		page_tail->index = page->index + i;
-		page_xchg_last_nid(page_tail, page_last_nid(page));
+		page_nid_xchg_last(page_tail, page_nid_last(page));
 
 		BUG_ON(!PageAnon(page_tail));
 		BUG_ON(!PageUptodate(page_tail));
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index e2df1c1..db6fc14 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2308,7 +2308,7 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long
 		 * it less likely we act on an unlikely task<->page
 		 * relation.
 		 */
-		last_nid = page_xchg_last_nid(page, polnid);
+		last_nid = page_nid_xchg_last(page, polnid);
 		if (last_nid != polnid)
 			goto out;
 	}
diff --git a/mm/migrate.c b/mm/migrate.c
index 8ef1cbf..88422a1 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1495,7 +1495,7 @@ static struct page *alloc_misplaced_dst_page(struct page *page,
 					  __GFP_NOWARN) &
 					 ~GFP_IOFS, 0);
 	if (newpage)
-		page_xchg_last_nid(newpage, page_last_nid(page));
+		page_nid_xchg_last(newpage, page_nid_last(page));
 
 	return newpage;
 }
@@ -1679,7 +1679,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 	if (!new_page)
 		goto out_fail;
 
-	page_xchg_last_nid(new_page, page_last_nid(page));
+	page_nid_xchg_last(new_page, page_nid_last(page));
 
 	isolated = numamigrate_isolate_page(pgdat, page);
 	if (!isolated) {
diff --git a/mm/mmzone.c b/mm/mmzone.c
index bce796e..2ac0afb 100644
--- a/mm/mmzone.c
+++ b/mm/mmzone.c
@@ -98,14 +98,14 @@ void lruvec_init(struct lruvec *lruvec)
 }
 
 #if defined(CONFIG_NUMA_BALANCING) && !defined(LAST_NID_NOT_IN_PAGE_FLAGS)
-int page_xchg_last_nid(struct page *page, int nid)
+int page_nid_xchg_last(struct page *page, int nid)
 {
 	unsigned long old_flags, flags;
 	int last_nid;
 
 	do {
 		old_flags = flags = page->flags;
-		last_nid = page_last_nid(page);
+		last_nid = page_nid_last(page);
 
 		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
 		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index df2022f..2d525c8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -287,7 +287,7 @@ static void bad_page(struct page *page)
 
 	/* Don't complain about poisoned pages */
 	if (PageHWPoison(page)) {
-		reset_page_mapcount(page); /* remove PageBuddy */
+		page_mapcount_reset(page); /* remove PageBuddy */
 		return;
 	}
 
@@ -319,7 +319,7 @@ static void bad_page(struct page *page)
 	dump_stack();
 out:
 	/* Leave bad fields for debug, except PageBuddy could make trouble */
-	reset_page_mapcount(page); /* remove PageBuddy */
+	page_mapcount_reset(page); /* remove PageBuddy */
 	add_taint(TAINT_BAD_PAGE);
 }
 
@@ -605,7 +605,7 @@ static inline int free_pages_check(struct page *page)
 		bad_page(page);
 		return 1;
 	}
-	reset_page_last_nid(page);
+	page_nid_reset_last(page);
 	if (page->flags & PAGE_FLAGS_CHECK_AT_PREP)
 		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
 	return 0;
@@ -3871,8 +3871,8 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
 		set_page_links(page, zone, nid, pfn);
 		mminit_verify_page_links(page, zone, nid, pfn);
 		init_page_count(page);
-		reset_page_mapcount(page);
-		reset_page_last_nid(page);
+		page_mapcount_reset(page);
+		page_nid_reset_last(page);
 		SetPageReserved(page);
 		/*
 		 * Mark the block movable so that blocks are reserved for
diff --git a/mm/slob.c b/mm/slob.c
index a99fdf7..eeed4a0 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -360,7 +360,7 @@ static void slob_free(void *block, int size)
 			clear_slob_page_free(sp);
 		spin_unlock_irqrestore(&slob_lock, flags);
 		__ClearPageSlab(sp);
-		reset_page_mapcount(sp);
+		page_mapcount_reset(sp);
 		slob_free_pages(b, 0);
 		return;
 	}
diff --git a/mm/slub.c b/mm/slub.c
index ba2ca53..ebcc44e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1408,7 +1408,7 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
 	__ClearPageSlab(page);
 
 	memcg_release_pages(s, order);
-	reset_page_mapcount(page);
+	page_mapcount_reset(page);
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += pages;
 	__free_memcg_kmem_pages(page, order);

[PATCH] mm: Rename page struct field helpers

[Mel Gorman]

The function names page_xchg_last_nid(), page_last_nid() and
reset_page_last_nid() were judged to be inconsistent so rename them
to a struct_field_op style pattern. As it looked jarring to have
reset_page_mapcount() and page_nid_reset_last() beside each other in
memmap_init_zone(), this patch also renames reset_page_mapcount() to
page_mapcount_reset(). There are others like init_page_count() but as it
is used throughout the arch code a rename would likely cause more conflicts
than it is worth.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 drivers/staging/ramster/zbud.c           |    2 +-
 drivers/staging/zsmalloc/zsmalloc-main.c |    2 +-
 include/linux/mm.h                       |   20 ++++++++++----------
 mm/huge_memory.c                         |    2 +-
 mm/mempolicy.c                           |    2 +-
 mm/migrate.c                             |    4 ++--
 mm/mmzone.c                              |    4 ++--
 mm/page_alloc.c                          |   10 +++++-----
 mm/slob.c                                |    2 +-
 mm/slub.c                                |    2 +-
 10 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/ramster/zbud.c b/drivers/staging/ramster/zbud.c
index a7c4361..cc2deff 100644
--- a/drivers/staging/ramster/zbud.c
+++ b/drivers/staging/ramster/zbud.c
@@ -401,7 +401,7 @@ static inline struct page *zbud_unuse_zbudpage(struct zbudpage *zbudpage,
 	else
 		zbud_pers_pageframes--;
 	zbudpage_spin_unlock(zbudpage);
-	reset_page_mapcount(page);
+	page_mapcount_reset(page);
 	init_page_count(page);
 	page->index = 0;
 	return page;
diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
index 09a9d35..c7785f2 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -475,7 +475,7 @@ static void reset_page(struct page *page)
 	set_page_private(page, 0);
 	page->mapping = NULL;
 	page->freelist = NULL;
-	reset_page_mapcount(page);
+	page_mapcount_reset(page);
 }
 
 static void free_zspage(struct page *first_page)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6e4468f..0aa0944 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -366,7 +366,7 @@ static inline struct page *compound_head(struct page *page)
  * both from it and to it can be tracked, using atomic_inc_and_test
  * and atomic_add_negative(-1).
  */
-static inline void reset_page_mapcount(struct page *page)
+static inline void page_mapcount_reset(struct page *page)
 {
 	atomic_set(&(page)->_mapcount, -1);
 }
@@ -657,28 +657,28 @@ static inline int page_to_nid(const struct page *page)
 
 #ifdef CONFIG_NUMA_BALANCING
 #ifdef LAST_NID_NOT_IN_PAGE_FLAGS
-static inline int page_xchg_last_nid(struct page *page, int nid)
+static inline int page_nid_xchg_last(struct page *page, int nid)
 {
 	return xchg(&page->_last_nid, nid);
 }
 
-static inline int page_last_nid(struct page *page)
+static inline int page_nid_last(struct page *page)
 {
 	return page->_last_nid;
 }
-static inline void reset_page_last_nid(struct page *page)
+static inline void page_nid_reset_last(struct page *page)
 {
 	page->_last_nid = -1;
 }
 #else
-static inline int page_last_nid(struct page *page)
+static inline int page_nid_last(struct page *page)
 {
 	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
 }
 
-extern int page_xchg_last_nid(struct page *page, int nid);
+extern int page_nid_xchg_last(struct page *page, int nid);
 
-static inline void reset_page_last_nid(struct page *page)
+static inline void page_nid_reset_last(struct page *page)
 {
 	int nid = (1 << LAST_NID_SHIFT) - 1;
 
@@ -687,17 +687,17 @@ static inline void reset_page_last_nid(struct page *page)
 }
 #endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
 #else
-static inline int page_xchg_last_nid(struct page *page, int nid)
+static inline int page_nid_xchg_last(struct page *page, int nid)
 {
 	return page_to_nid(page);
 }
 
-static inline int page_last_nid(struct page *page)
+static inline int page_nid_last(struct page *page)
 {
 	return page_to_nid(page);
 }
 
-static inline void reset_page_last_nid(struct page *page)
+static inline void page_nid_reset_last(struct page *page)
 {
 }
 #endif
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 648c102..c52311a 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1642,7 +1642,7 @@ static void __split_huge_page_refcount(struct page *page)
 		page_tail->mapping = page->mapping;
 
 		page_tail->index = page->index + i;
-		page_xchg_last_nid(page_tail, page_last_nid(page));
+		page_nid_xchg_last(page_tail, page_nid_last(page));
 
 		BUG_ON(!PageAnon(page_tail));
 		BUG_ON(!PageUptodate(page_tail));
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index e2df1c1..db6fc14 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2308,7 +2308,7 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long
 		 * it less likely we act on an unlikely task<->page
 		 * relation.
 		 */
-		last_nid = page_xchg_last_nid(page, polnid);
+		last_nid = page_nid_xchg_last(page, polnid);
 		if (last_nid != polnid)
 			goto out;
 	}
diff --git a/mm/migrate.c b/mm/migrate.c
index 8ef1cbf..88422a1 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1495,7 +1495,7 @@ static struct page *alloc_misplaced_dst_page(struct page *page,
 					  __GFP_NOWARN) &
 					 ~GFP_IOFS, 0);
 	if (newpage)
-		page_xchg_last_nid(newpage, page_last_nid(page));
+		page_nid_xchg_last(newpage, page_nid_last(page));
 
 	return newpage;
 }
@@ -1679,7 +1679,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 	if (!new_page)
 		goto out_fail;
 
-	page_xchg_last_nid(new_page, page_last_nid(page));
+	page_nid_xchg_last(new_page, page_nid_last(page));
 
 	isolated = numamigrate_isolate_page(pgdat, page);
 	if (!isolated) {
diff --git a/mm/mmzone.c b/mm/mmzone.c
index bce796e..2ac0afb 100644
--- a/mm/mmzone.c
+++ b/mm/mmzone.c
@@ -98,14 +98,14 @@ void lruvec_init(struct lruvec *lruvec)
 }
 
 #if defined(CONFIG_NUMA_BALANCING) && !defined(LAST_NID_NOT_IN_PAGE_FLAGS)
-int page_xchg_last_nid(struct page *page, int nid)
+int page_nid_xchg_last(struct page *page, int nid)
 {
 	unsigned long old_flags, flags;
 	int last_nid;
 
 	do {
 		old_flags = flags = page->flags;
-		last_nid = page_last_nid(page);
+		last_nid = page_nid_last(page);
 
 		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
 		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index df2022f..2d525c8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -287,7 +287,7 @@ static void bad_page(struct page *page)
 
 	/* Don't complain about poisoned pages */
 	if (PageHWPoison(page)) {
-		reset_page_mapcount(page); /* remove PageBuddy */
+		page_mapcount_reset(page); /* remove PageBuddy */
 		return;
 	}
 
@@ -319,7 +319,7 @@ static void bad_page(struct page *page)
 	dump_stack();
 out:
 	/* Leave bad fields for debug, except PageBuddy could make trouble */
-	reset_page_mapcount(page); /* remove PageBuddy */
+	page_mapcount_reset(page); /* remove PageBuddy */
 	add_taint(TAINT_BAD_PAGE);
 }
 
@@ -605,7 +605,7 @@ static inline int free_pages_check(struct page *page)
 		bad_page(page);
 		return 1;
 	}
-	reset_page_last_nid(page);
+	page_nid_reset_last(page);
 	if (page->flags & PAGE_FLAGS_CHECK_AT_PREP)
 		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
 	return 0;
@@ -3871,8 +3871,8 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
 		set_page_links(page, zone, nid, pfn);
 		mminit_verify_page_links(page, zone, nid, pfn);
 		init_page_count(page);
-		reset_page_mapcount(page);
-		reset_page_last_nid(page);
+		page_mapcount_reset(page);
+		page_nid_reset_last(page);
 		SetPageReserved(page);
 		/*
 		 * Mark the block movable so that blocks are reserved for
diff --git a/mm/slob.c b/mm/slob.c
index a99fdf7..eeed4a0 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -360,7 +360,7 @@ static void slob_free(void *block, int size)
 			clear_slob_page_free(sp);
 		spin_unlock_irqrestore(&slob_lock, flags);
 		__ClearPageSlab(sp);
-		reset_page_mapcount(sp);
+		page_mapcount_reset(sp);
 		slob_free_pages(b, 0);
 		return;
 	}
diff --git a/mm/slub.c b/mm/slub.c
index ba2ca53..ebcc44e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1408,7 +1408,7 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
 	__ClearPageSlab(page);
 
 	memcg_release_pages(s, order);
-	reset_page_mapcount(page);
+	page_mapcount_reset(page);
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += pages;
 	__free_memcg_kmem_pages(page, order);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Rename page struct field helpers

[Hugh Dickins]

On Thu, 24 Jan 2013, Mel Gorman wrote:

> The function names page_xchg_last_nid(), page_last_nid() and
> reset_page_last_nid() were judged to be inconsistent so rename them
> to a struct_field_op style pattern. As it looked jarring to have
> reset_page_mapcount() and page_nid_reset_last() beside each other in
> memmap_init_zone(), this patch also renames reset_page_mapcount() to
> page_mapcount_reset(). There are others like init_page_count() but as it
> is used throughout the arch code a rename would likely cause more conflicts
> than it is worth.
> 
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Mel Gorman <mgorman@suse.de>

Sorry for not piping up in that earlier thread, but I don't understand
Andrew's reasoning on this: it looks to me like unhelpful churn rather
than improvement (and I suspect your heart is not in it either, Mel).

It's true that sometimes we name things object_verb() and sometimes we
name things verb_object(), but we're always going to be inconsistent on
that, and this patch does not change the fact: page_mapcount_reset()
but set_page_private() (named by one akpm, I believe)?

Being English, I really prefer verb_object(); but there are often
subsystems or cfiles where object_verb() narrows the namespace more
nicely.

xchg_page_last_nid() instead of page_xchg_last_nid(), to match
reset_page_last_nid(): I think that would be a fine change.

page_nid_xchg_last() to exchange page->_last_nid?  You jest, sir!

Hugh

> ---
>  drivers/staging/ramster/zbud.c           |    2 +-
>  drivers/staging/zsmalloc/zsmalloc-main.c |    2 +-
>  include/linux/mm.h                       |   20 ++++++++++----------
>  mm/huge_memory.c                         |    2 +-
>  mm/mempolicy.c                           |    2 +-
>  mm/migrate.c                             |    4 ++--
>  mm/mmzone.c                              |    4 ++--
>  mm/page_alloc.c                          |   10 +++++-----
>  mm/slob.c                                |    2 +-
>  mm/slub.c                                |    2 +-
>  10 files changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/staging/ramster/zbud.c b/drivers/staging/ramster/zbud.c
> index a7c4361..cc2deff 100644
> --- a/drivers/staging/ramster/zbud.c
> +++ b/drivers/staging/ramster/zbud.c
> @@ -401,7 +401,7 @@ static inline struct page *zbud_unuse_zbudpage(struct zbudpage *zbudpage,
>  	else
>  		zbud_pers_pageframes--;
>  	zbudpage_spin_unlock(zbudpage);
> -	reset_page_mapcount(page);
> +	page_mapcount_reset(page);
>  	init_page_count(page);
>  	page->index = 0;
>  	return page;
> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
> index 09a9d35..c7785f2 100644
> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
> @@ -475,7 +475,7 @@ static void reset_page(struct page *page)
>  	set_page_private(page, 0);
>  	page->mapping = NULL;
>  	page->freelist = NULL;
> -	reset_page_mapcount(page);
> +	page_mapcount_reset(page);
>  }
>  
>  static void free_zspage(struct page *first_page)
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 6e4468f..0aa0944 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -366,7 +366,7 @@ static inline struct page *compound_head(struct page *page)
>   * both from it and to it can be tracked, using atomic_inc_and_test
>   * and atomic_add_negative(-1).
>   */
> -static inline void reset_page_mapcount(struct page *page)
> +static inline void page_mapcount_reset(struct page *page)
>  {
>  	atomic_set(&(page)->_mapcount, -1);
>  }
> @@ -657,28 +657,28 @@ static inline int page_to_nid(const struct page *page)
>  
>  #ifdef CONFIG_NUMA_BALANCING
>  #ifdef LAST_NID_NOT_IN_PAGE_FLAGS
> -static inline int page_xchg_last_nid(struct page *page, int nid)
> +static inline int page_nid_xchg_last(struct page *page, int nid)
>  {
>  	return xchg(&page->_last_nid, nid);
>  }
>  
> -static inline int page_last_nid(struct page *page)
> +static inline int page_nid_last(struct page *page)
>  {
>  	return page->_last_nid;
>  }
> -static inline void reset_page_last_nid(struct page *page)
> +static inline void page_nid_reset_last(struct page *page)
>  {
>  	page->_last_nid = -1;
>  }
>  #else
> -static inline int page_last_nid(struct page *page)
> +static inline int page_nid_last(struct page *page)
>  {
>  	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
>  }
>  
> -extern int page_xchg_last_nid(struct page *page, int nid);
> +extern int page_nid_xchg_last(struct page *page, int nid);
>  
> -static inline void reset_page_last_nid(struct page *page)
> +static inline void page_nid_reset_last(struct page *page)
>  {
>  	int nid = (1 << LAST_NID_SHIFT) - 1;
>  
> @@ -687,17 +687,17 @@ static inline void reset_page_last_nid(struct page *page)
>  }
>  #endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
>  #else
> -static inline int page_xchg_last_nid(struct page *page, int nid)
> +static inline int page_nid_xchg_last(struct page *page, int nid)
>  {
>  	return page_to_nid(page);
>  }
>  
> -static inline int page_last_nid(struct page *page)
> +static inline int page_nid_last(struct page *page)
>  {
>  	return page_to_nid(page);
>  }
>  
> -static inline void reset_page_last_nid(struct page *page)
> +static inline void page_nid_reset_last(struct page *page)
>  {
>  }
>  #endif
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 648c102..c52311a 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1642,7 +1642,7 @@ static void __split_huge_page_refcount(struct page *page)
>  		page_tail->mapping = page->mapping;
>  
>  		page_tail->index = page->index + i;
> -		page_xchg_last_nid(page_tail, page_last_nid(page));
> +		page_nid_xchg_last(page_tail, page_nid_last(page));
>  
>  		BUG_ON(!PageAnon(page_tail));
>  		BUG_ON(!PageUptodate(page_tail));
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index e2df1c1..db6fc14 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2308,7 +2308,7 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long
>  		 * it less likely we act on an unlikely task<->page
>  		 * relation.
>  		 */
> -		last_nid = page_xchg_last_nid(page, polnid);
> +		last_nid = page_nid_xchg_last(page, polnid);
>  		if (last_nid != polnid)
>  			goto out;
>  	}
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 8ef1cbf..88422a1 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1495,7 +1495,7 @@ static struct page *alloc_misplaced_dst_page(struct page *page,
>  					  __GFP_NOWARN) &
>  					 ~GFP_IOFS, 0);
>  	if (newpage)
> -		page_xchg_last_nid(newpage, page_last_nid(page));
> +		page_nid_xchg_last(newpage, page_nid_last(page));
>  
>  	return newpage;
>  }
> @@ -1679,7 +1679,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  	if (!new_page)
>  		goto out_fail;
>  
> -	page_xchg_last_nid(new_page, page_last_nid(page));
> +	page_nid_xchg_last(new_page, page_nid_last(page));
>  
>  	isolated = numamigrate_isolate_page(pgdat, page);
>  	if (!isolated) {
> diff --git a/mm/mmzone.c b/mm/mmzone.c
> index bce796e..2ac0afb 100644
> --- a/mm/mmzone.c
> +++ b/mm/mmzone.c
> @@ -98,14 +98,14 @@ void lruvec_init(struct lruvec *lruvec)
>  }
>  
>  #if defined(CONFIG_NUMA_BALANCING) && !defined(LAST_NID_NOT_IN_PAGE_FLAGS)
> -int page_xchg_last_nid(struct page *page, int nid)
> +int page_nid_xchg_last(struct page *page, int nid)
>  {
>  	unsigned long old_flags, flags;
>  	int last_nid;
>  
>  	do {
>  		old_flags = flags = page->flags;
> -		last_nid = page_last_nid(page);
> +		last_nid = page_nid_last(page);
>  
>  		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
>  		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index df2022f..2d525c8 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -287,7 +287,7 @@ static void bad_page(struct page *page)
>  
>  	/* Don't complain about poisoned pages */
>  	if (PageHWPoison(page)) {
> -		reset_page_mapcount(page); /* remove PageBuddy */
> +		page_mapcount_reset(page); /* remove PageBuddy */
>  		return;
>  	}
>  
> @@ -319,7 +319,7 @@ static void bad_page(struct page *page)
>  	dump_stack();
>  out:
>  	/* Leave bad fields for debug, except PageBuddy could make trouble */
> -	reset_page_mapcount(page); /* remove PageBuddy */
> +	page_mapcount_reset(page); /* remove PageBuddy */
>  	add_taint(TAINT_BAD_PAGE);
>  }
>  
> @@ -605,7 +605,7 @@ static inline int free_pages_check(struct page *page)
>  		bad_page(page);
>  		return 1;
>  	}
> -	reset_page_last_nid(page);
> +	page_nid_reset_last(page);
>  	if (page->flags & PAGE_FLAGS_CHECK_AT_PREP)
>  		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
>  	return 0;
> @@ -3871,8 +3871,8 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
>  		set_page_links(page, zone, nid, pfn);
>  		mminit_verify_page_links(page, zone, nid, pfn);
>  		init_page_count(page);
> -		reset_page_mapcount(page);
> -		reset_page_last_nid(page);
> +		page_mapcount_reset(page);
> +		page_nid_reset_last(page);
>  		SetPageReserved(page);
>  		/*
>  		 * Mark the block movable so that blocks are reserved for
> diff --git a/mm/slob.c b/mm/slob.c
> index a99fdf7..eeed4a0 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -360,7 +360,7 @@ static void slob_free(void *block, int size)
>  			clear_slob_page_free(sp);
>  		spin_unlock_irqrestore(&slob_lock, flags);
>  		__ClearPageSlab(sp);
> -		reset_page_mapcount(sp);
> +		page_mapcount_reset(sp);
>  		slob_free_pages(b, 0);
>  		return;
>  	}
> diff --git a/mm/slub.c b/mm/slub.c
> index ba2ca53..ebcc44e 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1408,7 +1408,7 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
>  	__ClearPageSlab(page);
>  
>  	memcg_release_pages(s, order);
> -	reset_page_mapcount(page);
> +	page_mapcount_reset(page);
>  	if (current->reclaim_state)
>  		current->reclaim_state->reclaimed_slab += pages;
>  	__free_memcg_kmem_pages(page, order);
> 

Re: [PATCH] mm: Rename page struct field helpers

[Hugh Dickins]

On Thu, 24 Jan 2013, Mel Gorman wrote:

> The function names page_xchg_last_nid(), page_last_nid() and
> reset_page_last_nid() were judged to be inconsistent so rename them
> to a struct_field_op style pattern. As it looked jarring to have
> reset_page_mapcount() and page_nid_reset_last() beside each other in
> memmap_init_zone(), this patch also renames reset_page_mapcount() to
> page_mapcount_reset(). There are others like init_page_count() but as it
> is used throughout the arch code a rename would likely cause more conflicts
> than it is worth.
> 
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Mel Gorman <mgorman@suse.de>

Sorry for not piping up in that earlier thread, but I don't understand
Andrew's reasoning on this: it looks to me like unhelpful churn rather
than improvement (and I suspect your heart is not in it either, Mel).

It's true that sometimes we name things object_verb() and sometimes we
name things verb_object(), but we're always going to be inconsistent on
that, and this patch does not change the fact: page_mapcount_reset()
but set_page_private() (named by one akpm, I believe)?

Being English, I really prefer verb_object(); but there are often
subsystems or cfiles where object_verb() narrows the namespace more
nicely.

xchg_page_last_nid() instead of page_xchg_last_nid(), to match
reset_page_last_nid(): I think that would be a fine change.

page_nid_xchg_last() to exchange page->_last_nid?  You jest, sir!

Hugh

> ---
>  drivers/staging/ramster/zbud.c           |    2 +-
>  drivers/staging/zsmalloc/zsmalloc-main.c |    2 +-
>  include/linux/mm.h                       |   20 ++++++++++----------
>  mm/huge_memory.c                         |    2 +-
>  mm/mempolicy.c                           |    2 +-
>  mm/migrate.c                             |    4 ++--
>  mm/mmzone.c                              |    4 ++--
>  mm/page_alloc.c                          |   10 +++++-----
>  mm/slob.c                                |    2 +-
>  mm/slub.c                                |    2 +-
>  10 files changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/staging/ramster/zbud.c b/drivers/staging/ramster/zbud.c
> index a7c4361..cc2deff 100644
> --- a/drivers/staging/ramster/zbud.c
> +++ b/drivers/staging/ramster/zbud.c
> @@ -401,7 +401,7 @@ static inline struct page *zbud_unuse_zbudpage(struct zbudpage *zbudpage,
>  	else
>  		zbud_pers_pageframes--;
>  	zbudpage_spin_unlock(zbudpage);
> -	reset_page_mapcount(page);
> +	page_mapcount_reset(page);
>  	init_page_count(page);
>  	page->index = 0;
>  	return page;
> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
> index 09a9d35..c7785f2 100644
> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
> @@ -475,7 +475,7 @@ static void reset_page(struct page *page)
>  	set_page_private(page, 0);
>  	page->mapping = NULL;
>  	page->freelist = NULL;
> -	reset_page_mapcount(page);
> +	page_mapcount_reset(page);
>  }
>  
>  static void free_zspage(struct page *first_page)
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 6e4468f..0aa0944 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -366,7 +366,7 @@ static inline struct page *compound_head(struct page *page)
>   * both from it and to it can be tracked, using atomic_inc_and_test
>   * and atomic_add_negative(-1).
>   */
> -static inline void reset_page_mapcount(struct page *page)
> +static inline void page_mapcount_reset(struct page *page)
>  {
>  	atomic_set(&(page)->_mapcount, -1);
>  }
> @@ -657,28 +657,28 @@ static inline int page_to_nid(const struct page *page)
>  
>  #ifdef CONFIG_NUMA_BALANCING
>  #ifdef LAST_NID_NOT_IN_PAGE_FLAGS
> -static inline int page_xchg_last_nid(struct page *page, int nid)
> +static inline int page_nid_xchg_last(struct page *page, int nid)
>  {
>  	return xchg(&page->_last_nid, nid);
>  }
>  
> -static inline int page_last_nid(struct page *page)
> +static inline int page_nid_last(struct page *page)
>  {
>  	return page->_last_nid;
>  }
> -static inline void reset_page_last_nid(struct page *page)
> +static inline void page_nid_reset_last(struct page *page)
>  {
>  	page->_last_nid = -1;
>  }
>  #else
> -static inline int page_last_nid(struct page *page)
> +static inline int page_nid_last(struct page *page)
>  {
>  	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
>  }
>  
> -extern int page_xchg_last_nid(struct page *page, int nid);
> +extern int page_nid_xchg_last(struct page *page, int nid);
>  
> -static inline void reset_page_last_nid(struct page *page)
> +static inline void page_nid_reset_last(struct page *page)
>  {
>  	int nid = (1 << LAST_NID_SHIFT) - 1;
>  
> @@ -687,17 +687,17 @@ static inline void reset_page_last_nid(struct page *page)
>  }
>  #endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
>  #else
> -static inline int page_xchg_last_nid(struct page *page, int nid)
> +static inline int page_nid_xchg_last(struct page *page, int nid)
>  {
>  	return page_to_nid(page);
>  }
>  
> -static inline int page_last_nid(struct page *page)
> +static inline int page_nid_last(struct page *page)
>  {
>  	return page_to_nid(page);
>  }
>  
> -static inline void reset_page_last_nid(struct page *page)
> +static inline void page_nid_reset_last(struct page *page)
>  {
>  }
>  #endif
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 648c102..c52311a 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1642,7 +1642,7 @@ static void __split_huge_page_refcount(struct page *page)
>  		page_tail->mapping = page->mapping;
>  
>  		page_tail->index = page->index + i;
> -		page_xchg_last_nid(page_tail, page_last_nid(page));
> +		page_nid_xchg_last(page_tail, page_nid_last(page));
>  
>  		BUG_ON(!PageAnon(page_tail));
>  		BUG_ON(!PageUptodate(page_tail));
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index e2df1c1..db6fc14 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2308,7 +2308,7 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long
>  		 * it less likely we act on an unlikely task<->page
>  		 * relation.
>  		 */
> -		last_nid = page_xchg_last_nid(page, polnid);
> +		last_nid = page_nid_xchg_last(page, polnid);
>  		if (last_nid != polnid)
>  			goto out;
>  	}
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 8ef1cbf..88422a1 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1495,7 +1495,7 @@ static struct page *alloc_misplaced_dst_page(struct page *page,
>  					  __GFP_NOWARN) &
>  					 ~GFP_IOFS, 0);
>  	if (newpage)
> -		page_xchg_last_nid(newpage, page_last_nid(page));
> +		page_nid_xchg_last(newpage, page_nid_last(page));
>  
>  	return newpage;
>  }
> @@ -1679,7 +1679,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  	if (!new_page)
>  		goto out_fail;
>  
> -	page_xchg_last_nid(new_page, page_last_nid(page));
> +	page_nid_xchg_last(new_page, page_nid_last(page));
>  
>  	isolated = numamigrate_isolate_page(pgdat, page);
>  	if (!isolated) {
> diff --git a/mm/mmzone.c b/mm/mmzone.c
> index bce796e..2ac0afb 100644
> --- a/mm/mmzone.c
> +++ b/mm/mmzone.c
> @@ -98,14 +98,14 @@ void lruvec_init(struct lruvec *lruvec)
>  }
>  
>  #if defined(CONFIG_NUMA_BALANCING) && !defined(LAST_NID_NOT_IN_PAGE_FLAGS)
> -int page_xchg_last_nid(struct page *page, int nid)
> +int page_nid_xchg_last(struct page *page, int nid)
>  {
>  	unsigned long old_flags, flags;
>  	int last_nid;
>  
>  	do {
>  		old_flags = flags = page->flags;
> -		last_nid = page_last_nid(page);
> +		last_nid = page_nid_last(page);
>  
>  		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
>  		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index df2022f..2d525c8 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -287,7 +287,7 @@ static void bad_page(struct page *page)
>  
>  	/* Don't complain about poisoned pages */
>  	if (PageHWPoison(page)) {
> -		reset_page_mapcount(page); /* remove PageBuddy */
> +		page_mapcount_reset(page); /* remove PageBuddy */
>  		return;
>  	}
>  
> @@ -319,7 +319,7 @@ static void bad_page(struct page *page)
>  	dump_stack();
>  out:
>  	/* Leave bad fields for debug, except PageBuddy could make trouble */
> -	reset_page_mapcount(page); /* remove PageBuddy */
> +	page_mapcount_reset(page); /* remove PageBuddy */
>  	add_taint(TAINT_BAD_PAGE);
>  }
>  
> @@ -605,7 +605,7 @@ static inline int free_pages_check(struct page *page)
>  		bad_page(page);
>  		return 1;
>  	}
> -	reset_page_last_nid(page);
> +	page_nid_reset_last(page);
>  	if (page->flags & PAGE_FLAGS_CHECK_AT_PREP)
>  		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
>  	return 0;
> @@ -3871,8 +3871,8 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
>  		set_page_links(page, zone, nid, pfn);
>  		mminit_verify_page_links(page, zone, nid, pfn);
>  		init_page_count(page);
> -		reset_page_mapcount(page);
> -		reset_page_last_nid(page);
> +		page_mapcount_reset(page);
> +		page_nid_reset_last(page);
>  		SetPageReserved(page);
>  		/*
>  		 * Mark the block movable so that blocks are reserved for
> diff --git a/mm/slob.c b/mm/slob.c
> index a99fdf7..eeed4a0 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -360,7 +360,7 @@ static void slob_free(void *block, int size)
>  			clear_slob_page_free(sp);
>  		spin_unlock_irqrestore(&slob_lock, flags);
>  		__ClearPageSlab(sp);
> -		reset_page_mapcount(sp);
> +		page_mapcount_reset(sp);
>  		slob_free_pages(b, 0);
>  		return;
>  	}
> diff --git a/mm/slub.c b/mm/slub.c
> index ba2ca53..ebcc44e 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1408,7 +1408,7 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
>  	__ClearPageSlab(page);
>  
>  	memcg_release_pages(s, order);
> -	reset_page_mapcount(page);
> +	page_mapcount_reset(page);
>  	if (current->reclaim_state)
>  		current->reclaim_state->reclaimed_slab += pages;
>  	__free_memcg_kmem_pages(page, order);
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Rename page struct field helpers

[Mel Gorman]

On Mon, Jan 28, 2013 at 08:39:35PM -0800, Hugh Dickins wrote:
> On Thu, 24 Jan 2013, Mel Gorman wrote:
> 
> > The function names page_xchg_last_nid(), page_last_nid() and
> > reset_page_last_nid() were judged to be inconsistent so rename them
> > to a struct_field_op style pattern. As it looked jarring to have
> > reset_page_mapcount() and page_nid_reset_last() beside each other in
> > memmap_init_zone(), this patch also renames reset_page_mapcount() to
> > page_mapcount_reset(). There are others like init_page_count() but as it
> > is used throughout the arch code a rename would likely cause more conflicts
> > than it is worth.
> > 
> > Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Mel Gorman <mgorman@suse.de>
> 
> Sorry for not piping up in that earlier thread, but I don't understand
> Andrew's reasoning on this: it looks to me like unhelpful churn rather
> than improvement (and I suspect your heart is not in it either, Mel).
> 

My heart was not in it because I already recognised the original names.
I've been nailed before about poor naming particularly when I was already
familiar with the existing names.

> It's true that sometimes we name things object_verb() and sometimes we
> name things verb_object(), but we're always going to be inconsistent on
> that, and this patch does not change the fact: page_mapcount_reset()
> but set_page_private() (named by one akpm, I believe)?
> 

I half toyed with the idea of renaming all of them but it was going to
generate a lot of churn that would inevitably cause irritating patch
conflicts with little benefit.

> Being English, I really prefer verb_object(); but there are often
> subsystems or cfiles where object_verb() narrows the namespace more
> nicely.
> 
> xchg_page_last_nid() instead of page_xchg_last_nid(), to match
> reset_page_last_nid(): I think that would be a fine change.
> 
> page_nid_xchg_last() to exchange page->_last_nid?  You jest, sir!
> 

page_nid_last also looked odd to me. page_nid_xchg_last() looked vaguely
similar to page_to_nid() and maybe that was the intent.  However, I also
found it a little misleading because the nids are completely different --
one nid is where the page resides and the other nid is related to what
CPU referenced the page during the last numa hinting fault.

Your suggestion is to always have a verb_struct_field pattern which
page_xchg_last_nid violates. That patch would look like the following.
Andrew?

---8<---
mm: Rename page_xchg_last_nid

Andrew found the functions names page_xchg_last_nid(), page_last_nid()
and reset_page_last_nid() to be inconsistent and were renamed
to page_nid_xchg_last(), page_nid_last() and page_nid_reset_last().
Hugh found this unhelpful and suggested a rename of page_xchg_last_nid to
keep with a verb_struct_field naming pattern.

This patch replaces mm-rename-page-struct-field-helpers.patch.

Suggested-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/mm.h |    6 +++---
 mm/huge_memory.c   |    2 +-
 mm/mempolicy.c     |    2 +-
 mm/migrate.c       |    4 ++--
 mm/mmzone.c        |    2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6e4468f..6356db0 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -657,7 +657,7 @@ static inline int page_to_nid(const struct page *page)
 
 #ifdef CONFIG_NUMA_BALANCING
 #ifdef LAST_NID_NOT_IN_PAGE_FLAGS
-static inline int page_xchg_last_nid(struct page *page, int nid)
+static inline int xchg_page_last_nid(struct page *page, int nid)
 {
 	return xchg(&page->_last_nid, nid);
 }
@@ -676,7 +676,7 @@ static inline int page_last_nid(struct page *page)
 	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
 }
 
-extern int page_xchg_last_nid(struct page *page, int nid);
+extern int xchg_page_last_nid(struct page *page, int nid);
 
 static inline void reset_page_last_nid(struct page *page)
 {
@@ -687,7 +687,7 @@ static inline void reset_page_last_nid(struct page *page)
 }
 #endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
 #else
-static inline int page_xchg_last_nid(struct page *page, int nid)
+static inline int xchg_page_last_nid(struct page *page, int nid)
 {
 	return page_to_nid(page);
 }
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 648c102..ed97040 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1642,7 +1642,7 @@ static void __split_huge_page_refcount(struct page *page)
 		page_tail->mapping = page->mapping;
 
 		page_tail->index = page->index + i;
-		page_xchg_last_nid(page_tail, page_last_nid(page));
+		xchg_page_last_nid(page_tail, page_last_nid(page));
 
 		BUG_ON(!PageAnon(page_tail));
 		BUG_ON(!PageUptodate(page_tail));
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index e2df1c1..61226db 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2308,7 +2308,7 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long
 		 * it less likely we act on an unlikely task<->page
 		 * relation.
 		 */
-		last_nid = page_xchg_last_nid(page, polnid);
+		last_nid = xchg_page_last_nid(page, polnid);
 		if (last_nid != polnid)
 			goto out;
 	}
diff --git a/mm/migrate.c b/mm/migrate.c
index 8ef1cbf..4d9b724 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1495,7 +1495,7 @@ static struct page *alloc_misplaced_dst_page(struct page *page,
 					  __GFP_NOWARN) &
 					 ~GFP_IOFS, 0);
 	if (newpage)
-		page_xchg_last_nid(newpage, page_last_nid(page));
+		xchg_page_last_nid(newpage, page_last_nid(page));
 
 	return newpage;
 }
@@ -1679,7 +1679,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 	if (!new_page)
 		goto out_fail;
 
-	page_xchg_last_nid(new_page, page_last_nid(page));
+	xchg_page_last_nid(new_page, page_last_nid(page));
 
 	isolated = numamigrate_isolate_page(pgdat, page);
 	if (!isolated) {
diff --git a/mm/mmzone.c b/mm/mmzone.c
index bce796e..de2a951 100644
--- a/mm/mmzone.c
+++ b/mm/mmzone.c
@@ -98,7 +98,7 @@ void lruvec_init(struct lruvec *lruvec)
 }
 
 #if defined(CONFIG_NUMA_BALANCING) && !defined(LAST_NID_NOT_IN_PAGE_FLAGS)
-int page_xchg_last_nid(struct page *page, int nid)
+int xchg_page_last_nid(struct page *page, int nid)
 {
 	unsigned long old_flags, flags;
 	int last_nid;

Re: [PATCH] mm: Rename page struct field helpers

[Mel Gorman]

On Mon, Jan 28, 2013 at 08:39:35PM -0800, Hugh Dickins wrote:
> On Thu, 24 Jan 2013, Mel Gorman wrote:
> 
> > The function names page_xchg_last_nid(), page_last_nid() and
> > reset_page_last_nid() were judged to be inconsistent so rename them
> > to a struct_field_op style pattern. As it looked jarring to have
> > reset_page_mapcount() and page_nid_reset_last() beside each other in
> > memmap_init_zone(), this patch also renames reset_page_mapcount() to
> > page_mapcount_reset(). There are others like init_page_count() but as it
> > is used throughout the arch code a rename would likely cause more conflicts
> > than it is worth.
> > 
> > Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Mel Gorman <mgorman@suse.de>
> 
> Sorry for not piping up in that earlier thread, but I don't understand
> Andrew's reasoning on this: it looks to me like unhelpful churn rather
> than improvement (and I suspect your heart is not in it either, Mel).
> 

My heart was not in it because I already recognised the original names.
I've been nailed before about poor naming particularly when I was already
familiar with the existing names.

> It's true that sometimes we name things object_verb() and sometimes we
> name things verb_object(), but we're always going to be inconsistent on
> that, and this patch does not change the fact: page_mapcount_reset()
> but set_page_private() (named by one akpm, I believe)?
> 

I half toyed with the idea of renaming all of them but it was going to
generate a lot of churn that would inevitably cause irritating patch
conflicts with little benefit.

> Being English, I really prefer verb_object(); but there are often
> subsystems or cfiles where object_verb() narrows the namespace more
> nicely.
> 
> xchg_page_last_nid() instead of page_xchg_last_nid(), to match
> reset_page_last_nid(): I think that would be a fine change.
> 
> page_nid_xchg_last() to exchange page->_last_nid?  You jest, sir!
> 

page_nid_last also looked odd to me. page_nid_xchg_last() looked vaguely
similar to page_to_nid() and maybe that was the intent.  However, I also
found it a little misleading because the nids are completely different --
one nid is where the page resides and the other nid is related to what
CPU referenced the page during the last numa hinting fault.

Your suggestion is to always have a verb_struct_field pattern which
page_xchg_last_nid violates. That patch would look like the following.
Andrew?

---8<---
mm: Rename page_xchg_last_nid

Andrew found the functions names page_xchg_last_nid(), page_last_nid()
and reset_page_last_nid() to be inconsistent and were renamed
to page_nid_xchg_last(), page_nid_last() and page_nid_reset_last().
Hugh found this unhelpful and suggested a rename of page_xchg_last_nid to
keep with a verb_struct_field naming pattern.

This patch replaces mm-rename-page-struct-field-helpers.patch.

Suggested-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/mm.h |    6 +++---
 mm/huge_memory.c   |    2 +-
 mm/mempolicy.c     |    2 +-
 mm/migrate.c       |    4 ++--
 mm/mmzone.c        |    2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6e4468f..6356db0 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -657,7 +657,7 @@ static inline int page_to_nid(const struct page *page)
 
 #ifdef CONFIG_NUMA_BALANCING
 #ifdef LAST_NID_NOT_IN_PAGE_FLAGS
-static inline int page_xchg_last_nid(struct page *page, int nid)
+static inline int xchg_page_last_nid(struct page *page, int nid)
 {
 	return xchg(&page->_last_nid, nid);
 }
@@ -676,7 +676,7 @@ static inline int page_last_nid(struct page *page)
 	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
 }
 
-extern int page_xchg_last_nid(struct page *page, int nid);
+extern int xchg_page_last_nid(struct page *page, int nid);
 
 static inline void reset_page_last_nid(struct page *page)
 {
@@ -687,7 +687,7 @@ static inline void reset_page_last_nid(struct page *page)
 }
 #endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
 #else
-static inline int page_xchg_last_nid(struct page *page, int nid)
+static inline int xchg_page_last_nid(struct page *page, int nid)
 {
 	return page_to_nid(page);
 }
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 648c102..ed97040 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1642,7 +1642,7 @@ static void __split_huge_page_refcount(struct page *page)
 		page_tail->mapping = page->mapping;
 
 		page_tail->index = page->index + i;
-		page_xchg_last_nid(page_tail, page_last_nid(page));
+		xchg_page_last_nid(page_tail, page_last_nid(page));
 
 		BUG_ON(!PageAnon(page_tail));
 		BUG_ON(!PageUptodate(page_tail));
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index e2df1c1..61226db 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2308,7 +2308,7 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long
 		 * it less likely we act on an unlikely task<->page
 		 * relation.
 		 */
-		last_nid = page_xchg_last_nid(page, polnid);
+		last_nid = xchg_page_last_nid(page, polnid);
 		if (last_nid != polnid)
 			goto out;
 	}
diff --git a/mm/migrate.c b/mm/migrate.c
index 8ef1cbf..4d9b724 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1495,7 +1495,7 @@ static struct page *alloc_misplaced_dst_page(struct page *page,
 					  __GFP_NOWARN) &
 					 ~GFP_IOFS, 0);
 	if (newpage)
-		page_xchg_last_nid(newpage, page_last_nid(page));
+		xchg_page_last_nid(newpage, page_last_nid(page));
 
 	return newpage;
 }
@@ -1679,7 +1679,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 	if (!new_page)
 		goto out_fail;
 
-	page_xchg_last_nid(new_page, page_last_nid(page));
+	xchg_page_last_nid(new_page, page_last_nid(page));
 
 	isolated = numamigrate_isolate_page(pgdat, page);
 	if (!isolated) {
diff --git a/mm/mmzone.c b/mm/mmzone.c
index bce796e..de2a951 100644
--- a/mm/mmzone.c
+++ b/mm/mmzone.c
@@ -98,7 +98,7 @@ void lruvec_init(struct lruvec *lruvec)
 }
 
 #if defined(CONFIG_NUMA_BALANCING) && !defined(LAST_NID_NOT_IN_PAGE_FLAGS)
-int page_xchg_last_nid(struct page *page, int nid)
+int xchg_page_last_nid(struct page *page, int nid)
 {
 	unsigned long old_flags, flags;
 	int last_nid;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Rename page struct field helpers

[Hugh Dickins]

On Wed, 30 Jan 2013, Mel Gorman wrote:
> mm: Rename page_xchg_last_nid
> 
> Andrew found the functions names page_xchg_last_nid(), page_last_nid()
> and reset_page_last_nid() to be inconsistent and were renamed
> to page_nid_xchg_last(), page_nid_last() and page_nid_reset_last().
> Hugh found this unhelpful and suggested a rename of page_xchg_last_nid to
> keep with a verb_struct_field naming pattern.
> 
> This patch replaces mm-rename-page-struct-field-helpers.patch.
> 
> Suggested-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Mel Gorman <mgorman@suse.de>

FWIW looks just right to me (I don't think I deserve a Suggested-by
and an Acked-by), thanks.  But we may get a Rejected-by from Andrew.

> ---
>  include/linux/mm.h |    6 +++---
>  mm/huge_memory.c   |    2 +-
>  mm/mempolicy.c     |    2 +-
>  mm/migrate.c       |    4 ++--
>  mm/mmzone.c        |    2 +-
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 6e4468f..6356db0 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -657,7 +657,7 @@ static inline int page_to_nid(const struct page *page)
>  
>  #ifdef CONFIG_NUMA_BALANCING
>  #ifdef LAST_NID_NOT_IN_PAGE_FLAGS
> -static inline int page_xchg_last_nid(struct page *page, int nid)
> +static inline int xchg_page_last_nid(struct page *page, int nid)
>  {
>  	return xchg(&page->_last_nid, nid);
>  }
> @@ -676,7 +676,7 @@ static inline int page_last_nid(struct page *page)
>  	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
>  }
>  
> -extern int page_xchg_last_nid(struct page *page, int nid);
> +extern int xchg_page_last_nid(struct page *page, int nid);
>  
>  static inline void reset_page_last_nid(struct page *page)
>  {
> @@ -687,7 +687,7 @@ static inline void reset_page_last_nid(struct page *page)
>  }
>  #endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
>  #else
> -static inline int page_xchg_last_nid(struct page *page, int nid)
> +static inline int xchg_page_last_nid(struct page *page, int nid)
>  {
>  	return page_to_nid(page);
>  }
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 648c102..ed97040 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1642,7 +1642,7 @@ static void __split_huge_page_refcount(struct page *page)
>  		page_tail->mapping = page->mapping;
>  
>  		page_tail->index = page->index + i;
> -		page_xchg_last_nid(page_tail, page_last_nid(page));
> +		xchg_page_last_nid(page_tail, page_last_nid(page));
>  
>  		BUG_ON(!PageAnon(page_tail));
>  		BUG_ON(!PageUptodate(page_tail));
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index e2df1c1..61226db 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2308,7 +2308,7 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long
>  		 * it less likely we act on an unlikely task<->page
>  		 * relation.
>  		 */
> -		last_nid = page_xchg_last_nid(page, polnid);
> +		last_nid = xchg_page_last_nid(page, polnid);
>  		if (last_nid != polnid)
>  			goto out;
>  	}
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 8ef1cbf..4d9b724 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1495,7 +1495,7 @@ static struct page *alloc_misplaced_dst_page(struct page *page,
>  					  __GFP_NOWARN) &
>  					 ~GFP_IOFS, 0);
>  	if (newpage)
> -		page_xchg_last_nid(newpage, page_last_nid(page));
> +		xchg_page_last_nid(newpage, page_last_nid(page));
>  
>  	return newpage;
>  }
> @@ -1679,7 +1679,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  	if (!new_page)
>  		goto out_fail;
>  
> -	page_xchg_last_nid(new_page, page_last_nid(page));
> +	xchg_page_last_nid(new_page, page_last_nid(page));
>  
>  	isolated = numamigrate_isolate_page(pgdat, page);
>  	if (!isolated) {
> diff --git a/mm/mmzone.c b/mm/mmzone.c
> index bce796e..de2a951 100644
> --- a/mm/mmzone.c
> +++ b/mm/mmzone.c
> @@ -98,7 +98,7 @@ void lruvec_init(struct lruvec *lruvec)
>  }
>  
>  #if defined(CONFIG_NUMA_BALANCING) && !defined(LAST_NID_NOT_IN_PAGE_FLAGS)
> -int page_xchg_last_nid(struct page *page, int nid)
> +int xchg_page_last_nid(struct page *page, int nid)
>  {
>  	unsigned long old_flags, flags;
>  	int last_nid;
> 

Re: [PATCH] mm: Rename page struct field helpers

[Hugh Dickins]

On Wed, 30 Jan 2013, Mel Gorman wrote:
> mm: Rename page_xchg_last_nid
> 
> Andrew found the functions names page_xchg_last_nid(), page_last_nid()
> and reset_page_last_nid() to be inconsistent and were renamed
> to page_nid_xchg_last(), page_nid_last() and page_nid_reset_last().
> Hugh found this unhelpful and suggested a rename of page_xchg_last_nid to
> keep with a verb_struct_field naming pattern.
> 
> This patch replaces mm-rename-page-struct-field-helpers.patch.
> 
> Suggested-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Mel Gorman <mgorman@suse.de>

FWIW looks just right to me (I don't think I deserve a Suggested-by
and an Acked-by), thanks.  But we may get a Rejected-by from Andrew.

> ---
>  include/linux/mm.h |    6 +++---
>  mm/huge_memory.c   |    2 +-
>  mm/mempolicy.c     |    2 +-
>  mm/migrate.c       |    4 ++--
>  mm/mmzone.c        |    2 +-
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 6e4468f..6356db0 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -657,7 +657,7 @@ static inline int page_to_nid(const struct page *page)
>  
>  #ifdef CONFIG_NUMA_BALANCING
>  #ifdef LAST_NID_NOT_IN_PAGE_FLAGS
> -static inline int page_xchg_last_nid(struct page *page, int nid)
> +static inline int xchg_page_last_nid(struct page *page, int nid)
>  {
>  	return xchg(&page->_last_nid, nid);
>  }
> @@ -676,7 +676,7 @@ static inline int page_last_nid(struct page *page)
>  	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
>  }
>  
> -extern int page_xchg_last_nid(struct page *page, int nid);
> +extern int xchg_page_last_nid(struct page *page, int nid);
>  
>  static inline void reset_page_last_nid(struct page *page)
>  {
> @@ -687,7 +687,7 @@ static inline void reset_page_last_nid(struct page *page)
>  }
>  #endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
>  #else
> -static inline int page_xchg_last_nid(struct page *page, int nid)
> +static inline int xchg_page_last_nid(struct page *page, int nid)
>  {
>  	return page_to_nid(page);
>  }
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 648c102..ed97040 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1642,7 +1642,7 @@ static void __split_huge_page_refcount(struct page *page)
>  		page_tail->mapping = page->mapping;
>  
>  		page_tail->index = page->index + i;
> -		page_xchg_last_nid(page_tail, page_last_nid(page));
> +		xchg_page_last_nid(page_tail, page_last_nid(page));
>  
>  		BUG_ON(!PageAnon(page_tail));
>  		BUG_ON(!PageUptodate(page_tail));
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index e2df1c1..61226db 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2308,7 +2308,7 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long
>  		 * it less likely we act on an unlikely task<->page
>  		 * relation.
>  		 */
> -		last_nid = page_xchg_last_nid(page, polnid);
> +		last_nid = xchg_page_last_nid(page, polnid);
>  		if (last_nid != polnid)
>  			goto out;
>  	}
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 8ef1cbf..4d9b724 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1495,7 +1495,7 @@ static struct page *alloc_misplaced_dst_page(struct page *page,
>  					  __GFP_NOWARN) &
>  					 ~GFP_IOFS, 0);
>  	if (newpage)
> -		page_xchg_last_nid(newpage, page_last_nid(page));
> +		xchg_page_last_nid(newpage, page_last_nid(page));
>  
>  	return newpage;
>  }
> @@ -1679,7 +1679,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  	if (!new_page)
>  		goto out_fail;
>  
> -	page_xchg_last_nid(new_page, page_last_nid(page));
> +	xchg_page_last_nid(new_page, page_last_nid(page));
>  
>  	isolated = numamigrate_isolate_page(pgdat, page);
>  	if (!isolated) {
> diff --git a/mm/mmzone.c b/mm/mmzone.c
> index bce796e..de2a951 100644
> --- a/mm/mmzone.c
> +++ b/mm/mmzone.c
> @@ -98,7 +98,7 @@ void lruvec_init(struct lruvec *lruvec)
>  }
>  
>  #if defined(CONFIG_NUMA_BALANCING) && !defined(LAST_NID_NOT_IN_PAGE_FLAGS)
> -int page_xchg_last_nid(struct page *page, int nid)
> +int xchg_page_last_nid(struct page *page, int nid)
>  {
>  	unsigned long old_flags, flags;
>  	int last_nid;
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm: uninline page_xchg_last_nid()

[Mel Gorman]

Andrew Morton pointed out that page_xchg_last_nid() and reset_page_last_nid()
were "getting nuttily large" and asked that it be investigated.

reset_page_last_nid() is on the page free path and it would be unfortunate
to make that path more expensive than it needs to be. Due to the internal
use of page_xchg_last_nid() it is already too expensive but fortunately,
it should also be impossible for the page->flags to be updated in parallel
when we call reset_page_last_nid(). Instead of unlining the function,
it uses a simplier implementation that assumes no parallel updates and
should now be sufficiently short for inlining.

page_xchg_last_nid() is called in paths that are already quite expensive
(splitting huge page, fault handling, migration) and it is reasonable to
uninline. There was not really a good place to place the function but
mm/mmzone.c was the closest fit IMO.

This patch saved 128 bytes of text in the vmlinux file for the kernel
configuration I used for testing automatic NUMA balancing.

Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/mm.h |   21 +++++----------------
 mm/mmzone.c        |   20 +++++++++++++++++++-
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index e25d47f..6e4468f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -676,25 +676,14 @@ static inline int page_last_nid(struct page *page)
 	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
 }
 
-static inline int page_xchg_last_nid(struct page *page, int nid)
-{
-	unsigned long old_flags, flags;
-	int last_nid;
-
-	do {
-		old_flags = flags = page->flags;
-		last_nid = page_last_nid(page);
-
-		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
-		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
-	} while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags));
-
-	return last_nid;
-}
+extern int page_xchg_last_nid(struct page *page, int nid);
 
 static inline void reset_page_last_nid(struct page *page)
 {
-	page_xchg_last_nid(page, (1 << LAST_NID_SHIFT) - 1);
+	int nid = (1 << LAST_NID_SHIFT) - 1;
+
+	page->flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
+	page->flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
 }
 #endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
 #else
diff --git a/mm/mmzone.c b/mm/mmzone.c
index 4596d81..bce796e 100644
--- a/mm/mmzone.c
+++ b/mm/mmzone.c
@@ -1,7 +1,7 @@
 /*
  * linux/mm/mmzone.c
  *
- * management codes for pgdats and zones.
+ * management codes for pgdats, zones and page flags
  */
 
 
@@ -96,3 +96,21 @@ void lruvec_init(struct lruvec *lruvec)
 	for_each_lru(lru)
 		INIT_LIST_HEAD(&lruvec->lists[lru]);
 }
+
+#if defined(CONFIG_NUMA_BALANCING) && !defined(LAST_NID_NOT_IN_PAGE_FLAGS)
+int page_xchg_last_nid(struct page *page, int nid)
+{
+	unsigned long old_flags, flags;
+	int last_nid;
+
+	do {
+		old_flags = flags = page->flags;
+		last_nid = page_last_nid(page);
+
+		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
+		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
+	} while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags));
+
+	return last_nid;
+}
+#endif

[PATCH] mm: uninline page_xchg_last_nid()

[Mel Gorman]

Andrew Morton pointed out that page_xchg_last_nid() and reset_page_last_nid()
were "getting nuttily large" and asked that it be investigated.

reset_page_last_nid() is on the page free path and it would be unfortunate
to make that path more expensive than it needs to be. Due to the internal
use of page_xchg_last_nid() it is already too expensive but fortunately,
it should also be impossible for the page->flags to be updated in parallel
when we call reset_page_last_nid(). Instead of unlining the function,
it uses a simplier implementation that assumes no parallel updates and
should now be sufficiently short for inlining.

page_xchg_last_nid() is called in paths that are already quite expensive
(splitting huge page, fault handling, migration) and it is reasonable to
uninline. There was not really a good place to place the function but
mm/mmzone.c was the closest fit IMO.

This patch saved 128 bytes of text in the vmlinux file for the kernel
configuration I used for testing automatic NUMA balancing.

Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 include/linux/mm.h |   21 +++++----------------
 mm/mmzone.c        |   20 +++++++++++++++++++-
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index e25d47f..6e4468f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -676,25 +676,14 @@ static inline int page_last_nid(struct page *page)
 	return (page->flags >> LAST_NID_PGSHIFT) & LAST_NID_MASK;
 }
 
-static inline int page_xchg_last_nid(struct page *page, int nid)
-{
-	unsigned long old_flags, flags;
-	int last_nid;
-
-	do {
-		old_flags = flags = page->flags;
-		last_nid = page_last_nid(page);
-
-		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
-		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
-	} while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags));
-
-	return last_nid;
-}
+extern int page_xchg_last_nid(struct page *page, int nid);
 
 static inline void reset_page_last_nid(struct page *page)
 {
-	page_xchg_last_nid(page, (1 << LAST_NID_SHIFT) - 1);
+	int nid = (1 << LAST_NID_SHIFT) - 1;
+
+	page->flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
+	page->flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
 }
 #endif /* LAST_NID_NOT_IN_PAGE_FLAGS */
 #else
diff --git a/mm/mmzone.c b/mm/mmzone.c
index 4596d81..bce796e 100644
--- a/mm/mmzone.c
+++ b/mm/mmzone.c
@@ -1,7 +1,7 @@
 /*
  * linux/mm/mmzone.c
  *
- * management codes for pgdats and zones.
+ * management codes for pgdats, zones and page flags
  */
 
 
@@ -96,3 +96,21 @@ void lruvec_init(struct lruvec *lruvec)
 	for_each_lru(lru)
 		INIT_LIST_HEAD(&lruvec->lists[lru]);
 }
+
+#if defined(CONFIG_NUMA_BALANCING) && !defined(LAST_NID_NOT_IN_PAGE_FLAGS)
+int page_xchg_last_nid(struct page *page, int nid)
+{
+	unsigned long old_flags, flags;
+	int last_nid;
+
+	do {
+		old_flags = flags = page->flags;
+		last_nid = page_last_nid(page);
+
+		flags &= ~(LAST_NID_MASK << LAST_NID_PGSHIFT);
+		flags |= (nid & LAST_NID_MASK) << LAST_NID_PGSHIFT;
+	} while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags));
+
+	return last_nid;
+}
+#endif

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 6/6] mm: numa: Cleanup flow of transhuge page migration

[Mel Gorman]

From: Hugh Dickins <hughd@google.com>

When correcting commit 04fa5d6a (mm: migrate: check page_count of
THP before migrating) Hugh Dickins noted that the control flow for
transhuge migration was difficult to follow. Unconditionally calling
put_page() in numamigrate_isolate_page() made the failure paths of both
migrate_misplaced_transhuge_page() and migrate_misplaced_page() more complex
that they should be. Further, he was extremely wary that an unlock_page()
should ever happen after a put_page() even if the put_page() should never
be the final put_page.

Hugh implemented the following cleanup to simplify the path by
calling putback_lru_page() inside numamigrate_isolate_page()
if it failed to isolate and always calling unlock_page() within
migrate_misplaced_transhuge_page(). There is no functional change after
this patch is applied but the code is easier to follow and unlock_page()
always happens before put_page().

[mgorman@suse.de: changelog only]
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 mm/huge_memory.c |   28 ++++++----------
 mm/migrate.c     |   95 ++++++++++++++++++++++++------------------------------
 2 files changed, 52 insertions(+), 71 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 6001ee6..648c102 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1298,7 +1298,6 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	int target_nid;
 	int current_nid = -1;
 	bool migrated;
-	bool page_locked = false;
 
 	spin_lock(&mm->page_table_lock);
 	if (unlikely(!pmd_same(pmd, *pmdp)))
@@ -1320,7 +1319,6 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	/* Acquire the page lock to serialise THP migrations */
 	spin_unlock(&mm->page_table_lock);
 	lock_page(page);
-	page_locked = true;
 
 	/* Confirm the PTE did not while locked */
 	spin_lock(&mm->page_table_lock);
@@ -1333,34 +1331,26 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
 
 	/* Migrate the THP to the requested node */
 	migrated = migrate_misplaced_transhuge_page(mm, vma,
-				pmdp, pmd, addr,
-				page, target_nid);
-	if (migrated)
-		current_nid = target_nid;
-	else {
-		spin_lock(&mm->page_table_lock);
-		if (unlikely(!pmd_same(pmd, *pmdp))) {
-			unlock_page(page);
-			goto out_unlock;
-		}
-		goto clear_pmdnuma;
-	}
+				pmdp, pmd, addr, page, target_nid);
+	if (!migrated)
+		goto check_same;
 
-	task_numa_fault(current_nid, HPAGE_PMD_NR, migrated);
+	task_numa_fault(target_nid, HPAGE_PMD_NR, true);
 	return 0;
 
+check_same:
+	spin_lock(&mm->page_table_lock);
+	if (unlikely(!pmd_same(pmd, *pmdp)))
+		goto out_unlock;
 clear_pmdnuma:
 	pmd = pmd_mknonnuma(pmd);
 	set_pmd_at(mm, haddr, pmdp, pmd);
 	VM_BUG_ON(pmd_numa(*pmdp));
 	update_mmu_cache_pmd(vma, addr, pmdp);
-	if (page_locked)
-		unlock_page(page);
-
 out_unlock:
 	spin_unlock(&mm->page_table_lock);
 	if (current_nid != -1)
-		task_numa_fault(current_nid, HPAGE_PMD_NR, migrated);
+		task_numa_fault(current_nid, HPAGE_PMD_NR, false);
 	return 0;
 }
 
diff --git a/mm/migrate.c b/mm/migrate.c
index 73e432d..8ef1cbf 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1555,41 +1555,40 @@ bool numamigrate_update_ratelimit(pg_data_t *pgdat, unsigned long nr_pages)
 
 int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 {
-	int ret = 0;
+	int page_lru;
 
 	VM_BUG_ON(compound_order(page) && !PageTransHuge(page));
 
 	/* Avoid migrating to a node that is nearly full */
-	if (migrate_balanced_pgdat(pgdat, 1UL << compound_order(page))) {
-		int page_lru;
+	if (!migrate_balanced_pgdat(pgdat, 1UL << compound_order(page)))
+		return 0;
 
-		if (isolate_lru_page(page)) {
-			put_page(page);
-			return 0;
-		}
+	if (isolate_lru_page(page))
+		return 0;
 
-		/* Page is isolated */
-		ret = 1;
-		page_lru = page_is_file_cache(page);
-		if (!PageTransHuge(page))
-			inc_zone_page_state(page, NR_ISOLATED_ANON + page_lru);
-		else
-			mod_zone_page_state(page_zone(page),
-					NR_ISOLATED_ANON + page_lru,
-					HPAGE_PMD_NR);
+	/*
+	 * migrate_misplaced_transhuge_page() skips page migration's usual
+	 * check on page_count(), so we must do it here, now that the page
+	 * has been isolated: a GUP pin, or any other pin, prevents migration.
+	 * The expected page count is 3: 1 for page's mapcount and 1 for the
+	 * caller's pin and 1 for the reference taken by isolate_lru_page().
+	 */
+	if (PageTransHuge(page) && page_count(page) != 3) {
+		putback_lru_page(page);
+		return 0;
 	}
 
+	page_lru = page_is_file_cache(page);
+	mod_zone_page_state(page_zone(page), NR_ISOLATED_ANON + page_lru,
+				hpage_nr_pages(page));
+
 	/*
-	 * Page is either isolated or there is not enough space on the target
-	 * node. If isolated, then it has taken a reference count and the
-	 * callers reference can be safely dropped without the page
-	 * disappearing underneath us during migration. Otherwise the page is
-	 * not to be migrated but the callers reference should still be
-	 * dropped so it does not leak.
+	 * Isolating the page has taken another reference, so the
+	 * caller's reference can be safely dropped without the page
+	 * disappearing underneath us during migration.
 	 */
 	put_page(page);
-
-	return ret;
+	return 1;
 }
 
 /*
@@ -1600,7 +1599,7 @@ int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 int migrate_misplaced_page(struct page *page, int node)
 {
 	pg_data_t *pgdat = NODE_DATA(node);
-	int isolated = 0;
+	int isolated;
 	int nr_remaining;
 	LIST_HEAD(migratepages);
 
@@ -1608,20 +1607,16 @@ int migrate_misplaced_page(struct page *page, int node)
 	 * Don't migrate pages that are mapped in multiple processes.
 	 * TODO: Handle false sharing detection instead of this hammer
 	 */
-	if (page_mapcount(page) != 1) {
-		put_page(page);
+	if (page_mapcount(page) != 1)
 		goto out;
-	}
 
 	/*
 	 * Rate-limit the amount of data that is being migrated to a node.
 	 * Optimal placement is no good if the memory bus is saturated and
 	 * all the time is being spent migrating!
 	 */
-	if (numamigrate_update_ratelimit(pgdat, 1)) {
-		put_page(page);
+	if (numamigrate_update_ratelimit(pgdat, 1))
 		goto out;
-	}
 
 	isolated = numamigrate_isolate_page(pgdat, page);
 	if (!isolated)
@@ -1638,12 +1633,19 @@ int migrate_misplaced_page(struct page *page, int node)
 	} else
 		count_vm_numa_event(NUMA_PAGE_MIGRATE);
 	BUG_ON(!list_empty(&migratepages));
-out:
 	return isolated;
+
+out:
+	put_page(page);
+	return 0;
 }
 #endif /* CONFIG_NUMA_BALANCING */
 
 #if defined(CONFIG_NUMA_BALANCING) && defined(CONFIG_TRANSPARENT_HUGEPAGE)
+/*
+ * Migrates a THP to a given target node. page must be locked and is unlocked
+ * before returning.
+ */
 int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 				struct vm_area_struct *vma,
 				pmd_t *pmd, pmd_t entry,
@@ -1674,29 +1676,15 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 
 	new_page = alloc_pages_node(node,
 		(GFP_TRANSHUGE | GFP_THISNODE) & ~__GFP_WAIT, HPAGE_PMD_ORDER);
-	if (!new_page) {
-		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
-		goto out_dropref;
-	}
+	if (!new_page)
+		goto out_fail;
+
 	page_xchg_last_nid(new_page, page_last_nid(page));
 
 	isolated = numamigrate_isolate_page(pgdat, page);
-
-	/*
-	 * Failing to isolate or a GUP pin prevents migration. The expected
-	 * page count is 2. 1 for anonymous pages without a mapping and 1
-	 * for the callers pin. If the page was isolated, the page will
-	 * need to be put back on the LRU.
-	 */
-	if (!isolated || page_count(page) != 2) {
-		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
+	if (!isolated) {
 		put_page(new_page);
-		if (isolated) {
-			putback_lru_page(page);
-			isolated = 0;
-			goto out;
-		}
-		goto out_keep_locked;
+		goto out_fail;
 	}
 
 	/* Prepare a page as a migration target */
@@ -1728,6 +1716,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 		putback_lru_page(page);
 
 		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
+		isolated = 0;
 		goto out;
 	}
 
@@ -1772,9 +1761,11 @@ out:
 			-HPAGE_PMD_NR);
 	return isolated;
 
+out_fail:
+	count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
 out_dropref:
+	unlock_page(page);
 	put_page(page);
-out_keep_locked:
 	return 0;
 }
 #endif /* CONFIG_NUMA_BALANCING */
-- 
1.7.9.2

[PATCH 6/6] mm: numa: Cleanup flow of transhuge page migration

[Mel Gorman]

From: Hugh Dickins <hughd@google.com>

When correcting commit 04fa5d6a (mm: migrate: check page_count of
THP before migrating) Hugh Dickins noted that the control flow for
transhuge migration was difficult to follow. Unconditionally calling
put_page() in numamigrate_isolate_page() made the failure paths of both
migrate_misplaced_transhuge_page() and migrate_misplaced_page() more complex
that they should be. Further, he was extremely wary that an unlock_page()
should ever happen after a put_page() even if the put_page() should never
be the final put_page.

Hugh implemented the following cleanup to simplify the path by
calling putback_lru_page() inside numamigrate_isolate_page()
if it failed to isolate and always calling unlock_page() within
migrate_misplaced_transhuge_page(). There is no functional change after
this patch is applied but the code is easier to follow and unlock_page()
always happens before put_page().

[mgorman@suse.de: changelog only]
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 mm/huge_memory.c |   28 ++++++----------
 mm/migrate.c     |   95 ++++++++++++++++++++++++------------------------------
 2 files changed, 52 insertions(+), 71 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 6001ee6..648c102 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1298,7 +1298,6 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	int target_nid;
 	int current_nid = -1;
 	bool migrated;
-	bool page_locked = false;
 
 	spin_lock(&mm->page_table_lock);
 	if (unlikely(!pmd_same(pmd, *pmdp)))
@@ -1320,7 +1319,6 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	/* Acquire the page lock to serialise THP migrations */
 	spin_unlock(&mm->page_table_lock);
 	lock_page(page);
-	page_locked = true;
 
 	/* Confirm the PTE did not while locked */
 	spin_lock(&mm->page_table_lock);
@@ -1333,34 +1331,26 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
 
 	/* Migrate the THP to the requested node */
 	migrated = migrate_misplaced_transhuge_page(mm, vma,
-				pmdp, pmd, addr,
-				page, target_nid);
-	if (migrated)
-		current_nid = target_nid;
-	else {
-		spin_lock(&mm->page_table_lock);
-		if (unlikely(!pmd_same(pmd, *pmdp))) {
-			unlock_page(page);
-			goto out_unlock;
-		}
-		goto clear_pmdnuma;
-	}
+				pmdp, pmd, addr, page, target_nid);
+	if (!migrated)
+		goto check_same;
 
-	task_numa_fault(current_nid, HPAGE_PMD_NR, migrated);
+	task_numa_fault(target_nid, HPAGE_PMD_NR, true);
 	return 0;
 
+check_same:
+	spin_lock(&mm->page_table_lock);
+	if (unlikely(!pmd_same(pmd, *pmdp)))
+		goto out_unlock;
 clear_pmdnuma:
 	pmd = pmd_mknonnuma(pmd);
 	set_pmd_at(mm, haddr, pmdp, pmd);
 	VM_BUG_ON(pmd_numa(*pmdp));
 	update_mmu_cache_pmd(vma, addr, pmdp);
-	if (page_locked)
-		unlock_page(page);
-
 out_unlock:
 	spin_unlock(&mm->page_table_lock);
 	if (current_nid != -1)
-		task_numa_fault(current_nid, HPAGE_PMD_NR, migrated);
+		task_numa_fault(current_nid, HPAGE_PMD_NR, false);
 	return 0;
 }
 
diff --git a/mm/migrate.c b/mm/migrate.c
index 73e432d..8ef1cbf 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1555,41 +1555,40 @@ bool numamigrate_update_ratelimit(pg_data_t *pgdat, unsigned long nr_pages)
 
 int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 {
-	int ret = 0;
+	int page_lru;
 
 	VM_BUG_ON(compound_order(page) && !PageTransHuge(page));
 
 	/* Avoid migrating to a node that is nearly full */
-	if (migrate_balanced_pgdat(pgdat, 1UL << compound_order(page))) {
-		int page_lru;
+	if (!migrate_balanced_pgdat(pgdat, 1UL << compound_order(page)))
+		return 0;
 
-		if (isolate_lru_page(page)) {
-			put_page(page);
-			return 0;
-		}
+	if (isolate_lru_page(page))
+		return 0;
 
-		/* Page is isolated */
-		ret = 1;
-		page_lru = page_is_file_cache(page);
-		if (!PageTransHuge(page))
-			inc_zone_page_state(page, NR_ISOLATED_ANON + page_lru);
-		else
-			mod_zone_page_state(page_zone(page),
-					NR_ISOLATED_ANON + page_lru,
-					HPAGE_PMD_NR);
+	/*
+	 * migrate_misplaced_transhuge_page() skips page migration's usual
+	 * check on page_count(), so we must do it here, now that the page
+	 * has been isolated: a GUP pin, or any other pin, prevents migration.
+	 * The expected page count is 3: 1 for page's mapcount and 1 for the
+	 * caller's pin and 1 for the reference taken by isolate_lru_page().
+	 */
+	if (PageTransHuge(page) && page_count(page) != 3) {
+		putback_lru_page(page);
+		return 0;
 	}
 
+	page_lru = page_is_file_cache(page);
+	mod_zone_page_state(page_zone(page), NR_ISOLATED_ANON + page_lru,
+				hpage_nr_pages(page));
+
 	/*
-	 * Page is either isolated or there is not enough space on the target
-	 * node. If isolated, then it has taken a reference count and the
-	 * callers reference can be safely dropped without the page
-	 * disappearing underneath us during migration. Otherwise the page is
-	 * not to be migrated but the callers reference should still be
-	 * dropped so it does not leak.
+	 * Isolating the page has taken another reference, so the
+	 * caller's reference can be safely dropped without the page
+	 * disappearing underneath us during migration.
 	 */
 	put_page(page);
-
-	return ret;
+	return 1;
 }
 
 /*
@@ -1600,7 +1599,7 @@ int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 int migrate_misplaced_page(struct page *page, int node)
 {
 	pg_data_t *pgdat = NODE_DATA(node);
-	int isolated = 0;
+	int isolated;
 	int nr_remaining;
 	LIST_HEAD(migratepages);
 
@@ -1608,20 +1607,16 @@ int migrate_misplaced_page(struct page *page, int node)
 	 * Don't migrate pages that are mapped in multiple processes.
 	 * TODO: Handle false sharing detection instead of this hammer
 	 */
-	if (page_mapcount(page) != 1) {
-		put_page(page);
+	if (page_mapcount(page) != 1)
 		goto out;
-	}
 
 	/*
 	 * Rate-limit the amount of data that is being migrated to a node.
 	 * Optimal placement is no good if the memory bus is saturated and
 	 * all the time is being spent migrating!
 	 */
-	if (numamigrate_update_ratelimit(pgdat, 1)) {
-		put_page(page);
+	if (numamigrate_update_ratelimit(pgdat, 1))
 		goto out;
-	}
 
 	isolated = numamigrate_isolate_page(pgdat, page);
 	if (!isolated)
@@ -1638,12 +1633,19 @@ int migrate_misplaced_page(struct page *page, int node)
 	} else
 		count_vm_numa_event(NUMA_PAGE_MIGRATE);
 	BUG_ON(!list_empty(&migratepages));
-out:
 	return isolated;
+
+out:
+	put_page(page);
+	return 0;
 }
 #endif /* CONFIG_NUMA_BALANCING */
 
 #if defined(CONFIG_NUMA_BALANCING) && defined(CONFIG_TRANSPARENT_HUGEPAGE)
+/*
+ * Migrates a THP to a given target node. page must be locked and is unlocked
+ * before returning.
+ */
 int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 				struct vm_area_struct *vma,
 				pmd_t *pmd, pmd_t entry,
@@ -1674,29 +1676,15 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 
 	new_page = alloc_pages_node(node,
 		(GFP_TRANSHUGE | GFP_THISNODE) & ~__GFP_WAIT, HPAGE_PMD_ORDER);
-	if (!new_page) {
-		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
-		goto out_dropref;
-	}
+	if (!new_page)
+		goto out_fail;
+
 	page_xchg_last_nid(new_page, page_last_nid(page));
 
 	isolated = numamigrate_isolate_page(pgdat, page);
-
-	/*
-	 * Failing to isolate or a GUP pin prevents migration. The expected
-	 * page count is 2. 1 for anonymous pages without a mapping and 1
-	 * for the callers pin. If the page was isolated, the page will
-	 * need to be put back on the LRU.
-	 */
-	if (!isolated || page_count(page) != 2) {
-		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
+	if (!isolated) {
 		put_page(new_page);
-		if (isolated) {
-			putback_lru_page(page);
-			isolated = 0;
-			goto out;
-		}
-		goto out_keep_locked;
+		goto out_fail;
 	}
 
 	/* Prepare a page as a migration target */
@@ -1728,6 +1716,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 		putback_lru_page(page);
 
 		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
+		isolated = 0;
 		goto out;
 	}
 
@@ -1772,9 +1761,11 @@ out:
 			-HPAGE_PMD_NR);
 	return isolated;
 
+out_fail:
+	count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
 out_dropref:
+	unlock_page(page);
 	put_page(page);
-out_keep_locked:
 	return 0;
 }
 #endif /* CONFIG_NUMA_BALANCING */
-- 
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 6/6] mm: numa: Cleanup flow of transhuge page migration

[Hugh Dickins]

On Tue, 22 Jan 2013, Mel Gorman wrote:
> From: Hugh Dickins <hughd@google.com>
> 
> When correcting commit 04fa5d6a (mm: migrate: check page_count of
> THP before migrating) Hugh Dickins noted that the control flow for
> transhuge migration was difficult to follow. Unconditionally calling
> put_page() in numamigrate_isolate_page() made the failure paths of both
> migrate_misplaced_transhuge_page() and migrate_misplaced_page() more complex
> that they should be. Further, he was extremely wary that an unlock_page()
> should ever happen after a put_page() even if the put_page() should never
> be the final put_page.

Yes, I wasn't entirely convinced by your argument for why the unlock_page()
after put_page() had to be safe, given that it was coming on !pmd_same()
paths where we were backing out because the situation has changed beyond
our ken.  Not that I'd ever experienced any trouble from those (a final
free with page locked is sure to complain of bad page state).

It left me wondering whether some of those !pmd_same checks are simply
unnecessary: can others change the pmd_numa once we hold the page lock?

It would be nice ot eliminate some of the backtracking (most especially
the "Reverse changes made by migrate_page_copy()" area - though that's
a path which was managing its own unlock_page before putback_lru_page)
if it actually cannot play a part; but I've done nothing to investigate
further, and it may be obvious to you that I'm just blathering.

And certainly not something to get into in this patch.

> 
> Hugh implemented the following cleanup to simplify the path by
> calling putback_lru_page() inside numamigrate_isolate_page()
> if it failed to isolate and always calling unlock_page() within
> migrate_misplaced_transhuge_page(). There is no functional change after
> this patch is applied but the code is easier to follow and unlock_page()
> always happens before put_page().
> 
> [mgorman@suse.de: changelog only]

Thanks a lot for taking this on board, Mel, doing changelog and updiff.
I've now checked against what I was running with troublefree for the
week of 3.8-rc3, and am happy for Andrew now to insert my

Signed-off-by: Hugh Dickins <hughd@google.com>

> Signed-off-by: Mel Gorman <mgorman@suse.de>
> ---
>  mm/huge_memory.c |   28 ++++++----------
>  mm/migrate.c     |   95 ++++++++++++++++++++++++------------------------------
>  2 files changed, 52 insertions(+), 71 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 6001ee6..648c102 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1298,7 +1298,6 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	int target_nid;
>  	int current_nid = -1;
>  	bool migrated;
> -	bool page_locked = false;
>  
>  	spin_lock(&mm->page_table_lock);
>  	if (unlikely(!pmd_same(pmd, *pmdp)))
> @@ -1320,7 +1319,6 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	/* Acquire the page lock to serialise THP migrations */
>  	spin_unlock(&mm->page_table_lock);
>  	lock_page(page);
> -	page_locked = true;
>  
>  	/* Confirm the PTE did not while locked */
>  	spin_lock(&mm->page_table_lock);
> @@ -1333,34 +1331,26 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  
>  	/* Migrate the THP to the requested node */
>  	migrated = migrate_misplaced_transhuge_page(mm, vma,
> -				pmdp, pmd, addr,
> -				page, target_nid);
> -	if (migrated)
> -		current_nid = target_nid;
> -	else {
> -		spin_lock(&mm->page_table_lock);
> -		if (unlikely(!pmd_same(pmd, *pmdp))) {
> -			unlock_page(page);
> -			goto out_unlock;
> -		}
> -		goto clear_pmdnuma;
> -	}
> +				pmdp, pmd, addr, page, target_nid);
> +	if (!migrated)
> +		goto check_same;
>  
> -	task_numa_fault(current_nid, HPAGE_PMD_NR, migrated);
> +	task_numa_fault(target_nid, HPAGE_PMD_NR, true);
>  	return 0;
>  
> +check_same:
> +	spin_lock(&mm->page_table_lock);
> +	if (unlikely(!pmd_same(pmd, *pmdp)))
> +		goto out_unlock;
>  clear_pmdnuma:
>  	pmd = pmd_mknonnuma(pmd);
>  	set_pmd_at(mm, haddr, pmdp, pmd);
>  	VM_BUG_ON(pmd_numa(*pmdp));
>  	update_mmu_cache_pmd(vma, addr, pmdp);
> -	if (page_locked)
> -		unlock_page(page);
> -
>  out_unlock:
>  	spin_unlock(&mm->page_table_lock);
>  	if (current_nid != -1)
> -		task_numa_fault(current_nid, HPAGE_PMD_NR, migrated);
> +		task_numa_fault(current_nid, HPAGE_PMD_NR, false);
>  	return 0;
>  }
>  
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 73e432d..8ef1cbf 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1555,41 +1555,40 @@ bool numamigrate_update_ratelimit(pg_data_t *pgdat, unsigned long nr_pages)
>  
>  int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  {
> -	int ret = 0;
> +	int page_lru;
>  
>  	VM_BUG_ON(compound_order(page) && !PageTransHuge(page));
>  
>  	/* Avoid migrating to a node that is nearly full */
> -	if (migrate_balanced_pgdat(pgdat, 1UL << compound_order(page))) {
> -		int page_lru;
> +	if (!migrate_balanced_pgdat(pgdat, 1UL << compound_order(page)))
> +		return 0;
>  
> -		if (isolate_lru_page(page)) {
> -			put_page(page);
> -			return 0;
> -		}
> +	if (isolate_lru_page(page))
> +		return 0;
>  
> -		/* Page is isolated */
> -		ret = 1;
> -		page_lru = page_is_file_cache(page);
> -		if (!PageTransHuge(page))
> -			inc_zone_page_state(page, NR_ISOLATED_ANON + page_lru);
> -		else
> -			mod_zone_page_state(page_zone(page),
> -					NR_ISOLATED_ANON + page_lru,
> -					HPAGE_PMD_NR);
> +	/*
> +	 * migrate_misplaced_transhuge_page() skips page migration's usual
> +	 * check on page_count(), so we must do it here, now that the page
> +	 * has been isolated: a GUP pin, or any other pin, prevents migration.
> +	 * The expected page count is 3: 1 for page's mapcount and 1 for the
> +	 * caller's pin and 1 for the reference taken by isolate_lru_page().
> +	 */
> +	if (PageTransHuge(page) && page_count(page) != 3) {
> +		putback_lru_page(page);
> +		return 0;
>  	}
>  
> +	page_lru = page_is_file_cache(page);
> +	mod_zone_page_state(page_zone(page), NR_ISOLATED_ANON + page_lru,
> +				hpage_nr_pages(page));
> +
>  	/*
> -	 * Page is either isolated or there is not enough space on the target
> -	 * node. If isolated, then it has taken a reference count and the
> -	 * callers reference can be safely dropped without the page
> -	 * disappearing underneath us during migration. Otherwise the page is
> -	 * not to be migrated but the callers reference should still be
> -	 * dropped so it does not leak.
> +	 * Isolating the page has taken another reference, so the
> +	 * caller's reference can be safely dropped without the page
> +	 * disappearing underneath us during migration.
>  	 */
>  	put_page(page);
> -
> -	return ret;
> +	return 1;
>  }
>  
>  /*
> @@ -1600,7 +1599,7 @@ int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  int migrate_misplaced_page(struct page *page, int node)
>  {
>  	pg_data_t *pgdat = NODE_DATA(node);
> -	int isolated = 0;
> +	int isolated;
>  	int nr_remaining;
>  	LIST_HEAD(migratepages);
>  
> @@ -1608,20 +1607,16 @@ int migrate_misplaced_page(struct page *page, int node)
>  	 * Don't migrate pages that are mapped in multiple processes.
>  	 * TODO: Handle false sharing detection instead of this hammer
>  	 */
> -	if (page_mapcount(page) != 1) {
> -		put_page(page);
> +	if (page_mapcount(page) != 1)
>  		goto out;
> -	}
>  
>  	/*
>  	 * Rate-limit the amount of data that is being migrated to a node.
>  	 * Optimal placement is no good if the memory bus is saturated and
>  	 * all the time is being spent migrating!
>  	 */
> -	if (numamigrate_update_ratelimit(pgdat, 1)) {
> -		put_page(page);
> +	if (numamigrate_update_ratelimit(pgdat, 1))
>  		goto out;
> -	}
>  
>  	isolated = numamigrate_isolate_page(pgdat, page);
>  	if (!isolated)
> @@ -1638,12 +1633,19 @@ int migrate_misplaced_page(struct page *page, int node)
>  	} else
>  		count_vm_numa_event(NUMA_PAGE_MIGRATE);
>  	BUG_ON(!list_empty(&migratepages));
> -out:
>  	return isolated;
> +
> +out:
> +	put_page(page);
> +	return 0;
>  }
>  #endif /* CONFIG_NUMA_BALANCING */
>  
>  #if defined(CONFIG_NUMA_BALANCING) && defined(CONFIG_TRANSPARENT_HUGEPAGE)
> +/*
> + * Migrates a THP to a given target node. page must be locked and is unlocked
> + * before returning.
> + */
>  int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  				struct vm_area_struct *vma,
>  				pmd_t *pmd, pmd_t entry,
> @@ -1674,29 +1676,15 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  
>  	new_page = alloc_pages_node(node,
>  		(GFP_TRANSHUGE | GFP_THISNODE) & ~__GFP_WAIT, HPAGE_PMD_ORDER);
> -	if (!new_page) {
> -		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
> -		goto out_dropref;
> -	}
> +	if (!new_page)
> +		goto out_fail;
> +
>  	page_xchg_last_nid(new_page, page_last_nid(page));
>  
>  	isolated = numamigrate_isolate_page(pgdat, page);
> -
> -	/*
> -	 * Failing to isolate or a GUP pin prevents migration. The expected
> -	 * page count is 2. 1 for anonymous pages without a mapping and 1
> -	 * for the callers pin. If the page was isolated, the page will
> -	 * need to be put back on the LRU.
> -	 */
> -	if (!isolated || page_count(page) != 2) {
> -		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
> +	if (!isolated) {
>  		put_page(new_page);
> -		if (isolated) {
> -			putback_lru_page(page);
> -			isolated = 0;
> -			goto out;
> -		}
> -		goto out_keep_locked;
> +		goto out_fail;
>  	}
>  
>  	/* Prepare a page as a migration target */
> @@ -1728,6 +1716,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  		putback_lru_page(page);
>  
>  		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
> +		isolated = 0;
>  		goto out;
>  	}
>  
> @@ -1772,9 +1761,11 @@ out:
>  			-HPAGE_PMD_NR);
>  	return isolated;
>  
> +out_fail:
> +	count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
>  out_dropref:
> +	unlock_page(page);
>  	put_page(page);
> -out_keep_locked:
>  	return 0;
>  }
>  #endif /* CONFIG_NUMA_BALANCING */
> -- 
> 1.7.9.2

Re: [PATCH 6/6] mm: numa: Cleanup flow of transhuge page migration

[Hugh Dickins]

On Tue, 22 Jan 2013, Mel Gorman wrote:
> From: Hugh Dickins <hughd@google.com>
> 
> When correcting commit 04fa5d6a (mm: migrate: check page_count of
> THP before migrating) Hugh Dickins noted that the control flow for
> transhuge migration was difficult to follow. Unconditionally calling
> put_page() in numamigrate_isolate_page() made the failure paths of both
> migrate_misplaced_transhuge_page() and migrate_misplaced_page() more complex
> that they should be. Further, he was extremely wary that an unlock_page()
> should ever happen after a put_page() even if the put_page() should never
> be the final put_page.

Yes, I wasn't entirely convinced by your argument for why the unlock_page()
after put_page() had to be safe, given that it was coming on !pmd_same()
paths where we were backing out because the situation has changed beyond
our ken.  Not that I'd ever experienced any trouble from those (a final
free with page locked is sure to complain of bad page state).

It left me wondering whether some of those !pmd_same checks are simply
unnecessary: can others change the pmd_numa once we hold the page lock?

It would be nice ot eliminate some of the backtracking (most especially
the "Reverse changes made by migrate_page_copy()" area - though that's
a path which was managing its own unlock_page before putback_lru_page)
if it actually cannot play a part; but I've done nothing to investigate
further, and it may be obvious to you that I'm just blathering.

And certainly not something to get into in this patch.

> 
> Hugh implemented the following cleanup to simplify the path by
> calling putback_lru_page() inside numamigrate_isolate_page()
> if it failed to isolate and always calling unlock_page() within
> migrate_misplaced_transhuge_page(). There is no functional change after
> this patch is applied but the code is easier to follow and unlock_page()
> always happens before put_page().
> 
> [mgorman@suse.de: changelog only]

Thanks a lot for taking this on board, Mel, doing changelog and updiff.
I've now checked against what I was running with troublefree for the
week of 3.8-rc3, and am happy for Andrew now to insert my

Signed-off-by: Hugh Dickins <hughd@google.com>

> Signed-off-by: Mel Gorman <mgorman@suse.de>
> ---
>  mm/huge_memory.c |   28 ++++++----------
>  mm/migrate.c     |   95 ++++++++++++++++++++++++------------------------------
>  2 files changed, 52 insertions(+), 71 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 6001ee6..648c102 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1298,7 +1298,6 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	int target_nid;
>  	int current_nid = -1;
>  	bool migrated;
> -	bool page_locked = false;
>  
>  	spin_lock(&mm->page_table_lock);
>  	if (unlikely(!pmd_same(pmd, *pmdp)))
> @@ -1320,7 +1319,6 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	/* Acquire the page lock to serialise THP migrations */
>  	spin_unlock(&mm->page_table_lock);
>  	lock_page(page);
> -	page_locked = true;
>  
>  	/* Confirm the PTE did not while locked */
>  	spin_lock(&mm->page_table_lock);
> @@ -1333,34 +1331,26 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  
>  	/* Migrate the THP to the requested node */
>  	migrated = migrate_misplaced_transhuge_page(mm, vma,
> -				pmdp, pmd, addr,
> -				page, target_nid);
> -	if (migrated)
> -		current_nid = target_nid;
> -	else {
> -		spin_lock(&mm->page_table_lock);
> -		if (unlikely(!pmd_same(pmd, *pmdp))) {
> -			unlock_page(page);
> -			goto out_unlock;
> -		}
> -		goto clear_pmdnuma;
> -	}
> +				pmdp, pmd, addr, page, target_nid);
> +	if (!migrated)
> +		goto check_same;
>  
> -	task_numa_fault(current_nid, HPAGE_PMD_NR, migrated);
> +	task_numa_fault(target_nid, HPAGE_PMD_NR, true);
>  	return 0;
>  
> +check_same:
> +	spin_lock(&mm->page_table_lock);
> +	if (unlikely(!pmd_same(pmd, *pmdp)))
> +		goto out_unlock;
>  clear_pmdnuma:
>  	pmd = pmd_mknonnuma(pmd);
>  	set_pmd_at(mm, haddr, pmdp, pmd);
>  	VM_BUG_ON(pmd_numa(*pmdp));
>  	update_mmu_cache_pmd(vma, addr, pmdp);
> -	if (page_locked)
> -		unlock_page(page);
> -
>  out_unlock:
>  	spin_unlock(&mm->page_table_lock);
>  	if (current_nid != -1)
> -		task_numa_fault(current_nid, HPAGE_PMD_NR, migrated);
> +		task_numa_fault(current_nid, HPAGE_PMD_NR, false);
>  	return 0;
>  }
>  
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 73e432d..8ef1cbf 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1555,41 +1555,40 @@ bool numamigrate_update_ratelimit(pg_data_t *pgdat, unsigned long nr_pages)
>  
>  int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  {
> -	int ret = 0;
> +	int page_lru;
>  
>  	VM_BUG_ON(compound_order(page) && !PageTransHuge(page));
>  
>  	/* Avoid migrating to a node that is nearly full */
> -	if (migrate_balanced_pgdat(pgdat, 1UL << compound_order(page))) {
> -		int page_lru;
> +	if (!migrate_balanced_pgdat(pgdat, 1UL << compound_order(page)))
> +		return 0;
>  
> -		if (isolate_lru_page(page)) {
> -			put_page(page);
> -			return 0;
> -		}
> +	if (isolate_lru_page(page))
> +		return 0;
>  
> -		/* Page is isolated */
> -		ret = 1;
> -		page_lru = page_is_file_cache(page);
> -		if (!PageTransHuge(page))
> -			inc_zone_page_state(page, NR_ISOLATED_ANON + page_lru);
> -		else
> -			mod_zone_page_state(page_zone(page),
> -					NR_ISOLATED_ANON + page_lru,
> -					HPAGE_PMD_NR);
> +	/*
> +	 * migrate_misplaced_transhuge_page() skips page migration's usual
> +	 * check on page_count(), so we must do it here, now that the page
> +	 * has been isolated: a GUP pin, or any other pin, prevents migration.
> +	 * The expected page count is 3: 1 for page's mapcount and 1 for the
> +	 * caller's pin and 1 for the reference taken by isolate_lru_page().
> +	 */
> +	if (PageTransHuge(page) && page_count(page) != 3) {
> +		putback_lru_page(page);
> +		return 0;
>  	}
>  
> +	page_lru = page_is_file_cache(page);
> +	mod_zone_page_state(page_zone(page), NR_ISOLATED_ANON + page_lru,
> +				hpage_nr_pages(page));
> +
>  	/*
> -	 * Page is either isolated or there is not enough space on the target
> -	 * node. If isolated, then it has taken a reference count and the
> -	 * callers reference can be safely dropped without the page
> -	 * disappearing underneath us during migration. Otherwise the page is
> -	 * not to be migrated but the callers reference should still be
> -	 * dropped so it does not leak.
> +	 * Isolating the page has taken another reference, so the
> +	 * caller's reference can be safely dropped without the page
> +	 * disappearing underneath us during migration.
>  	 */
>  	put_page(page);
> -
> -	return ret;
> +	return 1;
>  }
>  
>  /*
> @@ -1600,7 +1599,7 @@ int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  int migrate_misplaced_page(struct page *page, int node)
>  {
>  	pg_data_t *pgdat = NODE_DATA(node);
> -	int isolated = 0;
> +	int isolated;
>  	int nr_remaining;
>  	LIST_HEAD(migratepages);
>  
> @@ -1608,20 +1607,16 @@ int migrate_misplaced_page(struct page *page, int node)
>  	 * Don't migrate pages that are mapped in multiple processes.
>  	 * TODO: Handle false sharing detection instead of this hammer
>  	 */
> -	if (page_mapcount(page) != 1) {
> -		put_page(page);
> +	if (page_mapcount(page) != 1)
>  		goto out;
> -	}
>  
>  	/*
>  	 * Rate-limit the amount of data that is being migrated to a node.
>  	 * Optimal placement is no good if the memory bus is saturated and
>  	 * all the time is being spent migrating!
>  	 */
> -	if (numamigrate_update_ratelimit(pgdat, 1)) {
> -		put_page(page);
> +	if (numamigrate_update_ratelimit(pgdat, 1))
>  		goto out;
> -	}
>  
>  	isolated = numamigrate_isolate_page(pgdat, page);
>  	if (!isolated)
> @@ -1638,12 +1633,19 @@ int migrate_misplaced_page(struct page *page, int node)
>  	} else
>  		count_vm_numa_event(NUMA_PAGE_MIGRATE);
>  	BUG_ON(!list_empty(&migratepages));
> -out:
>  	return isolated;
> +
> +out:
> +	put_page(page);
> +	return 0;
>  }
>  #endif /* CONFIG_NUMA_BALANCING */
>  
>  #if defined(CONFIG_NUMA_BALANCING) && defined(CONFIG_TRANSPARENT_HUGEPAGE)
> +/*
> + * Migrates a THP to a given target node. page must be locked and is unlocked
> + * before returning.
> + */
>  int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  				struct vm_area_struct *vma,
>  				pmd_t *pmd, pmd_t entry,
> @@ -1674,29 +1676,15 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  
>  	new_page = alloc_pages_node(node,
>  		(GFP_TRANSHUGE | GFP_THISNODE) & ~__GFP_WAIT, HPAGE_PMD_ORDER);
> -	if (!new_page) {
> -		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
> -		goto out_dropref;
> -	}
> +	if (!new_page)
> +		goto out_fail;
> +
>  	page_xchg_last_nid(new_page, page_last_nid(page));
>  
>  	isolated = numamigrate_isolate_page(pgdat, page);
> -
> -	/*
> -	 * Failing to isolate or a GUP pin prevents migration. The expected
> -	 * page count is 2. 1 for anonymous pages without a mapping and 1
> -	 * for the callers pin. If the page was isolated, the page will
> -	 * need to be put back on the LRU.
> -	 */
> -	if (!isolated || page_count(page) != 2) {
> -		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
> +	if (!isolated) {
>  		put_page(new_page);
> -		if (isolated) {
> -			putback_lru_page(page);
> -			isolated = 0;
> -			goto out;
> -		}
> -		goto out_keep_locked;
> +		goto out_fail;
>  	}
>  
>  	/* Prepare a page as a migration target */
> @@ -1728,6 +1716,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
>  		putback_lru_page(page);
>  
>  		count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
> +		isolated = 0;
>  		goto out;
>  	}
>  
> @@ -1772,9 +1761,11 @@ out:
>  			-HPAGE_PMD_NR);
>  	return isolated;
>  
> +out_fail:
> +	count_vm_events(PGMIGRATE_FAIL, HPAGE_PMD_NR);
>  out_dropref:
> +	unlock_page(page);
>  	put_page(page);
> -out_keep_locked:
>  	return 0;
>  }
>  #endif /* CONFIG_NUMA_BALANCING */
> -- 
> 1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>