Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
To:     Oscar Salvador <osalvador@suse.de>
CC:     "mhocko@kernel.org" <mhocko@kernel.org>,
        "mike.kravetz@oracle.com" <mike.kravetz@oracle.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v2 09/16] mm,hwpoison: Unify THP handling for hard
 and soft offline
Thread-Topic: [RFC PATCH v2 09/16] mm,hwpoison: Unify THP handling for hard
 and soft offline
Thread-Index: AQHVhPZEHfGD3bd9MkGP7p/dXrA8ladkGtAA
Date:   Mon, 21 Oct 2019 07:04:40 +0000
Message-ID: <20191021070439.GC9037@hori.linux.bs1.fc.nec.co.jp>
References: <20191017142123.24245-1-osalvador@suse.de>
 <20191017142123.24245-10-osalvador@suse.de>
In-Reply-To: <20191017142123.24245-10-osalvador@suse.de>
Accept-Language: en-US, ja-JP
Content-Language: ja-JP
Content-Type: text/plain; charset="iso-2022-jp"
Content-ID: <ACAC5400327B2B44B0993E28BA42104F@gisp.nec.co.jp>
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@suse.de>
> ---
>  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@ah.jp.nec.com>

> -		}
> -		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
> 
>