Re: [RFC PATCH 3/4] mm: soft-offline: retry to split and soft-offline the raw error if the original THP offlining fails.

[Naoya Horiguchi]

On Mon, Aug 14, 2017 at 09:52:15PM -0400, Zi Yan wrote:
> From: Zi Yan <zi.yan@xxxxxxxxxxxxxx>
> 
> For THP soft-offline support, we first try to migrate a THP without
> splitting. If the migration fails, we split the THP and migrate the
> raw error page.
> 
> migrate_pages() does not split a THP if the migration reason is
> MR_MEMORY_FAILURE.
> 
> Signed-off-by: Zi Yan <zi.yan@xxxxxxxxxxxxxx>
> ---
>  mm/memory-failure.c | 77 +++++++++++++++++++++++++++++++++++++----------------
>  mm/migrate.c        | 16 +++++++++++
>  2 files changed, 70 insertions(+), 23 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 8a9ac6f9e1b0..c05107548d72 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c

...

> @@ -1657,23 +1658,53 @@ static int __soft_offline_page(struct page *page, int flags)
>  		 * cannot have PAGE_MAPPING_MOVABLE.
>  		 */
>  		if (!__PageMovable(page))
> -			inc_node_page_state(page, NR_ISOLATED_ANON +
> -						page_is_file_cache(page));
> -		list_add(&page->lru, &pagelist);
> +			mod_node_page_state(page_pgdat(hpage), NR_ISOLATED_ANON +
> +					page_is_file_cache(hpage), hpage_nr_pages(hpage));
> +retry_subpage:
> +		list_add(&hpage->lru, &pagelist);
>  		ret = migrate_pages(&pagelist, new_page, NULL, MPOL_MF_MOVE_ALL,
>  					MIGRATE_SYNC, MR_MEMORY_FAILURE);
>  		if (ret) {
> -			if (!list_empty(&pagelist))
> -				putback_movable_pages(&pagelist);
> -
> +			if (!list_empty(&pagelist)) {
> +				if (!PageTransHuge(hpage))
> +					putback_movable_pages(&pagelist);
> +				else {
> +					lock_page(hpage);
> +					if (split_huge_page_to_list(hpage, &pagelist)) {
> +						unlock_page(hpage);
> +						goto failed;

Don't you need to call putback_movable_pages() before returning?

> +					}
> +					unlock_page(hpage);
> +
> +					if (split)
> +						*split = 1;
> +					/*
> +					 * Pull the raw error page out and put back other subpages.
> +					 * Then retry the raw error page.
> +					 */
> +					list_del(&page->lru);
> +					putback_movable_pages(&pagelist);

If putback_movable_pages() is not called for the raw error page,
NR_ISOLATED_ANON or NR_ISOLATED_FILE stat remains incremented by 1 after return?

> +					hpage = page;
> +					goto retry_subpage;
> +				}
> +			}
> +failed:
>  			pr_info("soft offline: %#lx: migration failed %d, type %lx (%pGp)\n",
> -				pfn, ret, page->flags, &page->flags);
> +				pfn, ret, hpage->flags, &hpage->flags);
>  			if (ret > 0)
>  				ret = -EIO;
>  		}
> +		/*
> +		 * Set PageHWPoison on the raw error page.
> +		 *
> +		 * If the page is a THP, PageHWPoison is set then cleared
> +		 * in its head page in migrate_pages(). So we need to set the raw error
> +		 * page here. Otherwise, setting PageHWPoison again is fine.
> +		 */
> +		SetPageHWPoison(page);

When we failed to migrate the page, we don't have to set PageHWPoison
because we can still continue to use the page (which tends to cause corrected
error but is still usable) and we have a chance to retry soft-offline later.
So please set the flag only when page/thp migration succeeds.

...

> diff --git a/mm/migrate.c b/mm/migrate.c
> index f7b69282d216..b44df9cf72fd 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1118,6 +1118,15 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>  	}
>  
>  	if (unlikely(PageTransHuge(page) && !PageTransHuge(newpage))) {
> +		/*
> +		 * soft-offline wants to retry the raw error subpage, if the THP
> +		 * migration fails. So we do not split the THP here and exit directly.
> +		 */
> +		if (reason == MR_MEMORY_FAILURE) {
> +			rc = -ENOMEM;
> +			goto put_new;
> +		}
> +

This might imply that existing thp migration code have a room for improvment.

>  		lock_page(page);
>  		rc = split_huge_page(page);
>  		unlock_page(page);

This code maybe doesn't work. Even when split_huge_page() succeeds (rc = 0),
__unmap_and_move() migrates only the head page and all tail pages are ignored
(note that split_huge_page_to_list() sends tail pages to LRU list when
parameter 'list' is NULL.)  IOW, the retry logic in migrate_pages() doesn't
work for thp migration.

What I think is OK is that
  - when thp migration from soft offline fails,
    1. split the thp
    2. send only raw error page to migration page list and send the other
       healthy subpages to LRU list
    3. retry loop in migrate_pages()

  - when thp migration from other callers fails,
    1. split the thp
    2. send all subpages to migration page list
    3. retry loop in migrate_pages()


> @@ -1164,6 +1173,13 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>  			 */
>  			if (!test_set_page_hwpoison(page))
>  				num_poisoned_pages_inc();
> +
> +			/*
> +			 * Clear PageHWPoison in the head page. The caller
> +			 * is responsible for setting the raw error page.
> +			 */
> +			if (PageTransHuge(page))
> +				ClearPageHWPoison(page);

I think that you can write more simply with passing the pointer to
the raw error page from caller. It is also helpful for above-mentioned
split-retry issue.
unmap_and_move() has a parameter 'private' which is now only used
for get/put_new_page(), so I think we can extend it for our purpose.

Thanks,
Naoya Horiguchi