Re: [RFC PATCH v2 09/16] mm,hwpoison: Unify THP handling for hard and soft offline

[Naoya Horiguchi]

On Thu, Oct 17, 2019 at 04:21:16PM +0200, Oscar Salvador wrote:
> Place the THP's page handling in a helper and use it
> from both hard and soft-offline machinery, so we get rid
> of some duplicated code.
> 
> Signed-off-by: Oscar Salvador <osalvador@xxxxxxx>
> ---
>  mm/memory-failure.c | 48 ++++++++++++++++++++++--------------------------
>  1 file changed, 22 insertions(+), 26 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 836d671fb74f..37b230b8cfe7 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1066,6 +1066,25 @@ static int identify_page_state(unsigned long pfn, struct page *p,
>  	return page_action(ps, p, pfn);
>  }
>  
> +static int try_to_split_thp_page(struct page *page, const char *msg)
> +{
> +	lock_page(page);
> +	if (!PageAnon(page) || unlikely(split_huge_page(page))) {
> +		unsigned long pfn = page_to_pfn(page);
> +
> +		unlock_page(page);
> +		if (!PageAnon(page))
> +			pr_info("%s: %#lx: non anonymous thp\n", msg, pfn);
> +		else
> +			pr_info("%s: %#lx: thp split failed\n", msg, pfn);
> +		put_page(page);
> +		return -EBUSY;
> +	}
> +	unlock_page(page);
> +
> +	return 0;
> +}
> +
>  static int memory_failure_hugetlb(unsigned long pfn, int flags)
>  {
>  	struct page *p = pfn_to_page(pfn);
> @@ -1288,21 +1307,8 @@ int memory_failure(unsigned long pfn, int flags)
>  	}
>  
>  	if (PageTransHuge(hpage)) {
> -		lock_page(p);
> -		if (!PageAnon(p) || unlikely(split_huge_page(p))) {
> -			unlock_page(p);
> -			if (!PageAnon(p))
> -				pr_err("Memory failure: %#lx: non anonymous thp\n",
> -					pfn);
> -			else
> -				pr_err("Memory failure: %#lx: thp split failed\n",
> -					pfn);
> -			if (TestClearPageHWPoison(p))
> -				num_poisoned_pages_dec();
> -			put_page(p);
> +		if (try_to_split_thp_page(p, "Memory Failure") < 0)
>  			return -EBUSY;

Although this is not a cleanup thing, this failure path means that
hwpoison is handled (PG_hwpoison is marked), so action_result() should
be called.  I'll add a patch for this later.

Anyway, this cleanup patch looks fine to me.

Acked-by: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>

> -		}
> -		unlock_page(p);
>  		VM_BUG_ON_PAGE(!page_count(p), p);
>  		hpage = compound_head(p);
>  	}
> @@ -1801,19 +1807,9 @@ static int soft_offline_in_use_page(struct page *page)
>  	int mt;
>  	struct page *hpage = compound_head(page);
>  
> -	if (!PageHuge(page) && PageTransHuge(hpage)) {
> -		lock_page(page);
> -		if (!PageAnon(page) || unlikely(split_huge_page(page))) {
> -			unlock_page(page);
> -			if (!PageAnon(page))
> -				pr_info("soft offline: %#lx: non anonymous thp\n", page_to_pfn(page));
> -			else
> -				pr_info("soft offline: %#lx: thp split failed\n", page_to_pfn(page));
> -			put_page(page);
> +	if (!PageHuge(page) && PageTransHuge(hpage))
> +		if (try_to_split_thp_page(page, "soft offline") < 0)
>  			return -EBUSY;
> -		}
> -		unlock_page(page);
> -	}
>  
>  	/*
>  	 * Setting MIGRATE_ISOLATE here ensures that the page will be linked
> -- 
> 2.12.3
> 
>