* [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2017-12-08 16:15 ` Michal Hocko
0 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2017-12-08 16:15 UTC (permalink / raw)
To: linux-mm
Cc: Zi Yan, Naoya Horiguchi, Kirill A. Shutemov, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML, Michal Hocko
From: Michal Hocko <mhocko@suse.com>
do_pages_move is supposed to move user defined memory (an array of
addresses) to the user defined numa nodes (an array of nodes one for
each address). The user provided status array then contains resulting
numa node for each address or an error. The semantic of this function is
little bit confusing because only some errors are reported back. Notably
migrate_pages error is only reported via the return value. This patch
doesn't try to address these semantic nuances but rather change the
underlying implementation.
Currently we are processing user input (which can be really large)
in batches which are stored to a temporarily allocated page. Each
address is resolved to its struct page and stored to page_to_node
structure along with the requested target numa node. The array of these
structures is then conveyed down the page migration path via private
argument. new_page_node then finds the corresponding structure and
allocates the proper target page.
What is the problem with the current implementation and why to change
it? Apart from being quite ugly it also doesn't cope with unexpected
pages showing up on the migration list inside migrate_pages path.
That doesn't happen currently but the follow up patch would like to
make the thp migration code more clear and that would need to split a
THP into the list for some cases.
How does the new implementation work? Well, instead of batching into a
fixed size array we simply batch all pages that should be migrated to
the same node and isolate all of them into a linked list which doesn't
require any additional storage. This should work reasonably well because
page migration usually migrates larger ranges of memory to a specific
node. So the common case should work equally well as the current
implementation. Even if somebody constructs an input where the target
numa nodes would be interleaved we shouldn't see a large performance
impact because page migration alone doesn't really benefit from
batching. mmap_sem batching for the lookup is quite questionable and
isolate_lru_page which would benefit from batching is not using it even
in the current implementation.
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/internal.h | 1 +
mm/mempolicy.c | 5 +-
mm/migrate.c | 306 +++++++++++++++++++++++++--------------------------------
3 files changed, 139 insertions(+), 173 deletions(-)
diff --git a/mm/internal.h b/mm/internal.h
index e6bd35182dae..1a1bb5d59c15 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -538,4 +538,5 @@ static inline bool is_migrate_highatomic_page(struct page *page)
}
void setup_zone_pageset(struct zone *zone);
+extern struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x);
#endif /* __MM_INTERNAL_H */
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index f604b22ebb65..66c9c79b21be 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -942,7 +942,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
}
}
-static struct page *new_node_page(struct page *page, unsigned long node, int **x)
+/* page allocation callback for NUMA node migration */
+struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x)
{
if (PageHuge(page))
return alloc_huge_page_node(page_hstate(compound_head(page)),
@@ -986,7 +987,7 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
flags | MPOL_MF_DISCONTIG_OK, &pagelist);
if (!list_empty(&pagelist)) {
- err = migrate_pages(&pagelist, new_node_page, NULL, dest,
+ err = migrate_pages(&pagelist, alloc_new_node_page, NULL, dest,
MIGRATE_SYNC, MR_SYSCALL);
if (err)
putback_movable_pages(&pagelist);
diff --git a/mm/migrate.c b/mm/migrate.c
index 4d0be47a322a..9d7252ea2acd 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1444,141 +1444,104 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
}
#ifdef CONFIG_NUMA
-/*
- * Move a list of individual pages
- */
-struct page_to_node {
- unsigned long addr;
- struct page *page;
- int node;
- int status;
-};
-static struct page *new_page_node(struct page *p, unsigned long private,
- int **result)
+static int store_status(int __user *status, int start, int value, int nr)
{
- struct page_to_node *pm = (struct page_to_node *)private;
-
- while (pm->node != MAX_NUMNODES && pm->page != p)
- pm++;
+ while (nr-- > 0) {
+ if (put_user(value, status + start))
+ return -EFAULT;
+ start++;
+ }
- if (pm->node == MAX_NUMNODES)
- return NULL;
+ return 0;
+}
- *result = &pm->status;
+static int do_move_pages_to_node(struct mm_struct *mm,
+ struct list_head *pagelist, int node)
+{
+ int err;
- if (PageHuge(p))
- return alloc_huge_page_node(page_hstate(compound_head(p)),
- pm->node);
- else if (thp_migration_supported() && PageTransHuge(p)) {
- struct page *thp;
+ if (list_empty(pagelist))
+ return 0;
- thp = alloc_pages_node(pm->node,
- (GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
- HPAGE_PMD_ORDER);
- if (!thp)
- return NULL;
- prep_transhuge_page(thp);
- return thp;
- } else
- return __alloc_pages_node(pm->node,
- GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
+ err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
+ MIGRATE_SYNC, MR_SYSCALL);
+ if (err)
+ putback_movable_pages(pagelist);
+ return err;
}
/*
- * Move a set of pages as indicated in the pm array. The addr
- * field must be set to the virtual address of the page to be moved
- * and the node number must contain a valid target node.
- * The pm array ends with node = MAX_NUMNODES.
+ * Resolves the given address to a struct page, isolates it from the LRU and
+ * puts it to the given pagelist.
+ * Returns -errno if the page cannot be found/isolated or 0 when it has been
+ * queued or the page doesn't need to be migrated because it is already on
+ * the target node
*/
-static int do_move_page_to_node_array(struct mm_struct *mm,
- struct page_to_node *pm,
- int migrate_all)
+static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
+ int node, struct list_head *pagelist, bool migrate_all)
{
+ struct vm_area_struct *vma;
+ struct page *page;
+ unsigned int follflags;
int err;
- struct page_to_node *pp;
- LIST_HEAD(pagelist);
down_read(&mm->mmap_sem);
+ err = -EFAULT;
+ vma = find_vma(mm, addr);
+ if (!vma || addr < vma->vm_start || !vma_migratable(vma))
+ goto out;
- /*
- * Build a list of pages to migrate
- */
- for (pp = pm; pp->node != MAX_NUMNODES; pp++) {
- struct vm_area_struct *vma;
- struct page *page;
- struct page *head;
- unsigned int follflags;
-
- err = -EFAULT;
- vma = find_vma(mm, pp->addr);
- if (!vma || pp->addr < vma->vm_start || !vma_migratable(vma))
- goto set_status;
-
- /* FOLL_DUMP to ignore special (like zero) pages */
- follflags = FOLL_GET | FOLL_DUMP;
- if (!thp_migration_supported())
- follflags |= FOLL_SPLIT;
- page = follow_page(vma, pp->addr, follflags);
+ /* FOLL_DUMP to ignore special (like zero) pages */
+ follflags = FOLL_GET | FOLL_DUMP;
+ if (!thp_migration_supported())
+ follflags |= FOLL_SPLIT;
+ page = follow_page(vma, addr, follflags);
- err = PTR_ERR(page);
- if (IS_ERR(page))
- goto set_status;
+ err = PTR_ERR(page);
+ if (IS_ERR(page))
+ goto out;
- err = -ENOENT;
- if (!page)
- goto set_status;
+ err = -ENOENT;
+ if (!page)
+ goto out;
- err = page_to_nid(page);
+ err = 0;
+ if (page_to_nid(page) == node)
+ goto out_putpage;
- if (err == pp->node)
- /*
- * Node already in the right place
- */
- goto put_and_set;
+ err = -EACCES;
+ if (page_mapcount(page) > 1 &&
+ !migrate_all)
+ goto out_putpage;
- err = -EACCES;
- if (page_mapcount(page) > 1 &&
- !migrate_all)
- goto put_and_set;
-
- if (PageHuge(page)) {
- if (PageHead(page)) {
- isolate_huge_page(page, &pagelist);
- err = 0;
- pp->page = page;
- }
- goto put_and_set;
+ if (PageHuge(page)) {
+ if (PageHead(page)) {
+ isolate_huge_page(page, pagelist);
+ err = 0;
}
+ } else {
+ struct page *head;
- pp->page = compound_head(page);
head = compound_head(page);
err = isolate_lru_page(head);
- if (!err) {
- list_add_tail(&head->lru, &pagelist);
- mod_node_page_state(page_pgdat(head),
- NR_ISOLATED_ANON + page_is_file_cache(head),
- hpage_nr_pages(head));
- }
-put_and_set:
- /*
- * Either remove the duplicate refcount from
- * isolate_lru_page() or drop the page ref if it was
- * not isolated.
- */
- put_page(page);
-set_status:
- pp->status = err;
- }
-
- err = 0;
- if (!list_empty(&pagelist)) {
- err = migrate_pages(&pagelist, new_page_node, NULL,
- (unsigned long)pm, MIGRATE_SYNC, MR_SYSCALL);
if (err)
- putback_movable_pages(&pagelist);
- }
+ goto out_putpage;
+ err = 0;
+ list_add_tail(&head->lru, pagelist);
+ mod_node_page_state(page_pgdat(head),
+ NR_ISOLATED_ANON + page_is_file_cache(head),
+ hpage_nr_pages(head));
+ }
+out_putpage:
+ /*
+ * Either remove the duplicate refcount from
+ * isolate_lru_page() or drop the page ref if it was
+ * not isolated.
+ */
+ put_page(page);
+out:
up_read(&mm->mmap_sem);
return err;
}
@@ -1593,79 +1556,80 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
const int __user *nodes,
int __user *status, int flags)
{
- struct page_to_node *pm;
- unsigned long chunk_nr_pages;
- unsigned long chunk_start;
- int err;
-
- err = -ENOMEM;
- pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
- if (!pm)
- goto out;
+ int chunk_node = NUMA_NO_NODE;
+ LIST_HEAD(pagelist);
+ int chunk_start, i;
+ int err = 0, err1;
migrate_prep();
- /*
- * Store a chunk of page_to_node array in a page,
- * but keep the last one as a marker
- */
- chunk_nr_pages = (PAGE_SIZE / sizeof(struct page_to_node)) - 1;
-
- for (chunk_start = 0;
- chunk_start < nr_pages;
- chunk_start += chunk_nr_pages) {
- int j;
-
- if (chunk_start + chunk_nr_pages > nr_pages)
- chunk_nr_pages = nr_pages - chunk_start;
-
- /* fill the chunk pm with addrs and nodes from user-space */
- for (j = 0; j < chunk_nr_pages; j++) {
- const void __user *p;
- int node;
-
- err = -EFAULT;
- if (get_user(p, pages + j + chunk_start))
- goto out_pm;
- pm[j].addr = (unsigned long) p;
-
- if (get_user(node, nodes + j + chunk_start))
- goto out_pm;
-
- err = -ENODEV;
- if (node < 0 || node >= MAX_NUMNODES)
- goto out_pm;
+ for (i = chunk_start = 0; i < nr_pages; i++) {
+ const void __user *p;
+ unsigned long addr;
+ int node;
- if (!node_state(node, N_MEMORY))
- goto out_pm;
-
- err = -EACCES;
- if (!node_isset(node, task_nodes))
- goto out_pm;
+ err = -EFAULT;
+ if (get_user(p, pages + i))
+ goto out_flush;
+ if (get_user(node, nodes + i))
+ goto out_flush;
+ addr = (unsigned long)p;
+
+ err = -ENODEV;
+ if (node < 0 || node >= MAX_NUMNODES)
+ goto out_flush;
+ if (!node_state(node, N_MEMORY))
+ goto out_flush;
- pm[j].node = node;
+ err = -EACCES;
+ if (!node_isset(node, task_nodes))
+ goto out_flush;
+
+ if (chunk_node == NUMA_NO_NODE) {
+ chunk_node = node;
+ chunk_start = i;
+ } else if (node != chunk_node) {
+ err = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ if (err)
+ goto out;
+ err = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ if (err)
+ goto out;
+ chunk_start = i;
+ chunk_node = node;
}
- /* End marker for this chunk */
- pm[chunk_nr_pages].node = MAX_NUMNODES;
+ /*
+ * Errors in the page lookup or isolation are not fatal and we simply
+ * report them via status
+ */
+ err = add_page_for_migration(mm, addr, chunk_node,
+ &pagelist, flags & MPOL_MF_MOVE_ALL);
+ if (!err)
+ continue;
- /* Migrate this chunk */
- err = do_move_page_to_node_array(mm, pm,
- flags & MPOL_MF_MOVE_ALL);
- if (err < 0)
- goto out_pm;
+ err = store_status(status, i, err, 1);
+ if (err)
+ goto out_flush;
- /* Return status information */
- for (j = 0; j < chunk_nr_pages; j++)
- if (put_user(pm[j].status, status + j + chunk_start)) {
- err = -EFAULT;
- goto out_pm;
- }
+ err = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ if (err)
+ goto out;
+ if (i > chunk_start) {
+ err = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ if (err)
+ goto out;
+ }
+ chunk_node = NUMA_NO_NODE;
}
err = 0;
-
-out_pm:
- free_page((unsigned long)pm);
+out_flush:
+ /* Make sure we do not overwrite the existing error */
+ err1 = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ if (!err1)
+ err1 = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ if (!err)
+ err = err1;
out:
return err;
}
--
2.15.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-08 16:15 ` Michal Hocko
@ 2017-12-13 12:07 ` Kirill A. Shutemov
-1 siblings, 0 replies; 61+ messages in thread
From: Kirill A. Shutemov @ 2017-12-13 12:07 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML, Michal Hocko
On Fri, Dec 08, 2017 at 05:15:57PM +0100, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> do_pages_move is supposed to move user defined memory (an array of
> addresses) to the user defined numa nodes (an array of nodes one for
> each address). The user provided status array then contains resulting
> numa node for each address or an error. The semantic of this function is
> little bit confusing because only some errors are reported back. Notably
> migrate_pages error is only reported via the return value. This patch
> doesn't try to address these semantic nuances but rather change the
> underlying implementation.
>
> Currently we are processing user input (which can be really large)
> in batches which are stored to a temporarily allocated page. Each
> address is resolved to its struct page and stored to page_to_node
> structure along with the requested target numa node. The array of these
> structures is then conveyed down the page migration path via private
> argument. new_page_node then finds the corresponding structure and
> allocates the proper target page.
>
> What is the problem with the current implementation and why to change
> it? Apart from being quite ugly it also doesn't cope with unexpected
> pages showing up on the migration list inside migrate_pages path.
> That doesn't happen currently but the follow up patch would like to
> make the thp migration code more clear and that would need to split a
> THP into the list for some cases.
>
> How does the new implementation work? Well, instead of batching into a
> fixed size array we simply batch all pages that should be migrated to
> the same node and isolate all of them into a linked list which doesn't
> require any additional storage. This should work reasonably well because
> page migration usually migrates larger ranges of memory to a specific
> node. So the common case should work equally well as the current
> implementation. Even if somebody constructs an input where the target
> numa nodes would be interleaved we shouldn't see a large performance
> impact because page migration alone doesn't really benefit from
> batching. mmap_sem batching for the lookup is quite questionable and
> isolate_lru_page which would benefit from batching is not using it even
> in the current implementation.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/internal.h | 1 +
> mm/mempolicy.c | 5 +-
> mm/migrate.c | 306 +++++++++++++++++++++++++--------------------------------
> 3 files changed, 139 insertions(+), 173 deletions(-)
The approach looks fine to me.
But patch is rather large and hard to review. And how git mixed add/remove
lines doesn't help too. Any chance to split it up further?
One nitpick: I don't think 'chunk' terminology should go away with the
patch.
--
Kirill A. Shutemov
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2017-12-13 12:07 ` Kirill A. Shutemov
0 siblings, 0 replies; 61+ messages in thread
From: Kirill A. Shutemov @ 2017-12-13 12:07 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML, Michal Hocko
On Fri, Dec 08, 2017 at 05:15:57PM +0100, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> do_pages_move is supposed to move user defined memory (an array of
> addresses) to the user defined numa nodes (an array of nodes one for
> each address). The user provided status array then contains resulting
> numa node for each address or an error. The semantic of this function is
> little bit confusing because only some errors are reported back. Notably
> migrate_pages error is only reported via the return value. This patch
> doesn't try to address these semantic nuances but rather change the
> underlying implementation.
>
> Currently we are processing user input (which can be really large)
> in batches which are stored to a temporarily allocated page. Each
> address is resolved to its struct page and stored to page_to_node
> structure along with the requested target numa node. The array of these
> structures is then conveyed down the page migration path via private
> argument. new_page_node then finds the corresponding structure and
> allocates the proper target page.
>
> What is the problem with the current implementation and why to change
> it? Apart from being quite ugly it also doesn't cope with unexpected
> pages showing up on the migration list inside migrate_pages path.
> That doesn't happen currently but the follow up patch would like to
> make the thp migration code more clear and that would need to split a
> THP into the list for some cases.
>
> How does the new implementation work? Well, instead of batching into a
> fixed size array we simply batch all pages that should be migrated to
> the same node and isolate all of them into a linked list which doesn't
> require any additional storage. This should work reasonably well because
> page migration usually migrates larger ranges of memory to a specific
> node. So the common case should work equally well as the current
> implementation. Even if somebody constructs an input where the target
> numa nodes would be interleaved we shouldn't see a large performance
> impact because page migration alone doesn't really benefit from
> batching. mmap_sem batching for the lookup is quite questionable and
> isolate_lru_page which would benefit from batching is not using it even
> in the current implementation.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/internal.h | 1 +
> mm/mempolicy.c | 5 +-
> mm/migrate.c | 306 +++++++++++++++++++++++++--------------------------------
> 3 files changed, 139 insertions(+), 173 deletions(-)
The approach looks fine to me.
But patch is rather large and hard to review. And how git mixed add/remove
lines doesn't help too. Any chance to split it up further?
One nitpick: I don't think 'chunk' terminology should go away with the
patch.
--
Kirill A. Shutemov
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-13 12:07 ` Kirill A. Shutemov
@ 2017-12-13 12:17 ` Michal Hocko
-1 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2017-12-13 12:17 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Wed 13-12-17 15:07:33, Kirill A. Shutemov wrote:
[...]
> The approach looks fine to me.
>
> But patch is rather large and hard to review. And how git mixed add/remove
> lines doesn't help too. Any chance to split it up further?
I was trying to do that but this is a drop in replacement so it is quite
hard to do in smaller pieces. I've already put the allocation callback
cleanup into a separate one but this is about all that I figured how to
split. If you have any suggestions I am willing to try them out.
> One nitpick: I don't think 'chunk' terminology should go away with the
> patch.
Not sure what you mean here. I have kept chunk_start, chunk_node, so I
am not really changing that terminology
Thanks!
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2017-12-13 12:17 ` Michal Hocko
0 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2017-12-13 12:17 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Wed 13-12-17 15:07:33, Kirill A. Shutemov wrote:
[...]
> The approach looks fine to me.
>
> But patch is rather large and hard to review. And how git mixed add/remove
> lines doesn't help too. Any chance to split it up further?
I was trying to do that but this is a drop in replacement so it is quite
hard to do in smaller pieces. I've already put the allocation callback
cleanup into a separate one but this is about all that I figured how to
split. If you have any suggestions I am willing to try them out.
> One nitpick: I don't think 'chunk' terminology should go away with the
> patch.
Not sure what you mean here. I have kept chunk_start, chunk_node, so I
am not really changing that terminology
Thanks!
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-13 12:17 ` Michal Hocko
@ 2017-12-13 12:47 ` Kirill A. Shutemov
-1 siblings, 0 replies; 61+ messages in thread
From: Kirill A. Shutemov @ 2017-12-13 12:47 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Wed, Dec 13, 2017 at 01:17:03PM +0100, Michal Hocko wrote:
> On Wed 13-12-17 15:07:33, Kirill A. Shutemov wrote:
> [...]
> > The approach looks fine to me.
> >
> > But patch is rather large and hard to review. And how git mixed add/remove
> > lines doesn't help too. Any chance to split it up further?
>
> I was trying to do that but this is a drop in replacement so it is quite
> hard to do in smaller pieces. I've already put the allocation callback
> cleanup into a separate one but this is about all that I figured how to
> split. If you have any suggestions I am willing to try them out.
"git diff --patience" seems generate more readable output for the patch.
> > One nitpick: I don't think 'chunk' terminology should go away with the
> > patch.
>
> Not sure what you mean here. I have kept chunk_start, chunk_node, so I
> am not really changing that terminology
We don't really have chunks anymore, right? We still *may* have per-node
batching, but..
Maybe just 'start' and 'current_node'?
--
Kirill A. Shutemov
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2017-12-13 12:47 ` Kirill A. Shutemov
0 siblings, 0 replies; 61+ messages in thread
From: Kirill A. Shutemov @ 2017-12-13 12:47 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Wed, Dec 13, 2017 at 01:17:03PM +0100, Michal Hocko wrote:
> On Wed 13-12-17 15:07:33, Kirill A. Shutemov wrote:
> [...]
> > The approach looks fine to me.
> >
> > But patch is rather large and hard to review. And how git mixed add/remove
> > lines doesn't help too. Any chance to split it up further?
>
> I was trying to do that but this is a drop in replacement so it is quite
> hard to do in smaller pieces. I've already put the allocation callback
> cleanup into a separate one but this is about all that I figured how to
> split. If you have any suggestions I am willing to try them out.
"git diff --patience" seems generate more readable output for the patch.
> > One nitpick: I don't think 'chunk' terminology should go away with the
> > patch.
>
> Not sure what you mean here. I have kept chunk_start, chunk_node, so I
> am not really changing that terminology
We don't really have chunks anymore, right? We still *may* have per-node
batching, but..
Maybe just 'start' and 'current_node'?
--
Kirill A. Shutemov
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-13 12:47 ` Kirill A. Shutemov
@ 2017-12-13 14:10 ` Michal Hocko
-1 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2017-12-13 14:10 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Wed 13-12-17 15:47:31, Kirill A. Shutemov wrote:
> On Wed, Dec 13, 2017 at 01:17:03PM +0100, Michal Hocko wrote:
> > On Wed 13-12-17 15:07:33, Kirill A. Shutemov wrote:
> > [...]
> > > The approach looks fine to me.
> > >
> > > But patch is rather large and hard to review. And how git mixed add/remove
> > > lines doesn't help too. Any chance to split it up further?
> >
> > I was trying to do that but this is a drop in replacement so it is quite
> > hard to do in smaller pieces. I've already put the allocation callback
> > cleanup into a separate one but this is about all that I figured how to
> > split. If you have any suggestions I am willing to try them out.
>
> "git diff --patience" seems generate more readable output for the patch.
Hmm, I wasn't aware of this option. Are you suggesting I should use it
to general the patch to send?
> > > One nitpick: I don't think 'chunk' terminology should go away with the
> > > patch.
> >
> > Not sure what you mean here. I have kept chunk_start, chunk_node, so I
> > am not really changing that terminology
>
> We don't really have chunks anymore, right? We still *may* have per-node
> batching, but..
>
> Maybe just 'start' and 'current_node'?
Ohh, I've read your response that you want to preserve the naming. I can
certainly do the rename.
---
diff --git a/mm/migrate.c b/mm/migrate.c
index 9d7252ea2acd..5491045b60f9 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1556,14 +1556,14 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
const int __user *nodes,
int __user *status, int flags)
{
- int chunk_node = NUMA_NO_NODE;
+ int current_node = NUMA_NO_NODE;
LIST_HEAD(pagelist);
- int chunk_start, i;
+ int start, i;
int err = 0, err1;
migrate_prep();
- for (i = chunk_start = 0; i < nr_pages; i++) {
+ for (i = start = 0; i < nr_pages; i++) {
const void __user *p;
unsigned long addr;
int node;
@@ -1585,25 +1585,25 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
if (!node_isset(node, task_nodes))
goto out_flush;
- if (chunk_node == NUMA_NO_NODE) {
- chunk_node = node;
- chunk_start = i;
- } else if (node != chunk_node) {
- err = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ if (current_node == NUMA_NO_NODE) {
+ current_node = node;
+ start = i;
+ } else if (node != current_node) {
+ err = do_move_pages_to_node(mm, &pagelist, current_node);
if (err)
goto out;
- err = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ err = store_status(status, start, current_node, i - start);
if (err)
goto out;
- chunk_start = i;
- chunk_node = node;
+ start = i;
+ current_node = node;
}
/*
* Errors in the page lookup or isolation are not fatal and we simply
* report them via status
*/
- err = add_page_for_migration(mm, addr, chunk_node,
+ err = add_page_for_migration(mm, addr, current_node,
&pagelist, flags & MPOL_MF_MOVE_ALL);
if (!err)
continue;
@@ -1612,22 +1612,22 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
if (err)
goto out_flush;
- err = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ err = do_move_pages_to_node(mm, &pagelist, current_node);
if (err)
goto out;
- if (i > chunk_start) {
- err = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ if (i > start) {
+ err = store_status(status, start, current_node, i - start);
if (err)
goto out;
}
- chunk_node = NUMA_NO_NODE;
+ current_node = NUMA_NO_NODE;
}
err = 0;
out_flush:
/* Make sure we do not overwrite the existing error */
- err1 = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ err1 = do_move_pages_to_node(mm, &pagelist, current_node);
if (!err1)
- err1 = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ err1 = store_status(status, start, current_node, i - start);
if (!err)
err = err1;
out:
--
Michal Hocko
SUSE Labs
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2017-12-13 14:10 ` Michal Hocko
0 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2017-12-13 14:10 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Wed 13-12-17 15:47:31, Kirill A. Shutemov wrote:
> On Wed, Dec 13, 2017 at 01:17:03PM +0100, Michal Hocko wrote:
> > On Wed 13-12-17 15:07:33, Kirill A. Shutemov wrote:
> > [...]
> > > The approach looks fine to me.
> > >
> > > But patch is rather large and hard to review. And how git mixed add/remove
> > > lines doesn't help too. Any chance to split it up further?
> >
> > I was trying to do that but this is a drop in replacement so it is quite
> > hard to do in smaller pieces. I've already put the allocation callback
> > cleanup into a separate one but this is about all that I figured how to
> > split. If you have any suggestions I am willing to try them out.
>
> "git diff --patience" seems generate more readable output for the patch.
Hmm, I wasn't aware of this option. Are you suggesting I should use it
to general the patch to send?
> > > One nitpick: I don't think 'chunk' terminology should go away with the
> > > patch.
> >
> > Not sure what you mean here. I have kept chunk_start, chunk_node, so I
> > am not really changing that terminology
>
> We don't really have chunks anymore, right? We still *may* have per-node
> batching, but..
>
> Maybe just 'start' and 'current_node'?
Ohh, I've read your response that you want to preserve the naming. I can
certainly do the rename.
---
diff --git a/mm/migrate.c b/mm/migrate.c
index 9d7252ea2acd..5491045b60f9 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1556,14 +1556,14 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
const int __user *nodes,
int __user *status, int flags)
{
- int chunk_node = NUMA_NO_NODE;
+ int current_node = NUMA_NO_NODE;
LIST_HEAD(pagelist);
- int chunk_start, i;
+ int start, i;
int err = 0, err1;
migrate_prep();
- for (i = chunk_start = 0; i < nr_pages; i++) {
+ for (i = start = 0; i < nr_pages; i++) {
const void __user *p;
unsigned long addr;
int node;
@@ -1585,25 +1585,25 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
if (!node_isset(node, task_nodes))
goto out_flush;
- if (chunk_node == NUMA_NO_NODE) {
- chunk_node = node;
- chunk_start = i;
- } else if (node != chunk_node) {
- err = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ if (current_node == NUMA_NO_NODE) {
+ current_node = node;
+ start = i;
+ } else if (node != current_node) {
+ err = do_move_pages_to_node(mm, &pagelist, current_node);
if (err)
goto out;
- err = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ err = store_status(status, start, current_node, i - start);
if (err)
goto out;
- chunk_start = i;
- chunk_node = node;
+ start = i;
+ current_node = node;
}
/*
* Errors in the page lookup or isolation are not fatal and we simply
* report them via status
*/
- err = add_page_for_migration(mm, addr, chunk_node,
+ err = add_page_for_migration(mm, addr, current_node,
&pagelist, flags & MPOL_MF_MOVE_ALL);
if (!err)
continue;
@@ -1612,22 +1612,22 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
if (err)
goto out_flush;
- err = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ err = do_move_pages_to_node(mm, &pagelist, current_node);
if (err)
goto out;
- if (i > chunk_start) {
- err = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ if (i > start) {
+ err = store_status(status, start, current_node, i - start);
if (err)
goto out;
}
- chunk_node = NUMA_NO_NODE;
+ current_node = NUMA_NO_NODE;
}
err = 0;
out_flush:
/* Make sure we do not overwrite the existing error */
- err1 = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ err1 = do_move_pages_to_node(mm, &pagelist, current_node);
if (!err1)
- err1 = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ err1 = store_status(status, start, current_node, i - start);
if (!err)
err = err1;
out:
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-13 14:10 ` Michal Hocko
@ 2017-12-13 14:27 ` Kirill A. Shutemov
-1 siblings, 0 replies; 61+ messages in thread
From: Kirill A. Shutemov @ 2017-12-13 14:27 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Wed, Dec 13, 2017 at 03:10:39PM +0100, Michal Hocko wrote:
> On Wed 13-12-17 15:47:31, Kirill A. Shutemov wrote:
> > On Wed, Dec 13, 2017 at 01:17:03PM +0100, Michal Hocko wrote:
> > > On Wed 13-12-17 15:07:33, Kirill A. Shutemov wrote:
> > > [...]
> > > > The approach looks fine to me.
> > > >
> > > > But patch is rather large and hard to review. And how git mixed add/remove
> > > > lines doesn't help too. Any chance to split it up further?
> > >
> > > I was trying to do that but this is a drop in replacement so it is quite
> > > hard to do in smaller pieces. I've already put the allocation callback
> > > cleanup into a separate one but this is about all that I figured how to
> > > split. If you have any suggestions I am willing to try them out.
> >
> > "git diff --patience" seems generate more readable output for the patch.
>
> Hmm, I wasn't aware of this option. Are you suggesting I should use it
> to general the patch to send?
I don't know if it's better in general (it's not default after all), but it
seems helps for this particular case.
>
> > > > One nitpick: I don't think 'chunk' terminology should go away with the
> > > > patch.
> > >
> > > Not sure what you mean here. I have kept chunk_start, chunk_node, so I
> > > am not really changing that terminology
> >
> > We don't really have chunks anymore, right? We still *may* have per-node
> > batching, but..
> >
> > Maybe just 'start' and 'current_node'?
>
> Ohh, I've read your response that you want to preserve the naming. I can
> certainly do the rename.
Yep, that's better.
--
Kirill A. Shutemov
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2017-12-13 14:27 ` Kirill A. Shutemov
0 siblings, 0 replies; 61+ messages in thread
From: Kirill A. Shutemov @ 2017-12-13 14:27 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Wed, Dec 13, 2017 at 03:10:39PM +0100, Michal Hocko wrote:
> On Wed 13-12-17 15:47:31, Kirill A. Shutemov wrote:
> > On Wed, Dec 13, 2017 at 01:17:03PM +0100, Michal Hocko wrote:
> > > On Wed 13-12-17 15:07:33, Kirill A. Shutemov wrote:
> > > [...]
> > > > The approach looks fine to me.
> > > >
> > > > But patch is rather large and hard to review. And how git mixed add/remove
> > > > lines doesn't help too. Any chance to split it up further?
> > >
> > > I was trying to do that but this is a drop in replacement so it is quite
> > > hard to do in smaller pieces. I've already put the allocation callback
> > > cleanup into a separate one but this is about all that I figured how to
> > > split. If you have any suggestions I am willing to try them out.
> >
> > "git diff --patience" seems generate more readable output for the patch.
>
> Hmm, I wasn't aware of this option. Are you suggesting I should use it
> to general the patch to send?
I don't know if it's better in general (it's not default after all), but it
seems helps for this particular case.
>
> > > > One nitpick: I don't think 'chunk' terminology should go away with the
> > > > patch.
> > >
> > > Not sure what you mean here. I have kept chunk_start, chunk_node, so I
> > > am not really changing that terminology
> >
> > We don't really have chunks anymore, right? We still *may* have per-node
> > batching, but..
> >
> > Maybe just 'start' and 'current_node'?
>
> Ohh, I've read your response that you want to preserve the naming. I can
> certainly do the rename.
Yep, that's better.
--
Kirill A. Shutemov
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-08 16:15 ` Michal Hocko
@ 2017-12-13 14:39 ` Michal Hocko
-1 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2017-12-13 14:39 UTC (permalink / raw)
To: linux-mm
Cc: Zi Yan, Naoya Horiguchi, Kirill A. Shutemov, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
This patch has been generated with --patience parameter as suggested by
Kirill and it realy seems to provide a more compact diff.
---
>From 1f529769d099ca605888b29059014e7c8f0bfd50 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Fri, 8 Dec 2017 12:28:34 +0100
Subject: [PATCH] mm, numa: rework do_pages_move
do_pages_move is supposed to move user defined memory (an array of
addresses) to the user defined numa nodes (an array of nodes one for
each address). The user provided status array then contains resulting
numa node for each address or an error. The semantic of this function is
little bit confusing because only some errors are reported back. Notably
migrate_pages error is only reported via the return value. This patch
doesn't try to address these semantic nuances but rather change the
underlying implementation.
Currently we are processing user input (which can be really large)
in batches which are stored to a temporarily allocated page. Each
address is resolved to its struct page and stored to page_to_node
structure along with the requested target numa node. The array of these
structures is then conveyed down the page migration path via private
argument. new_page_node then finds the corresponding structure and
allocates the proper target page.
What is the problem with the current implementation and why to change
it? Apart from being quite ugly it also doesn't cope with unexpected
pages showing up on the migration list inside migrate_pages path.
That doesn't happen currently but the follow up patch would like to
make the thp migration code more clear and that would need to split a
THP into the list for some cases.
How does the new implementation work? Well, instead of batching into a
fixed size array we simply batch all pages that should be migrated to
the same node and isolate all of them into a linked list which doesn't
require any additional storage. This should work reasonably well because
page migration usually migrates larger ranges of memory to a specific
node. So the common case should work equally well as the current
implementation. Even if somebody constructs an input where the target
numa nodes would be interleaved we shouldn't see a large performance
impact because page migration alone doesn't really benefit from
batching. mmap_sem batching for the lookup is quite questionable and
isolate_lru_page which would benefit from batching is not using it even
in the current implementation.
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/internal.h | 1 +
mm/mempolicy.c | 5 +-
mm/migrate.c | 340 ++++++++++++++++++++++++++-------------------------------
3 files changed, 156 insertions(+), 190 deletions(-)
diff --git a/mm/internal.h b/mm/internal.h
index e6bd35182dae..1a1bb5d59c15 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -538,4 +538,5 @@ static inline bool is_migrate_highatomic_page(struct page *page)
}
void setup_zone_pageset(struct zone *zone);
+extern struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x);
#endif /* __MM_INTERNAL_H */
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index f604b22ebb65..66c9c79b21be 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -942,7 +942,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
}
}
-static struct page *new_node_page(struct page *page, unsigned long node, int **x)
+/* page allocation callback for NUMA node migration */
+struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x)
{
if (PageHuge(page))
return alloc_huge_page_node(page_hstate(compound_head(page)),
@@ -986,7 +987,7 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
flags | MPOL_MF_DISCONTIG_OK, &pagelist);
if (!list_empty(&pagelist)) {
- err = migrate_pages(&pagelist, new_node_page, NULL, dest,
+ err = migrate_pages(&pagelist, alloc_new_node_page, NULL, dest,
MIGRATE_SYNC, MR_SYSCALL);
if (err)
putback_movable_pages(&pagelist);
diff --git a/mm/migrate.c b/mm/migrate.c
index 4d0be47a322a..9d7252ea2acd 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1444,141 +1444,104 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
}
#ifdef CONFIG_NUMA
+
+static int store_status(int __user *status, int start, int value, int nr)
+{
+ while (nr-- > 0) {
+ if (put_user(value, status + start))
+ return -EFAULT;
+ start++;
+ }
+
+ return 0;
+}
+
+static int do_move_pages_to_node(struct mm_struct *mm,
+ struct list_head *pagelist, int node)
+{
+ int err;
+
+ if (list_empty(pagelist))
+ return 0;
+
+ err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
+ MIGRATE_SYNC, MR_SYSCALL);
+ if (err)
+ putback_movable_pages(pagelist);
+ return err;
+}
+
/*
- * Move a list of individual pages
+ * Resolves the given address to a struct page, isolates it from the LRU and
+ * puts it to the given pagelist.
+ * Returns -errno if the page cannot be found/isolated or 0 when it has been
+ * queued or the page doesn't need to be migrated because it is already on
+ * the target node
*/
-struct page_to_node {
- unsigned long addr;
+static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
+ int node, struct list_head *pagelist, bool migrate_all)
+{
+ struct vm_area_struct *vma;
struct page *page;
- int node;
- int status;
-};
-
-static struct page *new_page_node(struct page *p, unsigned long private,
- int **result)
-{
- struct page_to_node *pm = (struct page_to_node *)private;
-
- while (pm->node != MAX_NUMNODES && pm->page != p)
- pm++;
-
- if (pm->node == MAX_NUMNODES)
- return NULL;
-
- *result = &pm->status;
-
- if (PageHuge(p))
- return alloc_huge_page_node(page_hstate(compound_head(p)),
- pm->node);
- else if (thp_migration_supported() && PageTransHuge(p)) {
- struct page *thp;
-
- thp = alloc_pages_node(pm->node,
- (GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
- HPAGE_PMD_ORDER);
- if (!thp)
- return NULL;
- prep_transhuge_page(thp);
- return thp;
- } else
- return __alloc_pages_node(pm->node,
- GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
-}
-
-/*
- * Move a set of pages as indicated in the pm array. The addr
- * field must be set to the virtual address of the page to be moved
- * and the node number must contain a valid target node.
- * The pm array ends with node = MAX_NUMNODES.
- */
-static int do_move_page_to_node_array(struct mm_struct *mm,
- struct page_to_node *pm,
- int migrate_all)
-{
+ unsigned int follflags;
int err;
- struct page_to_node *pp;
- LIST_HEAD(pagelist);
down_read(&mm->mmap_sem);
+ err = -EFAULT;
+ vma = find_vma(mm, addr);
+ if (!vma || addr < vma->vm_start || !vma_migratable(vma))
+ goto out;
- /*
- * Build a list of pages to migrate
- */
- for (pp = pm; pp->node != MAX_NUMNODES; pp++) {
- struct vm_area_struct *vma;
- struct page *page;
+ /* FOLL_DUMP to ignore special (like zero) pages */
+ follflags = FOLL_GET | FOLL_DUMP;
+ if (!thp_migration_supported())
+ follflags |= FOLL_SPLIT;
+ page = follow_page(vma, addr, follflags);
+
+ err = PTR_ERR(page);
+ if (IS_ERR(page))
+ goto out;
+
+ err = -ENOENT;
+ if (!page)
+ goto out;
+
+ err = 0;
+ if (page_to_nid(page) == node)
+ goto out_putpage;
+
+ err = -EACCES;
+ if (page_mapcount(page) > 1 &&
+ !migrate_all)
+ goto out_putpage;
+
+ if (PageHuge(page)) {
+ if (PageHead(page)) {
+ isolate_huge_page(page, pagelist);
+ err = 0;
+ }
+ } else {
struct page *head;
- unsigned int follflags;
- err = -EFAULT;
- vma = find_vma(mm, pp->addr);
- if (!vma || pp->addr < vma->vm_start || !vma_migratable(vma))
- goto set_status;
-
- /* FOLL_DUMP to ignore special (like zero) pages */
- follflags = FOLL_GET | FOLL_DUMP;
- if (!thp_migration_supported())
- follflags |= FOLL_SPLIT;
- page = follow_page(vma, pp->addr, follflags);
-
- err = PTR_ERR(page);
- if (IS_ERR(page))
- goto set_status;
-
- err = -ENOENT;
- if (!page)
- goto set_status;
-
- err = page_to_nid(page);
-
- if (err == pp->node)
- /*
- * Node already in the right place
- */
- goto put_and_set;
-
- err = -EACCES;
- if (page_mapcount(page) > 1 &&
- !migrate_all)
- goto put_and_set;
-
- if (PageHuge(page)) {
- if (PageHead(page)) {
- isolate_huge_page(page, &pagelist);
- err = 0;
- pp->page = page;
- }
- goto put_and_set;
- }
-
- pp->page = compound_head(page);
head = compound_head(page);
err = isolate_lru_page(head);
- if (!err) {
- list_add_tail(&head->lru, &pagelist);
- mod_node_page_state(page_pgdat(head),
- NR_ISOLATED_ANON + page_is_file_cache(head),
- hpage_nr_pages(head));
- }
-put_and_set:
- /*
- * Either remove the duplicate refcount from
- * isolate_lru_page() or drop the page ref if it was
- * not isolated.
- */
- put_page(page);
-set_status:
- pp->status = err;
- }
-
- err = 0;
- if (!list_empty(&pagelist)) {
- err = migrate_pages(&pagelist, new_page_node, NULL,
- (unsigned long)pm, MIGRATE_SYNC, MR_SYSCALL);
if (err)
- putback_movable_pages(&pagelist);
- }
+ goto out_putpage;
+ err = 0;
+ list_add_tail(&head->lru, pagelist);
+ mod_node_page_state(page_pgdat(head),
+ NR_ISOLATED_ANON + page_is_file_cache(head),
+ hpage_nr_pages(head));
+ }
+out_putpage:
+ /*
+ * Either remove the duplicate refcount from
+ * isolate_lru_page() or drop the page ref if it was
+ * not isolated.
+ */
+ put_page(page);
+out:
up_read(&mm->mmap_sem);
return err;
}
@@ -1593,79 +1556,80 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
const int __user *nodes,
int __user *status, int flags)
{
- struct page_to_node *pm;
- unsigned long chunk_nr_pages;
- unsigned long chunk_start;
- int err;
-
- err = -ENOMEM;
- pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
- if (!pm)
- goto out;
+ int chunk_node = NUMA_NO_NODE;
+ LIST_HEAD(pagelist);
+ int chunk_start, i;
+ int err = 0, err1;
migrate_prep();
- /*
- * Store a chunk of page_to_node array in a page,
- * but keep the last one as a marker
- */
- chunk_nr_pages = (PAGE_SIZE / sizeof(struct page_to_node)) - 1;
-
- for (chunk_start = 0;
- chunk_start < nr_pages;
- chunk_start += chunk_nr_pages) {
- int j;
-
- if (chunk_start + chunk_nr_pages > nr_pages)
- chunk_nr_pages = nr_pages - chunk_start;
-
- /* fill the chunk pm with addrs and nodes from user-space */
- for (j = 0; j < chunk_nr_pages; j++) {
- const void __user *p;
- int node;
-
- err = -EFAULT;
- if (get_user(p, pages + j + chunk_start))
- goto out_pm;
- pm[j].addr = (unsigned long) p;
-
- if (get_user(node, nodes + j + chunk_start))
- goto out_pm;
-
- err = -ENODEV;
- if (node < 0 || node >= MAX_NUMNODES)
- goto out_pm;
-
- if (!node_state(node, N_MEMORY))
- goto out_pm;
-
- err = -EACCES;
- if (!node_isset(node, task_nodes))
- goto out_pm;
-
- pm[j].node = node;
+ for (i = chunk_start = 0; i < nr_pages; i++) {
+ const void __user *p;
+ unsigned long addr;
+ int node;
+
+ err = -EFAULT;
+ if (get_user(p, pages + i))
+ goto out_flush;
+ if (get_user(node, nodes + i))
+ goto out_flush;
+ addr = (unsigned long)p;
+
+ err = -ENODEV;
+ if (node < 0 || node >= MAX_NUMNODES)
+ goto out_flush;
+ if (!node_state(node, N_MEMORY))
+ goto out_flush;
+
+ err = -EACCES;
+ if (!node_isset(node, task_nodes))
+ goto out_flush;
+
+ if (chunk_node == NUMA_NO_NODE) {
+ chunk_node = node;
+ chunk_start = i;
+ } else if (node != chunk_node) {
+ err = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ if (err)
+ goto out;
+ err = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ if (err)
+ goto out;
+ chunk_start = i;
+ chunk_node = node;
}
- /* End marker for this chunk */
- pm[chunk_nr_pages].node = MAX_NUMNODES;
-
- /* Migrate this chunk */
- err = do_move_page_to_node_array(mm, pm,
- flags & MPOL_MF_MOVE_ALL);
- if (err < 0)
- goto out_pm;
-
- /* Return status information */
- for (j = 0; j < chunk_nr_pages; j++)
- if (put_user(pm[j].status, status + j + chunk_start)) {
- err = -EFAULT;
- goto out_pm;
- }
+ /*
+ * Errors in the page lookup or isolation are not fatal and we simply
+ * report them via status
+ */
+ err = add_page_for_migration(mm, addr, chunk_node,
+ &pagelist, flags & MPOL_MF_MOVE_ALL);
+ if (!err)
+ continue;
+
+ err = store_status(status, i, err, 1);
+ if (err)
+ goto out_flush;
+
+ err = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ if (err)
+ goto out;
+ if (i > chunk_start) {
+ err = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ if (err)
+ goto out;
+ }
+ chunk_node = NUMA_NO_NODE;
}
err = 0;
-
-out_pm:
- free_page((unsigned long)pm);
+out_flush:
+ /* Make sure we do not overwrite the existing error */
+ err1 = do_move_pages_to_node(mm, &pagelist, chunk_node);
+ if (!err1)
+ err1 = store_status(status, chunk_start, chunk_node, i - chunk_start);
+ if (!err)
+ err = err1;
out:
return err;
}
--
2.15.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2017-12-13 14:39 ` Michal Hocko
0 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2017-12-13 14:39 UTC (permalink / raw)
To: linux-mm
Cc: Zi Yan, Naoya Horiguchi, Kirill A. Shutemov, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
This patch has been generated with --patience parameter as suggested by
Kirill and it realy seems to provide a more compact diff.
---
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-13 14:39 ` Michal Hocko
@ 2017-12-14 15:35 ` Kirill A. Shutemov
-1 siblings, 0 replies; 61+ messages in thread
From: Kirill A. Shutemov @ 2017-12-14 15:35 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Wed, Dec 13, 2017 at 03:39:48PM +0100, Michal Hocko wrote:
> This patch has been generated with --patience parameter as suggested by
> Kirill and it realy seems to provide a more compact diff.
> ---
> From 1f529769d099ca605888b29059014e7c8f0bfd50 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Fri, 8 Dec 2017 12:28:34 +0100
> Subject: [PATCH] mm, numa: rework do_pages_move
>
> do_pages_move is supposed to move user defined memory (an array of
> addresses) to the user defined numa nodes (an array of nodes one for
> each address). The user provided status array then contains resulting
> numa node for each address or an error. The semantic of this function is
> little bit confusing because only some errors are reported back. Notably
> migrate_pages error is only reported via the return value. This patch
> doesn't try to address these semantic nuances but rather change the
> underlying implementation.
>
> Currently we are processing user input (which can be really large)
> in batches which are stored to a temporarily allocated page. Each
> address is resolved to its struct page and stored to page_to_node
> structure along with the requested target numa node. The array of these
> structures is then conveyed down the page migration path via private
> argument. new_page_node then finds the corresponding structure and
> allocates the proper target page.
>
> What is the problem with the current implementation and why to change
> it? Apart from being quite ugly it also doesn't cope with unexpected
> pages showing up on the migration list inside migrate_pages path.
> That doesn't happen currently but the follow up patch would like to
> make the thp migration code more clear and that would need to split a
> THP into the list for some cases.
>
> How does the new implementation work? Well, instead of batching into a
> fixed size array we simply batch all pages that should be migrated to
> the same node and isolate all of them into a linked list which doesn't
> require any additional storage. This should work reasonably well because
> page migration usually migrates larger ranges of memory to a specific
> node. So the common case should work equally well as the current
> implementation. Even if somebody constructs an input where the target
> numa nodes would be interleaved we shouldn't see a large performance
> impact because page migration alone doesn't really benefit from
> batching. mmap_sem batching for the lookup is quite questionable and
> isolate_lru_page which would benefit from batching is not using it even
> in the current implementation.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/internal.h | 1 +
> mm/mempolicy.c | 5 +-
> mm/migrate.c | 340 ++++++++++++++++++++++++++-------------------------------
> 3 files changed, 156 insertions(+), 190 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index e6bd35182dae..1a1bb5d59c15 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -538,4 +538,5 @@ static inline bool is_migrate_highatomic_page(struct page *page)
> }
>
> void setup_zone_pageset(struct zone *zone);
> +extern struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x);
> #endif /* __MM_INTERNAL_H */
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index f604b22ebb65..66c9c79b21be 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -942,7 +942,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
> }
> }
>
> -static struct page *new_node_page(struct page *page, unsigned long node, int **x)
> +/* page allocation callback for NUMA node migration */
> +struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x)
> {
> if (PageHuge(page))
> return alloc_huge_page_node(page_hstate(compound_head(page)),
> @@ -986,7 +987,7 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
> flags | MPOL_MF_DISCONTIG_OK, &pagelist);
>
> if (!list_empty(&pagelist)) {
> - err = migrate_pages(&pagelist, new_node_page, NULL, dest,
> + err = migrate_pages(&pagelist, alloc_new_node_page, NULL, dest,
> MIGRATE_SYNC, MR_SYSCALL);
> if (err)
> putback_movable_pages(&pagelist);
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4d0be47a322a..9d7252ea2acd 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1444,141 +1444,104 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> }
>
> #ifdef CONFIG_NUMA
> +
> +static int store_status(int __user *status, int start, int value, int nr)
> +{
> + while (nr-- > 0) {
> + if (put_user(value, status + start))
> + return -EFAULT;
> + start++;
> + }
> +
> + return 0;
> +}
> +
> +static int do_move_pages_to_node(struct mm_struct *mm,
> + struct list_head *pagelist, int node)
> +{
> + int err;
> +
> + if (list_empty(pagelist))
> + return 0;
> +
> + err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
> + MIGRATE_SYNC, MR_SYSCALL);
> + if (err)
> + putback_movable_pages(pagelist);
> + return err;
> +}
> +
> /*
> - * Move a list of individual pages
> + * Resolves the given address to a struct page, isolates it from the LRU and
> + * puts it to the given pagelist.
> + * Returns -errno if the page cannot be found/isolated or 0 when it has been
> + * queued or the page doesn't need to be migrated because it is already on
> + * the target node
> */
> -struct page_to_node {
> - unsigned long addr;
> +static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
> + int node, struct list_head *pagelist, bool migrate_all)
> +{
> + struct vm_area_struct *vma;
> struct page *page;
> - int node;
> - int status;
> -};
> -
> -static struct page *new_page_node(struct page *p, unsigned long private,
> - int **result)
> -{
> - struct page_to_node *pm = (struct page_to_node *)private;
> -
> - while (pm->node != MAX_NUMNODES && pm->page != p)
> - pm++;
> -
> - if (pm->node == MAX_NUMNODES)
> - return NULL;
> -
> - *result = &pm->status;
> -
> - if (PageHuge(p))
> - return alloc_huge_page_node(page_hstate(compound_head(p)),
> - pm->node);
> - else if (thp_migration_supported() && PageTransHuge(p)) {
> - struct page *thp;
> -
> - thp = alloc_pages_node(pm->node,
> - (GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
> - HPAGE_PMD_ORDER);
> - if (!thp)
> - return NULL;
> - prep_transhuge_page(thp);
> - return thp;
> - } else
> - return __alloc_pages_node(pm->node,
> - GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
> -}
> -
> -/*
> - * Move a set of pages as indicated in the pm array. The addr
> - * field must be set to the virtual address of the page to be moved
> - * and the node number must contain a valid target node.
> - * The pm array ends with node = MAX_NUMNODES.
> - */
> -static int do_move_page_to_node_array(struct mm_struct *mm,
> - struct page_to_node *pm,
> - int migrate_all)
> -{
> + unsigned int follflags;
> int err;
> - struct page_to_node *pp;
> - LIST_HEAD(pagelist);
>
> down_read(&mm->mmap_sem);
> + err = -EFAULT;
> + vma = find_vma(mm, addr);
> + if (!vma || addr < vma->vm_start || !vma_migratable(vma))
> + goto out;
>
> - /*
> - * Build a list of pages to migrate
> - */
> - for (pp = pm; pp->node != MAX_NUMNODES; pp++) {
> - struct vm_area_struct *vma;
> - struct page *page;
> + /* FOLL_DUMP to ignore special (like zero) pages */
> + follflags = FOLL_GET | FOLL_DUMP;
> + if (!thp_migration_supported())
> + follflags |= FOLL_SPLIT;
> + page = follow_page(vma, addr, follflags);
> +
> + err = PTR_ERR(page);
> + if (IS_ERR(page))
> + goto out;
> +
> + err = -ENOENT;
> + if (!page)
> + goto out;
> +
> + err = 0;
> + if (page_to_nid(page) == node)
> + goto out_putpage;
> +
> + err = -EACCES;
> + if (page_mapcount(page) > 1 &&
> + !migrate_all)
Non-sensible line break.
> + goto out_putpage;
> +
> + if (PageHuge(page)) {
> + if (PageHead(page)) {
> + isolate_huge_page(page, pagelist);
> + err = 0;
> + }
> + } else {
Hm. I think if the page is PageTail() we have to split the huge page.
If an user asks to migrate part of THP, we shouldn't migrate the whole page,
otherwise it's not transparent anymore.
Otherwise, the patch looks good to me.
--
Kirill A. Shutemov
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2017-12-14 15:35 ` Kirill A. Shutemov
0 siblings, 0 replies; 61+ messages in thread
From: Kirill A. Shutemov @ 2017-12-14 15:35 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Wed, Dec 13, 2017 at 03:39:48PM +0100, Michal Hocko wrote:
> This patch has been generated with --patience parameter as suggested by
> Kirill and it realy seems to provide a more compact diff.
> ---
> From 1f529769d099ca605888b29059014e7c8f0bfd50 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Fri, 8 Dec 2017 12:28:34 +0100
> Subject: [PATCH] mm, numa: rework do_pages_move
>
> do_pages_move is supposed to move user defined memory (an array of
> addresses) to the user defined numa nodes (an array of nodes one for
> each address). The user provided status array then contains resulting
> numa node for each address or an error. The semantic of this function is
> little bit confusing because only some errors are reported back. Notably
> migrate_pages error is only reported via the return value. This patch
> doesn't try to address these semantic nuances but rather change the
> underlying implementation.
>
> Currently we are processing user input (which can be really large)
> in batches which are stored to a temporarily allocated page. Each
> address is resolved to its struct page and stored to page_to_node
> structure along with the requested target numa node. The array of these
> structures is then conveyed down the page migration path via private
> argument. new_page_node then finds the corresponding structure and
> allocates the proper target page.
>
> What is the problem with the current implementation and why to change
> it? Apart from being quite ugly it also doesn't cope with unexpected
> pages showing up on the migration list inside migrate_pages path.
> That doesn't happen currently but the follow up patch would like to
> make the thp migration code more clear and that would need to split a
> THP into the list for some cases.
>
> How does the new implementation work? Well, instead of batching into a
> fixed size array we simply batch all pages that should be migrated to
> the same node and isolate all of them into a linked list which doesn't
> require any additional storage. This should work reasonably well because
> page migration usually migrates larger ranges of memory to a specific
> node. So the common case should work equally well as the current
> implementation. Even if somebody constructs an input where the target
> numa nodes would be interleaved we shouldn't see a large performance
> impact because page migration alone doesn't really benefit from
> batching. mmap_sem batching for the lookup is quite questionable and
> isolate_lru_page which would benefit from batching is not using it even
> in the current implementation.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/internal.h | 1 +
> mm/mempolicy.c | 5 +-
> mm/migrate.c | 340 ++++++++++++++++++++++++++-------------------------------
> 3 files changed, 156 insertions(+), 190 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index e6bd35182dae..1a1bb5d59c15 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -538,4 +538,5 @@ static inline bool is_migrate_highatomic_page(struct page *page)
> }
>
> void setup_zone_pageset(struct zone *zone);
> +extern struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x);
> #endif /* __MM_INTERNAL_H */
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index f604b22ebb65..66c9c79b21be 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -942,7 +942,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
> }
> }
>
> -static struct page *new_node_page(struct page *page, unsigned long node, int **x)
> +/* page allocation callback for NUMA node migration */
> +struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x)
> {
> if (PageHuge(page))
> return alloc_huge_page_node(page_hstate(compound_head(page)),
> @@ -986,7 +987,7 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
> flags | MPOL_MF_DISCONTIG_OK, &pagelist);
>
> if (!list_empty(&pagelist)) {
> - err = migrate_pages(&pagelist, new_node_page, NULL, dest,
> + err = migrate_pages(&pagelist, alloc_new_node_page, NULL, dest,
> MIGRATE_SYNC, MR_SYSCALL);
> if (err)
> putback_movable_pages(&pagelist);
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4d0be47a322a..9d7252ea2acd 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1444,141 +1444,104 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> }
>
> #ifdef CONFIG_NUMA
> +
> +static int store_status(int __user *status, int start, int value, int nr)
> +{
> + while (nr-- > 0) {
> + if (put_user(value, status + start))
> + return -EFAULT;
> + start++;
> + }
> +
> + return 0;
> +}
> +
> +static int do_move_pages_to_node(struct mm_struct *mm,
> + struct list_head *pagelist, int node)
> +{
> + int err;
> +
> + if (list_empty(pagelist))
> + return 0;
> +
> + err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
> + MIGRATE_SYNC, MR_SYSCALL);
> + if (err)
> + putback_movable_pages(pagelist);
> + return err;
> +}
> +
> /*
> - * Move a list of individual pages
> + * Resolves the given address to a struct page, isolates it from the LRU and
> + * puts it to the given pagelist.
> + * Returns -errno if the page cannot be found/isolated or 0 when it has been
> + * queued or the page doesn't need to be migrated because it is already on
> + * the target node
> */
> -struct page_to_node {
> - unsigned long addr;
> +static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
> + int node, struct list_head *pagelist, bool migrate_all)
> +{
> + struct vm_area_struct *vma;
> struct page *page;
> - int node;
> - int status;
> -};
> -
> -static struct page *new_page_node(struct page *p, unsigned long private,
> - int **result)
> -{
> - struct page_to_node *pm = (struct page_to_node *)private;
> -
> - while (pm->node != MAX_NUMNODES && pm->page != p)
> - pm++;
> -
> - if (pm->node == MAX_NUMNODES)
> - return NULL;
> -
> - *result = &pm->status;
> -
> - if (PageHuge(p))
> - return alloc_huge_page_node(page_hstate(compound_head(p)),
> - pm->node);
> - else if (thp_migration_supported() && PageTransHuge(p)) {
> - struct page *thp;
> -
> - thp = alloc_pages_node(pm->node,
> - (GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
> - HPAGE_PMD_ORDER);
> - if (!thp)
> - return NULL;
> - prep_transhuge_page(thp);
> - return thp;
> - } else
> - return __alloc_pages_node(pm->node,
> - GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
> -}
> -
> -/*
> - * Move a set of pages as indicated in the pm array. The addr
> - * field must be set to the virtual address of the page to be moved
> - * and the node number must contain a valid target node.
> - * The pm array ends with node = MAX_NUMNODES.
> - */
> -static int do_move_page_to_node_array(struct mm_struct *mm,
> - struct page_to_node *pm,
> - int migrate_all)
> -{
> + unsigned int follflags;
> int err;
> - struct page_to_node *pp;
> - LIST_HEAD(pagelist);
>
> down_read(&mm->mmap_sem);
> + err = -EFAULT;
> + vma = find_vma(mm, addr);
> + if (!vma || addr < vma->vm_start || !vma_migratable(vma))
> + goto out;
>
> - /*
> - * Build a list of pages to migrate
> - */
> - for (pp = pm; pp->node != MAX_NUMNODES; pp++) {
> - struct vm_area_struct *vma;
> - struct page *page;
> + /* FOLL_DUMP to ignore special (like zero) pages */
> + follflags = FOLL_GET | FOLL_DUMP;
> + if (!thp_migration_supported())
> + follflags |= FOLL_SPLIT;
> + page = follow_page(vma, addr, follflags);
> +
> + err = PTR_ERR(page);
> + if (IS_ERR(page))
> + goto out;
> +
> + err = -ENOENT;
> + if (!page)
> + goto out;
> +
> + err = 0;
> + if (page_to_nid(page) == node)
> + goto out_putpage;
> +
> + err = -EACCES;
> + if (page_mapcount(page) > 1 &&
> + !migrate_all)
Non-sensible line break.
> + goto out_putpage;
> +
> + if (PageHuge(page)) {
> + if (PageHead(page)) {
> + isolate_huge_page(page, pagelist);
> + err = 0;
> + }
> + } else {
Hm. I think if the page is PageTail() we have to split the huge page.
If an user asks to migrate part of THP, we shouldn't migrate the whole page,
otherwise it's not transparent anymore.
Otherwise, the patch looks good to me.
--
Kirill A. Shutemov
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-14 15:35 ` Kirill A. Shutemov
@ 2017-12-15 9:28 ` Michal Hocko
-1 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2017-12-15 9:28 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Thu 14-12-17 18:35:58, Kirill A. Shutemov wrote:
> On Wed, Dec 13, 2017 at 03:39:48PM +0100, Michal Hocko wrote:
[...]
> > + err = 0;
> > + if (page_to_nid(page) == node)
> > + goto out_putpage;
> > +
> > + err = -EACCES;
> > + if (page_mapcount(page) > 1 &&
> > + !migrate_all)
>
> Non-sensible line break.
fixed
> > + goto out_putpage;
> > +
> > + if (PageHuge(page)) {
> > + if (PageHead(page)) {
> > + isolate_huge_page(page, pagelist);
> > + err = 0;
> > + }
> > + } else {
>
> Hm. I think if the page is PageTail() we have to split the huge page.
> If an user asks to migrate part of THP, we shouldn't migrate the whole page,
> otherwise it's not transparent anymore.
Well, as I've said in the cover letter. There are more things which are
worth considering but I've tried to keep the original semantic so
further changes should be done in separete patches. I will work on those
but I would prefer this to stay smaller if you do not mind.
> Otherwise, the patch looks good to me.
Thanks for the review
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2017-12-15 9:28 ` Michal Hocko
0 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2017-12-15 9:28 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Thu 14-12-17 18:35:58, Kirill A. Shutemov wrote:
> On Wed, Dec 13, 2017 at 03:39:48PM +0100, Michal Hocko wrote:
[...]
> > + err = 0;
> > + if (page_to_nid(page) == node)
> > + goto out_putpage;
> > +
> > + err = -EACCES;
> > + if (page_mapcount(page) > 1 &&
> > + !migrate_all)
>
> Non-sensible line break.
fixed
> > + goto out_putpage;
> > +
> > + if (PageHuge(page)) {
> > + if (PageHead(page)) {
> > + isolate_huge_page(page, pagelist);
> > + err = 0;
> > + }
> > + } else {
>
> Hm. I think if the page is PageTail() we have to split the huge page.
> If an user asks to migrate part of THP, we shouldn't migrate the whole page,
> otherwise it's not transparent anymore.
Well, as I've said in the cover letter. There are more things which are
worth considering but I've tried to keep the original semantic so
further changes should be done in separete patches. I will work on those
but I would prefer this to stay smaller if you do not mind.
> Otherwise, the patch looks good to me.
Thanks for the review
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-15 9:28 ` Michal Hocko
@ 2017-12-15 9:51 ` Kirill A. Shutemov
-1 siblings, 0 replies; 61+ messages in thread
From: Kirill A. Shutemov @ 2017-12-15 9:51 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Fri, Dec 15, 2017 at 10:28:59AM +0100, Michal Hocko wrote:
> On Thu 14-12-17 18:35:58, Kirill A. Shutemov wrote:
> > On Wed, Dec 13, 2017 at 03:39:48PM +0100, Michal Hocko wrote:
> [...]
> > > + err = 0;
> > > + if (page_to_nid(page) == node)
> > > + goto out_putpage;
> > > +
> > > + err = -EACCES;
> > > + if (page_mapcount(page) > 1 &&
> > > + !migrate_all)
> >
> > Non-sensible line break.
>
> fixed
>
> > > + goto out_putpage;
> > > +
> > > + if (PageHuge(page)) {
> > > + if (PageHead(page)) {
> > > + isolate_huge_page(page, pagelist);
> > > + err = 0;
> > > + }
> > > + } else {
> >
> > Hm. I think if the page is PageTail() we have to split the huge page.
> > If an user asks to migrate part of THP, we shouldn't migrate the whole page,
> > otherwise it's not transparent anymore.
>
> Well, as I've said in the cover letter. There are more things which are
> worth considering but I've tried to keep the original semantic so
> further changes should be done in separete patches. I will work on those
> but I would prefer this to stay smaller if you do not mind.
Sure.
Fill free to use my ack.
--
Kirill A. Shutemov
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2017-12-15 9:51 ` Kirill A. Shutemov
0 siblings, 0 replies; 61+ messages in thread
From: Kirill A. Shutemov @ 2017-12-15 9:51 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Fri, Dec 15, 2017 at 10:28:59AM +0100, Michal Hocko wrote:
> On Thu 14-12-17 18:35:58, Kirill A. Shutemov wrote:
> > On Wed, Dec 13, 2017 at 03:39:48PM +0100, Michal Hocko wrote:
> [...]
> > > + err = 0;
> > > + if (page_to_nid(page) == node)
> > > + goto out_putpage;
> > > +
> > > + err = -EACCES;
> > > + if (page_mapcount(page) > 1 &&
> > > + !migrate_all)
> >
> > Non-sensible line break.
>
> fixed
>
> > > + goto out_putpage;
> > > +
> > > + if (PageHuge(page)) {
> > > + if (PageHead(page)) {
> > > + isolate_huge_page(page, pagelist);
> > > + err = 0;
> > > + }
> > > + } else {
> >
> > Hm. I think if the page is PageTail() we have to split the huge page.
> > If an user asks to migrate part of THP, we shouldn't migrate the whole page,
> > otherwise it's not transparent anymore.
>
> Well, as I've said in the cover letter. There are more things which are
> worth considering but I've tried to keep the original semantic so
> further changes should be done in separete patches. I will work on those
> but I would prefer this to stay smaller if you do not mind.
Sure.
Fill free to use my ack.
--
Kirill A. Shutemov
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-15 9:51 ` Kirill A. Shutemov
@ 2017-12-15 9:57 ` Michal Hocko
-1 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2017-12-15 9:57 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Fri 15-12-17 12:51:25, Kirill A. Shutemov wrote:
> On Fri, Dec 15, 2017 at 10:28:59AM +0100, Michal Hocko wrote:
> > On Thu 14-12-17 18:35:58, Kirill A. Shutemov wrote:
> > > On Wed, Dec 13, 2017 at 03:39:48PM +0100, Michal Hocko wrote:
> > [...]
> > > > + err = 0;
> > > > + if (page_to_nid(page) == node)
> > > > + goto out_putpage;
> > > > +
> > > > + err = -EACCES;
> > > > + if (page_mapcount(page) > 1 &&
> > > > + !migrate_all)
> > >
> > > Non-sensible line break.
> >
> > fixed
> >
> > > > + goto out_putpage;
> > > > +
> > > > + if (PageHuge(page)) {
> > > > + if (PageHead(page)) {
> > > > + isolate_huge_page(page, pagelist);
> > > > + err = 0;
> > > > + }
> > > > + } else {
> > >
> > > Hm. I think if the page is PageTail() we have to split the huge page.
> > > If an user asks to migrate part of THP, we shouldn't migrate the whole page,
> > > otherwise it's not transparent anymore.
> >
> > Well, as I've said in the cover letter. There are more things which are
> > worth considering but I've tried to keep the original semantic so
> > further changes should be done in separete patches. I will work on those
> > but I would prefer this to stay smaller if you do not mind.
>
> Sure.
>
> Fill free to use my ack.
Thanks a lot Kirill! I will wait for some more feedback and then
resubmit later next week.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2017-12-15 9:57 ` Michal Hocko
0 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2017-12-15 9:57 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML
On Fri 15-12-17 12:51:25, Kirill A. Shutemov wrote:
> On Fri, Dec 15, 2017 at 10:28:59AM +0100, Michal Hocko wrote:
> > On Thu 14-12-17 18:35:58, Kirill A. Shutemov wrote:
> > > On Wed, Dec 13, 2017 at 03:39:48PM +0100, Michal Hocko wrote:
> > [...]
> > > > + err = 0;
> > > > + if (page_to_nid(page) == node)
> > > > + goto out_putpage;
> > > > +
> > > > + err = -EACCES;
> > > > + if (page_mapcount(page) > 1 &&
> > > > + !migrate_all)
> > >
> > > Non-sensible line break.
> >
> > fixed
> >
> > > > + goto out_putpage;
> > > > +
> > > > + if (PageHuge(page)) {
> > > > + if (PageHead(page)) {
> > > > + isolate_huge_page(page, pagelist);
> > > > + err = 0;
> > > > + }
> > > > + } else {
> > >
> > > Hm. I think if the page is PageTail() we have to split the huge page.
> > > If an user asks to migrate part of THP, we shouldn't migrate the whole page,
> > > otherwise it's not transparent anymore.
> >
> > Well, as I've said in the cover letter. There are more things which are
> > worth considering but I've tried to keep the original semantic so
> > further changes should be done in separete patches. I will work on those
> > but I would prefer this to stay smaller if you do not mind.
>
> Sure.
>
> Fill free to use my ack.
Thanks a lot Kirill! I will wait for some more feedback and then
resubmit later next week.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-08 16:15 ` Michal Hocko
@ 2018-01-02 11:25 ` Anshuman Khandual
-1 siblings, 0 replies; 61+ messages in thread
From: Anshuman Khandual @ 2018-01-02 11:25 UTC (permalink / raw)
To: Michal Hocko, linux-mm
Cc: Zi Yan, Naoya Horiguchi, Kirill A. Shutemov, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML, Michal Hocko
On 12/08/2017 09:45 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> do_pages_move is supposed to move user defined memory (an array of
> addresses) to the user defined numa nodes (an array of nodes one for
> each address). The user provided status array then contains resulting
> numa node for each address or an error. The semantic of this function is
> little bit confusing because only some errors are reported back. Notably
> migrate_pages error is only reported via the return value. This patch
It does report back the migration failures as well. In new_page_node
there is '*result = &pm->status' which going forward in unmap_and_move
will hold migration error or node ID of the new page.
newpage = get_new_page(page, private, &result);
............
if (result) {
if (rc)
*result = rc;
else
*result = page_to_nid(newpage);
}
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2018-01-02 11:25 ` Anshuman Khandual
0 siblings, 0 replies; 61+ messages in thread
From: Anshuman Khandual @ 2018-01-02 11:25 UTC (permalink / raw)
To: Michal Hocko, linux-mm
Cc: Zi Yan, Naoya Horiguchi, Kirill A. Shutemov, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML, Michal Hocko
On 12/08/2017 09:45 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> do_pages_move is supposed to move user defined memory (an array of
> addresses) to the user defined numa nodes (an array of nodes one for
> each address). The user provided status array then contains resulting
> numa node for each address or an error. The semantic of this function is
> little bit confusing because only some errors are reported back. Notably
> migrate_pages error is only reported via the return value. This patch
It does report back the migration failures as well. In new_page_node
there is '*result = &pm->status' which going forward in unmap_and_move
will hold migration error or node ID of the new page.
newpage = get_new_page(page, private, &result);
............
if (result) {
if (rc)
*result = rc;
else
*result = page_to_nid(newpage);
}
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2018-01-02 11:25 ` Anshuman Khandual
@ 2018-01-02 12:12 ` Michal Hocko
-1 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2018-01-02 12:12 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Kirill A. Shutemov,
Vlastimil Babka, Andrew Morton, Andrea Reale, LKML
On Tue 02-01-18 16:55:46, Anshuman Khandual wrote:
> On 12/08/2017 09:45 PM, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> >
> > do_pages_move is supposed to move user defined memory (an array of
> > addresses) to the user defined numa nodes (an array of nodes one for
> > each address). The user provided status array then contains resulting
> > numa node for each address or an error. The semantic of this function is
> > little bit confusing because only some errors are reported back. Notably
> > migrate_pages error is only reported via the return value. This patch
>
> It does report back the migration failures as well. In new_page_node
> there is '*result = &pm->status' which going forward in unmap_and_move
> will hold migration error or node ID of the new page.
>
> newpage = get_new_page(page, private, &result);
> ............
> if (result) {
> if (rc)
> *result = rc;
> else
> *result = page_to_nid(newpage);
> }
>
This is true, except the user will not get this information. Have a look
how we do not copy status on error up in the do_pages_move layer.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2018-01-02 12:12 ` Michal Hocko
0 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2018-01-02 12:12 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Kirill A. Shutemov,
Vlastimil Babka, Andrew Morton, Andrea Reale, LKML
On Tue 02-01-18 16:55:46, Anshuman Khandual wrote:
> On 12/08/2017 09:45 PM, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> >
> > do_pages_move is supposed to move user defined memory (an array of
> > addresses) to the user defined numa nodes (an array of nodes one for
> > each address). The user provided status array then contains resulting
> > numa node for each address or an error. The semantic of this function is
> > little bit confusing because only some errors are reported back. Notably
> > migrate_pages error is only reported via the return value. This patch
>
> It does report back the migration failures as well. In new_page_node
> there is '*result = &pm->status' which going forward in unmap_and_move
> will hold migration error or node ID of the new page.
>
> newpage = get_new_page(page, private, &result);
> ............
> if (result) {
> if (rc)
> *result = rc;
> else
> *result = page_to_nid(newpage);
> }
>
This is true, except the user will not get this information. Have a look
how we do not copy status on error up in the do_pages_move layer.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2018-01-02 12:12 ` Michal Hocko
@ 2018-01-03 3:11 ` Anshuman Khandual
-1 siblings, 0 replies; 61+ messages in thread
From: Anshuman Khandual @ 2018-01-03 3:11 UTC (permalink / raw)
To: Michal Hocko, Anshuman Khandual
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Kirill A. Shutemov,
Vlastimil Babka, Andrew Morton, Andrea Reale, LKML
On 01/02/2018 05:42 PM, Michal Hocko wrote:
> On Tue 02-01-18 16:55:46, Anshuman Khandual wrote:
>> On 12/08/2017 09:45 PM, Michal Hocko wrote:
>>> From: Michal Hocko <mhocko@suse.com>
>>>
>>> do_pages_move is supposed to move user defined memory (an array of
>>> addresses) to the user defined numa nodes (an array of nodes one for
>>> each address). The user provided status array then contains resulting
>>> numa node for each address or an error. The semantic of this function is
>>> little bit confusing because only some errors are reported back. Notably
>>> migrate_pages error is only reported via the return value. This patch
>>
>> It does report back the migration failures as well. In new_page_node
>> there is '*result = &pm->status' which going forward in unmap_and_move
>> will hold migration error or node ID of the new page.
>>
>> newpage = get_new_page(page, private, &result);
>> ............
>> if (result) {
>> if (rc)
>> *result = rc;
>> else
>> *result = page_to_nid(newpage);
>> }
>>
>
> This is true, except the user will not get this information. Have a look
> how we do not copy status on error up in the do_pages_move layer.
Ahh, right, we dont. But as you have mentioned this patch does not
intend to change the semantics of status return thought it seems
like the right thing to do. We can just pass on the status to user
here before bailing out.
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2018-01-03 3:11 ` Anshuman Khandual
0 siblings, 0 replies; 61+ messages in thread
From: Anshuman Khandual @ 2018-01-03 3:11 UTC (permalink / raw)
To: Michal Hocko, Anshuman Khandual
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Kirill A. Shutemov,
Vlastimil Babka, Andrew Morton, Andrea Reale, LKML
On 01/02/2018 05:42 PM, Michal Hocko wrote:
> On Tue 02-01-18 16:55:46, Anshuman Khandual wrote:
>> On 12/08/2017 09:45 PM, Michal Hocko wrote:
>>> From: Michal Hocko <mhocko@suse.com>
>>>
>>> do_pages_move is supposed to move user defined memory (an array of
>>> addresses) to the user defined numa nodes (an array of nodes one for
>>> each address). The user provided status array then contains resulting
>>> numa node for each address or an error. The semantic of this function is
>>> little bit confusing because only some errors are reported back. Notably
>>> migrate_pages error is only reported via the return value. This patch
>>
>> It does report back the migration failures as well. In new_page_node
>> there is '*result = &pm->status' which going forward in unmap_and_move
>> will hold migration error or node ID of the new page.
>>
>> newpage = get_new_page(page, private, &result);
>> ............
>> if (result) {
>> if (rc)
>> *result = rc;
>> else
>> *result = page_to_nid(newpage);
>> }
>>
>
> This is true, except the user will not get this information. Have a look
> how we do not copy status on error up in the do_pages_move layer.
Ahh, right, we dont. But as you have mentioned this patch does not
intend to change the semantics of status return thought it seems
like the right thing to do. We can just pass on the status to user
here before bailing out.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2017-12-08 16:15 ` Michal Hocko
@ 2018-01-03 8:42 ` Anshuman Khandual
-1 siblings, 0 replies; 61+ messages in thread
From: Anshuman Khandual @ 2018-01-03 8:42 UTC (permalink / raw)
To: Michal Hocko, linux-mm
Cc: Zi Yan, Naoya Horiguchi, Kirill A. Shutemov, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML, Michal Hocko
On 12/08/2017 09:45 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> do_pages_move is supposed to move user defined memory (an array of
> addresses) to the user defined numa nodes (an array of nodes one for
> each address). The user provided status array then contains resulting
> numa node for each address or an error. The semantic of this function is
> little bit confusing because only some errors are reported back. Notably
> migrate_pages error is only reported via the return value. This patch
> doesn't try to address these semantic nuances but rather change the
> underlying implementation.
>
> Currently we are processing user input (which can be really large)
> in batches which are stored to a temporarily allocated page. Each
> address is resolved to its struct page and stored to page_to_node
> structure along with the requested target numa node. The array of these
> structures is then conveyed down the page migration path via private
> argument. new_page_node then finds the corresponding structure and
> allocates the proper target page.
>
> What is the problem with the current implementation and why to change
> it? Apart from being quite ugly it also doesn't cope with unexpected
> pages showing up on the migration list inside migrate_pages path.
> That doesn't happen currently but the follow up patch would like to
> make the thp migration code more clear and that would need to split a
> THP into the list for some cases.
>
> How does the new implementation work? Well, instead of batching into a
> fixed size array we simply batch all pages that should be migrated to
> the same node and isolate all of them into a linked list which doesn't
> require any additional storage. This should work reasonably well because
> page migration usually migrates larger ranges of memory to a specific
> node. So the common case should work equally well as the current
> implementation. Even if somebody constructs an input where the target
> numa nodes would be interleaved we shouldn't see a large performance
> impact because page migration alone doesn't really benefit from
> batching. mmap_sem batching for the lookup is quite questionable and
> isolate_lru_page which would benefit from batching is not using it even
> in the current implementation.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/internal.h | 1 +
> mm/mempolicy.c | 5 +-
> mm/migrate.c | 306 +++++++++++++++++++++++++--------------------------------
> 3 files changed, 139 insertions(+), 173 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index e6bd35182dae..1a1bb5d59c15 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -538,4 +538,5 @@ static inline bool is_migrate_highatomic_page(struct page *page)
> }
>
> void setup_zone_pageset(struct zone *zone);
> +extern struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x);
> #endif /* __MM_INTERNAL_H */
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index f604b22ebb65..66c9c79b21be 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -942,7 +942,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
> }
> }
>
> -static struct page *new_node_page(struct page *page, unsigned long node, int **x)
> +/* page allocation callback for NUMA node migration */
> +struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x)
> {
> if (PageHuge(page))
> return alloc_huge_page_node(page_hstate(compound_head(page)),
> @@ -986,7 +987,7 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
> flags | MPOL_MF_DISCONTIG_OK, &pagelist);
>
> if (!list_empty(&pagelist)) {
> - err = migrate_pages(&pagelist, new_node_page, NULL, dest,
> + err = migrate_pages(&pagelist, alloc_new_node_page, NULL, dest,
> MIGRATE_SYNC, MR_SYSCALL);
> if (err)
> putback_movable_pages(&pagelist);
This reuses the existing page allocation helper from migrate_pages() system
call. But all these allocator helper names for migrate_pages() function are
really confusing. Even in this case alloc_new_node_page and the original
new_node_page() which is still getting used in do_migrate_range() sounds
similar even if their implementation is quite different. IMHO either all of
them should be moved to the header file with proper differentiating names
or let them be there in their respective files with these generic names and
clean them up later.
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4d0be47a322a..9d7252ea2acd 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1444,141 +1444,104 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> }
>
> #ifdef CONFIG_NUMA
> -/*
> - * Move a list of individual pages
> - */
> -struct page_to_node {
> - unsigned long addr;
> - struct page *page;
> - int node;
> - int status;
> -};
>
> -static struct page *new_page_node(struct page *p, unsigned long private,
> - int **result)
> +static int store_status(int __user *status, int start, int value, int nr)
> {
> - struct page_to_node *pm = (struct page_to_node *)private;
> -
> - while (pm->node != MAX_NUMNODES && pm->page != p)
> - pm++;
> + while (nr-- > 0) {
> + if (put_user(value, status + start))
> + return -EFAULT;
> + start++;
> + }
>
> - if (pm->node == MAX_NUMNODES)
> - return NULL;
> + return 0;
> +}
Just a nit. new_page_node() and store_status() seems different. Then why
the git diff looks so clumsy.
>
> - *result = &pm->status;
> +static int do_move_pages_to_node(struct mm_struct *mm,
> + struct list_head *pagelist, int node)
> +{
> + int err;
>
> - if (PageHuge(p))
> - return alloc_huge_page_node(page_hstate(compound_head(p)),
> - pm->node);
> - else if (thp_migration_supported() && PageTransHuge(p)) {
> - struct page *thp;
> + if (list_empty(pagelist))
> + return 0;
>
> - thp = alloc_pages_node(pm->node,
> - (GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
> - HPAGE_PMD_ORDER);
> - if (!thp)
> - return NULL;
> - prep_transhuge_page(thp);
> - return thp;
> - } else
> - return __alloc_pages_node(pm->node,
> - GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
> + err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
> + MIGRATE_SYNC, MR_SYSCALL);
> + if (err)
> + putback_movable_pages(pagelist);
> + return err;
> }
Even this one. IIUC, do_move_pages_to_node() migrate a chunk of pages
at a time which belong to the same target node. Perhaps the name should
suggest so. All these helper page migration helper functions sound so
similar.
>
> /*
> - * Move a set of pages as indicated in the pm array. The addr
> - * field must be set to the virtual address of the page to be moved
> - * and the node number must contain a valid target node.
> - * The pm array ends with node = MAX_NUMNODES.
> + * Resolves the given address to a struct page, isolates it from the LRU and
> + * puts it to the given pagelist.
> + * Returns -errno if the page cannot be found/isolated or 0 when it has been
> + * queued or the page doesn't need to be migrated because it is already on
> + * the target node
> */
> -static int do_move_page_to_node_array(struct mm_struct *mm,
> - struct page_to_node *pm,
> - int migrate_all)
> +static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
> + int node, struct list_head *pagelist, bool migrate_all)
> {
> + struct vm_area_struct *vma;
> + struct page *page;
> + unsigned int follflags;
> int err;
> - struct page_to_node *pp;
> - LIST_HEAD(pagelist);
>
> down_read(&mm->mmap_sem);
Holding mmap_sem for individual pages makes sense. Current
implementation is holding it for an entire batch.
> + err = -EFAULT;
> + vma = find_vma(mm, addr);
> + if (!vma || addr < vma->vm_start || !vma_migratable(vma))
While here, should not we add 'addr > vma->vm_end' into this condition ?
> + goto out;
>
> - /*
> - * Build a list of pages to migrate
> - */
> - for (pp = pm; pp->node != MAX_NUMNODES; pp++) {
> - struct vm_area_struct *vma;
> - struct page *page;
> - struct page *head;
> - unsigned int follflags;
> -
> - err = -EFAULT;
> - vma = find_vma(mm, pp->addr);
> - if (!vma || pp->addr < vma->vm_start || !vma_migratable(vma))
> - goto set_status;
> -
> - /* FOLL_DUMP to ignore special (like zero) pages */
> - follflags = FOLL_GET | FOLL_DUMP;
> - if (!thp_migration_supported())
> - follflags |= FOLL_SPLIT;
> - page = follow_page(vma, pp->addr, follflags);
> + /* FOLL_DUMP to ignore special (like zero) pages */
> + follflags = FOLL_GET | FOLL_DUMP;
> + if (!thp_migration_supported())
> + follflags |= FOLL_SPLIT;
> + page = follow_page(vma, addr, follflags);
>
> - err = PTR_ERR(page);
> - if (IS_ERR(page))
> - goto set_status;
> + err = PTR_ERR(page);
> + if (IS_ERR(page))
> + goto out;
>
> - err = -ENOENT;
> - if (!page)
> - goto set_status;
> + err = -ENOENT;
> + if (!page)
> + goto out;
>
> - err = page_to_nid(page);
> + err = 0;
> + if (page_to_nid(page) == node)
> + goto out_putpage;
>
> - if (err == pp->node)
> - /*
> - * Node already in the right place
> - */
> - goto put_and_set;
> + err = -EACCES;
> + if (page_mapcount(page) > 1 &&
> + !migrate_all)
> + goto out_putpage;
>
> - err = -EACCES;
> - if (page_mapcount(page) > 1 &&
> - !migrate_all)
> - goto put_and_set;
> -
> - if (PageHuge(page)) {
> - if (PageHead(page)) {
> - isolate_huge_page(page, &pagelist);
> - err = 0;
> - pp->page = page;
> - }
> - goto put_and_set;
> + if (PageHuge(page)) {
> + if (PageHead(page)) {
> + isolate_huge_page(page, pagelist);
> + err = 0;
> }
> + } else {
> + struct page *head;
>
> - pp->page = compound_head(page);
> head = compound_head(page);
> err = isolate_lru_page(head);
> - if (!err) {
> - list_add_tail(&head->lru, &pagelist);
> - mod_node_page_state(page_pgdat(head),
> - NR_ISOLATED_ANON + page_is_file_cache(head),
> - hpage_nr_pages(head));
> - }
> -put_and_set:
> - /*
> - * Either remove the duplicate refcount from
> - * isolate_lru_page() or drop the page ref if it was
> - * not isolated.
> - */
> - put_page(page);
> -set_status:
> - pp->status = err;
> - }
> -
> - err = 0;
> - if (!list_empty(&pagelist)) {
> - err = migrate_pages(&pagelist, new_page_node, NULL,
> - (unsigned long)pm, MIGRATE_SYNC, MR_SYSCALL);
> if (err)
> - putback_movable_pages(&pagelist);
> - }
> + goto out_putpage;
>
> + err = 0;
> + list_add_tail(&head->lru, pagelist);
> + mod_node_page_state(page_pgdat(head),
> + NR_ISOLATED_ANON + page_is_file_cache(head),
> + hpage_nr_pages(head));
> + }
> +out_putpage:
> + /*
> + * Either remove the duplicate refcount from
> + * isolate_lru_page() or drop the page ref if it was
> + * not isolated.
> + */
> + put_page(page);
> +out:
> up_read(&mm->mmap_sem);
> return err;
> }
> @@ -1593,79 +1556,80 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> const int __user *nodes,
> int __user *status, int flags)
> {
> - struct page_to_node *pm;
> - unsigned long chunk_nr_pages;
> - unsigned long chunk_start;
> - int err;
> -
> - err = -ENOMEM;
> - pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
> - if (!pm)
> - goto out;
> + int chunk_node = NUMA_NO_NODE;
> + LIST_HEAD(pagelist);
> + int chunk_start, i;
> + int err = 0, err1;
err init might not be required, its getting assigned to -EFAULT right away.
>
> migrate_prep();
>
> - /*
> - * Store a chunk of page_to_node array in a page,
> - * but keep the last one as a marker
> - */
> - chunk_nr_pages = (PAGE_SIZE / sizeof(struct page_to_node)) - 1;
> -
> - for (chunk_start = 0;
> - chunk_start < nr_pages;
> - chunk_start += chunk_nr_pages) {
> - int j;
> -
> - if (chunk_start + chunk_nr_pages > nr_pages)
> - chunk_nr_pages = nr_pages - chunk_start;
> -
> - /* fill the chunk pm with addrs and nodes from user-space */
> - for (j = 0; j < chunk_nr_pages; j++) {
> - const void __user *p;
> - int node;
> -
> - err = -EFAULT;
> - if (get_user(p, pages + j + chunk_start))
> - goto out_pm;
> - pm[j].addr = (unsigned long) p;
> -
> - if (get_user(node, nodes + j + chunk_start))
> - goto out_pm;
> -
> - err = -ENODEV;
> - if (node < 0 || node >= MAX_NUMNODES)
> - goto out_pm;
> + for (i = chunk_start = 0; i < nr_pages; i++) {
> + const void __user *p;
> + unsigned long addr;
> + int node;
>
> - if (!node_state(node, N_MEMORY))
> - goto out_pm;
> -
> - err = -EACCES;
> - if (!node_isset(node, task_nodes))
> - goto out_pm;
> + err = -EFAULT;
> + if (get_user(p, pages + i))
> + goto out_flush;
> + if (get_user(node, nodes + i))
> + goto out_flush;
> + addr = (unsigned long)p;
> +
> + err = -ENODEV;
> + if (node < 0 || node >= MAX_NUMNODES)
> + goto out_flush;
> + if (!node_state(node, N_MEMORY))
> + goto out_flush;
>
> - pm[j].node = node;
> + err = -EACCES;
> + if (!node_isset(node, task_nodes))
> + goto out_flush;
> +
> + if (chunk_node == NUMA_NO_NODE) {
> + chunk_node = node;
> + chunk_start = i;
> + } else if (node != chunk_node) {
> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> + if (err)
> + goto out;
> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> + if (err)
> + goto out;
> + chunk_start = i;
> + chunk_node = node;
> }
>
> - /* End marker for this chunk */
> - pm[chunk_nr_pages].node = MAX_NUMNODES;
> + /*
> + * Errors in the page lookup or isolation are not fatal and we simply
> + * report them via status
> + */
> + err = add_page_for_migration(mm, addr, chunk_node,
> + &pagelist, flags & MPOL_MF_MOVE_ALL);
> + if (!err)
> + continue;
>
> - /* Migrate this chunk */
> - err = do_move_page_to_node_array(mm, pm,
> - flags & MPOL_MF_MOVE_ALL);
> - if (err < 0)
> - goto out_pm;
> + err = store_status(status, i, err, 1);
> + if (err)
> + goto out_flush;
>
> - /* Return status information */
> - for (j = 0; j < chunk_nr_pages; j++)
> - if (put_user(pm[j].status, status + j + chunk_start)) {
> - err = -EFAULT;
> - goto out_pm;
> - }
> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> + if (err)
> + goto out;
> + if (i > chunk_start) {
> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> + if (err)
> + goto out;
> + }
> + chunk_node = NUMA_NO_NODE;
This block of code is bit confusing.
1) Why attempt to migrate when just one page could not be isolated ?
2) 'i' is always greater than chunk_start except the starting page
3) Why reset chunk_node as NUMA_NO_NODE ?
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2018-01-03 8:42 ` Anshuman Khandual
0 siblings, 0 replies; 61+ messages in thread
From: Anshuman Khandual @ 2018-01-03 8:42 UTC (permalink / raw)
To: Michal Hocko, linux-mm
Cc: Zi Yan, Naoya Horiguchi, Kirill A. Shutemov, Vlastimil Babka,
Andrew Morton, Andrea Reale, LKML, Michal Hocko
On 12/08/2017 09:45 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> do_pages_move is supposed to move user defined memory (an array of
> addresses) to the user defined numa nodes (an array of nodes one for
> each address). The user provided status array then contains resulting
> numa node for each address or an error. The semantic of this function is
> little bit confusing because only some errors are reported back. Notably
> migrate_pages error is only reported via the return value. This patch
> doesn't try to address these semantic nuances but rather change the
> underlying implementation.
>
> Currently we are processing user input (which can be really large)
> in batches which are stored to a temporarily allocated page. Each
> address is resolved to its struct page and stored to page_to_node
> structure along with the requested target numa node. The array of these
> structures is then conveyed down the page migration path via private
> argument. new_page_node then finds the corresponding structure and
> allocates the proper target page.
>
> What is the problem with the current implementation and why to change
> it? Apart from being quite ugly it also doesn't cope with unexpected
> pages showing up on the migration list inside migrate_pages path.
> That doesn't happen currently but the follow up patch would like to
> make the thp migration code more clear and that would need to split a
> THP into the list for some cases.
>
> How does the new implementation work? Well, instead of batching into a
> fixed size array we simply batch all pages that should be migrated to
> the same node and isolate all of them into a linked list which doesn't
> require any additional storage. This should work reasonably well because
> page migration usually migrates larger ranges of memory to a specific
> node. So the common case should work equally well as the current
> implementation. Even if somebody constructs an input where the target
> numa nodes would be interleaved we shouldn't see a large performance
> impact because page migration alone doesn't really benefit from
> batching. mmap_sem batching for the lookup is quite questionable and
> isolate_lru_page which would benefit from batching is not using it even
> in the current implementation.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/internal.h | 1 +
> mm/mempolicy.c | 5 +-
> mm/migrate.c | 306 +++++++++++++++++++++++++--------------------------------
> 3 files changed, 139 insertions(+), 173 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index e6bd35182dae..1a1bb5d59c15 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -538,4 +538,5 @@ static inline bool is_migrate_highatomic_page(struct page *page)
> }
>
> void setup_zone_pageset(struct zone *zone);
> +extern struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x);
> #endif /* __MM_INTERNAL_H */
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index f604b22ebb65..66c9c79b21be 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -942,7 +942,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
> }
> }
>
> -static struct page *new_node_page(struct page *page, unsigned long node, int **x)
> +/* page allocation callback for NUMA node migration */
> +struct page *alloc_new_node_page(struct page *page, unsigned long node, int **x)
> {
> if (PageHuge(page))
> return alloc_huge_page_node(page_hstate(compound_head(page)),
> @@ -986,7 +987,7 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
> flags | MPOL_MF_DISCONTIG_OK, &pagelist);
>
> if (!list_empty(&pagelist)) {
> - err = migrate_pages(&pagelist, new_node_page, NULL, dest,
> + err = migrate_pages(&pagelist, alloc_new_node_page, NULL, dest,
> MIGRATE_SYNC, MR_SYSCALL);
> if (err)
> putback_movable_pages(&pagelist);
This reuses the existing page allocation helper from migrate_pages() system
call. But all these allocator helper names for migrate_pages() function are
really confusing. Even in this case alloc_new_node_page and the original
new_node_page() which is still getting used in do_migrate_range() sounds
similar even if their implementation is quite different. IMHO either all of
them should be moved to the header file with proper differentiating names
or let them be there in their respective files with these generic names and
clean them up later.
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4d0be47a322a..9d7252ea2acd 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1444,141 +1444,104 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> }
>
> #ifdef CONFIG_NUMA
> -/*
> - * Move a list of individual pages
> - */
> -struct page_to_node {
> - unsigned long addr;
> - struct page *page;
> - int node;
> - int status;
> -};
>
> -static struct page *new_page_node(struct page *p, unsigned long private,
> - int **result)
> +static int store_status(int __user *status, int start, int value, int nr)
> {
> - struct page_to_node *pm = (struct page_to_node *)private;
> -
> - while (pm->node != MAX_NUMNODES && pm->page != p)
> - pm++;
> + while (nr-- > 0) {
> + if (put_user(value, status + start))
> + return -EFAULT;
> + start++;
> + }
>
> - if (pm->node == MAX_NUMNODES)
> - return NULL;
> + return 0;
> +}
Just a nit. new_page_node() and store_status() seems different. Then why
the git diff looks so clumsy.
>
> - *result = &pm->status;
> +static int do_move_pages_to_node(struct mm_struct *mm,
> + struct list_head *pagelist, int node)
> +{
> + int err;
>
> - if (PageHuge(p))
> - return alloc_huge_page_node(page_hstate(compound_head(p)),
> - pm->node);
> - else if (thp_migration_supported() && PageTransHuge(p)) {
> - struct page *thp;
> + if (list_empty(pagelist))
> + return 0;
>
> - thp = alloc_pages_node(pm->node,
> - (GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
> - HPAGE_PMD_ORDER);
> - if (!thp)
> - return NULL;
> - prep_transhuge_page(thp);
> - return thp;
> - } else
> - return __alloc_pages_node(pm->node,
> - GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
> + err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
> + MIGRATE_SYNC, MR_SYSCALL);
> + if (err)
> + putback_movable_pages(pagelist);
> + return err;
> }
Even this one. IIUC, do_move_pages_to_node() migrate a chunk of pages
at a time which belong to the same target node. Perhaps the name should
suggest so. All these helper page migration helper functions sound so
similar.
>
> /*
> - * Move a set of pages as indicated in the pm array. The addr
> - * field must be set to the virtual address of the page to be moved
> - * and the node number must contain a valid target node.
> - * The pm array ends with node = MAX_NUMNODES.
> + * Resolves the given address to a struct page, isolates it from the LRU and
> + * puts it to the given pagelist.
> + * Returns -errno if the page cannot be found/isolated or 0 when it has been
> + * queued or the page doesn't need to be migrated because it is already on
> + * the target node
> */
> -static int do_move_page_to_node_array(struct mm_struct *mm,
> - struct page_to_node *pm,
> - int migrate_all)
> +static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
> + int node, struct list_head *pagelist, bool migrate_all)
> {
> + struct vm_area_struct *vma;
> + struct page *page;
> + unsigned int follflags;
> int err;
> - struct page_to_node *pp;
> - LIST_HEAD(pagelist);
>
> down_read(&mm->mmap_sem);
Holding mmap_sem for individual pages makes sense. Current
implementation is holding it for an entire batch.
> + err = -EFAULT;
> + vma = find_vma(mm, addr);
> + if (!vma || addr < vma->vm_start || !vma_migratable(vma))
While here, should not we add 'addr > vma->vm_end' into this condition ?
> + goto out;
>
> - /*
> - * Build a list of pages to migrate
> - */
> - for (pp = pm; pp->node != MAX_NUMNODES; pp++) {
> - struct vm_area_struct *vma;
> - struct page *page;
> - struct page *head;
> - unsigned int follflags;
> -
> - err = -EFAULT;
> - vma = find_vma(mm, pp->addr);
> - if (!vma || pp->addr < vma->vm_start || !vma_migratable(vma))
> - goto set_status;
> -
> - /* FOLL_DUMP to ignore special (like zero) pages */
> - follflags = FOLL_GET | FOLL_DUMP;
> - if (!thp_migration_supported())
> - follflags |= FOLL_SPLIT;
> - page = follow_page(vma, pp->addr, follflags);
> + /* FOLL_DUMP to ignore special (like zero) pages */
> + follflags = FOLL_GET | FOLL_DUMP;
> + if (!thp_migration_supported())
> + follflags |= FOLL_SPLIT;
> + page = follow_page(vma, addr, follflags);
>
> - err = PTR_ERR(page);
> - if (IS_ERR(page))
> - goto set_status;
> + err = PTR_ERR(page);
> + if (IS_ERR(page))
> + goto out;
>
> - err = -ENOENT;
> - if (!page)
> - goto set_status;
> + err = -ENOENT;
> + if (!page)
> + goto out;
>
> - err = page_to_nid(page);
> + err = 0;
> + if (page_to_nid(page) == node)
> + goto out_putpage;
>
> - if (err == pp->node)
> - /*
> - * Node already in the right place
> - */
> - goto put_and_set;
> + err = -EACCES;
> + if (page_mapcount(page) > 1 &&
> + !migrate_all)
> + goto out_putpage;
>
> - err = -EACCES;
> - if (page_mapcount(page) > 1 &&
> - !migrate_all)
> - goto put_and_set;
> -
> - if (PageHuge(page)) {
> - if (PageHead(page)) {
> - isolate_huge_page(page, &pagelist);
> - err = 0;
> - pp->page = page;
> - }
> - goto put_and_set;
> + if (PageHuge(page)) {
> + if (PageHead(page)) {
> + isolate_huge_page(page, pagelist);
> + err = 0;
> }
> + } else {
> + struct page *head;
>
> - pp->page = compound_head(page);
> head = compound_head(page);
> err = isolate_lru_page(head);
> - if (!err) {
> - list_add_tail(&head->lru, &pagelist);
> - mod_node_page_state(page_pgdat(head),
> - NR_ISOLATED_ANON + page_is_file_cache(head),
> - hpage_nr_pages(head));
> - }
> -put_and_set:
> - /*
> - * Either remove the duplicate refcount from
> - * isolate_lru_page() or drop the page ref if it was
> - * not isolated.
> - */
> - put_page(page);
> -set_status:
> - pp->status = err;
> - }
> -
> - err = 0;
> - if (!list_empty(&pagelist)) {
> - err = migrate_pages(&pagelist, new_page_node, NULL,
> - (unsigned long)pm, MIGRATE_SYNC, MR_SYSCALL);
> if (err)
> - putback_movable_pages(&pagelist);
> - }
> + goto out_putpage;
>
> + err = 0;
> + list_add_tail(&head->lru, pagelist);
> + mod_node_page_state(page_pgdat(head),
> + NR_ISOLATED_ANON + page_is_file_cache(head),
> + hpage_nr_pages(head));
> + }
> +out_putpage:
> + /*
> + * Either remove the duplicate refcount from
> + * isolate_lru_page() or drop the page ref if it was
> + * not isolated.
> + */
> + put_page(page);
> +out:
> up_read(&mm->mmap_sem);
> return err;
> }
> @@ -1593,79 +1556,80 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> const int __user *nodes,
> int __user *status, int flags)
> {
> - struct page_to_node *pm;
> - unsigned long chunk_nr_pages;
> - unsigned long chunk_start;
> - int err;
> -
> - err = -ENOMEM;
> - pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
> - if (!pm)
> - goto out;
> + int chunk_node = NUMA_NO_NODE;
> + LIST_HEAD(pagelist);
> + int chunk_start, i;
> + int err = 0, err1;
err init might not be required, its getting assigned to -EFAULT right away.
>
> migrate_prep();
>
> - /*
> - * Store a chunk of page_to_node array in a page,
> - * but keep the last one as a marker
> - */
> - chunk_nr_pages = (PAGE_SIZE / sizeof(struct page_to_node)) - 1;
> -
> - for (chunk_start = 0;
> - chunk_start < nr_pages;
> - chunk_start += chunk_nr_pages) {
> - int j;
> -
> - if (chunk_start + chunk_nr_pages > nr_pages)
> - chunk_nr_pages = nr_pages - chunk_start;
> -
> - /* fill the chunk pm with addrs and nodes from user-space */
> - for (j = 0; j < chunk_nr_pages; j++) {
> - const void __user *p;
> - int node;
> -
> - err = -EFAULT;
> - if (get_user(p, pages + j + chunk_start))
> - goto out_pm;
> - pm[j].addr = (unsigned long) p;
> -
> - if (get_user(node, nodes + j + chunk_start))
> - goto out_pm;
> -
> - err = -ENODEV;
> - if (node < 0 || node >= MAX_NUMNODES)
> - goto out_pm;
> + for (i = chunk_start = 0; i < nr_pages; i++) {
> + const void __user *p;
> + unsigned long addr;
> + int node;
>
> - if (!node_state(node, N_MEMORY))
> - goto out_pm;
> -
> - err = -EACCES;
> - if (!node_isset(node, task_nodes))
> - goto out_pm;
> + err = -EFAULT;
> + if (get_user(p, pages + i))
> + goto out_flush;
> + if (get_user(node, nodes + i))
> + goto out_flush;
> + addr = (unsigned long)p;
> +
> + err = -ENODEV;
> + if (node < 0 || node >= MAX_NUMNODES)
> + goto out_flush;
> + if (!node_state(node, N_MEMORY))
> + goto out_flush;
>
> - pm[j].node = node;
> + err = -EACCES;
> + if (!node_isset(node, task_nodes))
> + goto out_flush;
> +
> + if (chunk_node == NUMA_NO_NODE) {
> + chunk_node = node;
> + chunk_start = i;
> + } else if (node != chunk_node) {
> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> + if (err)
> + goto out;
> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> + if (err)
> + goto out;
> + chunk_start = i;
> + chunk_node = node;
> }
>
> - /* End marker for this chunk */
> - pm[chunk_nr_pages].node = MAX_NUMNODES;
> + /*
> + * Errors in the page lookup or isolation are not fatal and we simply
> + * report them via status
> + */
> + err = add_page_for_migration(mm, addr, chunk_node,
> + &pagelist, flags & MPOL_MF_MOVE_ALL);
> + if (!err)
> + continue;
>
> - /* Migrate this chunk */
> - err = do_move_page_to_node_array(mm, pm,
> - flags & MPOL_MF_MOVE_ALL);
> - if (err < 0)
> - goto out_pm;
> + err = store_status(status, i, err, 1);
> + if (err)
> + goto out_flush;
>
> - /* Return status information */
> - for (j = 0; j < chunk_nr_pages; j++)
> - if (put_user(pm[j].status, status + j + chunk_start)) {
> - err = -EFAULT;
> - goto out_pm;
> - }
> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> + if (err)
> + goto out;
> + if (i > chunk_start) {
> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> + if (err)
> + goto out;
> + }
> + chunk_node = NUMA_NO_NODE;
This block of code is bit confusing.
1) Why attempt to migrate when just one page could not be isolated ?
2) 'i' is always greater than chunk_start except the starting page
3) Why reset chunk_node as NUMA_NO_NODE ?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2018-01-03 8:42 ` Anshuman Khandual
@ 2018-01-03 8:58 ` Michal Hocko
-1 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2018-01-03 8:58 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Kirill A. Shutemov,
Vlastimil Babka, Andrew Morton, Andrea Reale, LKML
On Wed 03-01-18 14:12:17, Anshuman Khandual wrote:
> On 12/08/2017 09:45 PM, Michal Hocko wrote:
[...]
> > @@ -986,7 +987,7 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
> > flags | MPOL_MF_DISCONTIG_OK, &pagelist);
> >
> > if (!list_empty(&pagelist)) {
> > - err = migrate_pages(&pagelist, new_node_page, NULL, dest,
> > + err = migrate_pages(&pagelist, alloc_new_node_page, NULL, dest,
> > MIGRATE_SYNC, MR_SYSCALL);
> > if (err)
> > putback_movable_pages(&pagelist);
>
> This reuses the existing page allocation helper from migrate_pages() system
> call. But all these allocator helper names for migrate_pages() function are
> really confusing. Even in this case alloc_new_node_page and the original
> new_node_page() which is still getting used in do_migrate_range() sounds
> similar even if their implementation is quite different. IMHO either all of
> them should be moved to the header file with proper differentiating names
> or let them be there in their respective files with these generic names and
> clean them up later.
I believe I've tried that but I couldn't make them into a single header
file easily because of header file dependencies. I agree that their
names are quite confusing. Feel free to send a patch to clean this up.
>
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 4d0be47a322a..9d7252ea2acd 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -1444,141 +1444,104 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> > }
> >
> > #ifdef CONFIG_NUMA
> > -/*
> > - * Move a list of individual pages
> > - */
> > -struct page_to_node {
> > - unsigned long addr;
> > - struct page *page;
> > - int node;
> > - int status;
> > -};
> >
> > -static struct page *new_page_node(struct page *p, unsigned long private,
> > - int **result)
> > +static int store_status(int __user *status, int start, int value, int nr)
> > {
> > - struct page_to_node *pm = (struct page_to_node *)private;
> > -
> > - while (pm->node != MAX_NUMNODES && pm->page != p)
> > - pm++;
> > + while (nr-- > 0) {
> > + if (put_user(value, status + start))
> > + return -EFAULT;
> > + start++;
> > + }
> >
> > - if (pm->node == MAX_NUMNODES)
> > - return NULL;
> > + return 0;
> > +}
>
>
> Just a nit. new_page_node() and store_status() seems different. Then why
> the git diff looks so clumsy.
Kirill was suggesting to use --patience to general the diff which leads
to a slightly better output. It has been posted as a separate email [1].
Maybe you will find that one easier to review.
[1] http://lkml.kernel.org/r/20171213143948.GM25185@dhcp22.suse.cz
> >
> > - *result = &pm->status;
> > +static int do_move_pages_to_node(struct mm_struct *mm,
> > + struct list_head *pagelist, int node)
> > +{
> > + int err;
> >
> > - if (PageHuge(p))
> > - return alloc_huge_page_node(page_hstate(compound_head(p)),
> > - pm->node);
> > - else if (thp_migration_supported() && PageTransHuge(p)) {
> > - struct page *thp;
> > + if (list_empty(pagelist))
> > + return 0;
> >
> > - thp = alloc_pages_node(pm->node,
> > - (GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
> > - HPAGE_PMD_ORDER);
> > - if (!thp)
> > - return NULL;
> > - prep_transhuge_page(thp);
> > - return thp;
> > - } else
> > - return __alloc_pages_node(pm->node,
> > - GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
> > + err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
> > + MIGRATE_SYNC, MR_SYSCALL);
> > + if (err)
> > + putback_movable_pages(pagelist);
> > + return err;
> > }
>
> Even this one. IIUC, do_move_pages_to_node() migrate a chunk of pages
> at a time which belong to the same target node. Perhaps the name should
> suggest so. All these helper page migration helper functions sound so
> similar.
What do you suggest? I find do_move_pages_to_node quite explicit on its
purpose.
> > /*
> > - * Move a set of pages as indicated in the pm array. The addr
> > - * field must be set to the virtual address of the page to be moved
> > - * and the node number must contain a valid target node.
> > - * The pm array ends with node = MAX_NUMNODES.
> > + * Resolves the given address to a struct page, isolates it from the LRU and
> > + * puts it to the given pagelist.
> > + * Returns -errno if the page cannot be found/isolated or 0 when it has been
> > + * queued or the page doesn't need to be migrated because it is already on
> > + * the target node
> > */
> > -static int do_move_page_to_node_array(struct mm_struct *mm,
> > - struct page_to_node *pm,
> > - int migrate_all)
> > +static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
> > + int node, struct list_head *pagelist, bool migrate_all)
> > {
> > + struct vm_area_struct *vma;
> > + struct page *page;
> > + unsigned int follflags;
> > int err;
> > - struct page_to_node *pp;
> > - LIST_HEAD(pagelist);
> >
> > down_read(&mm->mmap_sem);
>
> Holding mmap_sem for individual pages makes sense. Current
> implementation is holding it for an entire batch.
I didn't bother to optimize this path to be honest. It is true that lock
batching can lead to improvements but that would complicate the code
(how many patches to batch?) so I've left that for later if somebody
actually sees any problem.
> > + err = -EFAULT;
> > + vma = find_vma(mm, addr);
> > + if (!vma || addr < vma->vm_start || !vma_migratable(vma))
>
> While here, should not we add 'addr > vma->vm_end' into this condition ?
No. See what find_vma does.
[...]
Please cut out the quoted reply to minimum
> > @@ -1593,79 +1556,80 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> > const int __user *nodes,
> > int __user *status, int flags)
> > {
> > - struct page_to_node *pm;
> > - unsigned long chunk_nr_pages;
> > - unsigned long chunk_start;
> > - int err;
> > -
> > - err = -ENOMEM;
> > - pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
> > - if (!pm)
> > - goto out;
> > + int chunk_node = NUMA_NO_NODE;
> > + LIST_HEAD(pagelist);
> > + int chunk_start, i;
> > + int err = 0, err1;
>
> err init might not be required, its getting assigned to -EFAULT right away.
No, nr_pages might be 0 AFAICS.
[...]
> > + if (chunk_node == NUMA_NO_NODE) {
> > + chunk_node = node;
> > + chunk_start = i;
> > + } else if (node != chunk_node) {
> > + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> > + if (err)
> > + goto out;
> > + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> > + if (err)
> > + goto out;
> > + chunk_start = i;
> > + chunk_node = node;
> > }
> >
> > - /* End marker for this chunk */
> > - pm[chunk_nr_pages].node = MAX_NUMNODES;
> > + /*
> > + * Errors in the page lookup or isolation are not fatal and we simply
> > + * report them via status
> > + */
> > + err = add_page_for_migration(mm, addr, chunk_node,
> > + &pagelist, flags & MPOL_MF_MOVE_ALL);
> > + if (!err)
> > + continue;
> >
> > - /* Migrate this chunk */
> > - err = do_move_page_to_node_array(mm, pm,
> > - flags & MPOL_MF_MOVE_ALL);
> > - if (err < 0)
> > - goto out_pm;
> > + err = store_status(status, i, err, 1);
> > + if (err)
> > + goto out_flush;
> >
> > - /* Return status information */
> > - for (j = 0; j < chunk_nr_pages; j++)
> > - if (put_user(pm[j].status, status + j + chunk_start)) {
> > - err = -EFAULT;
> > - goto out_pm;
> > - }
> > + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> > + if (err)
> > + goto out;
> > + if (i > chunk_start) {
> > + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> > + if (err)
> > + goto out;
> > + }
> > + chunk_node = NUMA_NO_NODE;
>
> This block of code is bit confusing.
I believe this is easier to grasp when looking at the resulting code.
>
> 1) Why attempt to migrate when just one page could not be isolated ?
> 2) 'i' is always greater than chunk_start except the starting page
> 3) Why reset chunk_node as NUMA_NO_NODE ?
This is all about flushing the pending state on an error and
distinguising a fresh batch.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2018-01-03 8:58 ` Michal Hocko
0 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2018-01-03 8:58 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Kirill A. Shutemov,
Vlastimil Babka, Andrew Morton, Andrea Reale, LKML
On Wed 03-01-18 14:12:17, Anshuman Khandual wrote:
> On 12/08/2017 09:45 PM, Michal Hocko wrote:
[...]
> > @@ -986,7 +987,7 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest,
> > flags | MPOL_MF_DISCONTIG_OK, &pagelist);
> >
> > if (!list_empty(&pagelist)) {
> > - err = migrate_pages(&pagelist, new_node_page, NULL, dest,
> > + err = migrate_pages(&pagelist, alloc_new_node_page, NULL, dest,
> > MIGRATE_SYNC, MR_SYSCALL);
> > if (err)
> > putback_movable_pages(&pagelist);
>
> This reuses the existing page allocation helper from migrate_pages() system
> call. But all these allocator helper names for migrate_pages() function are
> really confusing. Even in this case alloc_new_node_page and the original
> new_node_page() which is still getting used in do_migrate_range() sounds
> similar even if their implementation is quite different. IMHO either all of
> them should be moved to the header file with proper differentiating names
> or let them be there in their respective files with these generic names and
> clean them up later.
I believe I've tried that but I couldn't make them into a single header
file easily because of header file dependencies. I agree that their
names are quite confusing. Feel free to send a patch to clean this up.
>
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 4d0be47a322a..9d7252ea2acd 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -1444,141 +1444,104 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> > }
> >
> > #ifdef CONFIG_NUMA
> > -/*
> > - * Move a list of individual pages
> > - */
> > -struct page_to_node {
> > - unsigned long addr;
> > - struct page *page;
> > - int node;
> > - int status;
> > -};
> >
> > -static struct page *new_page_node(struct page *p, unsigned long private,
> > - int **result)
> > +static int store_status(int __user *status, int start, int value, int nr)
> > {
> > - struct page_to_node *pm = (struct page_to_node *)private;
> > -
> > - while (pm->node != MAX_NUMNODES && pm->page != p)
> > - pm++;
> > + while (nr-- > 0) {
> > + if (put_user(value, status + start))
> > + return -EFAULT;
> > + start++;
> > + }
> >
> > - if (pm->node == MAX_NUMNODES)
> > - return NULL;
> > + return 0;
> > +}
>
>
> Just a nit. new_page_node() and store_status() seems different. Then why
> the git diff looks so clumsy.
Kirill was suggesting to use --patience to general the diff which leads
to a slightly better output. It has been posted as a separate email [1].
Maybe you will find that one easier to review.
[1] http://lkml.kernel.org/r/20171213143948.GM25185@dhcp22.suse.cz
> >
> > - *result = &pm->status;
> > +static int do_move_pages_to_node(struct mm_struct *mm,
> > + struct list_head *pagelist, int node)
> > +{
> > + int err;
> >
> > - if (PageHuge(p))
> > - return alloc_huge_page_node(page_hstate(compound_head(p)),
> > - pm->node);
> > - else if (thp_migration_supported() && PageTransHuge(p)) {
> > - struct page *thp;
> > + if (list_empty(pagelist))
> > + return 0;
> >
> > - thp = alloc_pages_node(pm->node,
> > - (GFP_TRANSHUGE | __GFP_THISNODE) & ~__GFP_RECLAIM,
> > - HPAGE_PMD_ORDER);
> > - if (!thp)
> > - return NULL;
> > - prep_transhuge_page(thp);
> > - return thp;
> > - } else
> > - return __alloc_pages_node(pm->node,
> > - GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
> > + err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
> > + MIGRATE_SYNC, MR_SYSCALL);
> > + if (err)
> > + putback_movable_pages(pagelist);
> > + return err;
> > }
>
> Even this one. IIUC, do_move_pages_to_node() migrate a chunk of pages
> at a time which belong to the same target node. Perhaps the name should
> suggest so. All these helper page migration helper functions sound so
> similar.
What do you suggest? I find do_move_pages_to_node quite explicit on its
purpose.
> > /*
> > - * Move a set of pages as indicated in the pm array. The addr
> > - * field must be set to the virtual address of the page to be moved
> > - * and the node number must contain a valid target node.
> > - * The pm array ends with node = MAX_NUMNODES.
> > + * Resolves the given address to a struct page, isolates it from the LRU and
> > + * puts it to the given pagelist.
> > + * Returns -errno if the page cannot be found/isolated or 0 when it has been
> > + * queued or the page doesn't need to be migrated because it is already on
> > + * the target node
> > */
> > -static int do_move_page_to_node_array(struct mm_struct *mm,
> > - struct page_to_node *pm,
> > - int migrate_all)
> > +static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
> > + int node, struct list_head *pagelist, bool migrate_all)
> > {
> > + struct vm_area_struct *vma;
> > + struct page *page;
> > + unsigned int follflags;
> > int err;
> > - struct page_to_node *pp;
> > - LIST_HEAD(pagelist);
> >
> > down_read(&mm->mmap_sem);
>
> Holding mmap_sem for individual pages makes sense. Current
> implementation is holding it for an entire batch.
I didn't bother to optimize this path to be honest. It is true that lock
batching can lead to improvements but that would complicate the code
(how many patches to batch?) so I've left that for later if somebody
actually sees any problem.
> > + err = -EFAULT;
> > + vma = find_vma(mm, addr);
> > + if (!vma || addr < vma->vm_start || !vma_migratable(vma))
>
> While here, should not we add 'addr > vma->vm_end' into this condition ?
No. See what find_vma does.
[...]
Please cut out the quoted reply to minimum
> > @@ -1593,79 +1556,80 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> > const int __user *nodes,
> > int __user *status, int flags)
> > {
> > - struct page_to_node *pm;
> > - unsigned long chunk_nr_pages;
> > - unsigned long chunk_start;
> > - int err;
> > -
> > - err = -ENOMEM;
> > - pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
> > - if (!pm)
> > - goto out;
> > + int chunk_node = NUMA_NO_NODE;
> > + LIST_HEAD(pagelist);
> > + int chunk_start, i;
> > + int err = 0, err1;
>
> err init might not be required, its getting assigned to -EFAULT right away.
No, nr_pages might be 0 AFAICS.
[...]
> > + if (chunk_node == NUMA_NO_NODE) {
> > + chunk_node = node;
> > + chunk_start = i;
> > + } else if (node != chunk_node) {
> > + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> > + if (err)
> > + goto out;
> > + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> > + if (err)
> > + goto out;
> > + chunk_start = i;
> > + chunk_node = node;
> > }
> >
> > - /* End marker for this chunk */
> > - pm[chunk_nr_pages].node = MAX_NUMNODES;
> > + /*
> > + * Errors in the page lookup or isolation are not fatal and we simply
> > + * report them via status
> > + */
> > + err = add_page_for_migration(mm, addr, chunk_node,
> > + &pagelist, flags & MPOL_MF_MOVE_ALL);
> > + if (!err)
> > + continue;
> >
> > - /* Migrate this chunk */
> > - err = do_move_page_to_node_array(mm, pm,
> > - flags & MPOL_MF_MOVE_ALL);
> > - if (err < 0)
> > - goto out_pm;
> > + err = store_status(status, i, err, 1);
> > + if (err)
> > + goto out_flush;
> >
> > - /* Return status information */
> > - for (j = 0; j < chunk_nr_pages; j++)
> > - if (put_user(pm[j].status, status + j + chunk_start)) {
> > - err = -EFAULT;
> > - goto out_pm;
> > - }
> > + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> > + if (err)
> > + goto out;
> > + if (i > chunk_start) {
> > + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> > + if (err)
> > + goto out;
> > + }
> > + chunk_node = NUMA_NO_NODE;
>
> This block of code is bit confusing.
I believe this is easier to grasp when looking at the resulting code.
>
> 1) Why attempt to migrate when just one page could not be isolated ?
> 2) 'i' is always greater than chunk_start except the starting page
> 3) Why reset chunk_node as NUMA_NO_NODE ?
This is all about flushing the pending state on an error and
distinguising a fresh batch.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2018-01-03 8:58 ` Michal Hocko
@ 2018-01-03 9:36 ` Anshuman Khandual
-1 siblings, 0 replies; 61+ messages in thread
From: Anshuman Khandual @ 2018-01-03 9:36 UTC (permalink / raw)
To: Michal Hocko, Anshuman Khandual
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Kirill A. Shutemov,
Vlastimil Babka, Andrew Morton, Andrea Reale, LKML
On 01/03/2018 02:28 PM, Michal Hocko wrote:
> On Wed 03-01-18 14:12:17, Anshuman Khandual wrote:
>> On 12/08/2017 09:45 PM, Michal Hocko wrote:
> [...]
[...]
>>
>> This reuses the existing page allocation helper from migrate_pages() system
>> call. But all these allocator helper names for migrate_pages() function are
>> really confusing. Even in this case alloc_new_node_page and the original
>> new_node_page() which is still getting used in do_migrate_range() sounds
>> similar even if their implementation is quite different. IMHO either all of
>> them should be moved to the header file with proper differentiating names
>> or let them be there in their respective files with these generic names and
>> clean them up later.
>
> I believe I've tried that but I couldn't make them into a single header
> file easily because of header file dependencies. I agree that their
> names are quite confusing. Feel free to send a patch to clean this up.
Sure. Will try once this one gets into mmotm.
[...]
>>
>>
>> Just a nit. new_page_node() and store_status() seems different. Then why
>> the git diff looks so clumsy.
>
> Kirill was suggesting to use --patience to general the diff which leads
> to a slightly better output. It has been posted as a separate email [1].
> Maybe you will find that one easier to review.
>
> [1] http://lkml.kernel.org/r/20171213143948.GM25185@dhcp22.suse.cz
Yeah it does look better.
[...]
>>> - return thp;
>>> - } else
>>> - return __alloc_pages_node(pm->node,
>>> - GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
>>> + err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
>>> + MIGRATE_SYNC, MR_SYSCALL);
>>> + if (err)
>>> + putback_movable_pages(pagelist);
>>> + return err;
>>> }
>>
>> Even this one. IIUC, do_move_pages_to_node() migrate a chunk of pages
>> at a time which belong to the same target node. Perhaps the name should
>> suggest so. All these helper page migration helper functions sound so
>> similar.
>
> What do you suggest? I find do_move_pages_to_node quite explicit on its
> purpose.
Sure. Not a big deal.
[...]
>>> {
>>> + struct vm_area_struct *vma;
>>> + struct page *page;
>>> + unsigned int follflags;
>>> int err;
>>> - struct page_to_node *pp;
>>> - LIST_HEAD(pagelist);
>>>
>>> down_read(&mm->mmap_sem);
>>
>> Holding mmap_sem for individual pages makes sense. Current
>> implementation is holding it for an entire batch.
>
> I didn't bother to optimize this path to be honest. It is true that lock
> batching can lead to improvements but that would complicate the code
> (how many patches to batch?) so I've left that for later if somebody
> actually sees any problem.
>
>>> + err = -EFAULT;
>>> + vma = find_vma(mm, addr);
>>> + if (!vma || addr < vma->vm_start || !vma_migratable(vma))
>>
>> While here, should not we add 'addr > vma->vm_end' into this condition ?
>
> No. See what find_vma does.
>
Right.
> [...]
>
> Please cut out the quoted reply to minimum
Sure will do.
>
>>> @@ -1593,79 +1556,80 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>>> const int __user *nodes,
>>> int __user *status, int flags)
>>> {
>>> - struct page_to_node *pm;
>>> - unsigned long chunk_nr_pages;
>>> - unsigned long chunk_start;
>>> - int err;
>>> -
>>> - err = -ENOMEM;
>>> - pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
>>> - if (!pm)
>>> - goto out;
>>> + int chunk_node = NUMA_NO_NODE;
>>> + LIST_HEAD(pagelist);
>>> + int chunk_start, i;
>>> + int err = 0, err1;
>>
>> err init might not be required, its getting assigned to -EFAULT right away.
>
> No, nr_pages might be 0 AFAICS.
Right but there is another err = 0 after the for loop.
>
> [...]
>>> + if (chunk_node == NUMA_NO_NODE) {
>>> + chunk_node = node;
>>> + chunk_start = i;
>>> + } else if (node != chunk_node) {
>>> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
>>> + if (err)
>>> + goto out;
>>> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
>>> + if (err)
>>> + goto out;
>>> + chunk_start = i;
>>> + chunk_node = node;
>>> }
[...]
>>> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
>>> + if (err)
>>> + goto out;
>>> + if (i > chunk_start) {
>>> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
>>> + if (err)
>>> + goto out;
>>> + }
>>> + chunk_node = NUMA_NO_NODE;
>>
>> This block of code is bit confusing.
>
> I believe this is easier to grasp when looking at the resulting code.
>>
>> 1) Why attempt to migrate when just one page could not be isolated ?
>> 2) 'i' is always greater than chunk_start except the starting page
>> 3) Why reset chunk_node as NUMA_NO_NODE ?
>
> This is all about flushing the pending state on an error and
> distinguising a fresh batch.
Okay. Will test it out on a multi node system once I get hold of one.
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2018-01-03 9:36 ` Anshuman Khandual
0 siblings, 0 replies; 61+ messages in thread
From: Anshuman Khandual @ 2018-01-03 9:36 UTC (permalink / raw)
To: Michal Hocko, Anshuman Khandual
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Kirill A. Shutemov,
Vlastimil Babka, Andrew Morton, Andrea Reale, LKML
On 01/03/2018 02:28 PM, Michal Hocko wrote:
> On Wed 03-01-18 14:12:17, Anshuman Khandual wrote:
>> On 12/08/2017 09:45 PM, Michal Hocko wrote:
> [...]
[...]
>>
>> This reuses the existing page allocation helper from migrate_pages() system
>> call. But all these allocator helper names for migrate_pages() function are
>> really confusing. Even in this case alloc_new_node_page and the original
>> new_node_page() which is still getting used in do_migrate_range() sounds
>> similar even if their implementation is quite different. IMHO either all of
>> them should be moved to the header file with proper differentiating names
>> or let them be there in their respective files with these generic names and
>> clean them up later.
>
> I believe I've tried that but I couldn't make them into a single header
> file easily because of header file dependencies. I agree that their
> names are quite confusing. Feel free to send a patch to clean this up.
Sure. Will try once this one gets into mmotm.
[...]
>>
>>
>> Just a nit. new_page_node() and store_status() seems different. Then why
>> the git diff looks so clumsy.
>
> Kirill was suggesting to use --patience to general the diff which leads
> to a slightly better output. It has been posted as a separate email [1].
> Maybe you will find that one easier to review.
>
> [1] http://lkml.kernel.org/r/20171213143948.GM25185@dhcp22.suse.cz
Yeah it does look better.
[...]
>>> - return thp;
>>> - } else
>>> - return __alloc_pages_node(pm->node,
>>> - GFP_HIGHUSER_MOVABLE | __GFP_THISNODE, 0);
>>> + err = migrate_pages(pagelist, alloc_new_node_page, NULL, node,
>>> + MIGRATE_SYNC, MR_SYSCALL);
>>> + if (err)
>>> + putback_movable_pages(pagelist);
>>> + return err;
>>> }
>>
>> Even this one. IIUC, do_move_pages_to_node() migrate a chunk of pages
>> at a time which belong to the same target node. Perhaps the name should
>> suggest so. All these helper page migration helper functions sound so
>> similar.
>
> What do you suggest? I find do_move_pages_to_node quite explicit on its
> purpose.
Sure. Not a big deal.
[...]
>>> {
>>> + struct vm_area_struct *vma;
>>> + struct page *page;
>>> + unsigned int follflags;
>>> int err;
>>> - struct page_to_node *pp;
>>> - LIST_HEAD(pagelist);
>>>
>>> down_read(&mm->mmap_sem);
>>
>> Holding mmap_sem for individual pages makes sense. Current
>> implementation is holding it for an entire batch.
>
> I didn't bother to optimize this path to be honest. It is true that lock
> batching can lead to improvements but that would complicate the code
> (how many patches to batch?) so I've left that for later if somebody
> actually sees any problem.
>
>>> + err = -EFAULT;
>>> + vma = find_vma(mm, addr);
>>> + if (!vma || addr < vma->vm_start || !vma_migratable(vma))
>>
>> While here, should not we add 'addr > vma->vm_end' into this condition ?
>
> No. See what find_vma does.
>
Right.
> [...]
>
> Please cut out the quoted reply to minimum
Sure will do.
>
>>> @@ -1593,79 +1556,80 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>>> const int __user *nodes,
>>> int __user *status, int flags)
>>> {
>>> - struct page_to_node *pm;
>>> - unsigned long chunk_nr_pages;
>>> - unsigned long chunk_start;
>>> - int err;
>>> -
>>> - err = -ENOMEM;
>>> - pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
>>> - if (!pm)
>>> - goto out;
>>> + int chunk_node = NUMA_NO_NODE;
>>> + LIST_HEAD(pagelist);
>>> + int chunk_start, i;
>>> + int err = 0, err1;
>>
>> err init might not be required, its getting assigned to -EFAULT right away.
>
> No, nr_pages might be 0 AFAICS.
Right but there is another err = 0 after the for loop.
>
> [...]
>>> + if (chunk_node == NUMA_NO_NODE) {
>>> + chunk_node = node;
>>> + chunk_start = i;
>>> + } else if (node != chunk_node) {
>>> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
>>> + if (err)
>>> + goto out;
>>> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
>>> + if (err)
>>> + goto out;
>>> + chunk_start = i;
>>> + chunk_node = node;
>>> }
[...]
>>> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
>>> + if (err)
>>> + goto out;
>>> + if (i > chunk_start) {
>>> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
>>> + if (err)
>>> + goto out;
>>> + }
>>> + chunk_node = NUMA_NO_NODE;
>>
>> This block of code is bit confusing.
>
> I believe this is easier to grasp when looking at the resulting code.
>>
>> 1) Why attempt to migrate when just one page could not be isolated ?
>> 2) 'i' is always greater than chunk_start except the starting page
>> 3) Why reset chunk_node as NUMA_NO_NODE ?
>
> This is all about flushing the pending state on an error and
> distinguising a fresh batch.
Okay. Will test it out on a multi node system once I get hold of one.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
2018-01-03 9:36 ` Anshuman Khandual
@ 2018-01-03 9:52 ` Michal Hocko
-1 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2018-01-03 9:52 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Kirill A. Shutemov,
Vlastimil Babka, Andrew Morton, Andrea Reale, LKML
On Wed 03-01-18 15:06:49, Anshuman Khandual wrote:
> On 01/03/2018 02:28 PM, Michal Hocko wrote:
> > On Wed 03-01-18 14:12:17, Anshuman Khandual wrote:
> >> On 12/08/2017 09:45 PM, Michal Hocko wrote:
[...]
> >>> @@ -1593,79 +1556,80 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> >>> const int __user *nodes,
> >>> int __user *status, int flags)
> >>> {
> >>> - struct page_to_node *pm;
> >>> - unsigned long chunk_nr_pages;
> >>> - unsigned long chunk_start;
> >>> - int err;
> >>> -
> >>> - err = -ENOMEM;
> >>> - pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
> >>> - if (!pm)
> >>> - goto out;
> >>> + int chunk_node = NUMA_NO_NODE;
> >>> + LIST_HEAD(pagelist);
> >>> + int chunk_start, i;
> >>> + int err = 0, err1;
> >>
> >> err init might not be required, its getting assigned to -EFAULT right away.
> >
> > No, nr_pages might be 0 AFAICS.
>
> Right but there is another err = 0 after the for loop.
No we have
out_flush:
/* Make sure we do not overwrite the existing error */
err1 = do_move_pages_to_node(mm, &pagelist, current_node);
if (!err1)
err1 = store_status(status, start, current_node, i - start);
if (!err)
err = err1;
This is obviously not an act of beauty and probably a subject to a
cleanup but I just wanted this thing to be working first. Further
cleanups can go on top.
> > [...]
> >>> + if (chunk_node == NUMA_NO_NODE) {
> >>> + chunk_node = node;
> >>> + chunk_start = i;
> >>> + } else if (node != chunk_node) {
> >>> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> >>> + if (err)
> >>> + goto out;
> >>> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> >>> + if (err)
> >>> + goto out;
> >>> + chunk_start = i;
> >>> + chunk_node = node;
> >>> }
>
> [...]
>
> >>> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> >>> + if (err)
> >>> + goto out;
> >>> + if (i > chunk_start) {
> >>> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> >>> + if (err)
> >>> + goto out;
> >>> + }
> >>> + chunk_node = NUMA_NO_NODE;
> >>
> >> This block of code is bit confusing.
> >
> > I believe this is easier to grasp when looking at the resulting code.
> >>
> >> 1) Why attempt to migrate when just one page could not be isolated ?
> >> 2) 'i' is always greater than chunk_start except the starting page
> >> 3) Why reset chunk_node as NUMA_NO_NODE ?
> >
> > This is all about flushing the pending state on an error and
> > distinguising a fresh batch.
>
> Okay. Will test it out on a multi node system once I get hold of one.
Thanks. I have been testing this specific code path with the following
simple test program and numactl -m0. The code is rather crude so I've
always modified it manually to test different scenarios (this one keeps
every 1k page on the node node to test batching.
---
#include <sys/mman.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <stdio.h>
#include <errno.h>
#include <numaif.h>
int main()
{
unsigned long nr_pages = 10000;
size_t length = nr_pages << 12, i;
unsigned char *addr = mmap(NULL, length, PROT_READ | PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
void *addrs[nr_pages];
int nodes[nr_pages];
int status[nr_pages];
char cmd[128];
char ch;
if (addr == MAP_FAILED)
return 1;
madvise(addr, length, MADV_NOHUGEPAGE);
for (i = 0; i < length; i += 4096)
addr[i] = 1;
for (i = 0; i < nr_pages; i++)
{
addrs[i] = &addr[i * 4096];
if (i%1024)
nodes[i] = 1;
else
nodes[i] = 0;
status[i] = 0;
}
snprintf(cmd, sizeof(cmd)-1, "grep %lx /proc/%d/numa_maps", addr, getpid());
system(cmd);
snprintf(cmd, sizeof(cmd)-1, "grep %lx -A20 /proc/%d/smaps", addr, getpid());
system(cmd);
read(0, &ch, 1);
if (move_pages(0, nr_pages, addrs, nodes, status, MPOL_MF_MOVE)) {
printf("move_pages: err:%d\n", errno);
}
snprintf(cmd, sizeof(cmd)-1, "grep %lx /proc/%d/numa_maps", addr, getpid());
system(cmd);
snprintf(cmd, sizeof(cmd)-1, "grep %lx -A20 /proc/%d/smaps", addr, getpid());
system(cmd);
return 0;
}
---
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move
@ 2018-01-03 9:52 ` Michal Hocko
0 siblings, 0 replies; 61+ messages in thread
From: Michal Hocko @ 2018-01-03 9:52 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-mm, Zi Yan, Naoya Horiguchi, Kirill A. Shutemov,
Vlastimil Babka, Andrew Morton, Andrea Reale, LKML
On Wed 03-01-18 15:06:49, Anshuman Khandual wrote:
> On 01/03/2018 02:28 PM, Michal Hocko wrote:
> > On Wed 03-01-18 14:12:17, Anshuman Khandual wrote:
> >> On 12/08/2017 09:45 PM, Michal Hocko wrote:
[...]
> >>> @@ -1593,79 +1556,80 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> >>> const int __user *nodes,
> >>> int __user *status, int flags)
> >>> {
> >>> - struct page_to_node *pm;
> >>> - unsigned long chunk_nr_pages;
> >>> - unsigned long chunk_start;
> >>> - int err;
> >>> -
> >>> - err = -ENOMEM;
> >>> - pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
> >>> - if (!pm)
> >>> - goto out;
> >>> + int chunk_node = NUMA_NO_NODE;
> >>> + LIST_HEAD(pagelist);
> >>> + int chunk_start, i;
> >>> + int err = 0, err1;
> >>
> >> err init might not be required, its getting assigned to -EFAULT right away.
> >
> > No, nr_pages might be 0 AFAICS.
>
> Right but there is another err = 0 after the for loop.
No we have
out_flush:
/* Make sure we do not overwrite the existing error */
err1 = do_move_pages_to_node(mm, &pagelist, current_node);
if (!err1)
err1 = store_status(status, start, current_node, i - start);
if (!err)
err = err1;
This is obviously not an act of beauty and probably a subject to a
cleanup but I just wanted this thing to be working first. Further
cleanups can go on top.
> > [...]
> >>> + if (chunk_node == NUMA_NO_NODE) {
> >>> + chunk_node = node;
> >>> + chunk_start = i;
> >>> + } else if (node != chunk_node) {
> >>> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> >>> + if (err)
> >>> + goto out;
> >>> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> >>> + if (err)
> >>> + goto out;
> >>> + chunk_start = i;
> >>> + chunk_node = node;
> >>> }
>
> [...]
>
> >>> + err = do_move_pages_to_node(mm, &pagelist, chunk_node);
> >>> + if (err)
> >>> + goto out;
> >>> + if (i > chunk_start) {
> >>> + err = store_status(status, chunk_start, chunk_node, i - chunk_start);
> >>> + if (err)
> >>> + goto out;
> >>> + }
> >>> + chunk_node = NUMA_NO_NODE;
> >>
> >> This block of code is bit confusing.
> >
> > I believe this is easier to grasp when looking at the resulting code.
> >>
> >> 1) Why attempt to migrate when just one page could not be isolated ?
> >> 2) 'i' is always greater than chunk_start except the starting page
> >> 3) Why reset chunk_node as NUMA_NO_NODE ?
> >
> > This is all about flushing the pending state on an error and
> > distinguising a fresh batch.
>
> Okay. Will test it out on a multi node system once I get hold of one.
Thanks. I have been testing this specific code path with the following
simple test program and numactl -m0. The code is rather crude so I've
always modified it manually to test different scenarios (this one keeps
every 1k page on the node node to test batching.
---
#include <sys/mman.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <stdio.h>
#include <errno.h>
#include <numaif.h>
int main()
{
unsigned long nr_pages = 10000;
size_t length = nr_pages << 12, i;
unsigned char *addr = mmap(NULL, length, PROT_READ | PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
void *addrs[nr_pages];
int nodes[nr_pages];
int status[nr_pages];
char cmd[128];
char ch;
if (addr == MAP_FAILED)
return 1;
madvise(addr, length, MADV_NOHUGEPAGE);
for (i = 0; i < length; i += 4096)
addr[i] = 1;
for (i = 0; i < nr_pages; i++)
{
addrs[i] = &addr[i * 4096];
if (i%1024)
nodes[i] = 1;
else
nodes[i] = 0;
status[i] = 0;
}
snprintf(cmd, sizeof(cmd)-1, "grep %lx /proc/%d/numa_maps", addr, getpid());
system(cmd);
snprintf(cmd, sizeof(cmd)-1, "grep %lx -A20 /proc/%d/smaps", addr, getpid());
system(cmd);
read(0, &ch, 1);
if (move_pages(0, nr_pages, addrs, nodes, status, MPOL_MF_MOVE)) {
printf("move_pages: err:%d\n", errno);
}
snprintf(cmd, sizeof(cmd)-1, "grep %lx /proc/%d/numa_maps", addr, getpid());
system(cmd);
snprintf(cmd, sizeof(cmd)-1, "grep %lx -A20 /proc/%d/smaps", addr, getpid());
system(cmd);
return 0;
}
---
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 61+ messages in thread