[PATCH v2 7/7] mm/migrate: remove result argument on page allocation function for migration

[Joonsoo Kim]

Now, to allocate the page for migration, we implement their own functions
and pass it to migrate_pages(). These functions have 3 arguments, pointer
to original page, private data and result argument. Although there are
many allocation functions for migration, one of them, new_page_node() only
uses result argument. It uses result argument in following steps.

1. pass the pointer to allocation function to get an address for storing
information.
2. if we get an address and migration succeed, save node id of new page.
3. if we fail, save error number into it.

But, we don't need to store these information as it does.

First, we don't use error number in fail case. Call-path related to
new_page_node() is shown in the following.

do_move_page_to_node_array() -> migrate_pages() -> unmap_and_move()
-> new_page_node()

If unmap_and_move() failed, migrate_pages() also returns err, and then
do_move_page_to_node_array() skips to set page's status to user buffer.
So we don't need to set error number to each pages on failure case.

Next, we don't need to set node id of the new page in unmap_and_move(),
since it cannot be different with pm->node. In new_page_node(), we always
try to allocate the page in exact node by referencing pm->node. So it is
sufficient to set node id of the new page in new_page_node(), instead of
unmap_and_move().

These two changes make result argument useless, so we can remove it
entirely. It makes the code more undertandable.

Signed-off-by: Joonsoo Kim <iamjoon...@lge.com>

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 4308018..2755530 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -5,7 +5,7 @@
#include <linux/mempolicy.h>
#include <linux/migrate_mode.h>

-typedef struct page *new_page_t(struct page *, unsigned long private, int **);
+typedef struct page *new_page_t(struct page *, unsigned long private);

/*
* Return values from addresss_space_operations.migratepage():
diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
index 3fff8e7..2014211 100644
--- a/include/linux/page-isolation.h
+++ b/include/linux/page-isolation.h
@@ -62,7 +62,6 @@ int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn,
*/
int set_migratetype_isolate(struct page *page, bool skip_hwpoisoned_pages);
void unset_migratetype_isolate(struct page *page, unsigned migratetype);
-struct page *alloc_migrate_target(struct page *page, unsigned long private,
- int **resultp);
+struct page *alloc_migrate_target(struct page *page, unsigned long private);

#endif
diff --git a/mm/compaction.c b/mm/compaction.c
index f58bcd0..d340c9e 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -747,8 +747,7 @@ static void isolate_freepages(struct zone *zone,
* from the isolated freelists in the block we are migrating to.
*/
static struct page *compaction_alloc(struct page *migratepage,
- unsigned long data,
- int **result)
+ unsigned long data)
{
struct compact_control *cc = (struct compact_control *)data;
struct page *freepage;
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 1debdea..f1a4665 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1399,7 +1399,7 @@ int unpoison_memory(unsigned long pfn)
}
EXPORT_SYMBOL(unpoison_memory);

-static struct page *new_page(struct page *p, unsigned long private, int **x)
+static struct page *new_page(struct page *p, unsigned long private)
{
int nid = page_to_nid(p);
if (PageHuge(p))
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 6d04d37..978785e 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1027,7 +1027,7 @@ 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)
+static struct page *new_node_page(struct page *page, unsigned long node)
{
if (PageHuge(page))
return alloc_huge_page_node(page_hstate(compound_head(page)),
@@ -1186,7 +1186,7 @@ out:
* list of pages handed to migrate_pages()--which is how we get here--
* is in virtual address order.
*/
-static struct page *new_vma_page(struct page *page, unsigned long private, int **x)
+static struct page *new_vma_page(struct page *page, unsigned long private)
{
struct vm_area_struct *vma = (struct vm_area_struct *)private;
unsigned long uninitialized_var(address);
@@ -1220,7 +1220,7 @@ int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
return -ENOSYS;
}

-static struct page *new_vma_page(struct page *page, unsigned long private, int **x)
+static struct page *new_vma_page(struct page *page, unsigned long private)
{
return NULL;
}
diff --git a/mm/migrate.c b/mm/migrate.c
index b595f89..df8fa56 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -908,8 +908,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
struct page *page, int force, enum migrate_mode mode)
{
int rc = 0;
- int *result = NULL;
- struct page *newpage = get_new_page(page, private, &result);
+ struct page *newpage = get_new_page(page, private);

if (!newpage)
return -ENOMEM;
@@ -954,12 +953,6 @@ out:
* then this will free the page.
*/
putback_lru_page(newpage);
- if (result) {
- if (rc)
- *result = rc;
- else
- *result = page_to_nid(newpage);
- }
return rc;
}

@@ -986,7 +979,6 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
int force, enum migrate_mode mode)
{
int rc = 0;
- int *result = NULL;
struct page *new_hpage;
struct anon_vma *anon_vma = NULL;

@@ -1002,7 +994,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
return -ENOSYS;
}

- new_hpage = get_new_page(hpage, private, &result);
+ new_hpage = get_new_page(hpage, private);
if (!new_hpage)
return -ENOMEM;

@@ -1036,12 +1028,6 @@ out:
if (rc != -EAGAIN)
putback_active_hugepage(hpage);
put_page(new_hpage);
- if (result) {
- if (rc)
- *result = rc;
- else
- *result = page_to_nid(new_hpage);
- }
return rc;
}

@@ -1138,8 +1124,7 @@ struct page_to_node {
int status;
};

-static struct page *new_page_node(struct page *p, unsigned long private,
- int **result)
+static struct page *new_page_node(struct page *p, unsigned long private)
{
struct page_to_node *pm = (struct page_to_node *)private;

@@ -1149,7 +1134,7 @@ static struct page *new_page_node(struct page *p, unsigned long private,
if (pm->node == MAX_NUMNODES)
return NULL;

- *result = &pm->status;
+ pm->status = pm->node;

if (PageHuge(p))
return alloc_huge_page_node(page_hstate(compound_head(p)),
@@ -1535,8 +1520,7 @@ static bool migrate_balanced_pgdat(struct pglist_data *pgdat,
}

static struct page *alloc_misplaced_dst_page(struct page *page,
- unsigned long data,
- int **result)
+ unsigned long data)
{
int nid = (int) data;
struct page *newpage;
diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index d1473b2..80efb1c 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -248,8 +248,7 @@ int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn,
return ret ? 0 : -EBUSY;
}

-struct page *alloc_migrate_target(struct page *page, unsigned long private,
- int **resultp)
+struct page *alloc_migrate_target(struct page *page, unsigned long private)
{
gfp_t gfp_mask = GFP_USER | __GFP_MOVABLE;

--
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Joonsoo Kim]

fail_migrate_page() isn't used anywhere, so remove it.

Signed-off-by: Joonsoo Kim <iamjoon...@lge.com>

diff --git a/include/linux/migrate.h b/include/linux/migrate.h

index e4671f9..4308018 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -41,9 +41,6 @@ extern int migrate_page(struct address_space *,
extern int migrate_pages(struct list_head *l, new_page_t x,
unsigned long private, enum migrate_mode mode, int reason);

-extern int fail_migrate_page(struct address_space *,
- struct page *, struct page *);
-
extern int migrate_prep(void);
extern int migrate_prep_local(void);
extern int migrate_vmas(struct mm_struct *mm,
@@ -83,7 +80,6 @@ static inline int migrate_huge_page_move_mapping(struct address_space *mapping,

/* Possible settings for the migrate_page() method in address_operations */
#define migrate_page NULL
-#define fail_migrate_page NULL

#endif /* CONFIG_MIGRATION */

diff --git a/mm/migrate.c b/mm/migrate.c
index cdafdc0..b595f89 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -550,14 +550,6 @@ void migrate_page_copy(struct page *newpage, struct page *page)
* Migration functions
***********************************************************/

-/* Always fail migration. Used for mappings that are not movable */
-int fail_migrate_page(struct address_space *mapping,
- struct page *newpage, struct page *page)
-{
- return -EIO;
-}
-EXPORT_SYMBOL(fail_migrate_page);
-
/*
* Common logic to directly migrate a single page suitable for
* pages that do not use PagePrivate/PagePrivate2.

[Joonsoo Kim]

update_pageblock_skip() only fits to compaction which tries to isolate by
pageblock unit. If isolate_migratepages_range() is called by CMA, it try to
isolate regardless of pageblock unit and it don't reference
get_pageblock_skip() by ignore_skip_hint. We should also respect it on
update_pageblock_skip() to prevent from setting the wrong information.

Cc: <sta...@vger.kernel.org> # 3.7+
Acked-by: Vlastimil Babka <vba...@suse.cz>
Reviewed-by: Naoya Horiguchi <n-hor...@ah.jp.nec.com>
Signed-off-by: Joonsoo Kim <iamjoon...@lge.com>

diff --git a/mm/compaction.c b/mm/compaction.c
index 805165b..f58bcd0 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -134,6 +134,10 @@ static void update_pageblock_skip(struct compact_control *cc,
bool migrate_scanner)
{
struct zone *zone = cc->zone;
+
+ if (cc->ignore_skip_hint)
+ return;
+
if (!page)
return;

[Joonsoo Kim]

From: Naoya Horiguchi <n-hor...@ah.jp.nec.com>

Let's add a comment about where the failed page goes to, which makes
code more readable.

Acked-by: Christoph Lameter <c...@linux.com>
Signed-off-by: Naoya Horiguchi <n-hor...@ah.jp.nec.com>
Signed-off-by: Joonsoo Kim <iamjoon...@lge.com>

diff --git a/mm/migrate.c b/mm/migrate.c
index 3747fcd..c6ac87a 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1123,7 +1123,12 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
nr_succeeded++;
break;
default:
- /* Permanent failure */
+ /*
+ * Permanent failure (-EBUSY, -ENOSYS, etc.):
+ * unlike -EAGAIN case, the failed page is
+ * removed from migration page list and not
+ * retried in the next outer loop.
+ */
nr_failed++;
break;

[Joonsoo Kim]

Here is the patchset for correcting and cleaning-up migration
related stuff. These are random correction and clean-up, so
please see each patches ;)

Thanks.

Naoya Horiguchi (1):
mm/migrate: add comment about permanent failure path

Joonsoo Kim (6):
mm/migrate: correct failure handling if !hugepage_migration_support()
mm/mempolicy: correct putback method for isolate pages if failed
mm/migrate: remove putback_lru_pages, fix comment on
putback_movable_pages
mm/compaction: respect ignore_skip_hint in update_pageblock_skip
mm/migrate: remove unused function, fail_migrate_page()
mm/migrate: remove result argument on page allocation function for
migration

include/linux/migrate.h | 8 +----
include/linux/page-isolation.h | 3 +-
mm/compaction.c | 7 ++--
mm/memory-failure.c | 10 ++++--
mm/mempolicy.c | 8 ++---
mm/migrate.c | 74 +++++++++++++---------------------------
mm/page_isolation.c | 3 +-
7 files changed, 44 insertions(+), 69 deletions(-)

[Joonsoo Kim]

We should remove the page from the list if we fail without ENOSYS,
since migrate_pages() consider error cases except -ENOMEM and -EAGAIN
as permanent failure and it assumes that the page would be removed from
the list. Without this patch, we could overcount number of failure.

In addition, we should put back the new hugepage if
!hugepage_migration_support(). If not, we would leak hugepage memory.

Signed-off-by: Joonsoo Kim <iamjoon...@lge.com>

diff --git a/mm/migrate.c b/mm/migrate.c

index c6ac87a..b1cfd01 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1011,7 +1011,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
{
int rc = 0;
int *result = NULL;
- struct page *new_hpage = get_new_page(hpage, private, &result);
+ struct page *new_hpage;

struct anon_vma *anon_vma = NULL;

/*
@@ -1021,9 +1021,12 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
* tables or check whether the hugepage is pmd-based or not before
* kicking migration.
*/
- if (!hugepage_migration_support(page_hstate(hpage)))
+ if (!hugepage_migration_support(page_hstate(hpage))) {
+ putback_active_hugepage(hpage);
return -ENOSYS;
+ }

+ new_hpage = get_new_page(hpage, private, &result);
if (!new_hpage)
return -ENOMEM;

[Joonsoo Kim]

Some part of putback_lru_pages() and putback_movable_pages() is
duplicated, so it could confuse us what we should use.
We can remove putback_lru_pages() since it is not really needed now.
This makes us undestand and maintain the code more easily.

And comment on putback_movable_pages() is stale now, so fix it.

Signed-off-by: Joonsoo Kim <iamjoon...@lge.com>

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index f5096b5..e4671f9 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -35,7 +35,6 @@ enum migrate_reason {

#ifdef CONFIG_MIGRATION

-extern void putback_lru_pages(struct list_head *l);
extern void putback_movable_pages(struct list_head *l);

extern int migrate_page(struct address_space *,

struct page *, struct page *, enum migrate_mode);
@@ -58,7 +57,6 @@ extern int migrate_page_move_mapping(struct address_space *mapping,
struct buffer_head *head, enum migrate_mode mode);
#else

-static inline void putback_lru_pages(struct list_head *l) {}
static inline void putback_movable_pages(struct list_head *l) {}
static inline int migrate_pages(struct list_head *l, new_page_t x,

unsigned long private, enum migrate_mode mode, int reason)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index b7c1716..1debdea 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1569,7 +1569,13 @@ static int __soft_offline_page(struct page *page, int flags)
ret = migrate_pages(&pagelist, new_page, MPOL_MF_MOVE_ALL,
MIGRATE_SYNC, MR_MEMORY_FAILURE);
if (ret) {
- putback_lru_pages(&pagelist);
+ if (!list_empty(&pagelist)) {
+ list_del(&page->lru);
+ dec_zone_page_state(page, NR_ISOLATED_ANON +
+ page_is_file_cache(page));
+ putback_lru_page(page);
+ }
+
pr_info("soft offline: %#lx: migration failed %d, type %lx\n",
pfn, ret, page->flags);
if (ret > 0)
diff --git a/mm/migrate.c b/mm/migrate.c
index b1cfd01..cdafdc0 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -71,28 +71,12 @@ int migrate_prep_local(void)
}

/*
- * Add isolated pages on the list back to the LRU under page lock
- * to avoid leaking evictable pages back onto unevictable list.
- */
-void putback_lru_pages(struct list_head *l)
-{
- struct page *page;
- struct page *page2;
-
- list_for_each_entry_safe(page, page2, l, lru) {
- list_del(&page->lru);
- dec_zone_page_state(page, NR_ISOLATED_ANON +
- page_is_file_cache(page));
- putback_lru_page(page);
- }
-}
-
-/*
* Put previously isolated pages back onto the appropriate lists
* from where they were once taken off for compaction/migration.
*
- * This function shall be used instead of putback_lru_pages(),
- * whenever the isolated pageset has been built by isolate_migratepages_range()
+ * This function shall be used whenever the isolated pageset has been
+ * built from lru, balloon, hugetlbfs page. See isolate_migratepages_range()
+ * and isolate_huge_page().
*/
void putback_movable_pages(struct list_head *l)
{
@@ -1704,6 +1688,12 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page,
node, MIGRATE_ASYNC, MR_NUMA_MISPLACED);
if (nr_remaining) {
+ if (!list_empty(&migratepages)) {
+ list_del(&page->lru);
+ dec_zone_page_state(page, NR_ISOLATED_ANON +
+ page_is_file_cache(page));
+ putback_lru_page(page);
+ }
putback_lru_pages(&migratepages);
isolated = 0;
} else

[Joonsoo Kim]

queue_pages_range() isolates hugetlbfs pages and putback_lru_pages() can't
handle these. We should change it to putback_movable_pages().

Naoya said that it is worth going into stable, because it can break
in-use hugepage list.

Cc: <sta...@vger.kernel.org> # 3.12
Reviewed-by: Naoya Horiguchi <n-hor...@ah.jp.nec.com>
Signed-off-by: Joonsoo Kim <iamjoon...@lge.com>

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index eca4a31..6d04d37 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1318,7 +1318,7 @@ static long do_mbind(unsigned long start, unsigned long len,
if (nr_failed && (flags & MPOL_MF_STRICT))
err = -EIO;
} else
- putback_lru_pages(&pagelist);
+ putback_movable_pages(&pagelist);

up_write(&mm->mmap_sem);
mpol_out:

[Christoph Lameter]

On Mon, 9 Dec 2013, Joonsoo Kim wrote:

> fail_migrate_page() isn't used anywhere, so remove it.

Acked-by: Christoph Lameter <c...@linux.com>

[Christoph Lameter]

On Mon, 9 Dec 2013, Joonsoo Kim wrote:

> We should remove the page from the list if we fail without ENOSYS,
> since migrate_pages() consider error cases except -ENOMEM and -EAGAIN
> as permanent failure and it assumes that the page would be removed from
> the list. Without this patch, we could overcount number of failure.

Ok what does the patch do about this? I dont see any modifications. Remove
this part of the description?

> In addition, we should put back the new hugepage if
> !hugepage_migration_support(). If not, we would leak hugepage memory.

Ok looks like that is fixed by this patch.

Acked-by: Christoph Lameter <c...@linux.com>

[Rafael Aquini]

On Mon, Dec 09, 2013 at 06:10:44PM +0900, Joonsoo Kim wrote:
> queue_pages_range() isolates hugetlbfs pages and putback_lru_pages() can't
> handle these. We should change it to putback_movable_pages().
>
> Naoya said that it is worth going into stable, because it can break
> in-use hugepage list.
>
> Cc: <sta...@vger.kernel.org> # 3.12
> Reviewed-by: Naoya Horiguchi <n-hor...@ah.jp.nec.com>
> Signed-off-by: Joonsoo Kim <iamjoon...@lge.com>
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index eca4a31..6d04d37 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1318,7 +1318,7 @@ static long do_mbind(unsigned long start, unsigned long len,
> if (nr_failed && (flags & MPOL_MF_STRICT))
> err = -EIO;
> } else
> - putback_lru_pages(&pagelist);
> + putback_movable_pages(&pagelist);
>
> up_write(&mm->mmap_sem);
> mpol_out:

Acked-by: Rafael Aquini <aqu...@redhat.com>

[Christoph Lameter]

On Mon, 9 Dec 2013, Joonsoo Kim wrote:

> First, we don't use error number in fail case. Call-path related to
> new_page_node() is shown in the following.
>
> do_move_page_to_node_array() -> migrate_pages() -> unmap_and_move()
> -> new_page_node()
>
> If unmap_and_move() failed, migrate_pages() also returns err, and then
> do_move_page_to_node_array() skips to set page's status to user buffer.
> So we don't need to set error number to each pages on failure case.

I dont get this. new_page_node() sets the error condition in the
page_to_node array before this patch. There is no post processing in
do_move_page_to_node_array(). The function simply returns and relies on
new_page_node() to have set the page status. do_move_pages() then returns
the page status back to userspace. How does the change preserve these
diagnostics?

> Next, we don't need to set node id of the new page in unmap_and_move(),
> since it cannot be different with pm->node. In new_page_node(), we always
> try to allocate the page in exact node by referencing pm->node. So it is
> sufficient to set node id of the new page in new_page_node(), instead of
> unmap_and_move().

Thats a good thought.

[Naoya Horiguchi]

On Mon, Dec 09, 2013 at 06:10:47PM +0900, Joonsoo Kim wrote:
> fail_migrate_page() isn't used anywhere, so remove it.
>
> Signed-off-by: Joonsoo Kim <iamjoon...@lge.com>

Reviewed-by: Naoya Horiguchi <n-hor...@ah.jp.nec.com>

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

[Joonsoo Kim]

On Mon, Dec 09, 2013 at 04:17:32PM +0000, Christoph Lameter wrote:
> On Mon, 9 Dec 2013, Joonsoo Kim wrote:
>
> > We should remove the page from the list if we fail without ENOSYS,
> > since migrate_pages() consider error cases except -ENOMEM and -EAGAIN
> > as permanent failure and it assumes that the page would be removed from
> > the list. Without this patch, we could overcount number of failure.
>
> Ok what does the patch do about this? I dont see any modifications. Remove
> this part of the description?

Description is slightly wrong.
Following is correct one.

"We should remove the page from the list if we fail *with* ENOSYS,"

And this patch do this by adding putback_active_hugepage(hpage)
on ENOSYS case.

>
> > In addition, we should put back the new hugepage if
> > !hugepage_migration_support(). If not, we would leak hugepage memory.
>
> Ok looks like that is fixed by this patch.
>
> Acked-by: Christoph Lameter <c...@linux.com>

Thanks.

[Joonsoo Kim]

> >@@ -1704,6 +1688,12 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
> > nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page,
> > node, MIGRATE_ASYNC, MR_NUMA_MISPLACED);
> > if (nr_remaining) {
> >+ if (!list_empty(&migratepages)) {
> >+ list_del(&page->lru);
> >+ dec_zone_page_state(page, NR_ISOLATED_ANON +
> >+ page_is_file_cache(page));
> >+ putback_lru_page(page);
> >+ }
> > putback_lru_pages(&migratepages);
>

> You should remove this line. Otherwise,

Yes, you are right. I will send next version. T_T

>
> Reviewed-by: Wanpeng Li <liw...@linux.vnet.ibm.com>

Thanks.

[Joonsoo Kim]

On Tue, Dec 10, 2013 at 10:17:56AM +0800, Wanpeng Li wrote:
> Hi Joonsoo,

> The memory hotplug(do_migrate_range) and hwpoison(soft_offline_huge_page) callsets both
> will call putback_movable_pages/putback_active_hugepage for -ENOSYS case.

Hello Wanpeng.

Yes, those callsite handle error case, but error case handling should be done
in unmap_and_move_huge_page(). It is mentioned on patch 1. If we defer to
remove the pages from the list, nr_failed can be overcounted.

Thanks.

[Joonsoo Kim]

On Tue, Dec 10, 2013 at 05:51:47PM +0900, Joonsoo Kim wrote:
> > >@@ -1704,6 +1688,12 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
> > > nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page,
> > > node, MIGRATE_ASYNC, MR_NUMA_MISPLACED);
> > > if (nr_remaining) {
> > >+ if (!list_empty(&migratepages)) {
> > >+ list_del(&page->lru);
> > >+ dec_zone_page_state(page, NR_ISOLATED_ANON +
> > >+ page_is_file_cache(page));
> > >+ putback_lru_page(page);
> > >+ }
> > > putback_lru_pages(&migratepages);
> >
> > You should remove this line. Otherwise,
>
> Yes, you are right. I will send next version. T_T

Here is the next version.
Thanks.

-----------8<------------------------
From 5495abbe8ed8a39712d733b08110dcf97e0b53f5 Mon Sep 17 00:00:00 2001
From: Joonsoo Kim <iamjoon...@lge.com>
Date: Fri, 6 Dec 2013 16:58:18 +0900
Subject: [PATCH v2-fix 4/7] mm/migrate: remove putback_lru_pages, fix comment
on putback_movable_pages

Some part of putback_lru_pages() and putback_movable_pages() is
duplicated, so it could confuse us what we should use.
We can remove putback_lru_pages() since it is not really needed now.
This makes us undestand and maintain the code more easily.

And comment on putback_movable_pages() is stale now, so fix it.

Reviewed-by: Wanpeng Li <liw...@linux.vnet.ibm.com>

+ list_del(&page->lru);
+ dec_zone_page_state(page, NR_ISOLATED_ANON +
+ page_is_file_cache(page));
+ putback_lru_page(page);
+ }

+
pr_info("soft offline: %#lx: migration failed %d, type %lx\n",
pfn, ret, page->flags);
if (ret > 0)
diff --git a/mm/migrate.c b/mm/migrate.c

index b1cfd01..fa73ee3 100644

@@ -1704,7 +1688,12 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,

nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page,
node, MIGRATE_ASYNC, MR_NUMA_MISPLACED);
if (nr_remaining) {

- putback_lru_pages(&migratepages);

+ if (!list_empty(&migratepages)) {
+ list_del(&page->lru);
+ dec_zone_page_state(page, NR_ISOLATED_ANON +
+ page_is_file_cache(page));
+ putback_lru_page(page);
+ }

isolated = 0;
} else
count_vm_numa_event(NUMA_PAGE_MIGRATE);
--
1.7.9.5

[Joonsoo Kim]

On Mon, Dec 09, 2013 at 04:40:06PM +0000, Christoph Lameter wrote:
> On Mon, 9 Dec 2013, Joonsoo Kim wrote:
>
> > First, we don't use error number in fail case. Call-path related to
> > new_page_node() is shown in the following.
> >
> > do_move_page_to_node_array() -> migrate_pages() -> unmap_and_move()
> > -> new_page_node()
> >
> > If unmap_and_move() failed, migrate_pages() also returns err, and then
> > do_move_page_to_node_array() skips to set page's status to user buffer.
> > So we don't need to set error number to each pages on failure case.
>
> I dont get this. new_page_node() sets the error condition in the
> page_to_node array before this patch. There is no post processing in
> do_move_page_to_node_array(). The function simply returns and relies on
> new_page_node() to have set the page status. do_move_pages() then returns
> the page status back to userspace. How does the change preserve these
> diagnostics?

Hello, Christoph.

In do_move_pages(), if error occurs, 'goto out_pm' is executed and the
page status doesn't back to userspace. So we don't need to store err number.

Perhaps my description should be changed something like below.

*do_move_pages()* -> do_move_page_to_node_array() -> migrate_pages()

-> unmap_and_move() -> new_page_node()

If unmap_and_move() failed, migrate_pages() also returns err, and then

*do_move_pages()* skips to set page's status to user buffer.

So we don't need to set error number to each pages on failure case.

Is it sufficient explanation?

>
> > Next, we don't need to set node id of the new page in unmap_and_move(),
> > since it cannot be different with pm->node. In new_page_node(), we always
> > try to allocate the page in exact node by referencing pm->node. So it is
> > sufficient to set node id of the new page in new_page_node(), instead of
> > unmap_and_move().
>
> Thats a good thought.

Thanks!

[Christoph Lameter]

On Wed, 11 Dec 2013, Joonsoo Kim wrote:

> In do_move_pages(), if error occurs, 'goto out_pm' is executed and the
> page status doesn't back to userspace. So we don't need to store err number.

If a page cannot be moved then the error code is containing the number of
pages that could not be migrated. The check there is for err < 0.
So a positive number is not an error.

migrate_pages only returns an error code if we are running out of memory.

[Joonsoo Kim]

On Wed, Dec 11, 2013 at 04:00:56PM +0000, Christoph Lameter wrote:
> On Wed, 11 Dec 2013, Joonsoo Kim wrote:
>
> > In do_move_pages(), if error occurs, 'goto out_pm' is executed and the
> > page status doesn't back to userspace. So we don't need to store err number.
>
> If a page cannot be moved then the error code is containing the number of
> pages that could not be migrated. The check there is for err < 0.
> So a positive number is not an error.
>
> migrate_pages only returns an error code if we are running out of memory.

Ah... I missed it. I will drop this patch and send v3 for whole patchset.

Thanks.