Skip to contentSkip to conversation

Follow up work on NUMA Balancing

PATCH In review Mel Gorman 4 participants 21 messages Jan 22, 2013 – Jan 30, 2013

Subsystems (10)
Memory management Staging MM core Page allocator +6 more subsystems
lore
Share & tools
lore

Mel Gorman

author
#0 lore raw

cover Follow up work on NUMA Balancing

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
Signature · 7 lines
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#1 lore raw

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

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

Suggested-by Simon Jeons <[email protected]>
Signed-off-by Mel Gorman <[email protected]>
---
 include/linux/mm_types.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
include/linux/mm_types.h +3 -3
index f8f5162a..47047cb 100644
@@ -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;
Signature · 7 lines
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#2 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by Mel Gorman <[email protected]>
---
 mm/migrate.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
mm/migrate.c +4 -2
index c387786..73e432d 100644
@@ -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)) {
Signature · 7 lines
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#3 lore raw

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

In reply to 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 <[email protected]>
---
 include/linux/vmstat.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/vmstat.h +1 -1
index a13291f..5fd71a7 100644
@@ -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) \
Signature · 7 lines
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
maintainer
#4 lore raw

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

Quoted 4 lines · On Tue, 22 Jan 2013 17:12:39 +0000 Mel Gorman <[email protected]> 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.

Quoted 3 lines

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.

include/linux/vmstat.h static inline void vm_events_fold_cpu(int cpu) · line 91

Quoted 11 lines
include/linux/vmstat.h +1 -1
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#5 lore raw

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

Quoted 10 lines · Andrew Morton
Quoted 4 lines · On Tue, 22 Jan 2013 17:12:39 +0000 Mel Gorman <[email protected]> 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.

Quoted 6 lines
Quoted 3 lines

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.
====

Signature · 8 lines
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#6 lore raw

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

In reply to Mel Gorman

From: Peter Zijlstra <[email protected]>

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 <[email protected]>
---
 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
include/linux/mm.h +0 -40
index 66e2f7c..87420e6 100644
@@ -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.
include/linux/mm_types.h +1 -0
index 47047cb..d05d632 100644
@@ -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>
include/linux/mmzone.h +1 -21
index 73b64a3..5e6a814 100644
@@ -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)
include/linux/page-flags-layout.h new file +71 -0

71 lines not on this page · raw message

Signature · 7 lines
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#7 lore raw

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

In reply to Mel Gorman

From: Peter Zijlstra <[email protected]>

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."

[[email protected]: Minor modifications]
Signed-off-by Mel Gorman <[email protected]>
---
 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(-)
include/linux/mm.h +32 -1
index 87420e6..e25d47f 100644
@@ -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);
include/linux/mm_types.h +1 -1
index d05d632..ace9a5f 100644
@@ -174,7 +174,7 @@ struct page {
 	void *shadow;
 #endif
 
-#ifdef CONFIG_NUMA_BALANCING
+#ifdef LAST_NID_NOT_IN_PAGE_FLAGS
 	int _last_nid;
 #endif
 }
include/linux/page-flags-layout.h +25 -8
index 316805d..93506a1 100644
@@ -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 */
mm/memory.c +4 -0
index bb1369f..16e697c 100644
@@ -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;
Signature · 7 lines
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
maintainer
#8 lore raw

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

Quoted 7 lines · On Tue, 22 Jan 2013 17:12:41 +0000 Mel Gorman <[email protected]> wrote:

From: Peter Zijlstra <[email protected]>

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.

Quoted 2 lines
[[email protected]: Minor modifications]
Signed-off-by Mel Gorman <[email protected]>

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

Quoted 18 lines

...

include/linux/mm.h +15 -0
+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);

include/linux/mm.h static inline void reset_page_last_nid(struct page *page) · line 698

Quoted 10 lines
include/linux/mm.h +10 -0
+		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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#9 lore raw

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

Quoted 5 lines · Andrew Morton
Quoted 2 lines · On Tue, 22 Jan 2013 17:12:41 +0000 Mel Gorman <[email protected]> wrote:

From: Peter Zijlstra <[email protected]>

Quoted 8 lines
Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 5 lines
Quoted 2 lines
[[email protected]: Minor modifications]
Signed-off-by Mel Gorman <[email protected]>

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:".

Quoted 23 lines
Quoted 23 lines

...

include/linux/mm.h +20 -0
+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;
+}
Quoted 9 lines

include/linux/mm.h static inline void reset_page_last_nid(struct page *page) · line 698

Quoted 5 lines
include/linux/mm.h +5 -0
+
+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.

Quoted 3 lines

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

Will fix.

Signature · 8 lines
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#10 lore raw

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

Quoted 8 lines

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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#11 lore raw

PATCH 1/1 mm: init: Report on last-nid information stored in page->flags

In reply to Andrew Morton

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 <[email protected]>
---
 mm/mm_init.c |   31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)
mm/mm_init.c +19 -12
index 1ffd97a..c280a02 100644
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#12 lore raw

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

In reply to Andrew Morton
Quoted 4 lines · Andrew Morton

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.

Signature · 8 lines
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
maintainer
#13 lore raw

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

Quoted 8 lines · On Wed, 23 Jan 2013 14:25:07 +0000 Mel Gorman <[email protected]> wrote:
Quoted 4 lines · Andrew Morton

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().

Quoted 5 lines

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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#14 lore raw

PATCH 1/1 mm: Rename page struct field helpers

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 <[email protected]>
Signed-off-by Mel Gorman <[email protected]>
---
 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(-)
drivers/staging/ramster/zbud.c +1 -1
index a7c4361..cc2deff 100644
@@ -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;
drivers/staging/zsmalloc/zsmalloc-main.c +1 -1
index 09a9d35..c7785f2 100644
@@ -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)
include/linux/mm.h +10 -10
index 6e4468f..0aa0944 100644
@@ -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
mm/huge_memory.c +1 -1
index 648c102..c52311a 100644
@@ -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));
mm/mempolicy.c +1 -1
index e2df1c1..db6fc14 100644
@@ -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;
 	}
mm/migrate.c +2 -2
index 8ef1cbf..88422a1 100644
@@ -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) {
mm/mmzone.c +2 -2
index bce796e..2ac0afb 100644
@@ -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;
mm/page_alloc.c +5 -5
index df2022f..2d525c8 100644
@@ -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
mm/slob.c +1 -1
index a99fdf7..eeed4a0 100644
@@ -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;
 	}
mm/slub.c +1 -1
index ba2ca53..ebcc44e 100644
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#15 lore raw

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

Quoted 11 lines · 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 <[email protected]>
Signed-off-by Mel Gorman <[email protected]>

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

Quoted 238 lines
---
 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(-)
drivers/staging/ramster/zbud.c +1 -1
index a7c4361..cc2deff 100644
@@ -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;
drivers/staging/zsmalloc/zsmalloc-main.c +1 -1
index 09a9d35..c7785f2 100644
@@ -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)
include/linux/mm.h +10 -10
index 6e4468f..0aa0944 100644
@@ -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
mm/huge_memory.c +1 -1
index 648c102..c52311a 100644
@@ -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));
mm/mempolicy.c +1 -1
index e2df1c1..db6fc14 100644
@@ -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;
 	}
mm/migrate.c +2 -2
index 8ef1cbf..88422a1 100644
@@ -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) {
mm/mmzone.c +2 -2
index bce796e..2ac0afb 100644
@@ -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;
mm/page_alloc.c +5 -5
index df2022f..2d525c8 100644
@@ -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
mm/slob.c +1 -1
index a99fdf7..eeed4a0 100644
@@ -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;
 	}
mm/slub.c +0 -0
index ba2ca53..ebcc44e 100644

mm/slub.c static void __free_slab(struct kmem_cache *s, struct page *page) · line 1414

Quoted 8 lines
mm/slub.c +1 -1
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#16 lore raw

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

Quoted 11 lines · Hugh Dickins
Quoted 9 lines · 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.

Quoted 7 lines
Quoted 2 lines
Suggested-by Andrew Morton <[email protected]>
Signed-off-by Mel Gorman <[email protected]>

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.

Quoted 5 lines

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.

Quoted 9 lines

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 <[email protected]>
Signed-off-by Mel Gorman <[email protected]>
---
 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(-)
include/linux/mm.h +3 -3
index 6e4468f..6356db0 100644
@@ -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);
 }
mm/huge_memory.c +1 -1
index 648c102..ed97040 100644
@@ -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));
mm/mempolicy.c +1 -1
index e2df1c1..61226db 100644
@@ -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;
 	}
mm/migrate.c +2 -2
index 8ef1cbf..4d9b724 100644
@@ -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) {
mm/mmzone.c +1 -1
index bce796e..de2a951 100644
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#17 lore raw

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

Quoted 8 lines · Mel Gorman

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.

Quoted 4 lines

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

Suggested-by Hugh Dickins <[email protected]>
Signed-off-by Mel Gorman <[email protected]>

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.

Quoted 93 lines
---
 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(-)
include/linux/mm.h +3 -3
index 6e4468f..6356db0 100644
@@ -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);
 }
mm/huge_memory.c +1 -1
index 648c102..ed97040 100644
@@ -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));
mm/mempolicy.c +1 -1
index e2df1c1..61226db 100644
@@ -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;
 	}
mm/migrate.c +2 -2
index 8ef1cbf..4d9b724 100644
@@ -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) {
mm/mmzone.c +0 -0
index bce796e..de2a951 100644

mm/mmzone.c void lruvec_init(struct lruvec *lruvec) · line 104

Quoted 8 lines
mm/mmzone.c +1 -1
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#18 lore raw

PATCH 1/1 mm: uninline page_xchg_last_nid()

In reply to Andrew Morton

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 <[email protected]>
---
 include/linux/mm.h |   21 +++++----------------
 mm/mmzone.c        |   20 +++++++++++++++++++-
 2 files changed, 24 insertions(+), 17 deletions(-)
include/linux/mm.h +5 -16
index e25d47f..6e4468f 100644
@@ -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
mm/mmzone.c +19 -1
index 4596d81..bce796e 100644
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#19 lore raw

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

In reply to Mel Gorman

From: Hugh Dickins <[email protected]>

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().

[[email protected]: changelog only]
Signed-off-by Mel Gorman <[email protected]>
---
 mm/huge_memory.c |   28 ++++++----------
 mm/migrate.c     |   95 ++++++++++++++++++++++++------------------------------
 2 files changed, 52 insertions(+), 71 deletions(-)
mm/huge_memory.c +9 -19
index 6001ee6..648c102 100644
@@ -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;
 }
mm/migrate.c +43 -52

167 lines not on this page · raw message

Signature · 7 lines
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#20 lore raw

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

Quoted 10 lines · Mel Gorman

From: Hugh Dickins <[email protected]>

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.

Quoted 9 lines

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().

[[email protected]: 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 <[email protected]>
Quoted 250 lines
Signed-off-by Mel Gorman <[email protected]>
---
 mm/huge_memory.c |   28 ++++++----------
 mm/migrate.c     |   95 ++++++++++++++++++++++++------------------------------
 2 files changed, 52 insertions(+), 71 deletions(-)
mm/huge_memory.c +9 -19
index 6001ee6..648c102 100644
@@ -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;
 }
mm/migrate.c +43 -52
index 73e432d..8ef1cbf 100644
@@ -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 */
Signature · 1 line
1.7.9.2

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