Migrate Pages in lieu of discard

From: Dave Hansen <[email protected]>

Prepare for the kernel to auto-migrate pages to other memory nodes with a user defined node migration table. This allows creating single migration target for each NUMA node to enable the kernel to do NUMA page migrations instead of simply reclaiming colder pages. A node with no target is a "terminal node", so reclaim acts normally there. The migration target does not fundamentally _need_ to be a single node, but this implementation starts there to limit complexity.

If you consider the migration path as a graph, cycles (loops) in the graph are disallowed. This avoids wasting resources by constantly migrating (A->B, B->A, A->B ...). The expectation is that cycles will never be allowed.

Signed-off-by Dave Hansen <[email protected]>

Reviewed-by Yang Shi <[email protected]>

Cc Wei Xu <[email protected]>

Cc David Rientjes <[email protected]>

Cc Huang Ying <[email protected]>

Cc Dan Williams <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc osalvador <[email protected]>

I think this patch and patch#2 could be squashed

Reviewed-by Oscar Salvador <[email protected]>

From: Dave Hansen <[email protected]>

Prepare for the kernel to auto-migrate pages to other memory nodes with a user defined node migration table. This allows creating single migration target for each NUMA node to enable the kernel to do NUMA page migrations instead of simply reclaiming colder pages. A node with no target is a "terminal node", so reclaim acts normally there. The migration target does not fundamentally _need_ to be a single node, but this implementation starts there to limit complexity.

If you consider the migration path as a graph, cycles (loops) in the graph are disallowed. This avoids wasting resources by constantly migrating (A->B, B->A, A->B ...). The expectation is that cycles will never be allowed.

Signed-off-by Dave Hansen <[email protected]>

Reviewed-by Yang Shi <[email protected]>

Cc Wei Xu <[email protected]>

Cc David Rientjes <[email protected]>

Cc Huang Ying <[email protected]>

Cc Dan Williams <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc osalvador <[email protected]>

--

changes since 20200122:
* Make node_demotion[] __read_mostly

changes in July 2020:
- Remove loop from next_demotion_node() and get_online_mems(). This means that the node returned by next_demotion_node() might now be offline, but the worst case is that the allocation fails. That's fine since it is transient.
---

 b/mm/migrate.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff -puN mm/migrate.c~0006-node-Define-and-export-memory-migration-path mm/migrate.c

mm/migrate.c 2021-03-31 15:17:10.742000264 -0700 renamed from mm/migrate.c~0006-node-Define-and-export-memory-migration-path 2021-03-31 15:17:10.734000264 -0700 +17 -0

@@ -1163,6 +1163,23 @@ out:
        return rc;
 }
 
+static int node_demotion[MAX_NUMNODES] __read_mostly =
+       {[0 ...  MAX_NUMNODES - 1] = NUMA_NO_NODE};
+
+/**
+ * next_demotion_node() - Get the next node in the demotion path
+ * @node: The starting node to lookup the next node
+ *
+ * @returns: node id for next memory node in the demotion path hierarchy
+ * from @node; NUMA_NO_NODE if @node is terminal.  This does not keep
+ * @node online or guarantee that it *continues* to be the next demotion
+ * target.
+ */
+int next_demotion_node(int node)
+{
+       return node_demotion[node];
+}
+
 /*
  * Obtain the lock on page, remove all ptes and migrate the page
  * to the newly allocated page in newpage.

_

From: Dave Hansen <[email protected]>

When memory fills up on a node, memory contents can be automatically migrated to another node. The biggest problems are knowing when to migrate and to where the migration should be targeted.

The most straightforward way to generate the "to where" list would be to follow the page allocator fallback lists. Those lists already tell us if memory is full where to look next. It would also be logical to move memory in that order.

But, the allocator fallback lists have a fatal flaw: most nodes appear in all the lists. This would potentially lead to migration cycles (A->B, B->A, A->B, ...).

Instead of using the allocator fallback lists directly, keep a separate node migration ordering. But, reuse the same data used to generate page allocator fallback in the first place:
find_next_best_node().

This means that the firmware data used to populate node distances essentially dictates the ordering for now. It should also be architecture-neutral since all NUMA architectures have a working find_next_best_node().

The protocol for node_demotion[] access and writing is not standard. It has no specific locking and is intended to be read locklessly. Readers must take care to avoid observing changes that appear incoherent. This was done so that node_demotion[]

locking has no chance of becoming a bottleneck on large systems with lots of CPUs in direct reclaim.

This code is unused for now. It will be called later in the series.

Signed-off-by Dave Hansen <[email protected]>

Reviewed-by Yang Shi <[email protected]>

Cc Wei Xu <[email protected]>

Cc David Rientjes <[email protected]>

Cc Huang Ying <[email protected]>

Cc Dan Williams <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc osalvador <[email protected]>

mm/migrate.c renamed from mm/migrate.c~auto-setup-default-migration-path-from-firmware +49 -0

+static void __set_migration_target_nodes(void)
+{
+	nodemask_t next_pass	= NODE_MASK_NONE;
+	nodemask_t this_pass	= NODE_MASK_NONE;
+	nodemask_t used_targets = NODE_MASK_NONE;
+	int node;
+
+	/*
+	 * Avoid any oddities like cycles that could occur
+	 * from changes in the topology.  This will leave
+	 * a momentary gap when migration is disabled.
+	 */
+	disable_all_migrate_targets();
+
+	/*
+	 * Ensure that the "disable" is visible across the system.
+	 * Readers will see either a combination of before+disable
+	 * state or disable+after.  They will never see before and
+	 * after state together.
+	 *
+	 * The before+after state together might have cycles and
+	 * could cause readers to do things like loop until this
+	 * function finishes.  This ensures they can only see a
+	 * single "bad" read and would, for instance, only loop
+	 * once.
+	 */
+	smp_wmb();
+
+	/*
+	 * Allocations go close to CPUs, first.  Assume that
+	 * the migration path starts at the nodes with CPUs.
+	 */
+	next_pass = node_states[N_CPU];
+again:
+	this_pass = next_pass;
+	next_pass = NODE_MASK_NONE;
+	/*
+	 * To avoid cycles in the migration "graph", ensure
+	 * that migration sources are not future targets by
+	 * setting them in 'used_targets'.  Do this only
+	 * once per pass so that multiple source nodes can
+	 * share a target node.
+	 *
+	 * 'used_targets' will become unavailable in future
+	 * passes.  This limits some opportunities for
+	 * multiple source nodes to share a destination.
+	 */
+	nodes_or(used_targets, used_targets, this_pass);
+	for_each_node_mask(node, this_pass) {

Quoted 4 lines · Dave Hansen

The protocol for node_demotion[] access and writing is not standard. It has no specific locking and is intended to be read locklessly. Readers must take care to avoid observing changes that appear incoherent. This was done so that node_demotion[]

It might be just me being dense here, but that reads odd.

"Readers must take care to avoid observing changes that appear incoherent" - I am not sure what is that supposed to mean.

I guess you mean readers of next_demotion_node()?
And if so, how do they have to take care? And what would apply for "incoherent" terminology here?

Readers of node_demotion[] (such as next_demotion_node() callers) must take care to avoid observing changes that appear incoherent. For instance, even though no demotion cycles are allowed, it's possible that a cycle could be observed.

Let's say that there are three nodes, A, B and C. node_demotion[] is set up to have this path:

        A -> B -> C

Suppose it was modified to instead represent this path:

        A -> C -> B

There is nothing to stop a reader from seeing B->C and then a moment later seeting C->B. That *appears* to be a cycle. This can be avoided with RCU and will be implemented in a later patch.

Quoted 16 lines

mm/migrate.c renamed from mm/migrate.c~auto-setup-default-migration-path-from-firmware +16 -0

+again:
+	this_pass = next_pass;
+	next_pass = NODE_MASK_NONE;
+	/*
+	 * To avoid cycles in the migration "graph", ensure
+	 * that migration sources are not future targets by
+	 * setting them in 'used_targets'.  Do this only
+	 * once per pass so that multiple source nodes can
+	 * share a target node.
+	 *
+	 * 'used_targets' will become unavailable in future
+	 * passes.  This limits some opportunities for
+	 * multiple source nodes to share a destination.
+	 */
+	nodes_or(used_targets, used_targets, this_pass);
+	for_each_node_mask(node, this_pass) {

mm/migrate.c void migrate_vma_finalize(struct migrate · line 3343

Quoted 10 lines

mm/migrate.c renamed from mm/migrate.c~auto-setup-default-migration-path-from-firmware +10 -0

+		int target_node = establish_migrate_target(node, &used_targets);
+
+		if (target_node == NUMA_NO_NODE)
+			continue;
+
+		/* Visit targets from this pass in the next pass: */
+		node_set(target_node, next_pass);
+	}
+	/* Is another pass necessary? */
+	if (!nodes_empty(next_pass))

When I read this I was about puzzled and it took me a while to figure out how the passes were made.
I think this could benefit from a better explanation on how the passes are being performed e.g: why next_pass should be empty before leaving.

Other than that looks good to me.

                /*
                 * Visit targets from this pass in the next pass.
                 * Eventually, every node will have been part of
                 * a pass, and will become set in 'used_targets'.
                 */
                node_set(target_node, next_pass);
        }
        /*
         * 'next_pass' contains nodes which became migration
         * targets in this pass.  Make additional passes until
         * no more migrations targets are available.
         */
        if (!nodes_empty(next_pass))
                goto again;
}

I've fleshed out the description a bit. I hope this helps?

+/*
+ * node_demotion[] example:
+ *
+ * Consider a system with two sockets. Each socket has
+ * three classes of memory attached: fast, medium and slow.
+ * Each memory class is placed in its own NUMA node. The
+ * CPUs are placed in the node with the "fast" memory. The
+ * 6 NUMA nodes (0-5) might be split among the sockets like
+ * this:
+ *
+ * Socket A: 0, 1, 2
+ * Socket B: 3, 4, 5
+ *
+ * When Node 0 fills up, its memory should be migrated to
+ * Node 1. When Node 1 fills up, it should be migrated to
+ * Node 2. The migration path start on the nodes with the
+ * processors (since allocations default to this node) and
+ * fast memory, progress through medium and end with the
+ * slow memory:
+ *
+ * 0 -> 1 -> 2 -> stop
+ * 3 -> 4 -> 5 -> stop
+ *
+ * This is represented in the node_demotion[] like this:

+ *
+ * { 1, // Node 0 migrates to 1
+ * 2, // Node 1 migrates to 2
+ * -1, // Node 2 does not migrate
+ * 4, // Node 3 migrates to 4
+ * 5, // Node 4 migrates to 5
+ * -1} // Node 5 does not migrate
+ */

mm/migrate.c renamed from mm/migrate.c~auto-setup-default-migration-path-from-firmware +17 -0

+again:
+       this_pass = next_pass;
+       next_pass = NODE_MASK_NONE;
+       /*
+        * To avoid cycles in the migration "graph", ensure
+        * that migration sources are not future targets by
+        * setting them in 'used_targets'.  Do this only
+        * once per pass so that multiple source nodes can
+        * share a target node.
+        *
+        * 'used_targets' will become unavailable in future
+        * passes.  This limits some opportunities for
+        * multiple source nodes to share a destination.
+        */
+       nodes_or(used_targets, used_targets, this_pass);
+       for_each_node_mask(node, this_pass) {
+               int target_node = establish_migrate_target(node, &used_targets);

Quoted 11 lines · Dave Hansen

+ * When Node 0 fills up, its memory should be migrated to
+ * Node 1. When Node 1 fills up, it should be migrated to
+ * Node 2. The migration path start on the nodes with the
+ * processors (since allocations default to this node) and
+ * fast memory, progress through medium and end with the
+ * slow memory:
+ *
+ * 0 -> 1 -> 2 -> stop
+ * 3 -> 4 -> 5 -> stop
+ *
+ * This is represented in the node_demotion[] like this:

Quoted 8 lines

+ *
+ * { 1, // Node 0 migrates to 1
+ * 2, // Node 1 migrates to 2
+ * -1, // Node 2 does not migrate
+ * 4, // Node 3 migrates to 4
+ * 5, // Node 4 migrates to 5
+ * -1} // Node 5 does not migrate
+ */

In this example, if we want to support multiple nodes as the demotion target of a source node, we can group these nodes into three tiers (classes):

fast class:
0 -> {1, 4} // 1 is the preferred
3 -> {4, 1} // 4 is the preferred

medium class:
1 -> {2, 5} // 2 is the preferred
4 -> {5, 2} // 5 is the preferred

slow class:
2 -> stop
5 -> stop

In Dave's example, list is created in a way that stays local to the socket, and we go from the fast one to the slow one.

Quoted 29 lines · Wei Xu

Quoted 11 lines · Dave Hansen

+ * When Node 0 fills up, its memory should be migrated to
+ * Node 1. When Node 1 fills up, it should be migrated to
+ * Node 2. The migration path start on the nodes with the
+ * processors (since allocations default to this node) and
+ * fast memory, progress through medium and end with the
+ * slow memory:
+ *
+ * 0 -> 1 -> 2 -> stop
+ * 3 -> 4 -> 5 -> stop
+ *
+ * This is represented in the node_demotion[] like this:

Quoted 8 lines

+ *
+ * { 1, // Node 0 migrates to 1
+ * 2, // Node 1 migrates to 2
+ * -1, // Node 2 does not migrate
+ * 4, // Node 3 migrates to 4
+ * 5, // Node 4 migrates to 5
+ * -1} // Node 5 does not migrate
+ */

In this example, if we want to support multiple nodes as the demotion target of a source node, we can group these nodes into three tiers (classes):

fast class:
0 -> {1, 4} // 1 is the preferred
3 -> {4, 1} // 4 is the preferred

Quoted 7 lines

medium class:
1 -> {2, 5} // 2 is the preferred
4 -> {5, 2} // 5 is the preferred

slow class:
2 -> stop
5 -> stop

Hi Wei Xu,

I have some questions about it

Fast class/memory are pictured as those nodes with CPUs, while Slow class/memory are PMEM, right?
Then, what stands for medium class/memory?

My guest best is that fast class is something like HBM (High Bandwidth Memory), medium class is ordinary RAM, slow class is PMEM.

Quoted 1 line · David Hildenbrand

My guest best is that fast class is something like HBM (High Bandwidth

Quoted 1 line

Memory), medium class is ordinary RAM, slow class is PMEM.

I see, thanks for the hint David ;-)

Hi Wei Xu,

I have some questions about it

Fast class/memory are pictured as those nodes with CPUs, while Slow class/memory are PMEM, right?
Then, what stands for medium class/memory?

In Dave's example, list is created in a way that stays local to the socket, and we go from the fast one to the slow one.
In yours, lists are created taking the fastest nodes from all sockets and we work our way down, which means have cross-socket nodes in the list. How much of a penalty is that?

And while I get your point, I am not sure if that is what we pretend here. This patchset aims to place cold pages that are about to be reclaim in slower nodes to give them a second chance, while your design seems more to have kind of different memory clases and be able to place applications in one of those tiers depending on its demands or sysadmin-demand.

Could you expand some more?

Quoted 3 lines · Oscar Salvador

Fast class/memory are pictured as those nodes with CPUs, while Slow class/memory are PMEM, right?
Then, what stands for medium class/memory?

That is Dave's example. I think David's guess makes sense (HBM - fast, DRAM - medium, PMEM - slow). It may also be possible that we have DDR5 as fast, CXL-DDR4 as medium, and CXL-PMEM as slow. But the most likely use cases for now should be just two tiers: DRAM vs PMEM or other types of slower memory devices.

This can help enable more flexible demotion policies to be configured, such as to allow a cgroup to allocate from all fast tier nodes, but only demote to a local slow tier node. Such a policy can reduce memory stranding at the fast tier (compared to if memory hardwall is used) and still allow demotion from all fast tier nodes without incurring the expensive random accesses to the demoted pages if they were demoted to remote slow tier nodes.

Quoted 7 lines

This can help enable more flexible demotion policies to be configured, such as to allow a cgroup to allocate from all fast tier nodes, but only demote to a local slow tier node. Such a policy can reduce memory stranding at the fast tier (compared to if memory hardwall is used) and still allow demotion from all fast tier nodes without incurring the expensive random accesses to the demoted pages if they were demoted to remote slow tier nodes.

Could you explain this stranding effect in a bit more detail? I'm not quite following.

From: Dave Hansen <[email protected]>

Reclaim-based migration is attempting to optimize data placement in memory based on the system topology. If the system changes, so must the migration ordering.

The implementation is conceptually simple and entirely unoptimized. On any memory or CPU hotplug events, assume that a node was added or removed and recalculate all migration targets. This ensures that the node_demotion[] array is always ready to be used in case the new reclaim mode is enabled.

This recalculation is far from optimal, most glaringly that it does not even attempt to figure out the hotplug event would have some *actual* effect on the demotion order. But, given the expected paucity of hotplug events, this should be fine.

=== What does RCU provide? ===

Imaginge a simple loop which walks down the demotion path looking for the last node:

        terminal_node = start_node;
        while (node_demotion[terminal_node] != NUMA_NO_NODE) {
                terminal_node = node_demotion[terminal_node];
        }

The initial values are:

        node_demotion[0] = 1;
        node_demotion[1] = NUMA_NO_NODE;

and are updated to:

        node_demotion[0] = NUMA_NO_NODE;
        node_demotion[1] = 0;

What guarantees that the loop did not observe:

        node_demotion[0] = 1;
        node_demotion[1] = 0;

and would loop forever?

With RCU, a rcu_read_lock/unlock() can be placed around the loop. Since the write side does a synchronize_rcu(), the loop that observed the old contents is known to be complete after the synchronize_rcu() has completed.

RCU, combined with disable_all_migrate_targets(), ensures that the old migration state is not visible by the time __set_migration_target_nodes() is called.

=== What does READ_ONCE() provide? ===

READ_ONCE() forbids the compiler from merging or reordering successive reads of node_demotion[]. This ensures that any updates are *eventually* observed.

Consider the above loop again. The compiler could theoretically read the entirety of node_demotion[] into local storage (registers) and never go back to memory, and *permanently* observe bad values for node_demotion[].

Note: RCU does not provide any universal compiler-ordering guarantees:

	https://lore.kernel.org/lkml/[email protected]/

Signed-off-by Dave Hansen <[email protected]>

Reviewed-by Yang Shi <[email protected]>

Cc Wei Xu <[email protected]>

Cc David Rientjes <[email protected]>

Cc Huang Ying <[email protected]>

Cc Dan Williams <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc osalvador <[email protected]>

+#if defined(CONFIG_MEMORY_HOTPLUG)

I am not really into PMEM, and I ignore whether we need CONFIG_MEMORY_HOTPLUG in order to have such memory on the system. If so, the following can be partly ignored.

Quoted 3 lines · Oscar Salvador

I am not really into PMEM, and I ignore whether we need CONFIG_MEMORY_HOTPLUG in order to have such memory on the system. If so, the following can be partly ignored.

Ok, I refreshed by memory with [1].

Quoted 3 lines · Oscar Salvador

I am not really into PMEM, and I ignore whether we need CONFIG_MEMORY_HOTPLUG in order to have such memory on the system. If so, the following can be partly ignored.

Ok, I refreshed by memory with [1].
From that, it seems that in order to use PMEM as RAM we need CONFIG_MEMORY_HOTPLUG.
But is that always the case? Can happen that in some scenario PMEM comes ready to use and we do not need the hotplug trick?

The only way to add more System RAM is via add_memory() and friends like add_memory_driver_managed(). These all require CONFIG_MEMORY_HOTPLUG.

Quoted 2 lines · David Hildenbrand

The only way to add more System RAM is via add_memory() and friends like add_memory_driver_managed(). These all require CONFIG_MEMORY_HOTPLUG.

Yeah, my point was more towards whether PMEM can come in a way that it does not have to be hotplugged, but come functional by default (as RAM). But after having read all papers out there, I do not think that it is possible.

From: Yang Shi <[email protected]>

The migrate_pages() returns the number of pages that were not migrated, or an error code. When returning an error code, there is no way to know how many pages were migrated or not migrated.

In the following patch, migrate_pages() is used to demote pages to PMEM node, we need account how many pages are reclaimed (demoted) since page reclaim behavior depends on this. Add *nr_succeeded parameter to make migrate_pages() return how many pages are demoted successfully for all cases.

Signed-off-by Yang Shi <[email protected]>

Signed-off-by Dave Hansen <[email protected]>

Reviewed-by Yang Shi <[email protected]>

Cc Wei Xu <[email protected]>

Cc Huang Ying <[email protected]>

Cc Dan Williams <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc osalvador <[email protected]>

--

Note: Yang Shi originally wrote the patch, thus the SoB. There was also a Reviewed-by provided since there were some modifications made to this after the original work.

Changes since 20210302:
* Fix definition of CONFIG_MIGRATION=n stub migrate_pages(). Its parameters were wrong, but oddly enough did not generate any compile errors.

Changes since 20200122:
* Fix migrate_pages() to manipulate nr_succeeded *value* rather than the pointer.
---

 b/include/linux/migrate.h |    5 +++--
 b/mm/compaction.c         |    3 ++-
 b/mm/gup.c                |    3 ++-
 b/mm/memory-failure.c     |    4 +++-
 b/mm/memory_hotplug.c     |    4 +++-
 b/mm/mempolicy.c          |    8 ++++++--
 b/mm/migrate.c            |   19 +++++++++++--------
 b/mm/page_alloc.c         |    9 ++++++---
 8 files changed, 36 insertions(+), 19 deletions(-)

diff -puN include/linux/migrate.h~migrate_pages-add-success-return include/linux/migrate.h
--- a/include/linux/migrate.h~migrate_pages-add-success-return 2021-03-31 15:17:14.144000255 -0700
+++ b/include/linux/migrate.h 2021-03-31 15:17:14.182000255 -0700

include/linux/migrate.h renamed from include/linux/migrate.h~migrate_pages-add-success-return +2 -1

@@ -40,7 +40,8 @@ extern int migrate_page(struct address_s
                        struct page *newpage, struct page *page,
                        enum migrate_mode mode);
 extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t
 free,
-               unsigned long private, enum migrate_mode mode, int reason);
+               unsigned long private, enum migrate_mode mode, int reason,
+               unsigned int *nr_succeeded);
 extern struct page *alloc_migration_target(struct page *page, unsigned
 long private);

extern int isolate_movable_page(struct page *page, isolate_mode_t mode); extern void putback_movable_page(struct page *page);

include/linux/migrate.h renamed from include/linux/migrate.h~migrate_pages-add-success-return +1 -1

@@ -58,7 +59,7 @@ extern int migrate_page_move_mapping(str
 static inline void putback_movable_pages(struct list_head *l) {}
 static inline int migrate_pages(struct list_head *l, new_page_t new,
                free_page_t free, unsigned long private, enum migrate_mode
 mode,
-               int reason)
+               int reason, unsigned int *nr_succeeded)
        { return -ENOSYS; }
 static inline struct page *alloc_migration_target(struct page *page,

unsigned long private)
diff -puN mm/compaction.c~migrate_pages-add-success-return mm/compaction.c --- a/mm/compaction.c~migrate_pages-add-success-return 2021-03-31 15:17:14.146000255 -0700
+++ b/mm/compaction.c 2021-03-31 15:17:14.186000255 -0700

mm/compaction.c renamed from mm/compaction.c~migrate_pages-add-success-return +1 -0

@@ -2247,6 +2247,7 @@ compact_zone(struct compact_control *cc,
        unsigned long last_migrated_pfn;
        const bool sync = cc->mode != MIGRATE_ASYNC;
        bool update_cached;
+       unsigned int nr_succeeded = 0;
 
        /*
         * These counters track activities during zone compaction.

Initialize

mm/compaction.c renamed from mm/compaction.c~migrate_pages-add-success-return +1 -1

@@ -2364,7 +2365,7 @@ compact_zone(struct compact_control *cc,
 
                err = migrate_pages(&cc->migratepages, compaction_alloc,
                                compaction_free, (unsigned long)cc,
 cc->mode,
-                               MR_COMPACTION);
+                               MR_COMPACTION, &nr_succeeded);
 
                trace_mm_compaction_migratepages(cc->nr_migratepages, err,

                                                        &cc->migratepages);

diff -puN mm/gup.c~migrate_pages-add-success-return mm/gup.c --- a/mm/gup.c~migrate_pages-add-success-return 2021-03-31 15:17:14.150000255 -0700
+++ b/mm/gup.c 2021-03-31 15:17:14.190000255 -0700

mm/gup.c renamed from mm/gup.c~migrate_pages-add-success-return +2 -1

@@ -1550,6 +1550,7 @@ static long check_and_migrate_cma_pages(
        unsigned long i;
        unsigned long step;
        bool drain_allow = true;
+       unsigned int nr_succeeded = 0;
        bool migrate_allow = true;
        LIST_HEAD(cma_page_list);
        long ret = nr_pages;

@@ -1606,7 +1607,7 @@ check_again:
                                put_page(pages[i]);
 
                if (migrate_pages(&cma_page_list, alloc_migration_target,
 NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC,
 MR_CONTIG_RANGE)) {
+                       (unsigned long)&mtc, MIGRATE_SYNC,
 MR_CONTIG_RANGE, &nr_succeeded)) {

/*
* some of the pages failed migration. Do
get_user_pages
* without migration.
diff -puN mm/memory-failure.c~migrate_pages-add-success-return mm/memory-failure.c
--- a/mm/memory-failure.c~migrate_pages-add-success-return 2021-03-31
15:17:14.155000255 -0700
+++ b/mm/memory-failure.c 2021-03-31 15:17:14.194000255 -0700

mm/memory-failure.c renamed from mm/memory-failure.c~migrate_pages-add-success-return +3 -1

@@ -1809,6 +1809,7 @@ static int __soft_offline_page(struct pa
        unsigned long pfn = page_to_pfn(page);
        struct page *hpage = compound_head(page);
        char const *msg_page[] = {"page", "hugepage"};
+       unsigned int nr_succeeded = 0;
        bool huge = PageHuge(page);
        LIST_HEAD(pagelist);
        struct migration_target_control mtc = {

@@ -1852,7 +1853,8 @@ static int __soft_offline_page(struct pa
 
        if (isolate_page(hpage, &pagelist)) {
                ret = migrate_pages(&pagelist, alloc_migration_target,
 NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC,
 MR_MEMORY_FAILURE);
+                       (unsigned long)&mtc, MIGRATE_SYNC,
 MR_MEMORY_FAILURE,
+                       &nr_succeeded);

                if (!ret) {
                        bool release = !huge;

diff -puN mm/memory_hotplug.c~migrate_pages-add-success-return mm/memory_hotplug.c
--- a/mm/memory_hotplug.c~migrate_pages-add-success-return 2021-03-31
15:17:14.160000255 -0700
+++ b/mm/memory_hotplug.c 2021-03-31 15:17:14.197000255 -0700

mm/memory_hotplug.c renamed from mm/memory_hotplug.c~migrate_pages-add-success-return +3 -1

@@ -1392,6 +1392,7 @@ do_migrate_range(unsigned long start_pfn
        unsigned long pfn;
        struct page *page, *head;
        int ret = 0;
+       unsigned int nr_succeeded = 0;
        LIST_HEAD(source);
 
        for (pfn = start_pfn; pfn < end_pfn; pfn++) {

@@ -1466,7 +1467,8 @@ do_migrate_range(unsigned long start_pfn
                if (nodes_empty(nmask))
                        node_set(mtc.nid, nmask);
                ret = migrate_pages(&source, alloc_migration_target, NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC,
 MR_MEMORY_HOTPLUG);
+                       (unsigned long)&mtc, MIGRATE_SYNC,
 MR_MEMORY_HOTPLUG,
+                       &nr_succeeded);
                if (ret) {

list_for_each_entry(page, &source, lru) {
pr_warn("migrating pfn %lx failed ret:%d ",
diff -puN mm/mempolicy.c~migrate_pages-add-success-return mm/mempolicy.c
--- a/mm/mempolicy.c~migrate_pages-add-success-return 2021-03-31
15:17:14.163000255 -0700
+++ b/mm/mempolicy.c 2021-03-31 15:17:14.203000255 -0700

mm/mempolicy.c renamed from mm/mempolicy.c~migrate_pages-add-success-return +3 -1

@@ -1081,6 +1081,7 @@ static int migrate_page_add(struct page
 static int migrate_to_node(struct mm_struct *mm, int source, int dest,
                           int flags)
 {
+       unsigned int nr_succeeded = 0;
        nodemask_t nmask;
        LIST_HEAD(pagelist);
        int err = 0;

@@ -1103,7 +1104,8 @@ static int migrate_to_node(struct mm_str
 
        if (!list_empty(&pagelist)) {
                err = migrate_pages(&pagelist, alloc_migration_target,
 NULL,
-                               (unsigned long)&mtc, MIGRATE_SYNC,
 MR_SYSCALL);
+                               (unsigned long)&mtc, MIGRATE_SYNC,
 MR_SYSCALL,
+                               &nr_succeeded);

                if (err)
                        putback_movable_pages(&pagelist);
        }

mm/mempolicy.c renamed from mm/mempolicy.c~migrate_pages-add-success-return +3 -1

@@ -1278,6 +1280,7 @@ static long do_mbind(unsigned long start
                     nodemask_t *nmask, unsigned long flags)
 {
        struct mm_struct *mm = current->mm;
+       unsigned int nr_succeeded = 0;
        struct mempolicy *new;
        unsigned long end;
        int err;

@@ -1355,7 +1358,8 @@ static long do_mbind(unsigned long start
                if (!list_empty(&pagelist)) {
                        WARN_ON_ONCE(flags & MPOL_MF_LAZY);
                        nr_failed = migrate_pages(&pagelist, new_page,
 NULL,
-                               start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND);
+                               start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND,
+                               &nr_succeeded);
                        if (nr_failed)
                                putback_movable_pages(&pagelist);

                }

diff -puN mm/migrate.c~migrate_pages-add-success-return mm/migrate.c
--- a/mm/migrate.c~migrate_pages-add-success-return 2021-03-31
15:17:14.168000255 -0700
+++ b/mm/migrate.c 2021-03-31 15:17:14.207000255 -0700

(quoted diff) +0 -0

@@ -1493,6 +1493,7 @@ static inline int try_split_thp(struct p
  * @mode:              The migration mode that specifies the constraints

for

  *                     page migration, if any.
  * @reason:            The reason for page migration.
+ * @nr_succeeded:      The number of pages migrated successfully.

*
* The function returns after 10 attempts or if no pages are movable any
more
* because the list has become empty or no retryable pages exist any more.

mm/migrate.c renamed from mm/migrate.c~migrate_pages-add-success-return +1 -2

@@ -1503,12 +1504,11 @@ static inline int try_split_thp(struct p
  */
 int migrate_pages(struct list_head *from, new_page_t get_new_page,
                free_page_t put_new_page, unsigned long private,
-               enum migrate_mode mode, int reason)
+               enum migrate_mode mode, int reason, unsigned int
 *nr_succeeded)
 {
        int retry = 1;
        int thp_retry = 1;
        int nr_failed = 0;
-       int nr_succeeded = 0;
        int nr_thp_succeeded = 0;
        int nr_thp_failed = 0;

        int nr_thp_split = 0;

mm/migrate.c renamed from mm/migrate.c~migrate_pages-add-success-return +9 -6

@@ -1611,10 +1611,10 @@ retry:
                        case MIGRATEPAGE_SUCCESS:
                                if (is_thp) {
                                        nr_thp_succeeded++;
-                                       nr_succeeded += nr_subpages;
+                                       *nr_succeeded += nr_subpages;
                                        break;
                                }
-                               nr_succeeded++;
+                               (*nr_succeeded)++;
                                break;
                        default:
                                /*

@@ -1643,12 +1643,12 @@ out:
         */
        list_splice(&ret_pages, from);
 
-       count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
+       count_vm_events(PGMIGRATE_SUCCESS, *nr_succeeded);
        count_vm_events(PGMIGRATE_FAIL, nr_failed);
        count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
        count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
        count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
-       trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
+       trace_mm_migrate_pages(*nr_succeeded, nr_failed, nr_thp_succeeded,
                               nr_thp_failed, nr_thp_split, mode, reason);
 
        if (!swapwrite)

@@ -1716,6 +1716,7 @@ static int store_status(int __user *stat
 static int do_move_pages_to_node(struct mm_struct *mm,
                struct list_head *pagelist, int node)
 {
+       unsigned int nr_succeeded = 0;
        int err;
        struct migration_target_control mtc = {
                .nid = node,

@@ -1723,7 +1724,8 @@ static int do_move_pages_to_node(struct
        };
 
        err = migrate_pages(pagelist, alloc_migration_target, NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL);
+                       (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL,
+                       &nr_succeeded);
        if (err)
                putback_movable_pages(pagelist);
        return err;

@@ -2207,6 +2209,7 @@ int migrate_misplaced_page(struct page *
        pg_data_t *pgdat = NODE_DATA(node);
        int isolated;
        int nr_remaining;
+       unsigned int nr_succeeded = 0;
        LIST_HEAD(migratepages);
 
        /*

@@ -2230,7 +2233,7 @@ int migrate_misplaced_page(struct page *
        list_add(&page->lru, &migratepages);
        nr_remaining = migrate_pages(&migratepages,
 alloc_misplaced_dst_page,
                                     NULL, node, MIGRATE_ASYNC,
-                                    MR_NUMA_MISPLACED);
+                                    MR_NUMA_MISPLACED, &nr_succeeded);
        if (nr_remaining) {
                if (!list_empty(&migratepages)) {

                        list_del(&page->lru);

diff -puN mm/page_alloc.c~migrate_pages-add-success-return mm/page_alloc.c --- a/mm/page_alloc.c~migrate_pages-add-success-return 2021-03-31 15:17:14.178000255 -0700
+++ b/mm/page_alloc.c 2021-03-31 15:17:14.213000255 -0700

mm/page_alloc.c renamed from mm/page_alloc.c~migrate_pages-add-success-return +2 -1

@@ -8452,7 +8452,8 @@ static unsigned long pfn_max_align_up(un
 
 /* [start, end) must belong to a single zone. */
 static int __alloc_contig_migrate_range(struct compact_control *cc,
-                                       unsigned long start, unsigned long
 end)
+                                       unsigned long start, unsigned long
 end,
+                                       unsigned int *nr_succeeded)
 {

        /* This function is based on compact_zone() from compaction.c. */
        unsigned int nr_reclaimed;

(quoted diff) +2 -1

@@ -8490,7 +8491,8 @@ static int __alloc_contig_migrate_range(
                cc->nr_migratepages -= nr_reclaimed;
 
                ret = migrate_pages(&cc->migratepages,
 alloc_migration_target,
-                               NULL, (unsigned long)&mtc, cc->mode,
 MR_CONTIG_RANGE);
+                               NULL, (unsigned long)&mtc, cc->mode,
 MR_CONTIG_RANGE,
+                               nr_succeeded);

        }
        if (ret < 0) {
                putback_movable_pages(&cc->migratepages);

mm/page_alloc.c renamed from mm/page_alloc.c~migrate_pages-add-success-return +2 -1

@@ -8526,6 +8528,7 @@ int alloc_contig_range(unsigned long sta
        unsigned long outer_start, outer_end;
        unsigned int order;
        int ret = 0;
+       unsigned int nr_succeeded = 0;
 
        struct compact_control cc = {
                .nr_migratepages = 0,

@@ -8580,7 +8583,7 @@ int alloc_contig_range(unsigned long sta
         * allocated.  So, if we fall through be sure to clear ret so that
         * -EBUSY is not accidentally used or returned to caller.
         */
-       ret = __alloc_contig_migrate_range(&cc, start, end);
+       ret = __alloc_contig_migrate_range(&cc, start, end, &nr_succeeded);
        if (ret && ret != -EBUSY)
                goto done;
        ret =0;

_

A small suggestion: Given that migrate_pages() requires that *nr_succeeded should be initialized to 0 when it is called due to its use of *nr_succeeded in count_vm_events() and trace_mm_migrate_pages(), it would be less error-prone if migrate_pages() initializes *nr_succeeded itself.

From: Yang Shi <[email protected]>

The migrate_pages() returns the number of pages that were not migrated, or an error code. When returning an error code, there is no way to know how many pages were migrated or not migrated.

In the following patch, migrate_pages() is used to demote pages to PMEM node, we need account how many pages are reclaimed (demoted) since page reclaim behavior depends on this. Add *nr_succeeded parameter to make migrate_pages() return how many pages are demoted successfully for all cases.

Signed-off-by Yang Shi <[email protected]>

Signed-off-by Dave Hansen <[email protected]>

Reviewed-by Yang Shi <[email protected]>

Cc Wei Xu <[email protected]>

Cc Huang Ying <[email protected]>

Cc Dan Williams <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc osalvador <[email protected]>

--

Note: Yang Shi originally wrote the patch, thus the SoB. There was also a Reviewed-by provided since there were some modifications made to this after the original work.

Changes since 20210302:
* Fix definition of CONFIG_MIGRATION=n stub migrate_pages(). Its parameters were wrong, but oddly enough did not generate any compile errors.

Changes since 20200122:
* Fix migrate_pages() to manipulate nr_succeeded *value* rather than the pointer.
---

 b/include/linux/migrate.h |    5 +++--
 b/mm/compaction.c         |    3 ++-
 b/mm/gup.c                |    3 ++-
 b/mm/memory-failure.c     |    4 +++-
 b/mm/memory_hotplug.c     |    4 +++-
 b/mm/mempolicy.c          |    8 ++++++--
 b/mm/migrate.c            |   19 +++++++++++--------
 b/mm/page_alloc.c         |    9 ++++++---
 8 files changed, 36 insertions(+), 19 deletions(-)

diff -puN include/linux/migrate.h~migrate_pages-add-success-return include/linux/migrate.h

include/linux/migrate.h 2021-03-31 15:17:14.182000255 -0700 renamed from include/linux/migrate.h~migrate_pages-add-success-return 2021-03-31 15:17:14.144000255 -0700 +3 -2

@@ -40,7 +40,8 @@ extern int migrate_page(struct address_s
                        struct page *newpage, struct page *page,
                        enum migrate_mode mode);
 extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
-               unsigned long private, enum migrate_mode mode, int reason);
+               unsigned long private, enum migrate_mode mode, int reason,
+               unsigned int *nr_succeeded);
 extern struct page *alloc_migration_target(struct page *page, unsigned long private);
 extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
 extern void putback_movable_page(struct page *page);

@@ -58,7 +59,7 @@ extern int migrate_page_move_mapping(str
 static inline void putback_movable_pages(struct list_head *l) {}
 static inline int migrate_pages(struct list_head *l, new_page_t new,
                free_page_t free, unsigned long private, enum migrate_mode mode,
-               int reason)
+               int reason, unsigned int *nr_succeeded)
        { return -ENOSYS; }
 static inline struct page *alloc_migration_target(struct page *page,
                unsigned long private)

diff -puN mm/compaction.c~migrate_pages-add-success-return mm/compaction.c

mm/compaction.c 2021-03-31 15:17:14.186000255 -0700 renamed from mm/compaction.c~migrate_pages-add-success-return 2021-03-31 15:17:14.146000255 -0700 +2 -1

@@ -2247,6 +2247,7 @@ compact_zone(struct compact_control *cc,
        unsigned long last_migrated_pfn;
        const bool sync = cc->mode != MIGRATE_ASYNC;
        bool update_cached;
+       unsigned int nr_succeeded = 0;
 
        /*
         * These counters track activities during zone compaction.  Initialize

@@ -2364,7 +2365,7 @@ compact_zone(struct compact_control *cc,
 
                err = migrate_pages(&cc->migratepages, compaction_alloc,
                                compaction_free, (unsigned long)cc, cc->mode,
-                               MR_COMPACTION);
+                               MR_COMPACTION, &nr_succeeded);
 
                trace_mm_compaction_migratepages(cc->nr_migratepages, err,
                                                        &cc->migratepages);

diff -puN mm/gup.c~migrate_pages-add-success-return mm/gup.c

mm/gup.c 2021-03-31 15:17:14.190000255 -0700 renamed from mm/gup.c~migrate_pages-add-success-return 2021-03-31 15:17:14.150000255 -0700 +2 -1

@@ -1550,6 +1550,7 @@ static long check_and_migrate_cma_pages(
        unsigned long i;
        unsigned long step;
        bool drain_allow = true;
+       unsigned int nr_succeeded = 0;
        bool migrate_allow = true;
        LIST_HEAD(cma_page_list);
        long ret = nr_pages;

@@ -1606,7 +1607,7 @@ check_again:
                                put_page(pages[i]);
 
                if (migrate_pages(&cma_page_list, alloc_migration_target, NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) {
+                       (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE, &nr_succeeded)) {
                        /*
                         * some of the pages failed migration. Do get_user_pages
                         * without migration.

diff -puN mm/memory-failure.c~migrate_pages-add-success-return mm/memory-failure.c

mm/memory-failure.c 2021-03-31 15:17:14.194000255 -0700 renamed from mm/memory-failure.c~migrate_pages-add-success-return 2021-03-31 15:17:14.155000255 -0700 +3 -1

@@ -1809,6 +1809,7 @@ static int __soft_offline_page(struct pa
        unsigned long pfn = page_to_pfn(page);
        struct page *hpage = compound_head(page);
        char const *msg_page[] = {"page", "hugepage"};
+       unsigned int nr_succeeded = 0;
        bool huge = PageHuge(page);
        LIST_HEAD(pagelist);
        struct migration_target_control mtc = {

@@ -1852,7 +1853,8 @@ static int __soft_offline_page(struct pa
 
        if (isolate_page(hpage, &pagelist)) {
                ret = migrate_pages(&pagelist, alloc_migration_target, NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE);
+                       (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE,
+                       &nr_succeeded);
                if (!ret) {
                        bool release = !huge;
 

diff -puN mm/memory_hotplug.c~migrate_pages-add-success-return mm/memory_hotplug.c

mm/memory_hotplug.c 2021-03-31 15:17:14.197000255 -0700 renamed from mm/memory_hotplug.c~migrate_pages-add-success-return 2021-03-31 15:17:14.160000255 -0700 +3 -1

@@ -1392,6 +1392,7 @@ do_migrate_range(unsigned long start_pfn
        unsigned long pfn;
        struct page *page, *head;
        int ret = 0;
+       unsigned int nr_succeeded = 0;
        LIST_HEAD(source);
 
        for (pfn = start_pfn; pfn < end_pfn; pfn++) {

@@ -1466,7 +1467,8 @@ do_migrate_range(unsigned long start_pfn
                if (nodes_empty(nmask))
                        node_set(mtc.nid, nmask);
                ret = migrate_pages(&source, alloc_migration_target, NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
+                       (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG,
+                       &nr_succeeded);
                if (ret) {
                        list_for_each_entry(page, &source, lru) {
                                pr_warn("migrating pfn %lx failed ret:%d ",

diff -puN mm/mempolicy.c~migrate_pages-add-success-return mm/mempolicy.c

mm/mempolicy.c 2021-03-31 15:17:14.203000255 -0700 renamed from mm/mempolicy.c~migrate_pages-add-success-return 2021-03-31 15:17:14.163000255 -0700 +6 -2

@@ -1081,6 +1081,7 @@ static int migrate_page_add(struct page
 static int migrate_to_node(struct mm_struct *mm, int source, int dest,
                           int flags)
 {
+       unsigned int nr_succeeded = 0;
        nodemask_t nmask;
        LIST_HEAD(pagelist);
        int err = 0;

@@ -1103,7 +1104,8 @@ static int migrate_to_node(struct mm_str
 
        if (!list_empty(&pagelist)) {
                err = migrate_pages(&pagelist, alloc_migration_target, NULL,
-                               (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL);
+                               (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL,
+                               &nr_succeeded);
                if (err)
                        putback_movable_pages(&pagelist);
        }

@@ -1278,6 +1280,7 @@ static long do_mbind(unsigned long start
                     nodemask_t *nmask, unsigned long flags)
 {
        struct mm_struct *mm = current->mm;
+       unsigned int nr_succeeded = 0;
        struct mempolicy *new;
        unsigned long end;
        int err;

@@ -1355,7 +1358,8 @@ static long do_mbind(unsigned long start
                if (!list_empty(&pagelist)) {
                        WARN_ON_ONCE(flags & MPOL_MF_LAZY);
                        nr_failed = migrate_pages(&pagelist, new_page, NULL,
-                               start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND);
+                               start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND,
+                               &nr_succeeded);
                        if (nr_failed)
                                putback_movable_pages(&pagelist);
                }

diff -puN mm/migrate.c~migrate_pages-add-success-return mm/migrate.c

mm/migrate.c 2021-03-31 15:17:14.207000255 -0700 renamed from mm/migrate.c~migrate_pages-add-success-return 2021-03-31 15:17:14.168000255 -0700 +11 -8

@@ -1493,6 +1493,7 @@ static inline int try_split_thp(struct p
  * @mode:              The migration mode that specifies the constraints for
  *                     page migration, if any.
  * @reason:            The reason for page migration.
+ * @nr_succeeded:      The number of pages migrated successfully.
  *
  * The function returns after 10 attempts or if no pages are movable any more
  * because the list has become empty or no retryable pages exist any more.

@@ -1503,12 +1504,11 @@ static inline int try_split_thp(struct p
  */
 int migrate_pages(struct list_head *from, new_page_t get_new_page,
                free_page_t put_new_page, unsigned long private,
-               enum migrate_mode mode, int reason)
+               enum migrate_mode mode, int reason, unsigned int *nr_succeeded)
 {
        int retry = 1;
        int thp_retry = 1;
        int nr_failed = 0;
-       int nr_succeeded = 0;
        int nr_thp_succeeded = 0;
        int nr_thp_failed = 0;
        int nr_thp_split = 0;

@@ -1611,10 +1611,10 @@ retry:
                        case MIGRATEPAGE_SUCCESS:
                                if (is_thp) {
                                        nr_thp_succeeded++;
-                                       nr_succeeded += nr_subpages;
+                                       *nr_succeeded += nr_subpages;
                                        break;
                                }
-                               nr_succeeded++;
+                               (*nr_succeeded)++;
                                break;
                        default:
                                /*

@@ -1643,12 +1643,12 @@ out:
         */
        list_splice(&ret_pages, from);
 
-       count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
+       count_vm_events(PGMIGRATE_SUCCESS, *nr_succeeded);
        count_vm_events(PGMIGRATE_FAIL, nr_failed);
        count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
        count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
        count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
-       trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
+       trace_mm_migrate_pages(*nr_succeeded, nr_failed, nr_thp_succeeded,
                               nr_thp_failed, nr_thp_split, mode, reason);
 
        if (!swapwrite)

@@ -1716,6 +1716,7 @@ static int store_status(int __user *stat
 static int do_move_pages_to_node(struct mm_struct *mm,
                struct list_head *pagelist, int node)
 {
+       unsigned int nr_succeeded = 0;
        int err;
        struct migration_target_control mtc = {
                .nid = node,

@@ -1723,7 +1724,8 @@ static int do_move_pages_to_node(struct
        };
 
        err = migrate_pages(pagelist, alloc_migration_target, NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL);
+                       (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL,
+                       &nr_succeeded);
        if (err)
                putback_movable_pages(pagelist);
        return err;

@@ -2207,6 +2209,7 @@ int migrate_misplaced_page(struct page *
        pg_data_t *pgdat = NODE_DATA(node);
        int isolated;
        int nr_remaining;
+       unsigned int nr_succeeded = 0;
        LIST_HEAD(migratepages);
 
        /*

@@ -2230,7 +2233,7 @@ int migrate_misplaced_page(struct page *
        list_add(&page->lru, &migratepages);
        nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page,
                                     NULL, node, MIGRATE_ASYNC,
-                                    MR_NUMA_MISPLACED);
+                                    MR_NUMA_MISPLACED, &nr_succeeded);
        if (nr_remaining) {
                if (!list_empty(&migratepages)) {
                        list_del(&page->lru);

diff -puN mm/page_alloc.c~migrate_pages-add-success-return mm/page_alloc.c

mm/page_alloc.c 2021-03-31 15:17:14.213000255 -0700 renamed from mm/page_alloc.c~migrate_pages-add-success-return 2021-03-31 15:17:14.178000255 -0700 +6 -3

@@ -8452,7 +8452,8 @@ static unsigned long pfn_max_align_up(un
 
 /* [start, end) must belong to a single zone. */
 static int __alloc_contig_migrate_range(struct compact_control *cc,
-                                       unsigned long start, unsigned long end)
+                                       unsigned long start, unsigned long end,
+                                       unsigned int *nr_succeeded)
 {
        /* This function is based on compact_zone() from compaction.c. */
        unsigned int nr_reclaimed;

@@ -8490,7 +8491,8 @@ static int __alloc_contig_migrate_range(
                cc->nr_migratepages -= nr_reclaimed;
 
                ret = migrate_pages(&cc->migratepages, alloc_migration_target,
-                               NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE);
+                               NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE,
+                               nr_succeeded);
        }
        if (ret < 0) {
                putback_movable_pages(&cc->migratepages);

@@ -8526,6 +8528,7 @@ int alloc_contig_range(unsigned long sta
        unsigned long outer_start, outer_end;
        unsigned int order;
        int ret = 0;
+       unsigned int nr_succeeded = 0;
 
        struct compact_control cc = {
                .nr_migratepages = 0,

@@ -8580,7 +8583,7 @@ int alloc_contig_range(unsigned long sta
         * allocated.  So, if we fall through be sure to clear ret so that
         * -EBUSY is not accidentally used or returned to caller.
         */
-       ret = __alloc_contig_migrate_range(&cc, start, end);
+       ret = __alloc_contig_migrate_range(&cc, start, end, &nr_succeeded);
        if (ret && ret != -EBUSY)
                goto done;
        ret =0;

_

From: Yang Shi <[email protected]>

The migrate_pages() returns the number of pages that were not migrated, or an error code. When returning an error code, there is no way to know how many pages were migrated or not migrated.

In the following patch, migrate_pages() is used to demote pages to PMEM node, we need account how many pages are reclaimed (demoted) since page reclaim behavior depends on this. Add *nr_succeeded parameter to make migrate_pages() return how many pages are demoted successfully for all cases.

Signed-off-by Yang Shi <[email protected]>

Signed-off-by Dave Hansen <[email protected]>

Reviewed-by Yang Shi <[email protected]>

Cc Wei Xu <[email protected]>

Cc Huang Ying <[email protected]>

Cc Dan Williams <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc osalvador <[email protected]>

int migrate_pages(struct list_head *from, new_page_t get_new_page,

(quoted diff) +3 -4

 		free_page_t put_new_page, unsigned long private,
-		enum migrate_mode mode, int reason)
+		enum migrate_mode mode, int reason, unsigned int *nr_succeeded)
 {
 	int retry = 1;
 	int thp_retry = 1;
 	int nr_failed = 0;
-	int nr_succeeded = 0;
 	int nr_thp_succeeded = 0;
 	int nr_thp_failed = 0;
 	int nr_thp_split = 0;

@@ -1611,10 +1611,10 @@ retry:
 			case MIGRATEPAGE_SUCCESS:
 				if (is_thp) {
 					nr_thp_succeeded++;
-					nr_succeeded += nr_subpages;
+					*nr_succeeded += nr_subpages;
 					break;
 				}
-				nr_succeeded++;
+				(*nr_succeeded)++;
 				break;
 			default:
 				/*

@@ -1643,12 +1643,12 @@ out:
 	 */
 	list_splice(&ret_pages, from);

Quoted 22 lines · Dave Hansen

From: Yang Shi <[email protected]>

The migrate_pages() returns the number of pages that were not migrated, or an error code. When returning an error code, there is no way to know how many pages were migrated or not migrated.

In the following patch, migrate_pages() is used to demote pages to PMEM node, we need account how many pages are reclaimed (demoted) since page reclaim behavior depends on this. Add *nr_succeeded parameter to make migrate_pages() return how many pages are demoted successfully for all cases.

Signed-off-by Yang Shi <[email protected]>

Signed-off-by Dave Hansen <[email protected]>

Reviewed-by Yang Shi <[email protected]>

Cc Wei Xu <[email protected]>

Cc Huang Ying <[email protected]>

Cc Dan Williams <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc osalvador <[email protected]>

...

Quoted 28 lines

int migrate_pages(struct list_head *from, new_page_t get_new_page,
free_page_t put_new_page, unsigned long private,

(quoted diff) +3 -4

-             enum migrate_mode mode, int reason)
+             enum migrate_mode mode, int reason, unsigned int *nr_succeeded)
 {
      int retry = 1;
      int thp_retry = 1;
      int nr_failed = 0;
-     int nr_succeeded = 0;
      int nr_thp_succeeded = 0;
      int nr_thp_failed = 0;
      int nr_thp_split = 0;

@@ -1611,10 +1611,10 @@ retry:
                      case MIGRATEPAGE_SUCCESS:
                              if (is_thp) {
                                      nr_thp_succeeded++;
-                                     nr_succeeded += nr_subpages;
+                                     *nr_succeeded += nr_subpages;
                                      break;
                              }
-                             nr_succeeded++;
+                             (*nr_succeeded)++;
                              break;
                      default:
                              /*

@@ -1643,12 +1643,12 @@ out:
       */
      list_splice(&ret_pages, from);

Review out: · line 1652

Quoted 10 lines

(quoted diff) +2 -2

 
-     count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
+     count_vm_events(PGMIGRATE_SUCCESS, *nr_succeeded);
      count_vm_events(PGMIGRATE_FAIL, nr_failed);
      count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
      count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
      count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
-     trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
+     trace_mm_migrate_pages(*nr_succeeded, nr_failed, nr_thp_succeeded,
                             nr_thp_failed, nr_thp_split, mode, reason);

It seems that reclaiming is the only user who cared about how many pages could we migrated, could not do the following instead:

mm/migrate.c +4 -1

index 695a594e5860..d4170b7ea2fe 100644
@@ -1503,7 +1503,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
  */
 int migrate_pages(struct list_head *from, new_page_t get_new_page,
                free_page_t put_new_page, unsigned long private,
-               enum migrate_mode mode, int reason)
+               enum migrate_mode mode, int reason, unsigned int *ret_succeeded)
 {
        int retry = 1;
        int thp_retry = 1;

@@ -1654,6 +1654,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
        if (!swapwrite)
                current->flags &= ~PF_SWAPWRITE;
 
+       if (ret_succedded)
+               *ret_succedded = nr_succedded;
+
        return rc;
 }
 

And pass only a valid pointer from demote_page_list() and NULL from all the others?
I was just wondered after all those "unsigned int nr_succedded" in all other functions.
This would also solve the "be careful to initialize nr_succedded" problem?

--
Oscar Salvador
SUSE L3

Thanks, Oscar. Yes, kind of. But we have to remember to initialize "nr_succedded" pointer properly for every migrate_pages() callsite, right? And it doesn't prevent from returning wrong value if migrate_pages() is called multiple times by one caller although there might be not such case (calls migrate_pages() multiple times and care about nr_succeded) for now.

include/linux/migrate.h +3 -2

index 3a389633b68f..fd661cb2ce13 100644
@@ -40,7 +40,8 @@ extern int migrate_page(struct address_space *mapping,
 			struct page *newpage, struct page *page,
 			enum migrate_mode mode);
 extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
-		unsigned long private, enum migrate_mode mode, int reason);
+		unsigned long private, enum migrate_mode mode, int reason,
+		unsigned int *ret_succeeded);
 extern struct page *alloc_migration_target(struct page *page, unsigned long private);
 extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
 extern void putback_movable_page(struct page *page);

@@ -58,7 +59,7 @@ extern int migrate_page_move_mapping(struct address_space *mapping,
 static inline void putback_movable_pages(struct list_head *l) {}
 static inline int migrate_pages(struct list_head *l, new_page_t new,
 		free_page_t free, unsigned long private, enum migrate_mode mode,
-		int reason)
+		int reason, unsigned int *ret_succeeded)
 	{ return -ENOSYS; }
 static inline struct page *alloc_migration_target(struct page *page,
 		unsigned long private)

mm/compaction.c +1 -1

index e04f4476e68e..7238e8faff04 100644
@@ -2364,7 +2364,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
 
 		err = migrate_pages(&cc->migratepages, compaction_alloc,
 				compaction_free, (unsigned long)cc, cc->mode,
-				MR_COMPACTION);
+				MR_COMPACTION, NULL);
 
 		trace_mm_compaction_migratepages(cc->nr_migratepages, err,
 							&cc->migratepages);

mm/gup.c +1 -1

index e40579624f10..b70d463aa1fc 100644
@@ -1606,7 +1606,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm,
 				put_page(pages[i]);
 
 		if (migrate_pages(&cma_page_list, alloc_migration_target, NULL,
-			(unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) {
+			(unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE, NULL)) {
 			/*
 			 * some of the pages failed migration. Do get_user_pages
 			 * without migration.

mm/memory-failure.c +2 -1

index 24210c9bd843..a17e0f039076 100644
@@ -1852,7 +1852,8 @@ static int __soft_offline_page(struct page *page)
 
 	if (isolate_page(hpage, &pagelist)) {
 		ret = migrate_pages(&pagelist, alloc_migration_target, NULL,
-			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE);
+			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE,
+			NULL);
 		if (!ret) {
 			bool release = !huge;
 

mm/memory_hotplug.c +2 -1

index 0cdbbfbc5757..28496376de94 100644
@@ -1466,7 +1466,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		if (nodes_empty(nmask))
 			node_set(mtc.nid, nmask);
 		ret = migrate_pages(&source, alloc_migration_target, NULL,
-			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
+			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG,
+			NULL);
 		if (ret) {
 			list_for_each_entry(page, &source, lru) {
 				pr_warn("migrating pfn %lx failed ret:%d ",

mm/mempolicy.c +4 -2

index ab51132547b8..df260ed12102 100644
@@ -1103,7 +1103,8 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
 
 	if (!list_empty(&pagelist)) {
 		err = migrate_pages(&pagelist, alloc_migration_target, NULL,
-				(unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL);
+				(unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL,
+				NULL);
 		if (err)
 			putback_movable_pages(&pagelist);
 	}

@@ -1355,7 +1356,8 @@ static long do_mbind(unsigned long start, unsigned long len,
 		if (!list_empty(&pagelist)) {
 			WARN_ON_ONCE(flags & MPOL_MF_LAZY);
 			nr_failed = migrate_pages(&pagelist, new_page, NULL,
-				start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND);
+				start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND,
+				NULL);
 			if (nr_failed)
 				putback_movable_pages(&pagelist);
 		}

mm/migrate.c +10 -3

index 695a594e5860..087ed407b3ce 100644
@@ -1493,6 +1493,9 @@ static inline int try_split_thp(struct page *page, struct page **page2,
  * @mode:		The migration mode that specifies the constraints for
  *			page migration, if any.
  * @reason:		The reason for page migration.
+ * @ret_succeeded:	A pointer to place the value of the number of pages
+ *                      migrated successfully. The caller must pass a valid pointer
+ *                      if they care about it.
  *
  * The function returns after 10 attempts or if no pages are movable any more
  * because the list has become empty or no retryable pages exist any more.

@@ -1503,7 +1506,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
  */
 int migrate_pages(struct list_head *from, new_page_t get_new_page,
 		free_page_t put_new_page, unsigned long private,
-		enum migrate_mode mode, int reason)
+		enum migrate_mode mode, int reason, unsigned int *ret_succeeded)
 {
 	int retry = 1;
 	int thp_retry = 1;

@@ -1654,6 +1657,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 	if (!swapwrite)
 		current->flags &= ~PF_SWAPWRITE;
 
+	if (ret_succeeded)
+		*ret_succeeded = nr_succeeded;
+
 	return rc;
 }
 

@@ -1723,7 +1729,8 @@ static int do_move_pages_to_node(struct mm_struct *mm,
 	};
 
 	err = migrate_pages(pagelist, alloc_migration_target, NULL,
-			(unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL);
+			(unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL,
+			NULL);
 	if (err)
 		putback_movable_pages(pagelist);
 	return err;

@@ -2230,7 +2237,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
 	list_add(&page->lru, &migratepages);
 	nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page,
 				     NULL, node, MIGRATE_ASYNC,
-				     MR_NUMA_MISPLACED);
+				     MR_NUMA_MISPLACED, NULL);
 	if (nr_remaining) {
 		if (!list_empty(&migratepages)) {
 			list_del(&page->lru);

mm/page_alloc.c +0 -0

index 46f3d594369d..0c1bbadd5ca3 100644

Quoted 6 lines · Yang Shi

Thanks, Oscar. Yes, kind of. But we have to remember to initialize "nr_succedded" pointer properly for every migrate_pages() callsite, right? And it doesn't prevent from returning wrong value if migrate_pages() is called multiple times by one caller although there might be not such case (calls migrate_pages() multiple times and care about nr_succeded) for now.

Hi Yang,

I might be missing something but AFAICS you only need to initialize nr_succeded pointer where it matters.
The local nr_succeeded in migrate_pages() doesn't go, and so it gets initialized every time you call in it to 0.
And if you pass a valid pointer, *ret_succeeded == nr_succedeed.

I am talking about this (not even compile-tested):

include/linux/migrate.h +3 -2

index 3a389633b68f..fd661cb2ce13 100644
@@ -40,7 +40,8 @@ extern int migrate_page(struct address_space *mapping,
                        struct page *newpage, struct page *page,
                        enum migrate_mode mode);
 extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
-               unsigned long private, enum migrate_mode mode, int reason);
+               unsigned long private, enum migrate_mode mode, int reason,
+               unsigned int *ret_succeeded);
 extern struct page *alloc_migration_target(struct page *page, unsigned long private);
 extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
 extern void putback_movable_page(struct page *page);

@@ -58,7 +59,7 @@ extern int migrate_page_move_mapping(struct address_space *mapping,
 static inline void putback_movable_pages(struct list_head *l) {}
 static inline int migrate_pages(struct list_head *l, new_page_t new,
                free_page_t free, unsigned long private, enum migrate_mode mode,
-               int reason)
+               int reason, unsigned int *ret_succeeded)
        { return -ENOSYS; }
 static inline struct page *alloc_migration_target(struct page *page,
                unsigned long private)

mm/compaction.c +1 -1

index e04f4476e68e..7238e8faff04 100644
@@ -2364,7 +2364,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
 
                err = migrate_pages(&cc->migratepages, compaction_alloc,
                                compaction_free, (unsigned long)cc, cc->mode,
-                               MR_COMPACTION);
+                               MR_COMPACTION, NULL);
 
                trace_mm_compaction_migratepages(cc->nr_migratepages, err,
                                                        &cc->migratepages);

mm/gup.c +1 -1

index e40579624f10..b70d463aa1fc 100644
@@ -1606,7 +1606,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm,
                                put_page(pages[i]);
 
                if (migrate_pages(&cma_page_list, alloc_migration_target, NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) {
+                       (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE, NULL)) {
                        /*
                         * some of the pages failed migration. Do get_user_pages
                         * without migration.

mm/memory-failure.c +2 -1

index 24210c9bd843..a17e0f039076 100644
@@ -1852,7 +1852,8 @@ static int __soft_offline_page(struct page *page)
 
        if (isolate_page(hpage, &pagelist)) {
                ret = migrate_pages(&pagelist, alloc_migration_target, NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE);
+                       (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE,
+                       NULL);
                if (!ret) {
                        bool release = !huge;
 

mm/memory_hotplug.c +2 -1

index 0cdbbfbc5757..28496376de94 100644
@@ -1466,7 +1466,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
                if (nodes_empty(nmask))
                        node_set(mtc.nid, nmask);
                ret = migrate_pages(&source, alloc_migration_target, NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
+                       (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG,
+                       NULL);
                if (ret) {
                        list_for_each_entry(page, &source, lru) {
                                pr_warn("migrating pfn %lx failed ret:%d ",

mm/mempolicy.c +4 -2

index ab51132547b8..df260ed12102 100644
@@ -1103,7 +1103,8 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
 
        if (!list_empty(&pagelist)) {
                err = migrate_pages(&pagelist, alloc_migration_target, NULL,
-                               (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL);
+                               (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL,
+                               NULL);
                if (err)
                        putback_movable_pages(&pagelist);
        }

@@ -1355,7 +1356,8 @@ static long do_mbind(unsigned long start, unsigned long len,
                if (!list_empty(&pagelist)) {
                        WARN_ON_ONCE(flags & MPOL_MF_LAZY);
                        nr_failed = migrate_pages(&pagelist, new_page, NULL,
-                               start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND);
+                               start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND,
+                               NULL);
                        if (nr_failed)
                                putback_movable_pages(&pagelist);
                }

mm/migrate.c +4 -1

index 695a594e5860..087ed407b3ce 100644
@@ -1493,6 +1493,9 @@ static inline int try_split_thp(struct page *page, struct page **page2,
  * @mode:              The migration mode that specifies the constraints for
  *                     page migration, if any.
  * @reason:            The reason for page migration.
+ * @ret_succeeded:     A pointer to place the value of the number of pages
+ *                      migrated successfully. The caller must pass a valid pointer
+ *                      if they care about it.
  *
  * The function returns after 10 attempts or if no pages are movable any more
  * because the list has become empty or no retryable pages exist any more.

@@ -1503,7 +1506,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
  */
 int migrate_pages(struct list_head *from, new_page_t get_new_page,
                free_page_t put_new_page, unsigned long private,
-               enum migrate_mode mode, int reason)
+               enum migrate_mode mode, int reason, unsigned int *ret_succeeded)
 {
        int retry = 1;
        int thp_retry = 1;

mm/migrate.c +3 -2

        return rc;
 }
 

@@ -1723,7 +1729,8 @@ static int do_move_pages_to_node(struct mm_struct *mm,
        };
 
        err = migrate_pages(pagelist, alloc_migration_target, NULL,
-                       (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL);
+                       (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL,
+                       NULL);
        if (err)
                putback_movable_pages(pagelist);
        return err;

@@ -2230,7 +2237,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
        list_add(&page->lru, &migratepages);
        nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page,
                                     NULL, node, MIGRATE_ASYNC,
-                                    MR_NUMA_MISPLACED);
+                                    MR_NUMA_MISPLACED, NULL);
        if (nr_remaining) {
                if (!list_empty(&migratepages)) {
                        list_del(&page->lru);

mm/page_alloc.c +2 -1

index 46f3d594369d..0c1bbadd5ca3 100644
@@ -8490,7 +8490,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
                cc->nr_migratepages -= nr_reclaimed;
 
                ret = migrate_pages(&cc->migratepages, alloc_migration_target,
-                               NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE);
+                               NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE,
+                               NULL);
        }
        if (ret < 0) {
                putback_movable_pages(&cc->migratepages);

As I said I might be missing a point here, but I cannot see the problem you describe here.

--
Oscar Salvador
SUSE L3

Thanks a lot for the example code. You didn't miss anything. At first glance, I thought your suggestion seemed neater. Actually I misunderstood what Dave said about "That could really have caused some interesting problems." with multiple calls to migrate_pages(). I was thinking about:

unsigned long foo()

{
    unsigned long *ret_succeeded;

    migrate_pages(..., ret_succeeded);

    migrate_pages(..., ret_succeeded);

    return *ret_succeeded;
}

Quoted 7 lines · Yang Shi

Thanks a lot for the example code. You didn't miss anything. At first glance, I thought your suggestion seemed neater. Actually I misunderstood what Dave said about "That could really have caused some interesting problems." with multiple calls to migrate_pages(). I was thinking about:

unsigned long foo()

Quoted 9 lines

{
    unsigned long *ret_succeeded;

    migrate_pages(..., ret_succeeded);

    migrate_pages(..., ret_succeeded);

    return *ret_succeeded;
}

But that would not be a problem as well. I mean I am not sure what is foo() supposed to do.
I assume is supposed to return the *total* number of pages that were migrated?

Then could do something like:

unsigned long foo()

 {
     unsigned long ret_succeeded;
     unsigned long total_succeeded = 0;

     migrate_pages(..., &ret_succeeded);
     total_succeeded += ret_succeeded;

     migrate_pages(..., &ret_succeeded);
     total_succeeded += ret_succeeded;

     return *total_succeeded;

}

But AFAICS, you would have to do that with Wei Xu's version and with mine, no difference there.

IIUC, Dave's concern was that nr_succeeded was only set to 0 at the beginning of the function, and never reset back, which means, we would carry the sum of previous nr_succeeded instead of the nr_succeeded in that round. That would be misleading for e.g: reclaim in case we were to call migrate_pages() several times, as instead of a delta value, nr_succeeded would accumulate.

But that won't happen neither with Wei Xu's version nor with mine.

--
Oscar Salvador
SUSE L3

Quoted 7 lines · Yang Shi

Thanks a lot for the example code. You didn't miss anything. At first glance, I thought your suggestion seemed neater. Actually I misunderstood what Dave said about "That could really have caused some interesting problems." with multiple calls to migrate_pages(). I was thinking about:

unsigned long foo()

Quoted 9 lines

{
    unsigned long *ret_succeeded;

    migrate_pages(..., ret_succeeded);

    migrate_pages(..., ret_succeeded);

    return *ret_succeeded;
}

But that would not be a problem as well. I mean I am not sure what is foo() supposed to do.
I assume is supposed to return the *total* number of pages that were migrated?

Then could do something like:

unsigned long foo()

 {
     unsigned long ret_succeeded;
     unsigned long total_succeeded = 0;

     migrate_pages(..., &ret_succeeded);
     total_succeeded += ret_succeeded;

     migrate_pages(..., &ret_succeeded);
     total_succeeded += ret_succeeded;

     return *total_succeeded;

}

But AFAICS, you would have to do that with Wei Xu's version and with mine, no difference there.

IIUC, Dave's concern was that nr_succeeded was only set to 0 at the beginning of the function, and never reset back, which means, we would carry the sum of previous nr_succeeded instead of the nr_succeeded in that round. That would be misleading for e.g: reclaim in case we were to call migrate_pages() several times, as instead of a delta value, nr_succeeded would accumulate.

But that won't happen neither with Wei Xu's version nor with mine.

--
Oscar Salvador
SUSE L3

I also considered passing NULL to mean "I don't care about nr_succeeded". I mostly avoided it to reduce churn. But, looking at it here, it does seem cleaner.

Any objections to moving over to Oscar's suggestion?

mm/page_alloc.c static int __alloc_contig_migrate_range(struct compact_control *cc, · line 8497

Quoted 12 lines · Oscar Salvador

mm/page_alloc.c +2 -1

@@ -8490,7 +8490,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
              cc->nr_migratepages -= nr_reclaimed;
 
              ret = migrate_pages(&cc->migratepages, alloc_migration_target,
-                             NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE);
+                             NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE,
+                             NULL);
      }
      if (ret < 0) {
              putback_movable_pages(&cc->migratepages);

I also considered passing NULL to mean "I don't care about nr_succeeded". I mostly avoided it to reduce churn. But, looking at it here, it does seem cleaner.

Any objections to moving over to Oscar's suggestion?

From: Dave Hansen <[email protected]>

This is mostly derived from a patch from Yang Shi:

        https://lore.kernel.org/linux-mm/[email protected]/

Add code to the reclaim path (shrink_page_list()) to "demote" data to another NUMA node instead of discarding the data. This always avoids the cost of I/O needed to read the page back in and sometimes avoids the writeout cost when the pagee is dirty.

A second pass through shrink_page_list() will be made if any demotions fail. This essentally falls back to normal reclaim behavior in the

case that demotions fail. Previous versions of this patch may have simply failed to reclaim pages which were eligible for demotion but were unable to be demoted in practice.

Note: This just adds the start of infratructure for migration. It is

actually disabled next to the FIXME in migrate_demote_page_ok().

Signed-off-by Dave Hansen <[email protected]>

Cc Wei Xu <[email protected]>

Cc Yang Shi <[email protected]>

Cc David Rientjes <[email protected]>

Cc Huang Ying <[email protected]>

Cc Dan Williams <[email protected]>

Cc osalvador <[email protected]>

--
changes from 20210122:
* move from GFP_HIGHUSER -> GFP_HIGHUSER_MOVABLE (Ying)

changes from 202010:
* add MR_NUMA_MISPLACED to trace MIGRATE_REASON define
* make migrate_demote_page_ok() static, remove 'sc' arg until later patch
* remove unnecessary alloc_demote_page() hugetlb warning
* Simplify alloc_demote_page() gfp mask. Depend on __GFP_NORETRY to make it lightweight instead of fancier stuff like leaving out __GFP_IO/FS.
* Allocate migration page with alloc_migration_target() instead of allocating directly.
changes from 20200730:
* Add another pass through shrink_page_list() when demotion fails.
changes from 20210302:
* Use __GFP_THISNODE and revise the comment explaining the GFP mask constructionn

---

 b/include/linux/migrate.h        |    9 ++++
 b/include/trace/events/migrate.h |    3 -
 b/mm/vmscan.c                    |   82 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 1 deletion(-)

diff -puN include/linux/migrate.h~demote-with-migrate_pages include/linux/migrate.h

include/linux/migrate.h 2021-03-31 15:17:15.853000251 -0700 renamed from include/linux/migrate.h~demote-with-migrate_pages 2021-03-31 15:17:15.842000251 -0700 +9 -0

@@ -27,6 +27,7 @@ enum migrate_reason {
        MR_MEMPOLICY_MBIND,
        MR_NUMA_MISPLACED,
        MR_CONTIG_RANGE,
+       MR_DEMOTION,
        MR_TYPES
 };
 

@@ -196,6 +197,14 @@ struct migrate_vma {
 int migrate_vma_setup(struct migrate_vma *args);
 void migrate_vma_pages(struct migrate_vma *migrate);
 void migrate_vma_finalize(struct migrate_vma *migrate);
+int next_demotion_node(int node);
+
+#else /* CONFIG_MIGRATION disabled: */
+
+static inline int next_demotion_node(int node)
+{
+       return NUMA_NO_NODE;
+}
 
 #endif /* CONFIG_MIGRATION */
 

diff -puN include/trace/events/migrate.h~demote-with-migrate_pages include/trace/events/migrate.h

include/trace/events/migrate.h 2021-03-31 15:17:15.853000251 -0700 renamed from include/trace/events/migrate.h~demote-with-migrate_pages 2021-03-31 15:17:15.846000251 -0700 +2 -1

@@ -20,7 +20,8 @@
        EM( MR_SYSCALL,         "syscall_or_cpuset")            \
        EM( MR_MEMPOLICY_MBIND, "mempolicy_mbind")              \
        EM( MR_NUMA_MISPLACED,  "numa_misplaced")               \
-       EMe(MR_CONTIG_RANGE,    "contig_range")
+       EM( MR_CONTIG_RANGE,    "contig_range")                 \
+       EMe(MR_DEMOTION,        "demotion")
 
 /*
  * First define the enums in the above macros to be exported to userspace

diff -puN mm/vmscan.c~demote-with-migrate_pages mm/vmscan.c

mm/vmscan.c 2021-03-31 15:17:15.856000251 -0700 renamed from mm/vmscan.c~demote-with-migrate_pages 2021-03-31 15:17:15.848000251 -0700 +18 -0

@@ -41,6 +41,7 @@
 #include <linux/kthread.h>
 #include <linux/freezer.h>
 #include <linux/memcontrol.h>
+#include <linux/migrate.h>
 #include <linux/delayacct.h>
 #include <linux/sysctl.h>
 #include <linux/oom.h>

@@ -1035,6 +1036,23 @@ static enum page_references page_check_r
        return PAGEREF_RECLAIM;
 }
 
+static bool migrate_demote_page_ok(struct page *page)
+{
+       int next_nid = next_demotion_node(page_to_nid(page));
+
+       VM_BUG_ON_PAGE(!PageLocked(page), page);
+       VM_BUG_ON_PAGE(PageHuge(page), page);
+       VM_BUG_ON_PAGE(PageLRU(page), page);
+
+       if (next_nid == NUMA_NO_NODE)
+               return false;
+       if (PageTransHuge(page) && !thp_migration_supported())
+               return false;
+
+       // FIXME: actually enable this later in the series
+       return false;
+}
+
 /* Check if a page is dirty or under writeback */
 static void page_check_dirty_writeback(struct page *page,
                                       bool *dirty, bool *writeback)

mm/vmscan.c 2021-03-31 15:17:15.856000251 -0700 renamed from mm/vmscan.c~demote-with-migrate_pages 2021-03-31 15:17:15.848000251 -0700 +58 -0

+                * instead of migrated.
+                */
+               .gfp_mask = GFP_HIGHUSER_MOVABLE | __GFP_NORETRY |
+                           __GFP_THISNODE       | __GFP_NOWARN,
+               .nid = node
+       };
+
+       return alloc_migration_target(page, (unsigned long)&mtc);
+}
+
+/*
+ * Take pages on @demote_list and attempt to demote them to
+ * another node.  Pages which are not demoted are left on
+ * @demote_pages.
+ */
+static unsigned int demote_page_list(struct list_head *demote_pages,
+                                    struct pglist_data *pgdat,
+                                    struct scan_control *sc)
+{
+       int target_nid = next_demotion_node(pgdat->node_id);
+       unsigned int nr_succeeded = 0;
+       int err;
+
+       if (list_empty(demote_pages))
+               return 0;
+
+       /* Demotion ignores all cpuset and mempolicy settings */
+       err = migrate_pages(demote_pages, alloc_demote_page, NULL,
+                           target_nid, MIGRATE_ASYNC, MR_DEMOTION,
+                           &nr_succeeded);
+
+       return nr_succeeded;
+}
+
 /*
  * shrink_page_list() returns the number of reclaimed pages
  */

@@ -1076,12 +1134,15 @@ static unsigned int shrink_page_list(str
 {
        LIST_HEAD(ret_pages);
        LIST_HEAD(free_pages);
+       LIST_HEAD(demote_pages);
        unsigned int nr_reclaimed = 0;
        unsigned int pgactivate = 0;
+       bool do_demote_pass = true;
 
        memset(stat, 0, sizeof(*stat));
        cond_resched();
 
+retry:
        while (!list_empty(page_list)) {
                struct address_space *mapping;
                struct page *page;

@@ -1231,6 +1292,16 @@ static unsigned int shrink_page_list(str
                }
 
                /*
+                * Before reclaiming the page, try to relocate
+                * its contents to another node.
+                */
+               if (do_demote_pass && migrate_demote_page_ok(page)) {
+                       list_add(&page->lru, &demote_pages);
+                       unlock_page(page);
+                       continue;
+               }
+
+               /*
                 * Anonymous process memory has backing store?
                 * Try to allocate it some swap space here.
                 * Lazyfree page could be freed directly

@@ -1480,6 +1551,17 @@ keep:
                list_add(&page->lru, &ret_pages);
                VM_BUG_ON_PAGE(PageLRU(page) || PageUnevictable(page), page);
        }
+       /* 'page_list' is always empty here */
+
+       /* Migrate pages selected for demotion */
+       nr_reclaimed += demote_page_list(&demote_pages, pgdat, sc);
+       /* Pages that could not be demoted are still in @demote_pages */
+       if (!list_empty(&demote_pages)) {
+               /* Pages which failed to demoted go back on @page_list for retry: */
+               list_splice_init(&demote_pages, page_list);
+               do_demote_pass = false;
+               goto retry;
+       }
 
        pgactivate = stat->nr_activate[0] + stat->nr_activate[1];
 

_

Quoted 4 lines · Dave Hansen

From: Dave Hansen <[email protected]>

Quoted 8 lines

This is mostly derived from a patch from Yang Shi:

        https://lore.kernel.org/linux-mm/[email protected]/

Add code to the reclaim path (shrink_page_list()) to "demote" data to another NUMA node instead of discarding the data. This always avoids the cost of I/O needed to read the page back in and sometimes avoids the writeout cost when the pagee is dirty.

s/pagee/page

Quoted 3 lines

A second pass through shrink_page_list() will be made if any demotions fail. This essentally falls back to normal reclaim behavior in the

s/essentally/essentially

Quoted 5 lines

case that demotions fail. Previous versions of this patch may have simply failed to reclaim pages which were eligible for demotion but were unable to be demoted in practice.

Note: This just adds the start of infratructure for migration. It is

s/infratructure/infrastructure

mm/vmscan.c static void page_check_dirty_writeback(s · line 1091

Quoted 6 lines

mm/vmscan.c renamed from mm/vmscan.c~demote-with-migrate_pages +6 -0

+static struct page *alloc_demote_page(struct page *page, unsigned long node)
+{
+       struct migration_target_control mtc = {
+               /*
+                * Allocate from 'node', or fail the quickly and quietly.
+                * When this happens, 'page; will likely just be discarded

s/'page;/'page'

From: Dave Hansen <[email protected]>

This is mostly derived from a patch from Yang Shi:

	https://lore.kernel.org/linux-mm/[email protected]/

Add code to the reclaim path (shrink_page_list()) to "demote" data to another NUMA node instead of discarding the data. This always avoids the cost of I/O needed to read the page back in and sometimes avoids the writeout cost when the pagee is dirty.

A second pass through shrink_page_list() will be made if any demotions fail. This essentally falls back to normal reclaim behavior in the case that demotions fail. Previous versions of this patch may have simply failed to reclaim pages which were eligible for demotion but were unable to be demoted in practice.

Note: This just adds the start of infratructure for migration. It is actually disabled next to the FIXME in migrate_demote_page_ok().

Signed-off-by Dave Hansen <[email protected]>

Cc Wei Xu <[email protected]>

Cc Yang Shi <[email protected]>

Cc David Rientjes <[email protected]>

Cc Huang Ying <[email protected]>

Cc Dan Williams <[email protected]>

Cc osalvador <[email protected]>

From: Yang Shi <[email protected]>

Account the number of demoted pages into reclaim_state->nr_demoted.

Add pgdemote_kswapd and pgdemote_direct VM counters showed in /proc/vmstat.

[ daveh:
- __count_vm_events() a bit, and made them look at the THP size directly rather than getting data from migrate_pages()
]

Signed-off-by Yang Shi <[email protected]>

Signed-off-by Dave Hansen <[email protected]>

Reviewed-by Yang Shi <[email protected]>

Cc Wei Xu <[email protected]>

Cc David Rientjes <[email protected]>

Cc Huang Ying <[email protected]>

Cc Dan Williams <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc osalvador <[email protected]>

mm/vmscan.c renamed from mm/vmscan.c~mm-vmscan-anon-can-be-aged +9 -0

+/*
+ * Anonymous LRU management is a waste if there is
+ * ultimately no way to reclaim the memory.
+ */
+bool anon_should_be_aged(struct lruvec *lruvec)
+{
+       struct pglist_data *pgdat = lruvec_pgdat(lruvec);
+
+       /* Aging the anon LRU is valuable if swap is present: */

anon_should_be_aged() doesn't really need "lruvec". It essentially answers whether the pages of the given node can be swapped or demoted. So it would be clearer and less confusing if anon_should_be_aged() takes "pgdat" instead of "lruvec" as the argument. The call to mem_cgroup_lruvec(NULL, pgdat) in age_active_anon() can then be removed as well.